Updated Code from PR #2304 #3371
Conversation
This code was written by: @brandonpayton Co-authored-by: @brandonpayton
Signed-off-by: Kasumi <kasumi@kitsune.exposed>
|
| @@ -369,6 +369,12 @@ bool AuditLog::merge(AuditLog *from, std::string *error) { | |||
| return init(error); | |||
| } | |||
|
|
|||
| bool AuditLog::reopen(std::string *error) { | |||
| if (m_writer != NULL) { | |||
There was a problem hiding this comment.
As Sonarcloud shows here you should use nullptr instead of NULL. I see there are many other NULL literals in this file - feel free to replace them.
| extern "C" int msc_rules_reopen_audit_log(RulesSet *rules, const char **error) { | ||
| bool succeeded = true; | ||
| std::string errorStr; | ||
|
|
||
| if (rules->m_auditLog != NULL) { | ||
| succeeded = rules->m_auditLog->reopen(&errorStr); | ||
| } | ||
|
|
||
| if (!succeeded) { | ||
| if (!errorStr.empty()) { | ||
| *error = strdup(errorStr.c_str()); | ||
| } else { | ||
| // Guarantee an error message is always assigned in the event of a failure | ||
| *error = strdup("Unknown error reopening audit log"); | ||
| } | ||
| } | ||
|
|
||
| return succeeded ? 0 : -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
I know this is a completely valid solution, but as you can see in other "C" API functions, the authors tied to avoid all the logic and use C++ types (like here the bool and std::string).
My suggestion here: create a thin wrapper, an "extra" function.
You should add a new function to RulesSet class with name reopen(), move this body there, and in this "C" API you have to call only that function.
| // | ||
| // | ||
|
|
||
| functionConst:src/utils/shared_files.h:60 |
There was a problem hiding this comment.
Generally: please don't use this file to collect suppressions. If you need to add some suppression, please use an inline one.
But here: why it this necessary? It seems in that file in line 60 there is only a condition...
| // | ||
|
|
||
| functionConst:src/utils/shared_files.h:60 | ||
| useInitializationList:src/utils/shared_files.h:88 |
There was a problem hiding this comment.
Similar here, but it seems that file only has 77 lines, so adding line 88 makes no sense.
|
Hi @akama-aka, many thanks for this pull request, and special welcome to your first contribution! I added some questions above, please review them. And one more question - you wrote:
Could you try this patch on Windows? I think the result is almost the same, but I'm curios how Windows handles the Cc: @eduar-hte (regarding to Windows). And finally: I saw you added a "C" API function. I assume that will be used in Nginx to reopen audit.log files, right? Do you mind to add that feature to Nginx connector if we merge this? |
Thank you for your comment to this draft pr. I'll try to learn C/C++ more to understand more of it and try to fix those things up ^^ |
What this PR implements:
Implements the ability to reopen audit log files to ensure compatibility with Linux log rotation (
logrotate).Details:
This pull request introduces the functionality that allows ModSecurity to reopen its audit log files. This is necessary so that Linux-based log rotation tools such as
logrotatecan safely rotate, compress or archive the log files without having to restart ModSecurity.Sources and methodology:
The code in this PR is largely based on the work of @brandonpayton in the previous pull request #2304. The changes made here focus on fixing build bugs that were encountered when integrating the original code into the current ModSecurity release. These adjustments were developed with the help of AI models.
Reason:
Support for log rotation is an important feature for the practical use of ModSecurity, especially in production environments. Since the original pull request has stagnated and this feature is relevant for my own needs and those of other users, I decided to try again with AI support, even though this may not be the preferred approach. My own C/C++ skills are limited (or almost non-existent), but the need for this feature motivated me to take this step.
references
Base PR