Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
brandonpayton wants to merge 1 commit into owasp-modsecurity:master
base: master
Choose a base branch
Loading
from brandonpayton:add/audit-log-reopen

Conversation

Copy link
Contributor

@brandonpayton brandonpayton commented May 1, 2020
edited
Loading

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:

  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

jatgh, nikolas, tomsommer, and galibozek reacted with thumbs up emoji
Copy link
Contributor Author

brandonpayton commented May 1, 2020
edited
Loading

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

Copy link
Contributor

Thank you @brandonpayton!

brandonpayton reacted with heart emoji

Copy link
Contributor

@defanator can you have a look at it ?

Copy link

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

Copy link

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.

Copy link
Contributor

nikolas commented Jun 22, 2020

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

jatgh and JakubOnderka reacted with thumbs up emoji

Copy link

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

Copy link
Contributor

nikolas commented Jul 23, 2020

That bot is annoying.... help @zimmerle

zimmerle reacted with confused emoji

Copy link

fzipi commented Aug 21, 2020

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

nikolas reacted with thumbs up emoji

Copy link
Contributor

That bot is annoying.... help @zimmerle

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

Copy link
Contributor

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.

jatgh and wouterj reacted with thumbs up emoji

Copy link

fzipi commented Aug 21, 2020

Thanks for the comments @zimmerle ! Now we have a better picture 👍

zimmerle reacted with thumbs up emoji

Copy link

This pull request depends on a pull request in the ModSecurity repository: owasp-modsecurity/ModSecurity#2304
Is Modsecurity pull request 2304 getting attention?

Copy link

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?

Copy link
Member

airween commented Sep 26, 2024

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?

Copy link

XanC commented Sep 26, 2024
edited
Loading

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.

Copy link

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.

Copy link

akama-aka commented Apr 18, 2025
edited by airween
Loading

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) {

Copy link
Member

airween commented Apr 18, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@defanator defanator Awaiting requested review from defanator

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reopen audit log on SIGUSR1 and SIGHUP

AltStyle によって変換されたページ (->オリジナル) /