From 90db2a6f8f69f1cc29ab2da37af5906f69f50bed Mon Sep 17 00:00:00 2001 From: amirejaz Date: Wed, 27 May 2026 02:01:51 +0500 Subject: [PATCH 1/3] Validate CIMD scope, grant_types and response_types against AS policy C3 - Thread ScopesSupported into NewCIMDStorageDecorator so CIMD scope handling is consistent with DCR. Uses registration.ValidateScopes (same function as the DCR handler) to validate declared scopes against the AS allowlist and compute the effective scope list. When ScopesSupported is unset, the document's declared scopes are used directly; omitted scopes default to DefaultScopes. C4 - Reject CIMD documents that declare grant_types or response_types the embedded AS does not support for public clients (authorization_code + refresh_token; code). Consistent with DCR which returns invalid_client_metadata for the same cases. buildFositeClient now receives pre-computed scopes from fetch() rather than re-parsing doc.Scope, matching the DCR handler pattern where scope computation and validation happen before client construction. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/authserver/server_impl.go | 2 +- pkg/authserver/storage/cimd_decorator.go | 81 ++++++++-- pkg/authserver/storage/cimd_decorator_test.go | 149 ++++++++++++++++-- 3 files changed, 204 insertions(+), 28 deletions(-) diff --git a/pkg/authserver/server_impl.go b/pkg/authserver/server_impl.go index 39a22468ca..91932395c2 100644 --- a/pkg/authserver/server_impl.go +++ b/pkg/authserver/server_impl.go @@ -177,7 +177,7 @@ func newServer(ctx context.Context, cfg Config, stor storage.Storage, opts ...se // so that GetClient calls for HTTPS client_id values are intercepted at the // fosite level (not just the handler level). if cfg.CIMDEnabled { - stor, err = storage.NewCIMDStorageDecorator(stor, true, cfg.CIMDCacheMaxSize, cfg.CIMDCacheFallbackTTL) + stor, err = storage.NewCIMDStorageDecorator(stor, true, cfg.CIMDCacheMaxSize, cfg.CIMDCacheFallbackTTL, cfg.ScopesSupported) if err != nil { return nil, fmt.Errorf("failed to initialize CIMD storage decorator: %w", err) } diff --git a/pkg/authserver/storage/cimd_decorator.go b/pkg/authserver/storage/cimd_decorator.go index fbead1ef50..95c812304d 100644 --- a/pkg/authserver/storage/cimd_decorator.go +++ b/pkg/authserver/storage/cimd_decorator.go @@ -28,10 +28,11 @@ import ( // Only GetClient is overridden. DCR clients (opaque IDs) continue to work // exactly as before. type CIMDStorageDecorator struct { - Storage // embed full interface — all methods delegate - sf singleflight.Group // deduplicates concurrent fetches for the same URL - cache *lru.Cache[string, *cimdCacheEntry] - ttl time.Duration + Storage // embed full interface — all methods delegate + sf singleflight.Group // deduplicates concurrent fetches for the same URL + cache *lru.Cache[string, *cimdCacheEntry] + ttl time.Duration + scopesSupported []string // AS-configured scopes; nil means accept any } type cimdCacheEntry struct { @@ -43,11 +44,15 @@ type cimdCacheEntry struct { // it returns base unchanged (no allocation). cacheMaxSize must be >= 1; // fallbackTTL is the fixed TTL applied to every cache entry (Cache-Control // header parsing is not yet implemented; all entries use this value). +// scopesSupported is the AS-configured scope allowlist; documents that declare +// scopes outside this set are rejected at fetch time. Pass nil to skip scope +// validation (e.g. when ScopesSupported is unset and DefaultScopes applies). func NewCIMDStorageDecorator( base Storage, enabled bool, cacheMaxSize int, fallbackTTL time.Duration, + scopesSupported []string, ) (Storage, error) { if !enabled { return base, nil @@ -63,9 +68,10 @@ func NewCIMDStorageDecorator( } return &CIMDStorageDecorator{ - Storage: base, - cache: c, - ttl: fallbackTTL, + Storage: base, + cache: c, + ttl: fallbackTTL, + scopesSupported: scopesSupported, }, nil } @@ -129,7 +135,49 @@ func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Cli id, m, defaultCIMDTokenEndpointAuthMethod) } - client := buildFositeClient(doc) + // Reject documents that declare grant_types the embedded AS does not support. + // Consistent with DCR which restricts public clients to authorization_code + refresh_token. + allowedGrantTypes := map[string]bool{"authorization_code": true, "refresh_token": true} + for _, gt := range doc.GrantTypes { + if !allowedGrantTypes[gt] { + return nil, fmt.Errorf("%w: CIMD document at %s claims grant_type %q "+ + "but this server only supports %v for public clients", + fosite.ErrNotFound.WithHint("unsupported grant_type"), + id, gt, defaultCIMDGrantTypes) + } + } + + // Reject documents that declare response_types the embedded AS does not support. + allowedResponseTypes := map[string]bool{"code": true} + for _, rt := range doc.ResponseTypes { + if !allowedResponseTypes[rt] { + return nil, fmt.Errorf("%w: CIMD document at %s claims response_type %q "+ + "but this server only supports %v", + fosite.ErrNotFound.WithHint("unsupported response_type"), + id, rt, defaultCIMDResponseTypes) + } + } + + // Compute and validate the client scope list consistent with DCR. + // When ScopesSupported is configured: use registration.ValidateScopes which + // validates each declared scope against the allowlist and falls back to + // DefaultScopes (also validated) when the document omits the field — the + // same logic the DCR handler applies. + // When ScopesSupported is not configured: no AS-level validation; use the + // declared scopes directly, or nil to let buildFositeClient apply DefaultScopes. + var resolvedScopes []string + if len(d.scopesSupported) > 0 { + computed, dcrErr := registration.ValidateScopes(strings.Fields(doc.Scope), d.scopesSupported) + if dcrErr != nil { + return nil, fmt.Errorf("%w: CIMD document at %s: %s", + fosite.ErrNotFound.WithHint(string(dcrErr.Error)), id, dcrErr.ErrorDescription) + } + resolvedScopes = computed + } else if doc.Scope != "" { + resolvedScopes = strings.Fields(doc.Scope) + } + + client := buildFositeClient(doc, resolvedScopes) d.cache.Add(id, &cimdCacheEntry{ client: client, @@ -157,7 +205,9 @@ const defaultCIMDTokenEndpointAuthMethod = "none" // buildFositeClient converts a ClientMetadataDocument into a fosite.Client. // Redirect URIs containing http://localhost are wrapped in a LoopbackClient // so that RFC 8252 §7.3 dynamic port matching applies. -func buildFositeClient(doc *cimd.ClientMetadataDocument) fosite.Client { +// resolvedScopes is the already-validated scope list computed by fetch() via +// registration.ValidateScopes; when nil, DefaultScopes is used (unconstrained AS). +func buildFositeClient(doc *cimd.ClientMetadataDocument, resolvedScopes []string) fosite.Client { grantTypes := doc.GrantTypes if len(grantTypes) == 0 { grantTypes = defaultCIMDGrantTypes @@ -173,13 +223,12 @@ func buildFositeClient(doc *cimd.ClientMetadataDocument) fosite.Client { tokenEndpointAuthMethod = defaultCIMDTokenEndpointAuthMethod } - // When the document omits the scope field, apply the same defaults as DCR - // registration so CIMD clients can request openid/profile/email/offline_access - // without needing to enumerate them explicitly in the metadata document. - // Clone to avoid aliasing the package-level DefaultScopes slice. - scopes := slices.Clone(registration.DefaultScopes) - if doc.Scope != "" { - scopes = strings.Fields(doc.Scope) + // Scopes were computed and validated by fetch() via registration.ValidateScopes, + // consistent with the DCR handler. Fall back to DefaultScopes only when the + // decorator has no ScopesSupported restriction (unconstrained AS). + scopes := resolvedScopes + if len(scopes) == 0 { + scopes = slices.Clone(registration.DefaultScopes) } defaultClient := &fosite.DefaultClient{ diff --git a/pkg/authserver/storage/cimd_decorator_test.go b/pkg/authserver/storage/cimd_decorator_test.go index 99c3d66dc8..202cde45b9 100644 --- a/pkg/authserver/storage/cimd_decorator_test.go +++ b/pkg/authserver/storage/cimd_decorator_test.go @@ -8,6 +8,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "sync" "sync/atomic" "testing" @@ -62,7 +63,7 @@ func newTestBase(t *testing.T) *MemoryStorage { // newEnabledDecorator creates a CIMDStorageDecorator wrapping base. func newEnabledDecorator(t *testing.T, base *MemoryStorage, maxSize int, ttl time.Duration) *CIMDStorageDecorator { t.Helper() - got, err := NewCIMDStorageDecorator(base, true, maxSize, ttl) + got, err := NewCIMDStorageDecorator(base, true, maxSize, ttl, nil) require.NoError(t, err) return got.(*CIMDStorageDecorator) } @@ -77,7 +78,7 @@ func cimdURL(srv *httptest.Server, path string) string { func TestNewCIMDStorageDecorator_DisabledReturnsBase(t *testing.T) { t.Parallel() base := newTestBase(t) - got, err := NewCIMDStorageDecorator(base, false, 10, time.Minute) + got, err := NewCIMDStorageDecorator(base, false, 10, time.Minute, nil) require.NoError(t, err) assert.Same(t, base, got, "disabled decorator must return base unchanged") } @@ -85,21 +86,21 @@ func TestNewCIMDStorageDecorator_DisabledReturnsBase(t *testing.T) { func TestNewCIMDStorageDecorator_ZeroCacheSizeReturnsError(t *testing.T) { t.Parallel() base := newTestBase(t) - _, err := NewCIMDStorageDecorator(base, true, 0, time.Minute) + _, err := NewCIMDStorageDecorator(base, true, 0, time.Minute, nil) require.Error(t, err) } func TestNewCIMDStorageDecorator_NegativeCacheSizeReturnsError(t *testing.T) { t.Parallel() base := newTestBase(t) - _, err := NewCIMDStorageDecorator(base, true, -1, time.Minute) + _, err := NewCIMDStorageDecorator(base, true, -1, time.Minute, nil) require.Error(t, err) } func TestNewCIMDStorageDecorator_EnabledReturnsCIMDDecorator(t *testing.T) { t.Parallel() base := newTestBase(t) - got, err := NewCIMDStorageDecorator(base, true, 10, time.Minute) + got, err := NewCIMDStorageDecorator(base, true, 10, time.Minute, nil) require.NoError(t, err) require.NotNil(t, got) _, isCIMD := got.(*CIMDStorageDecorator) @@ -336,7 +337,7 @@ func TestBuildFositeClient_Defaults(t *testing.T) { RedirectURIs: []string{"https://example.com/callback"}, } - got := buildFositeClient(doc) + got := buildFositeClient(doc, nil) assert.Equal(t, "https://example.com/meta.json", got.GetID()) assert.True(t, got.IsPublic()) assert.ElementsMatch(t, []string{"authorization_code", "refresh_token"}, []string(got.GetGrantTypes())) @@ -355,7 +356,7 @@ func TestBuildFositeClient_ExplicitGrantTypes(t *testing.T) { GrantTypes: []string{"authorization_code"}, } - got := buildFositeClient(doc) + got := buildFositeClient(doc, nil) assert.ElementsMatch(t, []string{"authorization_code"}, []string(got.GetGrantTypes())) } @@ -368,7 +369,9 @@ func TestBuildFositeClient_ScopeParsing(t *testing.T) { Scope: "openid profile email", } - got := buildFositeClient(doc) + // Scope parsing is now done by fetch() before calling buildFositeClient. + resolvedScopes := strings.Fields(doc.Scope) + got := buildFositeClient(doc, resolvedScopes) assert.ElementsMatch(t, []string{"openid", "profile", "email"}, []string(got.GetScopes())) } @@ -380,7 +383,7 @@ func TestBuildFositeClient_LoopbackRedirectWrapsInLoopbackClient(t *testing.T) { RedirectURIs: []string{"http://localhost/callback"}, } - got := buildFositeClient(doc) + got := buildFositeClient(doc, nil) // LoopbackClient adds MatchRedirectURI — check the distinctive method is present. type loopbackMatcher interface { MatchRedirectURI(string) bool @@ -403,7 +406,7 @@ func TestBuildFositeClient_NonLoopbackRedirectReturnsOpenIDConnectClient(t *test RedirectURIs: []string{"https://example.com/callback"}, } - got := buildFositeClient(doc) + got := buildFositeClient(doc, nil) _, ok := got.(*fosite.DefaultOpenIDConnectClient) assert.True(t, ok, "non-loopback redirect URI must produce a DefaultOpenIDConnectClient") } @@ -416,7 +419,7 @@ func TestBuildFositeClient_TokenEndpointAuthMethodDefault(t *testing.T) { RedirectURIs: []string{"https://example.com/callback"}, } - got := buildFositeClient(doc) + got := buildFositeClient(doc, nil) if oidc, ok := got.(fosite.OpenIDConnectClient); ok { assert.Equal(t, "none", oidc.GetTokenEndpointAuthMethod()) } @@ -442,3 +445,127 @@ func TestFetch_RejectsUnsupportedTokenEndpointAuthMethod(t *testing.T) { _, err := dec.fetchOrCached(context.Background(), srv.URL+"/meta.json") require.Error(t, err, "fetch must fail when token_endpoint_auth_method is not \"none\"") } + +// --- C4: grant_types / response_types validation --- + +func TestFetch_RejectsUnsupportedGrantType(t *testing.T) { + t.Parallel() + for _, unsupported := range []string{"client_credentials", "implicit", "urn:ietf:params:oauth:grant-type:device_code"} { + t.Run(unsupported, func(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + clientID := "http://" + r.Host + r.URL.Path + doc := cimd.ClientMetadataDocument{ + ClientID: clientID, + RedirectURIs: []string{"https://example.com/callback"}, + GrantTypes: []string{unsupported}, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(doc) + })) + t.Cleanup(srv.Close) + dec := newEnabledDecorator(t, newTestBase(t), 10, time.Minute) + _, err := dec.fetchOrCached(context.Background(), srv.URL+"/meta.json") + require.Error(t, err, "unsupported grant_type %q must be rejected", unsupported) + }) + } +} + +func TestFetch_AcceptsSupportedGrantTypes(t *testing.T) { + t.Parallel() + srv := serveCIMDDoc(t, "/meta.json", nil) + dec := newEnabledDecorator(t, newTestBase(t), 10, time.Minute) + // Default grant_types (omitted in document) must succeed + _, err := dec.fetchOrCached(context.Background(), cimdURL(srv, "/meta.json")) + require.NoError(t, err) +} + +func TestFetch_RejectsUnsupportedResponseType(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + clientID := "http://" + r.Host + r.URL.Path + doc := cimd.ClientMetadataDocument{ + ClientID: clientID, + RedirectURIs: []string{"https://example.com/callback"}, + ResponseTypes: []string{"token"}, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(doc) + })) + t.Cleanup(srv.Close) + dec := newEnabledDecorator(t, newTestBase(t), 10, time.Minute) + _, err := dec.fetchOrCached(context.Background(), srv.URL+"/meta.json") + require.Error(t, err, "unsupported response_type \"token\" must be rejected") +} + +// --- C3: scope validation against ScopesSupported --- + +func TestBuildFositeClient_ScopeDefaultsToScopesSupported(t *testing.T) { + t.Parallel() + doc := &cimd.ClientMetadataDocument{ + ClientID: "https://example.com/meta.json", + RedirectURIs: []string{"https://example.com/callback"}, + // Scope deliberately omitted + } + scopesSupported := []string{"openid", "profile"} + got := buildFositeClient(doc, scopesSupported) + assert.ElementsMatch(t, scopesSupported, []string(got.GetScopes()), + "omitted scope should default to ScopesSupported, not DefaultScopes") +} + +func TestBuildFositeClient_ScopeDefaultsToDefaultScopesWhenNoScopesSupported(t *testing.T) { + t.Parallel() + doc := &cimd.ClientMetadataDocument{ + ClientID: "https://example.com/meta.json", + RedirectURIs: []string{"https://example.com/callback"}, + } + got := buildFositeClient(doc, nil) + assert.ElementsMatch(t, registration.DefaultScopes, []string(got.GetScopes()), + "omitted scope with no ScopesSupported should default to registration.DefaultScopes") +} + +func TestFetch_RejectsScopeOutsideScopesSupported(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + clientID := "http://" + r.Host + r.URL.Path + doc := cimd.ClientMetadataDocument{ + ClientID: clientID, + RedirectURIs: []string{"https://example.com/callback"}, + Scope: "openid profile email", + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(doc) + })) + t.Cleanup(srv.Close) + + // Decorator configured with scopesSupported=["openid"] only + got, err := NewCIMDStorageDecorator(newTestBase(t), true, 10, time.Minute, []string{"openid"}) + require.NoError(t, err) + dec := got.(*CIMDStorageDecorator) + + _, err = dec.fetchOrCached(context.Background(), srv.URL+"/meta.json") + require.Error(t, err, "scope outside ScopesSupported must be rejected") +} + +func TestFetch_AcceptsScopeWithinScopesSupported(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + clientID := "http://" + r.Host + r.URL.Path + doc := cimd.ClientMetadataDocument{ + ClientID: clientID, + RedirectURIs: []string{"https://example.com/callback"}, + Scope: "openid", + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(doc) + })) + t.Cleanup(srv.Close) + + got, err := NewCIMDStorageDecorator(newTestBase(t), true, 10, time.Minute, []string{"openid", "profile"}) + require.NoError(t, err) + dec := got.(*CIMDStorageDecorator) + + client, err := dec.fetchOrCached(context.Background(), srv.URL+"/meta.json") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"openid"}, []string(client.GetScopes())) +} From b8e80285770ea8cd9d233043f273b74bc4a1acca Mon Sep 17 00:00:00 2001 From: amirejaz Date: Tue, 26 May 2026 18:34:57 +0500 Subject: [PATCH 2/3] Expose CIMD config in MCPExternalAuthConfig CRD Adds EmbeddedAuthServerCIMDConfig to the CRD so operators can enable CIMD through the normal VirtualMCPServer manifest workflow instead of writing runconfig.json directly. Resolves the TODO(cimd) comment in pkg/authserver/config.go. The new cimd field on EmbeddedAuthServerConfig maps to authserver.CIMDRunConfig in the generated RunConfig. CacheFallbackTTL is stored as a Go duration string in the CRD (e.g. "5m") and parsed to time.Duration by the converter. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../v1beta1/mcpexternalauthconfig_types.go | 29 ++++ .../api/v1beta1/zz_generated.deepcopy.go | 20 +++ .../pkg/controllerutil/authserver.go | 17 +++ .../pkg/controllerutil/authserver_test.go | 125 ++++++++++++++++++ ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 54 ++++++++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 54 ++++++++ ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 54 ++++++++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 54 ++++++++ pkg/authserver/config.go | 4 - 9 files changed, 407 insertions(+), 4 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 25bd4f603f..44698f1e4c 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -299,6 +299,10 @@ type EmbeddedAuthServerConfig struct { // +listType=atomic // +optional BaselineClientScopes []string `json:"baselineClientScopes,omitempty"` + + // CIMD configures Client ID Metadata Document support. When omitted, CIMD is disabled. + // +optional + CIMD *EmbeddedAuthServerCIMDConfig `json:"cimd,omitempty"` } // TokenLifespanConfig holds configuration for token lifetimes. @@ -325,6 +329,31 @@ type TokenLifespanConfig struct { AuthCodeLifespan string `json:"authCodeLifespan,omitempty"` } +// EmbeddedAuthServerCIMDConfig configures Client ID Metadata Document (CIMD) support +// on the embedded authorization server. When enabled, the AS accepts HTTPS URLs as +// client_id values and resolves them via the CIMD protocol, allowing clients such as +// VS Code to authenticate without prior Dynamic Client Registration. +type EmbeddedAuthServerCIMDConfig struct { + // Enabled activates CIMD client lookup. When false (the default), the AS only + // accepts client_id values that were registered via DCR. + // +kubebuilder:default=false + Enabled bool `json:"enabled"` + + // CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + // Defaults to 256 when Enabled is true and this field is omitted. + // +kubebuilder:validation:Minimum=1 + // +optional + CacheMaxSize int `json:"cacheMaxSize,omitempty"` + + // CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + // Cache-Control header parsing is not yet implemented; all entries use this value. + // Format: Go duration string (e.g. "5m", "10m", "1h"). + // Defaults to 5 minutes when Enabled is true and this field is omitted. + // +kubebuilder:validation:Pattern=`^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$` + // +optional + CacheFallbackTTL string `json:"cacheFallbackTtl,omitempty"` +} + // UpstreamProviderType identifies the type of upstream Identity Provider. type UpstreamProviderType string diff --git a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go index 07077737b5..cbfcf8cf8d 100644 --- a/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go +++ b/cmd/thv-operator/api/v1beta1/zz_generated.deepcopy.go @@ -225,6 +225,21 @@ func (in *DCRUpstreamConfig) DeepCopy() *DCRUpstreamConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *EmbeddedAuthServerCIMDConfig) DeepCopyInto(out *EmbeddedAuthServerCIMDConfig) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EmbeddedAuthServerCIMDConfig. +func (in *EmbeddedAuthServerCIMDConfig) DeepCopy() *EmbeddedAuthServerCIMDConfig { + if in == nil { + return nil + } + out := new(EmbeddedAuthServerCIMDConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EmbeddedAuthServerConfig) DeepCopyInto(out *EmbeddedAuthServerConfig) { *out = *in @@ -260,6 +275,11 @@ func (in *EmbeddedAuthServerConfig) DeepCopyInto(out *EmbeddedAuthServerConfig) *out = make([]string, len(*in)) copy(*out, *in) } + if in.CIMD != nil { + in, out := &in.CIMD, &out.CIMD + *out = new(EmbeddedAuthServerCIMDConfig) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EmbeddedAuthServerConfig. diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index 346c4d87ad..c6600a0378 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "strings" + "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -582,6 +583,22 @@ func BuildAuthServerRunConfig( } config.Storage = storageCfg + // Build CIMD configuration + if authConfig.CIMD != nil && authConfig.CIMD.Enabled { + cimdCfg := &authserver.CIMDRunConfig{ + Enabled: authConfig.CIMD.Enabled, + CacheMaxSize: authConfig.CIMD.CacheMaxSize, + } + if authConfig.CIMD.CacheFallbackTTL != "" { + ttl, err := time.ParseDuration(authConfig.CIMD.CacheFallbackTTL) + if err != nil { + return nil, fmt.Errorf("cimd.cacheFallbackTtl: %w", err) + } + cimdCfg.CacheFallbackTTL = ttl + } + config.CIMD = cimdCfg + } + return config, nil } diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index fcb0bd6379..a414180c42 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2645,3 +2646,127 @@ func TestValidateAndAddAuthServerRefOptions(t *testing.T) { }) } } + +// TestBuildAuthServerRunConfig_CIMD verifies that BuildAuthServerRunConfig +// correctly converts the CRD EmbeddedAuthServerCIMDConfig into +// authserver.CIMDRunConfig. The four cases cover the nil path (CIMD off +// by default), explicit values (fields are mapped and TTL is parsed), zero +// optional fields (authserver applies its own defaults at startup), and an +// invalid TTL string (returns a parse error). +func TestBuildAuthServerRunConfig_CIMD(t *testing.T) { + t.Parallel() + + // baseAuthConfig returns a minimal EmbeddedAuthServerConfig that is valid + // enough for BuildAuthServerRunConfig to proceed past signing-key and + // upstream validation without requiring real secrets. + baseAuthConfig := func(cimd *mcpv1beta1.EmbeddedAuthServerCIMDConfig) *mcpv1beta1.EmbeddedAuthServerConfig { + return &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + CIMD: cimd, + } + } + + defaultAudiences := []string{"https://mcp.example.com"} + defaultScopes := []string{"openid", "offline_access"} + + tests := []struct { + name string + cimd *mcpv1beta1.EmbeddedAuthServerCIMDConfig + wantCIMD bool + wantErr bool + errContains string + checkFunc func(t *testing.T, got *authserver.CIMDRunConfig) + }{ + { + name: "nil CIMD leaves config.CIMD nil", + cimd: nil, + wantCIMD: false, + }, + { + name: "CIMD disabled leaves config.CIMD nil", + cimd: &mcpv1beta1.EmbeddedAuthServerCIMDConfig{ + Enabled: false, + CacheMaxSize: 100, + CacheFallbackTTL: "10m", + }, + wantCIMD: false, + }, + { + name: "CIMD enabled with explicit values maps all fields", + cimd: &mcpv1beta1.EmbeddedAuthServerCIMDConfig{ + Enabled: true, + CacheMaxSize: 512, + CacheFallbackTTL: "10m", + }, + wantCIMD: true, + checkFunc: func(t *testing.T, got *authserver.CIMDRunConfig) { + t.Helper() + assert.True(t, got.Enabled) + assert.Equal(t, 512, got.CacheMaxSize) + assert.Equal(t, 10*time.Minute, got.CacheFallbackTTL) + }, + }, + { + name: "CIMD enabled with zero optional fields leaves defaults to authserver", + cimd: &mcpv1beta1.EmbeddedAuthServerCIMDConfig{ + Enabled: true, + }, + wantCIMD: true, + checkFunc: func(t *testing.T, got *authserver.CIMDRunConfig) { + t.Helper() + assert.True(t, got.Enabled) + assert.Zero(t, got.CacheMaxSize, "zero means authserver applies its own default at startup") + assert.Zero(t, got.CacheFallbackTTL, "zero means authserver applies its own default at startup") + }, + }, + { + name: "invalid CacheFallbackTTL returns parse error", + cimd: &mcpv1beta1.EmbeddedAuthServerCIMDConfig{ + Enabled: true, + CacheFallbackTTL: "not-a-duration", + }, + wantErr: true, + errContains: "cimd.cacheFallbackTtl", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cfg, err := BuildAuthServerRunConfig( + "default", "test-server", + baseAuthConfig(tt.cimd), + defaultAudiences, defaultScopes, + "https://mcp.example.com", + ) + + if tt.wantErr { + require.Error(t, err) + if tt.errContains != "" { + assert.Contains(t, err.Error(), tt.errContains) + } + return + } + + require.NoError(t, err) + require.NotNil(t, cfg) + + if !tt.wantCIMD { + assert.Nil(t, cfg.CIMD, "expected config.CIMD to be nil") + return + } + + require.NotNil(t, cfg.CIMD, "expected config.CIMD to be set") + if tt.checkFunc != nil { + tt.checkFunc(t, cfg.CIMD) + } + }) + } +} 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 61b2c43c46..4218a7ffe0 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 @@ -237,6 +237,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -1516,6 +1543,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 4f639ef1ee..887443f812 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -110,6 +110,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -3034,6 +3061,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index d657a82b07..43edce708f 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -240,6 +240,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -1519,6 +1546,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index cedf9c2334..baba421153 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -113,6 +113,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing @@ -3037,6 +3064,33 @@ spec: maxItems: 10 type: array x-kubernetes-list-type: atomic + cimd: + description: CIMD configures Client ID Metadata Document support. + When omitted, CIMD is disabled. + properties: + cacheFallbackTtl: + description: |- + CacheFallbackTTL is the fixed TTL applied to every cached CIMD document. + Cache-Control header parsing is not yet implemented; all entries use this value. + Format: Go duration string (e.g. "5m", "10m", "1h"). + Defaults to 5 minutes when Enabled is true and this field is omitted. + pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ + type: string + cacheMaxSize: + description: |- + CacheMaxSize is the maximum number of CIMD documents held in the LRU cache. + Defaults to 256 when Enabled is true and this field is omitted. + minimum: 1 + type: integer + enabled: + default: false + description: |- + Enabled activates CIMD client lookup. When false (the default), the AS only + accepts client_id values that were registered via DCR. + type: boolean + required: + - enabled + type: object hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index 43bc2c2116..e83be618fc 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -129,10 +129,6 @@ func (c *RunConfig) validateBaselineClientScopes() error { } // CIMDRunConfig controls client_id metadata document (CIMD) support. -// -// TODO(cimd): expose these fields in the MCPExternalAuthConfig CRD so Kubernetes -// operators can configure CIMD through the normal CRD workflow instead of -// writing RunConfig YAML directly. type CIMDRunConfig struct { // Enabled activates CIMD client lookup when true. Enabled bool `json:"enabled" yaml:"enabled"` From 4f3e1cb6a6583dc6aef7ea0171aa757382c41643 Mon Sep 17 00:00:00 2001 From: amirejaz Date: Tue, 26 May 2026 22:40:47 +0500 Subject: [PATCH 3/3] Update PR5 converter for CacheFallbackTTL string type CIMDRunConfig.CacheFallbackTTL changed from time.Duration to string in PR3. The operator converter now passes the string through unchanged; parsing to time.Duration happens in resolveCIMDConfig in the runner, after CIMDRunConfig.Validate() has already confirmed the format. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../pkg/controllerutil/authserver.go | 19 ++++++------------- .../pkg/controllerutil/authserver_test.go | 14 +++++++++----- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index c6600a0378..84d4ed979d 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "strings" - "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -583,20 +582,14 @@ func BuildAuthServerRunConfig( } config.Storage = storageCfg - // Build CIMD configuration + // Build CIMD configuration. CacheFallbackTTL is passed as-is (string); + // resolveCIMDConfig in the runner parses it to time.Duration at startup. if authConfig.CIMD != nil && authConfig.CIMD.Enabled { - cimdCfg := &authserver.CIMDRunConfig{ - Enabled: authConfig.CIMD.Enabled, - CacheMaxSize: authConfig.CIMD.CacheMaxSize, + config.CIMD = &authserver.CIMDRunConfig{ + Enabled: authConfig.CIMD.Enabled, + CacheMaxSize: authConfig.CIMD.CacheMaxSize, + CacheFallbackTTL: authConfig.CIMD.CacheFallbackTTL, } - if authConfig.CIMD.CacheFallbackTTL != "" { - ttl, err := time.ParseDuration(authConfig.CIMD.CacheFallbackTTL) - if err != nil { - return nil, fmt.Errorf("cimd.cacheFallbackTtl: %w", err) - } - cimdCfg.CacheFallbackTTL = ttl - } - config.CIMD = cimdCfg } return config, nil diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index a414180c42..bec4c98179 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -8,7 +8,6 @@ import ( "fmt" "strings" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2709,7 +2708,7 @@ func TestBuildAuthServerRunConfig_CIMD(t *testing.T) { t.Helper() assert.True(t, got.Enabled) assert.Equal(t, 512, got.CacheMaxSize) - assert.Equal(t, 10*time.Minute, got.CacheFallbackTTL) + assert.Equal(t, "10m", got.CacheFallbackTTL) }, }, { @@ -2726,13 +2725,18 @@ func TestBuildAuthServerRunConfig_CIMD(t *testing.T) { }, }, { - name: "invalid CacheFallbackTTL returns parse error", + name: "invalid CacheFallbackTTL passes through to runner for validation", cimd: &mcpv1beta1.EmbeddedAuthServerCIMDConfig{ Enabled: true, CacheFallbackTTL: "not-a-duration", }, - wantErr: true, - errContains: "cimd.cacheFallbackTtl", + wantCIMD: true, + checkFunc: func(t *testing.T, got *authserver.CIMDRunConfig) { + t.Helper() + // The converter passes the string through; parse errors are caught + // by CIMDRunConfig.Validate() or resolveCIMDConfig in the runner. + assert.Equal(t, "not-a-duration", got.CacheFallbackTTL) + }, }, }