diff --git a/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs b/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs index cb6d41c5..69c3dc4b 100644 --- a/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs @@ -8,7 +8,9 @@ namespace Modgud.Api.Tests.Audit; /// -/// Proves the Phase 1 emission logic in EventSourcedUserStore.AppendSecurityChangeEvents: +/// Proves the Phase 1 emission logic in EventSourcedUserStore.ResetAccessFailedCountAsync +/// (it moved there from the former AppendSecurityChangeEvents diff when the +/// lockout counter was made concurrency-safe — see P0-4 / LockoutConcurrencyTests): /// a known-user failure streak is recorded as exactly ONE aggregated /// when the access-failed counter /// resolves (>0 → 0), not one event per attempt (Decision (b)). diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs index a89635fb..d66ff513 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs @@ -259,6 +259,41 @@ public async Task Magic_Grant_TokenReuse_Rejected() Assert.Contains("invalid_grant", body); } + [Fact] + public async Task Magic_Grant_LinkAlreadyConsumedInWebFlow_Rejected() + { + // Cross-channel replay guard. The WEB redemption marks the challenge + // consumed (ConsumedAt) rather than deleting it — it has to, because + // Marten does not version-check deletes and only a version-checked Store + // makes the one-time use win a concurrency race. This native grant used to + // query by user+token-hash and check nothing but expiry, so a link already + // used in the browser stayed fully redeemable here. + await EnableNativeGrantsAsync(); + await SeedNativeClientAsync("native-magic-app"); + + var (userId, token) = await SeedMagicLinkAsync(DefaultUser!.Id); + + // Consume it exactly as the web flow does, without touching this grant. + var ct = TestContext.Current.CancellationToken; + using (var scope = NewSystemTenantScope()) + { + var session = scope.ServiceProvider.GetRequiredService(); + var hash = MagicLinkChallenge.HashToken(token); + var challenge = await session.Query() + .FirstOrDefaultAsync(c => c.TokenHash == hash, ct); + Assert.NotNull(challenge); + challenge!.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(challenge); + await session.SaveChangesAsync(ct); + } + + var response = await PostMagicAsync(userId, token); + + Assert.False(response.IsSuccessStatusCode); + var body = await response.Content.ReadAsStringAsync(ct); + Assert.Contains("invalid_grant", body); + } + // ─────────────────────── Settings lifetime validation ───────────────────── [Fact] diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs index 37ac8982..4cef6b30 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs @@ -55,7 +55,7 @@ public async Task Begin_FlagOn_ReturnsCeremonyAndChallenge() Assert.False(string.IsNullOrEmpty(options.GetProperty("challenge").GetString())); // A single-use ceremony doc was persisted for the realm. - Assert.True(await CeremonyExistsAsync(ceremonyId)); + Assert.True(await CeremonyIsRedeemableAsync(ceremonyId)); } [Fact] @@ -145,7 +145,7 @@ public async Task PasskeyGrant_ExpiredCeremony_InvalidGrant_AndConsumed() var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); Assert.Contains("invalid_grant", body); // An expired ceremony is consumed (deleted) on the rejected path. - Assert.False(await CeremonyExistsAsync(ceremonyId)); + Assert.False(await CeremonyIsRedeemableAsync(ceremonyId)); } [Fact] @@ -162,7 +162,7 @@ public async Task PasskeyGrant_BogusAssertion_InvalidGrant_AndConsumesCeremony() Assert.False(response.IsSuccessStatusCode); var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); Assert.Contains("invalid_grant", body); - Assert.False(await CeremonyExistsAsync(ceremonyId)); + Assert.False(await CeremonyIsRedeemableAsync(ceremonyId)); } [Fact] @@ -189,7 +189,7 @@ public async Task PasskeyGrant_MalformedAssertionMatchingCredential_InvalidGrant var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); Assert.Contains("invalid_grant", body); // The ceremony is single-use — consumed even on this rejected path. - Assert.False(await CeremonyExistsAsync(ceremonyId)); + Assert.False(await CeremonyIsRedeemableAsync(ceremonyId)); } // ─────────────────── crypto success path (software authenticator) ───────── @@ -221,8 +221,8 @@ public async Task PasskeyGrant_ValidAssertion_NativeOrigin_MintsTokens() Assert.True(json.RootElement.TryGetProperty("refresh_token", out var rt) && !string.IsNullOrEmpty(rt.GetString())); Assert.False(response.Headers.Contains("Set-Cookie")); - // The ceremony is single-use — gone even on the success path. - Assert.False(await CeremonyExistsAsync(Guid.Parse(ceremonyId))); + // The ceremony is single-use — no longer redeemable, even on success. + Assert.False(await CeremonyIsRedeemableAsync(Guid.Parse(ceremonyId))); } [Fact] @@ -466,12 +466,19 @@ private async Task SeedCredentialAsync(byte[] credentialId, byte[]? publicKey = return all.FirstOrDefault(c => c.CredentialId.SequenceEqual(credentialId)); } - private async Task CeremonyExistsAsync(Guid ceremonyId) + /// + /// Asserts the security property (the ceremony can still be redeemed) rather + /// than the storage mechanism. Consuming is a version-checked Store of + /// ConsumedAt, not a Delete — Marten does not version-check deletes, so + /// a Delete would let two concurrent redemptions of one ceremony_id both mint + /// a token. A consumed row therefore still exists but must never be usable. + /// + private async Task CeremonyIsRedeemableAsync(Guid ceremonyId) { using var scope = NewSystemTenantScope(); var session = scope.ServiceProvider.GetRequiredService(); var doc = await session.LoadAsync(ceremonyId, TestContext.Current.CancellationToken); - return doc is not null; + return doc is not null && !doc.IsConsumed && !doc.IsExpired; } private Task SeedPasskeyClientAsync(string clientId, string? rpId = null) => diff --git a/src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs b/src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs new file mode 100644 index 00000000..60830a8b --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs @@ -0,0 +1,125 @@ +using System.Net; +using System.Net.Http.Json; +using Marten; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain; + +namespace Modgud.Api.Tests.Security; + +/// +/// Pins the account-lockout counter as concurrency-safe (P0-4). +/// +/// +/// The counter used to be incremented in memory (user.AccessFailedCount++) +/// and written back by EventSourcedUserStore.UpdateAsync as part of a +/// whole-document Store — which ASP.NET Identity triggers after EVERY +/// AccessFailedAsync. Concurrent failed logins therefore all read the same +/// value N and all wrote N+1: a burst of parallel attempts registered as roughly +/// ONE failure, so the five-attempt threshold never tripped and the lockout was +/// bypassable simply by not sending the guesses sequentially. +/// +/// +/// +/// The counter is now a server-side jsonb increment and the lockout fields are +/// written exclusively through the IUserLockoutStore methods, never as part +/// of a full-document Store. These tests drive the real HTTP endpoint so each +/// attempt gets its own DI scope and therefore its own Marten session — the only +/// arrangement in which the original race can actually occur. +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class LockoutConcurrencyTests : IntegrationTestBase +{ + public LockoutConcurrencyTests(SharedPostgresFixture fixture) : base(fixture) { } + + private const string Password = "TestPass1234"; + + [Fact] + public async Task Parallel_failed_logins_all_count_toward_the_lockout_threshold() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Race", lastname: "Lockout", acronym: "rl", + email: "rl@test.com", password: Password); + + // Well above the 5-attempt threshold, fired simultaneously. Under the old + // read-then-write increment these collapsed into a handful of recorded + // failures; every one of them must now land. + const int parallelAttempts = 12; + var anon = Factory.CreateClient(); + + await Task.WhenAll(Enumerable.Range(0, parallelAttempts).Select(_ => + anon.PostAsJsonAsync("/api/account/login", + new { UserName = "rl", Password = "Wrong123!@#", RememberMe = false }, ct))); + + // The decisive assertion: the CORRECT password is now refused, because the + // account is locked. This is the property an attacker defeats by going + // parallel — without the fix the burst leaves the counter below the + // threshold and this request succeeds. + var withCorrectPassword = await anon.PostAsJsonAsync("/api/account/login", + new { UserName = "rl", Password = Password, RememberMe = false }, ct); + + Assert.Equal(HttpStatusCode.Unauthorized, withCorrectPassword.StatusCode); + + // And the lockout is actually persisted, not just inferred from the response. + await using var qs = GetTenantedSession(); + var securityData = await qs.LoadAsync(user.Id, ct); + Assert.NotNull(securityData); + Assert.NotNull(securityData!.LockoutEnd); + Assert.True(securityData.LockoutEnd > DateTimeOffset.UtcNow, + "the lockout window must still be open right after the burst"); + } + + [Fact] + public async Task Parallel_failed_logins_are_each_recorded_in_the_counter() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Count", lastname: "Race", acronym: "cr", + email: "cr@test.com", password: Password); + + // Deliberately BELOW the threshold so the counter is observable: crossing it + // makes Identity reset the count to 0 and stamp LockoutEnd instead. + const int parallelAttempts = 4; + var anon = Factory.CreateClient(); + + await Task.WhenAll(Enumerable.Range(0, parallelAttempts).Select(_ => + anon.PostAsJsonAsync("/api/account/login", + new { UserName = "cr", Password = "Wrong123!@#", RememberMe = false }, ct))); + + await using var qs = GetTenantedSession(); + var securityData = await qs.LoadAsync(user.Id, ct); + Assert.NotNull(securityData); + + // Every attempt landed. The old in-memory increment typically left this at 1. + Assert.Equal(parallelAttempts, securityData!.AccessFailedCount); + Assert.Null(securityData.LockoutEnd); + } + + [Fact] + public async Task A_successful_login_still_clears_the_counter() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Clear", lastname: "Counter", acronym: "cc", + email: "cc@test.com", password: Password); + + var anon = Factory.CreateClient(); + for (var i = 0; i < 3; i++) + { + await anon.PostAsJsonAsync("/api/account/login", + new { UserName = "cc", Password = "Wrong123!@#", RememberMe = false }, ct); + } + + var ok = await anon.PostAsJsonAsync("/api/account/login", + new { UserName = "cc", Password = Password, RememberMe = false }, ct); + Assert.Equal(HttpStatusCode.OK, ok.StatusCode); + + // Now that the counter is DB-authoritative rather than a mirror written back + // by UpdateAsync, the reset must still travel all the way to the document. + await using var qs = GetTenantedSession(); + var securityData = await qs.LoadAsync(user.Id, ct); + Assert.NotNull(securityData); + Assert.Equal(0, securityData!.AccessFailedCount); + } +} diff --git a/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs new file mode 100644 index 00000000..4218d982 --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs @@ -0,0 +1,220 @@ +using System.Net; +using System.Net.Http.Json; +using JasperFx; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain; +using Modgud.Authentication.Identity; + +namespace Modgud.Api.Tests.Security; + +/// +/// Pins the one-time-use guarantee for the credentials that previously only +/// *claimed* it in a comment. Two gaps were closed: +/// +/// +/// Cross-channel replay — the web magic-link flow marks a +/// challenge consumed (it cannot Delete: Marten does not version-check +/// deletes, and a version-checked Store is what wins the race). The native +/// urn:cocoar:magic grant queried by user+hash and never looked at +/// IsConsumed, so a link already used in the browser stayed redeemable +/// through the native channel. +/// Concurrent redemption — EmailOtpChallenge and +/// PasskeyCeremony consumed via Delete, which Marten does NOT +/// version-check, so two simultaneous redemptions of one code/ceremony could +/// both succeed. Both now carry a ConsumedAt marker written through a +/// version-checked Store. +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class OneTimeCredentialAtomicityTests : IntegrationTestBase +{ + public OneTimeCredentialAtomicityTests(SharedPostgresFixture fixture) : base(fixture) { } + + // ── concurrent redemption: exactly one racer may win ───────────────────── + + [Fact] + public async Task PasskeyCeremony_ConcurrentConsume_OnlyOneWins() + { + var ct = TestContext.Current.CancellationToken; + var id = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Store(new PasskeyCeremony + { + Id = id, + OptionsJson = "{}", + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(PasskeyCeremony.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + }); + await seed.SaveChangesAsync(ct); + } + + await using var sA = GetTenantedDocumentSession(); + await using var sB = GetTenantedDocumentSession(); + var a = await sA.LoadAsync(id, ct); + var b = await sB.LoadAsync(id, ct); + Assert.NotNull(a); + Assert.NotNull(b); + // Both racers observed a live, unconsumed ceremony — the exact window the + // old Delete-based consume left open. + Assert.False(a!.IsConsumed); + Assert.False(b!.IsConsumed); + + a.ConsumedAt = DateTimeOffset.UtcNow; + sA.Store(a); + await sA.SaveChangesAsync(ct); + + b.ConsumedAt = DateTimeOffset.UtcNow; + sB.Store(b); + await Assert.ThrowsAsync(async () => await sB.SaveChangesAsync(ct)); + } + + [Fact] + public async Task EmailOtpChallenge_ConcurrentConsume_OnlyOneWins() + { + var ct = TestContext.Current.CancellationToken; + var userId = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Store(new EmailOtpChallenge + { + Id = userId, + CodeHash = "hash", + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(EmailOtpChallenge.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = "otp-race@test.com", + }); + await seed.SaveChangesAsync(ct); + } + + await using var sA = GetTenantedDocumentSession(); + await using var sB = GetTenantedDocumentSession(); + var a = await sA.LoadAsync(userId, ct); + var b = await sB.LoadAsync(userId, ct); + Assert.NotNull(a); + Assert.NotNull(b); + + a!.ConsumedAt = DateTimeOffset.UtcNow; + sA.Store(a); + await sA.SaveChangesAsync(ct); + + b!.ConsumedAt = DateTimeOffset.UtcNow; + sB.Store(b); + await Assert.ThrowsAsync(async () => await sB.SaveChangesAsync(ct)); + } + + // ── web passkey ceremony state must not be client-held ─────────────────── + + [Fact] + public async Task WebPasskeyLoginOptions_KeepsCeremonyServerSide_AndIgnoresForgedCookieState() + { + // The web login ceremony used to ship the FULL AssertionOptions to the + // browser as plain Base64 (no signature, no encryption) and parse it back + // as trusted input — so a client could rewrite the ceremony or replay an + // old challenge. The cookie must now carry nothing but an opaque id, with + // the authoritative state server-side, exactly like the native flow. + var ct = TestContext.Current.CancellationToken; + var client = Factory.CreateClient(); + + var resp = await client.PostAsJsonAsync("/api/account/passkey/login-options", new { }, ct); + Assert.True(resp.IsSuccessStatusCode, + $"login-options failed ({(int)resp.StatusCode}): {await resp.Content.ReadAsStringAsync(ct)}"); + + var setCookie = Assert.Single( + resp.Headers.GetValues("Set-Cookie").Where(c => c.StartsWith(CookieName, StringComparison.Ordinal))); + var cookieValue = setCookie[(CookieName.Length + 1)..].Split(';')[0]; + + // Opaque id, not ceremony state. + Assert.True(Guid.TryParse(cookieValue, out var ceremonyId), + $"cookie must carry only a ceremony id, got: {cookieValue[..Math.Min(40, cookieValue.Length)]}"); + + // The real options live server-side and the ceremony is still redeemable. + await using (var read = GetTenantedDocumentSession()) + { + var ceremony = await read.LoadAsync(ceremonyId, ct); + Assert.NotNull(ceremony); + Assert.False(ceremony!.IsConsumed); + Assert.Contains("challenge", ceremony.OptionsJson, StringComparison.OrdinalIgnoreCase); + } + + // A cookie in the OLD client-held format (Base64 of attacker-chosen + // options) must not authenticate anything — it isn't a ceremony id, so it + // never resolves to server state. + var forged = Convert.ToBase64String( + System.Text.Encoding.UTF8.GetBytes("""{"challenge":"AAAA","rpId":"localhost"}""")); + using var req = new HttpRequestMessage(HttpMethod.Post, "/api/account/passkey/login") + { + Content = JsonContent.Create(new { id = "x", response = new { } }), + }; + req.Headers.Add("Cookie", $"{CookieName}={forged}"); + var forgedResp = await client.SendAsync(req, ct); + Assert.Equal(HttpStatusCode.Unauthorized, forgedResp.StatusCode); + } + + private const string CookieName = "Modgud.Passkey.Challenge"; + + // ── the consumed marker must actually gate redemption ──────────────────── + + [Fact] + public async Task EmailOtp_ConsumedCode_IsRejected_AndReissueStillWorks() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Otp", lastname: "Reissue", acronym: "or", + email: "otp-reissue@test.com", password: "TestPass1234"); + + using var scope = Factory.Services.CreateScope(); + var otp = scope.ServiceProvider.GetRequiredService(); + + // Seed an already-consumed challenge with a known code. The hash matches + // the service's own scheme (unsalted SHA-256 hex over the raw code), so the + // rejection below can only come from the IsConsumed gate, not a hash miss. + const string code = "123456"; + var codeHash = Convert.ToHexStringLower( + System.Security.Cryptography.SHA256.HashData(System.Text.Encoding.UTF8.GetBytes(code))); + + await using (var seed = GetTenantedDocumentSession()) + { + // RequestOtpAsync requires the user's explicit email-OTP opt-in. + var u = await seed.LoadAsync(user.Id, ct); + u!.EmailOtpEnabled = true; + seed.Store(u); + seed.Store(new EmailOtpChallenge + { + Id = user.Id, + CodeHash = codeHash, + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(EmailOtpChallenge.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = "otp-reissue@test.com", + ConsumedAt = DateTimeOffset.UtcNow, + }); + await seed.SaveChangesAsync(ct); + } + + // A consumed code must not authenticate, even though it is unexpired and + // the hash matches. + var replay = await otp.VerifyOtpAsync(user.Id, code, ct); + Assert.True(replay.IsError); + + // Re-issuing must still work — two regressions are guarded here at once: + // (1) the load-then-mutate rewrite (the document is version-checked now, + // so storing a FRESH instance over the consumed row would be rejected), + // and (2) the rate-limit exemption for consumed challenges (the row now + // survives the consume, and without the exemption it would throttle the + // very next request and lock the user out of email OTP). + var reissue = await otp.RequestOtpAsync(user.Id, ct); + Assert.False(reissue.IsError, reissue.IsError ? reissue.FirstError.Description : null); + + await using var read = GetTenantedDocumentSession(); + var fresh = await read.LoadAsync(user.Id, ct); + Assert.NotNull(fresh); + Assert.False(fresh!.IsConsumed); + Assert.Equal(0, fresh.Attempts); + } +} diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index bf41480c..9bf8373d 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -729,9 +729,13 @@ private static async Task ExchangeNativeMagicAsync( return ForbidNativeGrant(Errors.InvalidGrant, "Invalid or expired link."); var hash = MagicLinkChallenge.HashToken(token); + // IsConsumed must be checked HERE too, not just in the web flow: the web + // redemption marks the challenge consumed rather than deleting it (so the + // version-checked Store can win the concurrency race), which left a link + // already used in the browser still redeemable through this native grant. var challenge = await session.Query() .FirstOrDefaultAsync(c => c.UserId == userId && c.TokenHash == hash, ct); - if (challenge is null || challenge.IsExpired) + if (challenge is null || challenge.IsExpired || challenge.IsConsumed) { if (challenge is not null) { session.Delete(challenge); await session.SaveChangesAsync(ct); } return ForbidNativeGrant(Errors.InvalidGrant, "Invalid or expired link."); @@ -809,18 +813,33 @@ private static async Task ExchangeNativePasskeyAsync( return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); var ceremony = await session.LoadAsync(ceremonyId, ct); - if (ceremony is null || ceremony.IsExpired) + if (ceremony is null || ceremony.IsExpired || ceremony.IsConsumed) { - if (ceremony is not null) { session.Delete(ceremony); await session.SaveChangesAsync(ct); } + if (ceremony is not null && ceremony.IsExpired) + { + session.Delete(ceremony); + await session.SaveChangesAsync(ct); + } return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); } // Single-use: consume ANY presented live ceremony as soon as it resolves — // before the assertion-presence check and the verify — so a captured // ceremony_id can never be replayed, even when paired with a - // missing/garbage assertion. - session.Delete(ceremony); - await session.SaveChangesAsync(ct); + // missing/garbage assertion. This is a VERSION-CHECKED Store of the + // ConsumedAt marker, not a Delete: Marten does not version-check deletes, + // so two concurrent redemptions of one ceremony_id would otherwise both + // proceed and each mint a token. The loser's SaveChangesAsync throws. + ceremony.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(ceremony); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); + } // ADR-0009 per-client RP-ID: a ceremony begun for a specific client may only // be redeemed by that same client. Skipped for a legacy/realm-scoped ceremony diff --git a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs index e370f492..a542f9e8 100644 --- a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs @@ -25,6 +25,14 @@ public record PasskeyDisplayDto(string Id, string DisplayName, DateTimeOffset Cr private const string RegistrationCacheKey = "fido2.attestationOptions"; + /// Carries only the server-side ceremony id — never the ceremony + /// state itself. + private const string PasskeyChallengeCookie = "Modgud.Passkey.Challenge"; + + /// The cookie is scoped to the passkey endpoints; every Append AND + /// Delete must use this exact path or the delete silently does nothing. + private const string PasskeyCookiePath = "/api/account/passkey"; + public static WebApplication MapPasskeyEndpoints(this WebApplication application, string path) { var group = application.MapGroup($"{path}/account/passkey") @@ -296,23 +304,51 @@ public static WebApplication MapPasskeyEndpoints(this WebApplication application UserVerification = UserVerificationRequirement.Required, }); - // Store challenge in a secure cookie (anonymous users don't have sessions). + // The ceremony state lives SERVER-SIDE, exactly like the native + // /connect/passkey/begin flow — the cookie only carries an opaque + // ceremony id. It used to carry the whole AssertionOptions as plain + // Base64 (no signature, no encryption) and the login endpoint parsed it + // back as trusted input, so a client could rewrite the ceremony options + // and replay an old challenge. Anonymous users have no session, but they + // don't need one: the id is a lookup key, and tampering with it merely + // fails to resolve. + // + // Opportunistic cleanup on the same traffic that creates the docs, as + // the native begin endpoint does — bounds orphaned-ceremony growth + // without a scheduled job (backed by the ExpiresAt index). + session.DeleteWhere(c => c.ExpiresAt < DateTimeOffset.UtcNow); + + var optionsJson = options.ToJson(); + var ceremony = new PasskeyCeremony + { + Id = Guid.NewGuid(), + OptionsJson = optionsJson, + // Realm-scoped web login: no per-client binding, and the RP ID is + // pinned so an admin editing the setting mid-ceremony can't cause a + // begin/redeem drift (same rationale as the native flow). + ClientId = null, + RpId = await rpIdResolver.GetPrimaryDomainAsync(ct), + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(PasskeyCeremony.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + }; + session.Store(ceremony); + await session.SaveChangesAsync(ct); + // Secure=Request.IsHttps mirrors CookieSecurePolicy.SameAsRequest: // the cookie carries Secure on HTTPS requests and not on plain HTTP // (dev only). With ForwardedHeaders middleware behind the reverse // proxy, IsHttps reflects the public scheme, so production deploys // always get Secure even when Kestrel itself listens on HTTP behind // the proxy. - var optionsJson = options.ToJson(); - context.Response.Cookies.Append("Modgud.Passkey.Challenge", - Convert.ToBase64String(Encoding.UTF8.GetBytes(optionsJson)), + context.Response.Cookies.Append(PasskeyChallengeCookie, + ceremony.Id.ToString(), new CookieOptions { HttpOnly = true, Secure = context.Request.IsHttps, SameSite = SameSiteMode.Strict, - MaxAge = TimeSpan.FromMinutes(5), - Path = "/api/account/passkey", + MaxAge = TimeSpan.FromMinutes(PasskeyCeremony.ExpirationMinutes), + Path = PasskeyCookiePath, }); return Results.Content(optionsJson, "application/json"); @@ -334,33 +370,57 @@ public static WebApplication MapPasskeyEndpoints(this WebApplication application { var fido2 = await fido2Factory.CreateAsync(ct); - // Retrieve challenge from cookie - var challengeCookie = context.Request.Cookies["Modgud.Passkey.Challenge"]; - if (string.IsNullOrEmpty(challengeCookie)) + // The cookie carries only the ceremony id; the authoritative options + // come from the server-side record. Delete MUST repeat the Path the + // cookie was set with — a pathless Delete does not match it and the + // stale cookie survives in the browser. + var challengeCookie = context.Request.Cookies[PasskeyChallengeCookie]; + context.Response.Cookies.Delete(PasskeyChallengeCookie, + new CookieOptions { Path = PasskeyCookiePath }); + + if (!Guid.TryParse(challengeCookie, out var ceremonyId)) return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + var ceremony = await session.LoadAsync(ceremonyId, ct); + if (ceremony is null || ceremony.IsExpired || ceremony.IsConsumed) + return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + + // Single-use: consume before verifying, via a version-checked Store of + // ConsumedAt (Marten does not version-check deletes), so a captured + // ceremony can never be replayed and two concurrent logins cannot both + // redeem one challenge. + ceremony.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(ceremony); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + } + AssertionOptions options; try { - var optionsJson = Encoding.UTF8.GetString(Convert.FromBase64String(challengeCookie)); - options = AssertionOptions.FromJson(optionsJson); + options = AssertionOptions.FromJson(ceremony.OptionsJson); } catch { return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); } - context.Response.Cookies.Delete("Modgud.Passkey.Challenge"); - // Verify the assertion via the shared verifier (the SAME FIDO2 verify - // the native urn:cocoar:passkey grant uses — no fork). Only the - // challenge transport differs: web reads AssertionOptions from the - // cookie above, native from the server-side ceremony doc. The web flow - // is realm-scoped: RP ID = PrimaryDomain (the fido2 above was built with - // it), so legacy null-RpId credentials resolve to it and still verify. + // the native urn:cocoar:passkey grant uses — no fork). Both flows now + // also share the challenge transport: a server-side ceremony doc. The + // web flow is realm-scoped: RP ID = PrimaryDomain (the fido2 above was + // built with it), so legacy null-RpId credentials resolve to it and + // still verify. Prefer the RP ID pinned at begin-time so an admin + // changing PrimaryDomain mid-ceremony can't cause a begin/redeem drift. var primaryDomain = await rpIdResolver.GetPrimaryDomainAsync(ct); + var ceremonyRpId = ceremony.RpId ?? primaryDomain; var storedCredential = await PasskeyAssertionVerifier.VerifyAsync( - fido2, options, body.GetRawText(), session, primaryDomain, primaryDomain, ct); + fido2, options, body.GetRawText(), session, ceremonyRpId, ceremonyRpId, ct); if (storedCredential is null) return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs index f6638696..d9d79c6d 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs @@ -149,7 +149,16 @@ public async Task RegisterAsync(LoginProvider config) { if (!options.Scope.Contains(scope)) options.Scope.Add(scope); } - options.Backchannel ??= new HttpClient(); + // Authority/MetadataAddress are admin-supplied, so discovery + the + // token/userinfo backchannel are SSRF sinks. Use the same transport + // guard as the CIMD and SAML metadata fetchers (DNS resolved and + // validated before connect — closing the rebinding window — redirects + // off, tight timeouts) instead of a bare HttpClient. + options.Backchannel ??= new HttpClient( + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("OIDC metadata/backchannel fetch")) + { + Timeout = TimeSpan.FromSeconds(15), + }; // Cookie + response-mode interaction, two cases: // diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs index eb03e58e..cf3bcf90 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Routing; using Modgud.Infrastructure.Persistence.Tenancy; @@ -93,9 +94,21 @@ private static async Task AcsAsync( [FromServices] SamlLoginFlow flow, CancellationToken ct) { + // This endpoint is anonymous and does real work per request (XML parse + + // signature validation over the whole document), so cap the body well + // below ASP.NET Core's 30 MB default — a SAMLResponse form post is a few + // KB. Same tightening AssetsEndpoints applies for the same reason. + http.Features.Get() + ?.MaxRequestBodySize = MaxAcsBodyBytes; + if (!manager.TryGetBySlug(TenantContext.Current, slug, out var provider) || provider is null) return Results.NotFound(); return await flow.HandleAcsAsync(provider, http, ct); } + + /// 512 KB — a signed SAMLResponse (base64, possibly with an + /// embedded encrypted assertion and a cert chain) is comfortably under this; + /// anything larger is abuse, not a login. + private const long MaxAcsBodyBytes = 512 * 1024; } diff --git a/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs b/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs index e82679ef..7af5310b 100644 --- a/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs +++ b/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs @@ -17,6 +17,17 @@ public class EmailOtpChallenge public const int ExpirationMinutes = 10; public const int RateLimitMinutes = 2; + /// + /// Set when the code was successfully redeemed. Consuming is a + /// version-checked Store of this marker rather than a Delete — + /// Marten does NOT enforce optimistic concurrency on deletes, so two + /// concurrent redemptions of the same code would both delete-and-proceed. + /// A later replay is rejected by the gate. The row + /// is reaped by expiry / the next issue for this user. + /// + public DateTimeOffset? ConsumedAt { get; set; } + public bool IsExpired => DateTimeOffset.UtcNow >= ExpiresAt; + public bool IsConsumed => ConsumedAt is not null; public bool HasExceededAttempts => Attempts >= MaxAttempts; } diff --git a/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs b/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs index a235b9e1..1739b59a 100644 --- a/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs +++ b/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs @@ -46,7 +46,18 @@ public class PasskeyCeremony public DateTimeOffset ExpiresAt { get; set; } public DateTimeOffset CreatedAt { get; set; } + /// + /// Set when the ceremony was consumed. Consuming is a version-checked + /// Store of this marker rather than a Delete — Marten does NOT + /// enforce optimistic concurrency on deletes, so two concurrent + /// redemptions of the same ceremony_id would both delete-and-proceed + /// and each mint a token. A later replay is rejected by the + /// gate; the row is reaped by expiry. + /// + public DateTimeOffset? ConsumedAt { get; set; } + public const int ExpirationMinutes = 5; public bool IsExpired => DateTimeOffset.UtcNow >= ExpiresAt; + public bool IsConsumed => ConsumedAt is not null; } diff --git a/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs b/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs index f3c9e42c..d46008d1 100644 --- a/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs +++ b/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs @@ -74,9 +74,14 @@ public async Task> RequestNativeRegistrationOtpAsync(Guid userId, // (2FA opt-in vs. native confirmed-mailbox) run before this is reached. private async Task> IssueChallengeAsync(ApplicationUser user, CancellationToken ct) { - // Rate limiting: check if a recent challenge exists + // Rate limiting: check if a recent challenge exists. A CONSUMED challenge + // must not throttle the next request — consuming now leaves the row in + // place (a version-checked Store is what makes the one-time use atomic; + // see VerifyOtpAsync), whereas it used to be deleted. Without the + // IsConsumed exemption a user who just logged in with a code would be + // locked out of requesting a new one for the whole rate-limit window. var existing = await session.LoadAsync(user.Id, ct); - if (existing is not null && !existing.IsExpired) + if (existing is not null && !existing.IsExpired && !existing.IsConsumed) { var timeSinceCreation = DateTimeOffset.UtcNow - existing.CreatedAt; if (timeSinceCreation.TotalMinutes < config.RateLimitMinutes) @@ -88,18 +93,46 @@ private async Task> IssueChallengeAsync(ApplicationUser user, Canc var code = GenerateOtpCode(); var codeHash = HashCode(code); - // Store challenge (overwrites existing) - var challenge = new EmailOtpChallenge + // Store the challenge, overwriting any existing one for this user. The + // document is version-checked (see MartenStoreOptionsExtensions), so a + // re-issue MUST mutate the row we just loaded rather than storing a + // fresh instance — a fresh instance carries no version and the update + // would be rejected. Resetting ConsumedAt/Attempts is what makes the + // new code usable after a previous one was consumed. + if (existing is not null) { - Id = user.Id, - CodeHash = codeHash, - Attempts = 0, - ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes), - CreatedAt = DateTimeOffset.UtcNow, - Email = user.Email!, - }; - session.Store(challenge); - await session.SaveChangesAsync(ct); + existing.CodeHash = codeHash; + existing.Attempts = 0; + existing.ConsumedAt = null; + existing.ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes); + existing.CreatedAt = DateTimeOffset.UtcNow; + existing.Email = user.Email!; + session.Store(existing); + } + else + { + session.Store(new EmailOtpChallenge + { + Id = user.Id, + CodeHash = codeHash, + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = user.Email!, + }); + } + + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + // Two issue requests raced. The other one won and its code is already + // in the user's mailbox — don't send a second, conflicting code. + return Error.Validation("EmailOtp.AlreadySent", + "A verification code was recently sent. Please wait before requesting a new one."); + } // Send email await emailService.SendTemplatedEmailAsync( @@ -121,7 +154,7 @@ public async Task> VerifyOtpAsync(Guid userId, string code, Cancel { var challenge = await session.LoadAsync(userId, ct); - if (challenge is null) + if (challenge is null || challenge.IsConsumed) return Error.Validation("EmailOtp.NoPendingChallenge", "No pending verification code found. Please request a new one."); @@ -157,9 +190,23 @@ public async Task> VerifyOtpAsync(Guid userId, string code, Cancel "The verification code is invalid."); } - // Success — delete challenge - session.Delete(challenge); - await session.SaveChangesAsync(ct); + // Success — consume the challenge with a VERSION-CHECKED Store rather than + // a Delete. Marten does not enforce optimistic concurrency on deletes, so + // two concurrent redemptions of the same correct code would both + // delete-and-proceed and both authenticate. Storing ConsumedAt makes the + // losing racer's SaveChangesAsync throw, and any later replay is rejected + // by the IsConsumed gate above. + challenge.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(challenge); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return Error.Validation("EmailOtp.InvalidCode", + "The verification code is invalid."); + } return true; } diff --git a/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs b/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs index 5065c51f..1448f749 100644 --- a/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs +++ b/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs @@ -1,4 +1,5 @@ using Marten; +using Marten.Patching; using Microsoft.AspNetCore.Identity; using Modgud.Authentication.Domain; using Modgud.Authentication.Events; @@ -112,20 +113,29 @@ public async Task UpdateAsync(ApplicationUser user, Cancellation var securityData = await session.LoadAsync(user.Id, cancellationToken); if (securityData is not null) { - // Detect and append security-related events by comparing - // the user's transient properties against the persisted UserSecurityData - AppendSecurityChangeEvents(user, securityData); - - // Sync security data - securityData.PasswordHash = user.PasswordHash; - securityData.SecurityStamp = user.SecurityStamp ?? securityData.SecurityStamp; - securityData.AccessFailedCount = user.AccessFailedCount; - securityData.LockoutEnd = user.LockoutEnd; - securityData.TwoFactorEnabled = user.TwoFactorEnabled; - securityData.AuthenticatorKey = user.AuthenticatorKey; - securityData.UpdateConcurrencyStamp(); - user.ConcurrencyStamp = securityData.ConcurrencyStamp; - session.Store(securityData); + // P0-4 — patch ONLY the fields this round-trip actually owns instead of + // storing the whole document. ASP.NET Identity calls UpdateAsync after + // EVERY AccessFailedAsync, so a full Store here rewrote the lockout + // fields from a load-time snapshot and silently undid whatever a + // concurrent request had just incremented: the five-attempt lockout was + // bypassable by firing attempts in parallel. AccessFailedCount and + // LockoutEnd are now written exclusively by the IUserLockoutStore + // methods below, via atomic jsonb patches, and are deliberately absent + // from this write set. The same reasoning protects the grace-period + // fields (SecureSetupDueAt, GracePeriodDaysOverride, TwoFactorExempt), + // which are owned by their own admin/2FA paths. + var newConcurrencyStamp = Guid.NewGuid().ToString(); + session.Patch(user.Id) + .Set(x => x.PasswordHash, user.PasswordHash); + session.Patch(user.Id) + .Set(x => x.SecurityStamp, user.SecurityStamp ?? securityData.SecurityStamp); + session.Patch(user.Id) + .Set(x => x.TwoFactorEnabled, user.TwoFactorEnabled); + session.Patch(user.Id) + .Set(x => x.AuthenticatorKey, user.AuthenticatorKey); + session.Patch(user.Id) + .Set(x => x.ConcurrencyStamp, newConcurrencyStamp); + user.ConcurrencyStamp = newConcurrencyStamp; } else { @@ -296,32 +306,124 @@ public Task SetNormalizedEmailAsync(ApplicationUser user, string? normalizedEmai // IUserLockoutStore - public Task GetLockoutEndDateAsync(ApplicationUser user, CancellationToken cancellationToken) + // P0-4 — the lockout counter and the lockout window are DB-authoritative. + // Both are read straight from UserSecurityData (the same "authoritative + // re-fetch" idiom as GetSecurityStampAsync / GetTwoFactorEnabledAsync) and + // written only through atomic jsonb patches, never as part of a whole-document + // Store. The transient mirrors on ApplicationUser are kept in sync so callers + // that inspect the user object still see the truth, but nothing depends on + // them for the lockout decision. + + public async Task GetLockoutEndDateAsync(ApplicationUser user, CancellationToken cancellationToken) { - return Task.FromResult(user.LockoutEnd); + cancellationToken.ThrowIfCancellationRequested(); + + // This is the predicate behind UserManager.IsLockedOutAsync. Reading the + // in-memory mirror meant a lockout set by a CONCURRENT request after this + // user was loaded went unseen for the rest of the request. + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + return user.LockoutEnd; + + user.LockoutEnd = securityData.LockoutEnd; + return securityData.LockoutEnd; } - public Task SetLockoutEndDateAsync(ApplicationUser user, DateTimeOffset? lockoutEnd, CancellationToken cancellationToken) + public async Task SetLockoutEndDateAsync(ApplicationUser user, DateTimeOffset? lockoutEnd, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); user.LockoutEnd = lockoutEnd; - return Task.CompletedTask; + + if (securityData is null) + return; // No document yet — UpdateAsync's create branch persists the mirror. + + // Lock/unlock audit events used to be derived inside UpdateAsync by diffing + // the in-memory user against the loaded document. That diff is gone with the + // whole-document Store, so the transition is detected here, at the only place + // that actually changes the value. + if (securityData.LockoutEnd != lockoutEnd) + { + if (lockoutEnd.HasValue && lockoutEnd > DateTimeOffset.UtcNow) + session.Events.Append(user.Id, new UserLockedOutEvent(user.Id, lockoutEnd.Value)); + else if (securityData.LockoutEnd.HasValue && !lockoutEnd.HasValue) + session.Events.Append(user.Id, new UserUnlockedEvent(user.Id)); + } + + session.Patch(user.Id).Set(x => x.LockoutEnd, lockoutEnd); } - public Task GetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task GetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { - return Task.FromResult(user.AccessFailedCount); + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + return user.AccessFailedCount; + + user.AccessFailedCount = securityData.AccessFailedCount; + return securityData.AccessFailedCount; } - public Task IncrementAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task IncrementAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { - user.AccessFailedCount++; - return Task.FromResult(user.AccessFailedCount); + cancellationToken.ThrowIfCancellationRequested(); + + // Server-side jsonb increment — the "Audit #24" pattern already used for the + // email-OTP attempt counter. A read-then-write increment lets N concurrent + // failed logins all read the same value and write value+1, so only ONE + // attempt in the burst is ever recorded and MaxFailedAccessAttempts never + // trips. The patch lands every attempt regardless of concurrency. + // + // The flush is required, not incidental: UserManager compares this return + // value against MaxFailedAccessAttempts to decide whether to lock, so we + // must read back a value that includes our own increment. + session.Patch(user.Id).Increment(x => x.AccessFailedCount, 1); + await session.SaveChangesAsync(cancellationToken); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + { + // Migration-created user with no security document yet — fall back to the + // pre-existing in-memory behaviour; UpdateAsync's create branch persists it. + user.AccessFailedCount++; + return user.AccessFailedCount; + } + + // A racer may have incremented further between our commit and this read, so + // this can be HIGHER than our own increment. That direction is fail-closed + // (the burst locks out at least as early as it should), which is exactly the + // bias we want on a brute-force counter. + user.AccessFailedCount = securityData.AccessFailedCount; + return securityData.AccessFailedCount; } - public Task ResetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task ResetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); user.AccessFailedCount = 0; - return Task.CompletedTask; + + if (securityData is null) + return; // No document yet — UpdateAsync's create branch persists the mirror. + + // A failure streak just resolved — the counter went from >0 back to 0 (a + // successful sign-in or an unlock). Record it as ONE aggregated audit event + // (Decision (b)), not one per attempt: no stream spam, and an attacker + // spraying a victim can't inflate that victim's stream. No IP (the aggregate + // has no single source); erasable with the user's stream. This detector used + // to live in UpdateAsync as a stale-vs-fresh diff; it now sits on the only + // path that actually resets the counter, and reads the authoritative value + // rather than the possibly-stale mirror. + if (securityData.AccessFailedCount > 0) + { + session.Events.Append(user.Id, new UserLoginFailuresObservedEvent( + user.Id, securityData.AccessFailedCount, DateTimeOffset.UtcNow)); + } + + session.Patch(user.Id).Set(x => x.AccessFailedCount, 0); } public Task GetLockoutEnabledAsync(ApplicationUser user, CancellationToken cancellationToken) @@ -386,40 +488,12 @@ public Task GetPhoneNumberConfirmedAsync(ApplicationUser user, Cancellatio public Task SetPhoneNumberConfirmedAsync(ApplicationUser user, bool confirmed, CancellationToken cancellationToken) => Task.CompletedTask; - private void AppendSecurityChangeEvents(ApplicationUser user, UserSecurityData securityData) - { - var events = new List(); - - // Lockout changed — detected by comparing user's transient properties - // against the persisted UserSecurityData document - if (user.LockoutEnd != securityData.LockoutEnd) - { - if (user.LockoutEnd.HasValue && user.LockoutEnd > DateTimeOffset.UtcNow) - { - events.Add(new UserLockedOutEvent(user.Id, user.LockoutEnd.Value)); - } - else if (securityData.LockoutEnd.HasValue && !user.LockoutEnd.HasValue) - { - events.Add(new UserUnlockedEvent(user.Id)); - } - } - - // A failure streak just resolved — the access-failed counter went from >0 - // back to 0 (a successful sign-in or an unlock reset it). Record it as ONE - // aggregated audit event (Decision (b)), not one per attempt: no stream spam, - // and an attacker spraying a victim can't inflate that victim's stream. No IP - // (the aggregate has no single source); erasable with the user's stream. - if (securityData.AccessFailedCount > 0 && user.AccessFailedCount == 0) - { - events.Add(new UserLoginFailuresObservedEvent( - user.Id, securityData.AccessFailedCount, DateTimeOffset.UtcNow)); - } - - if (events.Count > 0) - { - session.Events.Append(user.Id, events.ToArray()); - } - } + // The former AppendSecurityChangeEvents lived here. Both of its detectors + // (lockout transition, resolved failure streak) were stale-vs-fresh diffs that + // only worked because UpdateAsync wrote the whole security document from the + // in-memory user — the very pattern that made the lockout counter racy (P0-4). + // They now sit on SetLockoutEndDateAsync / ResetAccessFailedCountAsync, the + // only paths that actually change those values. private async Task PopulateSecurityDataAsync(ApplicationUser user, CancellationToken cancellationToken) { diff --git a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs index c0f9d212..b3f1d965 100644 --- a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs +++ b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs @@ -63,8 +63,15 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) options.Schema.For() .Identity(x => x.Id); + // Same one-time-use reasoning as MagicLinkChallenge below: the consume is + // a version-checked Store of ConsumedAt (Marten does not version-check + // deletes), so exactly one of two concurrent redemptions of the same code + // wins and the loser gets a ConcurrencyException. Re-issuing a code for a + // user mutates the loaded row rather than storing a fresh instance, so the + // version chain stays intact. options.Schema.For() - .Identity(x => x.Id); + .Identity(x => x.Id) + .UseOptimisticConcurrency(true); // Audit #25 — optimistic concurrency makes the "one-time use" consume // atomic. Login loads the challenge then deletes it; two concurrent @@ -121,9 +128,14 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) // ADR-0010 Phase 2 — cookieless WebAuthn assertion ceremony for the // native urn:cocoar:passkey grant. Single-use, short-TTL; keyed by the // server-generated ceremonyId. Indexed by ExpiresAt for an optional sweep. + // Optimistic concurrency + a ConsumedAt marker make the single-use + // guarantee real: the redeem path version-checks its Store, so two + // concurrent redemptions of one ceremony_id cannot both mint a token + // (a Delete would not be version-checked). options.Schema.For() .Identity(x => x.Id) - .Index(x => x.ExpiresAt); + .Index(x => x.ExpiresAt) + .UseOptimisticConcurrency(true); // ADR-0009 — cookieless WebAuthn ATTESTATION ceremony for native per-client // passkey enrollment (Bearer-authenticated; a native client has no session diff --git a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs index 91d86904..4bb6f01e 100644 --- a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs +++ b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs @@ -36,7 +36,17 @@ public static IServiceCollection AddModgudSaml(this IServiceCollection services) // so the fetcher itself can stay singleton (matches the singleton- // DynamicSamlSchemeManager that uses it) while still benefiting // from the framework's connection pooling + handler rotation. - services.AddHttpClient(SamlMetadataFetcher.HttpClientName); + // The metadata URL is admin-supplied, so the fetch is an SSRF sink: it + // gets the same transport-level guard as the CIMD fetcher (DNS resolved + // + validated before connect, redirects off, tight timeouts). A realm + // admin is a lower-trust tier than the platform operator, so + // "an admin configured it" is not a reason to skip this. + services.AddHttpClient(SamlMetadataFetcher.HttpClientName, client => + { + client.Timeout = TimeSpan.FromSeconds(5); + }) + .ConfigurePrimaryHttpMessageHandler(() => + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("SAML metadata fetch")); services.AddSingleton(); services.AddSingleton(); diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs b/src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs similarity index 87% rename from src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs rename to src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs index 0d4feb84..71e437b4 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs +++ b/src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs @@ -1,21 +1,23 @@ using System.Net; using System.Net.Sockets; -namespace Modgud.Infrastructure.OpenIddict.Cimd; +namespace Modgud.Infrastructure.Http; /// -/// SSRF address guard for the CIMD metadata fetcher. Classifies a resolved -/// as routable-public (allowed) or -/// non-public/special-use (blocked). Pure + dependency-free so the full -/// range table is unit-testable. +/// SSRF address guard for every server-side fetch of an operator- or +/// admin-supplied URL (CIMD client metadata, SAML IdP metadata, OIDC discovery +/// + backchannel). Classifies a resolved as +/// routable-public (allowed) or non-public/special-use (blocked). Pure + +/// dependency-free so the full range table is unit-testable. /// -/// The fetcher resolves DNS itself and connects to a validated IP via -/// SocketsHttpHandler.ConnectCallback — checking the resolved +/// Callers resolve DNS themselves and connect to a validated IP via +/// SocketsHttpHandler.ConnectCallback (see +/// ) — checking the resolved /// address (not the hostname) and connecting to exactly that address closes /// the DNS-rebinding window where a name resolves "public" at validation /// time and "private" at connect time. /// -public static class CimdIpGuard +public static class SsrfIpGuard { /// /// True if must NOT be connected to (loopback, diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs b/src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs similarity index 58% rename from src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs rename to src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs index f72b09c5..7cea5344 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs +++ b/src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs @@ -1,36 +1,46 @@ using System.Net.Sockets; -namespace Modgud.Infrastructure.OpenIddict.Cimd; +namespace Modgud.Infrastructure.Http; /// -/// Builds the primary for the CIMD metadata -/// fetcher with the SSRF defences baked into the transport itself: +/// Builds the primary for any server-side +/// fetch of an operator- or admin-supplied URL, with the SSRF defences baked +/// into the transport itself: /// /// redirects disabled — a 30x to an internal host can't be followed; /// a that resolves /// DNS, refuses any non-public resolved address -/// (), and connects to exactly that validated IP — +/// (), and connects to exactly that validated IP — /// closing the DNS-rebinding gap a name-then-connect check would leave; /// tight connect/response timeouts. /// /// TLS still validates against the request hostname (SNI + cert), so pinning /// the socket to the pre-validated IP doesn't weaken certificate checking. +/// +/// Used by every admin-supplied-URL fetcher: CIMD client metadata, SAML +/// IdP metadata, and the dynamic OIDC discovery/backchannel. A realm admin is a +/// lower-trust tier than the platform operator, so "an admin configured it" is +/// NOT a reason to skip this guard. /// -internal static class CimdHttpMessageHandlerFactory +public static class SsrfSafeHttpHandlerFactory { - public static SocketsHttpHandler Create() => new() + /// Short human-readable label for the fetch (e.g. + /// "SAML metadata fetch"), used in the refusal message so a blocked + /// connection is diagnosable from the log alone. + public static SocketsHttpHandler Create(string purpose) => new() { AllowAutoRedirect = false, AutomaticDecompression = System.Net.DecompressionMethods.None, ConnectTimeout = TimeSpan.FromSeconds(5), - // Don't reuse pooled connections across hosts longer than needed; a - // CIMD fetch is a one-shot lookup, not a chatty API. + // Don't reuse pooled connections across hosts longer than needed; these + // are one-shot metadata lookups, not chatty APIs. PooledConnectionLifetime = TimeSpan.FromMinutes(2), - ConnectCallback = ConnectAsync, + ConnectCallback = (context, cancellationToken) => + ConnectAsync(purpose, context, cancellationToken), }; private static async ValueTask ConnectAsync( - SocketsHttpConnectionContext context, CancellationToken cancellationToken) + string purpose, SocketsHttpConnectionContext context, CancellationToken cancellationToken) { var host = context.DnsEndPoint.Host; var port = context.DnsEndPoint.Port; @@ -40,7 +50,7 @@ private static async ValueTask ConnectAsync( foreach (var address in addresses) { - if (CimdIpGuard.IsBlocked(address)) continue; + if (SsrfIpGuard.IsBlocked(address)) continue; var socket = new Socket(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp) { @@ -61,6 +71,6 @@ private static async ValueTask ConnectAsync( // Every resolved address was non-public (or the host resolved to // nothing). Refuse — this is the SSRF block path. throw new IOException( - $"CIMD metadata fetch refused: '{host}' did not resolve to a routable public address."); + $"{purpose} refused: '{host}' did not resolve to a routable public address."); } } diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs index f8f07adb..8fbf5662 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs @@ -38,7 +38,8 @@ namespace Modgud.Infrastructure.OpenIddict.Cimd; public sealed class CimdClientResolver { /// Named client whose primary - /// handler carries the SSRF guard (). + /// handler carries the SSRF guard + /// (). public const string HttpClientName = "Modgud.Cimd.MetadataFetcher"; private const int MaxBodyBytes = 5 * 1024; diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs index a3005c59..0ac5f1d8 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs @@ -102,7 +102,8 @@ public static IServiceCollection AddOpenIddictWithMarten( { client.Timeout = TimeSpan.FromSeconds(5); }) - .ConfigurePrimaryHttpMessageHandler(() => Cimd.CimdHttpMessageHandlerFactory.Create()); + .ConfigurePrimaryHttpMessageHandler(() => + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("CIMD metadata fetch")); // Custom Marten stores (OpenIddict 7 pattern: register the store directly, // not the entity → store mapping) diff --git a/src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs b/src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs similarity index 77% rename from src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs rename to src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs index 30c1b65a..94545a86 100644 --- a/src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs @@ -1,15 +1,16 @@ using System.Net; -using Modgud.Infrastructure.OpenIddict.Cimd; +using Modgud.Infrastructure.Http; -namespace Modgud.Tests.Unit.OAuth.Cimd; +namespace Modgud.Tests.Unit.Infrastructure.Http; /// -/// Pins the SSRF address classifier. The CIMD fetcher resolves -/// DNS itself and refuses any non-public resolved address before connecting; -/// a gap here would re-open the server-side-request-forgery surface the whole -/// CIMD design is gated behind. +/// Pins the SSRF address classifier shared by every admin-supplied-URL fetch +/// (CIMD client metadata, SAML IdP metadata, OIDC discovery/backchannel). Each +/// resolves DNS itself and refuses any non-public resolved address before +/// connecting; a gap here would re-open the server-side-request-forgery surface +/// all three are gated behind. /// -public class CimdIpGuardTests +public class SsrfIpGuardTests { [Theory] // IPv4 special-use / non-routable @@ -44,7 +45,7 @@ public class CimdIpGuardTests [InlineData("::ffff:169.254.169.254")] public void Blocks_non_public_addresses(string address) { - Assert.True(CimdIpGuard.IsBlocked(IPAddress.Parse(address)), + Assert.True(SsrfIpGuard.IsBlocked(IPAddress.Parse(address)), $"{address} is non-public and must be blocked."); } @@ -57,7 +58,7 @@ public void Blocks_non_public_addresses(string address) [InlineData("2606:2800:220:1:248:1893:25c8:1946")] // example.com v6 public void Allows_routable_public_addresses(string address) { - Assert.False(CimdIpGuard.IsBlocked(IPAddress.Parse(address)), + Assert.False(SsrfIpGuard.IsBlocked(IPAddress.Parse(address)), $"{address} is a routable public address and must be allowed."); } }