From fa920725083d4c04aaa21515ef5521e246d03d6d Mon Sep 17 00:00:00 2001 From: Heimdall Date: Wed, 11 Nov 2026 08:08:45 +0000 Subject: [PATCH] OIDC guard: match endpoints by segment, drop the per-client budget, brake fails closed, prune takes an authorization's tokens; tests pin each Co-Authored-By: Claude Sonnet 5.5 --- docs/oidc-issuer.md | 21 ++-- .../Configuration/OidcIssuerOptions.cs | 7 +- src/Bifrost.Web/Features/Oidc/OidcGuard.cs | 84 ++++++------- tests/Bifrost.Web.Tests/OidcIssuerTests.cs | 114 ++++++++++++++++-- .../Bifrost.Web.Tests/Support/OidcTestKit.cs | 38 ++++++ 5 files changed, 193 insertions(+), 71 deletions(-) diff --git a/docs/oidc-issuer.md b/docs/oidc-issuer.md index 8d4e301..cb2ee08 100644 --- a/docs/oidc-issuer.md +++ b/docs/oidc-issuer.md @@ -45,7 +45,7 @@ echoes its value. | `Bifrost:Oidc:Clients:Mas:ClientSecretPath` | File holding the client secret. | | `Bifrost:Oidc:Clients:Mas:RedirectUri` | MAS's callback URL, registered exactly. | | `Bifrost:Oidc:BrakePath` | Optional. While a file exists at this path, `/connect/authorize`, `/token` and `/userinfo` answer 503 `temporarily_unavailable` and nothing is issued; delete the file to release. Checked per request, so no restart. Unset: no marker brake. | -| `Bifrost:Oidc:Limits:*` | `PerAddressPermits` (30), `PerClientPermits` (300), `Window` (1 min), `MaxTrackedKeys` (10000), `MaxTokenRows` (20000), `PruneInterval` (5 min). | +| `Bifrost:Oidc:Limits:*` | `PerAddressPermits` (30), `Window` (1 min), `MaxTrackedKeys` (10000), `MaxTokenRows` (20000), `PruneInterval` (5 min). | | `Bifrost:Oidc:Clients:Mas:BackchannelLogoutUri` | Optional. MAS's back-channel logout URL, `https:///upstream/backchannel-logout/`; absolute http(s) or startup fails. Unset: no logout is sent and no authorization is revoked. | There are no development or ephemeral keys in the host. Key files are provisioned like the other @@ -115,12 +115,19 @@ account. ## Limits, prune, brake -`OidcGuardMiddleware` sits ahead of OpenIddict and guards the three endpoints. In order: the marker brake (503); a per-address -budget (429 with `Retry-After`; the address is the connection's after forwarded-headers handling that trusts only the local -proxy, IPv6 keyed by /64); a per-client budget keyed by the client id the caller names (authorize query or form, token Basic -header; unreadable or over-long names share one key); and a cap on token rows: a new authorize request (no `request_uri`) -is refused with 503 while the table holds `MaxTokenRows`. Any failure inside the guard is a 503, never a pass. MAS's flow is -a handful of requests per sign-in from one address, far under the defaults. +`OidcGuardMiddleware` sits ahead of OpenIddict and guards the three endpoints, matched by path segment so a trailing slash +is no way around it. In order: the marker brake (503; a marker that cannot be read is treated as engaged); one per-address +budget over the three endpoints together (429 with `Retry-After`; the address is the connection's after forwarded-headers +handling that trusts only the local proxy, IPv6 keyed by /64); and a cap on token rows: a new authorize request (no +`request_uri`) is refused with 503 while the table holds `MaxTokenRows`. Any failure inside the guard is a 503, never a pass. +MAS's flow is a handful of requests per sign-in from one address, far under the defaults (30 a minute is about 15 sign-ins). + +There is no per-client budget. The one client's id is public and what a caller names is unverified, so a budget keyed on it +would let anyone spend the real client's permits and lock sign-in out. The bound on the anonymous endpoint is the address +budget plus the row cap, and the cap is itself a switch an attacker with many addresses can flip: it refuses new sign-ins +(never resumed ones) until the next prune frees rows, which is the bounded failure chosen over unbounded parked rows. +Not parking state before the user authenticates (as pushed authorization requests avoid by authenticating the client first) +would remove the switch, and OpenIddict's request caching does not offer that. `OidcPruner` runs every `PruneInterval` and deletes tokens past expiry (parked authorize requests, codes, access tokens) or no longer valid, then authorizations no longer valid or ad hoc with no token left (younger than five minutes are left alone). diff --git a/src/Bifrost.Web/Configuration/OidcIssuerOptions.cs b/src/Bifrost.Web/Configuration/OidcIssuerOptions.cs index 860c776..017648e 100644 --- a/src/Bifrost.Web/Configuration/OidcIssuerOptions.cs +++ b/src/Bifrost.Web/Configuration/OidcIssuerOptions.cs @@ -53,16 +53,13 @@ public sealed class OidcClientOptions } /// -/// Budgets on /connect/authorize, /connect/token and /connect/userinfo, and the bound on what the anonymous authorize -/// endpoint can park. Per source address and per client id, one fixed window; a refused caller gets 429 with Retry-After. -/// The defaults suit one client (MAS) behind one proxy. +/// One budget on /connect/authorize, /connect/token and /connect/userinfo together, per source address in one fixed window (a refused caller gets 429 with Retry-After), +/// and the bound on what the anonymous authorize endpoint can park. No per-client budget: the client id is public. The defaults suit one client (MAS) behind one proxy. /// public sealed class OidcLimitsOptions { public int PerAddressPermits { get; init; } = 30; - public int PerClientPermits { get; init; } = 300; - public TimeSpan Window { get; init; } = TimeSpan.FromMinutes(1); public int MaxTrackedKeys { get; init; } = 10_000; diff --git a/src/Bifrost.Web/Features/Oidc/OidcGuard.cs b/src/Bifrost.Web/Features/Oidc/OidcGuard.cs index 7349693..ccfd5e1 100644 --- a/src/Bifrost.Web/Features/Oidc/OidcGuard.cs +++ b/src/Bifrost.Web/Features/Oidc/OidcGuard.cs @@ -1,5 +1,4 @@ using System.Net; -using System.Text; using Bifrost.Web.Configuration; using Bifrost.Web.Data; using Microsoft.EntityFrameworkCore; @@ -13,14 +12,12 @@ using OpenIddict.Server.AspNetCore; namespace Bifrost.Web.Features.Oidc; /// -/// The issuer's front door, ahead of OpenIddict: the marker brake, the per-address and per-client budgets, and the bound +/// The issuer's front door, ahead of OpenIddict: the marker brake, the per-address budget, and the bound /// on token rows. The address is the connection's, already corrected by the forwarded-headers handling that trusts only /// the local proxy, so a caller cannot pick its own. It fails closed: anything this middleware cannot decide is a 503, never a pass. /// public sealed class OidcGuardMiddleware(RequestDelegate next, OidcIssuerOptions options, OidcRateLimit limit, ILogger logger) { - private const int MaxClientIdLength = 128; - public async Task InvokeAsync(HttpContext http, IOpenIddictTokenManager tokens) { var endpoint = EndpointOf(http.Request.Path); @@ -38,20 +35,14 @@ public sealed class OidcGuardMiddleware(RequestDelegate next, OidcIssuerOptions return; } - if (limit.Spend($"address:{AddressKey(http.Connection.RemoteIpAddress)}") is { } byAddress) + if (limit.Spend(AddressKey(http.Connection.RemoteIpAddress)) is { } byAddress) { await RefuseAsync(http, StatusCodes.Status429TooManyRequests, "too_many_requests", byAddress.RetryAfter); return; } - if (limit.Spend($"client:{endpoint}:{await ClientKeyAsync(http, endpoint)}") is { } byClient) - { - await RefuseAsync(http, StatusCodes.Status429TooManyRequests, "too_many_requests", byClient.RetryAfter); - return; - } - // A new authorize request parks a row for anyone who can name the (public) client; a resumed one carries request_uri and parks none. - if (endpoint == "authorize" && !http.Request.Query.ContainsKey("request_uri") && await tokens.CountAsync(http.RequestAborted) >= options.Limits.MaxTokenRows) + if (endpoint == Authorize && !http.Request.Query.ContainsKey("request_uri") && await tokens.CountAsync(http.RequestAborted) >= options.Limits.MaxTokenRows) { await RefuseAsync(http, StatusCodes.Status503ServiceUnavailable, "temporarily_unavailable", options.Limits.PruneInterval); return; @@ -67,13 +58,33 @@ public sealed class OidcGuardMiddleware(RequestDelegate next, OidcIssuerOptions await next(http); } + private const string Authorize = "authorize"; + + // OpenIddict answers these paths with or without a trailing slash, so the match is by segments, never exact equality. private static string? EndpointOf(PathString path) => - path.Equals(OidcServiceCollectionExtensions.AuthorizationPath, StringComparison.OrdinalIgnoreCase) ? "authorize" - : path.Equals(OidcServiceCollectionExtensions.TokenPath, StringComparison.OrdinalIgnoreCase) ? "token" - : path.Equals(OidcServiceCollectionExtensions.UserInfoPath, StringComparison.OrdinalIgnoreCase) ? "userinfo" + path.StartsWithSegments(OidcServiceCollectionExtensions.AuthorizationPath, StringComparison.OrdinalIgnoreCase) ? Authorize + : path.StartsWithSegments(OidcServiceCollectionExtensions.TokenPath, StringComparison.OrdinalIgnoreCase) ? "token" + : path.StartsWithSegments(OidcServiceCollectionExtensions.UserInfoPath, StringComparison.OrdinalIgnoreCase) ? "userinfo" : null; - private static bool BrakeEngaged(string path) => !string.IsNullOrWhiteSpace(path) && File.Exists(path); + // A marker that cannot be told apart from an absent one (a permission or I/O error) throws, and the guard answers 503: the brake never releases by accident. + private static bool BrakeEngaged(string path) + { + if (string.IsNullOrWhiteSpace(path)) + { + return false; + } + + try + { + File.GetAttributes(path); + return true; + } + catch (Exception exception) when (exception is FileNotFoundException or DirectoryNotFoundException) + { + return false; + } + } // An IPv6 caller owns its /64, so rotating the low bits buys nothing. private static string AddressKey(IPAddress? address) @@ -98,32 +109,6 @@ public sealed class OidcGuardMiddleware(RequestDelegate next, OidcIssuerOptions return new IPAddress(bytes).ToString() + "/64"; } - // The client the caller names, not one it proved: authorize names it in the query or form, token in its Basic header. - // An unreadable value shares one key, so garbage cannot mint keys. - private static async Task ClientKeyAsync(HttpContext http, string endpoint) - { - string? clientId = null; - if (endpoint == "authorize") - { - clientId = http.Request.Query["client_id"].FirstOrDefault() - ?? (http.Request.HasFormContentType ? (await http.Request.ReadFormAsync(http.RequestAborted))["client_id"].FirstOrDefault() : null); - } - else if (endpoint == "token" && http.Request.Headers.Authorization.FirstOrDefault() is { } header - && header.StartsWith("Basic ", StringComparison.OrdinalIgnoreCase)) - { - try - { - var decoded = Encoding.UTF8.GetString(Convert.FromBase64String(header["Basic ".Length..].Trim())); - clientId = Uri.UnescapeDataString(decoded.Split(':', 2)[0]); - } - catch (FormatException) - { - } - } - - return string.IsNullOrEmpty(clientId) || clientId.Length > MaxClientIdLength ? "unnamed" : clientId; - } - private static Task RefuseAsync(HttpContext http, int status, string error, TimeSpan retryAfter) { http.Response.StatusCode = status; @@ -133,20 +118,22 @@ public sealed class OidcGuardMiddleware(RequestDelegate next, OidcIssuerOptions } } -/// The issuer's per-address and per-client permits, on the shared fixed-window budget. +/// +/// The issuer's per-address permits on the shared fixed-window budget. There is no per-client budget: the one client's id is +/// public and what a caller names unverified, so a budget on it would let anyone spend the real client's permits. +/// public sealed class OidcRateLimit(OidcIssuerOptions options, TimeProvider clock) { private readonly FixedWindowBudget addresses = new(options.Limits.PerAddressPermits, options.Limits.Window, options.Limits.MaxTrackedKeys, clock); - private readonly FixedWindowBudget clients = new(options.Limits.PerClientPermits, options.Limits.Window, options.Limits.MaxTrackedKeys, clock); - public BudgetDenial? Spend(string key) => key.StartsWith("address:", StringComparison.Ordinal) ? addresses.Spend(key) : clients.Spend(key); + public BudgetDenial? Spend(string address) => addresses.Spend(address); } /// /// Deletes expired and orphaned authorizations and tokens on a schedule: tokens past their expiry (parked authorize /// requests, codes, access tokens) or no longer valid, then authorizations that are no longer valid or are ad hoc with no -/// token left. Live rows are never touched. Plain queries, not OpenIddict's ExecuteDelete prune, so the same code runs on -/// Postgres and on the in-memory test provider. +/// token left. Live rows are never touched. Plain queries, not OpenIddict's ExecuteDelete prune, which the in-memory test provider +/// cannot run; an authorization takes its remaining tokens with it, because the foreign key has no cascade on Postgres. /// public sealed class OidcPruner(IServiceScopeFactory scopes, OidcIssuerOptions options, TimeProvider clock, ILogger logger) : BackgroundService { @@ -176,10 +163,13 @@ public sealed class OidcPruner(IServiceScopeFactory scopes, OidcIssuerOptions op do { + // A token row references its authorization (no cascade in the database), so the tokens go with it. var authorizations = await db.Set() + .Include(authorization => authorization.Tokens) .Where(authorization => authorization.CreationDate < young && (authorization.Status != Statuses.Valid || (authorization.Type == AuthorizationTypes.AdHoc && !authorization.Tokens.Any()))) .Take(Batch).ToListAsync(cancellationToken); + db.RemoveRange(authorizations.SelectMany(authorization => authorization.Tokens)); db.RemoveRange(authorizations); await db.SaveChangesAsync(cancellationToken); removed = authorizations.Count; diff --git a/tests/Bifrost.Web.Tests/OidcIssuerTests.cs b/tests/Bifrost.Web.Tests/OidcIssuerTests.cs index b2346e6..56f2b9a 100644 --- a/tests/Bifrost.Web.Tests/OidcIssuerTests.cs +++ b/tests/Bifrost.Web.Tests/OidcIssuerTests.cs @@ -7,6 +7,7 @@ using System.Text.Json; using Bifrost.Web.Domain; using Bifrost.Web.Features.Shared; using Bifrost.Web.Tests.Support; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; @@ -207,28 +208,97 @@ public sealed class OidcIssuerTests : IClassFixture } [Fact] - public async Task A_flood_naming_one_client_is_refused_for_that_client_only() + public async Task A_flood_naming_the_real_client_does_not_spend_anyone_elses_permits_and_a_trailing_slash_is_no_way_around() { - using var factory = new OidcSettingsFactory(Settings(("Limits:PerClientPermits", "3"))); + using var factory = new OidcSettingsFactory(Settings(("Limits:PerAddressPermits", "4"))); using var client = factory.CreateClient(ClientOptions()); - var flooded = new List(); - for (var i = 0; i < 5; i++) + var statuses = new List(); + foreach (var path in new[] { NewRequest().Path, NewRequest().Path, "/connect/authorize/?client_id=x", "/connect/token/", "/connect/userinfo/" }) { - using var response = await client.GetAsync(NewRequest("noisy-client").Path); - flooded.Add(response.StatusCode); + using var response = await client.GetAsync(path); + statuses.Add(response.StatusCode); } - using var other = await client.GetAsync(NewRequest().Path); - Assert.Equal([HttpStatusCode.TooManyRequests, HttpStatusCode.TooManyRequests], flooded.Skip(3)); - Assert.NotEqual(HttpStatusCode.TooManyRequests, other.StatusCode); - Assert.NotEqual(HttpStatusCode.TooManyRequests, flooded[0]); + Assert.Equal(HttpStatusCode.TooManyRequests, statuses[4]); // the slashed forms spend the same budget + Assert.DoesNotContain(HttpStatusCode.TooManyRequests, statuses.Take(4)); + } + + [Theory] + [InlineData("/connect/authorize")] + [InlineData("/connect/authorize/")] + [InlineData("/connect/token/")] + [InlineData("/connect/userinfo/")] + [InlineData("/CONNECT/Userinfo")] + public async Task The_brake_holds_for_every_spelling_of_an_endpoint(string path) + { + var marker = Path.Combine(Path.GetTempPath(), $"oidc-brake-{Guid.NewGuid():N}"); + File.WriteAllText(marker, "braked"); + try + { + using var factory = new OidcSettingsFactory(Settings(("BrakePath", marker))); + using var client = factory.CreateClient(ClientOptions()); + using var response = await client.GetAsync(path == "/connect/authorize" || path == "/connect/authorize/" ? path + "?client_id=x" : path); + Assert.Equal(HttpStatusCode.ServiceUnavailable, response.StatusCode); + } + finally + { + File.Delete(marker); + } + } + + [Fact] + public async Task A_guard_that_cannot_decide_answers_503_and_never_passes_the_request_on() + { + var reached = false; + var options = new Bifrost.Web.Configuration.OidcIssuerOptions(); + var middleware = new Bifrost.Web.Features.Oidc.OidcGuardMiddleware( + _ => { reached = true; return Task.CompletedTask; }, options, + new Bifrost.Web.Features.Oidc.OidcRateLimit(options, TimeProvider.System), + Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance); + var http = new DefaultHttpContext(); + http.Request.Path = "/connect/authorize"; + http.Request.QueryString = new QueryString("?client_id=x"); + http.Response.Body = new MemoryStream(); + + await middleware.InvokeAsync(http, System.Reflection.DispatchProxy.Create()); + + Assert.False(reached); + Assert.Equal(StatusCodes.Status503ServiceUnavailable, http.Response.StatusCode); + } + + /// Every call fails: the row count the guard asks of the token table cannot be answered. + public class ThrowingProxy : System.Reflection.DispatchProxy + { + protected override object? Invoke(System.Reflection.MethodInfo? targetMethod, object?[]? args) => throw new InvalidOperationException("the token table is down"); + } + + [Fact] + public async Task A_resumed_authorize_request_is_not_refused_by_the_row_cap_but_a_fresh_one_is() + { + using var factory = new OidcSettingsFactory(Settings(("Limits:MaxTokenRows", "3"))); + _factory = factory; + var (client, _) = await SignedInAsync(MembershipStatus.Active, "Resumer"); + using var first = await client.GetAsync(NewRequest().Path); // parks one row, answers with a redirect carrying the request_uri + var resume = first.Headers.Location!.ToString(); + Assert.Contains("request_uri", resume); + + using var anonymous = factory.CreateClient(ClientOptions()); + for (var i = 0; i < 2; i++) + { + using var filler = await anonymous.GetAsync(NewRequest().Path); + } + + using var fresh = await anonymous.GetAsync(NewRequest().Path); + Assert.Equal(HttpStatusCode.ServiceUnavailable, fresh.StatusCode); + using var resumed = await client.GetAsync(resume); + Assert.NotEqual(HttpStatusCode.ServiceUnavailable, resumed.StatusCode); } [Fact] public async Task A_mas_flow_fits_the_default_budgets_and_the_window_reopens() { - using var factory = new OidcSettingsFactory(Settings(("Limits:PerAddressPermits", "30"), ("Limits:PerClientPermits", "300"))); + using var factory = new OidcSettingsFactory(Settings(("Limits:PerAddressPermits", "30"))); _factory = factory; var (client, identity) = await SignedInAsync(MembershipStatus.Active, "Within Budget"); @@ -325,7 +395,7 @@ public sealed class OidcIssuerTests : IClassFixture await factory.PruneAsync(); var after = await factory.TokenRowsAsync(); Assert.True(after < before, "expired rows went"); - Assert.True(after >= 1, "the live access token stayed"); + Assert.Contains("urn:ietf:params:oauth:token-type:access_token", await factory.TokenTypesAsync()); // the live one, not merely some row using var userinfo = new HttpRequestMessage(HttpMethod.Get, "/connect/userinfo"); userinfo.Headers.Authorization = new AuthenticationHeaderValue("Bearer", tokens.AccessToken); @@ -333,6 +403,26 @@ public sealed class OidcIssuerTests : IClassFixture Assert.NotEqual(HttpStatusCode.Unauthorized, response.StatusCode); } + [Fact] + public async Task The_prune_takes_a_revoked_authorization_with_its_tokens_and_touches_no_live_authorization() + { + using var factory = new OidcSettingsFactory(Settings()); + _factory = factory; + var (revoked, _) = await SignedInAsync(MembershipStatus.Active, "Revoked"); + await CompleteFlowAsync(revoked); + var (live, _) = await SignedInAsync(MembershipStatus.Active, "Live"); + await CompleteFlowAsync(live); + Assert.Equal(2, await factory.AuthorizationRowsAsync()); + Assert.Equal(6, await factory.TokensOfAuthorizationsAsync()); // a spent code, an access token and an id token each + await factory.RevokeFirstAuthorizationAsync(); + + factory.Clock.Advance(TimeSpan.FromMinutes(6)); // past the grace for a young row; the access tokens (an hour) are still alive + await factory.PruneAsync(); + + Assert.Equal(1, await factory.AuthorizationRowsAsync()); + Assert.Equal(2, await factory.TokenRowsAsync()); // no orphan stays either: the revoked one's tokens went with it; the live one's access and id tokens stayed, spent codes go + } + [Theory] [InlineData("unknown-client", OidcTestKit.RedirectUri, true)] [InlineData(OidcTestKit.ClientId, "https://evil.example.test/callback", true)] diff --git a/tests/Bifrost.Web.Tests/Support/OidcTestKit.cs b/tests/Bifrost.Web.Tests/Support/OidcTestKit.cs index f413c08..c6e9575 100644 --- a/tests/Bifrost.Web.Tests/Support/OidcTestKit.cs +++ b/tests/Bifrost.Web.Tests/Support/OidcTestKit.cs @@ -5,6 +5,7 @@ using Bifrost.Web.Features.Shared; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Hosting; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Options; @@ -280,6 +281,43 @@ public sealed class OidcSettingsFactory(IReadOnlyDictionary set public Task PruneAsync() => Services.GetRequiredService().PruneAsync(CancellationToken.None); + public async Task> TokenTypesAsync() + { + using var scope = Services.CreateScope(); + var manager = scope.ServiceProvider.GetRequiredService(); + var types = new List(); + await foreach (var token in manager.ListAsync()) + { + types.Add(await manager.GetTypeAsync(token)); + } + + return types; + } + + public async Task AuthorizationRowsAsync() + { + using var scope = Services.CreateScope(); + return (int)await scope.ServiceProvider.GetRequiredService().CountAsync(); + } + + /// Tokens that belong to some authorization (the request and code rows carry none once spent). + public async Task TokensOfAuthorizationsAsync() + { + using var scope = Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + return await db.Set().CountAsync(token => token.Authorization != null); + } + + public async Task RevokeFirstAuthorizationAsync() + { + using var scope = Services.CreateScope(); + var manager = scope.ServiceProvider.GetRequiredService(); + await foreach (var authorization in manager.ListAsync(count: 1, offset: 0)) + { + Assert.True(await manager.TryRevokeAsync(authorization)); + } + } + public async Task TokenRowsAsync() { using var scope = Services.CreateScope();