Allow audit config PUT when document does not exist

[terryquigleysas]

Description

• Category (Bug fix)
• Why these changes are required?

Previously, all Audit Log REST API operations (GET, PUT, PATCH) were blocked when no audit.yml was deployed at initial cluster creation. This created a chicken-and-egg problem where operators could not bootstrap the audit configuration via the API.

PUT now uses a new withCreateOrUpdateAuditApi() validation that allows bootstrapping when the document is absent and normal updates when it exists. GET and PATCH remain gated behind the existing check.

• What is the old behavior before changes and new behavior after changes?

If audit.yml is not deployed at initial cluster creation then none of the Audit Log REST API calls (GET,PUT,PATCH) will work.

Issues Resolved

Resolves #5862

Testing

New supporting tests added.

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 85cdad3)

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 Possible Issue The withCreateOrUpdateAuditApi method allows PUT requests to proceed when isAuditHotReloadingEnabled() returns false (document does not exist), but it does not verify that the request method is actually PUT. If this method is called with a non-PUT request when the document does not exist, it will incorrectly allow the operation to proceed. While the current code only calls this method for PUT requests (line 270), the method itself does not enforce this constraint, creating a potential misuse risk if the code is refactored. ValidationResult<RestRequest> withCreateOrUpdateAuditApi(final RestRequest request) { if (!securityApiDependencies.configurationRepository().isAuditHotReloadingEnabled()) { // Document does not exist — allow PUT to bootstrap the audit config return ValidationResult.success(request); } return withEnabledAuditApi(request); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 85cdad3
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Verify bootstrap condition logic The logic appears inverted. When isAuditHotReloadingEnabled() returns false, it typically means the feature is disabled, not that the document doesn't exist. Verify that this condition correctly identifies the bootstrap case versus the update case, as the current implementation may allow PUT operations when audit is disabled rather than when the document is missing. src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [280-286] ValidationResult<RestRequest> withCreateOrUpdateAuditApi(final RestRequest request) { + // Verify this condition correctly identifies document existence + // Consider using a dedicated method like documentExists() instead if (!securityApiDependencies.configurationRepository().isAuditHotReloadingEnabled()) { // Document does not exist — allow PUT to bootstrap the audit config return ValidationResult.success(request); } return withEnabledAuditApi(request); } Suggestion importance[1-10]: 7 __ Why: The suggestion raises a valid concern about the semantic meaning of isAuditHotReloadingEnabled(). The method name suggests it checks if hot reloading is enabled, but the code uses it to determine if the document exists. However, the 'improved_code' only adds a comment without actually changing the logic, and the suggestion asks for verification rather than providing a concrete fix.	Medium

---

Previous suggestions

Suggestions up to commit 6c61bfc

Category	Suggestion	Impact
Possible issue	Verify audit config existence logic The logic appears inverted. When isAuditHotReloadingEnabled() returns false, it typically means audit is disabled or the document doesn't exist. However, the method name suggests it should handle both create (when document doesn't exist) and update (when it does exist) cases. Consider verifying if the condition should check for document existence rather than hot reloading status. src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [280-286] ValidationResult<RestRequest> withCreateOrUpdateAuditApi(final RestRequest request) { if (!securityApiDependencies.configurationRepository().isAuditHotReloadingEnabled()) { // Document does not exist — allow PUT to bootstrap the audit config return ValidationResult.success(request); } + // Document exists — validate through normal audit API checks return withEnabledAuditApi(request); } Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify the logic but doesn't identify an actual bug. The test cases in the PR show the logic is intentional: when isAuditHotReloadingEnabled() is false (document doesn't exist), PUT is allowed for bootstrapping. The 'improved_code' only adds a comment without changing functionality, making this a low-impact verification request.	Low

[terryquigleysas]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns

Authorization bypass: The new withCreateOrUpdateAuditApi() method allows PUT requests to proceed when isAuditHotReloadingEnabled() returns false, which was previously blocked. This creates a potential authorization bypass where audit configuration can be modified even when audit hot reloading is disabled. An attacker or misconfigured client could exploit this to bootstrap or modify audit settings in environments where such changes should be restricted. The code assumes that a missing document always means bootstrapping is safe, but this may not align with the security policy enforced by the isAuditHotReloadingEnabled() check.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Security Concern
Allowing PUT when isAuditHotReloadingEnabled() returns false bypasses the intended validation. If the audit config document does not exist, PUT will succeed without verifying that audit hot reloading is actually enabled. This could allow unauthorized or unintended audit configuration changes in environments where audit hot reloading should be disabled. The bootstrap scenario should verify that the system is in a state where bootstrapping is appropriate, not just that the document is absent.

ValidationResult<RestRequest> withCreateOrUpdateAuditApi(final RestRequest request) {
    if (!securityApiDependencies.configurationRepository().isAuditHotReloadingEnabled()) {
        // Document does not exist — allow PUT to bootstrap the audit config
        return ValidationResult.success(request);
    }
    return withEnabledAuditApi(request);
}

The isAuditHotReloadingEnabled() check was never an authorization gate. It's a feature availability gate — "does the audit config doc exist in the index?" The actual authentication and
authorization (credentials, admin privileges) are enforced upstream in the request handler framework via verifyAccessForAllMethods() and the accessHandler predicate. A caller must already
have security API admin privileges to even reach withCreateOrUpdateAuditApi(). Our fix doesn't bypass any auth — it removes a chicken-and-egg feature gate

[terryquigleysas]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category **Suggestion                                                                                                                                    ** Impact
Possible issue
Verify audit config existence logic
The logic appears inverted. When isAuditHotReloadingEnabled() returns false, it typically means audit is disabled or the document doesn't exist. However, the method name suggests it should handle both create (when document doesn't exist) and update (when it does exist) cases. Consider verifying if the condition should check for document existence rather than hot reloading status.

src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java [280-286]

 ValidationResult<RestRequest> withCreateOrUpdateAuditApi(final RestRequest request) {
     if (!securityApiDependencies.configurationRepository().isAuditHotReloadingEnabled()) {
         // Document does not exist — allow PUT to bootstrap the audit config
         return ValidationResult.success(request);
     }
+    // Document exists — validate through normal audit API checks
     return withEnabledAuditApi(request);
 }

Suggestion importance[1-10]: 3
__

Why: The suggestion asks to verify the logic but doesn't identify an actual bug. The test cases in the PR show the logic is intentional: when isAuditHotReloadingEnabled() is false (document doesn't exist), PUT is allowed for bootstrapping. The 'improved_code' only adds a comment without changing functionality, making this a low-impact verification request.

Low

A note on what isAuditHotReloadingEnabled() checks. It returns false specifically when the audit config document doesn't exist (via ConfigurationLoaderSecurity7.noData() → isAuditConfigDocPresentInIndex.set(false)). There is no separate "document existence" check to use — this is the document-existence check. The method name is somewhat misleading, but the semantics appear to be correct.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.02%. Comparing base (ffbfcae) to head (6c61bfc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6158      +/-   ##
==========================================
+ Coverage   74.98%   75.02%   +0.03%     
==========================================
  Files         452      452              
  Lines       29108    29111       +3     
  Branches     4382     4382              
==========================================
+ Hits        21827    21840      +13     
+ Misses       5253     5242      -11     
- Partials     2028     2029       +1     

Files with missing lines	Coverage Δ
...nsearch/security/dlic/rest/api/AuditApiAction.java	94.28% <100.00%> (+3.24%)	⬆️

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

[github-actions]

Persistent review updated to latest commit 85cdad3

[cwperks]

Is it worth getting rid of withEnabledAuditApi entirely?

I see that there's this block which checks for doc existing in .opendistro_security.

I agree that hot reload being disabled should be intentional and not proxied by checking for the auditlog entry in the security index.

[terryquigleysas]

@cwperks As discussed in the SIG call, this may require more work. I will need to park it for now and revisit at a later time as I will be out of the office again for an extended period.