Skip to content

fix(sso): bind SAML responses to the AuthnRequest and reject replays - #3214

Open
marevol wants to merge 1 commit into
masterfrom
saml-response-validation
Open

fix(sso): bind SAML responses to the AuthnRequest and reject replays#3214
marevol wants to merge 1 commit into
masterfrom
saml-response-validation

Conversation

@marevol

@marevol marevol commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

The SAML ACS callback (SamlAuthenticator.getLoginCredential()) accepted any valid assertion and reported nothing when one was rejected. This PR closes both gaps.

1. SAML_STATE never bound the response to a request

SAML_STATE held a freshly generated UUID. auth.login(null, ...) discards auth.getLastRequestId() and java-saml puts the current URL in RelayState when the first argument is null, so the UUID was never transmitted to the IdP and never compared with anything — it only signalled "a SAML login is in progress".

The callback then called the no-arg auth.processResponse(), which passes requestId = null, short-circuiting the InResponseTo check in SamlResponse#isValid:

if (requestId != null && !Objects.equals(responseInResponseTo, requestId)) { ... }

SAML_STATE now holds auth.getLastRequestId() and is passed to processResponse(requestId), so the response is checked against the AuthnRequest this SP actually issued.

This is also what the other SSO providers already do — OpenIdConnectAuthenticator compares sesState.equals(reqState), and the Entra ID authenticator validates state and nonce. SAML was the only one that checked the session attribute for presence alone.

2. No replay protection

Saml2Settings#getReplayCache defaults to null and was never set, so java-saml skipped its assertion-ID replay check entirely. A captured, still-valid SAMLResponse could be replayed until its NotOnOrAfter passed.

A single InMemoryReplayCache is now held by the authenticator and attached in getSettings(). The cache is thread-safe and evicts expired entries opportunistically, so it needs no lifecycle management.

3. Failure reasons were unreachable

if (!auth.isAuthenticated()) { logger.debug("Authentication failed."); return null; }  // always taken on failure
final List<String> errors = auth.getErrors();                                          // never non-empty here
if (!errors.isEmpty()) { ... auth.getLastErrorReason() ... }

Auth#processResponse sets authenticated = true only when isValid() returns true and appends to errors only in the else branch, so the second block could never run with content. Bad signature, expired assertion, wrong audience and wrong Destination all surfaced as a DEBUG-level "Authentication failed." with no reason — nothing at all at the default log level.

The errors and getLastErrorReason() are now logged at WARN inside the !isAuthenticated() branch. This also makes the behaviour documented in sso-saml.rst ("saml.debug=true を設定すると、SAML認証に失敗した際の詳細な理由がログに出力されます") actually happen; isDebugActive() was previously referenced only from the unreachable block.

Behaviour changes

  • Concurrent SAML logins in multiple browser tabs now fail for all but the most recent AuthnRequest, since one session holds one request ID. This matches the existing OIDC behaviour.
  • Sessions that still hold the old UUID at upgrade time fail once and succeed on the retry.

Test

SamlAuthenticatorTest#test_getSettings_sharesReplayCacheAcrossRequests asserts the replay cache is attached and is the same instance across calls (this also puts the previously dead setDefaultSettings helper to use).

Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 -- SamlAuthenticatorTest

The SAML ACS callback accepted any valid assertion and reported nothing when
one was rejected.

- SAML_STATE held a fresh UUID that was never sent to the IdP and never
  compared with anything, so it only signalled "a SAML login is in progress".
  It now holds auth.getLastRequestId() and is passed to processResponse(),
  which makes java-saml compare the InResponseTo of the response against the
  ID of the AuthnRequest this SP actually issued.
- No ReplayCache was configured, so a captured assertion could be replayed
  until its NotOnOrAfter expired. A single InMemoryReplayCache is now shared
  by every request and attached to the settings in getSettings().
- The error-reporting block after processResponse() was unreachable: Auth
  records errors only when validation fails, and that path returns early at
  !isAuthenticated(). Every failure (bad signature, expired assertion, wrong
  audience or Destination) surfaced as a DEBUG-level "Authentication failed."
  with no reason. The reason is now logged at WARN inside that branch, which
  also makes the documented saml.debug=true behaviour work.

Behaviour notes:
- Concurrent SAML logins in multiple browser tabs now fail for all but the
  most recent AuthnRequest, matching how OpenIdConnectAuthenticator treats
  its state parameter.
- Sessions that still hold the old UUID fail once after an upgrade and
  succeed on the retry.
@marevol
marevol force-pushed the saml-response-validation branch from f278c39 to 4b38ec5 Compare August 10, 2026 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant