-
Notifications
You must be signed in to change notification settings - Fork 296
Support audit log rotation on SIGHUP and SIGUSR1 #198
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
Support audit log rotation on SIGHUP and SIGUSR1 #198
Conversation
It makes sense this one is failing CI because the changes it relies on aren't yet in ModSecurity.
Thank you @brandonpayton!
@defanator can you have a look at it ?
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days
jatgh
commented
Jun 16, 2020
Any updates on this? Still waiting for fix, log file doesn't change after rotation without nginx restart, which is very inconvenient.
@github-actions please re-open this. There are many people waiting for this pull request to be merged.
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days
That bot is annoying.... help @zimmerle
fzipi
commented
Aug 21, 2020
Can we add the no-stale
tag? What is needed to merge this one?
That bot is annoying.... help @zimmerle
Indeed. Part of a bigger plan. We are doing the fining tuning. Sorry for the inconvenience.
Can we add the
no-stale
tag?
Sure.
What is needed to merge this one?
This patch depends on some changes on libModSecurity as stated at owasp-modsecurity/ModSecurity#2304; Without the changes in the linked issues, this is not functional. The review will be started as soon as we got owasp-modsecurity/ModSecurity#2304 merged.
fzipi
commented
Aug 21, 2020
Thanks for the comments @zimmerle ! Now we have a better picture 👍
mike-mckenna
commented
Apr 29, 2022
This pull request depends on a pull request in the ModSecurity repository: owasp-modsecurity/ModSecurity#2304
Is Modsecurity pull request 2304 getting attention?
XanC
commented
Sep 26, 2024
On our systems I've been patching ModSecurity to support rotation, as well as applying this patch to ModSecurity-nginx.
One side effect is that every time the nginx log is rotated, by calling invoke-rc.d nginx rotate (whether manually or from logrotate), my /dev/null ends up being owned by nginx instead of root.
Am I doing something wrong?
Hi @XanC,
One side effect is that every time the nginx log is rotated, by calling invoke-rc.d nginx rotate (whether manually or from logrotate), my /dev/null ends up being owned by nginx instead of root.
Am I doing something wrong?
How looks like your logrotate config?
How looks like your logrotate config?
Thanks for taking a look!
/var/log/www/nginx/*.log {
daily
missingok
rotate 14
compress
compresscmd /usr/bin/xz
compressext .xz
compressoptions -9 -T5
delaycompress
notifempty
create 0640 www-data adm
sharedscripts
prerotate
if [ -d /etc/logrotate.d/httpd-prerotate ]; then \
run-parts /etc/logrotate.d/httpd-prerotate; \
fi \
endscript
postrotate
invoke-rc.d nginx rotate >/dev/null 2>&1
/usr/local/bin/apache-log-filter.pl frontend 1ドル
endscript
}
If I manually run invoke-rc.d nginx rotate
, then the problem happens. So I don't think it has to do with logrotate.
XanC
commented
Oct 16, 2024
Update: I'm working around the problem by adding a file at /etc/systemd/system/logrotate.d/override.conf:
[Service]
PrivateDevices=false
ExecStartPost=chown root /dev/null
It looks like this patch on line 629 of ngx_http_modsecurity_module.c reopens /dev/null. I'm not sure why it's doing that, but that's probably why it ends up being owned by nginx.
It is possible that the code will be renewed. I have applied the changes via patch and am getting the following errors when building Nginx:
/opt/ModSecurity-nginx/src/ngx_http_modsecurity_module.c:800:9: error: implicit declaration of function 'msc_rules_reopen_audit_log' [-Werror=implicit-function-declaration]
800 | if(msc_rules_reopen_audit_log(c->rules_set, &error) < 0) {
Hi @akama-aka,
yes, it seems that this patch needs some extra functions from libmodsecurity3 which aren't implemented yet.
See this pull request under the library repository, probably you have to apply that patch in the engine first.
Uh oh!
There was an error while loading. Please reload this page.
This is a PR that uses owasp-modsecurity/ModSecurity#2304 to support audit log rotation when nginx reloads config or reopens log files.
Thanks to @defanator for providing a proof-of-concept!
I tested this by:
lsof
to observe nginx has the audit log openlsof
to confirm the nginx now references the moved filenginx -s reload
andnginx -s reopen
When I wasn't convinced of the thread-safety of audit log writes and log reopen within nginx, I also attempted to test it with the following script using GNU parallel:
parallel -j+0 ./test-reopen.sh ::: {1..10000}
This test didn't hurt, but due to nginx's primarily single-threaded architecture, I believe log reopen is thread-safe here by default.
resolves #121