Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Jul 4, 2025

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.

@mhl-b mhl-b requested review from ywangd and DaveCTurner July 4, 2025 03:44
@mhl-b mhl-b marked this pull request as ready for review July 4, 2025 03:44
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 4, 2025
@mhl-b mhl-b added :Distributed Coordination/Network Http and internode communication implementations Team:Distributed Coordination Meta label for Distributed Coordination team >enhancement and removed needs:triage Requires assignment of a team area label labels Jul 4, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

@elasticsearchmachine
Copy link
Collaborator

Hi @mhl-b, I've created a changelog YAML for you.

Copy link
Member

@ywangd ywangd left a 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.

if (request.isStreamedContent()) {
return "Request body had not been received at the time of the audit event";
}

Comment on lines +1075 to +1077
public boolean includeRequestBody() {
return includeRequestBody;
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +58 to +62
if (get() instanceof LoggingAuditTrail trail) {
return trail.includeRequestBody();
} else {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

one-liner?

Suggested change
if (get() instanceof LoggingAuditTrail trail) {
return trail.includeRequestBody();
} else {
return false;
}
return get() instanceof LoggingAuditTrail trail && trail.includeRequestBody();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Network Http and internode communication implementations >enhancement Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants