Skip to content

Handle audit exceptions #64512

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

Conversation

albertzaharovits
Copy link
Contributor

This PR ensures that exceptions in the AuditTrail methods are properly handled.
Currently the log4j appender is configured to swallow exceptions, but after this is merged we actually document the advice to turn that on (or turn it on ourselves).

Related #29905
#62916

@albertzaharovits
Copy link
Contributor Author

@tvernum @ywangd I've raised this in relation to #62916 .
Log4j currently swallows exceptions, so the audit methods have so far been sprinkled liberally disregarding the behaviour when throw. But #62916 complicates it because the audit methods might throw (due to bugs or due to a mischievous transport clients); so we need to consider (and test) the throw behaviour. Then if the throw behaviour is robust, we can advise users to toggle the ignoreExceptions in the audit appender (or toggle it ourselves) and close #29905 (if auditing fails, you'd be getting failures for every subsequent requests - the known state).

@albertzaharovits
Copy link
Contributor Author

WIP only handles exceptions in the AuthorizationService and no tests.

@ywangd
Copy link
Member

ywangd commented Nov 5, 2020

A general comment: maybe we should add a setting for this behaviour, i.e. fail when an exception is caught during auditing time? Because I am not sure whether all customers would be more concerned about these exception than letting cluster continue to handle requests. Also #29905 says "add option" if you read it literally.

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

This approach seems right to me, although I agree with @ywangd's earlier comments - not everyone will want to fail all cluster activity just because auditing failed.

@arteam arteam added v8.1.0 and removed v8.0.0 labels Jan 12, 2022
@mark-vieira mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022
@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:12
@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
@albertzaharovits
Copy link
Contributor Author

I'm going to close this as I don't have the time to continue working on it.
Anyone please feel free to reuse anything you can scavange out of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants