Add new audit handler method for action responses #63708
Conversation
albertzaharovits
added
>non-issue
:Security/Audit
| @@ -47,6 +48,10 @@ void accessGranted(String requestId, Authentication authentication, String actio | |||
| void accessDenied(String requestId, Authentication authentication, String action, TransportRequest transportRequest, | |||
| AuthorizationInfo authorizationInfo); | |||
|
|
|||
| // this is the only audit method that is called *after* the action executed, when the response is available | |||
| void actionResponse(String requestId, Authentication authentication, String action, TransportRequest transportRequest, | |||
| TransportResponse transportResponse); | |||
There was a problem hiding this comment.
the new audit method.
| Authentication authentication = securityContext.getAuthentication(); | ||
| auditTrailService.get().actionResponse(requestId, authentication, action, request, response); | ||
| contextPreservingListener.onResponse(response); | ||
| }); |
There was a problem hiding this comment.
This intercepts the response for the action that is executed inside the rest handler (that do NOT go to other cluster nodes).
| public void sendResponse(Exception exception) throws IOException { | ||
| channel.sendResponse(exception); | ||
| } | ||
| }, task); |
There was a problem hiding this comment.
This intercepts the response for actions executed on other cluster nodes (when an action on the coordinating node submits requests to other nodes - or even the same node)
...urity/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java
Outdated
Show resolved
Hide resolved
| // it is however *only called for coordinating actions*, which are the actions that a client invokes as opposed to | ||
| // the actions that a node invokes in order to service a client request | ||
| void coordinatingActionResponse(String requestId, Authentication authentication, String action, TransportRequest transportRequest, | ||
| TransportResponse transportResponse); |
There was a problem hiding this comment.
This is not technically true, some actions' (TransportMasterNodeAction) responses are intercepted on the coordinating node, as well as on the master node.
The naming and descriptions are difficult to write to convey the subset of actions that are covered.
We could also bleed some internals in here and mention the SecurityActionFilter.
Open to suggestions.
There was a problem hiding this comment.
I think we could keep the previous name, i.e. actionResponse. Please let me explain: When I suggested pulling out the code from SecurityServerTransportInterceptor, it was in the context of getting auditing for API key ID in for v7.11.0. The reasons are:
- The response in
SecurityServerTransportInterceptoris a fairly low level indexing response and does not really provide extra valuable information than the one in the coordinating node. Therefore auditing on just the coordinating node is sufficient. - Creating API key is underlyingly a
TransportReplicationAction. UnlikeTransportMasterNodeAction, it does not go through theSecurityActionFilter(on the remote node). This means: on the remote node, we will not get another auditing entry fromSecurityActionFilter, i.e. there will only be a single auditing entry about API key creation and it is on the coordinating node.
So my motivation was rather narrowly focused on the particular use case. The agreement, as I understand it, is to defer the auditing code in SecurityServerTransportInterceptor until the next release. Because it is not necessary for API key auditing in this release and if it is not in use, it's better to be left out. But we also agreed that it could still be useful when we expand the usage scenario and this is subject to further discussion.
IIUC, your intention of changing the method name is for the possibility to have two different audit event types. I should have been clear that it was not my proposal. I consider whether auditing inside SecurityServerTransportInterceptor an implementation detail, which we chose to opt out for initial release, but could choose to add it later as an enhancement. For verbosity and duplication control, I'd prefer to solve it in a different way other than splitting it into different audit event types. I hope this makes sense.
There was a problem hiding this comment.
Thanks for the detailed explanation Yang, but I would like to focus the conversation on the scope of this PR only. We should consider the broader picture, as you describe, but ultimately this PR is what gets merged as a unit, and what we merge in the future is subject to debate still.
If we merge this PR, which adds the audit hook for the responses of actions intercepted by the SecurityActionFilter only, we add to the interface of the LoggingAuditTrail .
When we later merge the code to allow intercepting the responses of the other actions, we have the options to:
- reuse the same
LoggingAuditTrailhook method - rename the existing hook method, or
- add a new audit response hook method.
(or any mix of them, really, if we feel wild)
Because this is a private interface, I don't believe we have to name it in anticipation of its use. We should name it for the purpose that it serves right now, as we can always tinker with it later. For this reason, I would not favor the actionResponse name.
There was a problem hiding this comment.
Just trying to brainstorm here: How about calling this one actionResponse and the other one transportResponse?
As you said, this is a private interface, so we don't have to struggle too much with its name now. So please feel free to use whatever works for you.
There was a problem hiding this comment.
Yeah, this could work. I'll keep this in mind when we'll iterate on the interface.
|
@ywangd As convened, I have chopped this PR to insert the new audit hook method only on the responses of actions intercepted by the Ready for feedback. |
There was a problem hiding this comment.
LGTM. Thanks for the iterations!
Other than the following few minor comment. I also commented on the method name of the new audit method. The changes to AuthenticationServiceTests are quite noisy, but I cannot think of a better way of doing it without major revamping. I think I won’t bother with it given the changes do the right job and they are test only code.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Outdated
Show resolved
Hide resolved
...ecurity/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java
Outdated
Show resolved
Hide resolved
|
@tvernum are you planning on reviewing as well? |
...urity/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java
Outdated
Show resolved
Hide resolved
…ecurity/action/filter/SecurityActionFilter.java Co-authored-by: Tim Vernum <[email protected]>
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
albertzaharovits
changed the title
This adds a new method to the AuditTrail that intercepts the responses of transport-level actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response). After careful deliberation, the new method is called for the responses of actions that are intercepted by the `SecurityActionFilter` only, and not by the transport filter. In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the requestId as well as the authentication as arguments (in addition to the request itself and the response). This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon. Related elastic#63221
albertzaharovits
changed the title
This adds a new method to the AuditTrail that intercepts the responses of transport-level actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response). After careful deliberation, the new method is called for the responses of actions that are intercepted by the `SecurityActionFilter` only, and not by the transport filter. In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the requestId as well as the authentication as arguments (in addition to the request itself and the response). This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon. Related #63221
This adds a new method to the
AuditTrailthat intercepts the responses of transport actions. This new method is unlike all the other existing audit methods because it is called after the action has been run (so that it has access to the response).In order to facilitate the "linking" of the new audit event with the other existing events, the audit method receives the
requestIdand theauthenticationas arguments (in addition to therequestitself and theresponse).This is labeled non-issue because it is only the foundation upon which later features that actually print out (some) responses can be built upon.
Related #63221