Fix round-36 issues: DCR validation, auth-method default, shared metadata task, replay false-positive

Four verified fixes from the round-36 agent team (each confirmed against source
before acting):

- settings.go: Config.Validate now skips the client-credential block when
  Dynamic Client Registration is enabled and no clientID is pre-provisioned
  (RFC 7591 provides credentials at registration). Previously the
  unconditional 'clientID is required' made DCR unreachable via New.
- dynamic_client_registration.go: default registered token_endpoint_auth_method
  to client_secret_post to match the plugin's runtime default; an enforcing
  IdP would otherwise reject the plugin's token exchange.
- utilities.go: gate the shared per-provider singleton-metadata-refresh task
  stop on lastInstance (matching singleton-token-cleanup) so one instance's
  tear down no longer kills 2h metadata refresh for a surviving sibling.
- token_manager.go: stop self-recording the JTI into the per-instance
  tokenBlacklist on every verify. On the cookie path the same valid token is
  re-presented per request; an LRU eviction of the raw-token cache (MaxSize
  1000 / 5 MiB) then turned a legitimate cache miss into a false 'token
  replay detected'. Replay tracking stays in the shared shardedReplayCache.

Regression tests: new review_r36_regression_test.go (4 tests); updated
dynamic_client_registration_test.go and security_edge_cases_test.go which had
encoded the old defaults.

Verified: full source-order suite, full -race -count=1 -shuffle=on ./...
(all packages), and golangci-lint run ./... (0 issues).

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 {

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,
 		},

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": "[email protected]",
+		"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")
+	}
+}

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)
 	}
 }
 

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

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 {

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