fix(security): correlate SAML responses to the request that solicited them#160
Merged
Merged
Conversation
… them (P0-2) A captured, correctly-signed SAML Response could be presented repeatedly within its NotOnOrAfter window -- replay, login-CSRF, session swapping. Signature validation was enforced, but nothing tied a Response to a request this SP actually sent: StartLoginAsync discarded the generated AuthnRequest ID, `InResponseTo` appeared nowhere in the SAML code, and no Response or Assertion ID was ever checked against a single-use store. Any genuine Response was therefore acceptable at any time, any number of times. The issued AuthnRequest is now persisted as SamlPendingAuthnRequest and the ACS claims it via InResponseTo, which delivers correlation and single-use in one mechanism: replaying a Response reuses an InResponseTo that is already spent. Also binds the Response to the provider it was solicited from, so it cannot be presented at a different provider's ACS in the same realm. Modelled on DpopReplayStore, with two deliberate differences: - The consume is a version-checked Store of a ConsumedAt marker, not a Delete -- Marten does not version-check deletes, so a load-then-delete consume would let two concurrent presentations both succeed. Keeping the row until it expires also lets the ACS distinguish "already used" from "unknown id" in the audit trail. - State lives in the tenant DB rather than a cookie: the ACS POST is cross-site, so a SameSite=Lax cookie does not arrive (the same constraint that already degrades the SAML link-flow), and the check has to hold across every instance serving the realm. Correlation runs AFTER signature validation, not before. Consuming first would let an unauthenticated attacker burn a victim's pending request by POSTing a garbage Response with a guessed InResponseTo, turning a replay defense into a login-denial lever. Unsolicited (IdP-initiated) responses are now refused. Only SP-initiated login exists in this codebase -- there is no IdP-initiated entry point and SLO is out of scope -- so requiring InResponseTo matches what is actually supported. Tests cover single-use, concurrent presentation, unsolicited, unknown, expired, provider mismatch and the prune, driven against real Postgres. Verified as real coverage by dropping UseOptimisticConcurrency and confirming the concurrency test fails (both racers win). The full HTTP ACS path is not exercised end to end: there is no rig for minting signed SAML Responses, so the flow's wiring of this store rests on review, not on test. Unit 1395/1395, integration 532/532. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 21, 2026
windischb
added a commit
that referenced
this pull request
Jul 21, 2026
…tiated being out of scope (#162) The P0-2 correlation fix (#160) is admin-visible in two ways this doc did not cover: SAML login is now strictly SP-initiated (an IdP-initiated, unsolicited Response is refused, because the InResponseTo correlation is what enforces replay protection), and the ACS can now redirect to new error codes. - 'What flows through': add the correlation + single-use step. - 'Out of scope': state IdP-initiated SSO is not supported, with the reason. - 'Troubleshooting': explain saml-unsolicited / saml-unknown-request / saml-replayed / saml-provider-mismatch and their usual causes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
A captured, correctly-signed SAML Response could be presented repeatedly within its
NotOnOrAfterwindow — replay, login-CSRF, session swapping.Signature validation was enforced, but nothing tied a Response to a request this SP
actually sent:
StartLoginAsyncdiscarded the generated AuthnRequest ID,InResponseToappeared nowhere in the SAML code, and no Response or Assertion ID wasever checked against a single-use store. Any genuine Response was therefore acceptable
at any time, any number of times.
Approach
The issued AuthnRequest is persisted as
SamlPendingAuthnRequest, and the ACS claims itvia
InResponseTo. That delivers correlation and single-use in one mechanism ratherthan two: replaying a Response necessarily reuses an
InResponseTothat is alreadyspent, so a separate Response/Assertion-ID replay store would be redundant. It also
binds the Response to the provider it was solicited from, so it cannot be presented at a
different provider's ACS within the same realm.
Modelled on the existing
DpopReplayStore, with two deliberate differences:Storeof aConsumedAtmarker, not aDelete.Marten does not version-check deletes, so a load-then-delete consume would let two
concurrent presentations both succeed. Keeping the row until it expires also lets the
ACS distinguish "already used" from "unknown id" in the audit trail.
SameSite=Laxcookie does not arrive — the same constraint that already degrades theSAML link-flow — and the check has to hold across every instance serving the realm.
Two decisions worth reviewing
Correlation runs after signature validation, not before. Consuming first would let an
unauthenticated attacker burn a victim's pending request by POSTing a garbage Response
with a guessed
InResponseTo, turning a replay defense into a login-denial lever. Thesignature is what proves the Response came from the IdP; only then is it worth spending
the pending request on it.
Unsolicited (IdP-initiated) responses are now refused. This is a behaviour change.
It is consistent with what the codebase actually supports — there is no IdP-initiated
entry point and SLO is explicitly out of scope — so requiring
InResponseTomatchesreality rather than restricting it. If IdP-initiated SSO is ever wanted, it should be an
explicit per-provider opt-in; I did not build that speculatively.
Testing
Unit 1395/1395, integration 532/532.
New tests cover single-use, concurrent presentation, unsolicited, unknown request,
expired, provider mismatch, and the opportunistic prune — driven against real Postgres.
Verified as real coverage rather than a snapshot of current behaviour: dropping
UseOptimisticConcurrencymakes the concurrency test fail, with both racers winning.Coverage gap, stated plainly: the HTTP ACS path is not exercised end to end, so the
flow's wiring of this store rests on review rather than test. That matters more than
usual here, because the change makes SAML login depend on
InResponseToechoing theexact ID the SP generated — a mismatch would break every SAML login, not just replays.
A manual dev IdP does exist for this (
dev/saml-idp/, simpleSAMLphp via Docker, signsboth Responses and Assertions), it is simply not wired into the automated suite. The
durable fix is an in-process rig that mints signed Responses so the real ACS endpoint can
be driven from tests; that is its own piece of work and would unblock several other SAML
assertions besides this one.
Also adds
InternalsVisibleToforModgud.Api.TestsonModgud.Authentication, matchingwhat
Modgud.ApiandModgud.Client.AspNetCorealready do, so the test can drive thestore implementation directly.
🤖 Generated with Claude Code