-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add audit logging for stream content #130594
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
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Hi @mhl-b, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I left a somewhat important comment and appreciate if you could address it. Additionally, could you please remove the following code and maybe replace it by an assertion that the request is no longer streaming.
Lines 30 to 32 in 9be73c5
if (request.isStreamedContent()) { | |
return "Request body had not been received at the time of the audit event"; | |
} |
public boolean includeRequestBody() { | ||
return includeRequestBody; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems problematic. According to this comment, the non-volatile field includeRequestBody
is guaranteed to see the latest change because it is always read after the volatile field events
. It is true within LoggingAuditTrail
but no longer the case if we just return it here. I think we can either also read events
field here before return
or having SecurityRestFilter
register its own settings update consumer for the INCLUDE_REQUEST_BODY
setting so that it does not need to ask LoggingAuditTrail
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SecurityRestFilter needs to know both features : LoggingAuditTrail and includeRequestBody are enabled. Shouldnt be much of a difference. Kind of hacky, cutting through two layers of abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a read for events
before returnning should suffice in this case. Otherwise we may see stale value for includeRequestBody
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making includeRequestBody
volatile should be cleaner then, if I still need to read a volatile field. 9351f93
…rch into aggregating-audit-trail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if (get() instanceof LoggingAuditTrail trail) { | ||
return trail.includeRequestBody(); | ||
} else { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one-liner?
if (get() instanceof LoggingAuditTrail trail) { | |
return trail.includeRequestBody(); | |
} else { | |
return false; | |
} | |
return get() instanceof LoggingAuditTrail trail && trail.includeRequestBody(); |
Add support for HTTP content audit logging for streamed content. After #129302 we can aggregate streamed content within REST layer. When
INCLUDE_REQUEST_BODY
setting is specified every streamed request will be aggregated in SecurityRestFilter. This flag is effectively disable streaming support.