Add tenant to audit log (#5709)

[quangdutran]

Description

Add tenant info to audit log

• Category (Enhancement)
• Why these changes are required? Informative log
• What is the old behavior before changes and new behavior after changes? Just new field in the audit log

Issues Resolved

#5709

Testing

New test cases cover the verification of the new tenant field are in BasicAuditlogTest

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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 5dbb4d4)

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 Hardcoded header name getTenant(SecurityRequest request) reads the tenant header via the literal string "securitytenant". There is likely a defined constant (e.g., ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER or similar tenant header constant) used elsewhere for consistency. Using a hardcoded literal risks divergence if the header name is ever changed and misses case-insensitive matching semantics used by the rest of the codebase. private String getTenant(SecurityRequest request) { final String fromUser = getTenant(); if (fromUser != null) { return fromUser; } if (request == null) { return null; } return request.header("securitytenant"); } Tenant may be missing on failed login In logFailedLogin, getTenant(request) is called, but on a failed login there is typically no authenticated User in the thread context, so getTenant() returns null and the code falls back to request.header("securitytenant"). The test testTenantFieldOnFailedLogin exercises this path, but ensure the SecurityRequest header lookup is case-insensitive; header names in HTTP are case-insensitive and the incoming BasicHeader("securitytenant", ...) may be stored in different case depending on the underlying request wrapper, which could cause the tenant field to be silently omitted in real deployments. public void logFailedLogin(String effectiveUser, boolean securityadmin, String initiatingUser, SecurityRequest request) { if (!checkRestFilter(AuditCategory.FAILED_LOGIN, effectiveUser, request)) { return; } AuditMessage msg = new AuditMessage(AuditCategory.FAILED_LOGIN, clusterService, getOrigin(), Origin.REST); TransportAddress remoteAddress = getRemoteAddress(); msg.addRemoteAddress(remoteAddress); msg.addRestRequestInfo(request, auditConfigFilter); msg.addInitiatingUser(initiatingUser); msg.addEffectiveUser(effectiveUser); msg.addTenant(getTenant(request)); msg.addIsAdminDn(securityadmin); enrichWithUserContext(msg); save(msg); }

[github-actions]

Persistent review updated to latest commit 18e4b72

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5dbb4d4

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Support alternate tenant header names OpenSearch/Security typically accepts the tenant header case-insensitively and via both securitytenant and security_tenant variants. Using only the literal "securitytenant" lookup here may miss tenants supplied via the alternate/case-different header, leading to inconsistent audit records. Consider centralizing this lookup or checking both header names case-insensitively. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1296-1305] private String getTenant(SecurityRequest request) { final String fromUser = getTenant(); if (fromUser != null) { return fromUser; } if (request == null) { return null; } - return request.header("securitytenant"); + String tenant = request.header("securitytenant"); + if (tenant == null) { + tenant = request.header("security_tenant"); + } + return tenant; } Suggestion importance[1-10]: 5 __ Why: Valid concern regarding header name variants, but the suggestion is speculative without verification that OpenSearch Security actually supports security_tenant as an alternate header name.	Low
	Order tenant assignment consistently before enrichment The addTenant call is placed after enrichWithUserContext(msg) here, unlike the other REST methods in this PR where it is placed before enrichment. While functionally equivalent for setting the tenant, it is inconsistent with the pattern used in logFailedLogin, logSucceededLogin, and logMissingPrivileges. Move msg.addTenant(...) before enrichWithUserContext(msg) for consistency and to avoid future maintenance surprises if enrichment ever depends on the tenant field. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [285] AuditMessage msg = new AuditMessage(AuditCategory.GRANTED_PRIVILEGES, clusterService, getOrigin(), Origin.REST); msg.addRemoteAddress(getRemoteAddress()); msg.addRestRequestInfo(request, auditConfigFilter); msg.addEffectiveUser(effectiveUser); +msg.addTenant(getTenant(request)); enrichWithUserContext(msg); -msg.addTenant(getTenant(request)); save(msg); Suggestion importance[1-10]: 3 __ Why: Minor stylistic consistency improvement; functionally equivalent since enrichWithUserContext does not appear to depend on the tenant field.	Low

---

Previous suggestions

Suggestions up to commit 279670d

Category	Suggestion	Impact
General	Handle empty tenant fallback consistently When the user object exists but has no requested tenant, getTenant() returns null and the code correctly falls back to the header. However, if the user's requested tenant is an empty string (e.g., explicit global tenant), the fallback is skipped and an empty value is used. Consider treating empty as null to make the fallback consistent, or explicitly document the semantics. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1296-1305] private String getTenant(SecurityRequest request) { final String fromUser = getTenant(); - if (fromUser != null) { + if (fromUser != null && !fromUser.isEmpty()) { return fromUser; } if (request == null) { return null; } return request.header("securitytenant"); } Suggestion importance[1-10]: 5 __ Why: Valid observation: when the user's requested tenant is an empty string, the header fallback is skipped. Treating empty as null makes the fallback consistent, though impact is minor.	Low
	Align tenant addition ordering across methods In logGrantedPrivileges, addTenant is placed after enrichWithUserContext and just before save, unlike the other REST methods which add tenant before enrichment. This inconsistency doesn't affect the message content but should be aligned with the other methods for maintainability and to avoid future ordering-dependent bugs. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [280-286] AuditMessage msg = new AuditMessage(AuditCategory.GRANTED_PRIVILEGES, clusterService, getOrigin(), Origin.REST); msg.addRemoteAddress(getRemoteAddress()); msg.addRestRequestInfo(request, auditConfigFilter); msg.addEffectiveUser(effectiveUser); +msg.addTenant(getTenant(request)); enrichWithUserContext(msg); -msg.addTenant(getTenant(request)); save(msg); Suggestion importance[1-10]: 3 __ Why: A minor consistency/maintainability improvement. It does not affect functionality since the ordering of addTenant vs enrichWithUserContext doesn't change message content.	Low

Suggestions up to commit 18e4b72

Category	Suggestion	Impact
General	Treat empty tenant as missing for fallback getTenant() returns null when the user has no requested tenant, but may also return an empty string from User.getRequestedTenant(). The current null-only check would cause an empty tenant from the user to mask a valid securitytenant header. Treat empty tenant the same as null to fall back to the request header. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1113-1122] private String getTenant(SecurityRequest request) { final String fromUser = getTenant(); - if (fromUser != null) { + if (fromUser != null && !fromUser.isEmpty()) { return fromUser; } if (request == null) { return null; } return request.header("securitytenant"); } Suggestion importance[1-10]: 6 __ Why: Valid concern: User.getRequestedTenant() could return an empty string, which would prevent fallback to the securitytenant header. The fix is small but improves correctness of tenant resolution.	Low
	Deduplicate user retrieval logic getTenant() and getUser() duplicate the logic to resolve the User from the thread context. Extract a helper method to avoid divergence and double deserialization when both are called for the same audit message. src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java [1103-1111] -private String getTenant() { +private User getCurrentUser() { 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) ); } + return user; +} + +private String getTenant() { + User user = getCurrentUser(); return user == null ? null : user.getRequestedTenant(); } Suggestion importance[1-10]: 4 __ Why: Reasonable refactor to reduce duplication between getUser() and getTenant(), but it's a minor maintainability improvement with no functional impact.	Low

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.07%. Comparing base (0a85c75) to head (18e4b72).
⚠️ Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6181      +/-   ##
==========================================
+ Coverage   75.03%   75.07%   +0.04%     
==========================================
  Files         451      451              
  Lines       29249    29280      +31     
  Branches     4407     4411       +4     
==========================================
+ Hits        21946    21981      +35     
+ Misses       5264     5261       -3     
+ Partials     2039     2038       -1     

Files with missing lines	Coverage Δ
...search/security/auditlog/impl/RequestResolver.java	79.34% <100.00%> (+0.11%)	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	81.57% <66.66%> (-0.18%)	⬇️
...earch/security/auditlog/impl/AbstractAuditLog.java	77.09% <89.28%> (+0.56%)	⬆️

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

[cwperks]

@quangdutran I'm not opposed to this, but IMO I'd like to see a separate audit log category in general for anything dashboards related. For simplicity, that may mean any request to the .kibana alias, but for real completeness sake it would need to also be extended to plugin indices like the reporting plugin if those plugins have any tenant specific logic.

Requests to the .kibana alias shouldn't be too bad, but extending this beyond will be a challenge.

[DarshitChanpura]

@quangdutran Are you actively working on this? If so can you please address the comment above and the conflicts along with CodHygiene (./gradlew spotlessApply)

[quangdutran]

@quangdutran Are you actively working on this? If so can you please address the comment above and the conflicts along with CodHygiene (./gradlew spotlessApply)

sure, I will check this

[quangdutran]

@quangdutran I'm not opposed to this, but IMO I'd like to see a separate audit log category in general for anything dashboards related. For simplicity, that may mean any request to the .kibana alias, but for real completeness sake it would need to also be extended to plugin indices like the reporting plugin if those plugins have any tenant specific logic.

Requests to the .kibana alias shouldn't be too bad, but extending this beyond will be a challenge.

@cwperks You mean the tenant should be in a separated log category, something like DASHBOARD_REQUEST, targeting .kibaba requests right?
Will that address cases which tenant decision is made (authenticated category/ tenant switch)? Could you pls elaborate?

[cwperks]

@quangdutran Can you fix the conflicts on this PR?

I mean, in general OpenSearch Dashboards sends securitytenant header on all requests to the backend and not necessarily just on requests that pertain to getting tenant-specific resources like Dashboards and Visualizations. We should certainly add this field to the audit log, but I wanted to point out the larger problem of how to identify when a user is access a tenant-specific resource. Not to mention that supporting tenant-segregated resources is only support for OSD Saved Objects + the reporting plugin and not extended to other plugins. Workspaces have the same problem.

[github-actions]

Persistent review updated to latest commit 279670d

[github-actions]

Persistent review updated to latest commit 5dbb4d4