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 🔍

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 preserved across auth-domain iterations when a later domain extracts no credentials. If an earlier domain rejects real credentials and a later non-SAML challenging domain (e.g. Basic) also fires with no new credentials, the FAILED_LOGIN will be logged with the earlier rejected username rather than <NONE>. More importantly, notifyIpAuthFailureListeners is now called with the stale credentials from a prior domain instead of null, which may affect IP-based failure tracking/blocking behavior. Consider whether the preservation should be scoped only to the audit-log decision. if (ac != null) { authCredentials = ac; } if (ac == null) { // no credentials found in request if (!gRPC && anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) { continue; } if (authDomain.isChallenge()) { final Optional<SecurityResponse> restResponse = httpAuthenticator.reRequestAuthentication(request, null); if (restResponse.isPresent()) { final String authenticatorType = authDomain.getHttpAuthenticator().getType(); // saml will always hit this to re-request authentication if (!authenticatorType.equals(SAML_TYPE) || authCredentials != null) { auditLog.logFailedLogin( authCredentials == null ? "<NONE>" : authCredentials.getUsername(), false, null, request ); } if (authenticatorType.equals(BASIC_TYPE)) { log.warn("No 'Authorization' header, send 401 and 'WWW-Authenticate Basic'"); } notifyIpAuthFailureListeners(request, authCredentials);

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

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.