From 73f6451624914e25f85ed1c3e6fa50338708ac77 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Mon, 18 May 2026 16:07:15 -0400 Subject: [PATCH 1/2] Bump toolhive-core to v0.0.21 and use shared redis client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auth-server's storage package carried its own copy of the Redis connection layer (config types, TLS dialer, per-mode client construction, topology validation, timeout defaults). toolhive-core v0.0.21 ships a shared redis package designed to replace those duplicates so the next consumer migration can move stacklok-llm-gateway onto the same library without further drift. Replace the local RedisConfig/SentinelConfig/RedisTLSConfig/ACLUserConfig types and the buildTLSConfig/newTLSDialer/configureTLSDialer/validateConfig helpers with toolhive-core/redis.Config and NewClient. NewRedisStorage now takes a tcredis.Config plus the auth-server-specific keyPrefix; the constructor keeps the auth-server invariants (ACL password required, key prefix required) and delegates topology validation, defaults, TLS plumbing, and Ping verification to the shared package. The runner's convertRedisRunConfig now produces a tcredis.Config (ACL credentials resolved from env vars, durations parsed, CA cert file read). Topology checks that duplicated tcredis's Validate were dropped — those errors now surface from the shared package. Tests: delete the storage-side TLS helper tests (now exercising dependency code), trim the validation table down to the auth-server-specific cases, table-drive the three connection-failure tests, remove runner-side topology validation subtests, and update the env-var subtests to the new field layout. Co-Authored-By: Claude Opus 4.7 (1M context) --- go.mod | 7 +- go.sum | 18 +- pkg/authserver/runner/embeddedauthserver.go | 73 ++--- .../runner/embeddedauthserver_test.go | 101 +------ pkg/authserver/storage/redis.go | 278 ++---------------- pkg/authserver/storage/redis_test.go | 199 +++++-------- pkg/authserver/storage/redis_tls_test.go | 275 ----------------- 7 files changed, 146 insertions(+), 805 deletions(-) delete mode 100644 pkg/authserver/storage/redis_tls_test.go diff --git a/go.mod b/go.mod index 53793705b7..64628cfd05 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.26 require ( dario.cat/mergo v1.0.2 github.com/1password/onepassword-sdk-go v0.3.1 - github.com/alicebob/miniredis/v2 v2.37.0 + github.com/alicebob/miniredis/v2 v2.38.0 github.com/atotto/clipboard v0.1.4 github.com/aws/aws-sdk-go-v2 v1.41.7 github.com/aws/aws-sdk-go-v2/config v1.32.17 @@ -45,11 +45,11 @@ require ( github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/pressly/goose/v3 v3.27.0 github.com/prometheus/client_golang v1.23.2 - github.com/redis/go-redis/v9 v9.18.0 + github.com/redis/go-redis/v9 v9.19.0 github.com/shirou/gopsutil/v4 v4.26.3 github.com/spf13/viper v1.21.0 github.com/stacklok/toolhive-catalog v0.20260518.0 - github.com/stacklok/toolhive-core v0.0.20 + github.com/stacklok/toolhive-core v0.0.21 github.com/stretchr/testify v1.11.1 github.com/swaggo/swag/v2 v2.0.0-rc5 github.com/tailscale/hujson v0.0.0-20260302212456-ecc657c15afd @@ -133,7 +133,6 @@ require ( github.com/cyphar/filepath-securejoin v0.6.1 // indirect github.com/danieljoos/wincred v1.2.3 // indirect github.com/dgraph-io/ristretto v1.0.0 // indirect - github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect github.com/digitorus/pkcs7 v0.0.0-20230818184609-3a137a874352 // indirect github.com/digitorus/timestamp v0.0.0-20231217203849-220c5c2851b7 // indirect github.com/docker/cli v29.4.0+incompatible // indirect diff --git a/go.sum b/go.sum index fc5578f785..37fd722cdf 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,8 @@ github.com/ProtonMail/go-crypto v1.1.6 h1:ZcV+Ropw6Qn0AX9brlQLAUXfqLBc7Bl+f/DmNx github.com/ProtonMail/go-crypto v1.1.6/go.mod h1:rA3QumHc/FZ8pAHreoekgiAbzpNsfQAosU5td4SnOrE= github.com/adrg/xdg v0.5.3 h1:xRnxJXne7+oWDatRhR1JLnvuccuIeCoBu2rtuLqQB78= github.com/adrg/xdg v0.5.3/go.mod h1:nlTsY+NNiCBGCK2tpm09vRqfVzrc2fLmXGpBLF0zlTQ= -github.com/alicebob/miniredis/v2 v2.37.0 h1:RheObYW32G1aiJIj81XVt78ZHJpHonHLHW7OLIshq68= -github.com/alicebob/miniredis/v2 v2.37.0/go.mod h1:TcL7YfarKPGDAthEtl5NBeHZfeUQj6OXMm/+iu5cLMM= +github.com/alicebob/miniredis/v2 v2.38.0 h1:nZAzCR+Lj+Vxk4ZXzm2NuKq2O33RXj1XxJ2e2uP9jiw= +github.com/alicebob/miniredis/v2 v2.38.0/go.mod h1:TcL7YfarKPGDAthEtl5NBeHZfeUQj6OXMm/+iu5cLMM= github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be h1:9AeTilPcZAjCFIImctFaOjnTIavg87rW78vTPkQqLI8= github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be/go.mod h1:ySMOLuWl6zY27l47sB3qLNK6tF2fkHG55UZxx8oIVo4= github.com/antlr4-go/antlr/v4 v4.13.1 h1:SqQKkuVZ+zWkMMNkjy5FZe5mr5WURWnlpmOuzYWrPrQ= @@ -178,8 +178,6 @@ github.com/dgraph-io/ristretto v1.0.0 h1:SYG07bONKMlFDUYu5pEu3DGAh8c2OFNzKm6G9J4 github.com/dgraph-io/ristretto v1.0.0/go.mod h1:jTi2FiYEhQ1NsMmA7DeBykizjOuY88NhKBkepyu1jPc= github.com/dgryski/go-farm v0.0.0-20200201041132-a6ae2369ad13 h1:fAjc9m62+UWV/WAFKLNi6ZS0675eEUC9y3AlwSbQu1Y= github.com/dgryski/go-farm v0.0.0-20200201041132-a6ae2369ad13/go.mod h1:SqUrOPUnsFjfmXRMNPybcSiG0BgUW2AuFH8PAnS2iTw= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f h1:lO4WD4F/rVNCu3HqELle0jiPLLBs70cWOduZpkS1E78= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f/go.mod h1:cuUVRXasLTGF7a8hSLbxyZXjz+1KgoB3wDUb6vlszIc= github.com/digitorus/pkcs7 v0.0.0-20230713084857-e76b763bdc49/go.mod h1:SKVExuS+vpu2l9IoOc0RwqE7NYnb0JlcFHFnEJkVDzc= github.com/digitorus/pkcs7 v0.0.0-20230818184609-3a137a874352 h1:ge14PCmCvPjpMQMIAH7uKg0lrtNSOdpYsRXlwk3QbaE= github.com/digitorus/pkcs7 v0.0.0-20230818184609-3a137a874352/go.mod h1:SKVExuS+vpu2l9IoOc0RwqE7NYnb0JlcFHFnEJkVDzc= @@ -717,8 +715,8 @@ github.com/prometheus/otlptranslator v1.0.0 h1:s0LJW/iN9dkIH+EnhiD3BlkkP5QVIUVEo github.com/prometheus/otlptranslator v1.0.0/go.mod h1:vRYWnXvI6aWGpsdY/mOT/cbeVRBlPWtBNDb7kGR3uKM= github.com/prometheus/procfs v0.20.1 h1:XwbrGOIplXW/AU3YhIhLODXMJYyC1isLFfYCsTEycfc= github.com/prometheus/procfs v0.20.1/go.mod h1:o9EMBZGRyvDrSPH1RqdxhojkuXstoe4UlK79eF5TGGo= -github.com/redis/go-redis/v9 v9.18.0 h1:pMkxYPkEbMPwRdenAzUNyFNrDgHx9U+DrBabWNfSRQs= -github.com/redis/go-redis/v9 v9.18.0/go.mod h1:k3ufPphLU5YXwNTUcCRXGxUoF1fqxnhFQmscfkCoDA0= +github.com/redis/go-redis/v9 v9.19.0 h1:XPVaaPSnG6RhYf7p+rmSa9zZfeVAnWsH5h3lxthOm/k= +github.com/redis/go-redis/v9 v9.19.0/go.mod h1:v/M13XI1PVCDcm01VtPFOADfZtHf8YW3baQf57KlIkA= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= @@ -807,8 +805,8 @@ github.com/spf13/viper v1.21.0 h1:x5S+0EU27Lbphp4UKm1C+1oQO+rKx36vfCoaVebLFSU= github.com/spf13/viper v1.21.0/go.mod h1:P0lhsswPGWD/1lZJ9ny3fYnVqxiegrlNrEmgLjbTCAY= github.com/stacklok/toolhive-catalog v0.20260518.0 h1:J4Adoaedsm09RB/8Dx2PSlGgZ4BscYOgmUrWzMIcMY4= github.com/stacklok/toolhive-catalog v0.20260518.0/go.mod h1:e/zMn+cmBvXMGtRJu0WYO71h0UVRgXTq32849PkQgss= -github.com/stacklok/toolhive-core v0.0.20 h1:7XZTN6XAd8yHjHaLxtiTXOocVyBrKoRh7Ygw417/8Z0= -github.com/stacklok/toolhive-core v0.0.20/go.mod h1:ZEAdmyyHdxikO7CvpYY/xLctPxh1ZW3+ZneMMIbbZHw= +github.com/stacklok/toolhive-core v0.0.21 h1:UxCDSmhw0k4QP6gr0F1CLfPuI3bVHBu3VuHt/DLYH50= +github.com/stacklok/toolhive-core v0.0.21/go.mod h1:59Fo8iGJEEGJB6i/N/4fiB2Z+7AhBbPrguiggtED5TU= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= @@ -902,8 +900,8 @@ github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0= github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= github.com/zalando/go-keyring v0.2.8/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= -github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0= -github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA= +github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= +github.com/zeebo/xxh3 v1.1.0/go.mod h1:IisAie1LELR4xhVinxWS5+zf1lA4p0MW4T+w+W07F5s= github.com/zenazn/goji v0.9.0/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 19dedb1b74..89f72eb768 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -15,6 +15,7 @@ import ( "sync" "time" + tcredis "github.com/stacklok/toolhive-core/redis" "github.com/stacklok/toolhive/pkg/auth/dcr" "github.com/stacklok/toolhive/pkg/authserver" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" @@ -643,55 +644,47 @@ func createStorage(ctx context.Context, cfg *storage.RunConfig) (storage.Storage if err != nil { return nil, fmt.Errorf("invalid Redis config: %w", err) } - return storage.NewRedisStorage(ctx, *redisCfg) + return storage.NewRedisStorage(ctx, redisCfg, cfg.RedisConfig.KeyPrefix) } return nil, fmt.Errorf("unsupported storage type: %s", cfg.Type) } -// convertRedisRunConfig converts a serializable RedisRunConfig to the runtime RedisConfig. -// It resolves credentials from environment variables and parses duration strings. -func convertRedisRunConfig(rc *storage.RedisRunConfig) (*storage.RedisConfig, error) { +// convertRedisRunConfig converts a serializable RedisRunConfig to a runtime +// tcredis.Config. It resolves ACL credentials from environment variables and +// parses duration strings. Connection-mode topology and defaulting are handled +// by the shared toolhive-core redis package when the client is constructed. +func convertRedisRunConfig(rc *storage.RedisRunConfig) (tcredis.Config, error) { if rc == nil { - return nil, fmt.Errorf("redis config is required when storage type is redis") + return tcredis.Config{}, fmt.Errorf("redis config is required when storage type is redis") } - if rc.Addr != "" && rc.SentinelConfig != nil { - return nil, fmt.Errorf("addr and sentinel_config are mutually exclusive; exactly one must be set") - } - if rc.Addr == "" && rc.SentinelConfig == nil { - return nil, fmt.Errorf("one of addr (standalone or cluster) or sentinel_config (sentinel) is required") - } - if rc.ClusterMode && rc.SentinelConfig != nil { - return nil, fmt.Errorf("cluster mode cannot be used with sentinel configuration") - } - - cfg := &storage.RedisConfig{ + cfg := tcredis.Config{ Addr: rc.Addr, ClusterMode: rc.ClusterMode, - KeyPrefix: rc.KeyPrefix, } if rc.SentinelConfig != nil { - cfg.SentinelConfig = &storage.SentinelConfig{ + cfg.SentinelConfig = &tcredis.SentinelConfig{ MasterName: rc.SentinelConfig.MasterName, SentinelAddrs: rc.SentinelConfig.SentinelAddrs, - DB: rc.SentinelConfig.DB, } + cfg.DB = rc.SentinelConfig.DB } - aclCfg, err := convertRedisACLConfig(rc.ACLUserConfig) + username, password, err := convertRedisACLConfig(rc.ACLUserConfig) if err != nil { - return nil, fmt.Errorf("failed to convert ACL config: %w", err) + return tcredis.Config{}, fmt.Errorf("failed to convert ACL config: %w", err) } - cfg.ACLUserConfig = aclCfg + cfg.Username = username + cfg.Password = password - if err := applyRedisTimeouts(rc, cfg); err != nil { - return nil, fmt.Errorf("failed to apply redis timeouts: %w", err) + if err := applyRedisTimeouts(rc, &cfg); err != nil { + return tcredis.Config{}, fmt.Errorf("failed to apply redis timeouts: %w", err) } tlsCfg, err := convertRedisTLSRunConfig(rc.TLS) if err != nil { - return nil, fmt.Errorf("master TLS config: %w", err) + return tcredis.Config{}, fmt.Errorf("master TLS config: %w", err) } cfg.TLS = tlsCfg @@ -699,7 +692,7 @@ func convertRedisRunConfig(rc *storage.RedisRunConfig) (*storage.RedisConfig, er if rc.SentinelConfig != nil { sentinelTLSCfg, err := convertRedisTLSRunConfig(rc.SentinelTLS) if err != nil { - return nil, fmt.Errorf("sentinel TLS config: %w", err) + return tcredis.Config{}, fmt.Errorf("sentinel TLS config: %w", err) } cfg.SentinelTLS = sentinelTLSCfg } @@ -713,30 +706,27 @@ func convertRedisRunConfig(rc *storage.RedisRunConfig) (*storage.RedisConfig, er // for servers that do not support HELLO). This is required for managed Redis // tiers without ACL users (e.g. GCP Memorystore Basic/Standard HA, Azure Cache // for Redis). -func convertRedisACLConfig(rc *storage.ACLUserRunConfig) (*storage.ACLUserConfig, error) { +func convertRedisACLConfig(rc *storage.ACLUserRunConfig) (string, string, error) { if rc == nil { - return nil, fmt.Errorf("acl user config is required") + return "", "", fmt.Errorf("acl user config is required") } var username string if rc.UsernameEnvVar != "" { var err error username, err = resolveEnvVar(rc.UsernameEnvVar) if err != nil { - return nil, fmt.Errorf("failed to resolve Redis username: %w", err) + return "", "", fmt.Errorf("failed to resolve Redis username: %w", err) } } password, err := resolveEnvVar(rc.PasswordEnvVar) if err != nil { - return nil, fmt.Errorf("failed to resolve Redis password: %w", err) + return "", "", fmt.Errorf("failed to resolve Redis password: %w", err) } - return &storage.ACLUserConfig{ - Username: username, - Password: password, - }, nil + return username, password, nil } // applyRedisTimeouts parses and applies optional timeout duration strings to cfg. -func applyRedisTimeouts(rc *storage.RedisRunConfig, cfg *storage.RedisConfig) error { +func applyRedisTimeouts(rc *storage.RedisRunConfig, cfg *tcredis.Config) error { if rc.DialTimeout != "" { d, err := time.ParseDuration(rc.DialTimeout) if err != nil { @@ -761,15 +751,16 @@ func applyRedisTimeouts(rc *storage.RedisRunConfig, cfg *storage.RedisConfig) er return nil } -// convertRedisTLSRunConfig converts a RedisTLSRunConfig to runtime RedisTLSConfig. -// Returns an error if a CA cert file is configured but cannot be read — this is -// treated as a hard error because silently falling back to system CAs could mask -// a misconfiguration and cause confusing TLS failures downstream. -func convertRedisTLSRunConfig(rc *storage.RedisTLSRunConfig) (*storage.RedisTLSConfig, error) { +// convertRedisTLSRunConfig converts a RedisTLSRunConfig to a runtime +// tcredis.TLSConfig. Returns an error if a CA cert file is configured but +// cannot be read — this is treated as a hard error because silently falling +// back to system CAs could mask a misconfiguration and cause confusing TLS +// failures downstream. +func convertRedisTLSRunConfig(rc *storage.RedisTLSRunConfig) (*tcredis.TLSConfig, error) { if rc == nil { return nil, nil } - cfg := &storage.RedisTLSConfig{ + cfg := &tcredis.TLSConfig{ InsecureSkipVerify: rc.InsecureSkipVerify, } if rc.CACertFile != "" { diff --git a/pkg/authserver/runner/embeddedauthserver_test.go b/pkg/authserver/runner/embeddedauthserver_test.go index f9d9030d43..a0f5cbe782 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -1166,24 +1166,11 @@ func TestCreateStorage(t *testing.T) { assert.Contains(t, err.Error(), "redis config is required") }) - t.Run("redis type with missing sentinel config returns error", func(t *testing.T) { - t.Parallel() - - _, err := createStorage(ctx, &storage.RunConfig{ - Type: string(storage.TypeRedis), - RedisConfig: &storage.RedisRunConfig{ - KeyPrefix: "test:", - ACLUserConfig: &storage.ACLUserRunConfig{ - UsernameEnvVar: "REDIS_USER", - PasswordEnvVar: "REDIS_PASS", - }, - }, - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "one of addr (standalone or cluster) or sentinel_config (sentinel) is required") - }) } +// TestConvertRedisRunConfig covers the runner-owned conversion steps: nil +// guard and ACL credential resolution. Connection-mode topology validation is +// owned by the shared toolhive-core redis package and exercised in its tests. func TestConvertRedisRunConfig(t *testing.T) { t.Parallel() @@ -1194,19 +1181,6 @@ func TestConvertRedisRunConfig(t *testing.T) { assert.Contains(t, err.Error(), "redis config is required") }) - t.Run("missing sentinel config returns error", func(t *testing.T) { - t.Parallel() - _, err := convertRedisRunConfig(&storage.RedisRunConfig{ - KeyPrefix: "test:", - ACLUserConfig: &storage.ACLUserRunConfig{ - UsernameEnvVar: "USER", - PasswordEnvVar: "PASS", - }, - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "one of addr (standalone or cluster) or sentinel_config (sentinel) is required") - }) - t.Run("missing ACL user config returns error", func(t *testing.T) { t.Parallel() _, err := convertRedisRunConfig(&storage.RedisRunConfig{ @@ -1236,54 +1210,6 @@ func TestConvertRedisRunConfig(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "failed to resolve Redis username") }) - - t.Run("addr and sentinel config both set returns error", func(t *testing.T) { - t.Parallel() - _, err := convertRedisRunConfig(&storage.RedisRunConfig{ - Addr: "redis.example.com:6379", - SentinelConfig: &storage.SentinelRunConfig{ - MasterName: "mymaster", - SentinelAddrs: []string{"sentinel:26379"}, - }, - ACLUserConfig: &storage.ACLUserRunConfig{ - UsernameEnvVar: "USER", - PasswordEnvVar: "PASS", - }, - KeyPrefix: "thv:", - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "mutually exclusive") - }) - - t.Run("neither addr nor sentinel config returns error", func(t *testing.T) { - t.Parallel() - _, err := convertRedisRunConfig(&storage.RedisRunConfig{ - ACLUserConfig: &storage.ACLUserRunConfig{ - UsernameEnvVar: "USER", - PasswordEnvVar: "PASS", - }, - KeyPrefix: "thv:", - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "one of addr") - }) - - t.Run("cluster mode with sentinel also set returns error", func(t *testing.T) { - t.Parallel() - _, err := convertRedisRunConfig(&storage.RedisRunConfig{ - ClusterMode: true, - SentinelConfig: &storage.SentinelRunConfig{ - MasterName: "mymaster", - SentinelAddrs: []string{"sentinel:26379"}, - }, - ACLUserConfig: &storage.ACLUserRunConfig{ - PasswordEnvVar: "PASS", - }, - KeyPrefix: "thv:", - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "cluster mode cannot be used with sentinel") - }) } // TestConvertRedisRunConfig_WithEnvVars tests convertRedisRunConfig with environment variables. @@ -1309,16 +1235,13 @@ func TestConvertRedisRunConfig_WithEnvVars(t *testing.T) { WriteTimeout: "3s", }) require.NoError(t, err) - require.NotNil(t, cfg) - assert.Equal(t, "thv:auth:ns:name:", cfg.KeyPrefix) require.NotNil(t, cfg.SentinelConfig) assert.Equal(t, "mymaster", cfg.SentinelConfig.MasterName) assert.Equal(t, []string{"10.0.0.1:26379", "10.0.0.2:26379"}, cfg.SentinelConfig.SentinelAddrs) - assert.Equal(t, 3, cfg.SentinelConfig.DB) - require.NotNil(t, cfg.ACLUserConfig) - assert.Equal(t, "myuser", cfg.ACLUserConfig.Username) - assert.Equal(t, "mypass", cfg.ACLUserConfig.Password) + assert.Equal(t, 3, cfg.DB) + assert.Equal(t, "myuser", cfg.Username) + assert.Equal(t, "mypass", cfg.Password) assert.Equal(t, 10*time.Second, cfg.DialTimeout) assert.Equal(t, 5*time.Second, cfg.ReadTimeout) assert.Equal(t, 3*time.Second, cfg.WriteTimeout) @@ -1394,9 +1317,8 @@ func TestConvertRedisRunConfig_WithEnvVars(t *testing.T) { }, }) require.NoError(t, err) - require.NotNil(t, cfg.ACLUserConfig) - assert.Empty(t, cfg.ACLUserConfig.Username) - assert.Equal(t, "mypass", cfg.ACLUserConfig.Password) + assert.Empty(t, cfg.Username) + assert.Equal(t, "mypass", cfg.Password) }) t.Run("cluster mode resolves correctly", func(t *testing.T) { @@ -1413,14 +1335,11 @@ func TestConvertRedisRunConfig_WithEnvVars(t *testing.T) { KeyPrefix: "thv:auth:ns:name:", }) require.NoError(t, err) - require.NotNil(t, cfg) assert.Equal(t, "discovery.example.com:6379", cfg.Addr) assert.True(t, cfg.ClusterMode) assert.Nil(t, cfg.SentinelConfig) - require.NotNil(t, cfg.ACLUserConfig) - assert.Equal(t, "clusteruser", cfg.ACLUserConfig.Username) - assert.Equal(t, "clusterpass", cfg.ACLUserConfig.Password) - assert.Equal(t, "thv:auth:ns:name:", cfg.KeyPrefix) + assert.Equal(t, "clusteruser", cfg.Username) + assert.Equal(t, "clusterpass", cfg.Password) }) } diff --git a/pkg/authserver/storage/redis.go b/pkg/authserver/storage/redis.go index 76651109eb..594989f0ee 100644 --- a/pkg/authserver/storage/redis.go +++ b/pkg/authserver/storage/redis.go @@ -5,13 +5,10 @@ package storage import ( "context" - "crypto/tls" - "crypto/x509" "encoding/json" "errors" "fmt" "log/slog" - "net" "net/url" "slices" "time" @@ -19,16 +16,10 @@ import ( "github.com/ory/fosite" "github.com/redis/go-redis/v9" + tcredis "github.com/stacklok/toolhive-core/redis" "github.com/stacklok/toolhive/pkg/authserver/server/session" ) -// Default timeouts for Redis operations. -const ( - DefaultDialTimeout = 5 * time.Second - DefaultReadTimeout = 3 * time.Second - DefaultWriteTimeout = 3 * time.Second -) - // nullMarker is used to store nil upstream tokens in Redis. const nullMarker = "null" @@ -61,64 +52,6 @@ func warnOnCleanupErr(err error, operation, key string) { } } -// RedisConfig holds Redis connection configuration for runtime use. -type RedisConfig struct { - // Addr is the Redis server address (host:port). Required for standalone and cluster modes. - // Mutually exclusive with SentinelConfig. - Addr string - - // ClusterMode enables Redis Cluster protocol. Requires Addr to be set. - // Use for managed cluster-mode services (GCP Memorystore Cluster, AWS ElastiCache cluster mode). - ClusterMode bool - - // SentinelConfig is required for Sentinel mode. Mutually exclusive with Addr. - SentinelConfig *SentinelConfig - - // ACLUserConfig is required - ACL user authentication only. - ACLUserConfig *ACLUserConfig - - // KeyPrefix for multi-tenancy: "thv:auth:{ns}:{name}:". - KeyPrefix string - - // Timeouts (defaults: Dial=5s, Read=3s, Write=3s). - DialTimeout time.Duration - ReadTimeout time.Duration - WriteTimeout time.Duration - - // TLS configures TLS for connections to the Redis/Valkey master or cluster nodes. - // When nil, connections are plaintext. - TLS *RedisTLSConfig - - // SentinelTLS configures TLS for connections to Sentinel instances. - // Only applies when SentinelConfig is set. When nil, sentinel connections are plaintext. - SentinelTLS *RedisTLSConfig -} - -// RedisTLSConfig holds TLS configuration for Redis connections. -// Presence of this struct enables TLS for the connection type. -type RedisTLSConfig struct { - // InsecureSkipVerify skips certificate verification. - // Use for self-signed certificates (e.g., sentinel emulators). - InsecureSkipVerify bool - - // CACert is the PEM-encoded CA certificate for verifying the server. - // When empty, the system root CAs are used. - CACert []byte -} - -// SentinelConfig contains Redis Sentinel configuration. -type SentinelConfig struct { - MasterName string - SentinelAddrs []string - DB int -} - -// ACLUserConfig contains Redis ACL user authentication configuration. -type ACLUserConfig struct { - Username string - Password string //nolint:gosec // G117: field legitimately holds sensitive data -} - // RedisStorage implements the Storage interface backed by Redis. // Supports standalone mode (single endpoint), Sentinel failover mode, and // Cluster mode. It provides distributed storage for OAuth2 tokens, authorization @@ -142,172 +75,30 @@ type storedSession struct { Session json.RawMessage `json:"session"` } -// buildTLSConfig creates a *tls.Config from a RedisTLSConfig. -// Returns an error if a CA certificate is provided but cannot be parsed. -func buildTLSConfig(cfg *RedisTLSConfig) (*tls.Config, error) { - if cfg == nil { - return nil, nil - } - tc := &tls.Config{ - MinVersion: tls.VersionTLS12, - InsecureSkipVerify: cfg.InsecureSkipVerify, //nolint:gosec // G402: configurable per-deployment - } - if len(cfg.CACert) > 0 { - pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(cfg.CACert) { - return nil, fmt.Errorf("failed to parse CA certificate PEM data") - } - tc.RootCAs = pool - } - return tc, nil -} - -// newTLSDialer returns a dialer function that applies different TLS configs for -// master vs sentinel connections. When masterTLS is nil, master connections use -// plaintext. When sentinelTLS is nil, sentinel connections use plaintext. -// This is needed because go-redis applies a single TLSConfig to all connections. -func newTLSDialer( - masterTLS, sentinelTLS *tls.Config, - sentinelAddrs []string, - timeout time.Duration, -) func(ctx context.Context, network, addr string) (net.Conn, error) { - return func(_ context.Context, network, addr string) (net.Conn, error) { - d := &net.Dialer{Timeout: timeout} - isSentinel := slices.Contains(sentinelAddrs, addr) - - var tlsCfg *tls.Config - if isSentinel { - tlsCfg = sentinelTLS - } else { - tlsCfg = masterTLS - } - - if tlsCfg == nil { - return d.Dial(network, addr) - } - return tls.DialWithDialer(d, network, addr, tlsCfg) - } -} - -// configureTLSDialer sets up a custom dialer on the FailoverOptions when either -// master or sentinel TLS is enabled. go-redis applies a single TLSConfig to both -// sentinel and master connections, so when they need different treatment we install -// a custom dialer that selects the right config per target address. -func configureTLSDialer(opts *redis.FailoverOptions, masterCfg, sentinelCfg *RedisTLSConfig) error { - if masterCfg == nil && sentinelCfg == nil { - return nil - } - - var masterTLS *tls.Config - if masterCfg != nil { - var err error - masterTLS, err = buildTLSConfig(masterCfg) - if err != nil { - return fmt.Errorf("master TLS config: %w", err) - } - } - - var sentinelTLS *tls.Config - if sentinelCfg != nil { - var err error - sentinelTLS, err = buildTLSConfig(sentinelCfg) - if err != nil { - return fmt.Errorf("sentinel TLS config: %w", err) - } - } - - opts.Dialer = newTLSDialer(masterTLS, sentinelTLS, opts.SentinelAddrs, opts.DialTimeout) - return nil -} - -// NewRedisStorage creates Redis-backed storage. -// Supports standalone mode (Addr), cluster mode (Addr + ClusterMode), and Sentinel mode (SentinelConfig). -// Returns error if configuration validation fails or connection cannot be established. -func NewRedisStorage(ctx context.Context, cfg RedisConfig) (*RedisStorage, error) { - if err := validateConfig(&cfg); err != nil { - return nil, fmt.Errorf("invalid redis configuration: %w", err) - } - - // Apply defaults - if cfg.DialTimeout == 0 { - cfg.DialTimeout = DefaultDialTimeout - } - if cfg.ReadTimeout == 0 { - cfg.ReadTimeout = DefaultReadTimeout - } - if cfg.WriteTimeout == 0 { - cfg.WriteTimeout = DefaultWriteTimeout +// NewRedisStorage creates Redis-backed storage. Connection-mode topology, +// timeouts, TLS, and credentials are configured through cfg; keyPrefix is the +// per-tenant key prefix (e.g. "thv:auth:{ns}:{name}:"). The auth server +// requires ACL authentication, so cfg.Password and keyPrefix must be non-empty. +// +// Connection-mode validation, timeout defaults, client construction (standalone, +// cluster, or sentinel), TLS plumbing, and connectivity verification are +// delegated to the shared toolhive-core redis package. +func NewRedisStorage(ctx context.Context, cfg tcredis.Config, keyPrefix string) (*RedisStorage, error) { + if cfg.Password == "" { + return nil, errors.New("invalid redis configuration: ACL password is required") } - - var client redis.UniversalClient - - switch { - case cfg.SentinelConfig != nil: - opts := &redis.FailoverOptions{ - MasterName: cfg.SentinelConfig.MasterName, - SentinelAddrs: cfg.SentinelConfig.SentinelAddrs, - DB: cfg.SentinelConfig.DB, - Username: cfg.ACLUserConfig.Username, - Password: cfg.ACLUserConfig.Password, - DialTimeout: cfg.DialTimeout, - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - } - - // Configure TLS dialer if either master or sentinel TLS is enabled. - if err := configureTLSDialer(opts, cfg.TLS, cfg.SentinelTLS); err != nil { - return nil, err - } - - client = redis.NewFailoverClient(opts) - - case cfg.ClusterMode: - tlsCfg, err := buildTLSConfig(cfg.TLS) - if err != nil { - return nil, fmt.Errorf("TLS config: %w", err) - } - - opts := &redis.ClusterOptions{ - Addrs: []string{cfg.Addr}, - Username: cfg.ACLUserConfig.Username, - Password: cfg.ACLUserConfig.Password, - DialTimeout: cfg.DialTimeout, - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - TLSConfig: tlsCfg, - } - - client = redis.NewClusterClient(opts) - - default: - masterTLS, err := buildTLSConfig(cfg.TLS) - if err != nil { - return nil, fmt.Errorf("master TLS config: %w", err) - } - - opts := &redis.Options{ - Addr: cfg.Addr, - Username: cfg.ACLUserConfig.Username, - Password: cfg.ACLUserConfig.Password, - DialTimeout: cfg.DialTimeout, - ReadTimeout: cfg.ReadTimeout, - WriteTimeout: cfg.WriteTimeout, - TLSConfig: masterTLS, - } - - client = redis.NewClient(opts) + if keyPrefix == "" { + return nil, errors.New("invalid redis configuration: key prefix is required") } - // Test connection - if err := client.Ping(ctx).Err(); err != nil { - // Close the client to prevent resource leak - _ = client.Close() - return nil, fmt.Errorf("failed to connect to redis: %w", err) + client, err := tcredis.NewClient(ctx, &cfg) + if err != nil { + return nil, err } return &RedisStorage{ client: client, - keyPrefix: cfg.KeyPrefix, + keyPrefix: keyPrefix, }, nil } @@ -327,39 +118,6 @@ func defaultSessionFactory(subject, idpSessionID, clientID string) fosite.Sessio return session.New(subject, idpSessionID, clientID, session.UserClaims{}) } -func validateConfig(cfg *RedisConfig) error { - if cfg.ClusterMode && cfg.SentinelConfig != nil { - return errors.New("cluster mode cannot be used with sentinel configuration") - } - if cfg.Addr != "" && cfg.SentinelConfig != nil { - return errors.New("addr and sentinel configuration are mutually exclusive; exactly one must be set") - } - if cfg.ClusterMode && cfg.Addr == "" { - return errors.New("cluster mode requires addr to be set") - } - if cfg.Addr == "" && cfg.SentinelConfig == nil { - return errors.New("one of addr (standalone or cluster) or sentinel configuration is required") - } - if cfg.SentinelConfig != nil { - if cfg.SentinelConfig.MasterName == "" { - return errors.New("sentinel master name is required") - } - if len(cfg.SentinelConfig.SentinelAddrs) == 0 { - return errors.New("at least one sentinel address is required") - } - } - if cfg.ACLUserConfig == nil { - return errors.New("ACL user configuration is required") - } - if cfg.ACLUserConfig.Password == "" { - return errors.New("ACL password is required") - } - if cfg.KeyPrefix == "" { - return errors.New("key prefix is required") - } - return nil -} - // Health checks Redis connectivity. func (s *RedisStorage) Health(ctx context.Context) error { if err := s.client.Ping(ctx).Err(); err != nil { diff --git a/pkg/authserver/storage/redis_test.go b/pkg/authserver/storage/redis_test.go index a48a6f6eff..376b5666c1 100644 --- a/pkg/authserver/storage/redis_test.go +++ b/pkg/authserver/storage/redis_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + tcredis "github.com/stacklok/toolhive-core/redis" "github.com/stacklok/toolhive/pkg/authserver/server/session" ) @@ -93,79 +94,46 @@ func requireRedisNotFoundError(t *testing.T, err error) { // --- Configuration Tests --- -func TestRedisConfig_Validation(t *testing.T) { +// TestNewRedisStorage_Validation covers the auth-server-specific invariants +// enforced by NewRedisStorage. Connection-mode topology (Addr XOR Sentinel, +// cluster requires Addr, sentinel master name and addresses) is validated by +// the shared toolhive-core redis package and exercised in its own tests. +func TestNewRedisStorage_Validation(t *testing.T) { t.Parallel() + validACL := func() tcredis.Config { + return tcredis.Config{ + Addr: "localhost:6379", + Username: "user", + Password: "pass", + } + } + tests := []struct { - name string - cfg RedisConfig - wantErr string + name string + cfg tcredis.Config + keyPrefix string + wantErr string }{ { - name: "neither addr nor sentinel config", - cfg: RedisConfig{ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, KeyPrefix: "test:"}, - wantErr: "one of addr (standalone or cluster) or sentinel configuration is required", + name: "missing ACL password", + cfg: tcredis.Config{Addr: "localhost:6379", Username: "user"}, + keyPrefix: "test:", + wantErr: "ACL password is required", }, { - name: "addr and sentinel config both set", - cfg: RedisConfig{ - Addr: "localhost:6379", - SentinelConfig: &SentinelConfig{MasterName: "mymaster", SentinelAddrs: []string{"localhost:26379"}}, - ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, - KeyPrefix: "test:", - }, - wantErr: "mutually exclusive", - }, - { - name: "cluster mode with sentinel config", - cfg: RedisConfig{ - ClusterMode: true, - SentinelConfig: &SentinelConfig{MasterName: "m", SentinelAddrs: []string{"localhost:26379"}}, - ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, - KeyPrefix: "test:", - }, - wantErr: "cluster mode cannot be used with sentinel", - }, - { - name: "cluster mode without addr", - cfg: RedisConfig{ - ClusterMode: true, - ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, - KeyPrefix: "test:", - }, - wantErr: "cluster mode requires addr", - }, - { - name: "missing sentinel master name", - cfg: RedisConfig{SentinelConfig: &SentinelConfig{SentinelAddrs: []string{"localhost:26379"}}, ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, KeyPrefix: "test:"}, - wantErr: "sentinel master name is required", - }, - { - name: "missing sentinel addresses", - cfg: RedisConfig{SentinelConfig: &SentinelConfig{MasterName: "mymaster"}, ACLUserConfig: &ACLUserConfig{Username: "u", Password: "p"}, KeyPrefix: "test:"}, - wantErr: "at least one sentinel address is required", - }, - { - name: "missing ACL user config", - cfg: RedisConfig{Addr: "localhost:6379", KeyPrefix: "test:"}, - wantErr: "ACL user configuration is required", - }, - { - name: "missing ACL password", - cfg: RedisConfig{Addr: "localhost:6379", ACLUserConfig: &ACLUserConfig{Username: "user"}, KeyPrefix: "test:"}, - wantErr: "ACL password is required", - }, - { - name: "missing key prefix", - cfg: RedisConfig{Addr: "localhost:6379", ACLUserConfig: &ACLUserConfig{Username: "user", Password: "pass"}}, - wantErr: "key prefix is required", + name: "missing key prefix", + cfg: validACL(), + keyPrefix: "", + wantErr: "key prefix is required", }, } + ctx := context.Background() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := validateConfig(&tt.cfg) + _, err := NewRedisStorage(ctx, tt.cfg, tt.keyPrefix) require.Error(t, err) assert.Contains(t, err.Error(), tt.wantErr) }) @@ -175,46 +143,54 @@ func TestRedisConfig_Validation(t *testing.T) { func TestNewRedisStorage_ConnectionFailure(t *testing.T) { t.Parallel() - cfg := RedisConfig{ - SentinelConfig: &SentinelConfig{ - MasterName: "mymaster", - SentinelAddrs: []string{"localhost:99999"}, // Invalid port + tests := []struct { + name string + cfg tcredis.Config + }{ + { + name: "sentinel mode", + cfg: tcredis.Config{ + SentinelConfig: &tcredis.SentinelConfig{ + MasterName: "mymaster", + SentinelAddrs: []string{"localhost:99999"}, // Invalid port + }, + Username: "user", + Password: "pass", + DialTimeout: 100 * time.Millisecond, + }, }, - ACLUserConfig: &ACLUserConfig{ - Username: "user", - Password: "pass", + { + name: "standalone mode", + cfg: tcredis.Config{ + Addr: "localhost:19999", + Username: "user", + Password: "pass", + DialTimeout: 100 * time.Millisecond, + }, }, - KeyPrefix: "test:", - DialTimeout: 100 * time.Millisecond, - } - - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() - - _, err := NewRedisStorage(ctx, cfg) - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to connect to redis") -} - -func TestNewRedisStorage_Standalone_ConnectionFailure(t *testing.T) { - t.Parallel() - - cfg := RedisConfig{ - Addr: "localhost:19999", - ACLUserConfig: &ACLUserConfig{ - Username: "user", - Password: "pass", + { + name: "cluster mode", + cfg: tcredis.Config{ + Addr: "localhost:19998", + ClusterMode: true, + Username: "user", + Password: "pass", + DialTimeout: 100 * time.Millisecond, + }, }, - KeyPrefix: "test:", - DialTimeout: 100 * time.Millisecond, } - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + t.Cleanup(cancel) - _, err := NewRedisStorage(ctx, cfg) - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to connect to redis") + _, err := NewRedisStorage(ctx, tt.cfg, "test:") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to connect") + }) + } } func TestNewRedisStorage_Standalone_WithMiniredis(t *testing.T) { @@ -225,45 +201,20 @@ func TestNewRedisStorage_Standalone_WithMiniredis(t *testing.T) { // supplied, so we use RequireUserAuth to match the configured username/password. mr.RequireUserAuth("testuser", "testpass") - cfg := RedisConfig{ - Addr: mr.Addr(), - ACLUserConfig: &ACLUserConfig{ - Username: "testuser", - Password: "testpass", - }, - KeyPrefix: "test:", + cfg := tcredis.Config{ + Addr: mr.Addr(), + Username: "testuser", + Password: "testpass", } ctx := context.Background() - s, err := NewRedisStorage(ctx, cfg) + s, err := NewRedisStorage(ctx, cfg, "test:") require.NoError(t, err) t.Cleanup(func() { _ = s.Close() }) require.NoError(t, s.Health(ctx)) } -func TestNewRedisStorage_Cluster_ConnectionFailure(t *testing.T) { - t.Parallel() - - cfg := RedisConfig{ - Addr: "localhost:19998", - ClusterMode: true, - ACLUserConfig: &ACLUserConfig{ - Username: "user", - Password: "pass", - }, - KeyPrefix: "test:", - DialTimeout: 100 * time.Millisecond, - } - - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() - - _, err := NewRedisStorage(ctx, cfg) - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to connect to redis") -} - // --- Client Tests --- func TestRedisStorage_Client(t *testing.T) { diff --git a/pkg/authserver/storage/redis_tls_test.go b/pkg/authserver/storage/redis_tls_test.go deleted file mode 100644 index 2477c4729e..0000000000 --- a/pkg/authserver/storage/redis_tls_test.go +++ /dev/null @@ -1,275 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package storage - -import ( - "context" - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rand" - "crypto/tls" - "crypto/x509" - "crypto/x509/pkix" - "encoding/pem" - "math/big" - "net" - "testing" - "time" - - "github.com/redis/go-redis/v9" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestBuildTLSConfig(t *testing.T) { - t.Parallel() - - caCert, caPEM := generateTestCACert(t) - - tests := []struct { - name string - cfg *RedisTLSConfig - wantErr bool - check func(t *testing.T, tc *tls.Config) - }{ - { - name: "nil config returns nil", - cfg: nil, - check: func(t *testing.T, tc *tls.Config) { - t.Helper() - assert.Nil(t, tc) - }, - }, - { - name: "empty config returns basic TLS config", - cfg: &RedisTLSConfig{}, - check: func(t *testing.T, tc *tls.Config) { - t.Helper() - require.NotNil(t, tc) - assert.Equal(t, uint16(tls.VersionTLS12), tc.MinVersion) - assert.False(t, tc.InsecureSkipVerify) - assert.Nil(t, tc.RootCAs) - }, - }, - { - name: "insecure skip verify", - cfg: &RedisTLSConfig{InsecureSkipVerify: true}, - check: func(t *testing.T, tc *tls.Config) { - t.Helper() - require.NotNil(t, tc) - assert.True(t, tc.InsecureSkipVerify) - }, - }, - { - name: "custom CA cert", - cfg: &RedisTLSConfig{CACert: caPEM}, - check: func(t *testing.T, tc *tls.Config) { - t.Helper() - require.NotNil(t, tc) - require.NotNil(t, tc.RootCAs) - _, err := caCert.Verify(x509.VerifyOptions{Roots: tc.RootCAs}) - assert.NoError(t, err, "CA cert should be verifiable with the pool") - }, - }, - { - name: "invalid CA cert returns error", - cfg: &RedisTLSConfig{CACert: []byte("not-a-cert")}, - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - tc, err := buildTLSConfig(tt.cfg) - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - tt.check(t, tc) - }) - } -} - -func TestRedisTLSConfig_SeparateMasterAndSentinel(t *testing.T) { - t.Parallel() - - masterCfg := &RedisTLSConfig{} - sentinelCfg := &RedisTLSConfig{ - InsecureSkipVerify: true, - } - - masterTLS, err := buildTLSConfig(masterCfg) - require.NoError(t, err) - sentinelTLS, err := buildTLSConfig(sentinelCfg) - require.NoError(t, err) - - require.NotNil(t, masterTLS) - require.NotNil(t, sentinelTLS) - - assert.False(t, masterTLS.InsecureSkipVerify, "master should verify certs") - assert.True(t, sentinelTLS.InsecureSkipVerify, "sentinel should skip verification") -} - -func TestNewTLSDialer(t *testing.T) { - t.Parallel() - - timeout := 5 * time.Second - - t.Run("master TLS only: sentinel addr uses plaintext", func(t *testing.T) { - t.Parallel() - masterTLS := &tls.Config{MinVersion: tls.VersionTLS12} - - // Start a plaintext listener to simulate sentinel - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - defer ln.Close() - - localSentinelAddrs := []string{ln.Addr().String()} - dialer := newTLSDialer(masterTLS, nil, localSentinelAddrs, timeout) - require.NotNil(t, dialer) - - conn, err := dialer(context.Background(), "tcp", ln.Addr().String()) - require.NoError(t, err) - conn.Close() - }) - - t.Run("sentinel TLS only: non-sentinel addr uses plaintext", func(t *testing.T) { - t.Parallel() - sentinelTLS := &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} //nolint:gosec // test - sentinelAddrs := []string{"sentinel.example.com:26379"} - - // Start a plaintext listener to simulate master - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - defer ln.Close() - - dialer := newTLSDialer(nil, sentinelTLS, sentinelAddrs, timeout) - require.NotNil(t, dialer) - - conn, err := dialer(context.Background(), "tcp", ln.Addr().String()) - require.NoError(t, err) - conn.Close() - }) - - t.Run("sentinel address matching uses slices.Contains", func(t *testing.T) { - t.Parallel() - addrs := []string{"10.0.0.1:26379", "10.0.0.2:26379", "sentinel.redis.svc:26379"} - - // Start plaintext listener — not in sentinel list, so master config applies. - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - defer ln.Close() - - sentinelTLS := &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: true} //nolint:gosec // test - dialer := newTLSDialer(nil, sentinelTLS, addrs, timeout) - - conn, err := dialer(context.Background(), "tcp", ln.Addr().String()) - require.NoError(t, err) - conn.Close() - }) -} - -func TestConfigureTLSDialer(t *testing.T) { - t.Parallel() - - _, caPEM := generateTestCACert(t) - - tests := []struct { - name string - masterCfg *RedisTLSConfig - sentinelCfg *RedisTLSConfig - wantDialer bool - wantErr bool - }{ - { - name: "no TLS configs — no dialer", - wantDialer: false, - }, - { - name: "master TLS only — installs dialer", - masterCfg: &RedisTLSConfig{}, - wantDialer: true, - }, - { - name: "sentinel TLS only — installs dialer", - sentinelCfg: &RedisTLSConfig{}, - wantDialer: true, - }, - { - name: "both TLS — installs dialer", - masterCfg: &RedisTLSConfig{}, - sentinelCfg: &RedisTLSConfig{InsecureSkipVerify: true}, - wantDialer: true, - }, - { - name: "master TLS with invalid CA cert — returns error", - masterCfg: &RedisTLSConfig{CACert: []byte("bad-pem")}, - wantErr: true, - }, - { - name: "sentinel TLS with invalid CA cert — returns error", - sentinelCfg: &RedisTLSConfig{CACert: []byte("bad-pem")}, - wantErr: true, - }, - { - name: "both TLS with valid CA certs — installs dialer", - masterCfg: &RedisTLSConfig{CACert: caPEM}, - sentinelCfg: &RedisTLSConfig{CACert: caPEM}, - wantDialer: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - opts := &redis.FailoverOptions{ - SentinelAddrs: []string{"sentinel:26379"}, - DialTimeout: 5 * time.Second, - } - - err := configureTLSDialer(opts, tt.masterCfg, tt.sentinelCfg) - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - - if tt.wantDialer { - assert.NotNil(t, opts.Dialer, "expected custom dialer to be set") - } else { - assert.Nil(t, opts.Dialer, "expected no custom dialer") - } - }) - } -} - -// generateTestCACert creates a self-signed CA certificate for testing. -func generateTestCACert(t *testing.T) (*x509.Certificate, []byte) { - t.Helper() - - key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - require.NoError(t, err) - - template := &x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: "test-ca"}, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Hour), - IsCA: true, - KeyUsage: x509.KeyUsageCertSign, - } - - certDER, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) - require.NoError(t, err) - - cert, err := x509.ParseCertificate(certDER) - require.NoError(t, err) - - certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) - - return cert, certPEM -} From cb1b3d6706141bd659abb3167b6a2c4185951fe8 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Mon, 18 May 2026 16:50:20 -0400 Subject: [PATCH 2/2] Return named struct from convertRedisACLConfig Returning (username, password string, error) made it possible to swap the two same-typed values silently at the call site. Introduce an unexported redisACLCredentials struct so the role of each field is visible at the assignment. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/authserver/runner/embeddedauthserver.go | 24 ++++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 89f72eb768..8de70d745d 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -671,12 +671,12 @@ func convertRedisRunConfig(rc *storage.RedisRunConfig) (tcredis.Config, error) { cfg.DB = rc.SentinelConfig.DB } - username, password, err := convertRedisACLConfig(rc.ACLUserConfig) + acl, err := convertRedisACLConfig(rc.ACLUserConfig) if err != nil { return tcredis.Config{}, fmt.Errorf("failed to convert ACL config: %w", err) } - cfg.Username = username - cfg.Password = password + cfg.Username = acl.username + cfg.Password = acl.password if err := applyRedisTimeouts(rc, &cfg); err != nil { return tcredis.Config{}, fmt.Errorf("failed to apply redis timeouts: %w", err) @@ -700,29 +700,37 @@ func convertRedisRunConfig(rc *storage.RedisRunConfig) (tcredis.Config, error) { return cfg, nil } +// redisACLCredentials carries resolved Redis ACL credentials between +// convertRedisACLConfig and its caller. Named fields prevent positional +// swaps of two same-typed strings at the call site. +type redisACLCredentials struct { + username string + password string +} + // convertRedisACLConfig resolves ACL user credentials from environment variables. // When UsernameEnvVar is empty, no username is resolved; go-redis then sends // HELLO with "default" as the username (or falls back to legacy AUTH // for servers that do not support HELLO). This is required for managed Redis // tiers without ACL users (e.g. GCP Memorystore Basic/Standard HA, Azure Cache // for Redis). -func convertRedisACLConfig(rc *storage.ACLUserRunConfig) (string, string, error) { +func convertRedisACLConfig(rc *storage.ACLUserRunConfig) (redisACLCredentials, error) { if rc == nil { - return "", "", fmt.Errorf("acl user config is required") + return redisACLCredentials{}, fmt.Errorf("acl user config is required") } var username string if rc.UsernameEnvVar != "" { var err error username, err = resolveEnvVar(rc.UsernameEnvVar) if err != nil { - return "", "", fmt.Errorf("failed to resolve Redis username: %w", err) + return redisACLCredentials{}, fmt.Errorf("failed to resolve Redis username: %w", err) } } password, err := resolveEnvVar(rc.PasswordEnvVar) if err != nil { - return "", "", fmt.Errorf("failed to resolve Redis password: %w", err) + return redisACLCredentials{}, fmt.Errorf("failed to resolve Redis password: %w", err) } - return username, password, nil + return redisACLCredentials{username: username, password: password}, nil } // applyRedisTimeouts parses and applies optional timeout duration strings to cfg.