Audit authn success in the `SecurityRestFilter` by albertzaharovits · Pull Request #94120 · elastic/elasticsearch

albertzaharovits · 2023-02-24T15:22:22Z

Authentication for the HTTP channel is changing to not have access to the contents of the HTTP request body.
But auditing of authentication success still requires access to the request body.
Consequently, auditing of authentication success is now separated from the authentication logic:
auditing is invoked in the SecurityRestFilter#handleRequest, after the AuthenticationService
has done its job earlier in the handling flow for the HTTP request.

albertzaharovits · 2023-02-27T15:39:56Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java


    @Override
-    public void authenticationSuccess(String requestId, Authentication authentication, RestRequest request) {
+    public void authenticationSuccess(RestRequest request) {


This method must now be called in an authenticated context, when the HTTP request body is available.

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

albertzaharovits · 2023-02-27T15:47:02Z

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java


-        authenticationService.authenticate(maybeWrapRestRequest(request), ActionListener.wrap(authentication -> {
+        final RestRequest wrappedRequest = maybeWrapRestRequest(request);
+        authenticationService.authenticate(wrappedRequest, ActionListener.wrap(authentication -> {


This authenticationService.authenticate method is to be moved to a sooner site in the request handling flow (where it will not have access to the request body).

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

elasticsearchmachine · 2023-02-27T15:51:16Z

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

jakelandis · 2023-03-02T21:56:50Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

+            throw new ElasticsearchSecurityException("rest request attempted to inject a user", e);
+        }
+        if (authentication == null) {
+            throw new ElasticsearchSecurityException("Context is not authenticated");


nit: can you add a "should never happen..." comment here, or just remove this in favor of an assert authentication != null ? ( I prefer the later)

I added the comment. I strongly prefer a hard 500 when authN expectations are violated. It's going to become less of an obvious expectation (so less of an "assert" type of case) when we move the actual authN further away from this auditing point.

jakelandis · 2023-03-02T21:57:41Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

+    public void authenticationSuccess(RestRequest request) {
+        final String requestId = AuditUtil.extractRequestId(securityContext.getThreadContext());
+        if (requestId == null) {
+            throw new ElasticsearchSecurityException("Authenticated context must include request id");


nit: can you add a "should never happen..." comment here, or just remove this in favor of an assert requestId != null ? ( I prefer the later)

I added the comment, per https://github.com/elastic/elasticsearch/pull/94120/files#r1132377449

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

jakelandis · 2023-03-02T22:07:12Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

    }

+    /**
+     * This must be called after the HTTP request has been authenticated.


nit: not sure if there is easy access ... but if there is... can you add an assert in the code that authentication bits are present ?

It's easier to assert authentication in the called AuditTrail#authenticationSuccess, if I get you right.

jakelandis · 2023-03-02T22:26:44Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

        @Override
        void authenticationSuccess(Authentication authentication) {
-            auditTrail.authenticationSuccess(requestId, authentication, request);
+            // REST requests are audited in the {@code SecurityRestFilter} because they need access to the request body


This feels off ... it seems that we should have a reference to the AuditableRestRequest when we want to audit... even if we need to check instance type and downcast I think would be preferable to ignoring our own contract.

I agree this is not great, but I struggle to find better options...

Fundamentally, the AuditableRequest#authenticationSuccess is called after authentication was successful, and we don't want to audit HTTP requests that way (because we do the auditing in the SecurityRestFilter#handleRequest, for the RestRequest).

It is possible to alternatively have auditTrail.authenticationSuccess(requestId, authentication, request) be a noop. Or, to not have the context.getRequest().authenticationSuccess(authentication) called in the AuthenticatorChain if the request is a HTTP request. I think these options are more hackish.

What do you think?
We could also file this objection of yours, and come back to it later when the puzzle pieces are in place?

I think it is possible to change this method to be AuditableRequest.authenticationSuccess() and wrap the RestRequest earlier on in SecurityRestFilter. The authentication object should always be available in the threadContext (available for both AuditableRestRequest and AuditableTransportRequest) before this auditing method is called so that we don't it need to passed as an argument. In fact, having it available as both argument and threadContext is a potential duplication for bugs.

Concretely, what I am suggesting is:

Change the AuditableRequest#authenticationSuccess method to take no argument

No change to AuditTrail#authenticationSuccess methods

Let AuditableRestRequest and AuditableTransportRequest to extract authentication object from the threadContext and pass it down to the AuditTrail.

Wrap RestRequest as AuditableRestRequest earlier in SecurityRestFilter so that auditing can be consistently done with request.authenticationSuccess.

It is a bit larger changeset. But it helps maintain a more consistent audit logging contract.

Wrap RestRequest as AuditableRestRequest earlier in SecurityRestFilter so that auditing can be consistently done with request.authenticationSuccess.

I think this is glancing over important details.
Where are you proposing we invoke AuditableRequest#authenticationSuccess?
Currently, the callsite is in the AuthenticationService just after authn completed successfully AND has just been written to the thread context. The proposed implementation doesn't change the call site, but for REST requests only, turns it into a noop. It does that because the REST request body contents might be needed for auditing authn success and that won't be available, at the current callsite (i.e. after authN just finished and its result been written to the threadcontext). Instead, the proposed implementation, creates a new method in the AuditTrail class specifically for dealing with auditing of REST requests (to cover for the fact that now AuditableRequest#authenticationSuccess is a noop for REST requests) and calls that in the SecurityRestFilter.

I don't see an easy way to move the callsite of AuditableRequest#authenticationSuccess, which is what I think you're suggesting, because the AuditableRequest is tied to the AuthenticationService and it's lifetime doesn't go beyond the authN flow.
So maybe an option is to remove the AuditableRequest#authenticationSuccess completely and find a place to audit authN success directly via the AuditTrail class for both REST and transport requests?

We've met to discuss this.

Yang clarified that it's OK to audit authN success in the SecurityRestFilter.
The standing question was more about the interface to achieve it:

The AuditableRequest is not available in the SecurityRestFilter as it's constructed in the AuthenticationService and not exposed. The alternative options are to either reconstruct another instance of an AuditableRequest in the SecurityRestFilter or to expose the existing one from the AuthenticationService. Both are not simple changes, and it's not even clear why they're better compared to calling the AuditTrail#authenticationSuccess method directly, as the AuditTrail is already exposed for authZ event auditing.

We also discussed whether we should instead remove the AuditableRequest#authenticationSuccess method completely (for both REST and transport request types). This is again not a simple change, because we'd have to be careful to not audit authentication success for transport requests with existing authentication.

It's a close call, but, given the constraints, we're OK to go with the proposed approach to:

keep the AuditableRequest#authenticationSuccess method around and keep invoking it in the same way, in the AuthenticationService just after the authN has been set in the thread context, but

make it a noop for REST requests, and instead

directly call the AuditTrail#authenticationSuccess in the SecurityRestFilter, where the request body is available and the authN just completed (and is set in the thread context)

@albertzaharovits Thanks for summarizing our discussion. I think it might be useful to somehow make the summary available in the code, e.g. maybe add a comment with link to your above comment.

Pushed 8f5e1b3 .

…ectly

albertzaharovits · 2023-03-10T14:49:51Z

@ywangd @jakelandis ready for another round.

ywangd

This is looking promosing. I have a comment along the line of maintaining the audit logging contract that @jakelandis raised. I think it is a point that worth exploring a bit more. Thanks!

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

ywangd · 2023-03-21T05:37:46Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

        authenticate(
-            authListener -> authenticationService.authenticate(request, false, authListener),
+            authListener -> authenticationService.authenticate(request, false, authListener.delegateFailure((l, authentication) -> {
+                auditTrailService.get().authenticationSuccess(request);


Appologies, I may have been unclear. What I meant in #94120 (comment) is to audit both authentication and secondary authentication success in SecurityRestFilter, which I think is easier to follow and better visibility.

I agree it's easier to follow, but looking closer I don't think this is going to work correctly.

auditTrailService.get().authenticationSuccess(request); looks in the thread context for the authentication, but the SecondaryAuthenticator makes it such that the thread context doesn't contain the secondary authN when it invokes the listener of authenticateAndAttachToContext.

It is possible to have AuditTrail.authenticationSuccess taking an extra argument which specifies what key the authentication object is stored against, e.g.

void authenticationSuccess(RestRequest request, String authcContextKey);

or have it take a Function that knows how to extract authentication, e.g.

void authenticationSuccess(RestRequest request, Function<ThreadContext, Authentication> authcReader);

This would allow us to always call authenticationSuccess in the filter. It is not critical. I'll leave it to you to make the call. Thanks!

Hmmm... yeah, I haven't thought of that.
Though if authenticationSuccess audits the "current" authentication passing a selector doesn't look elegant to me.
Alternatively, we could change the "authenticationSuccess` method to audit all the authentications, but now it's getting "smarter" and it means we'll to wait for both to complete before we invoke the method. A less smarter option is to pass the authentication or the secondary authentication as arguments, instead of using a no-arg that looks at the "current" authn.

Overall I think there isn't much difference between all of these options. I think I'll leave it to what's there right now: ie. a simple method no-arg method, that audits the "current" authentication, but that needs to be called in the SecondaryAuthenticator too.

ywangd · 2023-03-21T05:56:12Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

        @Override
        void authenticationSuccess(Authentication authentication) {
-            auditTrail.authenticationSuccess(requestId, authentication, request);
+            // REST requests are audited in the {@code SecurityRestFilter} because they need access to the request body


I think it is possible to change this method to be AuditableRequest.authenticationSuccess() and wrap the RestRequest earlier on in SecurityRestFilter. The authentication object should always be available in the threadContext (available for both AuditableRestRequest and AuditableTransportRequest) before this auditing method is called so that we don't it need to passed as an argument. In fact, having it available as both argument and threadContext is a potential duplication for bugs.

Concretely, what I am suggesting is:

Change the AuditableRequest#authenticationSuccess method to take no argument

No change to AuditTrail#authenticationSuccess methods

Let AuditableRestRequest and AuditableTransportRequest to extract authentication object from the threadContext and pass it down to the AuditTrail.

Wrap RestRequest as AuditableRestRequest earlier in SecurityRestFilter so that auditing can be consistently done with request.authenticationSuccess.

It is a bit larger changeset. But it helps maintain a more consistent audit logging contract.

albertzaharovits · 2023-03-29T16:17:42Z

@ywangd I think we need to sync on #94120 (comment) . I'll reach out on Slack.

albertzaharovits · 2023-03-30T07:09:25Z

@ywangd Thank you for the thorough review and discussion!

Can you please take another look and verify that my 2 new comments #94120 (comment) and #94120 (comment) make sense to you?

ywangd

LGTM

Thanks for the discussion! There are tradeoffs in design decisions that we have to make due to various constraints. But importantly we have been explicit and agreed on the approach.

ywangd · 2023-03-31T00:04:13Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

        @Override
        void authenticationSuccess(Authentication authentication) {
-            auditTrail.authenticationSuccess(requestId, authentication, request);
+            // REST requests are audited in the {@code SecurityRestFilter} because they need access to the request body


@albertzaharovits Thanks for summarizing our discussion. I think it might be useful to somehow make the summary available in the code, e.g. maybe add a comment with link to your above comment.

ywangd · 2023-03-31T00:28:16Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java

        authenticate(
-            authListener -> authenticationService.authenticate(request, false, authListener),
+            authListener -> authenticationService.authenticate(request, false, authListener.delegateFailure((l, authentication) -> {
+                auditTrailService.get().authenticationSuccess(request);


It is possible to have AuditTrail.authenticationSuccess taking an extra argument which specifies what key the authentication object is stored against, e.g.

void authenticationSuccess(RestRequest request, String authcContextKey);

or have it take a Function that knows how to extract authentication, e.g.

void authenticationSuccess(RestRequest request, Function<ThreadContext, Authentication> authcReader);

This would allow us to always call authenticationSuccess in the filter. It is not critical. I'll leave it to you to make the call. Thanks!

ywangd · 2023-03-31T00:35:24Z

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

            }
-            secondaryAuthenticator.authenticateAndAttachToContext(request, ActionListener.wrap(secondaryAuthentication -> {
+            auditTrailService.get().authenticationSuccess(wrappedRequest);
+            secondaryAuthenticator.authenticateAndAttachToContext(wrappedRequest, ActionListener.wrap(secondaryAuthentication -> {


👍 Using wrappedRequest is in fact a bug fix! Good catch!

Not for this PR, but this prompts me to think: Do we still need audit request body on secondary authc success? It's the current behaviour. I don't think it's necessory. That said, secondary authc probably is not widely used to be a concern.

Do we still need audit request body on secondary authc success?

Since we couple request body auditing to authn success auditing, I can see how one might want to query for all the request bodies of requests issued by some principal, in which case I think the result should include the requests bodies where the queried principal was the secondary authentication. So, I think it makes sense to keep auditing the request body for secondary authn success, even though the same contents are audited twice. I think this is an argument in favor of having a standalone audit entry solely for the request body and linking it via the audit id to the other audit events.... But that's another discussion.

jakelandis

LGTM

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

albertzaharovits · 2023-04-03T10:22:54Z

@elasticsearchmachine run elasticsearch-ci/part-1-fips

albertzaharovits · 2023-04-03T10:51:59Z

@elasticsearchmachine run elasticsearch-ci/part-1-fips

Authentication for the HTTP channel is changing to not have access to the contents of the HTTP request body. But auditing of authentication success still requires access to the request body. Consequently, auditing of authentication success is now separated from the authentication logic: auditing is invoked in the SecurityRestFilter#handleRequest, after the AuthenticationService has done its job earlier in the handling flow for the HTTP request.

albertzaharovits added 2 commits February 24, 2023 15:43

Remove composite and index audit trails

61680e3

x-pack/plugin/core/src/main/java/module-info.java

19c741c

albertzaharovits self-assigned this Feb 24, 2023

elasticsearchmachine added the v8.8.0 label Feb 24, 2023

albertzaharovits added 6 commits February 24, 2023 17:55

Merge branch 'main' into remove-audit-dead-code

10d956e

Just main code

ee5c195

WIP

4bb1908

Merge branch 'main' into authn-success-for-rest-request

fe838db

Fix tests

372ee67

Nothing

d3f485a

albertzaharovits commented Feb 27, 2023

View reviewed changes

...ity/src/main/java/org/elasticsearch/xpack/security/authc/support/SecondaryAuthenticator.java Outdated Show resolved Hide resolved

albertzaharovits commented Feb 27, 2023

View reviewed changes

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java Outdated Show resolved Hide resolved

albertzaharovits changed the title ~~Authn success for rest request~~ Audit authn success in the SecurityRestFilter Feb 27, 2023

albertzaharovits added :Security/Audit X-Pack Audit logging >non-issue labels Feb 27, 2023

albertzaharovits requested a review from jakelandis February 27, 2023 15:50

albertzaharovits marked this pull request as ready for review February 27, 2023 15:50

elasticsearchmachine added the Team:Security Meta label for security team label Feb 27, 2023

jakelandis reviewed Mar 2, 2023

View reviewed changes

albertzaharovits added 4 commits March 10, 2023 13:32

Merge branch 'main' into authn-success-for-rest-request

fc603b7

nit should never happen

bc688ea

delegateFailure instead of runBefore

e14ddfe

SecondaryAuthenticator and SecurityRestFilter use auditingService dir…

39717a7

…ectly

albertzaharovits requested review from jakelandis and ywangd March 10, 2023 14:49

ywangd reviewed Mar 21, 2023

View reviewed changes

Merge branch 'main' into authn-success-for-rest-request

bfb96e8

Remove AuthenticationService#auditSuccess method

ede5e27

albertzaharovits requested a review from ywangd March 30, 2023 07:09

ywangd approved these changes Mar 31, 2023

View reviewed changes

albertzaharovits added 3 commits March 31, 2023 12:02

Merge branch 'main' into authn-success-for-rest-request

cc97e63

Merge branch 'main' into authn-success-for-rest-request

7eac80e

Comments on AuditableRestRequest#authenticationSuccess

8f5e1b3

jakelandis approved these changes Mar 31, 2023

View reviewed changes

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java Outdated Show resolved Hide resolved

jakelandis reviewed Mar 31, 2023

View reviewed changes

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java Outdated Show resolved Hide resolved

albertzaharovits added 4 commits April 3, 2023 10:12

Merge branch 'main' into authn-success-for-rest-request

c2207e9

show -> should

d79a89c

Merge branch 'main' into authn-success-for-rest-request

8cc91a4

Fix LoggingAuditTrailTests

0d4c3b5

albertzaharovits merged commit f353be2 into elastic:main Apr 3, 2023

albertzaharovits deleted the authn-success-for-rest-request branch April 3, 2023 11:30

Conversation

albertzaharovits commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Mar 10, 2023

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Mar 29, 2023

Uh oh!

albertzaharovits commented Mar 30, 2023

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

albertzaharovits commented Apr 3, 2023

Uh oh!

albertzaharovits commented Apr 3, 2023

albertzaharovits commented Feb 24, 2023 •

edited

Loading

albertzaharovits Mar 30, 2023 •

edited

Loading