Support reopening audit logs

[brandonpayton]

This PR adds support for reopening log files by:

• Exposing an msc_rules_reopen_audit_log(rules, error) function to trigger audit log reopen for a given rules set
• Adding a reopen(error) method to AuditLog
• Adding a reopen(error) method to the audit log Writer interface
• Adding a reopen(filename, error) method to SharedFiles

⚠️ Currently, I do not believe SharedFiles::reopen() to be thread-safe, and thread-safety must be ensured by the caller. Even if the SharedFiles general lock could be used, we may not want to lock for all SharedFile writes. Please let me know if you'd like to resolve the issue as part of this PR or a later PR.

This is based on work to support ModSecurity audit log rotation within Automattic. It is used in a ModSecurity-nginx patch, and the PR for that is here.

resolves #1968

[brandonpayton]

This is failing cppcheck. Will take a look and fix.

[brandonpayton]

Actually, the warning appears to be unrelated to this patch:

warning: src/utils/shared_files.h,88,performance,useInitializationList,When an object of a class is created, the constructors of all member variables are called consecutively in the order the variables are declared, even if you don't explicitly write them to the initialization list. You could avoid assigning 'm_memKeyStructure' a value by passing the value to the constructor in the initialization list.

[brandonpayton]

Sorry, I was incorrect. The previous warning is the only warning I'm seeing on my dev machine.

There is a relevant warning from TravisCI running make check-static:

warning: src/utils/shared_files.h,60,style,functionConst,The member function 'modsecurity::utils::SharedFiles::reopen' can be made a const function. Making this function 'const' should not cause compiler errors. Even though the function can be made const function technically it may not make sense conceptually. Think about your design and the task of the function first - is it a function that must not change object internal state?

[brandonpayton]

I added the above warning to the cppcheck suppressions list because SharedFiles::reopen() does change SharedFiles internal state.

[brandonpayton]

Regarding thread-safety:

I was reading the nginx source this evening and noticed it redirects stderr using a POSIX function, dup2. dup2 takes an old and a new descriptor and atomically retargets the new to refer to the old. As evidence, both glibc docs and POSIX docs (requires login) state that dup2 should be atomic.

This looks like a good way to address the thread-safety issue with the current SharedFiles::reopen() in this PR.

MS also provides an implementation of dup2 for POSIX compliance, and given that the POSIX docs require atomicity, it seems like this should be thread-safe on Windows as well.

[JokerQyou]

What's the current status of this PR?

[mike-mckenna]

Is this pull request getting attention? The code in this pull request will support the rotation of the audit log in Nginx. More specifically, the code in this pull request is required before ModSecurity-nginx pull request owasp-modsecurity/ModSecurity-nginx#198 can proceed.

[liudongmiao]

There should be a reopen for debug log.

I'd like to use a flush on modsecurity::utils::SharedFiles directly.

[tomsommer]

Any chance of getting this fixed? Sort of annoying not being able to reliably rotate logs

[brandonpayton]

As @liudongmiao mentioned here, this does not reopen the debug log. It would be good to generalize this PR to reopen logs in general.

I don't have time to work on this immediately but hope to get back to it in the next month or so. If anyone else wants to work on this before then, please feel free.

[Orgoth]

As @liudongmiao mentioned here, this does not reopen the debug log. It would be good to generalize this PR to reopen logs in general.

I don't have time to work on this immediately but hope to get back to it in the next month or so. If anyone else wants to work on this before then, please feel free.

Any news on this?

[airween]

Hi @Orgoth,

thanks for bringing this up - I try to take a look at this soon.