diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 65710c528f..2ae45bdb32 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -230,11 +230,26 @@ type EmbeddedAuthServerConfig struct { // +optional Storage *AuthServerStorageConfig `json:"storage,omitempty"` - // AllowedAudiences is the list of valid resource URIs that tokens can be issued for. - // For an embedded auth server, this can be determined by the servers (MCP or vMCP) it serves. - - // ScopesSupported is the list of OAuth 2.0 scopes that this authorization server supports. - // For an embedded auth server, this can be derived from the server's (MCP or vMCP) OIDC configuration. + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + // included in every client registration. The embedded auth server unions these + // scopes into the registered set returned by RFC 7591 Dynamic Client + // Registration, so a client that narrows the `scope` field at /oauth/register + // can still request the baseline scopes at /oauth/authorize. All values must + // be present in the upstream-derived scopesSupported set; the auth server + // fails to start if any value is missing. + // + // Security: every client registered via /oauth/register will gain the + // ability to request these scopes at /oauth/authorize, regardless of what + // the client itself requested. Keep the baseline narrow (typically + // "openid" and "offline_access"). Adding a privileged scope here — e.g. + // "admin:read" — would grant it to every DCR-registered client, including + // public clients like Claude Code, Cursor, and VS Code. + // +kubebuilder:validation:MaxItems=10 + // +kubebuilder:validation:items:MinLength=1 + // +kubebuilder:validation:items:Pattern=`^[\x21\x23-\x5B\x5D-\x7E]+$` + // +listType=atomic + // +optional + BaselineClientScopes []string `json:"baselineClientScopes,omitempty"` } // TokenLifespanConfig holds configuration for token lifetimes. diff --git a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go index b27e168797..551361b978 100644 --- a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go +++ b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go @@ -254,6 +254,11 @@ func (in *EmbeddedAuthServerConfig) DeepCopyInto(out *EmbeddedAuthServerConfig) *out = new(AuthServerStorageConfig) (*in).DeepCopyInto(*out) } + if in.BaselineClientScopes != nil { + in, out := &in.BaselineClientScopes, &out.BaselineClientScopes + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EmbeddedAuthServerConfig. diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index 177875da16..e735e691e3 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -530,6 +530,7 @@ func BuildAuthServerRunConfig( AuthorizationEndpointBaseURL: authConfig.AuthorizationEndpointBaseURL, AllowedAudiences: allowedAudiences, ScopesSupported: scopesSupported, + BaselineClientScopes: authConfig.BaselineClientScopes, } // Build signing key configuration diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 7988038f77..d423ed92ea 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -214,6 +214,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -1385,6 +1408,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index b00683e8d3..a0cb427164 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -87,6 +87,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -2723,6 +2746,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index fb4b731da4..bf2aaadbbd 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -217,6 +217,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -1388,6 +1411,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index bc484f596b..87826c3db5 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -90,6 +90,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -2726,6 +2749,29 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + baselineClientScopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be + included in every client registration. The embedded auth server unions these + scopes into the registered set returned by RFC 7591 Dynamic Client + Registration, so a client that narrows the `scope` field at /oauth/register + can still request the baseline scopes at /oauth/authorize. All values must + be present in the upstream-derived scopesSupported set; the auth server + fails to start if any value is missing. + + Security: every client registered via /oauth/register will gain the + ability to request these scopes at /oauth/authorize, regardless of what + the client itself requested. Keep the baseline narrow (typically + "openid" and "offline_access"). Adding a privileged scope here — e.g. + "admin:read" — would grant it to every DCR-registered client, including + public clients like Claude Code, Cursor, and VS Code. + items: + minLength: 1 + pattern: ^[\x21\x23-\x5B\x5D-\x7E]+$ + type: string + maxItems: 10 + type: array + x-kubernetes-list-type: atomic hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 315123821f..54ba19b62e 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1135,6 +1135,7 @@ _Appears in:_ | `tokenLifespans` _[api.v1beta1.TokenLifespanConfig](#apiv1beta1tokenlifespanconfig)_ | TokenLifespans configures the duration that various tokens are valid.
If not specified, defaults are applied (access: 1h, refresh: 7d, authCode: 10m). | | Optional: \{\}
| | `upstreamProviders` _[api.v1beta1.UpstreamProviderConfig](#apiv1beta1upstreamproviderconfig) array_ | UpstreamProviders configures connections to upstream Identity Providers.
The embedded auth server delegates authentication to these providers.
MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. | | MinItems: 1
Required: \{\}
| | `storage` _[api.v1beta1.AuthServerStorageConfig](#apiv1beta1authserverstorageconfig)_ | Storage configures the storage backend for the embedded auth server.
If not specified, defaults to in-memory storage. | | Optional: \{\}
| +| `baselineClientScopes` _string array_ | BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be
included in every client registration. The embedded auth server unions these
scopes into the registered set returned by RFC 7591 Dynamic Client
Registration, so a client that narrows the `scope` field at /oauth/register
can still request the baseline scopes at /oauth/authorize. All values must
be present in the upstream-derived scopesSupported set; the auth server
fails to start if any value is missing.
Security: every client registered via /oauth/register will gain the
ability to request these scopes at /oauth/authorize, regardless of what
the client itself requested. Keep the baseline narrow (typically
"openid" and "offline_access"). Adding a privileged scope here — e.g.
"admin:read" — would grant it to every DCR-registered client, including
public clients like Claude Code, Cursor, and VS Code. | | MaxItems: 10
items:MinLength: 1
items:Pattern: `^[\x21\x23-\x5B\x5D-\x7E]+$`
Optional: \{\}
| #### api.v1beta1.EmbeddingResourceOverrides diff --git a/docs/server/docs.go b/docs/server/docs.go index e23c144d6d..77cc37cb5e 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -516,6 +516,14 @@ const docTemplate = `{ "description": "AuthorizationEndpointBaseURL overrides the base URL used for the authorization_endpoint\nin the OAuth discovery document. When set, the discovery document will advertise\n` + "`" + `{authorization_endpoint_base_url}/oauth/authorize` + "`" + ` instead of ` + "`" + `{issuer}/oauth/authorize` + "`" + `.\nAll other endpoints remain derived from the issuer.", "type": "string" }, + "baseline_client_scopes": { + "description": "BaselineClientScopes is a baseline set of OAuth 2.0 scopes unioned into every\nDCR registration. All values must appear in ScopesSupported; the auth server\nrejects this RunConfig at startup otherwise. Empty means current behavior is\npreserved (registered scope = client-requested, or DefaultScopes if empty).\nWhen ScopesSupported is empty, the subset check uses registration.DefaultScopes\n(the same set applyDefaults would substitute at startup) — so\nBaselineClientScopes containing standard OIDC scopes works without enumerating\nScopesSupported explicitly.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, "hmac_secret_files": { "description": "HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes\nand refresh tokens (opaque tokens).\nFirst file is the current secret (must be at least 32 bytes), subsequent files\nare for rotation/verification of existing tokens.\nIf empty, an ephemeral secret will be auto-generated (development only).", "items": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 0be473b5da..2514d0e0e4 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -509,6 +509,14 @@ "description": "AuthorizationEndpointBaseURL overrides the base URL used for the authorization_endpoint\nin the OAuth discovery document. When set, the discovery document will advertise\n`{authorization_endpoint_base_url}/oauth/authorize` instead of `{issuer}/oauth/authorize`.\nAll other endpoints remain derived from the issuer.", "type": "string" }, + "baseline_client_scopes": { + "description": "BaselineClientScopes is a baseline set of OAuth 2.0 scopes unioned into every\nDCR registration. All values must appear in ScopesSupported; the auth server\nrejects this RunConfig at startup otherwise. Empty means current behavior is\npreserved (registered scope = client-requested, or DefaultScopes if empty).\nWhen ScopesSupported is empty, the subset check uses registration.DefaultScopes\n(the same set applyDefaults would substitute at startup) — so\nBaselineClientScopes containing standard OIDC scopes works without enumerating\nScopesSupported explicitly.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, "hmac_secret_files": { "description": "HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes\nand refresh tokens (opaque tokens).\nFirst file is the current secret (must be at least 32 bytes), subsequent files\nare for rotation/verification of existing tokens.\nIf empty, an ephemeral secret will be auto-generated (development only).", "items": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 26bbbfb2c9..250d82d1a0 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -566,6 +566,20 @@ components: `{authorization_endpoint_base_url}/oauth/authorize` instead of `{issuer}/oauth/authorize`. All other endpoints remain derived from the issuer. type: string + baseline_client_scopes: + description: |- + BaselineClientScopes is a baseline set of OAuth 2.0 scopes unioned into every + DCR registration. All values must appear in ScopesSupported; the auth server + rejects this RunConfig at startup otherwise. Empty means current behavior is + preserved (registered scope = client-requested, or DefaultScopes if empty). + When ScopesSupported is empty, the subset check uses registration.DefaultScopes + (the same set applyDefaults would substitute at startup) — so + BaselineClientScopes containing standard OIDC scopes works without enumerating + ScopesSupported explicitly. + items: + type: string + type: array + uniqueItems: false hmac_secret_files: description: |- HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index e82f4bdab8..5a57047b7e 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -71,6 +71,17 @@ type RunConfig struct { // If empty, defaults to registration.DefaultScopes (["openid", "profile", "email", "offline_access"]). ScopesSupported []string `json:"scopes_supported,omitempty" yaml:"scopes_supported,omitempty"` + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes unioned into every + // DCR registration. All values must appear in ScopesSupported; the auth server + // rejects this RunConfig at startup otherwise. Empty means current behavior is + // preserved (registered scope = client-requested, or DefaultScopes if empty). + // When ScopesSupported is empty, the subset check uses registration.DefaultScopes + // (the same set applyDefaults would substitute at startup) — so + // BaselineClientScopes containing standard OIDC scopes works without enumerating + // ScopesSupported explicitly. + //nolint:lll // field tags require full JSON+YAML names + BaselineClientScopes []string `json:"baseline_client_scopes,omitempty" yaml:"baseline_client_scopes,omitempty"` + // AllowedAudiences is the list of valid resource URIs that tokens can be issued for. // Per RFC 8707, the "resource" parameter in authorization and token requests is // validated against this list. Required for MCP compliance. @@ -81,6 +92,32 @@ type RunConfig struct { Storage *storage.RunConfig `json:"storage,omitempty" yaml:"storage,omitempty"` } +// Validate checks that the on-disk RunConfig is internally consistent. Called +// by the runner before resolving secrets and building the runtime Config; it +// catches operator-supplied misconfiguration early so server startup fails +// loudly instead of degrading silently at runtime. +func (c *RunConfig) Validate() error { + return c.validateBaselineClientScopes() +} + +// validateBaselineClientScopes ensures every entry in BaselineClientScopes is +// also present in ScopesSupported. If a baseline scope is not advertised by +// ScopesSupported, the embedded DCR handler would later try to register a +// client with a scope the server does not support, which fosite rejects at +// /oauth/authorize with invalid_scope. +// +// When ScopesSupported is empty, the check uses registration.DefaultScopes as +// the superset (matching what applyDefaults would substitute at startup), so +// operators can omit ScopesSupported and still configure standard OIDC scopes +// as baseline without error. +func (c *RunConfig) validateBaselineClientScopes() error { + effective := c.ScopesSupported + if len(effective) == 0 { + effective = registration.DefaultScopes + } + return registration.ValidateScopeSubset(c.BaselineClientScopes, effective, "baseline_client_scopes") +} + // SigningKeyRunConfig configures where to load signing keys from. // Keys are loaded from PEM-encoded files on disk (typically mounted from secrets). type SigningKeyRunConfig struct { @@ -456,6 +493,16 @@ type Config struct { // /.well-known/oauth-authorization-server discovery endpoints. ScopesSupported []string + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes the embedded + // DCR handler unions into every newly registered client's scope set. Empty + // means current behavior is preserved (DCR registers exactly what the client + // requested, or registration.DefaultScopes if the client requested none). + // All entries must also be present in ScopesSupported. When ScopesSupported + // is empty, the validation gate uses registration.DefaultScopes as the + // superset — so standard OIDC scopes (e.g. "offline_access") work without + // enumerating ScopesSupported explicitly. + BaselineClientScopes []string + // AllowedAudiences is the list of valid resource URIs that tokens can be issued for. // Per RFC 8707, the "resource" parameter in authorization and token requests is // validated against this list. MCP clients are required to include the resource @@ -502,6 +549,22 @@ func (c *Config) Validate() error { return fmt.Errorf("at least one allowed audience is required for MCP compliance (RFC 8707 resource parameter validation)") } + // BaselineClientScopes must be a subset of ScopesSupported. RunConfig.Validate + // catches this for the YAML-loaded path, but a caller that constructs Config + // directly bypasses that; failing here gives them a clearer call stack than + // the inner validateParams in the provider layer. + // When ScopesSupported is empty, use DefaultScopes as the superset (matching + // what applyDefaults substitutes at startup). + { + effective := c.ScopesSupported + if len(effective) == 0 { + effective = registration.DefaultScopes + } + if err := registration.ValidateScopeSubset(c.BaselineClientScopes, effective, "baseline_client_scopes"); err != nil { + return err + } + } + slog.Debug("authserver config validation passed", "issuer", c.Issuer, "upstream_count", len(c.Upstreams), diff --git a/pkg/authserver/config_test.go b/pkg/authserver/config_test.go index 49245529b5..e07345be8e 100644 --- a/pkg/authserver/config_test.go +++ b/pkg/authserver/config_test.go @@ -9,8 +9,11 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/keys" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" "github.com/stacklok/toolhive/pkg/authserver/upstream" ) @@ -106,6 +109,11 @@ func TestConfigValidate(t *testing.T) { {name: "OIDC with oauth2_config set rejects", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: []UpstreamConfig{{Name: "test", Type: UpstreamProviderTypeOIDC, OIDCConfig: validOIDCUpstream, OAuth2Config: validUpstream}}, AllowedAudiences: []string{"https://mcp.example.com"}}, wantErr: true, errMsg: "oauth2_config must not be set"}, {name: "OAuth2 with oidc_config set rejects", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: []UpstreamConfig{{Name: "test", Type: UpstreamProviderTypeOAuth2, OAuth2Config: validUpstream, OIDCConfig: validOIDCUpstream}}, AllowedAudiences: []string{"https://mcp.example.com"}}, wantErr: true, errMsg: "oidc_config must not be set"}, + // BaselineClientScopes subset gate (mirrors RunConfig.Validate but on the + // runtime Config — catches direct constructors that bypass YAML loading). + {name: "baseline scope not in scopes_supported", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, ScopesSupported: []string{"openid"}, BaselineClientScopes: []string{"offline_access"}}, wantErr: true, errMsg: `baseline_client_scopes contains "offline_access"`}, + {name: "nil supported with baseline in DefaultScopes passes", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, ScopesSupported: nil, BaselineClientScopes: []string{"offline_access"}}}, + // Valid configs {name: "valid minimal", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}}}, {name: "valid nil key provider", config: Config{Issuer: "https://example.com", HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}}}, @@ -350,6 +358,64 @@ func TestOAuth2UpstreamRunConfigValidate(t *testing.T) { } } +func TestRunConfigValidate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config RunConfig + wantErr bool + errMsg string + }{ + { + name: "nil baseline scopes passes", + config: RunConfig{ScopesSupported: []string{"openid", "profile"}, BaselineClientScopes: nil}, + }, + { + name: "empty baseline scopes passes", + config: RunConfig{ScopesSupported: []string{"openid", "profile"}, BaselineClientScopes: []string{}}, + }, + { + name: "single baseline entry in supported set passes", + config: RunConfig{ScopesSupported: []string{"openid", "profile", "email"}, BaselineClientScopes: []string{"openid"}}, + }, + { + name: "all baseline entries in supported set passes", + config: RunConfig{ScopesSupported: []string{"openid", "profile", "email", "offline_access"}, BaselineClientScopes: []string{"openid", "offline_access"}}, + }, + { + name: "baseline contains scope not in supported rejects with specific error", + config: RunConfig{ScopesSupported: []string{"openid"}, BaselineClientScopes: []string{"openid", "offline_access"}}, + wantErr: true, + errMsg: `"offline_access" which is not in scopes_supported`, + }, + { + name: "nil supported with baseline in DefaultScopes passes", + config: RunConfig{ScopesSupported: nil, BaselineClientScopes: []string{"offline_access"}}, + }, + { + name: "nil supported with baseline outside DefaultScopes rejects", + config: RunConfig{ScopesSupported: nil, BaselineClientScopes: []string{"custom_scope"}}, + wantErr: true, + errMsg: `"custom_scope"`, + }, + { + name: "first missing scope is reported when multiple are missing", + config: RunConfig{ScopesSupported: []string{"openid"}, BaselineClientScopes: []string{"foo", "bar"}}, + wantErr: true, + errMsg: "foo", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.config.Validate() + assertError(t, err, tt.wantErr, tt.errMsg) + }) + } +} + func TestDCRUpstreamConfigValidate(t *testing.T) { t.Parallel() @@ -439,3 +505,61 @@ func TestDCRUpstreamConfigValidate(t *testing.T) { }) } } + +func TestConfigApplyDefaults_BaselineClientScopes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + scopesSupported []string + baselineClientScopes []string + wantErr bool + errMsg string + wantDefaultScopes bool + }{ + { + name: "empty scopes_supported and empty baseline — defaults substituted", + wantDefaultScopes: true, + }, + { + name: "scopes_supported set and empty baseline — no substitution", + scopesSupported: []string{"openid", "profile"}, + }, + { + name: "scopes_supported set and baseline non-empty — no substitution no error", + scopesSupported: []string{"openid", "profile"}, + baselineClientScopes: []string{"openid"}, + }, + { + name: "empty scopes_supported with non-empty baseline applies DefaultScopes", + baselineClientScopes: []string{"openid"}, + wantDefaultScopes: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cfg := &Config{ + ScopesSupported: tt.scopesSupported, + BaselineClientScopes: tt.baselineClientScopes, + } + + err := cfg.applyDefaults() + + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errMsg) + return + } + + require.NoError(t, err) + if tt.wantDefaultScopes { + require.Equal(t, registration.DefaultScopes, cfg.ScopesSupported) + } else { + require.Equal(t, tt.scopesSupported, cfg.ScopesSupported) + } + }) + } +} diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 9d67d06a43..6e72c011ea 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -11,6 +11,7 @@ import ( "log/slog" "net/http" "os" + "slices" "sync" "time" @@ -60,6 +61,14 @@ func NewEmbeddedAuthServer(ctx context.Context, cfg *authserver.RunConfig) (*Emb return nil, fmt.Errorf("config is required") } + // Fail loudly on operator-supplied misconfiguration (e.g. a baseline + // scope absent from scopes_supported) BEFORE touching storage or any + // other side-effecting work, so a bad config never reaches the network + // or filesystem. + if err := cfg.Validate(); err != nil { + return nil, fmt.Errorf("invalid run config: %w", err) + } + // Create the storage backend FIRST so the DCR resolver and the auth // server share the same persistence. Both MemoryStorage and RedisStorage // satisfy storage.DCRCredentialStore (verified by package-level var _ @@ -152,7 +161,15 @@ func newEmbeddedAuthServerWithStorage( return nil, fmt.Errorf("failed to build upstream configs: %w", err) } - // 6. Build the resolved Config + // 6. Build the resolved Config. + // + // Defensive copies of the scope/audience slices: cfg is operator-supplied + // input that may be retained or mutated by the caller (e.g. tests, a + // future hot-reload path). The DCR handler reads these slices on every + // request, so a mid-request mutation of the original would race. Cloning + // here once at the boundary lets all downstream stages share by reference + // safely. Cost is negligible — each slice is bounded by validation (≤10 + // for BaselineClientScopes, low cardinality in practice for the others). resolvedCfg := authserver.Config{ Issuer: cfg.Issuer, AuthorizationEndpointBaseURL: cfg.AuthorizationEndpointBaseURL, @@ -162,8 +179,9 @@ func newEmbeddedAuthServerWithStorage( RefreshTokenLifespan: refreshLifespan, AuthCodeLifespan: authCodeLifespan, Upstreams: upstreams, - ScopesSupported: cfg.ScopesSupported, - AllowedAudiences: cfg.AllowedAudiences, + ScopesSupported: slices.Clone(cfg.ScopesSupported), + BaselineClientScopes: slices.Clone(cfg.BaselineClientScopes), + AllowedAudiences: slices.Clone(cfg.AllowedAudiences), } // 7. Create the auth server. authserver.New also asserts the DCR diff --git a/pkg/authserver/server/handlers/dcr.go b/pkg/authserver/server/handlers/dcr.go index 6a720f1da2..03329e570e 100644 --- a/pkg/authserver/server/handlers/dcr.go +++ b/pkg/authserver/server/handlers/dcr.go @@ -7,6 +7,7 @@ import ( "encoding/json" "log/slog" "net/http" + "slices" "strings" "time" @@ -63,6 +64,33 @@ func (h *Handler) RegisterClientHandler(w http.ResponseWriter, req *http.Request return } + // Union with the operator-configured scope baseline. RFC 7591 §3.2.1 permits + // the AS to replace requested client metadata values during registration; we + // use that to expand the registered scope set so a client whose DCR request + // narrowed the scope field can still request the baseline at /oauth/authorize. + // h.config.BaselineClientScopes is validated at startup to be a subset of + // ScopesSupported, so the union is guaranteed to be a subset of advertised + // scopes. Operators should keep the baseline narrow (e.g. openid, + // offline_access) — every DCR-registered client gains the ability to request + // these scopes at /oauth/authorize regardless of what they registered with. + if len(h.config.BaselineClientScopes) > 0 { + effective := unionScopes(scopes, h.config.BaselineClientScopes) + if !slices.Equal(effective, scopes) { + // Baseline-driven expansion is the intended behavior whenever + // baseline_client_scopes is configured, so per-registration + // audit lives at Debug rather than Warn. Operator-visible + // signal that the baseline is in effect comes from a one-time + // Info log at server startup (NewAuthorizationServerConfig). + slog.Debug("DCR registered scope set expanded by baseline_client_scopes", + "client_name", validated.ClientName, + "requested", scopes, + "effective", effective, + "baseline", h.config.BaselineClientScopes, + ) + scopes = effective + } + } + // Generate client ID clientID := uuid.NewString() @@ -123,9 +151,12 @@ func (h *Handler) RegisterClientHandler(w http.ResponseWriter, req *http.Request slog.Debug("registered new DCR client", logAttrs...) // Build response per RFC 7591 Section 3.2.1. - // Scope reflects the scopes actually granted to this client (from - // ValidateScopes above), not all server-supported scopes. This lets - // the client know exactly which scopes it can request. + // Scope reflects the scopes actually granted to this client: the + // client-supplied scope set was validated against ScopesSupported by + // ValidateScopes above, then (if configured) unioned with + // BaselineClientScopes — which is itself guaranteed by startup-time + // validation to be a subset of ScopesSupported. The unioned set is NOT + // re-validated here. response := registration.DCRResponse{ ClientID: clientID, ClientIDIssuedAt: time.Now().Unix(), diff --git a/pkg/authserver/server/handlers/dcr_test.go b/pkg/authserver/server/handlers/dcr_test.go index 4ff1825b76..2a301dab43 100644 --- a/pkg/authserver/server/handlers/dcr_test.go +++ b/pkg/authserver/server/handlers/dcr_test.go @@ -155,6 +155,116 @@ func TestRegisterClientHandler_ScopeInResponse(t *testing.T) { "DCR response should include granted scopes per RFC 7591 Section 3.2.1") } +func TestRegisterClientHandler_BaselineClientScopes(t *testing.T) { + t.Parallel() + + // DefaultScopes is ["openid","profile","email","offline_access"]. + // Tests build ScopesSupported from DefaultScopes plus any extra scopes + // required by the case. + + tests := []struct { + name string + requestScope string + baselineClientScopes []string + extraScopesSupported []string // appended to DefaultScopes in ScopesSupported + expectedScopes []string + }{ + { + // When scope is empty, ValidateScopes returns DefaultScopes. + // The baseline adds "custom:scope" (not in DefaultScopes), so the + // union expands the set: DefaultScopes + ["custom:scope"]. + name: "empty client scope with non-empty baseline adds baseline scope", + requestScope: "", + baselineClientScopes: []string{"custom:scope"}, + extraScopesSupported: []string{"custom:scope"}, + expectedScopes: append(append([]string{}, registration.DefaultScopes...), "custom:scope"), + }, + { + // Requested scopes already contain the baseline; no expansion occurs. + name: "baseline is subset of requested scopes no expansion", + requestScope: "openid profile email offline_access", + baselineClientScopes: []string{"openid"}, + extraScopesSupported: nil, + expectedScopes: []string{"openid", "profile", "email", "offline_access"}, + }, + { + // Partial overlap: baseline shares "openid" with the request but adds + // "offline_access" not in the request. Exercises the dedup+append paths + // of unionScopes in the same handler call. + name: "partial overlap baseline appends only non-overlapping scopes", + requestScope: "openid profile", + baselineClientScopes: []string{"openid", "offline_access"}, + extraScopesSupported: nil, + expectedScopes: []string{"openid", "profile", "offline_access"}, + }, + { + // Canonical regression: client registers with "openid" only, + // baseline adds "offline_access" → union is both. + name: "disjoint baseline expands registered scope set", + requestScope: "openid", + baselineClientScopes: []string{"offline_access"}, + extraScopesSupported: nil, + expectedScopes: []string{"openid", "offline_access"}, + }, + { + // Nil baseline must not alter the registered scope set. + name: "nil baseline preserves existing behavior", + requestScope: "openid", + baselineClientScopes: nil, + extraScopesSupported: nil, + expectedScopes: []string{"openid"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + stor := mocks.NewMockStorage(ctrl) + var capturedClient fosite.Client + stor.EXPECT().RegisterClient(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, c fosite.Client) error { + capturedClient = c + return nil + }) + + // Defensive copy of DefaultScopes (a package-level var) before extending, + // so per-case extraScopesSupported never mutates the global. + scopesSupported := append(append([]string{}, registration.DefaultScopes...), tc.extraScopesSupported...) + cfg := &server.AuthorizationServerConfig{ + Config: &fosite.Config{AccessTokenIssuer: "https://test-authserver"}, + ScopesSupported: scopesSupported, + BaselineClientScopes: tc.baselineClientScopes, + } + handler := &Handler{storage: stor, config: cfg} + + reqBody, err := json.Marshal(registration.DCRRequest{ + RedirectURIs: []string{"http://127.0.0.1:8080/callback"}, + Scope: tc.requestScope, + }) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPost, "/oauth/register", bytes.NewReader(reqBody)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + + handler.RegisterClientHandler(w, req) + + require.Equal(t, http.StatusCreated, w.Code) + + var resp registration.DCRResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.Equal(t, registration.FormatScopes(tc.expectedScopes), resp.Scope, + "DCR response scope must equal the union of requested and baseline scopes") + + require.NotNil(t, capturedClient, "storage was not called") + assert.Equal(t, fosite.Arguments(tc.expectedScopes), capturedClient.GetScopes(), + "the union of requested and baseline scopes must reach storage") + }) + } +} + func TestRegisterClientHandler_ClientIsStored(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/server/handlers/scopes.go b/pkg/authserver/server/handlers/scopes.go new file mode 100644 index 0000000000..6f4705c25b --- /dev/null +++ b/pkg/authserver/server/handlers/scopes.go @@ -0,0 +1,38 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package handlers + +// unionScopes returns the union of requested and baseline scopes, preserving +// the order of requested first, then appending any baseline scopes not already +// present. Duplicates are removed. Returns nil when the result is empty. +// +// This is used by the DCR registration handler to inject an +// operator-configured scope baseline into the registered client's scope set: +// if a client narrows the scope field at /oauth/register, the baseline scopes +// are still part of the client's registered set so that the client can +// request them later at /oauth/authorize without invalid_scope rejection. +// +// Both inputs must already be validated by the caller (e.g. via ValidateScopes +// for client-supplied scopes). unionScopes does not filter empty strings or +// validate scope syntax — it only deduplicates and merges in stable order. +func unionScopes(requested, baseline []string) []string { + seen := make(map[string]bool, len(requested)+len(baseline)) + out := make([]string, 0, len(requested)+len(baseline)) + for _, s := range requested { + if !seen[s] { + seen[s] = true + out = append(out, s) + } + } + for _, s := range baseline { + if !seen[s] { + seen[s] = true + out = append(out, s) + } + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/pkg/authserver/server/handlers/scopes_test.go b/pkg/authserver/server/handlers/scopes_test.go new file mode 100644 index 0000000000..93ebf32dce --- /dev/null +++ b/pkg/authserver/server/handlers/scopes_test.go @@ -0,0 +1,115 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package handlers + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestUnionScopes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + req []string + baseline []string + want []string + }{ + { + name: "both nil returns nil", + req: nil, + baseline: nil, + want: nil, + }, + { + name: "both empty returns nil", + req: []string{}, + baseline: []string{}, + want: nil, + }, + { + name: "nil requested empty baseline returns nil", + req: nil, + baseline: []string{}, + want: nil, + }, + { + name: "requested only preserved unchanged", + req: []string{"openid", "profile"}, + baseline: nil, + want: []string{"openid", "profile"}, + }, + { + name: "baseline only returned when no requested", + req: nil, + baseline: []string{"openid", "email"}, + want: []string{"openid", "email"}, + }, + { + name: "requested subset of baseline: requested first then non-overlapping baseline", + req: []string{"openid"}, + baseline: []string{"openid", "profile", "email"}, + want: []string{"openid", "profile", "email"}, + }, + { + name: "disjoint sets: requested first then baseline", + req: []string{"openid", "profile"}, + baseline: []string{"email", "offline_access"}, + want: []string{"openid", "profile", "email", "offline_access"}, + }, + { + name: "exact match produces no duplicates", + req: []string{"openid", "profile"}, + baseline: []string{"openid", "profile"}, + want: []string{"openid", "profile"}, + }, + { + name: "duplicates in requested are deduplicated requested-first order preserved", + req: []string{"openid", "openid", "profile"}, + baseline: nil, + want: []string{"openid", "profile"}, + }, + { + name: "duplicates in baseline are deduplicated", + req: nil, + baseline: []string{"openid", "profile", "openid"}, + want: []string{"openid", "profile"}, + }, + { + name: "duplicates in both are deduplicated with requested-first order", + req: []string{"openid", "openid", "profile"}, + baseline: []string{"profile", "email", "email"}, + want: []string{"openid", "profile", "email"}, + }, + { + name: "no expansion when baseline is subset of requested (WARN not triggered in handler)", + req: []string{"openid", "profile", "email"}, + baseline: []string{"openid", "profile"}, + want: []string{"openid", "profile", "email"}, + }, + { + name: "multi-element requested with strict-superset baseline preserves requested order then appends", + req: []string{"openid", "profile"}, + baseline: []string{"openid", "profile", "email", "offline_access"}, + want: []string{"openid", "profile", "email", "offline_access"}, + }, + { + name: "empty-string entry in requested is passed through unchanged (precondition: caller must validate)", + req: []string{"", "openid"}, + baseline: nil, + want: []string{"", "openid"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := unionScopes(tt.req, tt.baseline) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/authserver/server/provider.go b/pkg/authserver/server/provider.go index 5eb5acee34..da0ec8764f 100644 --- a/pkg/authserver/server/provider.go +++ b/pkg/authserver/server/provider.go @@ -18,6 +18,7 @@ import ( "context" "crypto" "fmt" + "log/slog" "net/url" "strings" "time" @@ -26,6 +27,7 @@ import ( "github.com/ory/fosite" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" ) // Token lifespan bounds for validation. @@ -58,6 +60,10 @@ type AuthorizationServerConfig struct { // This is advertised in /.well-known/openid-configuration and // /.well-known/oauth-authorization-server discovery endpoints. ScopesSupported []string + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes the DCR handler + // unions into every newly registered client's scope set. All entries are + // guaranteed to be a subset of ScopesSupported. + BaselineClientScopes []string // AuthorizationEndpointBaseURL overrides the base URL for the authorization_endpoint // in the discovery document. When empty, defaults to the issuer (AccessTokenIssuer). AuthorizationEndpointBaseURL string @@ -89,6 +95,10 @@ type AuthorizationServerParams struct { AllowedAudiences []string // ScopesSupported lists the OAuth 2.0 scope values advertised in discovery documents. ScopesSupported []string + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes the DCR handler + // unions into every newly registered client's scope set. All entries are + // guaranteed to be a subset of ScopesSupported. + BaselineClientScopes []string // AuthorizationEndpointBaseURL overrides the base URL for the authorization_endpoint // in the discovery document. When empty, defaults to Issuer. AuthorizationEndpointBaseURL string @@ -189,7 +199,14 @@ func validateParams(cfg *AuthorizationServerParams) error { return fmt.Errorf("authorization endpoint base URL: %w", err) } } - return validateAllowedAudiences(cfg.AllowedAudiences) + if err := validateAllowedAudiences(cfg.AllowedAudiences); err != nil { + return err + } + // Defense-in-depth: re-check the baseline-⊆-scopes_supported invariant. + // RunConfig.Validate performs the same check at the operator-supplied + // wire-format boundary; this gate covers callers that construct + // AuthorizationServerParams programmatically and bypass that path. + return registration.ValidateScopeSubset(cfg.BaselineClientScopes, cfg.ScopesSupported, "baseline_client_scopes") } // NewAuthorizationServerConfig creates an AuthorizationServerConfig from the provided configuration. @@ -201,6 +218,12 @@ func NewAuthorizationServerConfig(cfg *AuthorizationServerParams) (*Authorizatio return nil, err } + if len(cfg.BaselineClientScopes) > 0 { + slog.Info("DCR registrations will be auto-granted baseline scopes", + "scopes", cfg.BaselineClientScopes, + ) + } + // Build JWK from signing key jwk := jose.JSONWebKey{ Key: cfg.SigningKey, @@ -231,6 +254,7 @@ func NewAuthorizationServerConfig(cfg *AuthorizationServerParams) (*Authorizatio SigningJWKS: &jose.JSONWebKeySet{Keys: []jose.JSONWebKey{jwk}}, AllowedAudiences: cfg.AllowedAudiences, ScopesSupported: cfg.ScopesSupported, + BaselineClientScopes: cfg.BaselineClientScopes, AuthorizationEndpointBaseURL: cfg.AuthorizationEndpointBaseURL, }, nil } diff --git a/pkg/authserver/server/provider_test.go b/pkg/authserver/server/provider_test.go index 02482fa34d..d633d412ee 100644 --- a/pkg/authserver/server/provider_test.go +++ b/pkg/authserver/server/provider_test.go @@ -311,6 +311,26 @@ func TestNewAuthorizationServerConfig_InvalidConfig(t *testing.T) { }, wantErr: "rotated HMAC secret [1] must be at least 32 bytes", }, + { + // Defense-in-depth gate: a direct caller of NewAuthorizationServerConfig + // that bypasses RunConfig.Validate must still be rejected when + // BaselineClientScopes contains a scope outside ScopesSupported. + // Without this case, only the operator-YAML path is regression-protected. + name: "baseline scope not in scopes_supported", + params: &AuthorizationServerParams{ + Issuer: "https://auth.example.com", + AccessTokenLifespan: time.Hour, + RefreshTokenLifespan: time.Hour * 24, + AuthCodeLifespan: time.Minute * 10, + HMACSecrets: servercrypto.NewHMACSecrets([]byte("test-secret-with-32-bytes-long!!")), + SigningKeyID: "key-1", + SigningKeyAlgorithm: "RS256", + SigningKey: rsaKey, + ScopesSupported: []string{"openid"}, + BaselineClientScopes: []string{"offline_access"}, + }, + wantErr: `baseline_client_scopes contains "offline_access"`, + }, } for _, tt := range tests { diff --git a/pkg/authserver/server/registration/dcr.go b/pkg/authserver/server/registration/dcr.go index 6d2c1d1068..f33b3db4ee 100644 --- a/pkg/authserver/server/registration/dcr.go +++ b/pkg/authserver/server/registration/dcr.go @@ -18,12 +18,40 @@ package registration import ( + "fmt" "slices" "strings" "github.com/stacklok/toolhive/pkg/oauthproto" ) +// ValidateScopeSubset checks that every scope in subset is also present in +// superset, returning an error that names fieldName and the offending scope. +// +// Shared across the layers that validate baseline-scope configuration so the +// error message format is identical wherever the violation is caught (a +// caller using YAML-loaded config and a caller constructing config +// programmatically both see the same wording). +// +// fieldName should be the wire-format or display name of the field being +// validated (e.g. "baseline_client_scopes"). It is embedded verbatim in the +// returned error. +func ValidateScopeSubset(subset, superset []string, fieldName string) error { + if len(subset) == 0 { + return nil + } + supported := make(map[string]bool, len(superset)) + for _, s := range superset { + supported[s] = true + } + for _, s := range subset { + if !supported[s] { + return fmt.Errorf("%s contains %q which is not in scopes_supported", fieldName, s) + } + } + return nil +} + // DCR error codes per RFC 7591 Section 3.2.2 const ( // DCRErrorInvalidRedirectURI indicates that the value of one or more diff --git a/pkg/authserver/server/registration/dcr_test.go b/pkg/authserver/server/registration/dcr_test.go index 2e99b321cf..132bb143f3 100644 --- a/pkg/authserver/server/registration/dcr_test.go +++ b/pkg/authserver/server/registration/dcr_test.go @@ -615,3 +615,82 @@ func TestDefaultGrantTypesAndResponseTypes(t *testing.T) { // Verify default response types include code assert.Contains(t, defaultResponseTypes, "code") } + +func TestValidateScopeSubset(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + subset []string + superset []string + fieldName string + wantErr bool + errMsg string + }{ + { + name: "nil subset passes", + subset: nil, + superset: []string{"openid", "profile"}, + fieldName: "baseline_client_scopes", + }, + { + name: "empty subset passes", + subset: []string{}, + superset: []string{"openid", "profile"}, + fieldName: "baseline_client_scopes", + }, + { + name: "all subset entries present in superset passes", + subset: []string{"openid", "profile"}, + superset: []string{"openid", "profile", "email", "offline_access"}, + fieldName: "baseline_client_scopes", + }, + { + name: "single entry not in superset returns error", + subset: []string{"offline_access"}, + superset: []string{"openid"}, + fieldName: "baseline_client_scopes", + wantErr: true, + errMsg: `baseline_client_scopes contains "offline_access" which is not in scopes_supported`, + }, + { + name: "first offending entry reported in error", + subset: []string{"foo", "bar"}, + superset: []string{"openid"}, + fieldName: "baseline_client_scopes", + wantErr: true, + errMsg: `baseline_client_scopes contains "foo" which is not in scopes_supported`, + }, + { + name: "non-nil subset with nil superset returns error", + subset: []string{"openid"}, + superset: nil, + fieldName: "baseline_client_scopes", + wantErr: true, + errMsg: `baseline_client_scopes contains "openid" which is not in scopes_supported`, + }, + { + name: "fieldName is embedded in error message", + subset: []string{"missing"}, + superset: []string{"openid"}, + fieldName: "my_custom_field", + wantErr: true, + errMsg: `my_custom_field contains "missing" which is not in scopes_supported`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateScopeSubset(tt.subset, tt.superset, tt.fieldName) + + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMsg) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/authserver/server_impl.go b/pkg/authserver/server_impl.go index 55fcde584a..83d063adc5 100644 --- a/pkg/authserver/server_impl.go +++ b/pkg/authserver/server_impl.go @@ -131,6 +131,7 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage, opts ...se SigningKeyAlgorithm: signingKey.Algorithm, SigningKey: signingKey.Key, ScopesSupported: cfg.ScopesSupported, + BaselineClientScopes: cfg.BaselineClientScopes, AllowedAudiences: cfg.AllowedAudiences, AuthorizationEndpointBaseURL: cfg.AuthorizationEndpointBaseURL, } diff --git a/test/integration/authserver/authserver_integration_test.go b/test/integration/authserver/authserver_integration_test.go index 90fd677385..2fd8e22ef2 100644 --- a/test/integration/authserver/authserver_integration_test.go +++ b/test/integration/authserver/authserver_integration_test.go @@ -9,7 +9,9 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/x509" + "encoding/json" "encoding/pem" + "io" "net/http" "net/http/httptest" "net/url" @@ -466,6 +468,147 @@ func TestEmbeddedAuthServer_ResourceCleanup(t *testing.T) { require.NoError(t, err) } +// TestEmbeddedAuthServer_BaselineClientScopes_RegressionForDCRScopeNarrowing +// is a regression test for the Claude Code DCR scope-narrowing bug +// (anthropics/claude-code#4540). Claude Code registers with a narrowed scope +// (e.g. "openid") but later requests the full set at /oauth/authorize. +// The fix unions BaselineClientScopes into every DCR registration so the +// client's registered set always includes the operator-configured baseline, +// preventing fosite from rejecting the subsequent authorize request with +// invalid_scope. +// +//nolint:paralleltest,tparallel // Subtests intentionally sequential - second reuses first's client_id +func TestEmbeddedAuthServer_BaselineClientScopes_RegressionForDCRScopeNarrowing(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + upstream := helpers.NewMockUpstreamIDP(t) + + cfg := helpers.NewTestAuthServerConfig(t, upstream.URL(), + helpers.WithScopesSupported([]string{"openid", "offline_access"}), + helpers.WithBaselineClientScopes([]string{"offline_access"}), + ) + + authServer := helpers.NewEmbeddedAuthServer(ctx, t, cfg) + + server := httptest.NewServer(authServer.Handler()) + t.Cleanup(server.Close) + + client := helpers.NewOAuthClient(server.URL) + + var clientID string + + t.Run("DCR response echoes the baseline-augmented scope set", func(t *testing.T) { + clientMetadata := map[string]interface{}{ + "client_name": "Claude Code", + "redirect_uris": []string{"http://localhost:8080/callback"}, + "grant_types": []string{"authorization_code", "refresh_token"}, + // Narrow scope — the bug-trigger pattern from Claude Code + "scope": "openid", + } + + result, statusCode, err := client.RegisterClient(clientMetadata) + require.NoError(t, err) + require.Equal(t, http.StatusCreated, statusCode, "DCR registration should succeed") + + clientID = result["client_id"].(string) + require.NotEmpty(t, clientID) + + // The registered scope set must include "offline_access" from the baseline + // even though the client only requested "openid". + registeredScope, ok := result["scope"].(string) + require.True(t, ok, "scope field should be a string in DCR response") + // Order: requested scopes first, then non-overlapping baseline (unionScopes contract). + assert.Equal(t, "openid offline_access", registeredScope, + "DCR response scope must be the union of requested+baseline scopes") + }) + + t.Run("authorize accepts a request for the unioned scope set", func(t *testing.T) { + // Pre-fix: fosite would reject this with invalid_scope because the + // registered client only had "openid" in its scope set. Post-fix: the + // client has "openid offline_access" so the authorize request succeeds. + params := url.Values{ + "response_type": {"code"}, + "client_id": {clientID}, + "redirect_uri": {"http://localhost:8080/callback"}, + "scope": {"openid offline_access"}, + "state": {"test-state-baseline-regression"}, + "resource": {cfg.AllowedAudiences[0]}, + } + + resp, err := client.StartAuthorization(params) + require.NoError(t, err) + defer func() { + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + }() + + // Must redirect to upstream — NOT a 400 invalid_scope. + assert.Equal(t, http.StatusFound, resp.StatusCode, + "authorize must accept the full scope set that includes the baseline; pre-fix this returned 400 invalid_scope") + + location := resp.Header.Get("Location") + assert.NotEmpty(t, location) + + redirectURL, err := url.Parse(location) + require.NoError(t, err) + assert.Contains(t, redirectURL.String(), upstream.URL()) + }) + + t.Run("authorize rejects a scope not in scopes_supported", func(t *testing.T) { + // Negative case: even with the baseline expansion, scopes that aren't + // in ScopesSupported must still be rejected. This guards against silent + // privilege escalation if BaselineClientScopes ever drifts. + params := url.Values{ + "response_type": {"code"}, + "client_id": {clientID}, + "redirect_uri": {"http://localhost:8080/callback"}, + "scope": {"openid offline_access admin:read"}, + "state": {"test-state-baseline-negative"}, + "resource": {cfg.AllowedAudiences[0]}, + } + + resp, err := client.StartAuthorization(params) + require.NoError(t, err) + // Read the body up front: the 400 branch below needs to parse it, + // and reading once is simpler than teeing around a deferred drain. + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + // Fosite rejects with invalid_scope. Depending on whether the redirect URI + // has been validated by that point, this surfaces either as a 400 with + // a JSON body or as a redirect (3xx) to redirect_uri with + // error=invalid_scope in the query. Both must carry `invalid_scope` — + // asserting on the error code in both branches guards against a future + // fosite upgrade swapping in a different code (e.g. server_error) + // while the privilege-escalation regression silently slips through. + if resp.StatusCode >= http.StatusMultipleChoices && resp.StatusCode < http.StatusBadRequest { + // Redirect-with-error case — verify it points to the registered + // redirect_uri (loopback), NOT the upstream. + location := resp.Header.Get("Location") + require.NotEmpty(t, location) + assert.NotContains(t, location, upstream.URL(), + "rejected request must NOT redirect to the upstream IDP") + redirectURL, err := url.Parse(location) + require.NoError(t, err) + assert.Contains(t, redirectURL.RawQuery, "invalid_scope", + "rejection error must be invalid_scope") + } else { + require.Equal(t, http.StatusBadRequest, resp.StatusCode, + "unsupported scope must produce 400 invalid_scope (or redirect-with-error)") + var errResp struct { + Error string `json:"error"` + } + require.NoError(t, json.Unmarshal(body, &errResp), + "400 response body must be JSON with an error field") + assert.Equal(t, "invalid_scope", errResp.Error, + "400 rejection must carry the invalid_scope error code, not a different one") + } + }) +} + // generateTestECKey generates a test EC private key for signing. func generateTestECKey(t *testing.T) []byte { t.Helper() diff --git a/test/integration/authserver/helpers/authserver.go b/test/integration/authserver/helpers/authserver.go index ec18868660..54b34ba779 100644 --- a/test/integration/authserver/helpers/authserver.go +++ b/test/integration/authserver/helpers/authserver.go @@ -21,13 +21,14 @@ type AuthServerOption func(*authServerConfig) // authServerConfig holds configuration for creating a test auth server. type authServerConfig struct { - issuer string - upstreams []authserver.UpstreamRunConfig - allowedAudiences []string - signingKeyConfig *authserver.SigningKeyRunConfig - hmacSecretFiles []string - tokenLifespans *authserver.TokenLifespanRunConfig - scopesSupported []string + issuer string + upstreams []authserver.UpstreamRunConfig + allowedAudiences []string + signingKeyConfig *authserver.SigningKeyRunConfig + hmacSecretFiles []string + tokenLifespans *authserver.TokenLifespanRunConfig + scopesSupported []string + baselineClientScopes []string } // WithIssuer sets the issuer URL. @@ -79,6 +80,14 @@ func WithScopesSupported(scopes []string) AuthServerOption { } } +// WithBaselineClientScopes sets the baseline client scopes that are unioned +// into every DCR registration response regardless of what the client requested. +func WithBaselineClientScopes(scopes []string) AuthServerOption { + return func(c *authServerConfig) { + c.baselineClientScopes = scopes + } +} + // GetFreePort returns an available TCP port on localhost. func GetFreePort(tb testing.TB) int { tb.Helper() @@ -130,14 +139,15 @@ func NewTestAuthServerConfig(tb testing.TB, upstreamURL string, opts ...AuthServ } return &authserver.RunConfig{ - SchemaVersion: authserver.CurrentSchemaVersion, - Issuer: cfg.issuer, - SigningKeyConfig: cfg.signingKeyConfig, - HMACSecretFiles: cfg.hmacSecretFiles, - TokenLifespans: cfg.tokenLifespans, - Upstreams: cfg.upstreams, - ScopesSupported: cfg.scopesSupported, - AllowedAudiences: cfg.allowedAudiences, + SchemaVersion: authserver.CurrentSchemaVersion, + Issuer: cfg.issuer, + SigningKeyConfig: cfg.signingKeyConfig, + HMACSecretFiles: cfg.hmacSecretFiles, + TokenLifespans: cfg.tokenLifespans, + Upstreams: cfg.upstreams, + ScopesSupported: cfg.scopesSupported, + BaselineClientScopes: cfg.baselineClientScopes, + AllowedAudiences: cfg.allowedAudiences, } }