-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Move log opening to appropriate execution phase #2823
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
Conversation
Hello @TomasKorbar ,
There is at least one downside here.
Current functionality means that the attempt to open the audit log file occurs at startup. If it fails -- perhaps because the chosen location is unwriteable -- then Apache will fail to start and the user can see a message like:
ModSecurity: Failed to open the audit log file: /var/log/apache/modsec_audit.log
(As a side note, with these changes I could not see a failure message anywhere, including in Apache's error.log. Perhaps I didn't look carefully enough.)
In general, it's preferable to have configuration errors identified at startup whenever possible.
c92bbbc to
ec87dab
Compare
Hi @martinhsv
Log about not accessible file is now present and failure to open log prevents apache from starting.
Let me know if there are any more changes needed.
ec87dab to
85e97e1
Compare
@martinhsv Hi, could we get this merged please? I can provide further help if needed.
85e97e1 to
5698426
Compare
@martinhsv should be good now.
eflanagan0
commented
Apr 22, 2024
@marcstern I've verified this builds correctly on a NixOS system after cherry-picking the commit to v2.7.3. I can also start an Apache/2.4.59 server loading the compiled module.
@marcstern
marcstern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds sensible, but I wonder why it was handled differently than error logs at first. Does anybody see a potential problem with this fix?
Otherwise, I think it's the right moment to de-duplicate the code between dcfg->auditlog_name & dcfg->auditlog_name2 file opening. Can you use a static function that centralizes the common code?
Additional question (linked to code duplication):
Why do we have pipe_name = dcfg->auditlog_name + 1; and pipe_name = ap_server_root_relative(p, dcfg->auditlog2_name + 1);? Is this normal?
eflanagan0
commented
Apr 23, 2024
I realized this PR also will need rebased atop v2/master to incorporate the CI changes and kick off those checks.
When piped logs are opened during parsing of configuration it results in unexpected situations in apache httpd and can cause hang of process which is trying to log into auditlog. Code should work as before, with the exception of one additional condition evaluation when primary audit log is not set and secondary audit log path to piped executable is now not relative to server root.
5698426 to
bec3381
Compare
Quality Gate Passed Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
@marcstern Hi,
I added static function to deduplicate the code. Also the secondary pipe is now not evaluated relatively to server root, as i think that when you want to enter executable path, it is more useful to just specify absolute path, as the executable does not have to be in the same directory as server.
Please tell me about additional changes if necessary.
marcstern
commented
Oct 11, 2024
Using a hook looks indeed cleaner and more consistent with error log. Does anybody see a potential risk with this change?
Using a hook looks indeed cleaner and more consistent with error log. Does anybody see a potential risk with this change?
Let me check this a bit later (probably in next few days).
@airween @marcstern would you by any chance have time to look on this again?
Sorry, I completely forgot this. I just assigned this PR to myself, please let me review this soon.
Really sorry again!
notroj
commented
May 9, 2025
Moving this code to the hook and attaching the logs to plog looks exactly right to me to fix #2822 and it will make this code work like the logging code in httpd itself (e.g. mod_log_config). Is there anything else required here to get this merged?
@TomasKorbar, @notroj: meanwhile I found a few small issues in the part of standalone code and test (regression test). Let me fix them in next few days, and then I can try this with those modifications. If everything will be okay, I'll merge this PR.
A question: this modification probably does not affect the standalone module, but I want to be sure, so please confirm this.
notroj
commented
May 9, 2025
I'm not familiar with the standalone build but it seems to stub out ap_open_piped_log so I guess this patch won't make any difference there.
https://github.com/owasp-modsecurity/ModSecurity/blob/v2/master/standalone/server.c#L618
uhliarik
commented
Aug 6, 2025
Hi, is there any progress on merging this PR? @airween
Hi, is there any progress on merging this PR? @airween
Unfortunately not, I didn't have any time to take this issue.
When piped logs are opened during parsing of configuration it results in unexpected situations in apache httpd and can cause hang of process which is trying to log into auditlog.
Closes #2822