Skip to content

Commit 76fd736

Browse files
amirejazclaude
andcommitted
Address tgrunnagle feedback on CIMD wiring
C1 - Fix CacheFallbackTTL to string in CIMDRunConfig so YAML/JSON operators can write "5m" instead of nanoseconds. resolveCIMDConfig parses it with time.ParseDuration at runtime. C6 - Add slog.Debug when applyDefaults substitutes CIMD defaults, matching the pattern used by the sibling token-lifespan defaults. C7 - Add CIMDRunConfig.Validate() and call it from RunConfig.Validate() so CIMD misconfiguration fails at the wire-format boundary before the runner resolves secrets. C10 - Add test cases to TestConfigValidate and TestRunConfigValidate covering all new CIMD validation paths. C11 - Add TestNewServer_CIMDEnabled_WrapsStorage: verifies the CIMD decorator is actually installed in the storage chain when CIMDEnabled=true. C12 - Add comment in discovery.go explaining why client_id_metadata_document_supported is in OIDC discovery. C13 - Add doc comment on server.storage noting it may be decorated. C14 - Add note to IsClientIDMetadataDocumentURL about pkg/auth/remote callers and why the loopback widening is safe there. Regenerate swagger docs: CacheFallbackTTL now serialises as string. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 49def68 commit 76fd736

11 files changed

Lines changed: 111 additions & 8 deletions

File tree

docs/server/docs.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/server/swagger.yaml

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/authserver/config.go

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,11 @@ type RunConfig struct {
102102
// catches operator-supplied misconfiguration early so server startup fails
103103
// loudly instead of degrading silently at runtime.
104104
func (c *RunConfig) Validate() error {
105+
if c.CIMD != nil {
106+
if err := c.CIMD.Validate(); err != nil {
107+
return fmt.Errorf("cimd: %w", err)
108+
}
109+
}
105110
return c.validateBaselineClientScopes()
106111
}
107112

@@ -138,9 +143,29 @@ type CIMDRunConfig struct {
138143

139144
// CacheFallbackTTL is the fixed TTL applied to every cached CIMD document.
140145
// Cache-Control header parsing is not yet implemented; all entries use this value.
141-
// Defaults to 5 minutes when Enabled is true and this field is zero.
142-
//nolint:lll // field tags require full JSON+YAML names
143-
CacheFallbackTTL time.Duration `json:"cache_fallback_ttl,omitempty" yaml:"cache_fallback_ttl,omitempty" swaggertype:"string" example:"5m"`
146+
// Format: Go duration string (e.g. "5m", "10m", "1h").
147+
// Defaults to 5 minutes when Enabled is true and this field is omitted.
148+
CacheFallbackTTL string `json:"cache_fallback_ttl,omitempty" yaml:"cache_fallback_ttl,omitempty" example:"5m"`
149+
}
150+
151+
// Validate checks that the CIMDRunConfig fields are internally consistent.
152+
func (c *CIMDRunConfig) Validate() error {
153+
if !c.Enabled {
154+
return nil
155+
}
156+
if c.CacheMaxSize < 0 {
157+
return fmt.Errorf("cache_max_size must be non-negative when CIMD is enabled, got %d", c.CacheMaxSize)
158+
}
159+
if c.CacheFallbackTTL != "" {
160+
d, err := time.ParseDuration(c.CacheFallbackTTL)
161+
if err != nil {
162+
return fmt.Errorf("cache_fallback_ttl: %w", err)
163+
}
164+
if d < 0 {
165+
return fmt.Errorf("cache_fallback_ttl must be non-negative when CIMD is enabled, got %s", c.CacheFallbackTTL)
166+
}
167+
}
168+
return nil
144169
}
145170

146171
// SigningKeyRunConfig configures where to load signing keys from.
@@ -867,9 +892,11 @@ func (c *Config) applyDefaults() error {
867892
}
868893
if c.CIMDEnabled && c.CIMDCacheMaxSize == 0 {
869894
c.CIMDCacheMaxSize = 256
895+
slog.Debug("applied default cimd cache_max_size", "size", c.CIMDCacheMaxSize)
870896
}
871897
if c.CIMDEnabled && c.CIMDCacheFallbackTTL == 0 {
872898
c.CIMDCacheFallbackTTL = 5 * time.Minute
899+
slog.Debug("applied default cimd cache_fallback_ttl", "ttl", c.CIMDCacheFallbackTTL)
873900
}
874901
return nil
875902
}

