feat(audit): Add user_agent, user_roles, and auth_method to audit events

[Taiwo435]

Description

Enrich FGAC audit events with three fields that improve investigability:

• audit_request_user_agent: User-Agent string from REST headers
• audit_request_user_roles: mapped security roles from the authenticated user
• audit_request_auth_method: authentication backend that validated the user (e.g., internal, ldap)

Why

Current audit events identify WHO (effective_user) and WHAT (action, indices) but not HOW they authenticated or what role granted them access. Incident investigators need these fields to answer: "Was this a service account or human?", "Which role allowed this?", "What client tool was used?"

How it works

• Added enrichWithUserContext() helper to AbstractAuditLog
• Called from all FGAC audit methods: logFailedLogin, logSucceededLogin, logMissingPrivileges, logGrantedPrivileges, logIndexEvent, logClusterSettingsChange, logIndexSettingsChange, logSecurityIndexAttempt
• Graceful degradation: fields are absent (not null/empty) when User object is unavailable (e.g., failed login before auth completes, admin cert bypass)

Testing

• Integration test: AuditFieldEnrichmentTest covering:
  • user_agent present on REST events
  • user_roles present after successful auth
  • auth_method present after successful auth
  • Fields absent on failed login (graceful degradation)
  • Fields absent on admin cert bypass

Check List

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d9fe7ca)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: addUserRoles writes the full set of security role names into audit log entries (stored via auditInfo.put(USER_ROLES, Set.copyOf(roles))). Audit logs are often forwarded to external systems (SIEM, object storage, log aggregators) that may be accessible to operators who should not have visibility into role assignments. There is no configuration flag to disable this field. Depending on the deployment's access-control model for audit log consumers, this could expose privilege topology to unintended readers.

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add USER_AGENT, USER_ROLES, AUTH_METHOD constants and addXxx methods to AuditMessage Relevant files: src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java Sub-PR theme: Wire enrichWithUserContext into audit log methods and add integration tests Relevant files: src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java src/integrationTest/java/org/opensearch/security/AuditFieldEnrichmentTest.java

