Support audit log rotation on SIGHUP and SIGUSR1

[brandonpayton]

This is a PR that uses SpiderLabs/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:

1. Using lsof to observe nginx has the audit log open
2. Moving the audit log file
3. Using lsof to confirm the nginx now references the moved file
4. Signaling the nginx master process with SIGHUP or SIGUSR1.
5. Testing the same things with nginx -s reload and nginx -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}

#!/bin/bash

set -euo pipefail

read s < /dev/urandom
TEST=$(( ${#s} % 11 ))

if [[ TEST -eq 0 ]]; then
    echo "$1 reopen with SIGUSR1"
    kill -USR1 "$(pgrep -P1 nginx)"
elif [[ TEST -eq 1 ]]; then
    echo "$1 reload with SIGHUP"
    kill -HUP "$(pgrep -P1 nginx)"
else
    echo "$1 triggering modsec logging"
    # Matches a silly rule created for testing
    curl --silent http://localhost/index.html?asdf 2>&1 >/dev/null &
fi

pgrep -d", " nginx

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

[brandonpayton]

It makes sense this one is failing CI because the changes it relies on aren't yet in ModSecurity.

[zimmerle]

Thank you @brandonpayton!

[zimmerle]

@defanator can you have a look at it ?

[github-actions[bot]]

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]

Any updates on this? Still waiting for fix, log file doesn't change after rotation without nginx restart, which is very inconvenient.

[nikolas]

@github-actions please re-open this. There are many people waiting for this pull request to be merged.

[github-actions[bot]]

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

[nikolas]

That bot is annoying.... help @zimmerle

[fzipi]

Can we add the no-stale tag? What is needed to merge this one?

[zimmerle]

That bot is annoying.... help @zimmerle

Indeed. Part of a bigger plan. We are doing the fining tuning. Sorry for the inconvenience.

[zimmerle]

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 SpiderLabs/ModSecurity#2304; Without the changes in the linked issues, this is not functional. The review will be started as soon as we got SpiderLabs/ModSecurity#2304 merged.

[fzipi]

Thanks for the comments @zimmerle ! Now we have a better picture :+1:

[mike-mckenna]

This pull request depends on a pull request in the ModSecurity repository: https://github.com/SpiderLabs/ModSecurity/pull/2304 Is Modsecurity pull request 2304 getting attention?

[XanC]

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?

[airween]

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?

[XanC]

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]

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.