pkg/authserver/config_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ func TestConfigValidate(t *testing.T) {
114114
{name: "baseline scope not in scopes_supported", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, ScopesSupported: []string{"openid"}, BaselineClientScopes: []string{"offline_access"}}, wantErr: true, errMsg: `baseline_client_scopes contains "offline_access"`},
115115
{name: "nil supported with baseline in DefaultScopes passes", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, ScopesSupported: nil, BaselineClientScopes: []string{"offline_access"}}},
116116

117+
// CIMD validation
118+
{name: "CIMD enabled zero cache_max_size rejected", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, CIMDEnabled: true, CIMDCacheMaxSize: 0}, wantErr: true, errMsg: "cache_max_size must be >= 1"},
119+
{name: "CIMD enabled negative cache_max_size rejected", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, CIMDEnabled: true, CIMDCacheMaxSize: -1}, wantErr: true, errMsg: "cache_max_size must be >= 1"},
120+
{name: "CIMD enabled negative cache_fallback_ttl rejected", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, CIMDEnabled: true, CIMDCacheMaxSize: 256, CIMDCacheFallbackTTL: -time.Second}, wantErr: true, errMsg: "cache_fallback_ttl must be non-negative"},
121+
{name: "CIMD disabled ignores invalid cache fields", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, CIMDEnabled: false, CIMDCacheMaxSize: -1, CIMDCacheFallbackTTL: -time.Second}},
122+
{name: "CIMD enabled with valid bounds passes", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}, CIMDEnabled: true, CIMDCacheMaxSize: 256, CIMDCacheFallbackTTL: 5 * time.Minute}},
123+
117124
// Valid configs
118125
{name: "valid minimal", config: Config{Issuer: "https://example.com", KeyProvider: validKeyProvider, HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}}},
119126
{name: "valid nil key provider", config: Config{Issuer: "https://example.com", HMACSecrets: validHMAC, Upstreams: validUpstreams, AllowedAudiences: []string{"https://mcp.example.com"}}},
@@ -431,6 +438,14 @@ func TestRunConfigValidate(t *testing.T) {
431438
wantErr: true,
432439
errMsg: "foo",
433440
},
441+
// CIMD RunConfig validation
442+
{name: "CIMD nil passes", config: RunConfig{CIMD: nil}},
443+
{name: "CIMD disabled passes even with invalid fields", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: false, CacheMaxSize: -1, CacheFallbackTTL: "-1s"}}},
444+
{name: "CIMD enabled negative cache_max_size rejected", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: true, CacheMaxSize: -1}}, wantErr: true, errMsg: "cache_max_size"},
445+
{name: "CIMD enabled invalid TTL string rejected", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: true, CacheFallbackTTL: "not-a-duration"}}, wantErr: true, errMsg: "cache_fallback_ttl"},
446+
{name: "CIMD enabled negative TTL rejected", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: true, CacheFallbackTTL: "-5m"}}, wantErr: true, errMsg: "cache_fallback_ttl"},
447+
{name: "CIMD enabled valid passes", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: true, CacheMaxSize: 64, CacheFallbackTTL: "5m"}}},
448+
{name: "CIMD enabled omitted optional fields pass", config: RunConfig{CIMD: &CIMDRunConfig{Enabled: true}}},
434449
}
435450

