fix: emit FAILED_LOGIN audit event when Basic Auth rejects credentials in multi-auth (SAML + Basic) setup

[mvanhorn]

Description

• Category: Bug fix
• Why these changes are required? In the documented multi-auth configuration (Basic Auth with challenge: false at order 1, SAML with challenge: true later), a failed Basic Auth login through the Dashboards login form produced no FAILED_LOGIN audit event, so brute-force attempts were invisible to audit logs. The suppression added by Remove failed login attempt for saml authenticator #4762 (to silence false positives during normal SAML redirects) also fires when Basic Auth has already rejected real credentials, because the loop overwrites the previously extracted credentials with null in the SAML iteration and then returns from the suppressed branch without logging.
• What is the old behavior before changes and new behavior after changes? Before: rejected Basic Auth credentials in a multi-auth (SAML + Basic) setup logged nothing. After: BackendRegistry.authenticate preserves the last extracted credentials across auth-domain iterations (authCredentials is only overwritten when the new extraction is non-null), and the Remove failed login attempt for saml authenticator #4762 suppression is narrowed to apply only when no earlier domain rejected real credentials. A genuine failed Basic login now emits FAILED_LOGIN with the rejected username; an unauthenticated browser hitting the SAML redirect still logs nothing, preserving the [BUG] Audit Log publish Incorrect FAILED_LOGIN event for Successful login attempt by SAML user on SAML enabled Domain #4608 fix.

Issues Resolved

Fixes #6221

Is this a backport? No.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? No.

Testing

New BackendRegistryTest follows the existing BackendRegistryGrpcAuthTest pattern (mocked AdminDNs/XFFResolver/AuditLog/ThreadPool/ClusterInfoHolder, auth domains injected via a mocked DynamicConfigModel). Covers: rejected Basic credentials followed by a SAML challenge emits FAILED_LOGIN with the rejected username; a bare SAML redirect with no extracted credentials stays silent (the #4608 behavior); single-domain Basic rejection still logs as before.

Check List

• New functionality includes testing
• New functionality has been documented - N/A, no user-facing config or API change; behavior matches the documented audit expectations
• New Roles/Permissions have a corresponding security dashboards plugin PR - N/A, no new roles/permissions
• API changes companion pull request created - N/A, no API change
• 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 ec4bc9a)

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 Stale credentials across domains authCredentials is now only overwritten when the new extraction is non-null. This preserves credentials from an earlier rejecting domain into a later domain's challenge branch (the intended fix), but it also means that if a later auth domain extracts credentials, fails to authenticate, and the challenge is issued by a still-later domain, the audited username may come from an earlier domain rather than the one whose credentials were actually rejected. Verify whether this cross-domain leakage matters for any chain other than the tested Basic+SAML case, since authCredentials is also passed to notifyIpAuthFailureListeners and could attribute the failure to the wrong username. if (ac != null) { authCredentials = ac; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ec4bc9a
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid duplicate failed-login audit entries The authCredentials variable can retain credentials from a previous auth domain iteration that were already audited/rejected. If a later challenging domain (non-SAML) then triggers the re-request path without its own credentials, this will log the previous domain's username as failed a second time, producing duplicate audit entries. Consider distinguishing credentials extracted from the current domain (ac) from those retained across iterations, and only use retained credentials for the SAML challenge path. src/main/java/org/opensearch/security/auth/BackendRegistry.java [394-401] -if (!authenticatorType.equals(SAML_TYPE) || authCredentials != null) { - auditLog.logFailedLogin( - authCredentials == null ? "<NONE>" : authCredentials.getUsername(), - false, - null, - request - ); +if (!authenticatorType.equals(SAML_TYPE)) { + auditLog.logFailedLogin("<NONE>", false, null, request); +} else if (authCredentials != null) { + auditLog.logFailedLogin(authCredentials.getUsername(), false, null, request); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about potentially duplicate audit entries when authCredentials is retained from a previous auth domain and a subsequent non-SAML challenging domain triggers the re-request path. However, the improved code doesn't fully address the concern (it still uses the retained authCredentials implicitly) and the scenario may be rare in practice.	Low

---

