From e2b7f5bc5d227a2fc1c4674264cebce7bc33da06 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 10 Jun 2026 14:48:49 -0700 Subject: [PATCH 1/6] Define OBOConfig CRD schema for Entra OBO flow The mcpv1beta1.OBOConfig struct was an empty placeholder deferred to a follow-up RFC. The enterprise OBO overlay needs a user-facing config surface to read, so populate OBOConfig with the fields the Microsoft Entra OBO flow requires. Field names and semantics track the shared obo.MiddlewareParameters wire contract (not the upstream TokenExchangeConfig): tenantId (+ optional authority) maps to the contract's tokenUrl, clientSecretRef to clientSecretEnvVar, audience/scopes collapse via ExchangeTarget(), and the subject source is selected by subjectTokenProviderName. There is deliberately no externalTokenHeaderName -- the OBO subject comes from the authenticated Identity, not a request header. The schema is structurally valid upstream but inert: an OBO-typed config still surfaces Valid=False / Reason=EnterpriseRequired at reconcile because no OBO handler is registered in upstream builds. Field-level validation lives in kubebuilder markers plus a CEL rule (admission) and the enterprise handler (reconcile); the Go Validate() arm continues to defer. Regenerated deepcopy, CRD manifests, and CRD API docs. Added an envtest suite exercising the new admission-time validation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 150 ++++++++++-- .../mcpexternalauthconfig_types_test.go | 27 ++- .../api/v1beta1/zz_generated.deepcopy.go | 17 +- .../mcp-external-auth/obo_validation_test.go | 185 +++++++++++++++ ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 218 +++++++++++++++++- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 218 +++++++++++++++++- docs/operator/crd-api.md | 45 +++- 7 files changed, 813 insertions(+), 47 deletions(-) create mode 100644 cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 2170dce581..4a85a12731 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -106,22 +106,124 @@ type MCPExternalAuthConfigSpec struct { UpstreamInject *UpstreamInjectSpec `json:"upstreamInject,omitempty"` // OBO configures On-Behalf-Of (OBO) authentication. - // Only used when Type is "obo". The inner schema is intentionally empty in - // this revision; sub-fields land in a follow-up. Setting this field on an - // upstream-only build will cause the MCPExternalAuthConfig to transition to - // status.conditions[Valid] = False with Reason: EnterpriseRequired. + // Only used when Type is "obo". Setting this field on an upstream-only build + // causes the MCPExternalAuthConfig to transition to + // status.conditions[Valid] = False with Reason: EnterpriseRequired, because + // no OBO handler is registered. See OBOConfig for the field-to-runtime + // contract mapping. // +optional OBO *OBOConfig `json:"obo,omitempty"` } -// OBOConfig is a placeholder for On-Behalf-Of (OBO) external auth configuration. -// The inner schema is intentionally empty in this revision; sub-fields land in a -// follow-up RFC. The struct exists so OBO *OBOConfig compiles and the CRD -// schema admits `spec.obo: {}` — the CEL rule "obo configuration must be set -// if and only if type is 'obo'" requires has(self.obo), which evaluates true -// for an empty object. Stored objects with `obo: {}` will round-trip cleanly -// when sub-fields land, because Go zero values fill in. -type OBOConfig struct{} +// OBOConfig holds configuration for the On-Behalf-Of (OBO) external auth type. +// Only used when Type is "obo". +// +// This is the user-facing CRD surface for the Microsoft Entra OBO flow. It is +// structurally valid in upstream (OSS) builds but inert: an upstream-only build +// returns obo.ErrEnterpriseRequired at reconcile (Valid=False, Reason: +// EnterpriseRequired) because no OBO handler is registered via +// controllerutil.RegisterOBOHandler. A build with the enterprise OBO handler +// translates these fields into the runtime wire contract obo.MiddlewareParameters, +// so the field names and semantics here track that contract rather than the +// upstream TokenExchangeConfig (which uses different names, e.g. +// subjectProviderName / externalTokenHeaderName). In particular there is no +// externalTokenHeaderName: the OBO subject is sourced from the authenticated +// Identity, never from an inbound request header. +// +// Field-to-contract mapping performed by the operator (#1581): +// - tenantId (+ optional authority) → tokenUrl +// (https://login.microsoftonline.com//oauth2/v2.0/token, or the +// authority host for sovereign clouds) +// - clientSecretRef → resolved into a pod env var; only the env var name +// travels in the contract, as clientSecretEnvVar +// - audience / scopes → collapsed to a single exchange target by +// obo.MiddlewareParameters.ExchangeTarget() (space-joined scopes win, +// otherwise audience) +// - cacheSkew → the contract's integer-seconds cacheSkewSeconds +// +// +kubebuilder:validation:XValidation:rule="(has(self.audience) && size(self.audience) > 0) || (has(self.scopes) && size(self.scopes) > 0)",message="at least one of audience or scopes must be set" +// +//nolint:lll // CEL validation rules exceed line length limit +type OBOConfig struct { + // TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + // Accepts a tenant GUID, a verified domain name (e.g. + // contoso.onmicrosoft.com), or one of the well-known values "common", + // "organizations", "consumers". The operator interpolates it into the Entra + // token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) + // emitted as the runtime contract's tokenUrl, so the value is constrained to + // a single safe path segment (no slashes, whitespace, query, or fragment). + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9.-]*$` + TenantID string `json:"tenantId"` + + // Authority overrides the default Entra login host + // (https://login.microsoftonline.com) for sovereign or national clouds, e.g. + // https://login.microsoftonline.us (US Gov) or + // https://login.partner.microsoftonline.cn (China). When set, the operator + // builds the token endpoint as //oauth2/v2.0/token. + // Must be an HTTPS URL with no path, query, fragment, or trailing slash: the + // OBO exchange POSTs the client secret and the end-user assertion to this + // host, so it is a trust boundary and HTTPS is required. + // +kubebuilder:validation:Pattern=`^https://[^\s?#]+[^/\s?#]$` + // +optional + Authority string `json:"authority,omitempty"` + + // ClientID is the confidential client's application (client) ID registered + // in Entra. Emitted verbatim as the runtime contract's clientId. + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 + ClientID string `json:"clientId"` + + // ClientSecretRef references a Kubernetes Secret containing the confidential + // client's secret. v1 supports a shared client secret only. The operator + // injects the resolved value into the proxyrunner pod as an environment + // variable and emits only that variable's name in the runtime contract, as + // clientSecretEnvVar — the secret value never travels in the contract. + // +kubebuilder:validation:Required + ClientSecretRef *SecretKeyRef `json:"clientSecretRef"` + + // Audience is the backend target identifier requested in the exchanged + // token. Used as the exchange target when Scopes is empty. At least one of + // audience or scopes must be set. + // +optional + Audience string `json:"audience,omitempty"` + + // Scopes are the delegated scopes to request for the exchanged token, e.g. + // ["api:///.default"]. When non-empty they take precedence over + // Audience. At least one of audience or scopes must be set. + // +listType=atomic + // +optional + Scopes []string `json:"scopes,omitempty"` + + // SubjectTokenProviderName selects the source of the OBO subject (assertion) + // token from the request's authenticated Identity: + // - Omitted: use the inbound end-user token the client presented + // (Identity.Token) — the deployment with no embedded auth server, where + // the client holds an Entra token directly. + // - Set: use the named upstream provider's token + // (Identity.UpstreamTokens[]) — the embedded-auth-server + // deployment, where the inbound token is the proxy's own session token. + // The value must match a configured upstream provider name. + // The subject is always sourced from the authenticated Identity, never from + // an inbound request header, so the upstream auth middleware must run first. + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Pattern=`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$` + // +optional + SubjectTokenProviderName string `json:"subjectTokenProviderName,omitempty"` + + // CacheSkew overrides the OBO token cache's default expiry skew (the margin + // by which a cached token is treated as expired before its real expiry), + // e.g. "30s". The operator converts it to the runtime contract's + // integer-seconds cacheSkewSeconds. Must not be negative; a negative value + // is rejected by the registered OBO handler at reconcile time + // (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, + // the cache default applies. + // +optional + CacheSkew *metav1.Duration `json:"cacheSkew,omitempty"` +} // TokenExchangeConfig holds configuration for RFC-8693 OAuth 2.0 Token Exchange. // This configuration is used to exchange incoming authentication tokens for tokens @@ -1212,16 +1314,22 @@ func (r *MCPExternalAuthConfig) Validate() error { // has already run via r.validateTypeConfigConsistency() at the top // of this method, so this arm is reached only when the structural // invariant holds — and the matching CEL rule on the spec catches - // it at admission time. The remaining semantic validation - // (e.g., whether the cluster has an OBO handler registered) runs - // at reconcile time via the controllerutil.OBOValidate - // function-pointer hook: upstream-only builds return - // obo.ErrEnterpriseRequired, which the reconciler maps to - // status.conditions[Valid] = False / Reason: EnterpriseRequired. - // Out-of-tree builds that register a handler via + // it at admission time. Field-level validation of OBOConfig + // (required tenantId/clientId/clientSecretRef, at-least-one-of + // audience/scopes, authority/tenantId shape, duration format of + // cacheSkew) is enforced by the kubebuilder markers and the OBOConfig + // CEL rule at admission, not here: OBOConfig is a brand-new field set + // with no pre-CEL stored objects to backfill, and the + // semantic/protocol validation that would justify a reconcile-time + // backstop (including rejecting a negative cacheSkew) is owned by the + // registered handler, not the upstream type. That handler runs at reconcile + // time via the controllerutil.OBOValidate function-pointer hook: + // upstream-only builds return obo.ErrEnterpriseRequired, which the + // reconciler maps to status.conditions[Valid] = False / Reason: + // EnterpriseRequired. Out-of-tree builds that register a handler via // controllerutil.RegisterOBOHandler short-circuit the sentinel and - // run their own protocol-level checks. Splitting the tiers this - // way keeps the upstream CRD schema stable across builds. + // run their own protocol-level checks. Splitting the tiers this way + // keeps the upstream CRD schema stable across builds. return nil default: // Unknown type - should be caught by enum validation, but handle defensively diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go index 6b5a528eae..37bb553d41 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types_test.go @@ -276,7 +276,32 @@ func TestMCPExternalAuthConfig_Validate(t *testing.T) { errMsg: "upstreamInject requires a non-empty providerName", }, { - name: "valid obo type with placeholder OBOConfig", + name: "valid obo type with fully populated OBOConfig", + config: &MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-obo-full", + Namespace: "default", + }, + Spec: MCPExternalAuthConfigSpec{ + Type: ExternalAuthTypeOBO, + OBO: &OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: &SecretKeyRef{Name: "entra-client", Key: "clientSecret"}, + Audience: "api://backend", + }, + }, + }, + expectErr: false, + }, + { + // Go Validate() intentionally does NOT check OBOConfig fields: the + // required-field, pattern, and "at least one of audience or scopes" + // rules are enforced by the kubebuilder markers + CEL at admission, + // and the registered OBO handler validates semantics at reconcile. + // So a minimal obo block passes the Go method even though the + // apiserver would reject it (covered by the envtest CEL suite). + name: "obo type with minimal OBOConfig passes Go Validate (field checks deferred)", config: &MCPExternalAuthConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-obo", diff --git a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go index cbfcf8cf8d..91a49a5e90 100644 --- a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go +++ b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go @@ -789,7 +789,7 @@ func (in *MCPExternalAuthConfigSpec) DeepCopyInto(out *MCPExternalAuthConfigSpec if in.OBO != nil { in, out := &in.OBO, &out.OBO *out = new(OBOConfig) - **out = **in + (*in).DeepCopyInto(*out) } } @@ -2121,6 +2121,21 @@ func (in *OAuth2UpstreamConfig) DeepCopy() *OAuth2UpstreamConfig { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OBOConfig) DeepCopyInto(out *OBOConfig) { *out = *in + if in.ClientSecretRef != nil { + in, out := &in.ClientSecretRef, &out.ClientSecretRef + *out = new(SecretKeyRef) + **out = **in + } + if in.Scopes != nil { + in, out := &in.Scopes, &out.Scopes + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.CacheSkew != nil { + in, out := &in.CacheSkew, &out.CacheSkew + *out = new(v1.Duration) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OBOConfig. diff --git a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go new file mode 100644 index 0000000000..5bdf86be29 --- /dev/null +++ b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go @@ -0,0 +1,185 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" +) + +// These tests exercise the kubebuilder validation on OBOConfig (required +// fields, field patterns, and the "at least one of audience or scopes" CEL +// rule) through the real apiserver (envtest). They are the admission-time half +// of the OBOConfig validation contract: the upstream Go Validate() arm +// intentionally defers field-level validation to these markers and to the +// registered enterprise OBO handler, so the apiserver is where a malformed +// obo spec must be rejected. +var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", "cel", "validation"), func() { + const namespace = "default" + + // makeOBOConfig returns an MCPExternalAuthConfig of type "obo" whose only + // varying piece is the OBOConfig block, so each test controls exactly the + // fields under test. The referenced Secret need not exist: admission only + // validates the structural schema; secret resolution happens at reconcile. + makeOBOConfig := func(name string, obo *mcpv1beta1.OBOConfig) *mcpv1beta1.MCPExternalAuthConfig { + return &mcpv1beta1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: mcpv1beta1.MCPExternalAuthConfigSpec{ + Type: mcpv1beta1.ExternalAuthTypeOBO, + OBO: obo, + }, + } + } + + secretRef := &mcpv1beta1.SecretKeyRef{Name: "entra-client", Key: "clientSecret"} + + Context("valid configurations", func() { + It("should accept a minimal config with audience", func() { + cfg := makeOBOConfig("obo-valid-audience", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) + }) + + It("should accept a config using scopes instead of audience", func() { + cfg := makeOBOConfig("obo-valid-scopes", &mcpv1beta1.OBOConfig{ + TenantID: "contoso.onmicrosoft.com", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Scopes: []string{"api://backend/.default"}, + }) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) + }) + + It("should accept all optional fields set (authority, subjectTokenProviderName, cacheSkew)", func() { + cfg := makeOBOConfig("obo-valid-full", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "https://login.microsoftonline.us", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + Scopes: []string{"api://backend/.default"}, + SubjectTokenProviderName: "corp-idp", + CacheSkew: &metav1.Duration{Duration: 30_000_000_000}, // 30s + }) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) + }) + }) + + Context("required fields", func() { + It("should reject an empty OBOConfig (missing required fields)", func() { + cfg := makeOBOConfig("obo-empty", &mcpv1beta1.OBOConfig{}) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + + It("should reject a missing tenantId", func() { + cfg := makeOBOConfig("obo-no-tenant", &mcpv1beta1.OBOConfig{ + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("tenantId")) + }) + + It("should reject a missing clientId", func() { + cfg := makeOBOConfig("obo-no-client", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("clientId")) + }) + + It("should reject a missing clientSecretRef", func() { + cfg := makeOBOConfig("obo-no-secret", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + Audience: "api://backend", + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("clientSecretRef")) + }) + }) + + Context("at least one of audience or scopes", func() { + It("should reject when neither audience nor scopes is set", func() { + cfg := makeOBOConfig("obo-no-target", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("at least one of audience or scopes must be set")) + }) + + It("should reject when scopes is an empty list and audience is unset", func() { + cfg := makeOBOConfig("obo-empty-scopes", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Scopes: []string{}, + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("at least one of audience or scopes must be set")) + }) + }) + + Context("field patterns", func() { + It("should reject a tenantId containing a path separator", func() { + cfg := makeOBOConfig("obo-bad-tenant", &mcpv1beta1.OBOConfig{ + TenantID: "tenant/../evil", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + + It("should reject a non-HTTPS authority", func() { + cfg := makeOBOConfig("obo-http-authority", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "http://login.microsoftonline.us", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + + It("should reject an authority with a trailing slash", func() { + cfg := makeOBOConfig("obo-authority-slash", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "https://login.microsoftonline.us/", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + + It("should reject an uppercase subjectTokenProviderName", func() { + cfg := makeOBOConfig("obo-bad-subject", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + SubjectTokenProviderName: "Corp-IDP", + }) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + }) +}) 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 0239662a04..516461287d 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 @@ -1079,11 +1079,112 @@ spec: obo: description: |- OBO configures On-Behalf-Of (OBO) authentication. - Only used when Type is "obo". The inner schema is intentionally empty in - this revision; sub-fields land in a follow-up. Setting this field on an - upstream-only build will cause the MCPExternalAuthConfig to transition to - status.conditions[Valid] = False with Reason: EnterpriseRequired. + Only used when Type is "obo". Setting this field on an upstream-only build + causes the MCPExternalAuthConfig to transition to + status.conditions[Valid] = False with Reason: EnterpriseRequired, because + no OBO handler is registered. See OBOConfig for the field-to-runtime + contract mapping. + properties: + audience: + description: |- + Audience is the backend target identifier requested in the exchanged + token. Used as the exchange target when Scopes is empty. At least one of + audience or scopes must be set. + type: string + authority: + description: |- + Authority overrides the default Entra login host + (https://login.microsoftonline.com) for sovereign or national clouds, e.g. + https://login.microsoftonline.us (US Gov) or + https://login.partner.microsoftonline.cn (China). When set, the operator + builds the token endpoint as //oauth2/v2.0/token. + Must be an HTTPS URL with no path, query, fragment, or trailing slash: the + OBO exchange POSTs the client secret and the end-user assertion to this + host, so it is a trust boundary and HTTPS is required. + pattern: ^https://[^\s?#]+[^/\s?#]$ + type: string + cacheSkew: + description: |- + CacheSkew overrides the OBO token cache's default expiry skew (the margin + by which a cached token is treated as expired before its real expiry), + e.g. "30s". The operator converts it to the runtime contract's + integer-seconds cacheSkewSeconds. Must not be negative; a negative value + is rejected by the registered OBO handler at reconcile time + (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, + the cache default applies. + type: string + clientId: + description: |- + ClientID is the confidential client's application (client) ID registered + in Entra. Emitted verbatim as the runtime contract's clientId. + minLength: 1 + type: string + clientSecretRef: + description: |- + ClientSecretRef references a Kubernetes Secret containing the confidential + client's secret. v1 supports a shared client secret only. The operator + injects the resolved value into the proxyrunner pod as an environment + variable and emits only that variable's name in the runtime contract, as + clientSecretEnvVar — the secret value never travels in the contract. + 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 + scopes: + description: |- + Scopes are the delegated scopes to request for the exchanged token, e.g. + ["api:///.default"]. When non-empty they take precedence over + Audience. At least one of audience or scopes must be set. + items: + type: string + type: array + x-kubernetes-list-type: atomic + subjectTokenProviderName: + description: |- + SubjectTokenProviderName selects the source of the OBO subject (assertion) + token from the request's authenticated Identity: + - Omitted: use the inbound end-user token the client presented + (Identity.Token) — the deployment with no embedded auth server, where + the client holds an Entra token directly. + - Set: use the named upstream provider's token + (Identity.UpstreamTokens[]) — the embedded-auth-server + deployment, where the inbound token is the proxy's own session token. + The value must match a configured upstream provider name. + The subject is always sourced from the authenticated Identity, never from + an inbound request header, so the upstream auth middleware must run first. + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ + type: string + tenantId: + description: |- + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Accepts a tenant GUID, a verified domain name (e.g. + contoso.onmicrosoft.com), or one of the well-known values "common", + "organizations", "consumers". The operator interpolates it into the Entra + token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) + emitted as the runtime contract's tokenUrl, so the value is constrained to + a single safe path segment (no slashes, whitespace, query, or fragment). + maxLength: 253 + minLength: 1 + pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + type: string + required: + - clientId + - clientSecretRef + - tenantId type: object + x-kubernetes-validations: + - message: at least one of audience or scopes must be set + rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) + && size(self.scopes) > 0) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2388,11 +2489,112 @@ spec: obo: description: |- OBO configures On-Behalf-Of (OBO) authentication. - Only used when Type is "obo". The inner schema is intentionally empty in - this revision; sub-fields land in a follow-up. Setting this field on an - upstream-only build will cause the MCPExternalAuthConfig to transition to - status.conditions[Valid] = False with Reason: EnterpriseRequired. + Only used when Type is "obo". Setting this field on an upstream-only build + causes the MCPExternalAuthConfig to transition to + status.conditions[Valid] = False with Reason: EnterpriseRequired, because + no OBO handler is registered. See OBOConfig for the field-to-runtime + contract mapping. + properties: + audience: + description: |- + Audience is the backend target identifier requested in the exchanged + token. Used as the exchange target when Scopes is empty. At least one of + audience or scopes must be set. + type: string + authority: + description: |- + Authority overrides the default Entra login host + (https://login.microsoftonline.com) for sovereign or national clouds, e.g. + https://login.microsoftonline.us (US Gov) or + https://login.partner.microsoftonline.cn (China). When set, the operator + builds the token endpoint as //oauth2/v2.0/token. + Must be an HTTPS URL with no path, query, fragment, or trailing slash: the + OBO exchange POSTs the client secret and the end-user assertion to this + host, so it is a trust boundary and HTTPS is required. + pattern: ^https://[^\s?#]+[^/\s?#]$ + type: string + cacheSkew: + description: |- + CacheSkew overrides the OBO token cache's default expiry skew (the margin + by which a cached token is treated as expired before its real expiry), + e.g. "30s". The operator converts it to the runtime contract's + integer-seconds cacheSkewSeconds. Must not be negative; a negative value + is rejected by the registered OBO handler at reconcile time + (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, + the cache default applies. + type: string + clientId: + description: |- + ClientID is the confidential client's application (client) ID registered + in Entra. Emitted verbatim as the runtime contract's clientId. + minLength: 1 + type: string + clientSecretRef: + description: |- + ClientSecretRef references a Kubernetes Secret containing the confidential + client's secret. v1 supports a shared client secret only. The operator + injects the resolved value into the proxyrunner pod as an environment + variable and emits only that variable's name in the runtime contract, as + clientSecretEnvVar — the secret value never travels in the contract. + 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 + scopes: + description: |- + Scopes are the delegated scopes to request for the exchanged token, e.g. + ["api:///.default"]. When non-empty they take precedence over + Audience. At least one of audience or scopes must be set. + items: + type: string + type: array + x-kubernetes-list-type: atomic + subjectTokenProviderName: + description: |- + SubjectTokenProviderName selects the source of the OBO subject (assertion) + token from the request's authenticated Identity: + - Omitted: use the inbound end-user token the client presented + (Identity.Token) — the deployment with no embedded auth server, where + the client holds an Entra token directly. + - Set: use the named upstream provider's token + (Identity.UpstreamTokens[]) — the embedded-auth-server + deployment, where the inbound token is the proxy's own session token. + The value must match a configured upstream provider name. + The subject is always sourced from the authenticated Identity, never from + an inbound request header, so the upstream auth middleware must run first. + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ + type: string + tenantId: + description: |- + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Accepts a tenant GUID, a verified domain name (e.g. + contoso.onmicrosoft.com), or one of the well-known values "common", + "organizations", "consumers". The operator interpolates it into the Entra + token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) + emitted as the runtime contract's tokenUrl, so the value is constrained to + a single safe path segment (no slashes, whitespace, query, or fragment). + maxLength: 253 + minLength: 1 + pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + type: string + required: + - clientId + - clientSecretRef + - tenantId type: object + x-kubernetes-validations: + - message: at least one of audience or scopes must be set + rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) + && size(self.scopes) > 0) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange 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 b032071c98..e1bf297030 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1082,11 +1082,112 @@ spec: obo: description: |- OBO configures On-Behalf-Of (OBO) authentication. - Only used when Type is "obo". The inner schema is intentionally empty in - this revision; sub-fields land in a follow-up. Setting this field on an - upstream-only build will cause the MCPExternalAuthConfig to transition to - status.conditions[Valid] = False with Reason: EnterpriseRequired. + Only used when Type is "obo". Setting this field on an upstream-only build + causes the MCPExternalAuthConfig to transition to + status.conditions[Valid] = False with Reason: EnterpriseRequired, because + no OBO handler is registered. See OBOConfig for the field-to-runtime + contract mapping. + properties: + audience: + description: |- + Audience is the backend target identifier requested in the exchanged + token. Used as the exchange target when Scopes is empty. At least one of + audience or scopes must be set. + type: string + authority: + description: |- + Authority overrides the default Entra login host + (https://login.microsoftonline.com) for sovereign or national clouds, e.g. + https://login.microsoftonline.us (US Gov) or + https://login.partner.microsoftonline.cn (China). When set, the operator + builds the token endpoint as //oauth2/v2.0/token. + Must be an HTTPS URL with no path, query, fragment, or trailing slash: the + OBO exchange POSTs the client secret and the end-user assertion to this + host, so it is a trust boundary and HTTPS is required. + pattern: ^https://[^\s?#]+[^/\s?#]$ + type: string + cacheSkew: + description: |- + CacheSkew overrides the OBO token cache's default expiry skew (the margin + by which a cached token is treated as expired before its real expiry), + e.g. "30s". The operator converts it to the runtime contract's + integer-seconds cacheSkewSeconds. Must not be negative; a negative value + is rejected by the registered OBO handler at reconcile time + (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, + the cache default applies. + type: string + clientId: + description: |- + ClientID is the confidential client's application (client) ID registered + in Entra. Emitted verbatim as the runtime contract's clientId. + minLength: 1 + type: string + clientSecretRef: + description: |- + ClientSecretRef references a Kubernetes Secret containing the confidential + client's secret. v1 supports a shared client secret only. The operator + injects the resolved value into the proxyrunner pod as an environment + variable and emits only that variable's name in the runtime contract, as + clientSecretEnvVar — the secret value never travels in the contract. + 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 + scopes: + description: |- + Scopes are the delegated scopes to request for the exchanged token, e.g. + ["api:///.default"]. When non-empty they take precedence over + Audience. At least one of audience or scopes must be set. + items: + type: string + type: array + x-kubernetes-list-type: atomic + subjectTokenProviderName: + description: |- + SubjectTokenProviderName selects the source of the OBO subject (assertion) + token from the request's authenticated Identity: + - Omitted: use the inbound end-user token the client presented + (Identity.Token) — the deployment with no embedded auth server, where + the client holds an Entra token directly. + - Set: use the named upstream provider's token + (Identity.UpstreamTokens[]) — the embedded-auth-server + deployment, where the inbound token is the proxy's own session token. + The value must match a configured upstream provider name. + The subject is always sourced from the authenticated Identity, never from + an inbound request header, so the upstream auth middleware must run first. + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ + type: string + tenantId: + description: |- + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Accepts a tenant GUID, a verified domain name (e.g. + contoso.onmicrosoft.com), or one of the well-known values "common", + "organizations", "consumers". The operator interpolates it into the Entra + token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) + emitted as the runtime contract's tokenUrl, so the value is constrained to + a single safe path segment (no slashes, whitespace, query, or fragment). + maxLength: 253 + minLength: 1 + pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + type: string + required: + - clientId + - clientSecretRef + - tenantId type: object + x-kubernetes-validations: + - message: at least one of audience or scopes must be set + rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) + && size(self.scopes) > 0) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2391,11 +2492,112 @@ spec: obo: description: |- OBO configures On-Behalf-Of (OBO) authentication. - Only used when Type is "obo". The inner schema is intentionally empty in - this revision; sub-fields land in a follow-up. Setting this field on an - upstream-only build will cause the MCPExternalAuthConfig to transition to - status.conditions[Valid] = False with Reason: EnterpriseRequired. + Only used when Type is "obo". Setting this field on an upstream-only build + causes the MCPExternalAuthConfig to transition to + status.conditions[Valid] = False with Reason: EnterpriseRequired, because + no OBO handler is registered. See OBOConfig for the field-to-runtime + contract mapping. + properties: + audience: + description: |- + Audience is the backend target identifier requested in the exchanged + token. Used as the exchange target when Scopes is empty. At least one of + audience or scopes must be set. + type: string + authority: + description: |- + Authority overrides the default Entra login host + (https://login.microsoftonline.com) for sovereign or national clouds, e.g. + https://login.microsoftonline.us (US Gov) or + https://login.partner.microsoftonline.cn (China). When set, the operator + builds the token endpoint as //oauth2/v2.0/token. + Must be an HTTPS URL with no path, query, fragment, or trailing slash: the + OBO exchange POSTs the client secret and the end-user assertion to this + host, so it is a trust boundary and HTTPS is required. + pattern: ^https://[^\s?#]+[^/\s?#]$ + type: string + cacheSkew: + description: |- + CacheSkew overrides the OBO token cache's default expiry skew (the margin + by which a cached token is treated as expired before its real expiry), + e.g. "30s". The operator converts it to the runtime contract's + integer-seconds cacheSkewSeconds. Must not be negative; a negative value + is rejected by the registered OBO handler at reconcile time + (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, + the cache default applies. + type: string + clientId: + description: |- + ClientID is the confidential client's application (client) ID registered + in Entra. Emitted verbatim as the runtime contract's clientId. + minLength: 1 + type: string + clientSecretRef: + description: |- + ClientSecretRef references a Kubernetes Secret containing the confidential + client's secret. v1 supports a shared client secret only. The operator + injects the resolved value into the proxyrunner pod as an environment + variable and emits only that variable's name in the runtime contract, as + clientSecretEnvVar — the secret value never travels in the contract. + 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 + scopes: + description: |- + Scopes are the delegated scopes to request for the exchanged token, e.g. + ["api:///.default"]. When non-empty they take precedence over + Audience. At least one of audience or scopes must be set. + items: + type: string + type: array + x-kubernetes-list-type: atomic + subjectTokenProviderName: + description: |- + SubjectTokenProviderName selects the source of the OBO subject (assertion) + token from the request's authenticated Identity: + - Omitted: use the inbound end-user token the client presented + (Identity.Token) — the deployment with no embedded auth server, where + the client holds an Entra token directly. + - Set: use the named upstream provider's token + (Identity.UpstreamTokens[]) — the embedded-auth-server + deployment, where the inbound token is the proxy's own session token. + The value must match a configured upstream provider name. + The subject is always sourced from the authenticated Identity, never from + an inbound request header, so the upstream auth middleware must run first. + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ + type: string + tenantId: + description: |- + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Accepts a tenant GUID, a verified domain name (e.g. + contoso.onmicrosoft.com), or one of the well-known values "common", + "organizations", "consumers". The operator interpolates it into the Entra + token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) + emitted as the runtime contract's tokenUrl, so the value is constrained to + a single safe path segment (no slashes, whitespace, query, or fragment). + maxLength: 253 + minLength: 1 + pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + type: string + required: + - clientId + - clientSecretRef + - tenantId type: object + x-kubernetes-validations: + - message: at least one of audience or scopes must be set + rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) + && size(self.scopes) > 0) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index b0752ba5ea..dc09aa8907 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1700,7 +1700,7 @@ _Appears in:_ | `embeddedAuthServer` _[api.v1beta1.EmbeddedAuthServerConfig](#apiv1beta1embeddedauthserverconfig)_ | EmbeddedAuthServer configures an embedded OAuth2/OIDC authorization server
Only used when Type is "embeddedAuthServer" | | Optional: \{\}
| | `awsSts` _[api.v1beta1.AWSStsConfig](#apiv1beta1awsstsconfig)_ | AWSSts configures AWS STS authentication with SigV4 request signing
Only used when Type is "awsSts" | | Optional: \{\}
| | `upstreamInject` _[api.v1beta1.UpstreamInjectSpec](#apiv1beta1upstreaminjectspec)_ | UpstreamInject configures upstream token injection for backend requests.
Only used when Type is "upstreamInject". | | Optional: \{\}
| -| `obo` _[api.v1beta1.OBOConfig](#apiv1beta1oboconfig)_ | OBO configures On-Behalf-Of (OBO) authentication.
Only used when Type is "obo". The inner schema is intentionally empty in
this revision; sub-fields land in a follow-up. Setting this field on an
upstream-only build will cause the MCPExternalAuthConfig to transition to
status.conditions[Valid] = False with Reason: EnterpriseRequired. | | Optional: \{\}
| +| `obo` _[api.v1beta1.OBOConfig](#apiv1beta1oboconfig)_ | OBO configures On-Behalf-Of (OBO) authentication.
Only used when Type is "obo". Setting this field on an upstream-only build
causes the MCPExternalAuthConfig to transition to
status.conditions[Valid] = False with Reason: EnterpriseRequired, because
no OBO handler is registered. See OBOConfig for the field-to-runtime
contract mapping. | | Optional: \{\}
| #### api.v1beta1.MCPExternalAuthConfigStatus @@ -2778,19 +2778,47 @@ _Appears in:_ -OBOConfig is a placeholder for On-Behalf-Of (OBO) external auth configuration. -The inner schema is intentionally empty in this revision; sub-fields land in a -follow-up RFC. The struct exists so OBO *OBOConfig compiles and the CRD -schema admits `spec.obo: {}` — the CEL rule "obo configuration must be set -if and only if type is 'obo'" requires has(self.obo), which evaluates true -for an empty object. Stored objects with `obo: {}` will round-trip cleanly -when sub-fields land, because Go zero values fill in. +OBOConfig holds configuration for the On-Behalf-Of (OBO) external auth type. +Only used when Type is "obo". + +This is the user-facing CRD surface for the Microsoft Entra OBO flow. It is +structurally valid in upstream (OSS) builds but inert: an upstream-only build +returns obo.ErrEnterpriseRequired at reconcile (Valid=False, Reason: +EnterpriseRequired) because no OBO handler is registered via +controllerutil.RegisterOBOHandler. A build with the enterprise OBO handler +translates these fields into the runtime wire contract obo.MiddlewareParameters, +so the field names and semantics here track that contract rather than the +upstream TokenExchangeConfig (which uses different names, e.g. +subjectProviderName / externalTokenHeaderName). In particular there is no +externalTokenHeaderName: the OBO subject is sourced from the authenticated +Identity, never from an inbound request header. + +Field-to-contract mapping performed by the operator (#1581): + - tenantId (+ optional authority) → tokenUrl + (https://login.microsoftonline.com//oauth2/v2.0/token, or the + authority host for sovereign clouds) + - clientSecretRef → resolved into a pod env var; only the env var name + travels in the contract, as clientSecretEnvVar + - audience / scopes → collapsed to a single exchange target by + obo.MiddlewareParameters.ExchangeTarget() (space-joined scopes win, + otherwise audience) + - cacheSkew → the contract's integer-seconds cacheSkewSeconds _Appears in:_ - [api.v1beta1.MCPExternalAuthConfigSpec](#apiv1beta1mcpexternalauthconfigspec) +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier.
Accepts a tenant GUID, a verified domain name (e.g.
contoso.onmicrosoft.com), or one of the well-known values "common",
"organizations", "consumers". The operator interpolates it into the Entra
token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token)
emitted as the runtime contract's tokenUrl, so the value is constrained to
a single safe path segment (no slashes, whitespace, query, or fragment). | | MaxLength: 253
MinLength: 1
Pattern: `^[a-zA-Z0-9][a-zA-Z0-9.-]*$`
Required: \{\}
| +| `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint as //oauth2/v2.0/token.
Must be an HTTPS URL with no path, query, fragment, or trailing slash: the
OBO exchange POSTs the client secret and the end-user assertion to this
host, so it is a trust boundary and HTTPS is required. | | Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| +| `clientId` _string_ | ClientID is the confidential client's application (client) ID registered
in Entra. Emitted verbatim as the runtime contract's clientId. | | MinLength: 1
Required: \{\}
| +| `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the confidential
client's secret. v1 supports a shared client secret only. The operator
injects the resolved value into the proxyrunner pod as an environment
variable and emits only that variable's name in the runtime contract, as
clientSecretEnvVar — the secret value never travels in the contract. | | Required: \{\}
| +| `audience` _string_ | Audience is the backend target identifier requested in the exchanged
token. Used as the exchange target when Scopes is empty. At least one of
audience or scopes must be set. | | Optional: \{\}
| +| `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set. | | Optional: \{\}
| +| `subjectTokenProviderName` _string_ | SubjectTokenProviderName selects the source of the OBO subject (assertion)
token from the request's authenticated Identity:
- Omitted: use the inbound end-user token the client presented
(Identity.Token) — the deployment with no embedded auth server, where
the client holds an Entra token directly.
- Set: use the named upstream provider's token
(Identity.UpstreamTokens[]) — the embedded-auth-server
deployment, where the inbound token is the proxy's own session token.
The value must match a configured upstream provider name.
The subject is always sourced from the authenticated Identity, never from
an inbound request header, so the upstream auth middleware must run first. | | MaxLength: 63
MinLength: 1
Pattern: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`
Optional: \{\}
| +| `cacheSkew` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#duration-v1-meta)_ | CacheSkew overrides the OBO token cache's default expiry skew (the margin
by which a cached token is treated as expired before its real expiry),
e.g. "30s". The operator converts it to the runtime contract's
integer-seconds cacheSkewSeconds. Must not be negative; a negative value
is rejected by the registered OBO handler at reconcile time
(Valid=False, Reason: InvalidConfig in enterprise builds). When omitted,
the cache default applies. | | Optional: \{\}
| #### api.v1beta1.OIDCUpstreamConfig @@ -3139,6 +3167,7 @@ _Appears in:_ - [api.v1beta1.HeaderInjectionConfig](#apiv1beta1headerinjectionconfig) - [api.v1beta1.InlineOIDCSharedConfig](#apiv1beta1inlineoidcsharedconfig) - [api.v1beta1.OAuth2UpstreamConfig](#apiv1beta1oauth2upstreamconfig) +- [api.v1beta1.OBOConfig](#apiv1beta1oboconfig) - [api.v1beta1.OIDCUpstreamConfig](#apiv1beta1oidcupstreamconfig) - [api.v1beta1.RedisACLUserConfig](#apiv1beta1redisacluserconfig) - [api.v1beta1.RedisTLSConfig](#apiv1beta1redistlsconfig) From e13ce3b253ae7e2217739cf8e15239967fae83e8 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 10 Jun 2026 15:22:50 -0700 Subject: [PATCH 2/6] Align OBOConfig validation with downstream consumer Address code review and a field-by-field check against the downstream enterprise OBO consumers (the obo.MiddlewareParameters contract and the entra exchanger/cache/runtime that consume it): - Tighten tenantId to a GUID-or-domain pattern mirroring the exchanger's validateTenant, so a tenantId admitted by the CRD is one the runtime can consume. The previous loose pattern admitted aliases like "common" that the exchanger rejects, creating an admission/reconcile gap. - Correct the authority field: the exchanger deliberately allows a path (sovereign / B2C / CIAM endpoints use different token paths), so keep the path-permitting pattern and fix the doc comment that wrongly claimed "no path". - Require a non-blank audience or scope at admission (CEL trim()/exists), mirroring ExchangeTarget()'s trimming so a whitespace-only value is rejected up front rather than only at reconcile. Bound scopes (MaxItems + per-item length) to keep the CEL rule within the apiserver cost budget. - Make clientId and clientSecretRef optional at the CRD level, enforced by the operator per auth mode, so certificate / workload-identity client auth can be added later without a breaking schema change. Regenerated CRD manifests and API docs; extended the envtest CEL suite (16 specs) to cover the tightened rules through a real apiserver. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 82 ++++++++----- .../mcp-external-auth/obo_validation_test.go | 69 ++++++++--- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 110 ++++++++++++------ ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 110 ++++++++++++------ docs/operator/crd-api.md | 20 +++- 5 files changed, 264 insertions(+), 127 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 4a85a12731..0ed911a593 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -133,7 +133,7 @@ type MCPExternalAuthConfigSpec struct { // Field-to-contract mapping performed by the operator (#1581): // - tenantId (+ optional authority) → tokenUrl // (https://login.microsoftonline.com//oauth2/v2.0/token, or the -// authority host for sovereign clouds) +// configured authority base joined with the tenant for sovereign clouds) // - clientSecretRef → resolved into a pod env var; only the env var name // travels in the contract, as clientSecretEnvVar // - audience / scopes → collapsed to a single exchange target by @@ -141,48 +141,69 @@ type MCPExternalAuthConfigSpec struct { // otherwise audience) // - cacheSkew → the contract's integer-seconds cacheSkewSeconds // -// +kubebuilder:validation:XValidation:rule="(has(self.audience) && size(self.audience) > 0) || (has(self.scopes) && size(self.scopes) > 0)",message="at least one of audience or scopes must be set" +// The XValidation rule mirrors the emptiness semantics of +// obo.MiddlewareParameters.ExchangeTarget() (which TrimSpaces both fields): +// it requires a non-blank audience or at least one non-blank scope, so a +// present-but-whitespace value is rejected at admission rather than collapsing +// to an empty exchange target only at reconcile. It checks presence, not the +// scopes-win-over-audience preference — that collapse stays the single +// responsibility of ExchangeTarget(). +// +// +kubebuilder:validation:XValidation:rule="(has(self.audience) && self.audience.trim().size() > 0) || (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0))",message="at least one of audience or scopes must be set to a non-blank value" // //nolint:lll // CEL validation rules exceed line length limit type OBOConfig struct { - // TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. - // Accepts a tenant GUID, a verified domain name (e.g. - // contoso.onmicrosoft.com), or one of the well-known values "common", - // "organizations", "consumers". The operator interpolates it into the Entra - // token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) - // emitted as the runtime contract's tokenUrl, so the value is constrained to - // a single safe path segment (no slashes, whitespace, query, or fragment). + // TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, + // in one of the two forms the Entra v2.0 token endpoint addresses: a + // directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). + // Well-known aliases such as "common", "organizations", and "consumers" are + // NOT accepted — an OBO confidential-client exchange must target a specific + // tenant. The operator interpolates it into the token endpoint + // (//oauth2/v2.0/token), so the value is constrained to + // the GUID/domain shape (no path metacharacters). The pattern and 253-char + // cap mirror the enterprise exchanger's validateTenant, so any tenantId + // admitted here is one the runtime can consume. // +kubebuilder:validation:Required // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 - // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9.-]*$` + // +kubebuilder:validation:Pattern=`^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$` TenantID string `json:"tenantId"` // Authority overrides the default Entra login host // (https://login.microsoftonline.com) for sovereign or national clouds, e.g. // https://login.microsoftonline.us (US Gov) or // https://login.partner.microsoftonline.cn (China). When set, the operator - // builds the token endpoint as //oauth2/v2.0/token. - // Must be an HTTPS URL with no path, query, fragment, or trailing slash: the - // OBO exchange POSTs the client secret and the end-user assertion to this - // host, so it is a trust boundary and HTTPS is required. + // builds the token endpoint by joining , , and the + // v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing + // slash; a path IS permitted and is prefixed before the tenant segment, as + // some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the + // client secret and the end-user assertion to this host, so it is a trust + // boundary and HTTPS is required. Mirrors the enterprise exchanger's + // authority validation (no query/fragment), which deliberately allows + // arbitrary hosts and paths for non-public clouds. // +kubebuilder:validation:Pattern=`^https://[^\s?#]+[^/\s?#]$` // +optional Authority string `json:"authority,omitempty"` // ClientID is the confidential client's application (client) ID registered // in Entra. Emitted verbatim as the runtime contract's clientId. - // +kubebuilder:validation:Required - // +kubebuilder:validation:MinLength=1 - ClientID string `json:"clientId"` + // Optional at the CRD level so future client-authentication methods (e.g. + // certificate or workload-identity credentials, planned fast-follows) can be + // added without a breaking schema change. The operator enforces that clientId + // and clientSecretRef are both present for the v1 shared-secret flow. + // +optional + ClientID string `json:"clientId,omitempty"` // ClientSecretRef references a Kubernetes Secret containing the confidential // client's secret. v1 supports a shared client secret only. The operator // injects the resolved value into the proxyrunner pod as an environment // variable and emits only that variable's name in the runtime contract, as // clientSecretEnvVar — the secret value never travels in the contract. - // +kubebuilder:validation:Required - ClientSecretRef *SecretKeyRef `json:"clientSecretRef"` + // Optional at the CRD level for the same forward-compatibility reason as + // clientId (a certificate/workload-identity flow needs no client secret); + // the operator enforces presence for the v1 shared-secret flow. + // +optional + ClientSecretRef *SecretKeyRef `json:"clientSecretRef,omitempty"` // Audience is the backend target identifier requested in the exchanged // token. Used as the exchange target when Scopes is empty. At least one of @@ -192,7 +213,12 @@ type OBOConfig struct { // Scopes are the delegated scopes to request for the exchanged token, e.g. // ["api:///.default"]. When non-empty they take precedence over - // Audience. At least one of audience or scopes must be set. + // Audience. At least one of audience or scopes must be set. The MaxItems and + // per-item length caps are defensive bounds that also keep the + // audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + // +kubebuilder:validation:MaxItems=20 + // +kubebuilder:validation:items:MinLength=1 + // +kubebuilder:validation:items:MaxLength=256 // +listType=atomic // +optional Scopes []string `json:"scopes,omitempty"` @@ -1315,14 +1341,14 @@ func (r *MCPExternalAuthConfig) Validate() error { // of this method, so this arm is reached only when the structural // invariant holds — and the matching CEL rule on the spec catches // it at admission time. Field-level validation of OBOConfig - // (required tenantId/clientId/clientSecretRef, at-least-one-of - // audience/scopes, authority/tenantId shape, duration format of - // cacheSkew) is enforced by the kubebuilder markers and the OBOConfig - // CEL rule at admission, not here: OBOConfig is a brand-new field set - // with no pre-CEL stored objects to backfill, and the - // semantic/protocol validation that would justify a reconcile-time - // backstop (including rejecting a negative cacheSkew) is owned by the - // registered handler, not the upstream type. That handler runs at reconcile + // (required tenantId, at-least-one-of audience/scopes, authority/ + // tenantId shape, duration format of cacheSkew) is enforced by the + // kubebuilder markers and the OBOConfig CEL rule at admission, not here: + // OBOConfig is a brand-new field set with no pre-CEL stored objects to + // backfill, and the semantic/protocol validation that would justify a + // reconcile-time backstop (clientId/clientSecretRef presence for the + // chosen client-auth mode, rejecting a negative cacheSkew) is owned by + // the registered handler, not the upstream type. That handler runs at reconcile // time via the controllerutil.OBOValidate function-pointer hook: // upstream-only builds return obo.ErrEnterpriseRequired, which the // reconciler maps to status.conditions[Valid] = False / Reason: diff --git a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go index 5bdf86be29..e8f5633d5e 100644 --- a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go +++ b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go @@ -90,26 +90,16 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", Expect(err.Error()).To(ContainSubstring("tenantId")) }) - It("should reject a missing clientId", func() { - cfg := makeOBOConfig("obo-no-client", &mcpv1beta1.OBOConfig{ - TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", - ClientSecretRef: secretRef, - Audience: "api://backend", - }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("clientId")) - }) - - It("should reject a missing clientSecretRef", func() { - cfg := makeOBOConfig("obo-no-secret", &mcpv1beta1.OBOConfig{ + It("should accept a config without clientId or clientSecretRef (operator enforces per auth mode)", func() { + // clientId/clientSecretRef are optional at the CRD level so future + // client-auth methods (certificate, workload identity) need no + // breaking schema change; the operator enforces the v1 shared-secret + // combination. Admission must therefore accept their absence. + cfg := makeOBOConfig("obo-no-client-auth", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", - ClientID: "app-client-id", Audience: "api://backend", }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("clientSecretRef")) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) }) @@ -136,6 +126,30 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("at least one of audience or scopes must be set")) }) + + It("should reject a whitespace-only audience (mirrors ExchangeTarget trimming)", func() { + cfg := makeOBOConfig("obo-blank-audience", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: " ", + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("non-blank value")) + }) + + It("should reject scopes containing only blank entries", func() { + cfg := makeOBOConfig("obo-blank-scopes", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Scopes: []string{" "}, + }) + err := k8sClient.Create(ctx, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("non-blank value")) + }) }) Context("field patterns", func() { @@ -171,6 +185,27 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) + It("should reject a tenantId well-known alias like 'common' (OBO needs a specific tenant)", func() { + cfg := makeOBOConfig("obo-tenant-common", &mcpv1beta1.OBOConfig{ + TenantID: "common", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + }) + + It("should accept an authority with a path (B2C/CIAM/sovereign clouds use token paths)", func() { + cfg := makeOBOConfig("obo-authority-path", &mcpv1beta1.OBOConfig{ + TenantID: "contoso.onmicrosoft.com", + Authority: "https://contoso.ciamlogin.com/contoso.onmicrosoft.com", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) + }) + It("should reject an uppercase subjectTokenProviderName", func() { cfg := makeOBOConfig("obo-bad-subject", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", 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 516461287d..0f9dc9c20c 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 @@ -1097,10 +1097,14 @@ spec: (https://login.microsoftonline.com) for sovereign or national clouds, e.g. https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator - builds the token endpoint as //oauth2/v2.0/token. - Must be an HTTPS URL with no path, query, fragment, or trailing slash: the - OBO exchange POSTs the client secret and the end-user assertion to this - host, so it is a trust boundary and HTTPS is required. + builds the token endpoint by joining , , and the + v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing + slash; a path IS permitted and is prefixed before the tenant segment, as + some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the + client secret and the end-user assertion to this host, so it is a trust + boundary and HTTPS is required. Mirrors the enterprise exchanger's + authority validation (no query/fragment), which deliberately allows + arbitrary hosts and paths for non-public clouds. pattern: ^https://[^\s?#]+[^/\s?#]$ type: string cacheSkew: @@ -1117,7 +1121,10 @@ spec: description: |- ClientID is the confidential client's application (client) ID registered in Entra. Emitted verbatim as the runtime contract's clientId. - minLength: 1 + Optional at the CRD level so future client-authentication methods (e.g. + certificate or workload-identity credentials, planned fast-follows) can be + added without a breaking schema change. The operator enforces that clientId + and clientSecretRef are both present for the v1 shared-secret flow. type: string clientSecretRef: description: |- @@ -1126,6 +1133,9 @@ spec: injects the resolved value into the proxyrunner pod as an environment variable and emits only that variable's name in the runtime contract, as clientSecretEnvVar — the secret value never travels in the contract. + Optional at the CRD level for the same forward-compatibility reason as + clientId (a certificate/workload-identity flow needs no client secret); + the operator enforces presence for the v1 shared-secret flow. properties: key: description: Key is the key within the secret @@ -1141,9 +1151,14 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. + Audience. At least one of audience or scopes must be set. The MaxItems and + per-item length caps are defensive bounds that also keep the + audience-or-scopes CEL rule within the apiserver's per-rule cost budget. items: + maxLength: 256 + minLength: 1 type: string + maxItems: 20 type: array x-kubernetes-list-type: atomic subjectTokenProviderName: @@ -1165,26 +1180,28 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. - Accepts a tenant GUID, a verified domain name (e.g. - contoso.onmicrosoft.com), or one of the well-known values "common", - "organizations", "consumers". The operator interpolates it into the Entra - token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) - emitted as the runtime contract's tokenUrl, so the value is constrained to - a single safe path segment (no slashes, whitespace, query, or fragment). + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, + in one of the two forms the Entra v2.0 token endpoint addresses: a + directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). + Well-known aliases such as "common", "organizations", and "consumers" are + NOT accepted — an OBO confidential-client exchange must target a specific + tenant. The operator interpolates it into the token endpoint + (//oauth2/v2.0/token), so the value is constrained to + the GUID/domain shape (no path metacharacters). The pattern and 253-char + cap mirror the enterprise exchanger's validateTenant, so any tenantId + admitted here is one the runtime can consume. maxLength: 253 minLength: 1 - pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string required: - - clientId - - clientSecretRef - tenantId type: object x-kubernetes-validations: - - message: at least one of audience or scopes must be set - rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) - && size(self.scopes) > 0) + - message: at least one of audience or scopes must be set to a non-blank + value + rule: (has(self.audience) && self.audience.trim().size() > 0) || + (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2507,10 +2524,14 @@ spec: (https://login.microsoftonline.com) for sovereign or national clouds, e.g. https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator - builds the token endpoint as //oauth2/v2.0/token. - Must be an HTTPS URL with no path, query, fragment, or trailing slash: the - OBO exchange POSTs the client secret and the end-user assertion to this - host, so it is a trust boundary and HTTPS is required. + builds the token endpoint by joining , , and the + v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing + slash; a path IS permitted and is prefixed before the tenant segment, as + some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the + client secret and the end-user assertion to this host, so it is a trust + boundary and HTTPS is required. Mirrors the enterprise exchanger's + authority validation (no query/fragment), which deliberately allows + arbitrary hosts and paths for non-public clouds. pattern: ^https://[^\s?#]+[^/\s?#]$ type: string cacheSkew: @@ -2527,7 +2548,10 @@ spec: description: |- ClientID is the confidential client's application (client) ID registered in Entra. Emitted verbatim as the runtime contract's clientId. - minLength: 1 + Optional at the CRD level so future client-authentication methods (e.g. + certificate or workload-identity credentials, planned fast-follows) can be + added without a breaking schema change. The operator enforces that clientId + and clientSecretRef are both present for the v1 shared-secret flow. type: string clientSecretRef: description: |- @@ -2536,6 +2560,9 @@ spec: injects the resolved value into the proxyrunner pod as an environment variable and emits only that variable's name in the runtime contract, as clientSecretEnvVar — the secret value never travels in the contract. + Optional at the CRD level for the same forward-compatibility reason as + clientId (a certificate/workload-identity flow needs no client secret); + the operator enforces presence for the v1 shared-secret flow. properties: key: description: Key is the key within the secret @@ -2551,9 +2578,14 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. + Audience. At least one of audience or scopes must be set. The MaxItems and + per-item length caps are defensive bounds that also keep the + audience-or-scopes CEL rule within the apiserver's per-rule cost budget. items: + maxLength: 256 + minLength: 1 type: string + maxItems: 20 type: array x-kubernetes-list-type: atomic subjectTokenProviderName: @@ -2575,26 +2607,28 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. - Accepts a tenant GUID, a verified domain name (e.g. - contoso.onmicrosoft.com), or one of the well-known values "common", - "organizations", "consumers". The operator interpolates it into the Entra - token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) - emitted as the runtime contract's tokenUrl, so the value is constrained to - a single safe path segment (no slashes, whitespace, query, or fragment). + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, + in one of the two forms the Entra v2.0 token endpoint addresses: a + directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). + Well-known aliases such as "common", "organizations", and "consumers" are + NOT accepted — an OBO confidential-client exchange must target a specific + tenant. The operator interpolates it into the token endpoint + (//oauth2/v2.0/token), so the value is constrained to + the GUID/domain shape (no path metacharacters). The pattern and 253-char + cap mirror the enterprise exchanger's validateTenant, so any tenantId + admitted here is one the runtime can consume. maxLength: 253 minLength: 1 - pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string required: - - clientId - - clientSecretRef - tenantId type: object x-kubernetes-validations: - - message: at least one of audience or scopes must be set - rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) - && size(self.scopes) > 0) + - message: at least one of audience or scopes must be set to a non-blank + value + rule: (has(self.audience) && self.audience.trim().size() > 0) || + (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange 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 e1bf297030..d2db02c949 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1100,10 +1100,14 @@ spec: (https://login.microsoftonline.com) for sovereign or national clouds, e.g. https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator - builds the token endpoint as //oauth2/v2.0/token. - Must be an HTTPS URL with no path, query, fragment, or trailing slash: the - OBO exchange POSTs the client secret and the end-user assertion to this - host, so it is a trust boundary and HTTPS is required. + builds the token endpoint by joining , , and the + v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing + slash; a path IS permitted and is prefixed before the tenant segment, as + some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the + client secret and the end-user assertion to this host, so it is a trust + boundary and HTTPS is required. Mirrors the enterprise exchanger's + authority validation (no query/fragment), which deliberately allows + arbitrary hosts and paths for non-public clouds. pattern: ^https://[^\s?#]+[^/\s?#]$ type: string cacheSkew: @@ -1120,7 +1124,10 @@ spec: description: |- ClientID is the confidential client's application (client) ID registered in Entra. Emitted verbatim as the runtime contract's clientId. - minLength: 1 + Optional at the CRD level so future client-authentication methods (e.g. + certificate or workload-identity credentials, planned fast-follows) can be + added without a breaking schema change. The operator enforces that clientId + and clientSecretRef are both present for the v1 shared-secret flow. type: string clientSecretRef: description: |- @@ -1129,6 +1136,9 @@ spec: injects the resolved value into the proxyrunner pod as an environment variable and emits only that variable's name in the runtime contract, as clientSecretEnvVar — the secret value never travels in the contract. + Optional at the CRD level for the same forward-compatibility reason as + clientId (a certificate/workload-identity flow needs no client secret); + the operator enforces presence for the v1 shared-secret flow. properties: key: description: Key is the key within the secret @@ -1144,9 +1154,14 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. + Audience. At least one of audience or scopes must be set. The MaxItems and + per-item length caps are defensive bounds that also keep the + audience-or-scopes CEL rule within the apiserver's per-rule cost budget. items: + maxLength: 256 + minLength: 1 type: string + maxItems: 20 type: array x-kubernetes-list-type: atomic subjectTokenProviderName: @@ -1168,26 +1183,28 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. - Accepts a tenant GUID, a verified domain name (e.g. - contoso.onmicrosoft.com), or one of the well-known values "common", - "organizations", "consumers". The operator interpolates it into the Entra - token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) - emitted as the runtime contract's tokenUrl, so the value is constrained to - a single safe path segment (no slashes, whitespace, query, or fragment). + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, + in one of the two forms the Entra v2.0 token endpoint addresses: a + directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). + Well-known aliases such as "common", "organizations", and "consumers" are + NOT accepted — an OBO confidential-client exchange must target a specific + tenant. The operator interpolates it into the token endpoint + (//oauth2/v2.0/token), so the value is constrained to + the GUID/domain shape (no path metacharacters). The pattern and 253-char + cap mirror the enterprise exchanger's validateTenant, so any tenantId + admitted here is one the runtime can consume. maxLength: 253 minLength: 1 - pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string required: - - clientId - - clientSecretRef - tenantId type: object x-kubernetes-validations: - - message: at least one of audience or scopes must be set - rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) - && size(self.scopes) > 0) + - message: at least one of audience or scopes must be set to a non-blank + value + rule: (has(self.audience) && self.audience.trim().size() > 0) || + (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2510,10 +2527,14 @@ spec: (https://login.microsoftonline.com) for sovereign or national clouds, e.g. https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator - builds the token endpoint as //oauth2/v2.0/token. - Must be an HTTPS URL with no path, query, fragment, or trailing slash: the - OBO exchange POSTs the client secret and the end-user assertion to this - host, so it is a trust boundary and HTTPS is required. + builds the token endpoint by joining , , and the + v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing + slash; a path IS permitted and is prefixed before the tenant segment, as + some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the + client secret and the end-user assertion to this host, so it is a trust + boundary and HTTPS is required. Mirrors the enterprise exchanger's + authority validation (no query/fragment), which deliberately allows + arbitrary hosts and paths for non-public clouds. pattern: ^https://[^\s?#]+[^/\s?#]$ type: string cacheSkew: @@ -2530,7 +2551,10 @@ spec: description: |- ClientID is the confidential client's application (client) ID registered in Entra. Emitted verbatim as the runtime contract's clientId. - minLength: 1 + Optional at the CRD level so future client-authentication methods (e.g. + certificate or workload-identity credentials, planned fast-follows) can be + added without a breaking schema change. The operator enforces that clientId + and clientSecretRef are both present for the v1 shared-secret flow. type: string clientSecretRef: description: |- @@ -2539,6 +2563,9 @@ spec: injects the resolved value into the proxyrunner pod as an environment variable and emits only that variable's name in the runtime contract, as clientSecretEnvVar — the secret value never travels in the contract. + Optional at the CRD level for the same forward-compatibility reason as + clientId (a certificate/workload-identity flow needs no client secret); + the operator enforces presence for the v1 shared-secret flow. properties: key: description: Key is the key within the secret @@ -2554,9 +2581,14 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. + Audience. At least one of audience or scopes must be set. The MaxItems and + per-item length caps are defensive bounds that also keep the + audience-or-scopes CEL rule within the apiserver's per-rule cost budget. items: + maxLength: 256 + minLength: 1 type: string + maxItems: 20 type: array x-kubernetes-list-type: atomic subjectTokenProviderName: @@ -2578,26 +2610,28 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. - Accepts a tenant GUID, a verified domain name (e.g. - contoso.onmicrosoft.com), or one of the well-known values "common", - "organizations", "consumers". The operator interpolates it into the Entra - token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token) - emitted as the runtime contract's tokenUrl, so the value is constrained to - a single safe path segment (no slashes, whitespace, query, or fragment). + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, + in one of the two forms the Entra v2.0 token endpoint addresses: a + directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). + Well-known aliases such as "common", "organizations", and "consumers" are + NOT accepted — an OBO confidential-client exchange must target a specific + tenant. The operator interpolates it into the token endpoint + (//oauth2/v2.0/token), so the value is constrained to + the GUID/domain shape (no path metacharacters). The pattern and 253-char + cap mirror the enterprise exchanger's validateTenant, so any tenantId + admitted here is one the runtime can consume. maxLength: 253 minLength: 1 - pattern: ^[a-zA-Z0-9][a-zA-Z0-9.-]*$ + pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string required: - - clientId - - clientSecretRef - tenantId type: object x-kubernetes-validations: - - message: at least one of audience or scopes must be set - rule: (has(self.audience) && size(self.audience) > 0) || (has(self.scopes) - && size(self.scopes) > 0) + - message: at least one of audience or scopes must be set to a non-blank + value + rule: (has(self.audience) && self.audience.trim().size() > 0) || + (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index dc09aa8907..475a95bc83 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2796,7 +2796,7 @@ Identity, never from an inbound request header. Field-to-contract mapping performed by the operator (#1581): - tenantId (+ optional authority) → tokenUrl (https://login.microsoftonline.com//oauth2/v2.0/token, or the - authority host for sovereign clouds) + configured authority base joined with the tenant for sovereign clouds) - clientSecretRef → resolved into a pod env var; only the env var name travels in the contract, as clientSecretEnvVar - audience / scopes → collapsed to a single exchange target by @@ -2804,6 +2804,14 @@ Field-to-contract mapping performed by the operator (#1581): otherwise audience) - cacheSkew → the contract's integer-seconds cacheSkewSeconds +The XValidation rule mirrors the emptiness semantics of +obo.MiddlewareParameters.ExchangeTarget() (which TrimSpaces both fields): +it requires a non-blank audience or at least one non-blank scope, so a +present-but-whitespace value is rejected at admission rather than collapsing +to an empty exchange target only at reconcile. It checks presence, not the +scopes-win-over-audience preference — that collapse stays the single +responsibility of ExchangeTarget(). + _Appears in:_ @@ -2811,12 +2819,12 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier.
Accepts a tenant GUID, a verified domain name (e.g.
contoso.onmicrosoft.com), or one of the well-known values "common",
"organizations", "consumers". The operator interpolates it into the Entra
token endpoint (https://login.microsoftonline.com//oauth2/v2.0/token)
emitted as the runtime contract's tokenUrl, so the value is constrained to
a single safe path segment (no slashes, whitespace, query, or fragment). | | MaxLength: 253
MinLength: 1
Pattern: `^[a-zA-Z0-9][a-zA-Z0-9.-]*$`
Required: \{\}
| -| `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint as //oauth2/v2.0/token.
Must be an HTTPS URL with no path, query, fragment, or trailing slash: the
OBO exchange POSTs the client secret and the end-user assertion to this
host, so it is a trust boundary and HTTPS is required. | | Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| -| `clientId` _string_ | ClientID is the confidential client's application (client) ID registered
in Entra. Emitted verbatim as the runtime contract's clientId. | | MinLength: 1
Required: \{\}
| -| `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the confidential
client's secret. v1 supports a shared client secret only. The operator
injects the resolved value into the proxyrunner pod as an environment
variable and emits only that variable's name in the runtime contract, as
clientSecretEnvVar — the secret value never travels in the contract. | | Required: \{\}
| +| `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier,
in one of the two forms the Entra v2.0 token endpoint addresses: a
directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com).
Well-known aliases such as "common", "organizations", and "consumers" are
NOT accepted — an OBO confidential-client exchange must target a specific
tenant. The operator interpolates it into the token endpoint
(//oauth2/v2.0/token), so the value is constrained to
the GUID/domain shape (no path metacharacters). The pattern and 253-char
cap mirror the enterprise exchanger's validateTenant, so any tenantId
admitted here is one the runtime can consume. | | MaxLength: 253
MinLength: 1
Pattern: `^([0-9a-fA-F]\{8\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{12\}\|([a-zA-Z0-9]([a-zA-Z0-9-]\{0,61\}[a-zA-Z0-9])?\.)+[a-zA-Z]\{2,\})$`
Required: \{\}
| +| `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint by joining , , and the
v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing
slash; a path IS permitted and is prefixed before the tenant segment, as
some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the
client secret and the end-user assertion to this host, so it is a trust
boundary and HTTPS is required. Mirrors the enterprise exchanger's
authority validation (no query/fragment), which deliberately allows
arbitrary hosts and paths for non-public clouds. | | Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| +| `clientId` _string_ | ClientID is the confidential client's application (client) ID registered
in Entra. Emitted verbatim as the runtime contract's clientId.
Optional at the CRD level so future client-authentication methods (e.g.
certificate or workload-identity credentials, planned fast-follows) can be
added without a breaking schema change. The operator enforces that clientId
and clientSecretRef are both present for the v1 shared-secret flow. | | Optional: \{\}
| +| `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the confidential
client's secret. v1 supports a shared client secret only. The operator
injects the resolved value into the proxyrunner pod as an environment
variable and emits only that variable's name in the runtime contract, as
clientSecretEnvVar — the secret value never travels in the contract.
Optional at the CRD level for the same forward-compatibility reason as
clientId (a certificate/workload-identity flow needs no client secret);
the operator enforces presence for the v1 shared-secret flow. | | Optional: \{\}
| | `audience` _string_ | Audience is the backend target identifier requested in the exchanged
token. Used as the exchange target when Scopes is empty. At least one of
audience or scopes must be set. | | Optional: \{\}
| -| `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set. | | Optional: \{\}
| +| `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set. The MaxItems and
per-item length caps are defensive bounds that also keep the
audience-or-scopes CEL rule within the apiserver's per-rule cost budget. | | MaxItems: 20
items:MaxLength: 256
items:MinLength: 1
Optional: \{\}
| | `subjectTokenProviderName` _string_ | SubjectTokenProviderName selects the source of the OBO subject (assertion)
token from the request's authenticated Identity:
- Omitted: use the inbound end-user token the client presented
(Identity.Token) — the deployment with no embedded auth server, where
the client holds an Entra token directly.
- Set: use the named upstream provider's token
(Identity.UpstreamTokens[]) — the embedded-auth-server
deployment, where the inbound token is the proxy's own session token.
The value must match a configured upstream provider name.
The subject is always sourced from the authenticated Identity, never from
an inbound request header, so the upstream auth middleware must run first. | | MaxLength: 63
MinLength: 1
Pattern: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`
Optional: \{\}
| | `cacheSkew` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#duration-v1-meta)_ | CacheSkew overrides the OBO token cache's default expiry skew (the margin
by which a cached token is treated as expired before its real expiry),
e.g. "30s". The operator converts it to the runtime contract's
integer-seconds cacheSkewSeconds. Must not be negative; a negative value
is rejected by the registered OBO handler at reconcile time
(Valid=False, Reason: InvalidConfig in enterprise builds). When omitted,
the cache default applies. | | Optional: \{\}
| From afc8d8a40a3561f9a09a2db5d840ae03e1ac9161 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 10 Jun 2026 15:59:52 -0700 Subject: [PATCH 3/6] Drop issue-tracker reference from OBOConfig doc comment The OBOConfig field-mapping doc comment carried a bare "#1581" issue reference that does not resolve in this repository and is noise in the generated CRD descriptions. Describe the operator's OBO handler in prose instead. Regenerated CRD manifests and API docs. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go | 2 +- docs/operator/crd-api.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 0ed911a593..536f0faa53 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -130,7 +130,7 @@ type MCPExternalAuthConfigSpec struct { // externalTokenHeaderName: the OBO subject is sourced from the authenticated // Identity, never from an inbound request header. // -// Field-to-contract mapping performed by the operator (#1581): +// Field-to-contract mapping performed by the operator's OBO handler: // - tenantId (+ optional authority) → tokenUrl // (https://login.microsoftonline.com//oauth2/v2.0/token, or the // configured authority base joined with the tenant for sovereign clouds) diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 475a95bc83..62382b5ebf 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2793,7 +2793,7 @@ subjectProviderName / externalTokenHeaderName). In particular there is no externalTokenHeaderName: the OBO subject is sourced from the authenticated Identity, never from an inbound request header. -Field-to-contract mapping performed by the operator (#1581): +Field-to-contract mapping performed by the operator's OBO handler: - tenantId (+ optional authority) → tokenUrl (https://login.microsoftonline.com//oauth2/v2.0/token, or the configured authority base joined with the tenant for sovereign clouds) From 1e47a820058e06e00bbe5aaa876821752e4069b1 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 11 Jun 2026 08:08:00 -0700 Subject: [PATCH 4/6] Keep OBOConfig schema backward compatible The CRD schema-compatibility check failed: spec.obo shipped as an empty placeholder ({}) in v0.29.3, whose schema admitted any stored object with obo: {}. Marking the new tenantId field required (NoNewRequiredFields) and adding a CEL rule that rejects {} both narrow that released schema and would invalidate already-stored objects. Make every OBOConfig field optional and drop the audience-or-scopes CEL rule so the schema keeps admitting obo: {} and any subset of fields. Presence and combination requirements (a tenant, a client-auth credential, at least one of audience or scopes) are enforced by the registered OBO handler at reconcile, reported as Valid=False / Reason=InvalidConfig. Per-field patterns and bounds remain and validate only values that are present. Regenerated CRD manifests and API docs. Reworked the envtest suite to assert the empty placeholder and partial configs are admitted while the per-field patterns still reject malformed values. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 69 +++++++-------- .../mcp-external-auth/obo_validation_test.go | 84 +++++-------------- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 66 +++++++-------- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 66 +++++++-------- docs/operator/crd-api.md | 22 ++--- 5 files changed, 127 insertions(+), 180 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 536f0faa53..f69f678db8 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -141,33 +141,34 @@ type MCPExternalAuthConfigSpec struct { // otherwise audience) // - cacheSkew → the contract's integer-seconds cacheSkewSeconds // -// The XValidation rule mirrors the emptiness semantics of -// obo.MiddlewareParameters.ExchangeTarget() (which TrimSpaces both fields): -// it requires a non-blank audience or at least one non-blank scope, so a -// present-but-whitespace value is rejected at admission rather than collapsing -// to an empty exchange target only at reconcile. It checks presence, not the -// scopes-win-over-audience preference — that collapse stays the single -// responsibility of ExchangeTarget(). +// Every field is optional at the CRD level, and the schema deliberately carries +// no required field and no cross-field CEL rule. spec.obo shipped as an empty +// placeholder ({}) in earlier releases, so adding a required field or an +// admission rule that rejects {} would be a backward-incompatible narrowing of +// an already-stored, round-trippable object. Presence and combination +// requirements — a tenant, a client-auth credential, and at least one of +// audience/scopes — are therefore enforced by the registered OBO handler at +// reconcile, which reports a violation as Valid=False / Reason=InvalidConfig. +// The per-field patterns below still apply, but only to a value that is present. // -// +kubebuilder:validation:XValidation:rule="(has(self.audience) && self.audience.trim().size() > 0) || (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0))",message="at least one of audience or scopes must be set to a non-blank value" -// -//nolint:lll // CEL validation rules exceed line length limit +//nolint:lll // the tenantId GUID/domain pattern exceeds the line length limit type OBOConfig struct { - // TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, - // in one of the two forms the Entra v2.0 token endpoint addresses: a - // directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). - // Well-known aliases such as "common", "organizations", and "consumers" are - // NOT accepted — an OBO confidential-client exchange must target a specific - // tenant. The operator interpolates it into the token endpoint + // TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + // Optional at the CRD level (see the type doc); the operator enforces its + // presence, since an OBO confidential-client exchange must target a specific + // tenant. When set, it must be one of the two forms the Entra v2.0 token + // endpoint addresses: a directory GUID, or a verified domain name (e.g. + // contoso.onmicrosoft.com). Well-known aliases such as "common", + // "organizations", and "consumers" are NOT accepted. The operator + // interpolates it into the token endpoint // (//oauth2/v2.0/token), so the value is constrained to - // the GUID/domain shape (no path metacharacters). The pattern and 253-char + // the GUID/domain shape (no path metacharacters); the pattern and 253-char // cap mirror the enterprise exchanger's validateTenant, so any tenantId // admitted here is one the runtime can consume. - // +kubebuilder:validation:Required - // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 // +kubebuilder:validation:Pattern=`^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$` - TenantID string `json:"tenantId"` + // +optional + TenantID string `json:"tenantId,omitempty"` // Authority overrides the default Entra login host // (https://login.microsoftonline.com) for sovereign or national clouds, e.g. @@ -207,15 +208,16 @@ type OBOConfig struct { // Audience is the backend target identifier requested in the exchanged // token. Used as the exchange target when Scopes is empty. At least one of - // audience or scopes must be set. + // audience or scopes must be set; the operator enforces that at reconcile + // (it is not an admission-time rule — see the type doc). // +optional Audience string `json:"audience,omitempty"` // Scopes are the delegated scopes to request for the exchanged token, e.g. // ["api:///.default"]. When non-empty they take precedence over - // Audience. At least one of audience or scopes must be set. The MaxItems and - // per-item length caps are defensive bounds that also keep the - // audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + // Audience. At least one of audience or scopes must be set; the operator + // enforces that at reconcile. The MaxItems and per-item length caps are + // defensive bounds on an otherwise unbounded list. // +kubebuilder:validation:MaxItems=20 // +kubebuilder:validation:items:MinLength=1 // +kubebuilder:validation:items:MaxLength=256 @@ -1340,15 +1342,16 @@ func (r *MCPExternalAuthConfig) Validate() error { // has already run via r.validateTypeConfigConsistency() at the top // of this method, so this arm is reached only when the structural // invariant holds — and the matching CEL rule on the spec catches - // it at admission time. Field-level validation of OBOConfig - // (required tenantId, at-least-one-of audience/scopes, authority/ - // tenantId shape, duration format of cacheSkew) is enforced by the - // kubebuilder markers and the OBOConfig CEL rule at admission, not here: - // OBOConfig is a brand-new field set with no pre-CEL stored objects to - // backfill, and the semantic/protocol validation that would justify a - // reconcile-time backstop (clientId/clientSecretRef presence for the - // chosen client-auth mode, rejecting a negative cacheSkew) is owned by - // the registered handler, not the upstream type. That handler runs at reconcile + // it at admission time. OBOConfig carries no required field and no + // cross-field CEL rule: spec.obo shipped as an empty placeholder in an + // earlier release, so a required field or a rule rejecting {} would be a + // backward-incompatible narrowing. The kubebuilder markers check only + // per-field shape (tenantId GUID/domain, authority URL, + // subjectTokenProviderName, scopes/cacheSkew bounds) at admission, and + // only for values that are present. All presence and combination + // requirements (a tenant, a client-auth credential, at least one of + // audience/scopes) and protocol-level checks are owned by the registered + // handler, not the upstream type. That handler runs at reconcile // time via the controllerutil.OBOValidate function-pointer hook: // upstream-only builds return obo.ErrEnterpriseRequired, which the // reconciler maps to status.conditions[Valid] = False / Reason: diff --git a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go index e8f5633d5e..09588577e9 100644 --- a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go +++ b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go @@ -11,14 +11,15 @@ import ( mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" ) -// These tests exercise the kubebuilder validation on OBOConfig (required -// fields, field patterns, and the "at least one of audience or scopes" CEL -// rule) through the real apiserver (envtest). They are the admission-time half -// of the OBOConfig validation contract: the upstream Go Validate() arm -// intentionally defers field-level validation to these markers and to the -// registered enterprise OBO handler, so the apiserver is where a malformed -// obo spec must be rejected. -var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", "cel", "validation"), func() { +// These tests exercise the kubebuilder schema validation on OBOConfig through +// the real apiserver (envtest). OBOConfig has no required field and no +// cross-field rule: spec.obo shipped as an empty placeholder ({}) in an earlier +// release, so the schema must keep admitting {} (and any subset of fields), and +// presence/combination requirements are enforced by the registered OBO handler +// at reconcile, not at admission. Admission only validates per-field shape +// (patterns, length/item bounds) for values that are present — that is what +// these tests pin down. +var _ = Describe("MCPExternalAuthConfig OBOConfig schema validation", Label("k8s", "validation"), func() { const namespace = "default" // makeOBOConfig returns an MCPExternalAuthConfig of type "obo" whose only @@ -73,82 +74,43 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig CEL validation", Label("k8s", }) }) - Context("required fields", func() { - It("should reject an empty OBOConfig (missing required fields)", func() { + Context("permissive schema (no required fields, no cross-field rule)", func() { + // spec.obo shipped as an empty placeholder in v0.29.3, so the schema must + // keep admitting {} and any subset of fields; the operator enforces + // presence/combination requirements at reconcile. + It("should accept an empty OBOConfig (the v0.29.3 {} placeholder must still round-trip)", func() { cfg := makeOBOConfig("obo-empty", &mcpv1beta1.OBOConfig{}) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) - It("should reject a missing tenantId", func() { + It("should accept a config without tenantId (operator enforces presence at reconcile)", func() { cfg := makeOBOConfig("obo-no-tenant", &mcpv1beta1.OBOConfig{ ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("tenantId")) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) It("should accept a config without clientId or clientSecretRef (operator enforces per auth mode)", func() { - // clientId/clientSecretRef are optional at the CRD level so future - // client-auth methods (certificate, workload identity) need no - // breaking schema change; the operator enforces the v1 shared-secret - // combination. Admission must therefore accept their absence. + // All presence/combination requirements (a tenant, a client-auth + // credential, an exchange target) are enforced by the operator handler + // at reconcile, not at admission, so future client-auth methods + // (certificate, workload identity) need no breaking schema change. cfg := makeOBOConfig("obo-no-client-auth", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", Audience: "api://backend", }) Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) - }) - Context("at least one of audience or scopes", func() { - It("should reject when neither audience nor scopes is set", func() { + It("should accept a config with neither audience nor scopes (operator enforces at reconcile)", func() { cfg := makeOBOConfig("obo-no-target", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", ClientID: "app-client-id", ClientSecretRef: secretRef, }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("at least one of audience or scopes must be set")) - }) - - It("should reject when scopes is an empty list and audience is unset", func() { - cfg := makeOBOConfig("obo-empty-scopes", &mcpv1beta1.OBOConfig{ - TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", - ClientID: "app-client-id", - ClientSecretRef: secretRef, - Scopes: []string{}, - }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("at least one of audience or scopes must be set")) - }) - - It("should reject a whitespace-only audience (mirrors ExchangeTarget trimming)", func() { - cfg := makeOBOConfig("obo-blank-audience", &mcpv1beta1.OBOConfig{ - TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", - ClientID: "app-client-id", - ClientSecretRef: secretRef, - Audience: " ", - }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("non-blank value")) - }) - - It("should reject scopes containing only blank entries", func() { - cfg := makeOBOConfig("obo-blank-scopes", &mcpv1beta1.OBOConfig{ - TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", - ClientID: "app-client-id", - ClientSecretRef: secretRef, - Scopes: []string{" "}, - }) - err := k8sClient.Create(ctx, cfg) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("non-blank value")) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) }) 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 0f9dc9c20c..2530cc90f8 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 @@ -1089,7 +1089,8 @@ spec: description: |- Audience is the backend target identifier requested in the exchanged token. Used as the exchange target when Scopes is empty. At least one of - audience or scopes must be set. + audience or scopes must be set; the operator enforces that at reconcile + (it is not an admission-time rule — see the type doc). type: string authority: description: |- @@ -1151,9 +1152,9 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. The MaxItems and - per-item length caps are defensive bounds that also keep the - audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + Audience. At least one of audience or scopes must be set; the operator + enforces that at reconcile. The MaxItems and per-item length caps are + defensive bounds on an otherwise unbounded list. items: maxLength: 256 minLength: 1 @@ -1180,28 +1181,22 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, - in one of the two forms the Entra v2.0 token endpoint addresses: a - directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). - Well-known aliases such as "common", "organizations", and "consumers" are - NOT accepted — an OBO confidential-client exchange must target a specific - tenant. The operator interpolates it into the token endpoint + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Optional at the CRD level (see the type doc); the operator enforces its + presence, since an OBO confidential-client exchange must target a specific + tenant. When set, it must be one of the two forms the Entra v2.0 token + endpoint addresses: a directory GUID, or a verified domain name (e.g. + contoso.onmicrosoft.com). Well-known aliases such as "common", + "organizations", and "consumers" are NOT accepted. The operator + interpolates it into the token endpoint (//oauth2/v2.0/token), so the value is constrained to - the GUID/domain shape (no path metacharacters). The pattern and 253-char + the GUID/domain shape (no path metacharacters); the pattern and 253-char cap mirror the enterprise exchanger's validateTenant, so any tenantId admitted here is one the runtime can consume. maxLength: 253 - minLength: 1 pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string - required: - - tenantId type: object - x-kubernetes-validations: - - message: at least one of audience or scopes must be set to a non-blank - value - rule: (has(self.audience) && self.audience.trim().size() > 0) || - (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2516,7 +2511,8 @@ spec: description: |- Audience is the backend target identifier requested in the exchanged token. Used as the exchange target when Scopes is empty. At least one of - audience or scopes must be set. + audience or scopes must be set; the operator enforces that at reconcile + (it is not an admission-time rule — see the type doc). type: string authority: description: |- @@ -2578,9 +2574,9 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. The MaxItems and - per-item length caps are defensive bounds that also keep the - audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + Audience. At least one of audience or scopes must be set; the operator + enforces that at reconcile. The MaxItems and per-item length caps are + defensive bounds on an otherwise unbounded list. items: maxLength: 256 minLength: 1 @@ -2607,28 +2603,22 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, - in one of the two forms the Entra v2.0 token endpoint addresses: a - directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). - Well-known aliases such as "common", "organizations", and "consumers" are - NOT accepted — an OBO confidential-client exchange must target a specific - tenant. The operator interpolates it into the token endpoint + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Optional at the CRD level (see the type doc); the operator enforces its + presence, since an OBO confidential-client exchange must target a specific + tenant. When set, it must be one of the two forms the Entra v2.0 token + endpoint addresses: a directory GUID, or a verified domain name (e.g. + contoso.onmicrosoft.com). Well-known aliases such as "common", + "organizations", and "consumers" are NOT accepted. The operator + interpolates it into the token endpoint (//oauth2/v2.0/token), so the value is constrained to - the GUID/domain shape (no path metacharacters). The pattern and 253-char + the GUID/domain shape (no path metacharacters); the pattern and 253-char cap mirror the enterprise exchanger's validateTenant, so any tenantId admitted here is one the runtime can consume. maxLength: 253 - minLength: 1 pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string - required: - - tenantId type: object - x-kubernetes-validations: - - message: at least one of audience or scopes must be set to a non-blank - value - rule: (has(self.audience) && self.audience.trim().size() > 0) || - (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange 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 d2db02c949..5e477a9bd9 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1092,7 +1092,8 @@ spec: description: |- Audience is the backend target identifier requested in the exchanged token. Used as the exchange target when Scopes is empty. At least one of - audience or scopes must be set. + audience or scopes must be set; the operator enforces that at reconcile + (it is not an admission-time rule — see the type doc). type: string authority: description: |- @@ -1154,9 +1155,9 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. The MaxItems and - per-item length caps are defensive bounds that also keep the - audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + Audience. At least one of audience or scopes must be set; the operator + enforces that at reconcile. The MaxItems and per-item length caps are + defensive bounds on an otherwise unbounded list. items: maxLength: 256 minLength: 1 @@ -1183,28 +1184,22 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, - in one of the two forms the Entra v2.0 token endpoint addresses: a - directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). - Well-known aliases such as "common", "organizations", and "consumers" are - NOT accepted — an OBO confidential-client exchange must target a specific - tenant. The operator interpolates it into the token endpoint + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Optional at the CRD level (see the type doc); the operator enforces its + presence, since an OBO confidential-client exchange must target a specific + tenant. When set, it must be one of the two forms the Entra v2.0 token + endpoint addresses: a directory GUID, or a verified domain name (e.g. + contoso.onmicrosoft.com). Well-known aliases such as "common", + "organizations", and "consumers" are NOT accepted. The operator + interpolates it into the token endpoint (//oauth2/v2.0/token), so the value is constrained to - the GUID/domain shape (no path metacharacters). The pattern and 253-char + the GUID/domain shape (no path metacharacters); the pattern and 253-char cap mirror the enterprise exchanger's validateTenant, so any tenantId admitted here is one the runtime can consume. maxLength: 253 - minLength: 1 pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string - required: - - tenantId type: object - x-kubernetes-validations: - - message: at least one of audience or scopes must be set to a non-blank - value - rule: (has(self.audience) && self.audience.trim().size() > 0) || - (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange @@ -2519,7 +2514,8 @@ spec: description: |- Audience is the backend target identifier requested in the exchanged token. Used as the exchange target when Scopes is empty. At least one of - audience or scopes must be set. + audience or scopes must be set; the operator enforces that at reconcile + (it is not an admission-time rule — see the type doc). type: string authority: description: |- @@ -2581,9 +2577,9 @@ spec: description: |- Scopes are the delegated scopes to request for the exchanged token, e.g. ["api:///.default"]. When non-empty they take precedence over - Audience. At least one of audience or scopes must be set. The MaxItems and - per-item length caps are defensive bounds that also keep the - audience-or-scopes CEL rule within the apiserver's per-rule cost budget. + Audience. At least one of audience or scopes must be set; the operator + enforces that at reconcile. The MaxItems and per-item length caps are + defensive bounds on an otherwise unbounded list. items: maxLength: 256 minLength: 1 @@ -2610,28 +2606,22 @@ spec: type: string tenantId: description: |- - TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier, - in one of the two forms the Entra v2.0 token endpoint addresses: a - directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com). - Well-known aliases such as "common", "organizations", and "consumers" are - NOT accepted — an OBO confidential-client exchange must target a specific - tenant. The operator interpolates it into the token endpoint + TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier. + Optional at the CRD level (see the type doc); the operator enforces its + presence, since an OBO confidential-client exchange must target a specific + tenant. When set, it must be one of the two forms the Entra v2.0 token + endpoint addresses: a directory GUID, or a verified domain name (e.g. + contoso.onmicrosoft.com). Well-known aliases such as "common", + "organizations", and "consumers" are NOT accepted. The operator + interpolates it into the token endpoint (//oauth2/v2.0/token), so the value is constrained to - the GUID/domain shape (no path metacharacters). The pattern and 253-char + the GUID/domain shape (no path metacharacters); the pattern and 253-char cap mirror the enterprise exchanger's validateTenant, so any tenantId admitted here is one the runtime can consume. maxLength: 253 - minLength: 1 pattern: ^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}|([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,})$ type: string - required: - - tenantId type: object - x-kubernetes-validations: - - message: at least one of audience or scopes must be set to a non-blank - value - rule: (has(self.audience) && self.audience.trim().size() > 0) || - (has(self.scopes) && self.scopes.exists(s, s.trim().size() > 0)) tokenExchange: description: |- TokenExchange configures RFC-8693 OAuth 2.0 Token Exchange diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 62382b5ebf..d450c47a9b 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2804,13 +2804,15 @@ Field-to-contract mapping performed by the operator's OBO handler: otherwise audience) - cacheSkew → the contract's integer-seconds cacheSkewSeconds -The XValidation rule mirrors the emptiness semantics of -obo.MiddlewareParameters.ExchangeTarget() (which TrimSpaces both fields): -it requires a non-blank audience or at least one non-blank scope, so a -present-but-whitespace value is rejected at admission rather than collapsing -to an empty exchange target only at reconcile. It checks presence, not the -scopes-win-over-audience preference — that collapse stays the single -responsibility of ExchangeTarget(). +Every field is optional at the CRD level, and the schema deliberately carries +no required field and no cross-field CEL rule. spec.obo shipped as an empty +placeholder ({}) in earlier releases, so adding a required field or an +admission rule that rejects {} would be a backward-incompatible narrowing of +an already-stored, round-trippable object. Presence and combination +requirements — a tenant, a client-auth credential, and at least one of +audience/scopes — are therefore enforced by the registered OBO handler at +reconcile, which reports a violation as Valid=False / Reason=InvalidConfig. +The per-field patterns below still apply, but only to a value that is present. @@ -2819,12 +2821,12 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier,
in one of the two forms the Entra v2.0 token endpoint addresses: a
directory GUID, or a verified domain name (e.g. contoso.onmicrosoft.com).
Well-known aliases such as "common", "organizations", and "consumers" are
NOT accepted — an OBO confidential-client exchange must target a specific
tenant. The operator interpolates it into the token endpoint
(//oauth2/v2.0/token), so the value is constrained to
the GUID/domain shape (no path metacharacters). The pattern and 253-char
cap mirror the enterprise exchanger's validateTenant, so any tenantId
admitted here is one the runtime can consume. | | MaxLength: 253
MinLength: 1
Pattern: `^([0-9a-fA-F]\{8\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{12\}\|([a-zA-Z0-9]([a-zA-Z0-9-]\{0,61\}[a-zA-Z0-9])?\.)+[a-zA-Z]\{2,\})$`
Required: \{\}
| +| `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier.
Optional at the CRD level (see the type doc); the operator enforces its
presence, since an OBO confidential-client exchange must target a specific
tenant. When set, it must be one of the two forms the Entra v2.0 token
endpoint addresses: a directory GUID, or a verified domain name (e.g.
contoso.onmicrosoft.com). Well-known aliases such as "common",
"organizations", and "consumers" are NOT accepted. The operator
interpolates it into the token endpoint
(//oauth2/v2.0/token), so the value is constrained to
the GUID/domain shape (no path metacharacters); the pattern and 253-char
cap mirror the enterprise exchanger's validateTenant, so any tenantId
admitted here is one the runtime can consume. | | MaxLength: 253
Pattern: `^([0-9a-fA-F]\{8\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{12\}\|([a-zA-Z0-9]([a-zA-Z0-9-]\{0,61\}[a-zA-Z0-9])?\.)+[a-zA-Z]\{2,\})$`
Optional: \{\}
| | `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint by joining , , and the
v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing
slash; a path IS permitted and is prefixed before the tenant segment, as
some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the
client secret and the end-user assertion to this host, so it is a trust
boundary and HTTPS is required. Mirrors the enterprise exchanger's
authority validation (no query/fragment), which deliberately allows
arbitrary hosts and paths for non-public clouds. | | Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| | `clientId` _string_ | ClientID is the confidential client's application (client) ID registered
in Entra. Emitted verbatim as the runtime contract's clientId.
Optional at the CRD level so future client-authentication methods (e.g.
certificate or workload-identity credentials, planned fast-follows) can be
added without a breaking schema change. The operator enforces that clientId
and clientSecretRef are both present for the v1 shared-secret flow. | | Optional: \{\}
| | `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the confidential
client's secret. v1 supports a shared client secret only. The operator
injects the resolved value into the proxyrunner pod as an environment
variable and emits only that variable's name in the runtime contract, as
clientSecretEnvVar — the secret value never travels in the contract.
Optional at the CRD level for the same forward-compatibility reason as
clientId (a certificate/workload-identity flow needs no client secret);
the operator enforces presence for the v1 shared-secret flow. | | Optional: \{\}
| -| `audience` _string_ | Audience is the backend target identifier requested in the exchanged
token. Used as the exchange target when Scopes is empty. At least one of
audience or scopes must be set. | | Optional: \{\}
| -| `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set. The MaxItems and
per-item length caps are defensive bounds that also keep the
audience-or-scopes CEL rule within the apiserver's per-rule cost budget. | | MaxItems: 20
items:MaxLength: 256
items:MinLength: 1
Optional: \{\}
| +| `audience` _string_ | Audience is the backend target identifier requested in the exchanged
token. Used as the exchange target when Scopes is empty. At least one of
audience or scopes must be set; the operator enforces that at reconcile
(it is not an admission-time rule — see the type doc). | | Optional: \{\}
| +| `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set; the operator
enforces that at reconcile. The MaxItems and per-item length caps are
defensive bounds on an otherwise unbounded list. | | MaxItems: 20
items:MaxLength: 256
items:MinLength: 1
Optional: \{\}
| | `subjectTokenProviderName` _string_ | SubjectTokenProviderName selects the source of the OBO subject (assertion)
token from the request's authenticated Identity:
- Omitted: use the inbound end-user token the client presented
(Identity.Token) — the deployment with no embedded auth server, where
the client holds an Entra token directly.
- Set: use the named upstream provider's token
(Identity.UpstreamTokens[]) — the embedded-auth-server
deployment, where the inbound token is the proxy's own session token.
The value must match a configured upstream provider name.
The subject is always sourced from the authenticated Identity, never from
an inbound request header, so the upstream auth middleware must run first. | | MaxLength: 63
MinLength: 1
Pattern: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`
Optional: \{\}
| | `cacheSkew` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#duration-v1-meta)_ | CacheSkew overrides the OBO token cache's default expiry skew (the margin
by which a cached token is treated as expired before its real expiry),
e.g. "30s". The operator converts it to the runtime contract's
integer-seconds cacheSkewSeconds. Must not be negative; a negative value
is rejected by the registered OBO handler at reconcile time
(Valid=False, Reason: InvalidConfig in enterprise builds). When omitted,
the cache default applies. | | Optional: \{\}
| From fefbe8f4c72c596b10dea95a3ddc868917d3e94c Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 11 Jun 2026 08:46:25 -0700 Subject: [PATCH 5/6] Reject userinfo in OBO authority; correct doc comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses stacklok/toolhive#5494 review comments: - MEDIUM authority (3397159713): the pattern admitted embedded userinfo, so https://login.microsoftonline.com@attacker.example was accepted while the real host (per RFC 3986) is attacker.example — host confusion at the credential trust boundary. Exclude "@" from the pattern. - LOW authority doc (3397159761): note the CRD is intentionally stricter than the runtime validateHTTPSURL (which accepts http loopback and a trailing slash), rather than implying exact parity. - LOW cacheSkew doc (3397159770): the negative-skew rejection is an enterprise-handler concern, not enforced at admission or upstream; soften the comment so it does not promise rejection that no current build does. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 30 ++++++---- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 60 +++++++++++-------- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 60 +++++++++++-------- docs/operator/crd-api.md | 4 +- 4 files changed, 92 insertions(+), 62 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index f69f678db8..e89e1d8a61 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -175,14 +175,18 @@ type OBOConfig struct { // https://login.microsoftonline.us (US Gov) or // https://login.partner.microsoftonline.cn (China). When set, the operator // builds the token endpoint by joining , , and the - // v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing - // slash; a path IS permitted and is prefixed before the tenant segment, as - // some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the - // client secret and the end-user assertion to this host, so it is a trust - // boundary and HTTPS is required. Mirrors the enterprise exchanger's - // authority validation (no query/fragment), which deliberately allows - // arbitrary hosts and paths for non-public clouds. - // +kubebuilder:validation:Pattern=`^https://[^\s?#]+[^/\s?#]$` + // v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment, + // or trailing slash; a path IS permitted and is prefixed before the tenant + // segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange + // POSTs the client secret and the end-user assertion to this host, so it is a + // credential trust boundary: HTTPS is required and userinfo (user@host) is + // rejected to prevent host confusion (per RFC 3986 the real host follows the + // "@", so https://login.microsoftonline.com@attacker.example targets + // attacker.example). This is intentionally stricter than the downstream + // exchanger's validateHTTPSURL, which also accepts http for loopback hosts + // and tolerates a trailing slash — rejecting those at admission is the safe + // direction. + // +kubebuilder:validation:Pattern=`^https://[^\s?#@]+[^/\s?#@]$` // +optional Authority string `json:"authority,omitempty"` @@ -245,10 +249,12 @@ type OBOConfig struct { // CacheSkew overrides the OBO token cache's default expiry skew (the margin // by which a cached token is treated as expired before its real expiry), // e.g. "30s". The operator converts it to the runtime contract's - // integer-seconds cacheSkewSeconds. Must not be negative; a negative value - // is rejected by the registered OBO handler at reconcile time - // (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, - // the cache default applies. + // integer-seconds cacheSkewSeconds. Should not be negative, but the schema + // does not enforce that — metav1.Duration carries no numeric minimum — and + // upstream builds do not reject it. A negative value is rejected only by an + // enterprise build's OBO handler once that handler validates the converted + // parameters; it is not enforced at admission or in upstream-only builds. + // When omitted, the cache default applies. // +optional CacheSkew *metav1.Duration `json:"cacheSkew,omitempty"` } 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 2530cc90f8..4f17bfc8b8 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 @@ -1099,24 +1099,30 @@ spec: https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator builds the token endpoint by joining , , and the - v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing - slash; a path IS permitted and is prefixed before the tenant segment, as - some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the - client secret and the end-user assertion to this host, so it is a trust - boundary and HTTPS is required. Mirrors the enterprise exchanger's - authority validation (no query/fragment), which deliberately allows - arbitrary hosts and paths for non-public clouds. - pattern: ^https://[^\s?#]+[^/\s?#]$ + v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment, + or trailing slash; a path IS permitted and is prefixed before the tenant + segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange + POSTs the client secret and the end-user assertion to this host, so it is a + credential trust boundary: HTTPS is required and userinfo (user@host) is + rejected to prevent host confusion (per RFC 3986 the real host follows the + "@", so https://login.microsoftonline.com@attacker.example targets + attacker.example). This is intentionally stricter than the downstream + exchanger's validateHTTPSURL, which also accepts http for loopback hosts + and tolerates a trailing slash — rejecting those at admission is the safe + direction. + pattern: ^https://[^\s?#@]+[^/\s?#@]$ type: string cacheSkew: description: |- CacheSkew overrides the OBO token cache's default expiry skew (the margin by which a cached token is treated as expired before its real expiry), e.g. "30s". The operator converts it to the runtime contract's - integer-seconds cacheSkewSeconds. Must not be negative; a negative value - is rejected by the registered OBO handler at reconcile time - (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, - the cache default applies. + integer-seconds cacheSkewSeconds. Should not be negative, but the schema + does not enforce that — metav1.Duration carries no numeric minimum — and + upstream builds do not reject it. A negative value is rejected only by an + enterprise build's OBO handler once that handler validates the converted + parameters; it is not enforced at admission or in upstream-only builds. + When omitted, the cache default applies. type: string clientId: description: |- @@ -2521,24 +2527,30 @@ spec: https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator builds the token endpoint by joining , , and the - v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing - slash; a path IS permitted and is prefixed before the tenant segment, as - some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the - client secret and the end-user assertion to this host, so it is a trust - boundary and HTTPS is required. Mirrors the enterprise exchanger's - authority validation (no query/fragment), which deliberately allows - arbitrary hosts and paths for non-public clouds. - pattern: ^https://[^\s?#]+[^/\s?#]$ + v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment, + or trailing slash; a path IS permitted and is prefixed before the tenant + segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange + POSTs the client secret and the end-user assertion to this host, so it is a + credential trust boundary: HTTPS is required and userinfo (user@host) is + rejected to prevent host confusion (per RFC 3986 the real host follows the + "@", so https://login.microsoftonline.com@attacker.example targets + attacker.example). This is intentionally stricter than the downstream + exchanger's validateHTTPSURL, which also accepts http for loopback hosts + and tolerates a trailing slash — rejecting those at admission is the safe + direction. + pattern: ^https://[^\s?#@]+[^/\s?#@]$ type: string cacheSkew: description: |- CacheSkew overrides the OBO token cache's default expiry skew (the margin by which a cached token is treated as expired before its real expiry), e.g. "30s". The operator converts it to the runtime contract's - integer-seconds cacheSkewSeconds. Must not be negative; a negative value - is rejected by the registered OBO handler at reconcile time - (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, - the cache default applies. + integer-seconds cacheSkewSeconds. Should not be negative, but the schema + does not enforce that — metav1.Duration carries no numeric minimum — and + upstream builds do not reject it. A negative value is rejected only by an + enterprise build's OBO handler once that handler validates the converted + parameters; it is not enforced at admission or in upstream-only builds. + When omitted, the cache default applies. type: string clientId: description: |- 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 5e477a9bd9..5ded5f6e1d 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1102,24 +1102,30 @@ spec: https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator builds the token endpoint by joining , , and the - v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing - slash; a path IS permitted and is prefixed before the tenant segment, as - some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the - client secret and the end-user assertion to this host, so it is a trust - boundary and HTTPS is required. Mirrors the enterprise exchanger's - authority validation (no query/fragment), which deliberately allows - arbitrary hosts and paths for non-public clouds. - pattern: ^https://[^\s?#]+[^/\s?#]$ + v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment, + or trailing slash; a path IS permitted and is prefixed before the tenant + segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange + POSTs the client secret and the end-user assertion to this host, so it is a + credential trust boundary: HTTPS is required and userinfo (user@host) is + rejected to prevent host confusion (per RFC 3986 the real host follows the + "@", so https://login.microsoftonline.com@attacker.example targets + attacker.example). This is intentionally stricter than the downstream + exchanger's validateHTTPSURL, which also accepts http for loopback hosts + and tolerates a trailing slash — rejecting those at admission is the safe + direction. + pattern: ^https://[^\s?#@]+[^/\s?#@]$ type: string cacheSkew: description: |- CacheSkew overrides the OBO token cache's default expiry skew (the margin by which a cached token is treated as expired before its real expiry), e.g. "30s". The operator converts it to the runtime contract's - integer-seconds cacheSkewSeconds. Must not be negative; a negative value - is rejected by the registered OBO handler at reconcile time - (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, - the cache default applies. + integer-seconds cacheSkewSeconds. Should not be negative, but the schema + does not enforce that — metav1.Duration carries no numeric minimum — and + upstream builds do not reject it. A negative value is rejected only by an + enterprise build's OBO handler once that handler validates the converted + parameters; it is not enforced at admission or in upstream-only builds. + When omitted, the cache default applies. type: string clientId: description: |- @@ -2524,24 +2530,30 @@ spec: https://login.microsoftonline.us (US Gov) or https://login.partner.microsoftonline.cn (China). When set, the operator builds the token endpoint by joining , , and the - v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing - slash; a path IS permitted and is prefixed before the tenant segment, as - some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the - client secret and the end-user assertion to this host, so it is a trust - boundary and HTTPS is required. Mirrors the enterprise exchanger's - authority validation (no query/fragment), which deliberately allows - arbitrary hosts and paths for non-public clouds. - pattern: ^https://[^\s?#]+[^/\s?#]$ + v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment, + or trailing slash; a path IS permitted and is prefixed before the tenant + segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange + POSTs the client secret and the end-user assertion to this host, so it is a + credential trust boundary: HTTPS is required and userinfo (user@host) is + rejected to prevent host confusion (per RFC 3986 the real host follows the + "@", so https://login.microsoftonline.com@attacker.example targets + attacker.example). This is intentionally stricter than the downstream + exchanger's validateHTTPSURL, which also accepts http for loopback hosts + and tolerates a trailing slash — rejecting those at admission is the safe + direction. + pattern: ^https://[^\s?#@]+[^/\s?#@]$ type: string cacheSkew: description: |- CacheSkew overrides the OBO token cache's default expiry skew (the margin by which a cached token is treated as expired before its real expiry), e.g. "30s". The operator converts it to the runtime contract's - integer-seconds cacheSkewSeconds. Must not be negative; a negative value - is rejected by the registered OBO handler at reconcile time - (Valid=False, Reason: InvalidConfig in enterprise builds). When omitted, - the cache default applies. + integer-seconds cacheSkewSeconds. Should not be negative, but the schema + does not enforce that — metav1.Duration carries no numeric minimum — and + upstream builds do not reject it. A negative value is rejected only by an + enterprise build's OBO handler once that handler validates the converted + parameters; it is not enforced at admission or in upstream-only builds. + When omitted, the cache default applies. type: string clientId: description: |- diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index d450c47a9b..c1e9616005 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2822,13 +2822,13 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | | `tenantId` _string_ | TenantID is the Microsoft Entra (Azure AD) directory (tenant) identifier.
Optional at the CRD level (see the type doc); the operator enforces its
presence, since an OBO confidential-client exchange must target a specific
tenant. When set, it must be one of the two forms the Entra v2.0 token
endpoint addresses: a directory GUID, or a verified domain name (e.g.
contoso.onmicrosoft.com). Well-known aliases such as "common",
"organizations", and "consumers" are NOT accepted. The operator
interpolates it into the token endpoint
(//oauth2/v2.0/token), so the value is constrained to
the GUID/domain shape (no path metacharacters); the pattern and 253-char
cap mirror the enterprise exchanger's validateTenant, so any tenantId
admitted here is one the runtime can consume. | | MaxLength: 253
Pattern: `^([0-9a-fA-F]\{8\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{4\}-[0-9a-fA-F]\{12\}\|([a-zA-Z0-9]([a-zA-Z0-9-]\{0,61\}[a-zA-Z0-9])?\.)+[a-zA-Z]\{2,\})$`
Optional: \{\}
| -| `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint by joining , , and the
v2.0 token path. Must be an HTTPS URL with no query, fragment, or trailing
slash; a path IS permitted and is prefixed before the tenant segment, as
some sovereign / B2C / CIAM endpoints require. The OBO exchange POSTs the
client secret and the end-user assertion to this host, so it is a trust
boundary and HTTPS is required. Mirrors the enterprise exchanger's
authority validation (no query/fragment), which deliberately allows
arbitrary hosts and paths for non-public clouds. | | Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| +| `authority` _string_ | Authority overrides the default Entra login host
(https://login.microsoftonline.com) for sovereign or national clouds, e.g.
https://login.microsoftonline.us (US Gov) or
https://login.partner.microsoftonline.cn (China). When set, the operator
builds the token endpoint by joining , , and the
v2.0 token path. Must be an HTTPS URL with no userinfo, query, fragment,
or trailing slash; a path IS permitted and is prefixed before the tenant
segment, as some sovereign / B2C / CIAM endpoints require. The OBO exchange
POSTs the client secret and the end-user assertion to this host, so it is a
credential trust boundary: HTTPS is required and userinfo (user@host) is
rejected to prevent host confusion (per RFC 3986 the real host follows the
"@", so https://login.microsoftonline.com@attacker.example targets
attacker.example). This is intentionally stricter than the downstream
exchanger's validateHTTPSURL, which also accepts http for loopback hosts
and tolerates a trailing slash — rejecting those at admission is the safe
direction. | | Pattern: `^https://[^\s?#@]+[^/\s?#@]$`
Optional: \{\}
| | `clientId` _string_ | ClientID is the confidential client's application (client) ID registered
in Entra. Emitted verbatim as the runtime contract's clientId.
Optional at the CRD level so future client-authentication methods (e.g.
certificate or workload-identity credentials, planned fast-follows) can be
added without a breaking schema change. The operator enforces that clientId
and clientSecretRef are both present for the v1 shared-secret flow. | | Optional: \{\}
| | `clientSecretRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | ClientSecretRef references a Kubernetes Secret containing the confidential
client's secret. v1 supports a shared client secret only. The operator
injects the resolved value into the proxyrunner pod as an environment
variable and emits only that variable's name in the runtime contract, as
clientSecretEnvVar — the secret value never travels in the contract.
Optional at the CRD level for the same forward-compatibility reason as
clientId (a certificate/workload-identity flow needs no client secret);
the operator enforces presence for the v1 shared-secret flow. | | Optional: \{\}
| | `audience` _string_ | Audience is the backend target identifier requested in the exchanged
token. Used as the exchange target when Scopes is empty. At least one of
audience or scopes must be set; the operator enforces that at reconcile
(it is not an admission-time rule — see the type doc). | | Optional: \{\}
| | `scopes` _string array_ | Scopes are the delegated scopes to request for the exchanged token, e.g.
["api:///.default"]. When non-empty they take precedence over
Audience. At least one of audience or scopes must be set; the operator
enforces that at reconcile. The MaxItems and per-item length caps are
defensive bounds on an otherwise unbounded list. | | MaxItems: 20
items:MaxLength: 256
items:MinLength: 1
Optional: \{\}
| | `subjectTokenProviderName` _string_ | SubjectTokenProviderName selects the source of the OBO subject (assertion)
token from the request's authenticated Identity:
- Omitted: use the inbound end-user token the client presented
(Identity.Token) — the deployment with no embedded auth server, where
the client holds an Entra token directly.
- Set: use the named upstream provider's token
(Identity.UpstreamTokens[]) — the embedded-auth-server
deployment, where the inbound token is the proxy's own session token.
The value must match a configured upstream provider name.
The subject is always sourced from the authenticated Identity, never from
an inbound request header, so the upstream auth middleware must run first. | | MaxLength: 63
MinLength: 1
Pattern: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`
Optional: \{\}
| -| `cacheSkew` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#duration-v1-meta)_ | CacheSkew overrides the OBO token cache's default expiry skew (the margin
by which a cached token is treated as expired before its real expiry),
e.g. "30s". The operator converts it to the runtime contract's
integer-seconds cacheSkewSeconds. Must not be negative; a negative value
is rejected by the registered OBO handler at reconcile time
(Valid=False, Reason: InvalidConfig in enterprise builds). When omitted,
the cache default applies. | | Optional: \{\}
| +| `cacheSkew` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#duration-v1-meta)_ | CacheSkew overrides the OBO token cache's default expiry skew (the margin
by which a cached token is treated as expired before its real expiry),
e.g. "30s". The operator converts it to the runtime contract's
integer-seconds cacheSkewSeconds. Should not be negative, but the schema
does not enforce that — metav1.Duration carries no numeric minimum — and
upstream builds do not reject it. A negative value is rejected only by an
enterprise build's OBO handler once that handler validates the converted
parameters; it is not enforced at admission or in upstream-only builds.
When omitted, the cache default applies. | | Optional: \{\}
| #### api.v1beta1.OIDCUpstreamConfig From d2d5e8d30fbc94e4596cb7ec05157f450618d5fa Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 11 Jun 2026 08:55:28 -0700 Subject: [PATCH 6/6] Strengthen OBOConfig schema validation tests Addresses stacklok/toolhive#5494 review comments: - MEDIUM test quality (3397159791): pattern-reject specs asserted only ShouldNot(Succeed()), so a rejection for an unrelated reason would keep them green. Add a shared rejectsWithField helper and assert each error names the offending field (tenantId/authority/subjectTokenProviderName). - MEDIUM coverage (3397159798): add authority reject specs for a query string, a fragment, and embedded userinfo (the last locks the userinfo fix from the sibling commit). - LOW coverage (3397159806): add schema-bound specs (tenantId >253 chars, >20 scopes, a 257-char scope item) and an isolated accept for a valid lowercase subjectTokenProviderName. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../mcp-external-auth/obo_validation_test.go | 121 +++++++++++++++--- 1 file changed, 103 insertions(+), 18 deletions(-) diff --git a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go index 09588577e9..5493de4dd7 100644 --- a/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go +++ b/cmd/thv-operator/test-integration/mcp-external-auth/obo_validation_test.go @@ -4,6 +4,8 @@ package controllers import ( + "strings" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,6 +40,16 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig schema validation", Label("k8s secretRef := &mcpv1beta1.SecretKeyRef{Name: "entra-client", Key: "clientSecret"} + // rejectsWithField asserts admission fails AND the error names the field + // under test, so a future schema change that rejected for an unrelated + // reason cannot silently keep these specs green while dropping the coverage + // they claim. + rejectsWithField := func(name, field string, obo *mcpv1beta1.OBOConfig) { + err := k8sClient.Create(ctx, makeOBOConfig(name, obo)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(field)) + } + Context("valid configurations", func() { It("should accept a minimal config with audience", func() { cfg := makeOBOConfig("obo-valid-audience", &mcpv1beta1.OBOConfig{ @@ -115,49 +127,85 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig schema validation", Label("k8s }) Context("field patterns", func() { - It("should reject a tenantId containing a path separator", func() { - cfg := makeOBOConfig("obo-bad-tenant", &mcpv1beta1.OBOConfig{ + It("rejects a tenantId containing a path separator", func() { + rejectsWithField("obo-bad-tenant", "tenantId", &mcpv1beta1.OBOConfig{ TenantID: "tenant/../evil", ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", }) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) - It("should reject a non-HTTPS authority", func() { - cfg := makeOBOConfig("obo-http-authority", &mcpv1beta1.OBOConfig{ + It("rejects a tenantId well-known alias like 'common' (OBO needs a specific tenant)", func() { + rejectsWithField("obo-tenant-common", "tenantId", &mcpv1beta1.OBOConfig{ + TenantID: "common", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + }) + + It("rejects a non-HTTPS authority", func() { + rejectsWithField("obo-http-authority", "authority", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", Authority: "http://login.microsoftonline.us", ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", }) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) - It("should reject an authority with a trailing slash", func() { - cfg := makeOBOConfig("obo-authority-slash", &mcpv1beta1.OBOConfig{ + It("rejects an authority with a trailing slash", func() { + rejectsWithField("obo-authority-slash", "authority", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", Authority: "https://login.microsoftonline.us/", ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", }) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) - It("should reject a tenantId well-known alias like 'common' (OBO needs a specific tenant)", func() { - cfg := makeOBOConfig("obo-tenant-common", &mcpv1beta1.OBOConfig{ - TenantID: "common", + It("rejects an authority with a query string", func() { + rejectsWithField("obo-authority-query", "authority", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "https://login.microsoftonline.us?foo=bar", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + }) + + It("rejects an authority with a fragment", func() { + rejectsWithField("obo-authority-frag", "authority", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "https://login.microsoftonline.us#frag", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + }) + + It("rejects an authority with embedded userinfo (RFC 3986 host confusion)", func() { + rejectsWithField("obo-authority-userinfo", "authority", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + Authority: "https://login.microsoftonline.com@attacker.example", ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", }) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) - It("should accept an authority with a path (B2C/CIAM/sovereign clouds use token paths)", func() { + It("rejects an uppercase subjectTokenProviderName", func() { + rejectsWithField("obo-bad-subject", "subjectTokenProviderName", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + SubjectTokenProviderName: "Corp-IDP", + }) + }) + + It("accepts an authority with a path (B2C/CIAM/sovereign clouds use token paths)", func() { cfg := makeOBOConfig("obo-authority-path", &mcpv1beta1.OBOConfig{ TenantID: "contoso.onmicrosoft.com", Authority: "https://contoso.ciamlogin.com/contoso.onmicrosoft.com", @@ -168,15 +216,52 @@ var _ = Describe("MCPExternalAuthConfig OBOConfig schema validation", Label("k8s Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) }) - It("should reject an uppercase subjectTokenProviderName", func() { - cfg := makeOBOConfig("obo-bad-subject", &mcpv1beta1.OBOConfig{ + It("accepts a valid lowercase subjectTokenProviderName on its own", func() { + cfg := makeOBOConfig("obo-valid-subject", &mcpv1beta1.OBOConfig{ TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", ClientID: "app-client-id", ClientSecretRef: secretRef, Audience: "api://backend", - SubjectTokenProviderName: "Corp-IDP", + SubjectTokenProviderName: "corp-idp", + }) + Expect(k8sClient.Create(ctx, cfg)).Should(Succeed()) + }) + }) + + Context("schema bounds", func() { + It("rejects a tenantId longer than 253 characters", func() { + // A pattern-valid domain ("a." labels + a TLD) that exceeds the + // 253-char cap (which mirrors the exchanger's maxTenantLen), so the + // MaxLength bound fires rather than the pattern. + longTenant := strings.Repeat("a.", 126) + "co" // 254 chars + rejectsWithField("obo-tenant-toolong", "tenantId", &mcpv1beta1.OBOConfig{ + TenantID: longTenant, + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Audience: "api://backend", + }) + }) + + It("rejects more than 20 scopes", func() { + scopes := make([]string, 21) + for i := range scopes { + scopes[i] = "api://backend/.default" + } + rejectsWithField("obo-too-many-scopes", "scopes", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Scopes: scopes, + }) + }) + + It("rejects a scope item longer than 256 characters", func() { + rejectsWithField("obo-scope-toolong", "scopes", &mcpv1beta1.OBOConfig{ + TenantID: "72f988bf-86f1-41af-91ab-2d7cd011db47", + ClientID: "app-client-id", + ClientSecretRef: secretRef, + Scopes: []string{strings.Repeat("a", 257)}, }) - Expect(k8sClient.Create(ctx, cfg)).ShouldNot(Succeed()) }) }) })