436451
for _, tt := range tests {

pkg/authserver/runner/embeddedauthserver.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -789,11 +789,23 @@ func convertRedisTLSRunConfig(rc *storage.RedisTLSRunConfig) (*tcredis.TLSConfig
789789

790790
// resolveCIMDConfig extracts CIMD settings from a CIMDRunConfig.
791791
// Returns zero values when cfg is nil (CIMD disabled).
792+
// The CacheFallbackTTL string is parsed to time.Duration; callers must ensure
793+
// CIMDRunConfig.Validate() has already been called so the string is well-formed.
792794
func resolveCIMDConfig(cfg *authserver.CIMDRunConfig) (enabled bool, cacheMaxSize int, cacheFallbackTTL time.Duration) {
793795
if cfg == nil {
794796
return false, 0, 0
795797
}
796-
return cfg.Enabled, cfg.CacheMaxSize, cfg.CacheFallbackTTL
798+
var ttl time.Duration
799+
if cfg.CacheFallbackTTL != "" {
800+
var err error
801+
ttl, err = time.ParseDuration(cfg.CacheFallbackTTL)
802+
if err != nil {
803+
// Should not happen when called after CIMDRunConfig.Validate().
804+
slog.Warn("invalid cimd cache_fallback_ttl, zero will be replaced by default",
805+
"value", cfg.CacheFallbackTTL, "err", err)
806+
}
807+
}
808+
return cfg.Enabled, cfg.CacheMaxSize, ttl
797809
}
798810

799811
// resolveEnvVar reads a value from the named environment variable.

pkg/authserver/runner/embeddedauthserver_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2052,7 +2052,7 @@ func TestResolveCIMDConfig(t *testing.T) {
20522052
cfg := &authserver.CIMDRunConfig{
20532053
Enabled: true,
20542054
CacheMaxSize: 128,
2055-
CacheFallbackTTL: 10 * time.Minute,
2055+
CacheFallbackTTL: "10m",
20562056
}
20572057
enabled, size, ttl := resolveCIMDConfig(cfg)
20582058
assert.True(t, enabled)

pkg/authserver/server/handlers/discovery.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,11 @@ func (h *Handler) buildOAuthMetadata() sharedobauth.AuthorizationServerMetadata
115115
CodeChallengeMethodsSupported: []string{crypto.PKCEChallengeMethodS256},
116116
TokenEndpointAuthMethodsSupported: []string{sharedobauth.TokenEndpointAuthMethodNone},
117117

118+
// ClientIDMetadataDocumentSupported is defined in the CIMD draft as an
119+
// OAuth AS metadata field (RFC 8414), not in OIDC Discovery 1.0. It is
120+
// included here because MCP clients (e.g. VS Code) discover the AS via
121+
// /.well-known/openid-configuration and need this flag there to activate
122+
// CIMD. Spec-compliant OIDC consumers silently ignore unknown fields.
118123
ClientIDMetadataDocumentSupported: h.config.CIMDEnabled,
119124
}
120125
}

pkg/authserver/server_impl.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ import (
2323
// server is the internal implementation of the Server interface.
2424
type server struct {
2525
handler http.Handler
26+
// storage is the active storage backend, potentially wrapped by decorators
27+
// such as CIMDStorageDecorator. Code that needs the concrete type must walk
28+
// the Unwrap() chain rather than asserting directly.
2629
storage storage.Storage
2730
// dcrStore is the same storage.Storage value asserted to
2831
// storage.DCRCredentialStore. The assertion runs once at construction

pkg/authserver/server_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"crypto/rand"
99
"strings"
1010
"testing"
11+
"time"
1112

1213
"go.uber.org/mock/gomock"
1314

@@ -188,3 +189,37 @@ func TestNewServer_Success(t *testing.T) {
188189
t.Error("server.IDPTokenStorage() did not return expected storage")
189190
}
190191
}
192+
193+
func TestNewServer_CIMDEnabled_WrapsStorage(t *testing.T) {
194+
t.Parallel()
195+
196+
mockUpstream := upstreammocks.NewMockOAuth2Provider(gomock.NewController(t))
197+
198+
stor := storage.NewMemoryStorage()
199+
t.Cleanup(func() { _ = stor.Close() })
200+
201+
cfg := Config{
202+
Issuer: "https://example.com",
203+
KeyProvider: keys.NewGeneratingProvider(keys.DefaultAlgorithm),
204+
HMACSecrets: &servercrypto.HMACSecrets{Current: validHMACSecret()},
205+
Upstreams: []UpstreamConfig{{Name: "default", Type: UpstreamProviderTypeOAuth2, OAuth2Config: validUpstreamConfig()}},
206+
AllowedAudiences: []string{"https://mcp.example.com"},
207+
CIMDEnabled: true,
208+
CIMDCacheMaxSize: 16,
209+
CIMDCacheFallbackTTL: 5 * time.Minute,
210+
}
211+
212+
mockFactory := func(_ context.Context, _ *UpstreamConfig) (upstream.OAuth2Provider, error) {
213+
return mockUpstream, nil
214+
}
215+
216+
srv, err := newServer(context.Background(), cfg, stor, withUpstreamFactory(mockFactory))
217+
if err != nil {
218+
t.Fatalf("newServer() unexpected error: %v", err)
219+
}
220+
221+
_, ok := srv.storage.(*storage.CIMDStorageDecorator)
222+
if !ok {
223+
t.Errorf("expected storage to be *storage.CIMDStorageDecorator when CIMDEnabled=true, got %T", srv.storage)
224+
}
225+
}

0 commit comments

Comments
 (0)