Previous suggestions

Suggestions up to commit b0c41f1

Category	Suggestion	Impact
Possible issue	Avoid stale credentials leaking into SAML audit When iterating multiple auth domains, authCredentials may carry over credentials from a previously evaluated domain (e.g., Basic) that are unrelated to the current SAML challenge. Guarding on authCredentials != null alone can cause a false FAILED_LOGIN audit for SAML redirects that occur after a prior domain populated credentials. Consider also checking that the credentials belong to the current authenticator or resetting authCredentials between domains when appropriate. src/main/java/org/opensearch/security/auth/BackendRegistry.java [389-396] -if (!authenticatorType.equals(SAML_TYPE) || authCredentials != null) { +if (!authenticatorType.equals(SAML_TYPE) || (authCredentials != null && ac != null)) { auditLog.logFailedLogin( authCredentials == null ? "<NONE>" : authCredentials.getUsername(), false, null, request ); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about authCredentials potentially carrying over from a previous auth domain in the loop, which could lead to a false FAILED_LOGIN audit entry for a SAML redirect. However, the improved_code (adding && ac != null) essentially reverts to the original SAML-only behavior for this iteration, which may not correctly capture the intended scenario in the PR (logging failed Basic auth before SAML challenge). The concern has merit but the proposed fix is questionable.	Low

[cwperks]

@mvanhorn Can you run ./gradlew spotlessApply a commit the changes?

[cwperks]

I see why this is being done since extractCredentials from HTTPSamlAuthenticator will return null when doing basic auth, but it does feel hacky. I think this really illustrates that the reRequestAuthentication flow is not structured well. Because of the design, we require challenge: true for authenticators like the SAML authenticator when challenge really only makes sense for HTTP Basic auth.

Can we add a comment in here explaining the rationale?

[DarshitChanpura]

+1.

[DarshitChanpura]

thank you @mvanhorn for this improvment!

[DarshitChanpura]

since authCredentials now retains the rejected username across iterations, notifyIpAuthFailureListeners call here will also receive the actual username instead of null when firing from the SAML challenge path which may end up improving failed login tracking by attributing user (if available) to an IP.

[DarshitChanpura]

+1.

[DarshitChanpura]

@mvanhorn Are you actively working on this PR? If not, it will be closed as stale during next cleanup.

[github-actions]

Persistent review updated to latest commit ec4bc9a

[mvanhorn]

@DarshitChanpura yes, still working on it. Sorry for the gap.

@cwperks added the rationale comment you asked for in ec4bc9a, at the credential-retention site. I kept it to describing why the current flow forces this rather than claiming to have restructured reRequestAuthentication; I agree with you that the underlying design is awkward, but that felt like a separate change from this one.

I also went through the three red checks, since only one of them is mine:

• Spotless scan was ours: an import-order violation in BackendRegistryTest.java. Fixed in the same commit. ./gradlew spotlessCheck now exits 0 locally.
• code-ql is failing on Loaded a configuration file for version '4.36.3', but running version '4.36.2', a CodeQL Action tooling mismatch in the runner rather than a finding against this code.
• integration-tests (25, windows-latest) dies on BindTransportException: Failed to bind to [::1]:47300; Address already in use. The SSL handshake errors after it are downstream of the failed bind. It is the only failing leg in the matrix, and CI on main is red on five of its last six runs, so I do not think either of these two is attributable to this PR.

./gradlew test --tests "*BackendRegistryTest*" passes locally. @DarshitChanpura on your line 400 note, agreed, that is the intended effect: retaining the rejected username means notifyIpAuthFailureListeners can attribute the failure to a user on the SAML challenge path instead of firing with null.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.67%. Comparing base (a499676) to head (ec4bc9a).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6285      +/-   ##
==========================================
+ Coverage   75.01%   75.67%   +0.66%     
==========================================
  Files         451      456       +5     
  Lines       29248    30417    +1169     
  Branches     4407     4607     +200     
==========================================
+ Hits        21940    23019    +1079     
- Misses       5266     5277      +11     
- Partials     2042     2121      +79     

Files with missing lines	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	80.60% <100.00%> (+0.54%)	⬆️

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