Skip to content

Updated Code from PR #2304 #3371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: v3/master
Choose a base branch
from

Conversation

akama-aka
Copy link

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 logrotate can 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

Copy link

sonarqubecloud bot commented May 6, 2025

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +328 to 347
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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, but it seems that file only has 77 lines, so adding line 88 makes no sense.

@airween
Copy link
Member

airween commented May 6, 2025

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:

Implements the ability to reopen audit log files to ensure compatibility with Linux log rotation

Could you try this patch on Windows? I think the result is almost the same, but I'm curios how Windows handles the reopen() method. Do you have any idea how can we add a test case to check this method?

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?

@akama-aka
Copy link
Author

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:

Implements the ability to reopen audit log files to ensure compatibility with Linux log rotation

Could you try this patch on Windows? I think the result is almost the same, but I'm curios how Windows handles the reopen() method. Do you have any idea how can we add a test case to check this method?

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 ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants