diff --git a/pkg/authserver/server_impl.go b/pkg/authserver/server_impl.go index 65729b662d..000bdfbddc 100644 --- a/pkg/authserver/server_impl.go +++ b/pkg/authserver/server_impl.go @@ -310,6 +310,7 @@ func decorateStorageForCIMD(cfg Config, stor storage.Storage) (storage.Storage, FallbackTTL: cfg.CIMDCacheFallbackTTL, ScopesSupported: cfg.ScopesSupported, BaselineClientScopes: cfg.BaselineClientScopes, + AllowedAudiences: cfg.AllowedAudiences, }) if err != nil { return nil, fmt.Errorf("failed to initialize CIMD storage decorator: %w", err) diff --git a/pkg/authserver/storage/cimd_decorator.go b/pkg/authserver/storage/cimd_decorator.go index d1d05d5571..86848e59c2 100644 --- a/pkg/authserver/storage/cimd_decorator.go +++ b/pkg/authserver/storage/cimd_decorator.go @@ -34,6 +34,7 @@ type CIMDStorageDecorator struct { ttl time.Duration scopesSupported []string // AS-configured scopes; nil means accept any baselineClientScopes []string // unioned into every client's scope set, same as DCR + allowedAudiences []string // AS-configured audiences granted to every CIMD client } type cimdCacheEntry struct { @@ -42,7 +43,7 @@ type cimdCacheEntry struct { } // CIMDDecoratorConfig holds the configuration for NewCIMDStorageDecorator. -// Using a struct prevents silent swaps of the two adjacent []string fields. +// Using a struct prevents silent swaps of the three adjacent []string fields. type CIMDDecoratorConfig struct { // Enabled returns base unchanged when false, avoiding an allocation. Enabled bool @@ -56,6 +57,13 @@ type CIMDDecoratorConfig struct { // BaselineClientScopes is unioned into every CIMD client's scope set, // matching DCR handler behaviour. BaselineClientScopes []string + // AllowedAudiences is the server's own allowed-audience list, granted to + // every CIMD client exactly as registration.New() grants it to DCR clients + // (see #3796). CIMD documents don't declare audience, so without this a + // CIMD client's fosite.Client.Audience stays empty and every refresh_token + // grant is rejected by fosite's DefaultAudienceMatchingStrategy, which + // matches the granted audience against the client's own audience list. + AllowedAudiences []string } // NewCIMDStorageDecorator wraps base with CIMD client lookup. @@ -80,6 +88,7 @@ func NewCIMDStorageDecorator(base Storage, cfg CIMDDecoratorConfig) (Storage, er ttl: cfg.FallbackTTL, scopesSupported: slices.Clone(cfg.ScopesSupported), baselineClientScopes: slices.Clone(cfg.BaselineClientScopes), + allowedAudiences: slices.Clone(cfg.AllowedAudiences), }, nil } @@ -267,7 +276,14 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli return nil, err } - client := registration.MarkDCRIssued(buildFositeClient(doc, resolvedScopes, grantTypes, responseTypes, authMethod)) + client := registration.MarkDCRIssued(buildFositeClient(fositeClientParams{ + doc: doc, + resolvedScopes: resolvedScopes, + grantTypes: grantTypes, + responseTypes: responseTypes, + tokenEndpointAuthMethod: authMethod, + allowedAudiences: d.allowedAudiences, + })) // Best-effort write-through: persist the resolved client in the underlying // storage so backends whose session rehydration resolves the client @@ -391,49 +407,69 @@ func negotiateTokenEndpointAuthMethod(doc *cimd.ClientMetadataDocument) (string, return "", false } +// fositeClientParams carries the inputs buildFositeClient turns into a +// fosite.Client. Every field is computed by fetch() beforehand, so the +// builder applies no validation, filtering, or negotiation of its own. A +// struct keeps the four []string values from being swapped silently at the +// call site, for the same reason CIMDDecoratorConfig is one. +type fositeClientParams struct { + doc *cimd.ClientMetadataDocument + // resolvedScopes is the already-validated scope list computed by fetch() + // via registration.ValidateScopes; when empty, DefaultScopes is used — + // this occurs when the decorator has no ScopesSupported restriction + // (unconstrained AS). + resolvedScopes []string + // grantTypes and responseTypes are the already-filtered lists computed by + // fetch() via registration.FilterPublicGrantTypes/FilterPublicResponseTypes — + // the document's declared values with unsupported entries dropped, never + // empty (the filters apply defaults and reject empty intersections). The + // stored client therefore carries only grant/response types this server + // can actually serve, not the document's full declaration. + grantTypes []string + responseTypes []string + // tokenEndpointAuthMethod is the already-negotiated value computed by + // fetch() via negotiateTokenEndpointAuthMethod; the builder applies no + // empty-to-default fallback of its own, so the resolver is the single + // authority over which method a CIMD-derived client ends up with. + tokenEndpointAuthMethod string + // allowedAudiences is the decorator's configured AllowedAudiences, granted + // to the client verbatim (mirroring registration.New() for DCR clients, + // #3796): CIMD documents never declare audience, so the AS applies its own + // audience policy by granting the audiences it would validate a "resource" + // parameter against, rather than leaving the client's audience list empty. + // An empty list reproduces the prior nil behavior. + allowedAudiences []string +} + // buildFositeClient converts a ClientMetadataDocument into a fosite.Client. // RFC 8252 §7.3 loopback dynamic-port matching for a "http://localhost" redirect // URI is provided generically by registration.RegisteredLoopbackRedirectURI // (keyed on IsPublic() + GetRedirectURIs()), so no wrapper type is needed here. -// resolvedScopes is the already-validated scope list computed by fetch() via -// registration.ValidateScopes; when empty, DefaultScopes is used — this occurs when -// the decorator has no ScopesSupported restriction (unconstrained AS). -// grantTypes and responseTypes are the already-filtered lists computed by -// fetch() via registration.FilterPublicGrantTypes/FilterPublicResponseTypes — -// the document's declared values with unsupported entries dropped, never -// empty (the filters apply defaults and reject empty intersections). The -// stored client therefore carries only grant/response types this server can -// actually serve, not the document's full declaration. -// tokenEndpointAuthMethod is the already-negotiated value computed by fetch() -// via negotiateTokenEndpointAuthMethod; this function no longer applies its -// own empty-to-default fallback, so the resolver is the single authority over -// which method a CIMD-derived client ends up with. -func buildFositeClient( - doc *cimd.ClientMetadataDocument, resolvedScopes, grantTypes, responseTypes []string, - tokenEndpointAuthMethod string, -) fosite.Client { +func buildFositeClient(p fositeClientParams) fosite.Client { // Scopes were computed and validated by fetch() via registration.ValidateScopes, // consistent with the DCR handler. Fall back to DefaultScopes only when the // decorator has no ScopesSupported restriction (unconstrained AS). - scopes := resolvedScopes + scopes := p.resolvedScopes if len(scopes) == 0 { scopes = slices.Clone(registration.DefaultScopes) } defaultClient := &fosite.DefaultClient{ - ID: doc.ClientID, - RedirectURIs: doc.RedirectURIs, - GrantTypes: grantTypes, - ResponseTypes: responseTypes, + ID: p.doc.ClientID, + RedirectURIs: p.doc.RedirectURIs, + GrantTypes: p.grantTypes, + ResponseTypes: p.responseTypes, Scopes: scopes, - // CIMD clients don't pre-declare audience; leave empty so the AS - // applies its own audience policy rather than rejecting all values. - Audience: nil, + // CIMD clients don't pre-declare audience; the AS applies its own + // audience policy by granting the audiences it would validate a + // "resource" parameter against (see the allowedAudiences field), rather + // than rejecting all values with an empty list. + Audience: slices.Clone(p.allowedAudiences), Public: true, } return &fosite.DefaultOpenIDConnectClient{ DefaultClient: defaultClient, - TokenEndpointAuthMethod: tokenEndpointAuthMethod, + TokenEndpointAuthMethod: p.tokenEndpointAuthMethod, } } diff --git a/pkg/authserver/storage/cimd_decorator_test.go b/pkg/authserver/storage/cimd_decorator_test.go index dddc9a9b1c..94baa94446 100644 --- a/pkg/authserver/storage/cimd_decorator_test.go +++ b/pkg/authserver/storage/cimd_decorator_test.go @@ -462,9 +462,13 @@ func TestCIMDStorageDecorator_GetClient_CIMDURLHitsCacheDirectly(t *testing.T) { // FilterPublicResponseTypes return for an omitted declaration) and the // default negotiated auth method, for tests that don't exercise those fields. func buildFositeClientWithDefaults(doc *cimd.ClientMetadataDocument, scopes []string) fosite.Client { - return buildFositeClient(doc, scopes, - []string{"authorization_code", "refresh_token"}, []string{"code"}, - defaultCIMDTokenEndpointAuthMethod) + return buildFositeClient(fositeClientParams{ + doc: doc, + resolvedScopes: scopes, + grantTypes: []string{"authorization_code", "refresh_token"}, + responseTypes: []string{"code"}, + tokenEndpointAuthMethod: defaultCIMDTokenEndpointAuthMethod, + }) } func TestBuildFositeClient_PassesThroughGrantAndResponseTypes(t *testing.T) { @@ -478,8 +482,12 @@ func TestBuildFositeClient_PassesThroughGrantAndResponseTypes(t *testing.T) { GrantTypes: []string{"authorization_code", "urn:ietf:params:oauth:grant-type:device_code"}, } - got := buildFositeClient(doc, nil, []string{"authorization_code"}, []string{"code"}, - defaultCIMDTokenEndpointAuthMethod) + got := buildFositeClient(fositeClientParams{ + doc: doc, + grantTypes: []string{"authorization_code"}, + responseTypes: []string{"code"}, + tokenEndpointAuthMethod: defaultCIMDTokenEndpointAuthMethod, + }) assert.Equal(t, "https://example.com/meta.json", got.GetID()) assert.True(t, got.IsPublic()) assert.ElementsMatch(t, []string{"authorization_code"}, []string(got.GetGrantTypes()), @@ -977,3 +985,150 @@ func TestCIMDStorageDecorator_PersistFailureDoesNotFailResolution(t *testing.T) require.NoError(t, err, "a write-through persistence failure must not fail the resolution") assert.NotNil(t, client) } + +// --- Audience (#6489) --- + +// TestBuildFositeClient_GrantsConfiguredAudience verifies that buildFositeClient +// carries the decorator's configured allowedAudiences onto the built client, +// mirroring how registration.New() grants DCR clients the server's +// AllowedAudiences (#3796). Without this, fosite's refresh handler rejects +// every refresh_token grant for a CIMD client because its audience whitelist +// is empty. +func TestBuildFositeClient_GrantsConfiguredAudience(t *testing.T) { + t.Parallel() + doc := &cimd.ClientMetadataDocument{ + ClientID: "https://example.com/meta.json", + RedirectURIs: []string{"https://example.com/callback"}, + } + allowed := []string{"https://mcp.example.com"} + + got := buildFositeClient(fositeClientParams{ + doc: doc, + grantTypes: []string{"authorization_code"}, + responseTypes: []string{"code"}, + tokenEndpointAuthMethod: defaultCIMDTokenEndpointAuthMethod, + allowedAudiences: allowed, + }) + + assert.Equal(t, fosite.Arguments(allowed), got.GetAudience(), + "CIMD client must inherit the server's AllowedAudiences so refresh token requests succeed") +} + +// TestBuildFositeClient_EmptyAudienceConfigKeepsPriorNilBehaviour verifies that +// an empty/unset AllowedAudiences config keeps the client's audience list +// empty, exactly as before this field was introduced (no regression for +// deployments that don't configure AllowedAudiences). +func TestBuildFositeClient_EmptyAudienceConfigKeepsPriorNilBehaviour(t *testing.T) { + t.Parallel() + doc := &cimd.ClientMetadataDocument{ + ClientID: "https://example.com/meta.json", + RedirectURIs: []string{"https://example.com/callback"}, + } + + got := buildFositeClient(fositeClientParams{ + doc: doc, + grantTypes: []string{"authorization_code"}, + responseTypes: []string{"code"}, + tokenEndpointAuthMethod: defaultCIMDTokenEndpointAuthMethod, + }) + + assert.Empty(t, got.GetAudience(), "unset AllowedAudiences must leave the client's audience list empty") +} + +// TestCIMDStorageDecorator_FetchOrCached_ClientCarriesConfiguredAudience is the +// red-green regression test for #6489 at the decorator level: it drives the +// real fetch() path (CIMD document fetch, caching) through a decorator +// constructed with AllowedAudiences configured, and asserts the resulting +// fosite.Client carries those audiences. Before the fix, this client's +// GetAudience() was always empty regardless of configuration. +// +// The same audiences must reach the snapshot fetch() persists in the +// underlying storage (#6187): Redis session rehydration resolves the client +// through that row rather than through the decorator, so a refresh_token +// grant on a rehydrated session is checked against the persisted audience +// list, not the in-memory one. +func TestCIMDStorageDecorator_FetchOrCached_ClientCarriesConfiguredAudience(t *testing.T) { + t.Parallel() + srv := serveCIMDDoc(t, "/meta.json", nil) + allowed := []string{"https://mcp.example.com"} + base := newTestBase(t) + + got, err := NewCIMDStorageDecorator(base, CIMDDecoratorConfig{ + Enabled: true, + CacheMaxSize: 10, + FallbackTTL: time.Minute, + AllowedAudiences: allowed, + }) + require.NoError(t, err) + dec := got.(*CIMDStorageDecorator) + + client, err := dec.fetchOrCached(context.Background(), cimdURL(srv, "/meta.json")) + require.NoError(t, err) + assert.Equal(t, fosite.Arguments(allowed), client.GetAudience(), + "a CIMD client resolved through the decorator must carry the configured AllowedAudiences") + + persisted, err := base.GetClient(context.Background(), cimdURL(srv, "/meta.json")) + require.NoError(t, err) + assert.Equal(t, fosite.Arguments(allowed), persisted.GetAudience(), + "the persisted snapshot that session rehydration resolves must carry the same audiences") +} + +// TestCIMDStorageDecorator_FetchOrCached_UnsetAudienceConfigKeepsClientAudienceEmpty +// proves the negative: a decorator constructed without AllowedAudiences resolves +// a client whose audience list is empty, preserving pre-fix behaviour for +// deployments that don't configure it. +func TestCIMDStorageDecorator_FetchOrCached_UnsetAudienceConfigKeepsClientAudienceEmpty(t *testing.T) { + t.Parallel() + srv := serveCIMDDoc(t, "/meta.json", nil) + + dec := newEnabledDecorator(t, newTestBase(t), 10, time.Minute) + + client, err := dec.fetchOrCached(context.Background(), cimdURL(srv, "/meta.json")) + require.NoError(t, err) + assert.Empty(t, client.GetAudience()) +} + +// TestCIMDStorageDecorator_RefreshGrantAudienceMatch is the #6489 regression +// test at the boundary that actually decides a refresh_token grant's outcome: +// fosite's RefreshTokenGrantHandler calls +// +// Config.GetAudienceStrategy(ctx)(request.GetClient().GetAudience(), originalRequest.GetGrantedAudience()) +// +// (github.com/ory/fosite/handler/oauth2/flow_refresh.go, HandleTokenEndpointRequest) +// with fosite.DefaultAudienceMatchingStrategy as the default strategy +// (fosite/config_default.go). This test drives that exact call with a +// client built by the real decorator fetch() path and a granted audience +// matching what the token handler grants for an authorization_code exchange +// (see pkg/authserver/server/handlers/token.go, GrantAudience(AllowedAudiences[0])). +// +// Before the fix, dec.allowedAudiences was never threaded into the built +// client, so client.GetAudience() was always empty and this call failed with +// "has not been whitelisted by the OAuth 2.0 Client" for every CIMD client — +// this test fails at pre-fix HEAD and passes with the fix. +func TestCIMDStorageDecorator_RefreshGrantAudienceMatch(t *testing.T) { + t.Parallel() + srv := serveCIMDDoc(t, "/meta.json", nil) + allowedAudiences := []string{"https://mcp.example.com"} + + got, err := NewCIMDStorageDecorator(newTestBase(t), CIMDDecoratorConfig{ + Enabled: true, + CacheMaxSize: 10, + FallbackTTL: time.Minute, + AllowedAudiences: allowedAudiences, + }) + require.NoError(t, err) + dec := got.(*CIMDStorageDecorator) + + client, err := dec.fetchOrCached(context.Background(), cimdURL(srv, "/meta.json")) + require.NoError(t, err) + + // grantedAudience mirrors what the original authorization_code exchange + // granted onto the session (token.go's single-AllowedAudiences default + // path), which the refresh handler re-validates against the client's own + // audience list on every refresh_token grant. + grantedAudience := []string{allowedAudiences[0]} + + err = fosite.DefaultAudienceMatchingStrategy(client.GetAudience(), grantedAudience) + require.NoError(t, err, + "a CIMD client must pass fosite's refresh-grant audience check against its own previously granted audience") +}