Skip to content

fix(security): correlate SAML responses to the request that solicited them#160

Merged
windischb merged 1 commit into
developfrom
fix/security-saml-replay-protection
Jul 20, 2026
Merged

fix(security): correlate SAML responses to the request that solicited them#160
windischb merged 1 commit into
developfrom
fix/security-saml-replay-protection

Conversation

@windischb

@windischb windischb commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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.

Approach

The issued AuthnRequest is persisted as SamlPendingAuthnRequest, and the ACS claims it
via InResponseTo. That delivers correlation and single-use in one mechanism rather
than two: replaying a Response necessarily reuses an InResponseTo that is already
spent, 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:

  • 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, not 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.

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. The
signature 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 InResponseTo matches
reality 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
UseOptimisticConcurrency makes 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 InResponseTo echoing the
exact 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, signs
both 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 InternalsVisibleTo for Modgud.Api.Tests on Modgud.Authentication, matching
what Modgud.Api and Modgud.Client.AspNetCore already do, so the test can drive the
store implementation directly.

🤖 Generated with Claude Code

… 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>
@windischb
windischb merged commit 8b67cad into develop Jul 20, 2026
8 checks passed
@windischb
windischb deleted the fix/security-saml-replay-protection branch July 20, 2026 14:26
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>
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