⚡ Recommended focus areas for review Security Role Exposure enrichWithUserContext writes the user's security roles into every audit event. If audit logs are shipped to a less-privileged store (e.g., an external SIEM or S3 bucket readable by non-admins), the full role list becomes visible to anyone who can read audit logs. This is a deliberate design choice per the PR description, but it may violate least-privilege expectations in deployments where audit log readers should not know role assignments. Worth an explicit call-out in documentation or a config flag to opt in/out. private void enrichWithUserContext(AuditMessage msg) { User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); if (user == null && threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) != null) { user = this.userFactory.fromSerializedBase64( threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) ); } if (user == null) { return; } msg.addUserRoles(user.getSecurityRoles()); msg.addAuthMethod(user.getAuthenticatedBy()); } Duplicate User Lookup enrichWithUserContext duplicates the exact same ThreadContext lookup logic already present in getUser(). Both methods independently call getTransient and fall back to fromSerializedBase64. This means every audit event that calls enrichWithUserContext deserializes the user twice (once via getUser() for addEffectiveUser, once here). Consider extracting a shared resolveUser() helper to avoid the redundant deserialization. private void enrichWithUserContext(AuditMessage msg) { User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); if (user == null && threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) != null) { user = this.userFactory.fromSerializedBase64( threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) ); } if (user == null) { return; } msg.addUserRoles(user.getSecurityRoles()); msg.addAuthMethod(user.getAuthenticatedBy()); } Flawed Failed-Login Assertion shouldNotFailOnFailedLogin asserts that user_agent != null && roles == null && authMethod == null. However, enrichWithUserContext is now called unconditionally in logFailedLogin (line 196 of AbstractAuditLog). If a stale User object happens to be present in ThreadContext from a prior request on the same thread (thread pool reuse), roles and authMethod could be non-null, causing the test to fail spuriously. More importantly, the production code calling enrichWithUserContext on a failed login contradicts the PR description's "graceful degradation" intent — the test may pass only because the test thread happens to have no prior user in context. public void shouldNotFailOnFailedLogin() { // Use wrong credentials — auth fails, User object won't be in ThreadContext try (TestRestClient client = cluster.getRestClient("nonexistent", "wrongpassword")) { client.get("_cluster/health"); } // FAILED_LOGIN event should have user_agent (from REST headers) but NOT roles/auth (no User object) auditLogsRule.assertAtLeast(1, (AuditMessage msg) -> { if (msg.getCategory() != AuditCategory.FAILED_LOGIN) return false; Map<String, Object> fields = msg.getAsMap(); // user_agent comes from headers — available even on failed login Object userAgent = fields.get(AuditMessage.USER_AGENT); // roles and auth_method come from User object — NOT available on failed login Object roles = fields.get(AuditMessage.USER_ROLES); Object authMethod = fields.get(AuditMessage.AUTH_METHOD); return userAgent != null && roles == null && authMethod == null; }); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d9fe7ca

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make roles field null-safe and deterministic Set.copyOf throws NPE if any element in roles is null. Additionally, using an unordered Set makes audit log entries non-deterministic, which complicates log processing and testing. Convert to a sorted immutable list to ensure stable, serializable output. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [499-503] public void addUserRoles(Set<String> roles) { if (roles != null && !roles.isEmpty()) { - auditInfo.put(USER_ROLES, Set.copyOf(roles)); + auditInfo.put(USER_ROLES, roles.stream().filter(Objects::nonNull).sorted().collect(java.util.stream.Collectors.toUnmodifiableList())); } } Suggestion importance[1-10]: 4 __ Why: Changing the type from Set to List alters the serialization format and may affect consumers; the NPE concern for null elements in getSecurityRoles() is unlikely in practice. Deterministic ordering is a minor readability/testability benefit.	Low
	Guard against null user-agent header value The first entry returned by findFirst() on a stream over a HashMap's entry set is non-deterministic, but more importantly userAgentValues.get(0) can be null if the header value list contains a null element, which would then silently pass through. Guard against a null first element to avoid inserting a null-effective value. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [418-429] Map<String, List<String>> headers = request.getHeaders(); if (headers != null) { - List<String> userAgentValues = headers.entrySet() + headers.entrySet() .stream() .filter(e -> "user-agent".equalsIgnoreCase(e.getKey())) .map(Map.Entry::getValue) + .filter(v -> v != null && !v.isEmpty() && v.get(0) != null) .findFirst() - .orElse(null); - if (userAgentValues != null && !userAgentValues.isEmpty()) { - addUserAgent(userAgentValues.get(0)); - } + .ifPresent(v -> addUserAgent(v.get(0))); } Suggestion importance[1-10]: 3 __ Why: The addUserAgent method already null-checks the input, so a null first element would be safely ignored. The change is a minor defensive improvement.	Low

---

Previous suggestions

Suggestions up to commit 82db9ad

Category	Suggestion	Impact
General	Store roles as sorted stable collection Set.copyOf returns an unordered immutable set, which can make audit output ordering non-deterministic and harder to diff/test. Consider storing a sorted, immutable collection (e.g., a sorted List) so the roles field is stable across events and serializers. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [499-503] public void addUserRoles(Set<String> roles) { if (roles != null && !roles.isEmpty()) { - auditInfo.put(USER_ROLES, Set.copyOf(roles)); + auditInfo.put(USER_ROLES, roles.stream().sorted().collect(java.util.stream.Collectors.toUnmodifiableList())); } } Suggestion importance[1-10]: 5 __ Why: Sorting roles for deterministic audit output improves testability and diffability, a reasonable minor enhancement but not critical.	Low
	Guard against null user-agent value Guard against a null first element in the user-agent header list. If a client sends a header entry whose value list starts with null, userAgentValues.get(0) will pass a null to addUserAgent, which then still calls isEmpty() safely, but it's clearer and safer to also filter null/empty at extraction time. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [420-428] List<String> userAgentValues = headers.entrySet() .stream() .filter(e -> "user-agent".equalsIgnoreCase(e.getKey())) .map(Map.Entry::getValue) .findFirst() .orElse(null); -if (userAgentValues != null && !userAgentValues.isEmpty()) { +if (userAgentValues != null && !userAgentValues.isEmpty() && userAgentValues.get(0) != null) { addUserAgent(userAgentValues.get(0)); } Suggestion importance[1-10]: 3 __ Why: The extra null check is defensive but the existing addUserAgent already handles null safely, so impact is minimal.	Low

Suggestions up to commit 9ac7d7f

Category	Suggestion	Impact
Possible issue	Fallback to serialized user header for enrichment Unlike getUser(), this method does not fall back to reconstructing the User from the OPENDISTRO_SECURITY_USER_HEADER when the transient value is absent. On transport-hop audit events (where the User arrives via serialized header), roles and auth_method will be silently missing. Mirror the fallback logic used in getUser() for consistent enrichment. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1137-1144] private void enrichWithUserContext(AuditMessage msg) { User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + if (user == null && threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) != null) { + user = this.userFactory.fromSerializedBase64( + threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) + ); + } if (user == null) { return; } msg.addUserRoles(user.getSecurityRoles()); msg.addAuthMethod(user.getAuthenticatedBy()); } Suggestion importance[1-10]: 7 __ Why: Valid concern — mirroring the getUser() fallback ensures enrichment works consistently on transport hops where the User is serialized in a header, avoiding missing roles/auth_method fields.	Medium
General	Respect header exclusion filter for user-agent The User-Agent header value could be logged before header filtering/exclusion is applied, potentially leaking a header the operator configured to exclude via shouldExcludeSensitiveHeaders. Respect the filter's excluded headers setting before adding user_agent so this field does not bypass the audit exclusion configuration. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [418-429] -Map<String, List<String>> headers = request.getHeaders(); -if (headers != null) { - List<String> userAgentValues = headers.entrySet() - .stream() - .filter(e -> "user-agent".equalsIgnoreCase(e.getKey())) - .map(Map.Entry::getValue) - .findFirst() - .orElse(null); - if (userAgentValues != null && !userAgentValues.isEmpty()) { - addUserAgent(userAgentValues.get(0)); +if (!filter.shouldExcludeSensitiveHeaders() || !isSensitiveHeader("user-agent", filter)) { + Map<String, List<String>> headers = request.getHeaders(); + if (headers != null) { + headers.entrySet() + .stream() + .filter(e -> "user-agent".equalsIgnoreCase(e.getKey())) + .map(Map.Entry::getValue) + .filter(v -> v != null && !v.isEmpty()) + .findFirst() + .ifPresent(v -> addUserAgent(v.get(0))); } } Suggestion importance[1-10]: 5 __ Why: User-Agent is typically not considered a sensitive header (unlike Authorization), but respecting the filter's exclusion policy for consistency is reasonable. The suggested isSensitiveHeader helper is not defined, so the improved code is not directly usable.	Low
	Store an immutable copy of roles Storing the raw Set reference from user.getSecurityRoles() in auditInfo couples the audit message to the live user object; if the underlying set is mutable or later mutated, the audit output could change unexpectedly. Store an immutable copy (e.g., sorted list) to guarantee a stable serialized value and deterministic ordering. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [499-503] public void addUserRoles(Set<String> roles) { if (roles != null && !roles.isEmpty()) { - auditInfo.put(USER_ROLES, roles); + auditInfo.put(USER_ROLES, Collections.unmodifiableList(new ArrayList<>(new java.util.TreeSet<>(roles)))); } } Suggestion importance[1-10]: 4 __ Why: Making a defensive copy improves safety and determinism of the audit output, but the risk of live mutation is low in practice. A minor robustness improvement.	Low

[github-actions]

Persistent review updated to latest commit 82db9ad

[github-actions]

Persistent review updated to latest commit d9fe7ca

[codecov]

Codecov Report

❌ Patch coverage is 80.48780% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.33%. Comparing base (283edfc) to head (d9fe7ca).

Files with missing lines	Patch %	Lines
...pensearch/security/auditlog/impl/AuditMessage.java	73.68%	0 Missing and 5 partials ⚠️
...earch/security/auditlog/impl/AbstractAuditLog.java	86.36%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6344      +/-   ##
==========================================
- Coverage   75.39%   75.33%   -0.06%     
==========================================
  Files         456      456              
  Lines       29924    29964      +40     
  Branches     4530     4534       +4     
==========================================
+ Hits        22560    22574      +14     
- Misses       5265     5286      +21     
- Partials     2099     2104       +5     

Files with missing lines	Coverage Δ
...earch/security/auditlog/impl/AbstractAuditLog.java	76.63% <86.36%> (+0.30%)	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	81.56% <73.68%> (-0.57%)	⬇️

... and 8 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.