From ee7d4cc20c64b9f1ae9b4f4bf22eb1321b5e6967 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 23 Apr 2026 15:41:57 -0700 Subject: [PATCH 1/4] Expose DCR config in operator CRD for OAuth2 upstreams Implements changes for issue #5040 (Phase 2 DCR CRD surface): - Add DCRUpstreamConfig CRD type (discoveryUrl, registrationEndpoint, initialAccessTokenRef, softwareId, softwareStatement) and a new dcrConfig field on OAuth2UpstreamConfig so Kubernetes users can configure RFC 7591 Dynamic Client Registration on upstream providers. - Make OAuth2UpstreamConfig.clientId optional and add CEL validation requiring exactly one of clientId or dcrConfig, and exactly one of discoveryUrl or registrationEndpoint inside dcrConfig. Mirror the checks at runtime via validateOAuth2DCRConfig for defense-in-depth. - Wire the conversion in controllerutil/authserver.go so DCRConfig is mapped onto authserver.DCRUpstreamConfig. InitialAccessTokenRef is resolved to an env var (TOOLHIVE_UPSTREAM_DCR_INITIAL_ACCESS_TOKEN_*) populated from the referenced Secret, mirroring the ClientSecretRef pattern. Extract small helpers for env-var generation to keep cyclomatic complexity within lint limits. - Regenerate zz_generated.deepcopy.go, CRD YAML manifests, and CRD API reference docs. - Add table-driven validation tests covering DCR+ClientID conflict, both endpoints set, neither endpoint set, valid single-endpoint cases, and neither-auth configuration. Add conversion tests covering DCR discoveryUrl/registrationEndpoint paths and initial-access-token env var wiring. Co-Authored-By: Claude Opus 4.7 (1M context) Address code review feedback Fixed issues from code review of the DCR CRD surface commit: - CRITICAL: CEL markers contained a Unicode smart quote (U+201D) that gofmt's doc-comment formatter reintroduced on every lint-fix. Rewrote both markers to use CEL's size(...) > 0 idiom instead of `!= ''`, which sidesteps the typographic normalization entirely and keeps regeneration idempotent. Verified no U+2018-U+201F characters remain in source or CRDs. - HIGH: buildUpstreamRunConfig now calls the exported mcpv1beta1.ValidateOAuth2DCRConfig before producing a RunConfig, so malformed ClientID/DCRConfig pairs that bypass admission fail at reconcile time rather than at authserver startup. Error propagation threaded through BuildAuthServerRunConfig; split OIDC and OAuth2 branches into helpers to stay under the gocyclo limit. - HIGH: Added table case exercising validateUpstreamProvider rejection of an OIDC-typed provider whose OAuth2Config carries a DCRConfig. - MEDIUM: Added kubebuilder CEL XValidation on UpstreamProviderConfig enforcing oidcConfig/oauth2Config mutual exclusivity paired to the declared type, closing the silent-pod-failure YAML-apply gap. - MEDIUM: Added MaxLength=255 to SoftwareID and MaxLength=4096 to SoftwareStatement to prevent unbounded input from inflating CRs beyond etcd object limits. - MEDIUM: Pinned the "neither ClientID nor DCRConfig" error assertion to the scoped `oauth2Config:` prefix; added a regression case exercising the non-DCR OAuth2 path (ClientID only, DCRConfig nil); added a new TestBuildAuthServerRunConfig_InvalidDCR suite covering all four invalid DCR/ClientID pairings at the conversion layer. - MEDIUM: Renamed UpstreamDCRInitialAccessTokenEnvVar to UpstreamDCRInitialAccessTokenEnvVarPrefix and expanded the godoc on both prefix constants to show the resolved _ form. All task lint/lint-fix/license-check pass; regenerated CRDs and deepcopy are idempotent; affected unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) Address iteration-2 review feedback Polish items raised in the second review pass: - MEDIUM: Trim duplicate upstream name from reconcile-time DCR validation errors. Added scopedFieldPath() helper in cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go so ValidateOAuth2DCRConfig prepends a dotted prefix only when one is given, and the conversion call site now passes an empty prefix so BuildAuthServerRunConfig's outer "upstream %q: %w" wrap is the only mention of the upstream name. Strengthened TestBuildAuthServerRunConfig_InvalidDCR to assert the upstream name appears exactly once in the error string. - MEDIUM: Make the UpstreamProviderConfig CEL rule fail closed for unrecognized future provider types. Restructured the rule from a binary discriminator into a chain of equality checks ending in an explicit `false`, and updated the message to "type must be 'oidc' or 'oauth2'; ...". Added a contributor-facing doc comment reminding future authors to extend both the rule and validateUpstreamProvider when adding a new UpstreamProviderType. - MEDIUM: Refresh the godoc on extractUpstreamSecretRefs to describe the actual invariants that hold post-CEL: OIDC providers can only return a clientSecretRef; OAuth2 providers can return both independently; other (currently unreachable) types return nil/nil. Cross-linked to the CEL rule and noted that BuildAuthServerRunConfig is the reconcile-time backstop callers should not rely on this helper to enforce. Regenerated CRD YAMLs and crd-api.md prose. task lint, lint-fix, license-check, and the affected unit tests pass; regeneration is idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 156 ++++++++- .../mcpexternalauthconfig_types_test.go | 119 +++++++ .../api/v1beta1/zz_generated.deepcopy.go | 25 ++ .../pkg/controllerutil/authserver.go | 313 ++++++++++++----- .../pkg/controllerutil/authserver_test.go | 320 ++++++++++++++++++ ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 206 ++++++++++- ...olhive.stacklok.dev_virtualmcpservers.yaml | 206 ++++++++++- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 206 ++++++++++- ...olhive.stacklok.dev_virtualmcpservers.yaml | 206 ++++++++++- docs/operator/crd-api.md | 53 ++- 10 files changed, 1674 insertions(+), 136 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index fb24749204..b3730398a0 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -273,6 +273,21 @@ const ( ) // UpstreamProviderConfig defines configuration for an upstream Identity Provider. +// +// Exactly one of OIDCConfig or OAuth2Config must be set and must match the +// declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers +// set OAuth2Config. The CEL rule below enforces the pairing at admission; the +// matching Go-level check in validateUpstreamProvider provides defense-in-depth +// for stored objects. +// +// The rule is structured as a chain of equality checks ending in an explicit +// `false`, so adding a new UpstreamProviderType value without extending this +// rule fails admission instead of silently demanding the OAuth2 shape. When +// adding a new type, extend both this rule and validateUpstreamProvider. +// +// +kubebuilder:validation:XValidation:rule="self.type == 'oidc' ? (has(self.oidcConfig) && !has(self.oauth2Config)) : self.type == 'oauth2' ? (has(self.oauth2Config) && !has(self.oidcConfig)) : false",message="type must be 'oidc' or 'oauth2'; oidcConfig must be set when type is 'oidc' and oauth2Config must be set when type is 'oauth2' (and the other must not be set)" +// +//nolint:lll // CEL validation rules exceed line length limit type UpstreamProviderConfig struct { // Name uniquely identifies this upstream provider. // Used for routing decisions and session binding in multi-upstream scenarios. @@ -354,6 +369,14 @@ type OIDCUpstreamConfig struct { // OAuth2UpstreamConfig contains configuration for pure OAuth 2.0 providers. // OAuth 2.0 providers require explicit endpoint configuration. +// +// Exactly one of ClientID or DCRConfig must be set: ClientID is used when the +// client is pre-provisioned out of band, DCRConfig enables RFC 7591 Dynamic +// Client Registration at runtime. +// +// +kubebuilder:validation:XValidation:rule="(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)",message="exactly one of clientId or dcrConfig must be set" +// +//nolint:lll // CEL validation rules exceed line length limit type OAuth2UpstreamConfig struct { // AuthorizationEndpoint is the URL for the OAuth authorization endpoint. // +kubebuilder:validation:Required @@ -374,8 +397,10 @@ type OAuth2UpstreamConfig struct { UserInfo *UserInfoConfig `json:"userInfo,omitempty"` // ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. - // +kubebuilder:validation:Required - ClientID string `json:"clientId"` + // Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + // at runtime via RFC 7591 Dynamic Client Registration and must be left empty. + // +optional + ClientID string `json:"clientId,omitempty"` // ClientSecretRef references a Kubernetes Secret containing the OAuth 2.0 client secret. // Optional for public clients using PKCE instead of client secret. @@ -410,6 +435,81 @@ type OAuth2UpstreamConfig struct { // +kubebuilder:validation:MaxProperties=16 // +optional AdditionalAuthorizationParams map[string]string `json:"additionalAuthorizationParams,omitempty"` + + // DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + // authorization server. When set, the client credentials are obtained at + // runtime rather than being pre-provisioned, and ClientID must be left empty. + // Mutually exclusive with ClientID. + // +optional + DCRConfig *DCRUpstreamConfig `json:"dcrConfig,omitempty"` +} + +// DCRUpstreamConfig configures RFC 7591 Dynamic Client Registration for an +// OAuth 2.0 upstream. When present on an OAuth2 upstream, the authserver +// performs registration at runtime to obtain client credentials, replacing +// the need to pre-provision a ClientID. +// +// Exactly one of DiscoveryURL or RegistrationEndpoint must be set. DiscoveryURL +// points at an RFC 8414 / OIDC Discovery document from which the registration +// endpoint is resolved; RegistrationEndpoint is used directly when the upstream +// does not publish discovery metadata. +// +// The XOR rule uses has() alone (not has() + size() > 0) to keep the rule's +// estimated CEL cost under the apiserver's per-rule static budget. With +// `omitempty` on both fields, an unset field is absent on the wire and has() +// returns false; the explicit-empty-string edge case is rejected at reconcile +// time by ValidateOAuth2DCRConfig. +// +// +kubebuilder:validation:XValidation:rule="has(self.discoveryUrl) != has(self.registrationEndpoint)",message="exactly one of discoveryUrl or registrationEndpoint must be set" +// +//nolint:lll // CEL validation rules exceed line length limit +type DCRUpstreamConfig struct { + // DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + // issues a single GET against this URL (no well-known-path fallback) and + // reads registration_endpoint, authorization_endpoint, token_endpoint, + // token_endpoint_auth_methods_supported, and scopes_supported from the + // response. + // Mutually exclusive with RegistrationEndpoint. + // MaxLength bounds CEL cost estimation on the parent struct's + // XValidation rule and matches the conventional URL length cap. + // +optional + // +kubebuilder:validation:Pattern=`^https?://.*$` + // +kubebuilder:validation:MaxLength=2048 + DiscoveryURL string `json:"discoveryUrl,omitempty"` + + // RegistrationEndpoint is the RFC 7591 registration endpoint URL used + // directly, bypassing discovery. When using this field, the caller is + // expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + // explicit Scopes list on the parent OAuth2UpstreamConfig. + // Mutually exclusive with DiscoveryURL. + // MaxLength bounds CEL cost estimation on the parent struct's + // XValidation rule and matches the conventional URL length cap. + // +optional + // +kubebuilder:validation:Pattern=`^https?://.*$` + // +kubebuilder:validation:MaxLength=2048 + RegistrationEndpoint string `json:"registrationEndpoint,omitempty"` + + // InitialAccessTokenRef is an optional reference to a Kubernetes Secret + // carrying an RFC 7591 §3 initial access token. When set, the resolver + // presents the token value as a Bearer credential on the registration + // request. Mirrors the ClientSecretRef pattern. + // +optional + InitialAccessTokenRef *SecretKeyRef `json:"initialAccessTokenRef,omitempty"` + + // SoftwareID is the RFC 7591 "software_id" registration metadata value, + // identifying the client software independent of any particular + // registration instance. Typically a UUID or short identifier. + // +optional + // +kubebuilder:validation:MaxLength=255 + SoftwareID string `json:"softwareId,omitempty"` + + // SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + // metadata about the client software, signed by a party the authorization + // server trusts. Bounded to 4096 characters to prevent unbounded + // user input from inflating the CR beyond etcd object limits. + // +optional + // +kubebuilder:validation:MaxLength=4096 + SoftwareStatement string `json:"softwareStatement,omitempty"` } // TokenResponseMapping maps non-standard token response fields to standard OAuth 2.0 fields @@ -970,10 +1070,62 @@ func (*MCPExternalAuthConfig) validateUpstreamProvider(index int, provider *Upst return fmt.Errorf("%s: unsupported provider type: %s", prefix, provider.Type) } + // Validate OAuth2-specific DCR / ClientID constraints (defense-in-depth with CEL). + if provider.Type == UpstreamProviderTypeOAuth2 && provider.OAuth2Config != nil { + if err := ValidateOAuth2DCRConfig(prefix, provider.OAuth2Config); err != nil { + return err + } + } + // Validate additionalAuthorizationParams does not contain reserved keys return ValidateAdditionalAuthorizationParams(prefix, provider.AdditionalAuthorizationParams()) } +// ValidateOAuth2DCRConfig enforces the mutual exclusivity between ClientID and +// DCRConfig and, when DCRConfig is present, between DiscoveryURL and +// RegistrationEndpoint. These rules mirror the CEL validation on +// OAuth2UpstreamConfig and DCRUpstreamConfig and provide defense-in-depth for +// stored objects (e.g., objects stored before CEL rules were added or +// validated through code paths that bypass admission). +// +// The prefix is embedded in error messages to identify the offending upstream +// (e.g., "upstreamProviders[i]"). Pass an empty string when the caller already +// wraps the error with the upstream identifier, to avoid duplicating it. +// Exported so the controllerutil conversion layer can reuse the same +// invariants when building runtime configs, rejecting malformed objects at +// reconcile time rather than waiting until the authserver process parses them. +func ValidateOAuth2DCRConfig(prefix string, cfg *OAuth2UpstreamConfig) error { + hasClientID := cfg.ClientID != "" + hasDCR := cfg.DCRConfig != nil + + scoped := scopedFieldPath(prefix, "oauth2Config") + if hasClientID == hasDCR { + return fmt.Errorf("%s: exactly one of clientId or dcrConfig must be set", scoped) + } + + if !hasDCR { + return nil + } + + hasDiscoveryURL := cfg.DCRConfig.DiscoveryURL != "" + hasRegistrationEndpoint := cfg.DCRConfig.RegistrationEndpoint != "" + if hasDiscoveryURL == hasRegistrationEndpoint { + return fmt.Errorf("%s.dcrConfig: exactly one of discoveryUrl or registrationEndpoint must be set", scoped) + } + return nil +} + +// scopedFieldPath joins a non-empty prefix to a field name with a dot. When +// the prefix is empty, it returns just the field name so callers that already +// supply their own scope (e.g., a wrapping `fmt.Errorf("upstream %q: %w", ...)`) +// don't end up with a leading dot. +func scopedFieldPath(prefix, field string) string { + if prefix == "" { + return field + } + return prefix + "." + field +} + // AdditionalAuthorizationParams returns the additional authorization parameters // from whichever upstream config is set, or nil if none. func (p *UpstreamProviderConfig) AdditionalAuthorizationParams() map[string]string { diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go index 235d74a826..09e92438fc 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go @@ -571,6 +571,125 @@ func TestMCPExternalAuthConfig_validateUpstreamProvider(t *testing.T) { }, expectErr: false, }, + { + name: "OAuth2 provider with valid DCRConfig (discoveryUrl only)", + provider: UpstreamProviderConfig{ + Name: "dcr-discovery", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + }, + expectErr: false, + }, + { + name: "OAuth2 provider with valid DCRConfig (registrationEndpoint only)", + provider: UpstreamProviderConfig{ + Name: "dcr-endpoint", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + Scopes: []string{"openid"}, + DCRConfig: &DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + }, + }, + }, + expectErr: false, + }, + { + name: "OAuth2 provider with DCRConfig and non-empty ClientID", + provider: UpstreamProviderConfig{ + Name: "dcr-with-clientid", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + ClientID: "pre-provisioned-id", + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + }, + expectErr: true, + errMsg: "exactly one of clientId or dcrConfig must be set", + }, + { + name: "OAuth2 provider with DCRConfig specifying both endpoints", + provider: UpstreamProviderConfig{ + Name: "dcr-both-endpoints", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + RegistrationEndpoint: "https://idp.example.com/register", + }, + }, + }, + expectErr: true, + errMsg: "exactly one of discoveryUrl or registrationEndpoint must be set", + }, + { + name: "OAuth2 provider with DCRConfig specifying neither endpoint", + provider: UpstreamProviderConfig{ + Name: "dcr-neither-endpoint", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{}, + }, + }, + expectErr: true, + errMsg: "exactly one of discoveryUrl or registrationEndpoint must be set", + }, + { + name: "OAuth2 provider with neither ClientID nor DCRConfig", + provider: UpstreamProviderConfig{ + Name: "oauth2-missing-auth", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + }, + }, + expectErr: true, + // Pin the scoped prefix so a rename of the oauth2Config label surfaces as a test failure. + errMsg: "oauth2Config: exactly one of clientId or dcrConfig must be set", + }, + { + // Regression guard: conversion only maps DCRConfig in the OAuth2 + // branch, so an OIDC-typed provider carrying OAuth2Config/DCRConfig + // must be rejected at validate time rather than silently dropped. + name: "OIDC provider with OAuth2Config carrying DCRConfig is rejected", + provider: UpstreamProviderConfig{ + Name: "mismatched-oidc", + Type: UpstreamProviderTypeOIDC, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + }, + expectErr: true, + errMsg: "oidcConfig must be set when type is 'oidc' and must not be set otherwise", + }, } for _, tt := range tests { diff --git a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go index 90dc52121b..b27e168797 100644 --- a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go +++ b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go @@ -204,6 +204,26 @@ func (in *ConfigMapAuthzRef) DeepCopy() *ConfigMapAuthzRef { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DCRUpstreamConfig) DeepCopyInto(out *DCRUpstreamConfig) { + *out = *in + if in.InitialAccessTokenRef != nil { + in, out := &in.InitialAccessTokenRef, &out.InitialAccessTokenRef + *out = new(SecretKeyRef) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DCRUpstreamConfig. +func (in *DCRUpstreamConfig) DeepCopy() *DCRUpstreamConfig { + if in == nil { + return nil + } + out := new(DCRUpstreamConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EmbeddedAuthServerConfig) DeepCopyInto(out *EmbeddedAuthServerConfig) { *out = *in @@ -2030,6 +2050,11 @@ func (in *OAuth2UpstreamConfig) DeepCopyInto(out *OAuth2UpstreamConfig) { (*out)[key] = val } } + if in.DCRConfig != nil { + in, out := &in.DCRConfig, &out.DCRConfig + *out = new(DCRUpstreamConfig) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OAuth2UpstreamConfig. diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index bccf0df3bb..862f32547e 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -55,27 +55,38 @@ const ( // UpstreamClientSecretEnvVar is the prefix for upstream client secret environment variables. // Actual names are TOOLHIVE_UPSTREAM_CLIENT_SECRET_ where PROVIDER is the - // upstream name uppercased with hyphens replaced by underscores. + // upstream name uppercased with hyphens replaced by underscores (e.g., + // "acme-idp" -> TOOLHIVE_UPSTREAM_CLIENT_SECRET_ACME_IDP). // #nosec G101 -- This is an environment variable name, not a hardcoded credential UpstreamClientSecretEnvVar = "TOOLHIVE_UPSTREAM_CLIENT_SECRET" + // UpstreamDCRInitialAccessTokenEnvVarPrefix is the prefix for RFC 7591 + // initial access token environment variables used with Dynamic Client + // Registration. Actual env var names are constructed as + // _ where PROVIDER is the upstream name uppercased + // with hyphens replaced by underscores (e.g., "acme-idp" -> + // TOOLHIVE_UPSTREAM_DCR_INITIAL_ACCESS_TOKEN_ACME_IDP). + // #nosec G101 -- This is an environment variable name, not a hardcoded credential + UpstreamDCRInitialAccessTokenEnvVarPrefix = "TOOLHIVE_UPSTREAM_DCR_INITIAL_ACCESS_TOKEN" + // DefaultSentinelPort is the default Redis Sentinel port DefaultSentinelPort = 26379 ) -// upstreamSecretBinding binds an upstream provider to its env var name for the -// client secret. Both GenerateAuthServerEnvVars (Pod env) and -// buildUpstreamRunConfig (runtime config) MUST use these bindings so the -// env var names stay consistent. +// upstreamSecretBinding binds an upstream provider to the env var names for +// the secrets it owns (client secret and, optionally, the DCR initial access +// token). Both GenerateAuthServerEnvVars (Pod env) and buildUpstreamRunConfig +// (runtime config) MUST use these bindings so the env var names stay consistent. type upstreamSecretBinding struct { - Provider *mcpv1beta1.UpstreamProviderConfig - EnvVarName string + Provider *mcpv1beta1.UpstreamProviderConfig + EnvVarName string + DCRInitialAccessTokenEnvVar string } -// buildUpstreamSecretBindings computes the canonical env var name for each -// upstream provider's client secret. The env var name is derived from the -// provider's Name field (uppercased, hyphens replaced with underscores) to -// keep bindings stable across provider reordering in the CRD. +// buildUpstreamSecretBindings computes the canonical env var names for each +// upstream provider's secrets. Names are derived from the provider's Name +// field (uppercased, hyphens replaced with underscores) to keep bindings +// stable across provider reordering in the CRD. func buildUpstreamSecretBindings( providers []mcpv1beta1.UpstreamProviderConfig, ) []upstreamSecretBinding { @@ -83,13 +94,83 @@ func buildUpstreamSecretBindings( for i := range providers { suffix := strings.ToUpper(strings.ReplaceAll(providers[i].Name, "-", "_")) bindings[i] = upstreamSecretBinding{ - Provider: &providers[i], - EnvVarName: fmt.Sprintf("%s_%s", UpstreamClientSecretEnvVar, suffix), + Provider: &providers[i], + EnvVarName: fmt.Sprintf("%s_%s", UpstreamClientSecretEnvVar, suffix), + DCRInitialAccessTokenEnvVar: fmt.Sprintf("%s_%s", UpstreamDCRInitialAccessTokenEnvVarPrefix, suffix), } } return bindings } +// buildUpstreamSecretEnvVars returns the Pod env vars that expose the +// client-secret and, when DCR is configured, the initial access token for a +// single upstream provider. Returns nil if the provider has no relevant +// secret references. +func buildUpstreamSecretEnvVars(b *upstreamSecretBinding) []corev1.EnvVar { + clientSecretRef, initialAccessTokenRef := extractUpstreamSecretRefs(b.Provider) + + var envVars []corev1.EnvVar + if clientSecretRef != nil { + envVars = append(envVars, envVarFromSecretRef(b.EnvVarName, clientSecretRef)) + } + if initialAccessTokenRef != nil { + envVars = append(envVars, envVarFromSecretRef(b.DCRInitialAccessTokenEnvVar, initialAccessTokenRef)) + } + return envVars +} + +// extractUpstreamSecretRefs returns the client-secret and DCR initial-access-token +// secret references for an upstream provider. +// +// What can be returned, given the admission-time invariants on +// UpstreamProviderConfig (see the kubebuilder XValidation rule on the type and +// the matching Go-level check in validateUpstreamProvider, which together +// enforce that exactly one of OIDCConfig / OAuth2Config is set and that it +// matches Type): +// - OIDC providers: only `clientSecretRef` is ever non-nil. +// `initialAccessTokenRef` is always nil because DCR is OAuth2-only and +// OAuth2Config must be nil for OIDC-typed providers. +// - OAuth2 providers: `clientSecretRef` and `initialAccessTokenRef` are +// independent — either, both, or neither may be non-nil. +// - Any other (currently unreachable) Type value: both are nil. +// +// Callers must not rely on the third bullet to mask an admission-bypassing +// object — `BuildAuthServerRunConfig` is the reconcile-time backstop for that. +func extractUpstreamSecretRefs( + provider *mcpv1beta1.UpstreamProviderConfig, +) (clientSecretRef, initialAccessTokenRef *mcpv1beta1.SecretKeyRef) { + switch provider.Type { + case mcpv1beta1.UpstreamProviderTypeOIDC: + if provider.OIDCConfig != nil { + clientSecretRef = provider.OIDCConfig.ClientSecretRef + } + case mcpv1beta1.UpstreamProviderTypeOAuth2: + if provider.OAuth2Config != nil { + clientSecretRef = provider.OAuth2Config.ClientSecretRef + if provider.OAuth2Config.DCRConfig != nil { + initialAccessTokenRef = provider.OAuth2Config.DCRConfig.InitialAccessTokenRef + } + } + } + return clientSecretRef, initialAccessTokenRef +} + +// envVarFromSecretRef builds a corev1.EnvVar that sources its value from the +// given SecretKeyRef. +func envVarFromSecretRef(name string, ref *mcpv1beta1.SecretKeyRef) corev1.EnvVar { + return corev1.EnvVar{ + Name: name, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: ref.Name, + }, + Key: ref.Key, + }, + }, + } +} + // EmbeddedAuthServerConfigName returns the config name that should be used for // embedded auth server volume/env generation, or empty string if neither ref applies. // AuthServerRef takes precedence; externalAuthConfigRef is used as a fallback. @@ -282,33 +363,7 @@ func GenerateAuthServerEnvVars( // Generate env vars for upstream client secrets using shared bindings for _, b := range buildUpstreamSecretBindings(authConfig.UpstreamProviders) { - // Extract client secret reference based on provider type - var clientSecretRef *mcpv1beta1.SecretKeyRef - - switch b.Provider.Type { - case mcpv1beta1.UpstreamProviderTypeOIDC: - if b.Provider.OIDCConfig != nil { - clientSecretRef = b.Provider.OIDCConfig.ClientSecretRef - } - case mcpv1beta1.UpstreamProviderTypeOAuth2: - if b.Provider.OAuth2Config != nil { - clientSecretRef = b.Provider.OAuth2Config.ClientSecretRef - } - } - - if clientSecretRef != nil { - envVars = append(envVars, corev1.EnvVar{ - Name: b.EnvVarName, - ValueFrom: &corev1.EnvVarSource{ - SecretKeyRef: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: clientSecretRef.Name, - }, - Key: clientSecretRef.Key, - }, - }, - }) - } + envVars = append(envVars, buildUpstreamSecretEnvVars(&b)...) } // Generate env vars for Redis ACL credentials if configured @@ -511,7 +566,11 @@ func BuildAuthServerRunConfig( bindings := buildUpstreamSecretBindings(authConfig.UpstreamProviders) config.Upstreams = make([]authserver.UpstreamRunConfig, 0, len(bindings)) for _, b := range bindings { - config.Upstreams = append(config.Upstreams, *buildUpstreamRunConfig(b.Provider, b.EnvVarName, resourceURL)) + upstream, err := buildUpstreamRunConfig(&b, resourceURL) + if err != nil { + return nil, fmt.Errorf("upstream %q: %w", b.Provider.Name, err) + } + config.Upstreams = append(config.Upstreams, *upstream) } // Build storage configuration @@ -657,14 +716,20 @@ func defaultRedirectURI(resourceURL string) string { } // buildUpstreamRunConfig converts CRD UpstreamProviderConfig to authserver.UpstreamRunConfig. -// The envVarName is computed by buildUpstreamSecretBindings to keep Pod env -// and runtime config in sync. When a provider's RedirectURI is empty, it is -// defaulted to {resourceURL}/oauth/callback. +// The binding carries the provider and the env var names computed by +// buildUpstreamSecretBindings so Pod env and runtime config stay in sync. +// When a provider's RedirectURI is empty, it is defaulted to +// {resourceURL}/oauth/callback. +// +// Returns an error when the OAuth2 provider's ClientID and DCRConfig +// combination is invalid (the same XOR enforced at admission by CEL). +// Failing at reconcile time — rather than at authserver startup — matches +// the project convention of rejecting malformed objects as early as possible. func buildUpstreamRunConfig( - provider *mcpv1beta1.UpstreamProviderConfig, - envVarName string, + b *upstreamSecretBinding, resourceURL string, -) *authserver.UpstreamRunConfig { +) (*authserver.UpstreamRunConfig, error) { + provider := b.Provider config := &authserver.UpstreamRunConfig{ Name: provider.Name, Type: authserver.UpstreamProviderType(provider.Type), @@ -673,59 +738,121 @@ func buildUpstreamRunConfig( switch provider.Type { case mcpv1beta1.UpstreamProviderTypeOIDC: if provider.OIDCConfig != nil { - redirectURI := provider.OIDCConfig.RedirectURI - if redirectURI == "" && resourceURL != "" { - redirectURI = defaultRedirectURI(resourceURL) - } - config.OIDCConfig = &authserver.OIDCUpstreamRunConfig{ - IssuerURL: provider.OIDCConfig.IssuerURL, - ClientID: provider.OIDCConfig.ClientID, - RedirectURI: redirectURI, - Scopes: provider.OIDCConfig.Scopes, - AdditionalAuthorizationParams: provider.OIDCConfig.AdditionalAuthorizationParams, - } - // If client secret is configured, reference it via env var - if provider.OIDCConfig.ClientSecretRef != nil { - config.OIDCConfig.ClientSecretEnvVar = envVarName - } - if provider.OIDCConfig.UserInfoOverride != nil { - config.OIDCConfig.UserInfoOverride = buildUserInfoRunConfig(provider.OIDCConfig.UserInfoOverride) - } + config.OIDCConfig = buildOIDCUpstreamRunConfig(provider.OIDCConfig, b.EnvVarName, resourceURL) } case mcpv1beta1.UpstreamProviderTypeOAuth2: if provider.OAuth2Config != nil { - redirectURI := provider.OAuth2Config.RedirectURI - if redirectURI == "" && resourceURL != "" { - redirectURI = defaultRedirectURI(resourceURL) - } - config.OAuth2Config = &authserver.OAuth2UpstreamRunConfig{ - AuthorizationEndpoint: provider.OAuth2Config.AuthorizationEndpoint, - TokenEndpoint: provider.OAuth2Config.TokenEndpoint, - ClientID: provider.OAuth2Config.ClientID, - RedirectURI: redirectURI, - Scopes: provider.OAuth2Config.Scopes, - AdditionalAuthorizationParams: provider.OAuth2Config.AdditionalAuthorizationParams, - } - // If client secret is configured, reference it via env var - if provider.OAuth2Config.ClientSecretRef != nil { - config.OAuth2Config.ClientSecretEnvVar = envVarName - } - if provider.OAuth2Config.UserInfo != nil { - config.OAuth2Config.UserInfo = buildUserInfoRunConfig(provider.OAuth2Config.UserInfo) - } - if provider.OAuth2Config.TokenResponseMapping != nil { - m := provider.OAuth2Config.TokenResponseMapping - config.OAuth2Config.TokenResponseMapping = &authserver.TokenResponseMappingRunConfig{ - AccessTokenPath: m.AccessTokenPath, - ScopePath: m.ScopePath, - RefreshTokenPath: m.RefreshTokenPath, - ExpiresInPath: m.ExpiresInPath, - } + oauth2, err := buildOAuth2UpstreamRunConfig(provider, b, resourceURL) + if err != nil { + return nil, err } + config.OAuth2Config = oauth2 } } - return config + return config, nil +} + +// buildOIDCUpstreamRunConfig converts a CRD OIDCUpstreamConfig to the +// runtime representation. `clientSecretEnvVar` is the resolved env var name +// used when ClientSecretRef is configured. +func buildOIDCUpstreamRunConfig( + cfg *mcpv1beta1.OIDCUpstreamConfig, + clientSecretEnvVar string, + resourceURL string, +) *authserver.OIDCUpstreamRunConfig { + redirectURI := cfg.RedirectURI + if redirectURI == "" && resourceURL != "" { + redirectURI = defaultRedirectURI(resourceURL) + } + runConfig := &authserver.OIDCUpstreamRunConfig{ + IssuerURL: cfg.IssuerURL, + ClientID: cfg.ClientID, + RedirectURI: redirectURI, + Scopes: cfg.Scopes, + AdditionalAuthorizationParams: cfg.AdditionalAuthorizationParams, + } + if cfg.ClientSecretRef != nil { + runConfig.ClientSecretEnvVar = clientSecretEnvVar + } + if cfg.UserInfoOverride != nil { + runConfig.UserInfoOverride = buildUserInfoRunConfig(cfg.UserInfoOverride) + } + return runConfig +} + +// buildOAuth2UpstreamRunConfig converts a CRD OAuth2UpstreamConfig to the +// runtime representation. It rejects malformed ClientID/DCRConfig pairs +// before producing a RunConfig — mirroring the CEL XValidation rules on +// OAuth2UpstreamConfig / DCRUpstreamConfig — so objects that reached etcd +// without passing admission (stored-before-CEL, apiserver patches, test +// fixtures bypassing validation) fail at reconcile time rather than at +// authserver startup. +func buildOAuth2UpstreamRunConfig( + provider *mcpv1beta1.UpstreamProviderConfig, + b *upstreamSecretBinding, + resourceURL string, +) (*authserver.OAuth2UpstreamRunConfig, error) { + cfg := provider.OAuth2Config + // Pass an empty prefix so the upstream name appears once — supplied by the + // outer wrap in BuildAuthServerRunConfig — instead of duplicating it on + // both the inner and outer error messages. + if err := mcpv1beta1.ValidateOAuth2DCRConfig("", cfg); err != nil { + return nil, err + } + + redirectURI := cfg.RedirectURI + if redirectURI == "" && resourceURL != "" { + redirectURI = defaultRedirectURI(resourceURL) + } + runConfig := &authserver.OAuth2UpstreamRunConfig{ + AuthorizationEndpoint: cfg.AuthorizationEndpoint, + TokenEndpoint: cfg.TokenEndpoint, + ClientID: cfg.ClientID, + RedirectURI: redirectURI, + Scopes: cfg.Scopes, + AdditionalAuthorizationParams: cfg.AdditionalAuthorizationParams, + } + if cfg.ClientSecretRef != nil { + runConfig.ClientSecretEnvVar = b.EnvVarName + } + if cfg.UserInfo != nil { + runConfig.UserInfo = buildUserInfoRunConfig(cfg.UserInfo) + } + if cfg.TokenResponseMapping != nil { + m := cfg.TokenResponseMapping + runConfig.TokenResponseMapping = &authserver.TokenResponseMappingRunConfig{ + AccessTokenPath: m.AccessTokenPath, + ScopePath: m.ScopePath, + RefreshTokenPath: m.RefreshTokenPath, + ExpiresInPath: m.ExpiresInPath, + } + } + if cfg.DCRConfig != nil { + runConfig.DCRConfig = buildDCRUpstreamRunConfig(cfg.DCRConfig, b.DCRInitialAccessTokenEnvVar) + } + return runConfig, nil +} + +// buildDCRUpstreamRunConfig converts CRD DCRUpstreamConfig to +// authserver.DCRUpstreamConfig. When an InitialAccessTokenRef is present on +// the CRD, the resolver reads the token value from the supplied env var +// (populated from the secret ref by GenerateAuthServerEnvVars), mirroring the +// ClientSecretRef → env-var pattern. +func buildDCRUpstreamRunConfig( + dcr *mcpv1beta1.DCRUpstreamConfig, + initialAccessTokenEnvVar string, +) *authserver.DCRUpstreamConfig { + rc := &authserver.DCRUpstreamConfig{ + DiscoveryURL: dcr.DiscoveryURL, + RegistrationEndpoint: dcr.RegistrationEndpoint, + SoftwareID: dcr.SoftwareID, + SoftwareStatement: dcr.SoftwareStatement, + } + if dcr.InitialAccessTokenRef != nil { + rc.InitialAccessTokenEnvVar = initialAccessTokenEnvVar + } + return rc } // buildUserInfoRunConfig converts CRD UserInfoConfig to authserver.UserInfoRunConfig. diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index 7add4da7af..ec392f6e70 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -6,6 +6,7 @@ package controllerutil import ( "context" "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -497,6 +498,86 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { }, wantSecretNames: []string{"okta-secret", "github-secret"}, }, + { + name: "OAuth2 provider with DCR initial access token ref emits separate env var", + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + InitialAccessTokenRef: &mcpv1beta1.SecretKeyRef{ + Name: "acme-dcr-token", + Key: "token", + }, + }, + }, + }, + }, + }, + wantEnvNames: []string{UpstreamDCRInitialAccessTokenEnvVarPrefix + "_ACME_IDP"}, + wantSecretNames: []string{"acme-dcr-token"}, + }, + { + name: "OAuth2 provider with DCR and client secret emits both env vars", + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + ClientSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "acme-secret", + Key: "client-secret", + }, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + InitialAccessTokenRef: &mcpv1beta1.SecretKeyRef{ + Name: "acme-dcr-token", + Key: "token", + }, + }, + }, + }, + }, + }, + wantEnvNames: []string{ + UpstreamClientSecretEnvVar + "_ACME_IDP", + UpstreamDCRInitialAccessTokenEnvVarPrefix + "_ACME_IDP", + }, + wantSecretNames: []string{"acme-secret", "acme-dcr-token"}, + }, + { + name: "OAuth2 provider with DCR but no initial access token ref emits no DCR env var", + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "public-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + }, + }, + }, + wantEnvNames: nil, + }, } for _, tt := range tests { @@ -1175,6 +1256,142 @@ func TestBuildAuthServerRunConfig(t *testing.T) { assert.Equal(t, "https://mcp.example.com/oauth/callback", config.Upstreams[0].OIDCConfig.RedirectURI) }, }, + { + name: "with OAuth2 upstream using DCR (discoveryUrl)", + resourceURL: defaultResourceURL, + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + SoftwareID: "toolhive", + SoftwareStatement: "jwt-statement", + InitialAccessTokenRef: &mcpv1beta1.SecretKeyRef{ + Name: "acme-dcr-token", + Key: "token", + }, + }, + }, + }, + }, + }, + allowedAudiences: defaultAudiences, + scopesSupported: defaultScopes, + checkFunc: func(t *testing.T, config *authserver.RunConfig) { + t.Helper() + require.Len(t, config.Upstreams, 1) + upstream := config.Upstreams[0] + require.NotNil(t, upstream.OAuth2Config) + assert.Empty(t, upstream.OAuth2Config.ClientID, + "ClientID should be empty when DCRConfig is used") + require.NotNil(t, upstream.OAuth2Config.DCRConfig) + assert.Equal(t, + "https://idp.example.com/.well-known/openid-configuration", + upstream.OAuth2Config.DCRConfig.DiscoveryURL) + assert.Empty(t, upstream.OAuth2Config.DCRConfig.RegistrationEndpoint) + assert.Equal(t, "toolhive", upstream.OAuth2Config.DCRConfig.SoftwareID) + assert.Equal(t, "jwt-statement", upstream.OAuth2Config.DCRConfig.SoftwareStatement) + assert.Equal(t, + UpstreamDCRInitialAccessTokenEnvVarPrefix+"_ACME_IDP", + upstream.OAuth2Config.DCRConfig.InitialAccessTokenEnvVar) + assert.Empty(t, upstream.OAuth2Config.DCRConfig.InitialAccessTokenFile) + }, + }, + { + name: "with OAuth2 upstream using DCR (registrationEndpoint)", + resourceURL: defaultResourceURL, + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + Scopes: []string{"openid"}, + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + }, + }, + }, + }, + }, + allowedAudiences: defaultAudiences, + scopesSupported: defaultScopes, + checkFunc: func(t *testing.T, config *authserver.RunConfig) { + t.Helper() + require.Len(t, config.Upstreams, 1) + upstream := config.Upstreams[0] + require.NotNil(t, upstream.OAuth2Config) + require.NotNil(t, upstream.OAuth2Config.DCRConfig) + assert.Equal(t, "https://idp.example.com/register", + upstream.OAuth2Config.DCRConfig.RegistrationEndpoint) + assert.Empty(t, upstream.OAuth2Config.DCRConfig.DiscoveryURL) + // No InitialAccessTokenRef set: env var name should stay empty. + assert.Empty(t, upstream.OAuth2Config.DCRConfig.InitialAccessTokenEnvVar) + }, + }, + { + // Regression guard: the non-DCR OAuth2 path must leave DCRConfig + // nil and carry ClientID through untouched. Without this case, + // refactors of buildUpstreamRunConfig could populate DCRConfig + // (or drop ClientID) silently. + name: "with OAuth2 upstream using ClientID only (no DCRConfig)", + resourceURL: defaultResourceURL, + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "github", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://github.com/login/oauth/authorize", + TokenEndpoint: "https://github.com/login/oauth/access_token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://api.github.com/user"}, + ClientID: "pre-provisioned-id", + }, + }, + }, + }, + allowedAudiences: defaultAudiences, + scopesSupported: defaultScopes, + checkFunc: func(t *testing.T, config *authserver.RunConfig) { + t.Helper() + require.Len(t, config.Upstreams, 1) + upstream := config.Upstreams[0] + require.NotNil(t, upstream.OAuth2Config) + assert.Equal(t, "pre-provisioned-id", upstream.OAuth2Config.ClientID) + assert.Nil(t, upstream.OAuth2Config.DCRConfig, + "DCRConfig should remain nil when only ClientID is set") + }, + }, } for _, tt := range tests { @@ -1190,6 +1407,109 @@ func TestBuildAuthServerRunConfig(t *testing.T) { } } +// TestBuildAuthServerRunConfig_InvalidDCR verifies that BuildAuthServerRunConfig +// rejects OAuth2 upstreams whose ClientID / DCRConfig pairing violates the +// mutual-exclusivity invariants (matching the CEL XValidation rules on +// OAuth2UpstreamConfig and DCRUpstreamConfig). This is the reconcile-time +// defense-in-depth against stored objects that bypassed admission. +func TestBuildAuthServerRunConfig_InvalidDCR(t *testing.T) { + t.Parallel() + + defaultAudiences := []string{"http://test-server.default.svc.cluster.local:8080"} + defaultScopes := []string{"openid", "offline_access"} + + tests := []struct { + name string + oauth2Cfg *mcpv1beta1.OAuth2UpstreamConfig + wantErr string + }{ + { + name: "both ClientID and DCRConfig set", + oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + ClientID: "pre-provisioned-id", + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + wantErr: "exactly one of clientId or dcrConfig must be set", + }, + { + name: "neither ClientID nor DCRConfig set", + oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + }, + wantErr: "exactly one of clientId or dcrConfig must be set", + }, + { + name: "DCRConfig with both endpoints set", + oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + RegistrationEndpoint: "https://idp.example.com/register", + }, + }, + wantErr: "exactly one of discoveryUrl or registrationEndpoint must be set", + }, + { + name: "DCRConfig with neither endpoint set", + oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{}, + }, + wantErr: "exactly one of discoveryUrl or registrationEndpoint must be set", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + authConfig := &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: tt.oauth2Cfg, + }, + }, + } + + config, err := BuildAuthServerRunConfig( + "default", "test-server", authConfig, + defaultAudiences, defaultScopes, + "http://test-server.default.svc.cluster.local:8080", + ) + + require.Error(t, err, "expected BuildAuthServerRunConfig to fail on invalid DCR pairing") + assert.Nil(t, config) + assert.Contains(t, err.Error(), tt.wantErr, + "error should surface the mutual-exclusivity violation") + // The upstream name must appear exactly once: the outer wrap in + // BuildAuthServerRunConfig supplies it, and the inner validator is + // invoked with an empty prefix so it doesn't duplicate the name. + assert.Equal(t, 1, strings.Count(err.Error(), "acme-idp"), + "upstream name should appear exactly once in error: %q", err.Error()) + }) + } +} + func TestAddEmbeddedAuthServerConfigOptions_Validation(t *testing.T) { t.Parallel() 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 6e4f0d6f65..b9d83b5ea0 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 @@ -488,8 +488,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -523,8 +534,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -541,6 +554,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -657,9 +739,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -799,6 +885,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: @@ -1536,8 +1629,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -1571,8 +1675,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -1589,6 +1695,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -1705,9 +1880,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -1847,6 +2026,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: 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 a51fe4b5bd..8735ffbe30 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 @@ -361,8 +361,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -396,8 +407,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -414,6 +427,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -530,9 +612,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -672,6 +758,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: @@ -2857,8 +2950,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -2892,8 +2996,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -2910,6 +3016,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -3026,9 +3201,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -3168,6 +3347,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: 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 4ea0c1cb07..2ab3edb743 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -491,8 +491,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -526,8 +537,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -544,6 +557,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -660,9 +742,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -802,6 +888,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: @@ -1539,8 +1632,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -1574,8 +1678,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -1592,6 +1698,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -1708,9 +1883,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -1850,6 +2029,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: 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 6078670479..909455c2a2 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -364,8 +364,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -399,8 +410,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -417,6 +430,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -533,9 +615,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -675,6 +761,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: @@ -2860,8 +2953,19 @@ spec: The embedded auth server delegates authentication to these providers. MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. items: - description: UpstreamProviderConfig defines configuration for - an upstream Identity Provider. + description: |- + UpstreamProviderConfig defines configuration for an upstream Identity Provider. + + Exactly one of OIDCConfig or OAuth2Config must be set and must match the + declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers + set OAuth2Config. The CEL rule below enforces the pairing at admission; the + matching Go-level check in validateUpstreamProvider provides defense-in-depth + for stored objects. + + The rule is structured as a chain of equality checks ending in an explicit + `false`, so adding a new UpstreamProviderType value without extending this + rule fails admission instead of silently demanding the OAuth2 shape. When + adding a new type, extend both this rule and validateUpstreamProvider. properties: name: description: |- @@ -2895,8 +2999,10 @@ spec: pattern: ^https?://.*$ type: string clientId: - description: ClientID is the OAuth 2.0 client identifier - registered with the upstream IDP. + description: |- + ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. + Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained + at runtime via RFC 7591 Dynamic Client Registration and must be left empty. type: string clientSecretRef: description: |- @@ -2913,6 +3019,75 @@ spec: - key - name type: object + dcrConfig: + description: |- + DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream + authorization server. When set, the client credentials are obtained at + runtime rather than being pre-provisioned, and ClientID must be left empty. + Mutually exclusive with ClientID. + properties: + discoveryUrl: + description: |- + DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver + issues a single GET against this URL (no well-known-path fallback) and + reads registration_endpoint, authorization_endpoint, token_endpoint, + token_endpoint_auth_methods_supported, and scopes_supported from the + response. + Mutually exclusive with RegistrationEndpoint. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + initialAccessTokenRef: + description: |- + InitialAccessTokenRef is an optional reference to a Kubernetes Secret + carrying an RFC 7591 §3 initial access token. When set, the resolver + presents the token value as a Bearer credential on the registration + request. Mirrors the ClientSecretRef pattern. + properties: + key: + description: Key is the key within the secret + type: string + name: + description: Name is the name of the secret + type: string + required: + - key + - name + type: object + registrationEndpoint: + description: |- + RegistrationEndpoint is the RFC 7591 registration endpoint URL used + directly, bypassing discovery. When using this field, the caller is + expected to also supply AuthorizationEndpoint, TokenEndpoint, and an + explicit Scopes list on the parent OAuth2UpstreamConfig. + Mutually exclusive with DiscoveryURL. + MaxLength bounds CEL cost estimation on the parent struct's + XValidation rule and matches the conventional URL length cap. + maxLength: 2048 + pattern: ^https?://.*$ + type: string + softwareId: + description: |- + SoftwareID is the RFC 7591 "software_id" registration metadata value, + identifying the client software independent of any particular + registration instance. Typically a UUID or short identifier. + maxLength: 255 + type: string + softwareStatement: + description: |- + SoftwareStatement is the RFC 7591 "software_statement" JWT asserting + metadata about the client software, signed by a party the authorization + server trusts. Bounded to 4096 characters to prevent unbounded + user input from inflating the CR beyond etcd object limits. + maxLength: 4096 + type: string + type: object + x-kubernetes-validations: + - message: exactly one of discoveryUrl or registrationEndpoint + must be set + rule: has(self.discoveryUrl) != has(self.registrationEndpoint) redirectUri: description: |- RedirectURI is the callback URL where the upstream IDP will redirect after authentication. @@ -3029,9 +3204,13 @@ spec: type: object required: - authorizationEndpoint - - clientId - tokenEndpoint type: object + x-kubernetes-validations: + - message: exactly one of clientId or dcrConfig must be + set + rule: '(has(self.clientId) && size(self.clientId) > 0) + ? !has(self.dcrConfig) : has(self.dcrConfig)' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -3171,6 +3350,13 @@ spec: - name - type type: object + x-kubernetes-validations: + - message: type must be 'oidc' or 'oauth2'; oidcConfig must + be set when type is 'oidc' and oauth2Config must be set + when type is 'oauth2' (and the other must not be set) + rule: 'self.type == ''oidc'' ? (has(self.oidcConfig) && !has(self.oauth2Config)) + : self.type == ''oauth2'' ? (has(self.oauth2Config) && !has(self.oidcConfig)) + : false' minItems: 1 type: array x-kubernetes-list-map-keys: diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index e60f05015e..417593459e 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1077,6 +1077,40 @@ _Appears in:_ | `key` _string_ | Key is the key in the ConfigMap that contains the authorization configuration | authz.json | Optional: \{\}
| +#### api.v1beta1.DCRUpstreamConfig + + + +DCRUpstreamConfig configures RFC 7591 Dynamic Client Registration for an +OAuth 2.0 upstream. When present on an OAuth2 upstream, the authserver +performs registration at runtime to obtain client credentials, replacing +the need to pre-provision a ClientID. + +Exactly one of DiscoveryURL or RegistrationEndpoint must be set. DiscoveryURL +points at an RFC 8414 / OIDC Discovery document from which the registration +endpoint is resolved; RegistrationEndpoint is used directly when the upstream +does not publish discovery metadata. + +The XOR rule uses has() alone (not has() + size() > 0) to keep the rule's +estimated CEL cost under the apiserver's per-rule static budget. With +`omitempty` on both fields, an unset field is absent on the wire and has() +returns false; the explicit-empty-string edge case is rejected at reconcile +time by ValidateOAuth2DCRConfig. + + + +_Appears in:_ +- [api.v1beta1.OAuth2UpstreamConfig](#apiv1beta1oauth2upstreamconfig) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `discoveryUrl` _string_ | DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver
issues a single GET against this URL (no well-known-path fallback) and
reads registration_endpoint, authorization_endpoint, token_endpoint,
token_endpoint_auth_methods_supported, and scopes_supported from the
response.
Mutually exclusive with RegistrationEndpoint.
MaxLength bounds CEL cost estimation on the parent struct's
XValidation rule and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https?://.*$`
Optional: \{\}
| +| `registrationEndpoint` _string_ | RegistrationEndpoint is the RFC 7591 registration endpoint URL used
directly, bypassing discovery. When using this field, the caller is
expected to also supply AuthorizationEndpoint, TokenEndpoint, and an
explicit Scopes list on the parent OAuth2UpstreamConfig.
Mutually exclusive with DiscoveryURL.
MaxLength bounds CEL cost estimation on the parent struct's
XValidation rule and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https?://.*$`
Optional: \{\}
| +| `initialAccessTokenRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | InitialAccessTokenRef is an optional reference to a Kubernetes Secret
carrying an RFC 7591 §3 initial access token. When set, the resolver
presents the token value as a Bearer credential on the registration
request. Mirrors the ClientSecretRef pattern. | | Optional: \{\}
| +| `softwareId` _string_ | SoftwareID is the RFC 7591 "software_id" registration metadata value,
identifying the client software independent of any particular
registration instance. Typically a UUID or short identifier. | | MaxLength: 255
Optional: \{\}
| +| `softwareStatement` _string_ | SoftwareStatement is the RFC 7591 "software_statement" JWT asserting
metadata about the client software, signed by a party the authorization
server trusts. Bounded to 4096 characters to prevent unbounded
user input from inflating the CR beyond etcd object limits. | | MaxLength: 4096
Optional: \{\}
| + + #### api.v1beta1.EmbeddedAuthServerConfig @@ -2558,6 +2592,10 @@ _Appears in:_ OAuth2UpstreamConfig contains configuration for pure OAuth 2.0 providers. OAuth 2.0 providers require explicit endpoint configuration. +Exactly one of ClientID or DCRConfig must be set: ClientID is used when the +client is pre-provisioned out of band, DCRConfig enables RFC 7591 Dynamic +Client Registration at runtime. + _Appears in:_ @@ -2568,12 +2606,13 @@ _Appears in:_ | `authorizationEndpoint` _string_ | AuthorizationEndpoint is the URL for the OAuth authorization endpoint. | | Pattern: `^https?://.*$`
Required: \{\}
| | `tokenEndpoint` _string_ | TokenEndpoint is the URL for the OAuth token endpoint. | | Pattern: `^https?://.*$`
Required: \{\}
| | `userInfo` _[api.v1beta1.UserInfoConfig](#apiv1beta1userinfoconfig)_ | UserInfo contains configuration for fetching user information from the upstream provider.
When omitted, the embedded auth server runs in synthesis mode for this
upstream: a non-PII subject derived from the access token, no Name/Email.
Use this shape for upstreams with no userinfo surface (e.g., MCP
authorization servers per the MCP spec). | | Optional: \{\}
| -| `clientId` _string_ | ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. | | Required: \{\}
| +| `clientId` _string_ | ClientID is the OAuth 2.0 client identifier registered with the upstream IDP.
Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained
at runtime via RFC 7591 Dynamic Client Registration and must be left empty. | | Optional: \{\}
| | `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the OAuth 2.0 client secret.
Optional for public clients using PKCE instead of client secret. | | Optional: \{\}
| | `redirectUri` _string_ | RedirectURI is the callback URL where the upstream IDP will redirect after authentication.
When not specified, defaults to `\{resourceUrl\}/oauth/callback` where `resourceUrl` is the
URL associated with the resource (e.g., MCPServer or vMCP) using this config. | | Optional: \{\}
| | `scopes` _string array_ | Scopes are the OAuth scopes to request from the upstream IDP. | | Optional: \{\}
| | `tokenResponseMapping` _[api.v1beta1.TokenResponseMapping](#apiv1beta1tokenresponsemapping)_ | TokenResponseMapping configures custom field extraction from non-standard token responses.
Some OAuth providers (e.g., GovSlack) nest token fields under non-standard paths
instead of returning them at the top level. When set, ToolHive performs the token
exchange HTTP call directly and extracts fields using the configured dot-notation paths.
If nil, standard OAuth 2.0 token response parsing is used. | | Optional: \{\}
| | `additionalAuthorizationParams` _object (keys:string, values:string)_ | AdditionalAuthorizationParams are extra query parameters to include in
authorization requests sent to the upstream provider.
This is useful for providers that require custom parameters, such as
Google's access_type=offline for obtaining refresh tokens.
Framework-managed parameters (response_type, client_id, redirect_uri,
scope, state, code_challenge, code_challenge_method, nonce) are not allowed. | | MaxProperties: 16
Optional: \{\}
| +| `dcrConfig` _[api.v1beta1.DCRUpstreamConfig](#apiv1beta1dcrupstreamconfig)_ | DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream
authorization server. When set, the client credentials are obtained at
runtime rather than being pre-provisioned, and ClientID must be left empty.
Mutually exclusive with ClientID. | | Optional: \{\}
| #### api.v1beta1.OIDCUpstreamConfig @@ -2946,6 +2985,7 @@ SecretKeyRef is a reference to a key within a Secret _Appears in:_ - [api.v1beta1.BearerTokenConfig](#apiv1beta1bearertokenconfig) +- [api.v1beta1.DCRUpstreamConfig](#apiv1beta1dcrupstreamconfig) - [api.v1beta1.EmbeddedAuthServerConfig](#apiv1beta1embeddedauthserverconfig) - [api.v1beta1.EmbeddingServerSpec](#apiv1beta1embeddingserverspec) - [api.v1beta1.HeaderFromSecret](#apiv1beta1headerfromsecret) @@ -3212,6 +3252,17 @@ _Appears in:_ UpstreamProviderConfig defines configuration for an upstream Identity Provider. +Exactly one of OIDCConfig or OAuth2Config must be set and must match the +declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers +set OAuth2Config. The CEL rule below enforces the pairing at admission; the +matching Go-level check in validateUpstreamProvider provides defense-in-depth +for stored objects. + +The rule is structured as a chain of equality checks ending in an explicit +`false`, so adding a new UpstreamProviderType value without extending this +rule fails admission instead of silently demanding the OAuth2 shape. When +adding a new type, extend both this rule and validateUpstreamProvider. + _Appears in:_ From 1fb613142e6322cc9a74cc0effcb3e10d62fec46 Mon Sep 17 00:00:00 2001 From: Trey Date: Tue, 5 May 2026 07:54:17 -0700 Subject: [PATCH 2/4] Tighten and align upstream OAuth2/DCR validation Addresses stacklok/toolhive#5069 review comments: - F1 MEDIUM mcpexternalauthconfig_types.go (3174398097): raise softwareStatement cap to 16384 so realistic signed JWTs with x5c chains / OIDC-Federation metadata fit, document plaintext exposure (F11) and operator guidance. - F2 MEDIUM mcpexternalauthconfig_types.go (3174398101): tighten DCR URL patterns to https-only with no whitespace/fragment/trailing-slash; document why HTTPS is required. - F3 MEDIUM mcpexternalauthconfig_types.go (3174398115): reject ClientSecretRef + DCRConfig in CEL and ValidateOAuth2DCRConfig; in DCR mode the client_secret comes from the registration response, so a static ClientSecretRef is dead config or a competing source of truth. - F6 LOW mcpexternalauthconfig_types.go (3174398125): replace incorrect CEL-cost rationale on URL MaxLength with defensive size cap rationale. - F7 LOW mcpexternalauthconfig_types_test.go (3174398129): reshape OIDC-with-stray-OAuth2 test so OIDC discriminator passes and the OAuth2 leak path is what trips the rule. - F9 LOW mcpexternalauthconfig_types.go (3174398139): unify the validateUpstreamProvider discriminator into a single CEL-aligned check and message; reconcile-time and admission-time errors now match. - F10 LOW mcpexternalauthconfig_types.go (3174398143): document the layered XOR behavior (`dcrConfig: {}` traversing to the inner DCR rule); tightening the outer rule would inflate CEL cost. - F11 LOW mcpexternalauthconfig_types.go (3174398143): document softwareStatement plaintext exposure and point to InitialAccessTokenRef for callers that need confidentiality. - F15 LOW mcpexternalauthconfig_types.go (3174398165): enforce DCR URL and softwareStatement length caps in ValidateOAuth2DCRConfig with boundary tests, so reconcile-time matches admission-time on length too. Regenerated CRDs (operator-manifests) and docs/operator/crd-api.md (crd-ref-docs) so propagated text picks up the doc-comment corrections. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 122 ++++++++++++++---- .../mcpexternalauthconfig_types_test.go | 114 ++++++++++++++-- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 78 ++++++++--- ...olhive.stacklok.dev_virtualmcpservers.yaml | 78 ++++++++--- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 78 ++++++++--- ...olhive.stacklok.dev_virtualmcpservers.yaml | 78 ++++++++--- docs/operator/crd-api.md | 23 +++- 7 files changed, 463 insertions(+), 108 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index b3730398a0..7dc6f35d75 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -374,7 +374,25 @@ type OIDCUpstreamConfig struct { // client is pre-provisioned out of band, DCRConfig enables RFC 7591 Dynamic // Client Registration at runtime. // +// ClientSecretRef is mutually exclusive with DCRConfig: when DCRConfig is set, +// the client_secret is obtained from the registration response (RFC 7591 +// §3.2.1) and any static ClientSecretRef would be either dead config or a +// competing source of truth. The XValidation rule below rejects the +// combination at admission; ValidateOAuth2DCRConfig is the matching +// reconcile-time backstop. +// +// Layered XOR behavior: the ClientID/DCRConfig rule treats `clientId: ""` as +// absent (size>0) but treats `dcrConfig: {}` as present (has() is true for an +// empty object). For input `{ clientId: "", dcrConfig: {} }` the outer rule +// passes and the inner DCRUpstreamConfig XOR fires with "exactly one of +// discoveryUrl or registrationEndpoint must be set". This is intentional — +// adding a non-empty subfield check (e.g., +// `has(self.dcrConfig.discoveryUrl) || has(self.dcrConfig.registrationEndpoint)`) +// would inflate CEL cost on an already-budget-bound rule, and the inner +// message is still actionable. +// // +kubebuilder:validation:XValidation:rule="(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)",message="exactly one of clientId or dcrConfig must be set" +// +kubebuilder:validation:XValidation:rule="!(has(self.dcrConfig) && has(self.clientSecretRef))",message="clientSecretRef must not be set when dcrConfig is set; the client_secret is obtained at runtime via Dynamic Client Registration" // //nolint:lll // CEL validation rules exceed line length limit type OAuth2UpstreamConfig struct { @@ -470,10 +488,13 @@ type DCRUpstreamConfig struct { // token_endpoint_auth_methods_supported, and scopes_supported from the // response. // Mutually exclusive with RegistrationEndpoint. - // MaxLength bounds CEL cost estimation on the parent struct's - // XValidation rule and matches the conventional URL length cap. + // HTTPS is required because the registration endpoint resolved from this + // document carries the initial access token and the issued client_secret + // (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + // object budget, regex evaluation cost) and matches the conventional URL + // length cap. // +optional - // +kubebuilder:validation:Pattern=`^https?://.*$` + // +kubebuilder:validation:Pattern=`^https://[^\s?#]+[^/\s?#]$` // +kubebuilder:validation:MaxLength=2048 DiscoveryURL string `json:"discoveryUrl,omitempty"` @@ -482,10 +503,12 @@ type DCRUpstreamConfig struct { // expected to also supply AuthorizationEndpoint, TokenEndpoint, and an // explicit Scopes list on the parent OAuth2UpstreamConfig. // Mutually exclusive with DiscoveryURL. - // MaxLength bounds CEL cost estimation on the parent struct's - // XValidation rule and matches the conventional URL length cap. + // HTTPS is required because the registration endpoint carries the initial + // access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + // MaxLength is a defensive size cap (etcd object budget, regex evaluation + // cost) and matches the conventional URL length cap. // +optional - // +kubebuilder:validation:Pattern=`^https?://.*$` + // +kubebuilder:validation:Pattern=`^https://[^\s?#]+[^/\s?#]$` // +kubebuilder:validation:MaxLength=2048 RegistrationEndpoint string `json:"registrationEndpoint,omitempty"` @@ -505,10 +528,22 @@ type DCRUpstreamConfig struct { // SoftwareStatement is the RFC 7591 "software_statement" JWT asserting // metadata about the client software, signed by a party the authorization - // server trusts. Bounded to 4096 characters to prevent unbounded - // user input from inflating the CR beyond etcd object limits. - // +optional - // +kubebuilder:validation:MaxLength=4096 + // server trusts. + // + // Stored inline on the CR. The JWT is signed but not encrypted, so its + // contents are visible to anyone with get/list/watch on this resource and + // appear in etcd backups in plaintext. Treat the value as non-confidential + // (signed attestation, not a secret). Operators that rotate software + // statements like bearer credentials should keep them at the authorization + // server side and rely on the registration endpoint's initial access + // token (see InitialAccessTokenRef) instead of placing them on the CR. + // + // Bounded to 16384 characters as a defensive size cap (etcd object + // budget, regex evaluation cost). Real-world signed statements with + // embedded x5c certificate chains, JWKS keys, or OIDC-Federation + // trust-framework metadata routinely exceed 4 KB. + // +optional + // +kubebuilder:validation:MaxLength=16384 SoftwareStatement string `json:"softwareStatement,omitempty"` } @@ -1056,22 +1091,25 @@ func (r *MCPExternalAuthConfig) validateEmbeddedAuthServer() error { return nil } -// validateUpstreamProvider validates a single upstream provider configuration +// validateUpstreamProvider validates a single upstream provider configuration. +// The discriminator check mirrors the combined CEL XValidation rule on +// UpstreamProviderConfig: a single boolean test produces a single message that +// matches what admission emits, so reconcile-time and admission-time errors +// stay aligned. func (*MCPExternalAuthConfig) validateUpstreamProvider(index int, provider *UpstreamProviderConfig) error { prefix := fmt.Sprintf("upstreamProviders[%d]", index) - if (provider.OIDCConfig == nil) == (provider.Type == UpstreamProviderTypeOIDC) { - return fmt.Errorf("%s: oidcConfig must be set when type is 'oidc' and must not be set otherwise", prefix) - } - if (provider.OAuth2Config == nil) == (provider.Type == UpstreamProviderTypeOAuth2) { - return fmt.Errorf("%s: oauth2Config must be set when type is 'oauth2' and must not be set otherwise", prefix) - } - if provider.Type != UpstreamProviderTypeOIDC && provider.Type != UpstreamProviderTypeOAuth2 { - return fmt.Errorf("%s: unsupported provider type: %s", prefix, provider.Type) + typeOK := provider.Type == UpstreamProviderTypeOIDC || provider.Type == UpstreamProviderTypeOAuth2 + configOK := (provider.Type == UpstreamProviderTypeOIDC && provider.OIDCConfig != nil && provider.OAuth2Config == nil) || + (provider.Type == UpstreamProviderTypeOAuth2 && provider.OAuth2Config != nil && provider.OIDCConfig == nil) + if !typeOK || !configOK { + return fmt.Errorf("%s: type must be 'oidc' or 'oauth2'; oidcConfig must be set when type is 'oidc' "+ + "and oauth2Config must be set when type is 'oauth2' (and the other must not be set)", prefix) } // Validate OAuth2-specific DCR / ClientID constraints (defense-in-depth with CEL). - if provider.Type == UpstreamProviderTypeOAuth2 && provider.OAuth2Config != nil { + // The discriminator above guarantees OAuth2Config != nil when type is oauth2. + if provider.Type == UpstreamProviderTypeOAuth2 { if err := ValidateOAuth2DCRConfig(prefix, provider.OAuth2Config); err != nil { return err } @@ -1081,12 +1119,28 @@ func (*MCPExternalAuthConfig) validateUpstreamProvider(index int, provider *Upst return ValidateAdditionalAuthorizationParams(prefix, provider.AdditionalAuthorizationParams()) } +// Length caps for DCR-related string fields. Mirror the +// +kubebuilder:validation:MaxLength markers on DCRUpstreamConfig so that +// ValidateOAuth2DCRConfig is a true reconcile-time backstop for length +// constraints, not just for the XOR rules. +const ( + // MaxDCRURLLength matches the MaxLength marker on + // DCRUpstreamConfig.DiscoveryURL and DCRUpstreamConfig.RegistrationEndpoint. + MaxDCRURLLength = 2048 + + // MaxSoftwareStatementLength matches the MaxLength marker on + // DCRUpstreamConfig.SoftwareStatement. + MaxSoftwareStatementLength = 16384 +) + // ValidateOAuth2DCRConfig enforces the mutual exclusivity between ClientID and -// DCRConfig and, when DCRConfig is present, between DiscoveryURL and -// RegistrationEndpoint. These rules mirror the CEL validation on -// OAuth2UpstreamConfig and DCRUpstreamConfig and provide defense-in-depth for -// stored objects (e.g., objects stored before CEL rules were added or -// validated through code paths that bypass admission). +// DCRConfig, between ClientSecretRef and DCRConfig, and (when DCRConfig is +// present) between DiscoveryURL and RegistrationEndpoint. It also enforces the +// MaxLength caps declared on DCRUpstreamConfig so reconcile-time matches +// admission-time. These rules mirror the CEL validation on OAuth2UpstreamConfig +// and DCRUpstreamConfig and provide defense-in-depth for stored objects (e.g., +// objects stored before CEL rules were added or validated through code paths +// that bypass admission). // // The prefix is embedded in error messages to identify the offending upstream // (e.g., "upstreamProviders[i]"). Pass an empty string when the caller already @@ -1107,11 +1161,29 @@ func ValidateOAuth2DCRConfig(prefix string, cfg *OAuth2UpstreamConfig) error { return nil } + if cfg.ClientSecretRef != nil { + return fmt.Errorf( + "%s: clientSecretRef must not be set when dcrConfig is set; "+ + "the client_secret is obtained at runtime via Dynamic Client Registration", scoped) + } + hasDiscoveryURL := cfg.DCRConfig.DiscoveryURL != "" hasRegistrationEndpoint := cfg.DCRConfig.RegistrationEndpoint != "" if hasDiscoveryURL == hasRegistrationEndpoint { return fmt.Errorf("%s.dcrConfig: exactly one of discoveryUrl or registrationEndpoint must be set", scoped) } + + if l := len(cfg.DCRConfig.DiscoveryURL); l > MaxDCRURLLength { + return fmt.Errorf("%s.dcrConfig.discoveryUrl: length %d exceeds maximum %d", scoped, l, MaxDCRURLLength) + } + if l := len(cfg.DCRConfig.RegistrationEndpoint); l > MaxDCRURLLength { + return fmt.Errorf("%s.dcrConfig.registrationEndpoint: length %d exceeds maximum %d", scoped, l, MaxDCRURLLength) + } + if l := len(cfg.DCRConfig.SoftwareStatement); l > MaxSoftwareStatementLength { + return fmt.Errorf( + "%s.dcrConfig.softwareStatement: length %d exceeds maximum %d", + scoped, l, MaxSoftwareStatementLength) + } return nil } diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go index 09e92438fc..2e01c53de6 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go @@ -4,6 +4,7 @@ package v1beta1 import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -11,6 +12,24 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// makeStringOfLen returns a string of exactly n ASCII bytes. +func makeStringOfLen(n int) string { + return strings.Repeat("a", n) +} + +// makeURLOfLen returns an https URL whose total length is exactly n. +// The host is padded with 'a' characters; useful for boundary-length tests +// against +kubebuilder:validation:Pattern URL regexes that require a host. +func makeURLOfLen(n int) string { + const prefix = "https://" + if n <= len(prefix) { + // Caller asked for something below the minimum URL shape. Fall back to + // a plain string of length n so callers see the length they asked for. + return strings.Repeat("a", n) + } + return prefix + strings.Repeat("a", n-len(prefix)) +} + func TestMCPExternalAuthConfig_Validate(t *testing.T) { t.Parallel() @@ -502,7 +521,10 @@ func TestMCPExternalAuthConfig_validateUpstreamProvider(t *testing.T) { OIDCConfig: &OIDCUpstreamConfig{IssuerURL: "https://oauth.example.com", ClientID: "client-id"}, }, expectErr: true, - errMsg: "oidcConfig must be set when type is 'oidc' and must not be set otherwise", + // Pin the suffix that uniquely identifies the unified discriminator + // message — distinguishes it from the per-config substring used in + // the "missing oidcConfig" / "missing oauth2Config" cases above. + errMsg: "(and the other must not be set)", }, { name: "OIDC provider with valid additionalAuthorizationParams", @@ -671,24 +693,100 @@ func TestMCPExternalAuthConfig_validateUpstreamProvider(t *testing.T) { errMsg: "oauth2Config: exactly one of clientId or dcrConfig must be set", }, { - // Regression guard: conversion only maps DCRConfig in the OAuth2 - // branch, so an OIDC-typed provider carrying OAuth2Config/DCRConfig - // must be rejected at validate time rather than silently dropped. - name: "OIDC provider with OAuth2Config carrying DCRConfig is rejected", + // Regression guard for DCR leakage on OIDC-typed providers: + // conversion only maps DCRConfig in the OAuth2 branch, so a stray + // OAuth2Config/DCRConfig payload on an OIDC-typed provider must be + // rejected at validate time rather than silently dropped. Set + // OIDCConfig so the OIDC half of the discriminator passes; the + // stray OAuth2Config is what trips the rule. + name: "OIDC provider with stray OAuth2Config carrying DCRConfig is rejected", provider: UpstreamProviderConfig{ - Name: "mismatched-oidc", - Type: UpstreamProviderTypeOIDC, + Name: "mismatched-oidc", + Type: UpstreamProviderTypeOIDC, + OIDCConfig: &OIDCUpstreamConfig{IssuerURL: "https://oidc.example.com", ClientID: "client-id"}, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, + }, + }, + expectErr: true, + // The unified discriminator message is the same regardless of + // which side trips it; pin the OAuth2-specific phrasing to + // document that this case is exercising the OAuth2-leakage path. + errMsg: "oauth2Config must be set when type is 'oauth2'", + }, + { + name: "OAuth2 provider with DCRConfig and ClientSecretRef is rejected", + provider: UpstreamProviderConfig{ + Name: "dcr-with-client-secret", + Type: UpstreamProviderTypeOAuth2, OAuth2Config: &OAuth2UpstreamConfig{ AuthorizationEndpoint: "https://idp.example.com/authorize", TokenEndpoint: "https://idp.example.com/token", UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + ClientSecretRef: &SecretKeyRef{Name: "stray", Key: "client-secret"}, DCRConfig: &DCRUpstreamConfig{ DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", }, }, }, expectErr: true, - errMsg: "oidcConfig must be set when type is 'oidc' and must not be set otherwise", + errMsg: "clientSecretRef must not be set when dcrConfig is set", + }, + { + name: "OAuth2 provider with DCRConfig discoveryUrl at MaxLength is accepted", + provider: UpstreamProviderConfig{ + Name: "dcr-url-at-cap", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: makeURLOfLen(MaxDCRURLLength), + }, + }, + }, + expectErr: false, + }, + { + name: "OAuth2 provider with DCRConfig discoveryUrl over MaxLength is rejected", + provider: UpstreamProviderConfig{ + Name: "dcr-url-over-cap", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: makeURLOfLen(MaxDCRURLLength + 1), + }, + }, + }, + expectErr: true, + errMsg: "dcrConfig.discoveryUrl: length", + }, + { + name: "OAuth2 provider with DCRConfig softwareStatement over MaxLength is rejected", + provider: UpstreamProviderConfig{ + Name: "dcr-software-statement-over-cap", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + DCRConfig: &DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + SoftwareStatement: makeStringOfLen(MaxSoftwareStatementLength + 1), + }, + }, + }, + expectErr: true, + errMsg: "dcrConfig.softwareStatement: length", }, } 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 b9d83b5ea0..13004927dc 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 @@ -569,10 +569,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -598,10 +601,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -614,9 +619,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -746,6 +763,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -1710,10 +1731,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -1739,10 +1763,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -1755,9 +1781,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -1887,6 +1925,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. 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 8735ffbe30..3b1a72d21e 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 @@ -442,10 +442,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -471,10 +474,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -487,9 +492,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -619,6 +636,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -3031,10 +3052,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -3060,10 +3084,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -3076,9 +3102,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -3208,6 +3246,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. 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 2ab3edb743..e8e5c0c340 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -572,10 +572,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -601,10 +604,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -617,9 +622,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -749,6 +766,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -1713,10 +1734,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -1742,10 +1766,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -1758,9 +1784,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -1890,6 +1928,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. 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 909455c2a2..a1e0d000c6 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -445,10 +445,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -474,10 +477,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -490,9 +495,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -622,6 +639,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. @@ -3034,10 +3055,13 @@ spec: token_endpoint_auth_methods_supported, and scopes_supported from the response. Mutually exclusive with RegistrationEndpoint. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint resolved from this + document carries the initial access token and the issued client_secret + (RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd + object budget, regex evaluation cost) and matches the conventional URL + length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string initialAccessTokenRef: description: |- @@ -3063,10 +3087,12 @@ spec: expected to also supply AuthorizationEndpoint, TokenEndpoint, and an explicit Scopes list on the parent OAuth2UpstreamConfig. Mutually exclusive with DiscoveryURL. - MaxLength bounds CEL cost estimation on the parent struct's - XValidation rule and matches the conventional URL length cap. + HTTPS is required because the registration endpoint carries the initial + access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3). + MaxLength is a defensive size cap (etcd object budget, regex evaluation + cost) and matches the conventional URL length cap. maxLength: 2048 - pattern: ^https?://.*$ + pattern: ^https://[^\s?#]+[^/\s?#]$ type: string softwareId: description: |- @@ -3079,9 +3105,21 @@ spec: description: |- SoftwareStatement is the RFC 7591 "software_statement" JWT asserting metadata about the client software, signed by a party the authorization - server trusts. Bounded to 4096 characters to prevent unbounded - user input from inflating the CR beyond etcd object limits. - maxLength: 4096 + server trusts. + + Stored inline on the CR. The JWT is signed but not encrypted, so its + contents are visible to anyone with get/list/watch on this resource and + appear in etcd backups in plaintext. Treat the value as non-confidential + (signed attestation, not a secret). Operators that rotate software + statements like bearer credentials should keep them at the authorization + server side and rely on the registration endpoint's initial access + token (see InitialAccessTokenRef) instead of placing them on the CR. + + Bounded to 16384 characters as a defensive size cap (etcd object + budget, regex evaluation cost). Real-world signed statements with + embedded x5c certificate chains, JWKS keys, or OIDC-Federation + trust-framework metadata routinely exceed 4 KB. + maxLength: 16384 type: string type: object x-kubernetes-validations: @@ -3211,6 +3249,10 @@ spec: set rule: '(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)' + - message: clientSecretRef must not be set when dcrConfig + is set; the client_secret is obtained at runtime via + Dynamic Client Registration + rule: '!(has(self.dcrConfig) && has(self.clientSecretRef))' oidcConfig: description: |- OIDCConfig contains OIDC-specific configuration. diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 417593459e..24f5239a4b 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1104,11 +1104,11 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `discoveryUrl` _string_ | DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver
issues a single GET against this URL (no well-known-path fallback) and
reads registration_endpoint, authorization_endpoint, token_endpoint,
token_endpoint_auth_methods_supported, and scopes_supported from the
response.
Mutually exclusive with RegistrationEndpoint.
MaxLength bounds CEL cost estimation on the parent struct's
XValidation rule and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https?://.*$`
Optional: \{\}
| -| `registrationEndpoint` _string_ | RegistrationEndpoint is the RFC 7591 registration endpoint URL used
directly, bypassing discovery. When using this field, the caller is
expected to also supply AuthorizationEndpoint, TokenEndpoint, and an
explicit Scopes list on the parent OAuth2UpstreamConfig.
Mutually exclusive with DiscoveryURL.
MaxLength bounds CEL cost estimation on the parent struct's
XValidation rule and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https?://.*$`
Optional: \{\}
| +| `discoveryUrl` _string_ | DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver
issues a single GET against this URL (no well-known-path fallback) and
reads registration_endpoint, authorization_endpoint, token_endpoint,
token_endpoint_auth_methods_supported, and scopes_supported from the
response.
Mutually exclusive with RegistrationEndpoint.
HTTPS is required because the registration endpoint resolved from this
document carries the initial access token and the issued client_secret
(RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd
object budget, regex evaluation cost) and matches the conventional URL
length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| +| `registrationEndpoint` _string_ | RegistrationEndpoint is the RFC 7591 registration endpoint URL used
directly, bypassing discovery. When using this field, the caller is
expected to also supply AuthorizationEndpoint, TokenEndpoint, and an
explicit Scopes list on the parent OAuth2UpstreamConfig.
Mutually exclusive with DiscoveryURL.
HTTPS is required because the registration endpoint carries the initial
access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3).
MaxLength is a defensive size cap (etcd object budget, regex evaluation
cost) and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| | `initialAccessTokenRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | InitialAccessTokenRef is an optional reference to a Kubernetes Secret
carrying an RFC 7591 §3 initial access token. When set, the resolver
presents the token value as a Bearer credential on the registration
request. Mirrors the ClientSecretRef pattern. | | Optional: \{\}
| | `softwareId` _string_ | SoftwareID is the RFC 7591 "software_id" registration metadata value,
identifying the client software independent of any particular
registration instance. Typically a UUID or short identifier. | | MaxLength: 255
Optional: \{\}
| -| `softwareStatement` _string_ | SoftwareStatement is the RFC 7591 "software_statement" JWT asserting
metadata about the client software, signed by a party the authorization
server trusts. Bounded to 4096 characters to prevent unbounded
user input from inflating the CR beyond etcd object limits. | | MaxLength: 4096
Optional: \{\}
| +| `softwareStatement` _string_ | SoftwareStatement is the RFC 7591 "software_statement" JWT asserting
metadata about the client software, signed by a party the authorization
server trusts.
Stored inline on the CR. The JWT is signed but not encrypted, so its
contents are visible to anyone with get/list/watch on this resource and
appear in etcd backups in plaintext. Treat the value as non-confidential
(signed attestation, not a secret). Operators that rotate software
statements like bearer credentials should keep them at the authorization
server side and rely on the registration endpoint's initial access
token (see InitialAccessTokenRef) instead of placing them on the CR.
Bounded to 16384 characters as a defensive size cap (etcd object
budget, regex evaluation cost). Real-world signed statements with
embedded x5c certificate chains, JWKS keys, or OIDC-Federation
trust-framework metadata routinely exceed 4 KB. | | MaxLength: 16384
Optional: \{\}
| @@ -2596,6 +2596,23 @@ Exactly one of ClientID or DCRConfig must be set: ClientID is used when the client is pre-provisioned out of band, DCRConfig enables RFC 7591 Dynamic Client Registration at runtime. +ClientSecretRef is mutually exclusive with DCRConfig: when DCRConfig is set, +the client_secret is obtained from the registration response (RFC 7591 +§3.2.1) and any static ClientSecretRef would be either dead config or a +competing source of truth. The XValidation rule below rejects the +combination at admission; ValidateOAuth2DCRConfig is the matching +reconcile-time backstop. + +Layered XOR behavior: the ClientID/DCRConfig rule treats `clientId: ""` as +absent (size>0) but treats `dcrConfig: {}` as present (has() is true for an +empty object). For input `{ clientId: "", dcrConfig: {} }` the outer rule +passes and the inner DCRUpstreamConfig XOR fires with "exactly one of +discoveryUrl or registrationEndpoint must be set". This is intentional — +adding a non-empty subfield check (e.g., +`has(self.dcrConfig.discoveryUrl) || has(self.dcrConfig.registrationEndpoint)`) +would inflate CEL cost on an already-budget-bound rule, and the inner +message is still actionable. + _Appears in:_ From 01ae2328873d6880ea9613d0beda6d348f043263 Mon Sep 17 00:00:00 2001 From: Trey Date: Tue, 5 May 2026 07:57:19 -0700 Subject: [PATCH 3/4] Refactor ValidateOAuth2DCRConfig and OAuth2 conversion helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses stacklok/toolhive#5069 review comments: - F4 MEDIUM mcpexternalauthconfig_types.go (3174398117): drop the unenforced `prefix` parameter on ValidateOAuth2DCRConfig and the single-use scopedFieldPath helper. The validator now always emits scoped to "oauth2Config[.dcrConfig[.field]]"; callers wrap with their own outer scope (validateUpstreamProvider wraps with "upstreamProviders[i]: %w"; buildOAuth2UpstreamRunConfig relies on the existing outer "upstream %q: %w" wrap in BuildAuthServerRunConfig). A future caller passing the upstream name can no longer accidentally double-prefix. - F8 LOW authserver.go (3174398131): make buildOAuth2UpstreamRunConfig parallel to buildOIDCUpstreamRunConfig — drop the redundant `provider` and `*upstreamSecretBinding` parameters in favor of a `cfg *OAuth2UpstreamConfig` plus two scalar env-var names. Removes the unenforced provider/binding aliasing invariant. - F13 LOW authserver.go (3174398155): drop named returns from extractUpstreamSecretRefs to match the unnamed-returns convention used by sibling helpers. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 41 ++++++---------- .../pkg/controllerutil/authserver.go | 47 ++++++++++--------- 2 files changed, 41 insertions(+), 47 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 7dc6f35d75..d8049f0c09 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -1110,8 +1110,8 @@ func (*MCPExternalAuthConfig) validateUpstreamProvider(index int, provider *Upst // Validate OAuth2-specific DCR / ClientID constraints (defense-in-depth with CEL). // The discriminator above guarantees OAuth2Config != nil when type is oauth2. if provider.Type == UpstreamProviderTypeOAuth2 { - if err := ValidateOAuth2DCRConfig(prefix, provider.OAuth2Config); err != nil { - return err + if err := ValidateOAuth2DCRConfig(provider.OAuth2Config); err != nil { + return fmt.Errorf("%s: %w", prefix, err) } } @@ -1142,19 +1142,19 @@ const ( // objects stored before CEL rules were added or validated through code paths // that bypass admission). // -// The prefix is embedded in error messages to identify the offending upstream -// (e.g., "upstreamProviders[i]"). Pass an empty string when the caller already -// wraps the error with the upstream identifier, to avoid duplicating it. +// Errors are scoped to "oauth2Config[.dcrConfig[.field]]" so callers can wrap +// with their own outer scope (e.g. "upstreamProviders[i]: %w" or +// "upstream %q: %w") without duplicating the field name. +// // Exported so the controllerutil conversion layer can reuse the same // invariants when building runtime configs, rejecting malformed objects at // reconcile time rather than waiting until the authserver process parses them. -func ValidateOAuth2DCRConfig(prefix string, cfg *OAuth2UpstreamConfig) error { +func ValidateOAuth2DCRConfig(cfg *OAuth2UpstreamConfig) error { hasClientID := cfg.ClientID != "" hasDCR := cfg.DCRConfig != nil - scoped := scopedFieldPath(prefix, "oauth2Config") if hasClientID == hasDCR { - return fmt.Errorf("%s: exactly one of clientId or dcrConfig must be set", scoped) + return fmt.Errorf("oauth2Config: exactly one of clientId or dcrConfig must be set") } if !hasDCR { @@ -1163,41 +1163,30 @@ func ValidateOAuth2DCRConfig(prefix string, cfg *OAuth2UpstreamConfig) error { if cfg.ClientSecretRef != nil { return fmt.Errorf( - "%s: clientSecretRef must not be set when dcrConfig is set; "+ - "the client_secret is obtained at runtime via Dynamic Client Registration", scoped) + "oauth2Config: clientSecretRef must not be set when dcrConfig is set; " + + "the client_secret is obtained at runtime via Dynamic Client Registration") } hasDiscoveryURL := cfg.DCRConfig.DiscoveryURL != "" hasRegistrationEndpoint := cfg.DCRConfig.RegistrationEndpoint != "" if hasDiscoveryURL == hasRegistrationEndpoint { - return fmt.Errorf("%s.dcrConfig: exactly one of discoveryUrl or registrationEndpoint must be set", scoped) + return fmt.Errorf("oauth2Config.dcrConfig: exactly one of discoveryUrl or registrationEndpoint must be set") } if l := len(cfg.DCRConfig.DiscoveryURL); l > MaxDCRURLLength { - return fmt.Errorf("%s.dcrConfig.discoveryUrl: length %d exceeds maximum %d", scoped, l, MaxDCRURLLength) + return fmt.Errorf("oauth2Config.dcrConfig.discoveryUrl: length %d exceeds maximum %d", l, MaxDCRURLLength) } if l := len(cfg.DCRConfig.RegistrationEndpoint); l > MaxDCRURLLength { - return fmt.Errorf("%s.dcrConfig.registrationEndpoint: length %d exceeds maximum %d", scoped, l, MaxDCRURLLength) + return fmt.Errorf("oauth2Config.dcrConfig.registrationEndpoint: length %d exceeds maximum %d", l, MaxDCRURLLength) } if l := len(cfg.DCRConfig.SoftwareStatement); l > MaxSoftwareStatementLength { return fmt.Errorf( - "%s.dcrConfig.softwareStatement: length %d exceeds maximum %d", - scoped, l, MaxSoftwareStatementLength) + "oauth2Config.dcrConfig.softwareStatement: length %d exceeds maximum %d", + l, MaxSoftwareStatementLength) } return nil } -// scopedFieldPath joins a non-empty prefix to a field name with a dot. When -// the prefix is empty, it returns just the field name so callers that already -// supply their own scope (e.g., a wrapping `fmt.Errorf("upstream %q: %w", ...)`) -// don't end up with a leading dot. -func scopedFieldPath(prefix, field string) string { - if prefix == "" { - return field - } - return prefix + "." + field -} - // AdditionalAuthorizationParams returns the additional authorization parameters // from whichever upstream config is set, or nil if none. func (p *UpstreamProviderConfig) AdditionalAuthorizationParams() map[string]string { diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index 862f32547e..47670fb01e 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -127,18 +127,19 @@ func buildUpstreamSecretEnvVars(b *upstreamSecretBinding) []corev1.EnvVar { // the matching Go-level check in validateUpstreamProvider, which together // enforce that exactly one of OIDCConfig / OAuth2Config is set and that it // matches Type): -// - OIDC providers: only `clientSecretRef` is ever non-nil. -// `initialAccessTokenRef` is always nil because DCR is OAuth2-only and +// - OIDC providers: only the client-secret ref is ever non-nil. The +// initial-access-token ref is always nil because DCR is OAuth2-only and // OAuth2Config must be nil for OIDC-typed providers. -// - OAuth2 providers: `clientSecretRef` and `initialAccessTokenRef` are -// independent — either, both, or neither may be non-nil. +// - OAuth2 providers: the two refs are independent — either, both, or +// neither may be non-nil. // - Any other (currently unreachable) Type value: both are nil. // // Callers must not rely on the third bullet to mask an admission-bypassing // object — `BuildAuthServerRunConfig` is the reconcile-time backstop for that. func extractUpstreamSecretRefs( provider *mcpv1beta1.UpstreamProviderConfig, -) (clientSecretRef, initialAccessTokenRef *mcpv1beta1.SecretKeyRef) { +) (*mcpv1beta1.SecretKeyRef, *mcpv1beta1.SecretKeyRef) { + var clientSecretRef, initialAccessTokenRef *mcpv1beta1.SecretKeyRef switch provider.Type { case mcpv1beta1.UpstreamProviderTypeOIDC: if provider.OIDCConfig != nil { @@ -742,7 +743,8 @@ func buildUpstreamRunConfig( } case mcpv1beta1.UpstreamProviderTypeOAuth2: if provider.OAuth2Config != nil { - oauth2, err := buildOAuth2UpstreamRunConfig(provider, b, resourceURL) + oauth2, err := buildOAuth2UpstreamRunConfig( + provider.OAuth2Config, b.EnvVarName, b.DCRInitialAccessTokenEnvVar, resourceURL) if err != nil { return nil, err } @@ -782,22 +784,25 @@ func buildOIDCUpstreamRunConfig( } // buildOAuth2UpstreamRunConfig converts a CRD OAuth2UpstreamConfig to the -// runtime representation. It rejects malformed ClientID/DCRConfig pairs -// before producing a RunConfig — mirroring the CEL XValidation rules on -// OAuth2UpstreamConfig / DCRUpstreamConfig — so objects that reached etcd -// without passing admission (stored-before-CEL, apiserver patches, test -// fixtures bypassing validation) fail at reconcile time rather than at -// authserver startup. +// runtime representation. The signature mirrors buildOIDCUpstreamRunConfig: +// the caller passes the resolved env-var names directly. `clientSecretEnvVar` +// is used when ClientSecretRef is configured; `initialAccessTokenEnvVar` is +// used when DCRConfig.InitialAccessTokenRef is configured. +// +// It rejects malformed ClientID/DCRConfig pairs before producing a RunConfig — +// mirroring the CEL XValidation rules on OAuth2UpstreamConfig / +// DCRUpstreamConfig — so objects that reached etcd without passing admission +// (stored-before-CEL, apiserver patches, test fixtures bypassing validation) +// fail at reconcile time rather than at authserver startup. The validator +// error is returned unwrapped so the outer wrap in BuildAuthServerRunConfig +// (`upstream %q: %w`) supplies the upstream name exactly once. func buildOAuth2UpstreamRunConfig( - provider *mcpv1beta1.UpstreamProviderConfig, - b *upstreamSecretBinding, + cfg *mcpv1beta1.OAuth2UpstreamConfig, + clientSecretEnvVar string, + initialAccessTokenEnvVar string, resourceURL string, ) (*authserver.OAuth2UpstreamRunConfig, error) { - cfg := provider.OAuth2Config - // Pass an empty prefix so the upstream name appears once — supplied by the - // outer wrap in BuildAuthServerRunConfig — instead of duplicating it on - // both the inner and outer error messages. - if err := mcpv1beta1.ValidateOAuth2DCRConfig("", cfg); err != nil { + if err := mcpv1beta1.ValidateOAuth2DCRConfig(cfg); err != nil { return nil, err } @@ -814,7 +819,7 @@ func buildOAuth2UpstreamRunConfig( AdditionalAuthorizationParams: cfg.AdditionalAuthorizationParams, } if cfg.ClientSecretRef != nil { - runConfig.ClientSecretEnvVar = b.EnvVarName + runConfig.ClientSecretEnvVar = clientSecretEnvVar } if cfg.UserInfo != nil { runConfig.UserInfo = buildUserInfoRunConfig(cfg.UserInfo) @@ -829,7 +834,7 @@ func buildOAuth2UpstreamRunConfig( } } if cfg.DCRConfig != nil { - runConfig.DCRConfig = buildDCRUpstreamRunConfig(cfg.DCRConfig, b.DCRInitialAccessTokenEnvVar) + runConfig.DCRConfig = buildDCRUpstreamRunConfig(cfg.DCRConfig, initialAccessTokenEnvVar) } return runConfig, nil } From 1b0e7817a33483810f0198c12e7d9167eefd9a66 Mon Sep 17 00:00:00 2001 From: Trey Date: Tue, 5 May 2026 08:04:50 -0700 Subject: [PATCH 4/4] Strengthen DCR env-var assertions and consolidate redundant tests Addresses stacklok/toolhive#5069 review comments: - F5 MEDIUM authserver_test.go (3174398120): assert SecretKeyRef.Key alongside Name in TestGenerateAuthServerEnvVars; the existing rows used distinct keys ("client-secret" vs "token") but the assertion ignored them, so a refactor that crossed the keys would have passed silently. - F14 LOW authserver_test.go (3174398158): replace the now-invalid ClientSecretRef-plus-DCRConfig case (rejected by the F3 CEL/Go rule) with a multi-upstream DCR row exercising two OAuth2 providers, each with their own InitialAccessTokenRef. A regression that hashed names instead of sanitize-and-uppercase no longer passes silently. - F16 LOW authserver_test.go (3174398170): reduce TestBuildAuthServerRunConfig_InvalidDCR to a single representative case. ValidateOAuth2DCRConfig itself is exhaustively covered in the v1beta1 types_test table; this test now only pins the conversion-layer responsibility (outer `upstream %q:` wrap, single occurrence of the upstream name) which a single case fully exercises. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../pkg/controllerutil/authserver_test.go | 194 ++++++++---------- 1 file changed, 90 insertions(+), 104 deletions(-) diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index ec392f6e70..48e60d5c89 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -348,6 +348,7 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { authConfig *mcpv1beta1.EmbeddedAuthServerConfig wantEnvNames []string wantSecretNames []string // parallel to wantEnvNames; asserts SecretKeyRef.Name + wantSecretKeys []string // parallel to wantEnvNames; asserts SecretKeyRef.Key }{ { name: "nil config returns empty slice", @@ -382,7 +383,9 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { }, }, }, - wantEnvNames: []string{UpstreamClientSecretEnvVar + "_OKTA"}, + wantEnvNames: []string{UpstreamClientSecretEnvVar + "_OKTA"}, + wantSecretNames: []string{"oidc-client-secret"}, + wantSecretKeys: []string{"client-secret"}, }, { name: "OIDC provider without client secret ref (public client)", @@ -424,7 +427,9 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { }, }, }, - wantEnvNames: []string{UpstreamClientSecretEnvVar + "_GITHUB"}, + wantEnvNames: []string{UpstreamClientSecretEnvVar + "_GITHUB"}, + wantSecretNames: []string{"github-client-secret"}, + wantSecretKeys: []string{"client-secret"}, }, { name: "OAuth2 provider without client secret ref", @@ -497,6 +502,7 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { UpstreamClientSecretEnvVar + "_GITHUB", }, wantSecretNames: []string{"okta-secret", "github-secret"}, + wantSecretKeys: []string{"client-secret", "client-secret"}, }, { name: "OAuth2 provider with DCR initial access token ref emits separate env var", @@ -523,9 +529,16 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { }, wantEnvNames: []string{UpstreamDCRInitialAccessTokenEnvVarPrefix + "_ACME_IDP"}, wantSecretNames: []string{"acme-dcr-token"}, + wantSecretKeys: []string{"token"}, }, { - name: "OAuth2 provider with DCR and client secret emits both env vars", + // Regression guard for upstream-secret-binding name derivation. A + // hash-based naming scheme would not produce stable, distinct + // env-var names per provider; sanitize-and-uppercase does. Using + // two OAuth2 + DCR + InitialAccessTokenRef providers exercises + // the multi-upstream branch of GenerateAuthServerEnvVars and pins + // the per-upstream env-var/secret-ref/key triple end to end. + name: "multi-upstream DCR providers each get distinct initial-access-token env vars", authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ Issuer: "https://auth.example.com", UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ @@ -533,18 +546,30 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { Name: "acme-idp", Type: mcpv1beta1.UpstreamProviderTypeOAuth2, OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ - AuthorizationEndpoint: "https://idp.example.com/authorize", - TokenEndpoint: "https://idp.example.com/token", - UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, - ClientSecretRef: &mcpv1beta1.SecretKeyRef{ - Name: "acme-secret", - Key: "client-secret", + AuthorizationEndpoint: "https://acme.example.com/authorize", + TokenEndpoint: "https://acme.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://acme.example.com/userinfo"}, + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://acme.example.com/.well-known/openid-configuration", + InitialAccessTokenRef: &mcpv1beta1.SecretKeyRef{ + Name: "acme-dcr-secret", + Key: "acme-token", + }, }, + }, + }, + { + Name: "globex-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://globex.example.com/authorize", + TokenEndpoint: "https://globex.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://globex.example.com/userinfo"}, DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ - DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + RegistrationEndpoint: "https://globex.example.com/register", InitialAccessTokenRef: &mcpv1beta1.SecretKeyRef{ - Name: "acme-dcr-token", - Key: "token", + Name: "globex-dcr-secret", + Key: "globex-token", }, }, }, @@ -552,10 +577,11 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { }, }, wantEnvNames: []string{ - UpstreamClientSecretEnvVar + "_ACME_IDP", UpstreamDCRInitialAccessTokenEnvVarPrefix + "_ACME_IDP", + UpstreamDCRInitialAccessTokenEnvVarPrefix + "_GLOBEX_IDP", }, - wantSecretNames: []string{"acme-secret", "acme-dcr-token"}, + wantSecretNames: []string{"acme-dcr-secret", "globex-dcr-secret"}, + wantSecretKeys: []string{"acme-token", "globex-token"}, }, { name: "OAuth2 provider with DCR but no initial access token ref emits no DCR env var", @@ -597,7 +623,12 @@ func TestGenerateAuthServerEnvVars(t *testing.T) { require.NotNil(t, envVars[i].ValueFrom) require.NotNil(t, envVars[i].ValueFrom.SecretKeyRef) if len(tt.wantSecretNames) > i { - assert.Equal(t, tt.wantSecretNames[i], envVars[i].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, tt.wantSecretNames[i], envVars[i].ValueFrom.SecretKeyRef.Name, + "env %s should reference secret name %s", wantName, tt.wantSecretNames[i]) + } + if len(tt.wantSecretKeys) > i { + assert.Equal(t, tt.wantSecretKeys[i], envVars[i].ValueFrom.SecretKeyRef.Key, + "env %s should reference secret key %s", wantName, tt.wantSecretKeys[i]) } } }) @@ -1408,106 +1439,61 @@ func TestBuildAuthServerRunConfig(t *testing.T) { } // TestBuildAuthServerRunConfig_InvalidDCR verifies that BuildAuthServerRunConfig -// rejects OAuth2 upstreams whose ClientID / DCRConfig pairing violates the -// mutual-exclusivity invariants (matching the CEL XValidation rules on -// OAuth2UpstreamConfig and DCRUpstreamConfig). This is the reconcile-time -// defense-in-depth against stored objects that bypassed admission. +// surfaces ValidateOAuth2DCRConfig errors with a single outer +// `upstream %q:` wrap and no inner-prefix duplication. +// +// ValidateOAuth2DCRConfig itself is exhaustively tested in +// TestMCPExternalAuthConfig_validateUpstreamProvider in the v1beta1 package +// (each XOR violation, the ClientSecretRef ⊥ DCRConfig rule, and the length +// caps). Mirroring those cases here would duplicate that table; the unique +// thing this test pins is the conversion-layer wrapping behavior, which is +// fully exercised by a single representative violation. func TestBuildAuthServerRunConfig_InvalidDCR(t *testing.T) { t.Parallel() - defaultAudiences := []string{"http://test-server.default.svc.cluster.local:8080"} - defaultScopes := []string{"openid", "offline_access"} - - tests := []struct { - name string - oauth2Cfg *mcpv1beta1.OAuth2UpstreamConfig - wantErr string - }{ - { - name: "both ClientID and DCRConfig set", - oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ - AuthorizationEndpoint: "https://idp.example.com/authorize", - TokenEndpoint: "https://idp.example.com/token", - UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, - ClientID: "pre-provisioned-id", - DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ - DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", - }, - }, - wantErr: "exactly one of clientId or dcrConfig must be set", + authConfig := &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, }, - { - name: "neither ClientID nor DCRConfig set", - oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ - AuthorizationEndpoint: "https://idp.example.com/authorize", - TokenEndpoint: "https://idp.example.com/token", - UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, - }, - wantErr: "exactly one of clientId or dcrConfig must be set", + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, }, - { - name: "DCRConfig with both endpoints set", - oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ - AuthorizationEndpoint: "https://idp.example.com/authorize", - TokenEndpoint: "https://idp.example.com/token", - UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, - DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ - DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", - RegistrationEndpoint: "https://idp.example.com/register", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + { + Name: "acme-idp", + Type: mcpv1beta1.UpstreamProviderTypeOAuth2, + OAuth2Config: &mcpv1beta1.OAuth2UpstreamConfig{ + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, + ClientID: "pre-provisioned-id", + DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", + }, }, }, - wantErr: "exactly one of discoveryUrl or registrationEndpoint must be set", - }, - { - name: "DCRConfig with neither endpoint set", - oauth2Cfg: &mcpv1beta1.OAuth2UpstreamConfig{ - AuthorizationEndpoint: "https://idp.example.com/authorize", - TokenEndpoint: "https://idp.example.com/token", - UserInfo: &mcpv1beta1.UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, - DCRConfig: &mcpv1beta1.DCRUpstreamConfig{}, - }, - wantErr: "exactly one of discoveryUrl or registrationEndpoint must be set", }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - authConfig := &mcpv1beta1.EmbeddedAuthServerConfig{ - Issuer: "https://auth.example.com", - SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ - {Name: "signing-key", Key: "private.pem"}, - }, - HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ - {Name: "hmac-secret", Key: "hmac"}, - }, - UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ - { - Name: "acme-idp", - Type: mcpv1beta1.UpstreamProviderTypeOAuth2, - OAuth2Config: tt.oauth2Cfg, - }, - }, - } - - config, err := BuildAuthServerRunConfig( - "default", "test-server", authConfig, - defaultAudiences, defaultScopes, - "http://test-server.default.svc.cluster.local:8080", - ) + config, err := BuildAuthServerRunConfig( + "default", "test-server", authConfig, + []string{"http://test-server.default.svc.cluster.local:8080"}, + []string{"openid", "offline_access"}, + "http://test-server.default.svc.cluster.local:8080", + ) - require.Error(t, err, "expected BuildAuthServerRunConfig to fail on invalid DCR pairing") - assert.Nil(t, config) - assert.Contains(t, err.Error(), tt.wantErr, - "error should surface the mutual-exclusivity violation") - // The upstream name must appear exactly once: the outer wrap in - // BuildAuthServerRunConfig supplies it, and the inner validator is - // invoked with an empty prefix so it doesn't duplicate the name. - assert.Equal(t, 1, strings.Count(err.Error(), "acme-idp"), - "upstream name should appear exactly once in error: %q", err.Error()) - }) - } + require.Error(t, err, "expected BuildAuthServerRunConfig to fail on invalid DCR pairing") + assert.Nil(t, config) + assert.Contains(t, err.Error(), "exactly one of clientId or dcrConfig must be set", + "outer wrap should surface the validator's diagnostic") + assert.True(t, strings.HasPrefix(err.Error(), `upstream "acme-idp":`), + "outer wrap should prefix with upstream %%q (got %q)", err.Error()) + // The upstream name must appear exactly once: the outer wrap in + // BuildAuthServerRunConfig supplies it, and ValidateOAuth2DCRConfig is + // called without a prefix so it doesn't duplicate the name. + assert.Equal(t, 1, strings.Count(err.Error(), "acme-idp"), + "upstream name should appear exactly once in error: %q", err.Error()) } func TestAddEmbeddedAuthServerConfigOptions_Validation(t *testing.T) {