Skip to content

Conversation

@albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Feb 24, 2023

This PR removes:

  • the CompositeAuditTrail which is designed to fan-out auditing events to multiple logger implementation types. This is not needed because since v7.0 there's only one audit logger implementation, the logfile.
  • any traces of the index-based logger implementation, namely the permission of the internal _xpack user to read the audit log index.

Related: #37707

@albertzaharovits albertzaharovits self-assigned this Feb 24, 2023
@albertzaharovits albertzaharovits changed the title Removes dead code related to auditing Remove dead code related to auditing Feb 24, 2023
@albertzaharovits albertzaharovits marked this pull request as ready for review February 24, 2023 13:54
@elasticsearchmachine elasticsearchmachine added v8.8.0 Team:Security Meta label for security team labels Feb 24, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

Linking the PR (#37707) that removed the index audit here for reference.

Comment on lines +37 to +38
public AuditTrailService(@Nullable AuditTrail auditTrail, XPackLicenseState licenseState) {
this.auditTrail = auditTrail;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use NOOP_AUDIT_TRAIL when the pass-in auditTrail is null? I think it's better if we don't have to deal with null within the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely better to not have to deal with nulls. Problem is that null here means that auditing is not enabled, while NOOP is intended to be used when license level does not allow auditing.
I prefer to not have to deal with the two meanings using the same logger just now...
But thanks for the good point!

@albertzaharovits albertzaharovits merged commit 7294508 into elastic:main Feb 27, 2023
@albertzaharovits
Copy link
Contributor Author

Thank you for the review Yang!

@albertzaharovits albertzaharovits deleted the remove-audit-dead-code branch February 27, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Security/Audit X-Pack Audit logging Team:Security Meta label for security team v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants