diff --git a/docs/integrate/oauth.md b/docs/integrate/oauth.md index 906758b9..c09887a1 100644 --- a/docs/integrate/oauth.md +++ b/docs/integrate/oauth.md @@ -89,23 +89,36 @@ can't accept them cross-realm. ### AccessTokenTypeHandler OpenIddict has `UseReferenceAccessTokens()` globally. We want to choose -between reference and JWT **per client**: +between reference and JWT **per client**. The switch is a per-request +`GenerateTokenContext` handler that, for JWT clients, flips the access +token's per-request `IsReferenceToken` / `PersistTokenPayload` off: ```csharp -public async ValueTask HandleAsync(ProcessSignInContext context) +public async ValueTask HandleAsync(GenerateTokenContext context) { + // Access tokens only — refresh tokens keep the global setting. + if (context.TokenType != OpenIddictConstants.TokenTypeIdentifiers.AccessToken) return; + var app = await _querySession.Query() - .FirstOrDefaultAsync(a => a.ClientId == clientId && !a.IsDeleted); + .FirstOrDefaultAsync(a => a.ClientId == context.ClientId && !a.IsDeleted); if (app?.AccessTokenType == AccessTokenType.Jwt) { - // Disable reference token storage for this request. - // OpenIddict generates a self-contained JWT instead. - context.Options.UseReferenceAccessTokens = false; + // Emit a self-contained JWT for this request instead of an + // opaque, server-stored reference identifier. + context.IsReferenceToken = false; + context.PersistTokenPayload = false; } } ``` +It hooks `GenerateTokenContext` (a per-request context) and must **never** +mutate `context.Options`: that object is the process-wide +`OpenIddictServerOptions` singleton (`IOptionsMonitor.CurrentValue`), so +writing `Options.UseReferenceAccessTokens` there leaks across requests — one +JWT client's sign-in would flip the global default off for every later +reference client — and races concurrent requests. + Tokens default to reference (= server-side stored, opaque, revocable). Per client this can be switched to JWT when the round-trip profile is a problem. @@ -159,8 +172,8 @@ For authorization-code exchange: 1. POST /connect/token with grant_type=authorization_code + code + verifier 2. OpenIddict validates the code (exists, not expired, not used) 3. PKCE challenge is verified -4. ProcessSignIn fires → AccessTokenTypeHandler decides - reference vs. JWT +4. Token generation fires → AccessTokenTypeHandler decides + reference vs. JWT (per access token) 5. Tokens are issued: - Reference: OpenIddictTokenDocument(s) created, reference IDs returned - JWT: signed JWTs returned, no DB entry diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/UserInfoPerAudienceTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/UserInfoPerAudienceTests.cs index 9429dd94..2dd2a03e 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/UserInfoPerAudienceTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Authorization/UserInfoPerAudienceTests.cs @@ -1128,6 +1128,65 @@ await GrantAsync(user.Id, roleAppSlug: "rs-refapp", resourceType: "policy", Assert.Equal(HttpStatusCode.Unauthorized, (await rs.GetAsync("/policy/read", TestContext.Current.CancellationToken)).StatusCode); } + [Fact] + public async Task ReferenceClient_TokenFormat_IsNotLeaked_By_A_Prior_JwtClient() + { + // Regression for a cross-request state-leak in AccessTokenTypeHandler. It + // used to switch a JWT client onto self-contained tokens by mutating the + // SHARED OpenIddictServerOptions singleton + // (context.Options.UseReferenceAccessTokens = false) and never restoring + // it. Because IOptionsMonitor.CurrentValue hands every request the same + // options instance, the first JWT-client sign-in flipped the global + // reference default OFF process-wide — so every Reference-configured + // client that authenticated afterwards silently received a self-contained + // JWT instead of an opaque reference token, losing instant revocation. + // + // This drives a JWT client FIRST (to flip the flag under the old code) + // then a Reference client in the SAME process, and asserts the reference + // client's token is still opaque. + var app = await CreateAppAsync("leak-app", "Leak App", + permissions: [("policy", "read"), ("policy", "write")]); + const string audience = "https://leak-api.example.com"; + await CreateOAuthApiAsync(audience, app.Id); + const string scopeName = "leak-api"; + await CreateScopeAsync(scopeName, [audience], app.Id); + + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Leak", lastname: "Guard", acronym: "lg", + email: "lg@test.com", password: "TestPass1234"); + await GrantAsync(user.Id, roleAppSlug: "leak-app", resourceType: "policy", + actions: ["write"], groupBoundTo: ["leak-app"]); + + // 1) JWT client authenticates first — under the bug this flips the shared + // singleton's UseReferenceAccessTokens to false for the whole process. + var jwtSecret = "TestClientSecret_" + Guid.NewGuid().ToString("N"); + var jwtClientId = "test-jwt-leaker-" + Guid.NewGuid().ToString("N"); + await CreateOAuthClientAsync( + jwtClientId, jwtSecret, "http://localhost/jwt-callback", [app.Id], + ["openid", "roles", "permissions", scopeName], AccessTokenType.Jwt); + var jwtToken = await DriveAuthCodeFlowAsync( + username: "lg", password: "TestPass1234", clientId: jwtClientId, clientSecret: jwtSecret, + redirectUri: "http://localhost/jwt-callback", + scope: $"openid roles permissions {scopeName}", resources: [audience]); + Assert.Contains('.', jwtToken); // sanity: the JWT client really got a JWT. + + // 2) Reference client authenticates AFTER — its token MUST stay opaque. + var refSecret = "TestClientSecret_" + Guid.NewGuid().ToString("N"); + var refClientId = "test-ref-victim-" + Guid.NewGuid().ToString("N"); + await CreateOAuthClientAsync( + refClientId, refSecret, "http://localhost/ref-callback", [app.Id], + ["openid", "roles", "permissions", scopeName], AccessTokenType.Reference); + var refToken = await DriveAuthCodeFlowAsync( + username: "lg", password: "TestPass1234", clientId: refClientId, clientSecret: refSecret, + redirectUri: "http://localhost/ref-callback", + scope: $"openid roles permissions {scopeName}", resources: [audience]); + + // An OpenIddict reference access token is Base64Url(random 256 bytes) — + // opaque, no dots. A dot means the client was handed a self-contained JWT + // (the leak); the Base64Url alphabet (A–Z a–z 0–9 - _) never contains one. + Assert.DoesNotContain('.', refToken); + } + private static async Task SendWithTokenAsync(HttpClient client, string path, string token) { using var req = new HttpRequestMessage(HttpMethod.Get, path); diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/AccessTokenTypeHandler.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/AccessTokenTypeHandler.cs index e82ef79c..0a93bb1c 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/AccessTokenTypeHandler.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/AccessTokenTypeHandler.cs @@ -1,24 +1,47 @@ using Modgud.Domain.OAuth.Applications; using Modgud.Domain.OAuth.Common; using Marten; +using OpenIddict.Abstractions; using OpenIddict.Server; +using static OpenIddict.Server.OpenIddictServerEvents; +using static OpenIddict.Server.OpenIddictServerHandlers; namespace Modgud.Infrastructure.OpenIddict; /// -/// OpenIddict server event handler that switches access tokens from reference to JWT -/// based on the per-client setting. +/// OpenIddict token-generation handler that switches a client's ACCESS token +/// from the global reference-token default to a self-contained JWT, based on the +/// per-client setting. /// -/// By default, UseReferenceAccessTokens() is enabled globally. For clients -/// configured with , this handler disables reference -/// token storage for that request so OpenIddict generates a self-contained JWT instead. +/// UseReferenceAccessTokens() is enabled globally. For a client +/// configured with , this handler flips OFF — +/// for the access token of this request only — the two per-request switches +/// OpenIddict reads at generation time (IsReferenceToken and +/// PersistTokenPayload), so a self-contained JWT is emitted instead of an +/// opaque reference identifier. +/// +/// It hooks — a fresh, per-request +/// context — and MUST NOT mutate context.Options. That options object is +/// the process-wide singleton +/// (IOptionsMonitor.CurrentValue hands every request the same instance); +/// writing Options.UseReferenceAccessTokens there leaks across requests +/// and races concurrent ones — a single JWT client's sign-in would flip the +/// global default off for every later reference client, silently downgrading +/// their opaque tokens to self-contained JWTs and losing instant revocation. +/// +/// Only access tokens are touched; refresh (and any other) token types keep +/// the reference semantics they derive from the global options. /// -public sealed class AccessTokenTypeHandler : IOpenIddictServerHandler +public sealed class AccessTokenTypeHandler : IOpenIddictServerHandler { public static OpenIddictServerHandlerDescriptor Descriptor { get; } - = OpenIddictServerHandlerDescriptor.CreateBuilder() + = OpenIddictServerHandlerDescriptor.CreateBuilder() .UseScopedHandler() - .SetOrder(OpenIddictServerHandlers.GenerateAccessToken.Descriptor.Order - 1) + // Run before OpenIddict persists/attaches the payload (AttachTokenPayload, + // which reads IsReferenceToken + PersistTokenPayload). CreateTokenEntry is + // the earliest stable anchor and ignores both flags, so slotting just + // ahead of it guarantees every downstream handler sees our override. + .SetOrder(Protection.CreateTokenEntry.Descriptor.Order - 500) .SetType(OpenIddictServerHandlerType.Custom) .Build(); @@ -31,13 +54,19 @@ public AccessTokenTypeHandler(IQuerySession querySession, Cimd.CimdClientResolve _cimdResolver = cimdResolver; } - public async ValueTask HandleAsync(OpenIddictServerEvents.ProcessSignInContext context) + public async ValueTask HandleAsync(GenerateTokenContext context) { + ArgumentNullException.ThrowIfNull(context); + + // Access tokens only — leave refresh/authorization-code/… to the global + // reference settings. + if (context.TokenType != OpenIddictConstants.TokenTypeIdentifiers.AccessToken) return; + var clientId = context.ClientId; if (string.IsNullOrEmpty(clientId)) return; var app = await _querySession.Query() - .FirstOrDefaultAsync(a => a.ClientId == clientId && !a.IsDeleted); + .FirstOrDefaultAsync(a => a.ClientId == clientId && !a.IsDeleted, context.CancellationToken); // CIMD clients are non-persisted, so the direct query misses them — // fall back to the resolver (cache-warm after the store's resolve @@ -46,7 +75,11 @@ public async ValueTask HandleAsync(OpenIddictServerEvents.ProcessSignInContext c if (app is null || app.AccessTokenType != AccessTokenType.Jwt) return; - // For JWT clients: disable reference token storage for this request. - context.Options.UseReferenceAccessTokens = false; + // JWT client: emit a self-contained token — don't persist the payload as a + // reference and don't hand back a reference identifier. The token ENTRY is + // still created for revocation tracking (governed by CreateTokenEntry / + // DisableTokenStorage, which we leave untouched). + context.IsReferenceToken = false; + context.PersistTokenPayload = false; } }