fix(sso): bind SAML responses to the AuthnRequest and reject replays - #3214
Open
marevol wants to merge 1 commit into
Open
fix(sso): bind SAML responses to the AuthnRequest and reject replays#3214marevol wants to merge 1 commit into
marevol wants to merge 1 commit into
Conversation
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
force-pushed
the
saml-response-validation
branch
from
August 10, 2026 00:51
f278c39 to
4b38ec5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_STATEnever bound the response to a requestSAML_STATEheld a freshly generated UUID.auth.login(null, ...)discardsauth.getLastRequestId()and java-saml puts the current URL inRelayStatewhen the first argument isnull, 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 passesrequestId = null, short-circuiting theInResponseTocheck inSamlResponse#isValid:SAML_STATEnow holdsauth.getLastRequestId()and is passed toprocessResponse(requestId), so the response is checked against the AuthnRequest this SP actually issued.This is also what the other SSO providers already do —
OpenIdConnectAuthenticatorcomparessesState.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#getReplayCachedefaults tonulland was never set, so java-saml skipped its assertion-ID replay check entirely. A captured, still-validSAMLResponsecould be replayed until itsNotOnOrAfterpassed.A single
InMemoryReplayCacheis now held by the authenticator and attached ingetSettings(). The cache is thread-safe and evicts expired entries opportunistically, so it needs no lifecycle management.3. Failure reasons were unreachable
Auth#processResponsesetsauthenticated = trueonly whenisValid()returns true and appends toerrorsonly in the else branch, so the second block could never run with content. Bad signature, expired assertion, wrong audience and wrongDestinationall 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 insso-saml.rst("saml.debug=trueを設定すると、SAML認証に失敗した際の詳細な理由がログに出力されます") actually happen;isDebugActive()was previously referenced only from the unreachable block.Behaviour changes
Test
SamlAuthenticatorTest#test_getSettings_sharesReplayCacheAcrossRequestsasserts the replay cache is attached and is the same instance across calls (this also puts the previously deadsetDefaultSettingshelper to use).