feat(audit): Add audit_request_id correlation field to all FGAC audit…

[Taiwo435]

Description

Add an audit_request_id field to all audit events so that multiple events generated by a single request (AUTHENTICATED → GRANTED_PRIVILEGES → COMPLIANCE_DOC_WRITE) can be correlated.

How it works

• SecurityRestFilter reads the X-Request-Id header if present, otherwise generates a UUID
• Sanitizes client-provided values (truncates to 128 chars, strips control characters)
• Stored in ThreadContext as a transient for local events
• AuditLogImpl.save() reads the transient first, falls back to X-Request-Id header (propagated by core across nodes)
• All events from the same request share the same ID

Cross-node propagation

When a client sends X-Request-Id, OpenSearch core propagates it across nodes via Task.REQUEST_HEADERS. This means compliance events (COMPLIANCE_DOC_WRITE, COMPLIANCE_DOC_READ) on remote shard-hosting nodes also get the correlation ID.

Known limitation: Server-generated UUIDs (when no X-Request-Id header is sent) appear on user-initiated events (AUTHENTICATED, GRANTED_PRIVILEGES for the user action) but NOT on internally-spawned events COMPLIANCE_DOC_WRITE, auto-mapping INDEX_EVENT). This is because internal sub-actions run in a fresh ThreadContext where transients are cleared. When the client provides X-Request-Id, ALL events get the correlation ID because headers survive context changes.

References

• OpenSearch core X-Request-Id support: Add Add X-Request-Id header to uniquely identify a search request OpenSearch#19798

Testing

• Integration tests (AuditRequestIdCorrelationTest):
  • Same request → all events share one ID
  • Client-provided X-Request-Id header used verbatim
  • UUID generated when no header present
  • Different requests → different IDs
  • Compliance doc write gets the request ID (when X-Request-Id sent)
• Unit tests (AuditMessageTest): addRequestId() behavior
• Existing test fix (WhoAmITests): exclude audit_request_id from structural equality comparison

Check List

• New functionality includes testing
• Commits are signed per the DCO using --signoff

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 94d6b7e.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/filter/SecurityRestFilter.java	188	medium	Client-controlled X-Request-Id header is trusted and stored verbatim (after basic sanitization) into audit log correlation fields. An unauthenticated or malicious client can supply an arbitrary string (up to 128 chars, minus control chars) to appear in audit records, enabling audit trail spoofing — e.g., setting an ID matching a known legitimate request to create false correlations or obfuscate a malicious request's audit footprint. No authentication or signature validation is applied to the header value before it is accepted as authoritative.
src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java	95	low	The threadPool field visibility was widened from private to protected, expanding the accessible API surface for any subclass. While the immediate use in AuditLogImpl appears legitimate, this makes the thread context (and any secrets or transient values stored in it) accessible to future or third-party subclasses without explicit intent by the original authors.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 94d6b7e)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Sanitization Order The sanitization logic truncates to 128 chars before stripping control characters. If a client sends a value with control characters within the first 128 chars, the resulting sanitized ID could be shorter than expected, but more importantly, if control chars appear after position 128 they are truncated away (fine), but the truncation may cut a multi-byte sequence. Consider stripping control characters first, then truncating, so the length limit reflects the final stored value. // 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}]", ""); } Empty String Accepted addRequestId only rejects null, so an empty string is stored as the audit request ID (as also asserted by testAddRequestIdEmptyStringIsStored). Since SecurityRestFilter already checks isEmpty() and generates a UUID in that case, this is unlikely to trigger from REST, but a caller passing "" (e.g., via header propagation edge cases) would produce a useless correlation value. Consider rejecting empty/blank values here too. public void addRequestId(String requestId) { if (requestId != null) { auditInfo.put(REQUEST_ID, requestId); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 94d6b7e
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fallback to UUID after sanitization empties ID After sanitizing the client-provided X-Request-Id, the value may become empty (e.g., if the header contained only control characters, or the truncation/strip yields an empty string). In that case, fall back to a generated UUID so the audit correlation field is never empty and cannot be spoofed with a blank value. src/main/java/org/opensearch/security/filter/SecurityRestFilter.java [186-194] } 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}]", ""); + if (requestId.isEmpty()) { + requestId = UUID.randomUUID().toString(); + } } threadContext.putTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID, requestId); Suggestion importance[1-10]: 7 __ Why: Valid edge case: if the client-provided X-Request-Id contains only control characters or becomes empty after sanitization, the correlation ID would be empty. Falling back to a UUID ensures the field is always meaningful.	Medium
	Avoid storing empty request IDs The current implementation stores empty strings, as documented by testAddRequestIdEmptyStringIsStored. Storing an empty audit correlation ID pollutes downstream logs and defeats the purpose of the field. Reject empty/blank values here so the field is either meaningful or absent. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [177-181] public void addRequestId(String requestId) { - if (requestId != null) { + if (requestId != null && !requestId.isEmpty()) { auditInfo.put(REQUEST_ID, requestId); } } Suggestion importance[1-10]: 5 __ Why: Reasonable defensive check to avoid empty correlation IDs, though the caller in SecurityRestFilter already sanitizes input. The suggestion would require updating the existing test testAddRequestIdEmptyStringIsStored.	Low

---

Previous suggestions

Suggestions up to commit d1ac125

Category	Suggestion	Impact
General	Fallback to reading request header directly ThreadContext.getHeader(Task.X_REQUEST_ID) returns the value OpenSearch itself sets from the request; however, if this filter runs before core sets it, the client-supplied X-Request-Id may not be reflected. Read the header directly from the RestRequest (case-insensitively) as a fallback so client-provided trace IDs are always honored, matching the integration test expectations. src/main/java/org/opensearch/security/filter/SecurityRestFilter.java [181-188] // 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(); + List<String> hdrs = request.getHeaders().get(Task.X_REQUEST_ID); + if (hdrs != null && !hdrs.isEmpty() && hdrs.get(0) != null && !hdrs.get(0).isEmpty()) { + requestId = hdrs.get(0); + } else { + requestId = UUID.randomUUID().toString(); + } } threadContext.putTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID, requestId); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a plausible concern about header propagation timing, but ThreadContext.getHeader(Task.X_REQUEST_ID) typically works for client-supplied headers. The improved code adds a reasonable fallback but the necessity is speculative.	Low
	Skip empty request id values Empty strings are stored as-is (as confirmed by testAddRequestIdEmptyStringIsStored), which pollutes audit logs with a useless "audit_request_id":"" field. Skip empty/blank values so the field is only added when meaningful, and update the test accordingly. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [177-181] public void addRequestId(String requestId) { - if (requestId != null) { + if (requestId != null && !requestId.isEmpty()) { auditInfo.put(REQUEST_ID, requestId); } } Suggestion importance[1-10]: 5 __ Why: Skipping empty strings is a reasonable defensive improvement to avoid polluting audit logs with an empty field, though the impact is minor since callers control the input.	Low
	Make UUID pattern assertion less brittle UUID.randomUUID().toString() produces a version 4 UUID that includes uppercase-free hex but the OpenSearch REST client (or intermediaries) may also inject a X-Opaque-Id/X-Request-Id header that does not match this pattern, making this test flaky. Also, the test client may set its own X-Request-Id; assert on shape only after confirming no header was injected, or relax the pattern to [0-9a-fA-F-]+. src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java [112-115] -// Should have a generated UUID (36 chars: 8-4-4-4-12) +// Should have a generated UUID (36 chars: 8-4-4-4-12) or client-injected id 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}")); +assertThat(requestId, matchesPattern("[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}")); Suggestion importance[1-10]: 3 __ Why: UUID.randomUUID().toString() always produces lowercase hex, so the original pattern is correct. The concern about injected headers is speculative and the relaxation offers minor value.	Low

[github-actions]

Persistent review updated to latest commit 94d6b7e

[codecov]

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.40%. Comparing base (283edfc) to head (94d6b7e).

Files with missing lines	Patch %	Lines
...opensearch/security/filter/SecurityRestFilter.java	50.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6343      +/-   ##
==========================================
+ Coverage   75.39%   75.40%   +0.01%     
==========================================
  Files         456      456              
  Lines       29924    29939      +15     
  Branches     4530     4534       +4     
==========================================
+ Hits        22560    22576      +16     
  Misses       5265     5265              
+ Partials     2099     2098       -1     

Files with missing lines	Coverage Δ
...earch/security/auditlog/impl/AbstractAuditLog.java	76.32% <ø> (ø)
...pensearch/security/auditlog/impl/AuditLogImpl.java	92.85% <100.00%> (+0.30%)	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	82.33% <100.00%> (+0.20%)	⬆️
...g/opensearch/security/support/ConfigConstants.java	95.65% <ø> (ø)
...opensearch/security/filter/SecurityRestFilter.java	86.98% <50.00%> (-1.84%)	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.