Skip to content

Handle streaming request body in audit log #127798

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 7, 2025
Merged

Handle streaming request body in audit log #127798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8d86417
Handle streaming request body in audit log
DaveCTurner May 7, 2025
97ee0b4
Update docs/changelog/127798.yaml
DaveCTurner May 7, 2025
b085e91
Merge branch 'main' into 2025/05/07/audit-log-streaming-request-body
DaveCTurner May 7, 2025
35cddcb
Merge branch 'main' into 2025/05/07/audit-log-streaming-request-body
DaveCTurner May 7, 2025
a873639
Merge branch 'main' into 2025/05/07/audit-log-streaming-request-body
DaveCTurner May 7, 2025
c5f9b3b
Merge branch 'main' into 2025/05/07/audit-log-streaming-request-body
DaveCTurner May 7, 2025
423234b
Merge branch 'main' into 2025/05/07/audit-log-streaming-request-body
DaveCTurner May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/127798.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 127798
summary: Handle streaming request body in audit log
area: Audit
type: bug
issues: []
23 changes: 23 additions & 0 deletions ...curity/qa/audit/src/javaRestTest/java/org/elasticsearch/xpack/security/audit/AuditIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

package org.elasticsearch.xpack.security.audit;

import org.apache.http.entity.ContentType;
import org.apache.http.entity.StringEntity;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.common.bytes.BytesReference;
Expand All @@ -27,6 +29,7 @@
import org.junit.ClassRule;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.time.Instant;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
Expand All @@ -37,6 +40,7 @@
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;

import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.hasKey;
Expand Down Expand Up @@ -103,6 +107,25 @@ public void testFilteringOfRequestBodies() throws Exception {
});
}

public void testAuditAuthenticationSuccessForStreamingRequest() throws Exception {
final Request request = new Request("POST", "/testindex/_bulk");
request.setEntity(new StringEntity("""
{"index":{}}
{}
""", ContentType.create("application/x-ndjson", StandardCharsets.UTF_8)));
executeAndVerifyAudit(
request,
AuditLevel.AUTHENTICATION_SUCCESS,
event -> assertThat(
event,
allOf(
hasEntry(LoggingAuditTrail.AUTHENTICATION_TYPE_FIELD_NAME, "REALM"),
hasEntry(LoggingAuditTrail.REQUEST_BODY_FIELD_NAME, "Request body had not been received at the time of the audit event")
)
)
);
}

private void executeAndVerifyAudit(Request request, AuditLevel eventType, CheckedConsumer<Map<String, Object>, Exception> assertions)
throws Exception {
Instant start = Instant.now();
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public class AuditUtil {

public static String restRequestContent(RestRequest request) {
if (request.hasContent()) {
if (request.isStreamedContent()) {
return "Request body had not been received at the time of the audit event";
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to eventually support this use case? If not, can we provide more details here? Something like

Audit logging with request body is not supported when the request is streamed. To disable request streaming, set [rest.incremental_bulk] to [false].

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think eventually we'll need this for streaming requests (because eventually we will be handling all requests as streaming). It's not reasonable to log an arbitrarily-large body in a single audit event tho, instead we will need to record each chunk in the audit log as they arrive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that today because of how we try and log the whole body in a single message we end up truncating it anyway after a few kiB even with rest.incremental_bulk: true.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining. The fix here is certainly a lot better than throwing ClassCastException. But we may still want to create an issue to say a future fix is pending?

we end up truncating it anyway after a few kiB

IIRC, we don't truncate audit logs. At least payloads of a few hundred KB are fully logged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started a design doc and opened ES-11760

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, we don't truncate audit logs

You're right, TIL. And yet we seem to truncate other logs messages emitted by Log4J. I wonder why (but not hard enough to go digging further).

}
var content = request.content();
try {
return XContentHelper.convertToJson(content, false, false, request.getXContentType());
Expand Down
Loading