Log request body to audit logs #5071
Conversation
olkkoti
requested review from
cwperks,
DarshitChanpura,
derek-ho,
nibix,
peternied,
RyanL1997,
reta and
willyborankin
as code owners
olkkoti
changed the title
There was a problem hiding this comment.
wdyt about adding auditLog.logSucceededLogin(user.getName(), request); before each call to delegate.handleRequest(request, channel, client);?
We could also remove these calls from the BackendRegistry and rely on them to be called within here.
To enable audit logs for this category, you would need to add then AUTHENTICATED category as one of the enabled categories.
|
How about like this? |
olkkoti
force-pushed
the
auditlog-without-rest-authorization
branch
4 times, most recently
from
aef31e2 to
c49ca35
Compare
|
Hmm. Apparently at least in test cases user is allowed to be null here. I guess then we just dont do audit log? Or would it be better to call it with null effectiveUser? |
olkkoti
force-pushed
the
auditlog-without-rest-authorization
branch
3 times, most recently
from
e837d29 to
e4f6268
Compare
| if (effectiveUser != authenticatedUser) { | ||
| threadPool.getThreadContext() | ||
| .putTransient(ConfigConstants.OPENDISTRO_SECURITY_IMPERSONATION_INITIATING_USER, authenticatedUser.getName()); | ||
| } | ||
| UserSubject subject = new UserSubjectImpl(threadPool, effectiveUser); | ||
| threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject); |
There was a problem hiding this comment.
Thank you @olkkoti . Apologies for not getting back sooner.
Curious, why not use the value from this persistent header?
In the case where impersonation is being used, looks like the current behavior is to audit log the authenticated user (not the user being impersonated - makes sense). In that case, maybe we can use the new header you are introducing in here and use it if the header is populated.
There was a problem hiding this comment.
I am not sure what you mean. The signature of logSucceededLogin looks like this:
public void logSucceededLogin(String effectiveUser, boolean securityAdmin, String initiatingUser, SecurityRequest request)
I understood it so that the user being impersonated should be effectiveUser and authenticated user should be initiatingUser. So I added the authenticated user to context so I can log it later in SecurityRestFilter.
There was a problem hiding this comment.
Actually now when I look closer, it seems that logSucceededLogin was called with effectiveUser and initiatingUser, even if they are the same user. So, I can always add authenticated user in context even there is no impersonation going on and always log it. Tests will probably pass with this also.
olkkoti
force-pushed
the
auditlog-without-rest-authorization
branch
from
e4f6268 to
017f342
Compare
…try in order to get request body. This fixes the issue opensearch-project#4094 Signed-off-by: Timo Olkkonen <timo.olkkonen@reaktor.com>
olkkoti
force-pushed
the
auditlog-without-rest-authorization
branch
from
017f342 to
0092ae8
Compare
Description
Currently we do not get request body to audit log, as requests are already authenticated in header verifier level, which does not have access to request body.
SecurityRestFilter has the whole RestRequest object, but it does not authenticate it again. Also, it does not authorize the request if the route is not a named one. The idea here is to do logGrantedPrivileges call even if we do not authorize. This way we will get the request body to audit log.
Issues Resolved
[BUG] audit_request_body missing for REST since 2.11
Testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.