diff --git a/dynamic_client_registration.go b/dynamic_client_registration.go index a5199c8..1457075 100644 --- a/dynamic_client_registration.go +++ b/dynamic_client_registration.go @@ -292,8 +292,10 @@ func (r *DynamicClientRegistrar) buildRegistrationRequest() ([]byte, error) { if metadata.TokenEndpointAuthMethod != "" { reqData["token_endpoint_auth_method"] = metadata.TokenEndpointAuthMethod } else { - // Default to client_secret_basic for confidential clients - reqData["token_endpoint_auth_method"] = "client_secret_basic" + // Default must match the plugin's runtime client-auth default + // (client_secret_post, settings.go Config.Validate) so an IdP that + // enforces the registered method does not reject the token exchange. + reqData["token_endpoint_auth_method"] = "client_secret_post" } if metadata.DefaultMaxAge > 0 { diff --git a/dynamic_client_registration_test.go b/dynamic_client_registration_test.go index 29c4b9a..24103ba 100644 --- a/dynamic_client_registration_test.go +++ b/dynamic_client_registration_test.go @@ -598,7 +598,7 @@ func TestBuildRegistrationRequest(t *testing.T) { "redirect_uris": []interface{}{"https://example.com/callback"}, "response_types": []interface{}{"code"}, "grant_types": []interface{}{"authorization_code", "refresh_token"}, - "token_endpoint_auth_method": "client_secret_basic", + "token_endpoint_auth_method": "client_secret_post", }, expectError: false, }, diff --git a/review_r36_regression_test.go b/review_r36_regression_test.go new file mode 100644 index 0000000..40f77ce --- /dev/null +++ b/review_r36_regression_test.go @@ -0,0 +1,175 @@ +package traefikoidc + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "sync/atomic" + "testing" + "time" +) + +// TestConfigValidate_DCRSkipsCredentialCheck covers the P1 fix: with Dynamic +// Client Registration (RFC 7591) enabled and no pre-provisioned client ID, +// Config.Validate must NOT reject the config for missing clientID/clientSecret +// (the provider provisions them at registration). Any other credential +// combination is still validated as before. +func TestConfigValidate_DCRSkipsCredentialCheck(t *testing.T) { + base := Config{ + ProviderURL: "https://issuer.example.com", + CallbackURL: "/callback", + SessionEncryptionKey: "0123456789abcdef0123456789abcdef", // >= MinSessionEncryptionKeyLength + RateLimit: 100, // >= MinRateLimit (10) + } + + // Control: without DCR, an empty clientID must still be rejected. + cfg := base + if err := cfg.Validate(); err == nil { + t.Fatal("expected 'clientID is required' error when DCR is disabled and clientID is empty") + } + + // P1 fix: with DCR enabled and empty clientID, validation must pass. + cfg = base + cfg.DynamicClientRegistration = &DynamicClientRegistrationConfig{Enabled: true} + if err := cfg.Validate(); err != nil { + t.Fatalf("expected validation to pass with DCR enabled and empty clientID, got: %v", err) + } + + // With DCR enabled but an explicit clientID, credential checks still apply: + // a missing clientSecret with the default method must still be rejected. + cfg = base + cfg.DynamicClientRegistration = &DynamicClientRegistrationConfig{Enabled: true} + cfg.ClientID = "explicit-client" + if err := cfg.Validate(); err == nil { + t.Fatal("expected 'clientSecret is required' error when clientID is supplied under DCR") + } +} + +// TestDCRBuildRegistrationRequestDefaultAuthMethod covers the P2 fix: the +// token_endpoint_auth_method registered with the provider must match the +// plugin's runtime client-auth default (client_secret_post, settings.go +// Config.Validate) so an IdP that enforces the registered method does not +// reject the plugin's token exchange. +func TestDCRBuildRegistrationRequestDefaultAuthMethod(t *testing.T) { + cfg := &DynamicClientRegistrationConfig{ + Enabled: true, + ClientMetadata: &ClientRegistrationMetadata{ + RedirectURIs: []string{"/callback"}, + }, + } + r := NewDynamicClientRegistrar(nil, nil, cfg, "https://issuer.example.com") + body, err := r.buildRegistrationRequest() + if err != nil { + t.Fatalf("buildRegistrationRequest failed: %v", err) + } + var data map[string]interface{} + if err := json.Unmarshal(body, &data); err != nil { + t.Fatalf("failed to parse registration body: %v", err) + } + got, _ := data["token_endpoint_auth_method"].(string) + if got != "client_secret_post" { + t.Errorf("expected default token_endpoint_auth_method to be client_secret_post, got %q", got) + } +} + +// TestVerifyTokenNoSelfReplayMarkAfterCacheEviction covers the P2 fix: on the +// cookie path the same still-valid token is re-presented on every request, so +// verifyTokenWithOpts must not self-record its own JTI into the per-instance +// tokenBlacklist. Doing so turns an LRU eviction of the raw-token cache +// (MaxSize 1000 / 5 MiB) into a false "token replay detected" for a valid, +// unexpired session. Cross-path replay detection (shardedReplayCache) and +// re-presentation after a cache eviction must both keep working. +func TestVerifyTokenNoSelfReplayMarkAfterCacheEviction(t *testing.T) { + ts := NewTestSuite(t) + ts.Setup() + cleanupReplayCache() + initReplayCache() + + jti := generateRandomString(16) + now := time.Now() + token, err := createTestJWT(ts.rsaPrivateKey, "RS256", "test-key-id", map[string]interface{}{ + "iss": "https://test-issuer.com", + "aud": "test-client-id", + "exp": now.Add(1 * time.Hour).Unix(), + "iat": now.Unix(), + "nbf": now.Unix(), + "sub": "test-subject", + "email": "user@example.com", + "nonce": "test-nonce", + "jti": jti, + }) + if err != nil { + t.Fatalf("failed to create test JWT: %v", err) + } + + // First verification succeeds and records the JTI in the shared + // replay cache for cross-path replay detection. + if err := ts.tOidc.VerifyToken(token); err != nil { + t.Fatalf("first verification should succeed: %v", err) + } + if !shardedReplayCache.Exists(jti) { + t.Error("JTI should be recorded in the shared shardedReplayCache for cross-path replay detection") + } + // The per-instance tokenBlacklist must NOT be self-marked (this was the + // false-positive source after raw-token-cache eviction). + if blacklisted, exists := ts.tOidc.tokenBlacklist.Get(jti); exists && blacklisted != nil { + t.Error("per-instance tokenBlacklist must not self-record the JTI (would cause false replay after cache eviction)") + } + + // Simulate an LRU eviction of the raw-token cache entry, then re-present + // the same still-valid token: it must be accepted, not flagged as replay. + ts.tOidc.tokenCache.Delete(token) + if err := ts.tOidc.VerifyToken(token); err != nil { + t.Errorf("re-presentation after raw-token-cache eviction should succeed, got: %v", err) + } +} + +// TestMetadataRefreshSharedTaskNotStoppedOnFirstClose covers the GlueLifecycle +// P2 fix: the per-provider singleton metadata-refresh task (name derived from +// providerURL only) is shared by every live instance pointing at that +// provider. Close must gate stopping it on the LAST live instance (matching +// singleton-token-cleanup), otherwise one instance's teardown (e.g. a config +// reload) would kill 2h metadata refresh for a surviving sibling that never +// re-registers. +func TestMetadataRefreshSharedTaskNotStoppedOnFirstClose(t *testing.T) { + // The live-instance counter is a shared package global. Pin it so this + // test's relative count is deterministic regardless of shuffle order, and + // restore it afterwards. + prev := atomic.LoadInt32(&liveInstanceCount) + defer atomic.StoreInt32(&liveInstanceCount, prev) + atomic.StoreInt32(&liveInstanceCount, 0) + + rm := GetResourceManager() + providerURL := "https://issuer.example.com" + hash := sha256.Sum256([]byte(providerURL)) + taskName := "singleton-metadata-refresh-" + hex.EncodeToString(hash[:])[0:6] + + // Clear any leftover task from prior tests and register a fresh one. + _ = rm.StopBackgroundTask(taskName) + if err := rm.RegisterBackgroundTask(taskName, time.Hour, func() {}); err != nil { + t.Fatalf("failed to register metadata task: %v", err) + } + if err := rm.StartBackgroundTask(taskName); err != nil { + t.Fatalf("failed to start metadata task: %v", err) + } + + // Two live instances share this provider (same task name). + registerLiveInstance() + registerLiveInstance() + + first := &TraefikOidc{name: "meter-first", providerURL: providerURL} + if err := first.Close(); err != nil { + t.Fatalf("first Close failed: %v", err) + } + if !rm.IsTaskRunning(taskName) { + t.Fatal("metadata-refresh task must survive the first (non-last) instance Close") + } + + second := &TraefikOidc{name: "meter-second", providerURL: providerURL} + if err := second.Close(); err != nil { + t.Fatalf("second Close failed: %v", err) + } + if rm.IsTaskRunning(taskName) { + t.Error("metadata-refresh task should be stopped once the last instance closes") + } +} diff --git a/security_edge_cases_test.go b/security_edge_cases_test.go index ccc78e9..d0dcbd9 100644 --- a/security_edge_cases_test.go +++ b/security_edge_cases_test.go @@ -359,29 +359,26 @@ func TestJWTReplayAttack(t *testing.T) { t.Fatalf("First verification of token failed unexpectedly: %v", err) } - // Verify that the JTI was blacklisted - if blacklisted, exists := tOidc.tokenBlacklist.Get(fixedJTI); !exists || blacklisted == nil { - t.Fatalf("JTI was not added to blacklist after first verification") + // Replay tracking for this path lives in the shared shardedReplayCache + // (see verifyTokenWithOpts); the per-instance tokenBlacklist holds only + // external revocations and is intentionally NOT self-marked. + if !shardedReplayCache.Exists(fixedJTI) { + t.Fatalf("JTI was not recorded in the shared replay cache after first verification") + } + if blacklisted, exists := tOidc.tokenBlacklist.Get(fixedJTI); exists && blacklisted != nil { + t.Errorf("per-instance tokenBlacklist must not self-record the JTI (would cause false replay after cache eviction)") } - // Since there's a special bypass for tokens starting with the test JWT prefix, - // we need to test with a direct check of the blacklisted JTI instead - - // Directly verify that a replay would be caught by checking the blacklist - if blacklisted, exists := tOidc.tokenBlacklist.Get(fixedJTI); !exists || blacklisted == nil { - t.Errorf("JTI was not properly blacklisted for replay protection") + // Real replay detection still works: presenting the same token through the + // replay-checking path must be flagged as a replay. + replayed, err := parseJWT(replayJWT) + if err != nil { + t.Fatalf("failed to parse replayed JWT: %v", err) } - - // Also verify our JTI replay detection function directly - claims, _ := extractClaims(replayJWT) - if claims != nil { - if jti, ok := claims["jti"].(string); ok && jti != "" { - if blacklisted, exists := tOidc.tokenBlacklist.Get(jti); exists && blacklisted != nil { - t.Logf("Replay protection verified: JTI %s is correctly blacklisted", jti) - } else { - t.Errorf("JTI %s was not found in blacklist", jti) - } - } + if err := replayed.Verify("https://test-issuer.com", "test-client-id", false); err == nil { + t.Error("expected replay to be detected via the shared replay cache") + } else if !strings.Contains(err.Error(), "token replay detected") { + t.Errorf("expected 'token replay detected', got: %v", err) } } diff --git a/settings.go b/settings.go index a0a3776..6322d9d 100644 --- a/settings.go +++ b/settings.go @@ -422,34 +422,41 @@ func (c *Config) Validate() error { return fmt.Errorf("callbackURL must start with /") } - // Validate client credentials - if c.ClientID == "" { - return fmt.Errorf("clientID is required") - } - authMethod := c.ClientAuthMethod - if authMethod == "" { - authMethod = "client_secret_post" - } - switch authMethod { - case "client_secret_post", "client_secret_basic": - if c.ClientSecret == "" { - return fmt.Errorf("clientSecret is required when clientAuthMethod is %q", authMethod) + // Validate client credentials. With Dynamic Client Registration (RFC 7591) + // enabled, clientID (and typically clientSecret) are not known at + // construction time — the provider provisions them at registration. Skip + // these checks in that case; the providerURL / callbackURL / session-key + // checks above and below still run. + dcrEnabled := c.DynamicClientRegistration != nil && c.DynamicClientRegistration.Enabled + if !(dcrEnabled && c.ClientID == "") { + if c.ClientID == "" { + return fmt.Errorf("clientID is required") } - case "private_key_jwt": - if c.ClientAssertionPrivateKey == "" && c.ClientAssertionKeyPath == "" { - return fmt.Errorf("clientAssertionPrivateKey or clientAssertionKeyPath is required when clientAuthMethod is private_key_jwt") + authMethod := c.ClientAuthMethod + if authMethod == "" { + authMethod = "client_secret_post" } - if c.ClientAssertionPrivateKey != "" && c.ClientAssertionKeyPath != "" { - return fmt.Errorf("only one of clientAssertionPrivateKey or clientAssertionKeyPath may be set") + switch authMethod { + case "client_secret_post", "client_secret_basic": + if c.ClientSecret == "" { + return fmt.Errorf("clientSecret is required when clientAuthMethod is %q", authMethod) + } + case "private_key_jwt": + if c.ClientAssertionPrivateKey == "" && c.ClientAssertionKeyPath == "" { + return fmt.Errorf("clientAssertionPrivateKey or clientAssertionKeyPath is required when clientAuthMethod is private_key_jwt") + } + if c.ClientAssertionPrivateKey != "" && c.ClientAssertionKeyPath != "" { + return fmt.Errorf("only one of clientAssertionPrivateKey or clientAssertionKeyPath may be set") + } + if c.ClientAssertionKeyID == "" { + return fmt.Errorf("clientAssertionKeyID is required when clientAuthMethod is private_key_jwt") + } + if c.ClientAssertionAlg != "" && !isSupportedClientAssertionAlg(c.ClientAssertionAlg) { + return fmt.Errorf("clientAssertionAlg %q is not supported (use RS256/384/512, PS256/384/512, or ES256/384/512)", c.ClientAssertionAlg) + } + default: + return fmt.Errorf("clientAuthMethod %q is not supported", authMethod) } - if c.ClientAssertionKeyID == "" { - return fmt.Errorf("clientAssertionKeyID is required when clientAuthMethod is private_key_jwt") - } - if c.ClientAssertionAlg != "" && !isSupportedClientAssertionAlg(c.ClientAssertionAlg) { - return fmt.Errorf("clientAssertionAlg %q is not supported (use RS256/384/512, PS256/384/512, or ES256/384/512)", c.ClientAssertionAlg) - } - default: - return fmt.Errorf("clientAuthMethod %q is not supported", authMethod) } // Validate session encryption key diff --git a/token_manager.go b/token_manager.go index 7345745..07eceef 100644 --- a/token_manager.go +++ b/token_manager.go @@ -130,11 +130,12 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { t.cacheVerifiedToken(token, jwt.Claims) - // Replay marking: add JTI to blacklist so subsequent presentations of - // the SAME token can short-circuit via cache. Bearer path suppresses - // this Set (opts.skipReplayMarking=true) because bearer tokens are - // designed for reuse until exp; the cache-evict-then-replay scenario - // would otherwise trigger false replay detection. + // Replay marking: record the JTI in the shared shardedReplayCache so + // cross-path replay detection (jwt.Verify) works. The bearer path + // suppresses this entirely (opts.skipReplayMarking=true) because bearer + // tokens are designed for reuse until exp; the cache-evict-then-replay + // scenario would otherwise trigger false replay detection. See the note + // below on why t.tokenBlacklist is deliberately not used. if jti, ok := jwt.Claims["jti"].(string); ok && jti != "" && !t.disableReplayDetection && !opts.skipReplayMarking { expiry := time.Now().Add(defaultBlacklistDuration) if expClaim, expOk := jwt.Claims["exp"].(float64); expOk { @@ -146,15 +147,18 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { // else: keep default expiry for expired tokens or tokens >24h } - if t.tokenBlacklist != nil { - t.tokenBlacklist.Set(jti, true, time.Until(expiry)) - t.safeLogDebugf("Added JTI %s to blacklist cache", jti) - } else { - t.safeLogErrorf("Token blacklist not available, skipping JTI %s blacklist", jti) - } - - // Use sharded cache for replay detection - no global mutex needed - // This reduces lock contention by ~64x under high load + // Deliberately do NOT add the JTI to the per-instance + // t.tokenBlacklist here. That store is queried on every + // cache-miss re-presentation (the JTI Get above), and on the + // cookie path the SAME still-valid token is re-presented on every + // request for the session. Self-marking it would mean an LRU + // eviction of the raw-token cache (MaxSize 1000 / 5 MiB) turns a + // cache miss on a legitimate token into a false "token replay + // detected" (token_manager.go line ~110), forcing a valid session + // to re-authenticate. t.tokenBlacklist therefore holds only + // EXTERNAL revocations (RevokeToken). Replay tracking lives in the + // shared shardedReplayCache below, which is the store jwt.Verify + // consults for genuine replay detection. initReplayCache() duration := time.Until(expiry) if duration > 0 { diff --git a/utilities.go b/utilities.go index 85bef8d..6e87d8e 100644 --- a/utilities.go +++ b/utilities.go @@ -244,8 +244,13 @@ func (t *TraefikOidc) Close() error { if lastInstance { _ = rm.StopBackgroundTask("singleton-token-cleanup") // best effort, last instance only } - // Stop metadata refresh task using same hash-based name as startMetadataRefresh - if t.providerURL != "" { + // Stop metadata refresh task using same hash-based name as + // startMetadataRefresh. The name derives only from providerURL + // (main.go), so it is SHARED by every live instance pointing at the + // same provider; stopping it on one instance's Close would kill 2h + // metadata refresh for its surviving sibling (which never re-registers). + // Gate on lastInstance, matching singleton-token-cleanup above. + if lastInstance && t.providerURL != "" { hash := sha256.Sum256([]byte(t.providerURL)) taskName := "singleton-metadata-refresh-" + hex.EncodeToString(hash[:])[0:6] _ = rm.StopBackgroundTask(taskName) // Safe to ignore: best effort cleanup