From 893a7ea251df0184160cad970a3175771858b428 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 18:26:59 +0100 Subject: [PATCH] R80: req secret for introspection, pool conn on panic --- internal/cache/backends/redis.go | 11 +++-- internal/cache/backends/review_r80_test.go | 48 ++++++++++++++++++++++ review_r80_test.go | 43 +++++++++++++++++++ settings.go | 9 ++++ 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 internal/cache/backends/review_r80_test.go create mode 100644 review_r80_test.go diff --git a/internal/cache/backends/redis.go b/internal/cache/backends/redis.go index a27f813..e906efc 100644 --- a/internal/cache/backends/redis.go +++ b/internal/cache/backends/redis.go @@ -376,9 +376,14 @@ func (r *RedisBackend) executeWithRetry(ctx context.Context, operation func(*Red } } - // Execute the operation - err = operation(conn) - r.pool.Put(conn) + // Execute the operation, guaranteeing the borrowed connection is + // returned to the pool even if the operation panics (previously a + // panic here leaked the connection and desynced the pool counters; + // other pool call sites use defer for this). + func() { + defer func() { r.pool.Put(conn) }() + err = operation(conn) + }() // Check context after operation - if canceled, don't bother retrying if ctx.Err() != nil { diff --git a/internal/cache/backends/review_r80_test.go b/internal/cache/backends/review_r80_test.go new file mode 100644 index 0000000..ca95ac9 --- /dev/null +++ b/internal/cache/backends/review_r80_test.go @@ -0,0 +1,48 @@ +package backends + +import ( + "context" + "net" + "testing" + "time" +) + +// TestExecuteWithRetry_ReturnsConnOnPanic is a regression: executeWithRetry +// returned the borrowed connection imperatively with no defer, so an operation +// panic leaked the Redis connection out of the pool and desynced its counters. +// The fix guarantees the connection is returned on all exit paths including +// panic. +func TestExecuteWithRetry_ReturnsConnOnPanic(t *testing.T) { + pool, err := NewConnectionPool(&PoolConfig{MaxConnections: 1, ConnectTimeout: 50 * time.Millisecond}) + if err != nil { + t.Fatalf("NewConnectionPool: %v", err) + } + + // Seed one available connection so executeWithRetry's Get succeeds without + // dialing (no Redis server needed). + pc, sw := net.Pipe() + defer sw.Close() + rc := &RedisConn{conn: pc, readTimeout: time.Second, writeTimeout: time.Second} + pool.Put(rc) + + if got := len(pool.connections); got != 1 { + t.Fatalf("setup: expected one seeded connection, got %d", got) + } + + r := &RedisBackend{pool: pool} + + func() { + defer func() { + if v := recover(); v == nil { + t.Fatalf("expected operation to panic") + } + }() + _ = r.executeWithRetry(context.Background(), func(c *RedisConn) error { + panic("boom") + }) + }() + + if got := len(pool.connections); got != 1 { + t.Fatalf("expected connection returned to pool after operation panic, got %d", got) + } +} diff --git a/review_r80_test.go b/review_r80_test.go new file mode 100644 index 0000000..1b12fe2 --- /dev/null +++ b/review_r80_test.go @@ -0,0 +1,43 @@ +package traefikoidc + +import ( + "strings" + "testing" +) + +// TestIntrospectionRequiresClientSecret is a regression: Config.Validate allowed +// requireTokenIntrospection together with clientAuthMethod=private_key_jwt and no +// client secret, but the introspection path always authenticates via +// client_secret_basic (SetBasicAuth in token_introspection.go) — so the dead +// config passed validation yet every introspection sent empty Basic credentials +// and failed. Validate must reject it. +func TestIntrospectionRequiresClientSecret(t *testing.T) { + mk := func() *Config { + c := CreateConfig() + c.ProviderURL = "https://provider.example.com" + c.ClientID = "test-client" + c.SessionEncryptionKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + c.CallbackURL = "/callback" + c.RequireTokenIntrospection = true + c.ClientAuthMethod = "private_key_jwt" + c.ClientAssertionPrivateKey = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg" + c.ClientAssertionKeyID = "key-id-1" + return c + } + + // OLD behavior: no client secret -> Validate still passed, introspection dead. + noSecret := mk() + noSecret.ClientSecret = "" + if err := noSecret.Validate(); err == nil { + t.Fatalf("expected Validate to require clientSecret when introspection is enabled") + } else if !strings.Contains(err.Error(), "clientSecret is required when requireTokenIntrospection") { + t.Fatalf("unexpected error: %v", err) + } + + // With a secret the config is valid. + withSecret := mk() + withSecret.ClientSecret = "secret" + if err := withSecret.Validate(); err != nil { + t.Fatalf("expected Validate to pass with a client secret, got: %v", err) + } +} diff --git a/settings.go b/settings.go index 0efb6aa..b5e42d3 100644 --- a/settings.go +++ b/settings.go @@ -459,6 +459,15 @@ func (c *Config) Validate() error { } } + // The token-introspection path always authenticates with client_secret_basic + // (token_introspection.go SetBasicAuth), regardless of clientAuthMethod. + // Reject the validated-but-broken config where introspection is enabled but + // no client secret is supplied (e.g. private_key_jwt with no secret), which + // would otherwise send empty Basic credentials and fail every introspection. + if c.RequireTokenIntrospection && c.ClientSecret == "" { + return fmt.Errorf("clientSecret is required when requireTokenIntrospection is enabled (introspection authenticates via client_secret_basic)") + } + // Validate session encryption key if c.SessionEncryptionKey == "" { return fmt.Errorf("sessionEncryptionKey is required")