From 94d6b7e4ca22bd7a229e83773fbaea8a9de93f25 Mon Sep 17 00:00:00 2001 From: Muzzamil Jolaade Date: Thu, 16 Jul 2026 15:39:36 -0700 Subject: [PATCH] feat(audit): Add audit_request_id correlation field to all FGAC audit events Add a unique request correlation ID to every audit event so multiple events from the same REST request can be traced together. Uses the client-provided X-Request-Id header when available, otherwise generates a UUID per request as fallback. Implementation: - SecurityRestFilter: reads X-Request-Id header or generates UUID, stashes in ThreadContext as transient - AuditLogImpl.save(): reads from ThreadContext and stamps on every message before routing to sinks - AuditMessage: new REQUEST_ID field constant and addRequestId() setter Works across all FGAC event types: AUTHENTICATED, GRANTED_PRIVILEGES, MISSING_PRIVILEGES, INDEX_EVENT, COMPLIANCE_DOC_READ/WRITE, etc. Signed-off-by: Muzzamil Jolaade --- .../AuditRequestIdCorrelationTest.java | 157 ++++++++++++++++++ .../opensearch/security/rest/WhoAmITests.java | 5 + .../auditlog/impl/AbstractAuditLog.java | 2 +- .../security/auditlog/impl/AuditLogImpl.java | 8 + .../security/auditlog/impl/AuditMessage.java | 8 + .../security/filter/SecurityRestFilter.java | 17 ++ .../security/support/ConfigConstants.java | 6 + .../auditlog/impl/AuditMessageTest.java | 30 ++++ 8 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java diff --git a/src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java b/src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java new file mode 100644 index 0000000000..f708e1546a --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java @@ -0,0 +1,157 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.security; + +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.hc.core5.http.message.BasicHeader; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; + +import org.opensearch.security.auditlog.impl.AuditCategory; +import org.opensearch.security.auditlog.impl.AuditMessage; +import org.opensearch.test.framework.AuditCompliance; +import org.opensearch.test.framework.AuditConfiguration; +import org.opensearch.test.framework.AuditFilters; +import org.opensearch.test.framework.audit.AuditLogsRule; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.matchesPattern; +import static org.hamcrest.Matchers.notNullValue; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN; + +/** + * Integration tests for the audit_request_id correlation field. + * Verifies that all audit events from a single REST request share the same + * request ID, and that different requests produce different IDs. + */ +public class AuditRequestIdCorrelationTest { + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(USER_ADMIN) + .audit( + new AuditConfiguration(true).compliance( + new AuditCompliance().enabled(true).writeWatchedIndices(List.of("watched-*")).writeLogDiffs(true) + ).filters(new AuditFilters().enabledRest(true).enabledTransport(true)) + ) + .build(); + + @Rule + public AuditLogsRule auditLogsRule = new AuditLogsRule(); + + @Test + public void shouldCorrelateMultipleEventsFromSameRequest() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + client.putJson("correlation-test/_doc/1", "{\"field\":\"value\"}"); + } + + auditLogsRule.waitForAuditLogs(); + List messages = auditLogsRule.getCurrentTestAuditMessages(); + + // Should produce multiple events (AUTHENTICATED + GRANTED_PRIVILEGES at minimum) + assertThat(messages.size(), greaterThanOrEqualTo(2)); + + // All events should have the same audit_request_id + Set requestIds = messages.stream().map(msg -> msg.getAsMap().get(AuditMessage.REQUEST_ID)).collect(Collectors.toSet()); + + // All non-null and all the same + assertThat("All events should have a request ID", requestIds.stream().allMatch(id -> id != null), equalTo(true)); + assertThat("All events from same request should share one ID", requestIds.size(), equalTo(1)); + } + + @Test + public void shouldUseClientProvidedXRequestIdHeader() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + client.putJson("header-id-test/_doc/1", "{\"data\":\"test\"}", new BasicHeader("X-Request-Id", "custom-trace-id-abc123")); + } + + auditLogsRule.waitForAuditLogs(); + List messages = auditLogsRule.getCurrentTestAuditMessages(); + + assertThat(messages.size(), greaterThanOrEqualTo(1)); + + // All events should use the client-provided ID + for (AuditMessage msg : messages) { + assertThat(msg.getAsMap().get(AuditMessage.REQUEST_ID), equalTo("custom-trace-id-abc123")); + } + } + + @Test + public void shouldGenerateUuidWhenNoXRequestIdHeader() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + client.get("_cluster/health"); + } + + auditLogsRule.waitForAuditLogs(); + List messages = auditLogsRule.getCurrentTestAuditMessages(); + + assertThat(messages.size(), greaterThanOrEqualTo(1)); + + // Should have a generated UUID (36 chars: 8-4-4-4-12) + String requestId = (String) messages.get(0).getAsMap().get(AuditMessage.REQUEST_ID); + assertThat(requestId, notNullValue()); + assertThat(requestId, matchesPattern("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}")); + } + + @Test + public void shouldProduceDifferentIdsForDifferentRequests() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + client.get("_cluster/health"); + client.get("_cluster/health"); + } + auditLogsRule.waitForAuditLogs(); + List messages = auditLogsRule.getCurrentTestAuditMessages(); + + // Collect all distinct request IDs + Set requestIds = messages.stream() + .map(msg -> msg.getAsMap().get(AuditMessage.REQUEST_ID)) + .filter(id -> id != null) + .collect(Collectors.toSet()); + + // Two requests should produce at least 2 distinct IDs + assertThat("Different requests should have different IDs", requestIds.size(), greaterThanOrEqualTo(2)); + } + + @Test + public void shouldIncludeRequestIdInComplianceDocWriteEvent() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + client.putJson( + "watched-corr/_doc/1", + "{\"field\":\"compliance-write-test\"}", + new BasicHeader("X-Request-Id", "compliance-trace-123") + ); + } + + auditLogsRule.waitForAuditLogs(); + List messages = auditLogsRule.getCurrentTestAuditMessages(); + + // Find compliance doc write event + List complianceWrites = messages.stream() + .filter(msg -> msg.getCategory() == AuditCategory.COMPLIANCE_DOC_WRITE) + .collect(Collectors.toList()); + + assertThat("Should produce a COMPLIANCE_DOC_WRITE event", complianceWrites.size(), greaterThanOrEqualTo(1)); + + // Compliance event should have the client-provided request ID + String complianceRequestId = (String) complianceWrites.get(0).getAsMap().get(AuditMessage.REQUEST_ID); + assertThat("Compliance write event should have request ID", complianceRequestId, equalTo("compliance-trace-123")); + } +} diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java index c0ac25ce38..8112033d82 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -42,6 +42,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.notNullValue; import static org.opensearch.rest.RestRequest.Method.GET; import static org.opensearch.security.auditlog.impl.AuditCategory.GRANTED_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.MISSING_PRIVILEGES; @@ -322,6 +323,10 @@ private void assertCommonFields(Set commonFields, Map re } else if (key.equals("audit_request_layer")) { assertThat(restMsgFields.get(key).toString(), equalTo("REST")); assertThat(transportMsgFields.get(key).toString(), equalTo("TRANSPORT")); + } else if (key.equals(AuditMessage.REQUEST_ID)) { + // Request IDs are unique per request — skip equality check, just verify non-null + assertThat(restMsgFields.get(key), notNullValue()); + assertThat(transportMsgFields.get(key), notNullValue()); } else { assertThat(restMsgFields.get(key), equalTo(transportMsgFields.get(key))); } diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java index 9ccd0c1372..e8893fc6b3 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java @@ -92,7 +92,7 @@ public abstract class AbstractAuditLog implements AuditLog { protected final Logger log = LogManager.getLogger(this.getClass()); - private final ThreadPool threadPool; + protected final ThreadPool threadPool; private final IndexNameExpressionResolver resolver; private final ClusterService clusterService; private final Settings settings; diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java index 362aa82666..2eb8774ea4 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java @@ -30,6 +30,7 @@ import org.opensearch.security.auditlog.config.AuditConfig; import org.opensearch.security.auditlog.routing.AuditMessageRouter; import org.opensearch.security.filter.SecurityRequest; +import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.UserFactory; import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; @@ -134,6 +135,13 @@ public void close() throws IOException { @Override protected void save(final AuditMessage msg) { if (enabled) { + // Try transient first (coordinating node, includes server-generated UUID), + // fall back to X-Request-Id header (propagated by core to remote nodes) + String requestId = threadPool.getThreadContext().getTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID); + if (requestId == null) { + requestId = threadPool.getThreadContext().getHeader(Task.X_REQUEST_ID); + } + msg.addRequestId(requestId); messageRouter.route(msg); } } diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java index ded8cb3878..ab70ed7ab0 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java @@ -141,6 +141,8 @@ public final class AuditMessage { public static final String SPLIT_MESSAGE_IDENTIFIER = "audit_split_message_id"; + public static final String REQUEST_ID = "audit_request_id"; + private static final DateTimeFormatter DEFAULT_FORMAT = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss.SSSZZ"); private final Map auditInfo = new HashMap(50); private final AuditCategory msgCategory; @@ -172,6 +174,12 @@ public void addRemoteAddress(TransportAddress remoteAddress) { } } + public void addRequestId(String requestId) { + if (requestId != null) { + auditInfo.put(REQUEST_ID, requestId); + } + } + public void addIsAdminDn(boolean isAdminDn) { auditInfo.put(IS_ADMIN_DN, isAdminDn); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 210d3f20c5..fe86baf8a2 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.UUID; import java.util.regex.Pattern; import javax.net.ssl.SSLPeerUnverifiedException; @@ -71,6 +72,7 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.HTTPHelper; import org.opensearch.security.user.User; +import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.client.node.NodeClient; @@ -176,6 +178,21 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); + // Stash audit correlation ID: use X-Request-Id header if present, otherwise generate UUID + if (threadContext.getTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID) == null) { + String requestId = threadContext.getHeader(Task.X_REQUEST_ID); + if (requestId == null || requestId.isEmpty()) { + requestId = UUID.randomUUID().toString(); + } else { + // Sanitize: limit length and strip control characters to prevent log injection + if (requestId.length() > 128) { + requestId = requestId.substring(0, 128); + } + requestId = requestId.replaceAll("[\\p{Cntrl}]", ""); + } + threadContext.putTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID, requestId); + } + RestRequest filteredRequest = maybeFilterRestRequest(request); final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 8ef4db809f..0a2c0624ff 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -88,6 +88,12 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address"; public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address_header"; + /** + * ThreadContext transient key for the audit request correlation ID. + * Set at REST entry point from X-Request-Id header or generated UUID. + */ + public static final String SECURITY_AUDIT_REQUEST_ID = OPENDISTRO_SECURITY_CONFIG_PREFIX + "audit_request_id"; + public static final String OPENDISTRO_SECURITY_INITIAL_ACTION_CLASS_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "initial_action_class_header"; diff --git a/src/test/java/org/opensearch/security/auditlog/impl/AuditMessageTest.java b/src/test/java/org/opensearch/security/auditlog/impl/AuditMessageTest.java index 3103e92908..38573bbf95 100644 --- a/src/test/java/org/opensearch/security/auditlog/impl/AuditMessageTest.java +++ b/src/test/java/org/opensearch/security/auditlog/impl/AuditMessageTest.java @@ -271,4 +271,34 @@ public void testToJsonSplitIndices() { // all messages share the same ID assertThat(splitMessages.stream().map(AuditMessageTest::getSplitMessageId).distinct().count(), is(1L)); } + + // ===================================================================== + // audit_request_id — correlation field tests + // ===================================================================== + + @Test + public void testAddRequestIdPopulatesField() { + message.addRequestId("4bf92f3577b34da6a3ce929d0e0e4736"); + assertThat(message.getAsMap().get(AuditMessage.REQUEST_ID), is("4bf92f3577b34da6a3ce929d0e0e4736")); + } + + @Test + public void testAddRequestIdNullIsIgnored() { + message.addRequestId(null); + assertNull(message.getAsMap().get(AuditMessage.REQUEST_ID)); + } + + @Test + public void testAddRequestIdEmptyStringIsStored() { + // Empty string is technically valid — the null check only skips null + message.addRequestId(""); + assertThat(message.getAsMap().get(AuditMessage.REQUEST_ID), is("")); + } + + @Test + public void testAddRequestIdAppearsInJson() throws Exception { + message.addRequestId("trace-abc-123"); + String json = message.toJson(); + assertThat(json, containsString("\"audit_request_id\":\"trace-abc-123\"")); + } }