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

Compress audit logs #64472

Merged
merged 13 commits into from
Dec 2, 2020
Merged

Compress audit logs #64472

merged 13 commits into from
Dec 2, 2020

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Nov 2, 2020

audit logs should be compressed when rolling over due to size based
triggering policy breaching 1GB.
Files are not being deleted.

closes #63843

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

audit logs should be compressed when rolling over due to size based
triggering policy breaching 1GB.
Total number of zipped files should be the same as for other log = 4

closes elastic#63843
@pgomulka pgomulka added :Core/Infra/Logging Log management and logging utilities :Security/Audit X-Pack Audit logging v8.0.0 v7.11.0 labels Nov 2, 2020
@pgomulka pgomulka self-assigned this Nov 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Logging)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Audit)

@elasticmachine elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels Nov 2, 2020
@pgomulka pgomulka removed the v7.11.0 label Nov 6, 2020
@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@tvernum tvernum removed their request for review November 11, 2020 00:16
@tvernum
Copy link
Contributor

tvernum commented Nov 11, 2020

I'll leave this review in @albertzaharovits's hands.

appender.audit_rolling.policies.type = Policies
appender.audit_rolling.policies.time.type = TimeBasedTriggeringPolicy
appender.audit_rolling.policies.time.interval = 1
appender.audit_rolling.policies.time.modulate = true
appender.audit_rolling.policies.size.type = SizeBasedTriggeringPolicy
appender.audit_rolling.policies.size.size = 1GB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to explicitly set the nomax file index attribute.

appender.audit_rolling.strategy.type = DefaultRolloverStrategy
appender.audit_rolling.strategy.fileIndex = nomax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right, good find.

@albertzaharovits
Copy link
Contributor

@pgomulka Sorry for the huge delay on this PR.

Because the log file name changes, some folks might consider it breaking,
though only past rolled-over names are changed which I think is key. This change
brings the audit logs more in line with the other logs which is also worth pointing out.
But folks might have automated tools set up for all the logs and we might
get into arguments. To be safe, I would drop a breaking change notice, on 7.x, on the lines:

diff --git a/docs/reference/migration/migrate_7_11.asciidoc b/docs/reference/migration/migrate_7_11.asciidoc
index 20ab1574350..76a9ccbb6f0 100644
--- a/docs/reference/migration/migrate_7_11.asciidoc
+++ b/docs/reference/migration/migrate_7_11.asciidoc
@@ -32,3 +32,26 @@ will be applied to the values of a keyword field; for example, a
 field configured with a lowercase normalizer will return highlighted
 snippets in lower case.
 ====
+
+[discrete]
+[[breaking_711_security_changes]]
+=== Security changes
+
+[[audit-logs-are-rolled-over-and-archived-by-size]]
+.Audit logs are rolled-over and archived by size.
+[%collapsible]
+====
+*Details* +
+In addition to the existing daily rollover, the security audit logs are
+now rolled-over by disk size limit as well. Moreover, the rolled-over logs
+are also gzip compressed.
+
+*Impact* +
+The names of rolled over audit logfiles (but not the name of the current log)
+have changed.
+If you've setup automated tools to consume these files, you must configure them
+to use the new names and to possibly account for gzip archives instead of plaintext.
+The Docker build of Elasticsearch is not affected since it logs on stdout where
+rollover is not performed.
+
+====

Similarly for docs/reference/migration/migrate_8_0.asciidoc on master.

Also for safety I would label the PR as >breaking .

Also I would work a bit on the description of the PR, on the same lines with the deprecation notice.

@albertzaharovits
Copy link
Contributor

albertzaharovits commented Dec 1, 2020

After writing the above I noticed you've removed the 7.11 label.
I'm curious for the motivation.

@pgomulka
Copy link
Contributor Author

pgomulka commented Dec 1, 2020

I removed the 7.11 label as the logs compression would be only enabled in 8.0 exactly for the reason you described #63843 (comment)
This is a breaking change and we should deprecate in 7.x

@pgomulka
Copy link
Contributor Author

pgomulka commented Dec 1, 2020

@elasticmachine update branch

@albertzaharovits
Copy link
Contributor

Alright, then. It's cautious to merge to master only and we've been not compressing for a long time and few have complained about it.

Thank you for solving!

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Logging Log management and logging utilities :Security/Audit X-Pack Audit logging Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log4j2.properties should compress audit logs by default. maybe all logs.
5 participants