Skip to content
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

nginx: use copytruncate for modsecurity log rotation #957

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

osnyx
Copy link
Member

@osnyx osnyx commented Apr 4, 2024

Because modsecurity is not re-opening its logfile after rotation and continues to write into the same file descriptor, we need to use copytruncate.
Better handling of that situation is stuck upstream for several years. owasp-modsecurity/ModSecurity-nginx#121

We use the presence of /var/log/modesc_*.log as a heuristic for modsecurity being enabled, these files are now rotated with copytruncate.
All other nginx logs are still rotated by moving and reloading.
Note that, due to overlapping wildcard matches, this specific case got a higher logrotate match priority and needs an ignoreduplicates.

copytruncate is non-atomic and might loose some logs written between
copying and the truncation being done.

PL-132296

@flyingcircusio/release-managers

Release process

Impact: -

Changelog:

  • nginx, modsecurity: Fix improper rotation and continuous growth of /var/log/nginx/modsec_audit.log
    • log files of the pattern /var/log/nginx/modsec_*.log are now rotated via copying and then truncating the open log
    • all nginx modsecurity logfiles need to follow the naming scheme modsec_* to not accidentally grow unrotated. This is the naming convention suggested by our nginx modsecurity configuration examples.
    • Logs written directly at the time of the rotation might be lost.

PR release workflow (internal)

  • PR has internal ticket
  • internal issue ID (PL-…) part of branch name
  • internal issue ID mentioned in PR description text
  • ticket is on Platform agile board
  • ticket state set to Pull request ready
  • if ticket is more urgent than within the next few days, directly contact a member of the Platform team

Design notes

  • Provide a feature toggle if the change might need to be adjusted/reverted quickly depending on context. Consider whether the default should be on or off. Example: rate limiting.
  • All customer-facing features and (NixOS) options need to be discoverable from documentation. Add or update relevant documentation such that hosted and guided customers can understand it as well.

Security implications

  • Security requirements defined? (WHERE)
    • work around an (upstream) issue with modsecurity to prevent disk from filling up with logs (avaialbility)
    • rotation needs to be tested, overlap of logrotate rules
  • Security requirements tested? (EVIDENCE)
    • automated tests still pass
    • manually tested rotation on testvm, including modification of dates and stamps

@osnyx
Copy link
Member Author

osnyx commented Apr 4, 2024

We need to decide whether this also needs a NixOS tests. I do have an idea on how to build one, but this would further delay this PR.

@osnyx osnyx force-pushed the PL-132296-modescurity-nginx-restart branch from 31476b9 to 76c3668 Compare April 4, 2024 15:03
Because modsecurity is not re-opening its logfile after rotation and continues to write into the same file descriptor, we need to use `copytruncate`.
Better handling of that situation is stuck upstream for several years. owasp-modsecurity/ModSecurity-nginx#121

We use the presence of `/var/log/modesc_*.log` as a heuristic for modsecurity being enabled, these files are now rotated with copytruncate.
All other nginx logs are still rotated by moving and reloading.
Note that, due to overlapping wildcard matches, this specific case got a higher logrotate match priority and needs an `ignoreduplicates`.

`copytruncate` is non-atomic and might loose some logs written between
copying and the truncation being done.

PL-132296
@osnyx osnyx force-pushed the PL-132296-modescurity-nginx-restart branch from 76c3668 to 760d9ee Compare April 4, 2024 15:04
@dpausp dpausp changed the title nginx: service needs to be restarted for modsecurity log rotation nginx: use copytruncate for modsecurity log rotation Apr 4, 2024
@ctheune ctheune merged commit 7f952bf into fc-23.11-dev Apr 4, 2024
2 checks passed
@ctheune ctheune deleted the PL-132296-modescurity-nginx-restart branch April 4, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants