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).