From 967ebe4aef894f2771578cf45b743ac742ad5d10 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 20 Jul 2026 15:46:41 +0200 Subject: [PATCH] fix(security): correlate SAML responses to the request that solicited 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) --- .../Security/SamlReplayProtectionTests.cs | 187 ++++++++++++++++++ .../Api/ExternalAuth/Saml/SamlLoginFlow.cs | 51 +++++ .../Domain/Saml/SamlPendingAuthnRequest.cs | 53 +++++ .../Saml/SamlAuthnRequestStore.cs | 118 +++++++++++ .../Modgud.Authentication.csproj | 1 + .../Setup/MartenStoreOptionsExtensions.cs | 11 ++ .../Modgud.Authentication/Setup/SamlSetup.cs | 4 + 7 files changed, 425 insertions(+) create mode 100644 src/dotnet/Modgud.Api.Tests/Security/SamlReplayProtectionTests.cs create mode 100644 src/dotnet/Modgud.Authentication/Domain/Saml/SamlPendingAuthnRequest.cs create mode 100644 src/dotnet/Modgud.Authentication/Identity/LoginProviders/Saml/SamlAuthnRequestStore.cs diff --git a/src/dotnet/Modgud.Api.Tests/Security/SamlReplayProtectionTests.cs b/src/dotnet/Modgud.Api.Tests/Security/SamlReplayProtectionTests.cs new file mode 100644 index 00000000..020edb34 --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Security/SamlReplayProtectionTests.cs @@ -0,0 +1,187 @@ +using Marten; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain.Saml; +using Modgud.Authentication.Identity.LoginProviders.Saml; + +namespace Modgud.Api.Tests.Security; + +/// +/// Pins SAML request correlation and single-use (P0-2). +/// +/// +/// The ACS validated status and signatures but never checked that the Response +/// answered a request this SP actually sent: StartLoginAsync discarded the +/// AuthnRequest ID and InResponseTo appeared nowhere in the SAML code. A +/// captured, genuinely-signed Response could therefore be presented repeatedly +/// within its NotOnOrAfter window — replay, login-CSRF, session swapping. +/// +/// +/// +/// These tests drive against real +/// Postgres, which is where the guarantee lives. The full HTTP ACS path is not +/// exercised end to end because the repo has no rig for minting signed SAML +/// Responses; the flow's wiring of this store is covered by review, not by test. +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class SamlReplayProtectionTests : IntegrationTestBase +{ + public SamlReplayProtectionTests(SharedPostgresFixture fixture) : base(fixture) { } + + private static string NewRequestId() => $"_id{Guid.NewGuid():N}"; + + [Fact] + public async Task A_response_can_answer_its_request_exactly_once() + { + var ct = TestContext.Current.CancellationToken; + var requestId = NewRequestId(); + var providerId = Guid.NewGuid(); + + await using (var s = GetTenantedDocumentSession()) + await new MartenSamlAuthnRequestStore(s).RecordAsync(requestId, providerId, ct); + + await using (var s = GetTenantedDocumentSession()) + { + var first = await new MartenSamlAuthnRequestStore(s) + .TryConsumeAsync(requestId, providerId, ct); + Assert.Equal(SamlAuthnRequestConsumeResult.Consumed, first); + } + + // The replay: the very same captured Response presented again. + await using (var s = GetTenantedDocumentSession()) + { + var second = await new MartenSamlAuthnRequestStore(s) + .TryConsumeAsync(requestId, providerId, ct); + Assert.Equal(SamlAuthnRequestConsumeResult.AlreadyConsumed, second); + } + } + + [Fact] + public async Task Concurrent_presentations_of_one_response_cannot_both_win() + { + var ct = TestContext.Current.CancellationToken; + var requestId = NewRequestId(); + var providerId = Guid.NewGuid(); + + await using (var s = GetTenantedDocumentSession()) + await new MartenSamlAuthnRequestStore(s).RecordAsync(requestId, providerId, ct); + + // Separate sessions, both loading before either commits — the shape a + // load-then-delete consume would lose, since Marten does not version-check + // deletes. + await using var sA = GetTenantedDocumentSession(); + await using var sB = GetTenantedDocumentSession(); + var storeA = new MartenSamlAuthnRequestStore(sA); + var storeB = new MartenSamlAuthnRequestStore(sB); + + var results = await Task.WhenAll( + storeA.TryConsumeAsync(requestId, providerId, ct), + storeB.TryConsumeAsync(requestId, providerId, ct)); + + Assert.Equal(1, results.Count(r => r == SamlAuthnRequestConsumeResult.Consumed)); + Assert.Equal(1, results.Count(r => r == SamlAuthnRequestConsumeResult.AlreadyConsumed)); + } + + [Fact] + public async Task An_unsolicited_response_is_refused() + { + var ct = TestContext.Current.CancellationToken; + await using var s = GetTenantedDocumentSession(); + var store = new MartenSamlAuthnRequestStore(s); + + // IdP-initiated SSO: no InResponseTo at all. Only SP-initiated login + // exists in this codebase, so an unsolicited Response is never legitimate. + Assert.Equal(SamlAuthnRequestConsumeResult.Unsolicited, + await store.TryConsumeAsync(null, Guid.NewGuid(), ct)); + Assert.Equal(SamlAuthnRequestConsumeResult.Unsolicited, + await store.TryConsumeAsync(" ", Guid.NewGuid(), ct)); + } + + [Fact] + public async Task A_response_referencing_an_unknown_request_is_refused() + { + var ct = TestContext.Current.CancellationToken; + await using var s = GetTenantedDocumentSession(); + + Assert.Equal(SamlAuthnRequestConsumeResult.Unknown, + await new MartenSamlAuthnRequestStore(s) + .TryConsumeAsync(NewRequestId(), Guid.NewGuid(), ct)); + } + + [Fact] + public async Task A_response_cannot_be_presented_at_a_different_providers_acs() + { + var ct = TestContext.Current.CancellationToken; + var requestId = NewRequestId(); + var solicitedFrom = Guid.NewGuid(); + var presentedAt = Guid.NewGuid(); + + await using (var s = GetTenantedDocumentSession()) + await new MartenSamlAuthnRequestStore(s).RecordAsync(requestId, solicitedFrom, ct); + + await using (var s = GetTenantedDocumentSession()) + { + Assert.Equal(SamlAuthnRequestConsumeResult.ProviderMismatch, + await new MartenSamlAuthnRequestStore(s) + .TryConsumeAsync(requestId, presentedAt, ct)); + } + + // And the mismatch must not have spent it for the rightful provider. + await using (var s = GetTenantedDocumentSession()) + { + Assert.Equal(SamlAuthnRequestConsumeResult.Consumed, + await new MartenSamlAuthnRequestStore(s) + .TryConsumeAsync(requestId, solicitedFrom, ct)); + } + } + + [Fact] + public async Task A_stale_request_is_refused() + { + var ct = TestContext.Current.CancellationToken; + var requestId = NewRequestId(); + var providerId = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Insert(new SamlPendingAuthnRequest + { + Id = requestId, + LoginProviderId = providerId, + CreatedAt = DateTimeOffset.UtcNow.AddHours(-2), + ExpiresAt = DateTimeOffset.UtcNow.AddHours(-1), + }); + await seed.SaveChangesAsync(ct); + } + + await using var s = GetTenantedDocumentSession(); + Assert.Equal(SamlAuthnRequestConsumeResult.Expired, + await new MartenSamlAuthnRequestStore(s).TryConsumeAsync(requestId, providerId, ct)); + } + + [Fact] + public async Task Recording_a_request_prunes_ones_that_can_no_longer_be_answered() + { + var ct = TestContext.Current.CancellationToken; + var stale = NewRequestId(); + var providerId = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Insert(new SamlPendingAuthnRequest + { + Id = stale, + LoginProviderId = providerId, + CreatedAt = DateTimeOffset.UtcNow.AddHours(-2), + ExpiresAt = DateTimeOffset.UtcNow.AddHours(-1), + }); + await seed.SaveChangesAsync(ct); + } + + await using (var s = GetTenantedDocumentSession()) + await new MartenSamlAuthnRequestStore(s).RecordAsync(NewRequestId(), providerId, ct); + + await using var check = GetTenantedDocumentSession(); + Assert.Null(await check.LoadAsync(stale, ct)); + } +} diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs index 73c0bfba..0c029e35 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlLoginFlow.cs @@ -34,6 +34,7 @@ public class SamlLoginFlow( SamlSpCertificateService spCertService, ExternalLoginProcessor processor, SignInManager signInManager, + ISamlAuthnRequestStore authnRequestStore, ISessionService sessionService, ISecurityAuditLog securityAudit, ILogger logger) @@ -107,6 +108,14 @@ public async Task StartLoginAsync( binding.Bind(authnRequest); + // Remember the request BEFORE handing the browser to the IdP. This record + // is what makes the Response's InResponseTo checkable at the ACS — without + // it, any correctly-signed Response is acceptable at any time, which is + // precisely the precondition for replay. Recorded after Bind so we persist + // the exact ID that went onto the wire. + await authnRequestStore.RecordAsync( + authnRequest.Id.Value, provider.LoginProviderId, ct); + logger.LogInformation( "SAML AuthnRequest built for provider {Id} → IdP {IdpEntity}", provider.LoginProviderId, provider.IdpMetadata.EntityId); @@ -220,6 +229,48 @@ public async Task HandleAcsAsync( return Results.Redirect($"/login?error=saml-{Uri.EscapeDataString(sigError)}"); } + // Request correlation + single use. Deliberately AFTER the signature + // checks: 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 actually came from the IdP; only + // then is it worth spending the pending request on it. + // + // This is the replay defense proper: signature validation alone accepts a + // GENUINE, correctly-signed Response any number of times within its + // NotOnOrAfter window (login-CSRF, session swapping). Claiming the + // AuthnRequest we issued makes each Response answerable exactly once, and + // only at the provider it was solicited from. + var correlation = await authnRequestStore.TryConsumeAsync( + saml2Response.InResponseToAsString, provider.LoginProviderId, ct); + if (correlation != SamlAuthnRequestConsumeResult.Consumed) + { + var reason = correlation switch + { + SamlAuthnRequestConsumeResult.Unsolicited => "unsolicited", + SamlAuthnRequestConsumeResult.AlreadyConsumed => "replayed", + SamlAuthnRequestConsumeResult.Expired => "expired", + SamlAuthnRequestConsumeResult.ProviderMismatch => "provider-mismatch", + _ => "unknown-request", + }; + + logger.LogWarning( + "SAML response rejected ({Reason}) for provider {Id} — InResponseTo={InResponseTo}", + reason, provider.LoginProviderId, saml2Response.InResponseToAsString); + + securityAudit.Record(new SecurityAuditRecord + { + EventType = AuditEvents.ExternalLoginRejected, + Level = "Warning", + Ip = ip, + Status = "rejected", + Reason = $"SAML: request correlation failed ({reason}) for provider {provider.LoginProviderId}", + Message = $"SAML response refused for provider {provider.Slug} — request correlation failed ({reason})", + }); + ModgudMeters.RecordLogin(ModgudMeters.LoginMethod.External, ModgudMeters.LoginOutcome.Failure); + return Results.Redirect($"/login?error=saml-{reason}"); + } + // ITfoxtec's response builds its own ClaimsIdentity from the // assertion. We translate that into the OIDC-shaped principal // ExternalLoginProcessor expects (iss/sub + attribute claims under diff --git a/src/dotnet/Modgud.Authentication/Domain/Saml/SamlPendingAuthnRequest.cs b/src/dotnet/Modgud.Authentication/Domain/Saml/SamlPendingAuthnRequest.cs new file mode 100644 index 00000000..aad04c63 --- /dev/null +++ b/src/dotnet/Modgud.Authentication/Domain/Saml/SamlPendingAuthnRequest.cs @@ -0,0 +1,53 @@ +namespace Modgud.Authentication.Domain.Saml; + +/// +/// A SAML AuthnRequest this SP issued and is still willing to accept a Response +/// for. Persisting it is what makes InResponseTo checkable: without a +/// record of the requests we sent, any correctly-signed Response is acceptable +/// at any time, which is exactly what replay needs. +/// +/// +/// Stored per-realm (the tenant-scoped Marten session routes it to the calling +/// realm's database), so a Response solicited in one realm can't be presented in +/// another. is the AuthnRequest's own ID — the natural key, and +/// the value the IdP echoes back in InResponseTo. +/// +/// +/// +/// The consume is a version-checked Store of , +/// NOT a delete: Marten does not version-check deletes, so two concurrent +/// redemptions of one captured Response would both pass a load-then-delete +/// check. Keeping the consumed row until it expires also gives the ACS a +/// truthful "already used" signal instead of an indistinguishable "unknown id". +/// +/// +public sealed class SamlPendingAuthnRequest +{ + /// + /// How long we accept a Response for a request we sent. Generous enough for a + /// slow interactive IdP login (MFA prompts, password resets at the IdP), short + /// enough to bound the window in which a captured Response is worth anything. + /// + public const int ExpirationMinutes = 15; + + /// The AuthnRequest ID we generated — echoed back as InResponseTo. + public string Id { get; set; } = default!; + + /// + /// The provider this request went to. Checked at the ACS so a Response + /// solicited from one IdP cannot be presented at another provider's ACS + /// within the same realm. + /// + public Guid LoginProviderId { get; set; } + + public DateTimeOffset CreatedAt { get; set; } + + public DateTimeOffset ExpiresAt { get; set; } + + /// Set when a Response for this request was accepted. Single-use marker. + public DateTimeOffset? ConsumedAt { get; set; } + + public bool IsConsumed => ConsumedAt.HasValue; + + public bool IsExpired => DateTimeOffset.UtcNow > ExpiresAt; +} diff --git a/src/dotnet/Modgud.Authentication/Identity/LoginProviders/Saml/SamlAuthnRequestStore.cs b/src/dotnet/Modgud.Authentication/Identity/LoginProviders/Saml/SamlAuthnRequestStore.cs new file mode 100644 index 00000000..25df33b5 --- /dev/null +++ b/src/dotnet/Modgud.Authentication/Identity/LoginProviders/Saml/SamlAuthnRequestStore.cs @@ -0,0 +1,118 @@ +using Marten; +using Modgud.Authentication.Domain.Saml; + +namespace Modgud.Authentication.Identity.LoginProviders.Saml; + +/// +/// Tracks the AuthnRequests this SP has issued so an inbound SAML Response can be +/// correlated to one (InResponseTo) and accepted exactly once. +/// +/// +/// Backed by the tenant-scoped Marten session rather than a cookie or a +/// process-local cache, for two reasons: 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 +/// pointed at the same realm database, not just the one that issued the request. +/// +/// +public interface ISamlAuthnRequestStore +{ + /// + /// Record an AuthnRequest we are about to send, so the matching Response can + /// be correlated back to it. + /// + Task RecordAsync(string requestId, Guid loginProviderId, CancellationToken ct); + + /// + /// Atomically claim the pending request identified by . + /// Returns the outcome so the caller can tell an unsolicited Response from a + /// replayed one — they warrant different audit reasons, though both are refused. + /// + Task TryConsumeAsync( + string? requestId, Guid loginProviderId, CancellationToken ct); +} + +public enum SamlAuthnRequestConsumeResult +{ + /// Correlated to a live pending request, now spent. + Consumed, + + /// No InResponseTo at all — an unsolicited (IdP-initiated) Response. + Unsolicited, + + /// The referenced request is unknown to this realm, or already pruned. + Unknown, + + /// The referenced request was already answered — a replay. + AlreadyConsumed, + + /// The request is ours but too old to still answer. + Expired, + + /// The request was sent to a different provider than the ACS it arrived at. + ProviderMismatch, +} + +internal sealed class MartenSamlAuthnRequestStore(IDocumentSession session) : ISamlAuthnRequestStore +{ + public async Task RecordAsync(string requestId, Guid loginProviderId, CancellationToken ct) + { + var now = DateTimeOffset.UtcNow; + + // Opportunistic prune of requests that can no longer be answered anyway — + // keeps the table bounded without a background job. The ExpiresAt index + // makes this a small range delete, usually zero rows. + session.DeleteWhere(x => x.ExpiresAt < now); + + // Insert, not Store: the ID is generated per request, so a collision would + // mean something is badly wrong and should fail loudly rather than + // silently overwrite a pending request. + session.Insert(new SamlPendingAuthnRequest + { + Id = requestId, + LoginProviderId = loginProviderId, + CreatedAt = now, + ExpiresAt = now.AddMinutes(SamlPendingAuthnRequest.ExpirationMinutes), + }); + + await session.SaveChangesAsync(ct); + } + + public async Task TryConsumeAsync( + string? requestId, Guid loginProviderId, CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(requestId)) + return SamlAuthnRequestConsumeResult.Unsolicited; + + var pending = await session.LoadAsync(requestId, ct); + if (pending is null) + return SamlAuthnRequestConsumeResult.Unknown; + + if (pending.LoginProviderId != loginProviderId) + return SamlAuthnRequestConsumeResult.ProviderMismatch; + + if (pending.IsConsumed) + return SamlAuthnRequestConsumeResult.AlreadyConsumed; + + if (pending.IsExpired) + return SamlAuthnRequestConsumeResult.Expired; + + // Version-checked Store of the consume marker (see the type doc for why + // this is not a Delete). Mutating the LOADED instance keeps the version + // chain intact; storing a freshly-built instance would carry no version + // and the concurrency check would not fire. + pending.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(pending); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + // A concurrent replay of the same captured Response claimed it first. + return SamlAuthnRequestConsumeResult.AlreadyConsumed; + } + + return SamlAuthnRequestConsumeResult.Consumed; + } +} diff --git a/src/dotnet/Modgud.Authentication/Modgud.Authentication.csproj b/src/dotnet/Modgud.Authentication/Modgud.Authentication.csproj index 616bd361..0afe2a99 100644 --- a/src/dotnet/Modgud.Authentication/Modgud.Authentication.csproj +++ b/src/dotnet/Modgud.Authentication/Modgud.Authentication.csproj @@ -54,6 +54,7 @@ + diff --git a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs index b3f1d965..1f340674 100644 --- a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs +++ b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs @@ -200,6 +200,17 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) options.Schema.For() .Identity(x => x.Id); + // SAML request correlation — the AuthnRequests we've issued and are still + // willing to accept a Response for. Same one-time-use reasoning as + // MagicLinkChallenge/EmailOtpChallenge: the consume is a version-checked + // Store of ConsumedAt (Marten does NOT version-check deletes), so two + // concurrent presentations of one captured Response cannot both sign in. + // Indexed on ExpiresAt for the opportunistic prune on each new request. + options.Schema.For() + .Identity(x => x.Id) + .UseOptimisticConcurrency(true) + .Index(x => x.ExpiresAt); + // Identity events options.Events.MapEventType("user_identity_setup"); options.Events.MapEventType("user_username_changed"); diff --git a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs index 4bb6f01e..ca7301bc 100644 --- a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs +++ b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs @@ -64,6 +64,10 @@ public static IServiceCollection AddModgudSaml(this IServiceCollection services) services.AddScoped(); services.AddScoped(); + // AuthnRequest correlation store (InResponseTo + single use). Scoped — + // it consumes the tenant-scoped IDocumentSession. + services.AddScoped(); + // SamlContextBuilder needs IHttpContextAccessor to derive the SP // EntityID + ACS URL from the current request — register if not // already done elsewhere in the host setup (it's idempotent).