diff --git a/diode-server/cmd/ingester/main.go b/diode-server/cmd/ingester/main.go index f02259c0..dcdb3e7e 100644 --- a/diode-server/cmd/ingester/main.go +++ b/diode-server/cmd/ingester/main.go @@ -17,6 +17,7 @@ import ( "github.com/netboxlabs/diode/diode-server/pprof" "github.com/netboxlabs/diode/diode-server/server" "github.com/netboxlabs/diode/diode-server/telemetry" + diodetls "github.com/netboxlabs/diode/diode-server/tls" "github.com/netboxlabs/diode/diode-server/version" ) @@ -66,22 +67,12 @@ func main() { metricRecorder.SetServiceInfo(ctx, fmt.Sprintf("%s.%s", version.GetBuildVersion(), version.GetBuildCommit())) - redisTLSConfig, err := cfg.RedisTLS.ToTLSConfig() + redisOptions, err := diodetls.NewRedisOptions(cfg.RedisHost, cfg.RedisPort, cfg.RedisUsername, cfg.RedisPassword, cfg.RedisStreamDB, &cfg.RedisTLS) if err != nil { - s.Logger().Error("failed to create TLS config for Redis", "error", err) + s.Logger().Error("failed to create Redis options", "error", err) metricRecorder.RecordServiceStartupAttempt(ctx, false) os.Exit(1) } - - redisOptions := redis.Options{ - Addr: fmt.Sprintf("%s:%s", cfg.RedisHost, cfg.RedisPort), - Password: cfg.RedisPassword, - DB: cfg.RedisStreamDB, - TLSConfig: redisTLSConfig, - } - if cfg.RedisUsername != "" { - redisOptions.Username = cfg.RedisUsername - } redisStreamClient := redis.NewClient(&redisOptions) if _, err := redisStreamClient.Ping(ctx).Result(); err != nil { diff --git a/diode-server/cmd/reconciler/main.go b/diode-server/cmd/reconciler/main.go index a5cc128f..f5c1ac85 100644 --- a/diode-server/cmd/reconciler/main.go +++ b/diode-server/cmd/reconciler/main.go @@ -26,6 +26,7 @@ import ( "github.com/netboxlabs/diode/diode-server/reconciler" "github.com/netboxlabs/diode/diode-server/server" "github.com/netboxlabs/diode/diode-server/telemetry" + diodetls "github.com/netboxlabs/diode/diode-server/tls" "github.com/netboxlabs/diode/diode-server/version" ) @@ -85,22 +86,12 @@ func main() { } } - redisTLSConfig, err := cfg.RedisTLS.ToTLSConfig() + redisOptions, err := diodetls.NewRedisOptions(cfg.RedisHost, cfg.RedisPort, cfg.RedisUsername, cfg.RedisPassword, cfg.RedisDB, &cfg.RedisTLS) if err != nil { - s.Logger().Error("failed to create TLS config for Redis", "error", err) + s.Logger().Error("failed to create Redis options", "error", err) metricRecorder.RecordServiceStartupAttempt(ctx, false) os.Exit(1) } - - redisOptions := redis.Options{ - Addr: fmt.Sprintf("%s:%s", cfg.RedisHost, cfg.RedisPort), - Password: cfg.RedisPassword, - DB: cfg.RedisDB, - TLSConfig: redisTLSConfig, - } - if cfg.RedisUsername != "" { - redisOptions.Username = cfg.RedisUsername - } redisClient := redis.NewClient(&redisOptions) if _, err := redisClient.Ping(ctx).Result(); err != nil { @@ -109,14 +100,11 @@ func main() { os.Exit(1) } - redisStreamOptions := redis.Options{ - Addr: fmt.Sprintf("%s:%s", cfg.RedisHost, cfg.RedisPort), - Password: cfg.RedisPassword, - DB: cfg.RedisStreamDB, - TLSConfig: redisTLSConfig, - } - if cfg.RedisUsername != "" { - redisStreamOptions.Username = cfg.RedisUsername + redisStreamOptions, err := diodetls.NewRedisOptions(cfg.RedisHost, cfg.RedisPort, cfg.RedisUsername, cfg.RedisPassword, cfg.RedisStreamDB, &cfg.RedisTLS) + if err != nil { + s.Logger().Error("failed to create Redis stream options", "error", err) + metricRecorder.RecordServiceStartupAttempt(ctx, false) + os.Exit(1) } redisStreamClient := redis.NewClient(&redisStreamOptions) diff --git a/diode-server/reconciler/server.go b/diode-server/reconciler/server.go index 279624cf..8e348d8c 100644 --- a/diode-server/reconciler/server.go +++ b/diode-server/reconciler/server.go @@ -13,6 +13,7 @@ import ( "google.golang.org/grpc/reflection" "github.com/netboxlabs/diode/diode-server/gen/diode/v1/reconcilerpb" + diodetls "github.com/netboxlabs/diode/diode-server/tls" ) // Server is a reconciler Server @@ -32,19 +33,9 @@ func NewServer(ctx context.Context, logger *slog.Logger, repository Repository, var cfg Config envconfig.MustProcess("", &cfg) - redisTLSConfig, err := cfg.RedisTLS.ToTLSConfig() + redisOptions, err := diodetls.NewRedisOptions(cfg.RedisHost, cfg.RedisPort, cfg.RedisUsername, cfg.RedisPassword, cfg.RedisDB, &cfg.RedisTLS) if err != nil { - return nil, fmt.Errorf("failed to create TLS config for Redis: %v", err) - } - - redisOptions := redis.Options{ - Addr: fmt.Sprintf("%s:%s", cfg.RedisHost, cfg.RedisPort), - Password: cfg.RedisPassword, - DB: cfg.RedisDB, - TLSConfig: redisTLSConfig, - } - if cfg.RedisUsername != "" { - redisOptions.Username = cfg.RedisUsername + return nil, fmt.Errorf("failed to create Redis options: %v", err) } redisClient := redis.NewClient(&redisOptions) diff --git a/diode-server/tls/redis.go b/diode-server/tls/redis.go new file mode 100644 index 00000000..cc5fb411 --- /dev/null +++ b/diode-server/tls/redis.go @@ -0,0 +1,34 @@ +package tls + +import ( + "fmt" + + "github.com/redis/go-redis/v9" +) + +// NewRedisOptions builds redis.Options with consistent TLS and ACL username +// handling. All Redis client creation should use this helper to ensure the +// username is never accidentally omitted (which would cause go-redis to +// authenticate as the "default" Redis user instead of the intended ACL user). +func NewRedisOptions(host, port, username, password string, db int, tlsCfg *Config) (redis.Options, error) { + if tlsCfg == nil { + return redis.Options{}, fmt.Errorf("TLS config must not be nil (use &tls.Config{} for no TLS)") + } + + goTLS, err := tlsCfg.ToTLSConfig() + if err != nil { + return redis.Options{}, fmt.Errorf("failed to create TLS config for Redis: %w", err) + } + + opts := redis.Options{ + Addr: fmt.Sprintf("%s:%s", host, port), + Password: password, + DB: db, + TLSConfig: goTLS, + } + if username != "" { + opts.Username = username + } + + return opts, nil +} diff --git a/diode-server/tls/redis_test.go b/diode-server/tls/redis_test.go new file mode 100644 index 00000000..89c53bd2 --- /dev/null +++ b/diode-server/tls/redis_test.go @@ -0,0 +1,147 @@ +package tls + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewRedisOptions(t *testing.T) { + tests := []struct { + name string + host string + port string + username string + password string + db int + tlsCfg *Config + wantAddr string + wantUser string + wantPass string + wantDB int + wantTLS bool + wantErr bool + }{ + { + name: "basic options without TLS", + host: "localhost", + port: "6379", + username: "", + password: "secret", + db: 0, + tlsCfg: &Config{Enabled: false}, + wantAddr: "localhost:6379", + wantUser: "", + wantPass: "secret", + wantDB: 0, + wantTLS: false, + }, + { + name: "with ACL username", + host: "redis.example.com", + port: "6380", + username: "redis-user", + password: "secret", + db: 1, + tlsCfg: &Config{Enabled: false}, + wantAddr: "redis.example.com:6380", + wantUser: "redis-user", + wantPass: "secret", + wantDB: 1, + wantTLS: false, + }, + { + name: "with TLS enabled", + host: "redis.example.com", + port: "6380", + username: "redis-user", + password: "secret", + db: 0, + tlsCfg: &Config{Enabled: true, SkipVerify: true}, + wantAddr: "redis.example.com:6380", + wantUser: "redis-user", + wantPass: "secret", + wantDB: 0, + wantTLS: true, + }, + { + name: "empty username is not set", + host: "localhost", + port: "6379", + username: "", + password: "secret", + db: 0, + tlsCfg: &Config{Enabled: false}, + wantAddr: "localhost:6379", + wantUser: "", + wantPass: "secret", + wantDB: 0, + wantTLS: false, + }, + { + name: "nil TLS config returns error", + host: "localhost", + port: "6379", + username: "", + password: "secret", + db: 0, + tlsCfg: nil, + wantErr: true, + }, + { + name: "invalid CA path returns error", + host: "localhost", + port: "6379", + username: "", + password: "secret", + db: 0, + tlsCfg: &Config{Enabled: true, CaPath: "/nonexistent/ca.crt"}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts, err := NewRedisOptions(tt.host, tt.port, tt.username, tt.password, tt.db, tt.tlsCfg) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + assert.Equal(t, tt.wantAddr, opts.Addr) + assert.Equal(t, tt.wantUser, opts.Username) + assert.Equal(t, tt.wantPass, opts.Password) + assert.Equal(t, tt.wantDB, opts.DB) + + if tt.wantTLS { + assert.NotNil(t, opts.TLSConfig, "TLSConfig should be set when TLS is enabled") + } else { + assert.Nil(t, opts.TLSConfig, "TLSConfig should be nil when TLS is disabled") + } + }) + } +} + +func TestNewRedisOptionsUsernameNotSetWhenEmpty(t *testing.T) { + // This test specifically verifies the core fix: when username is empty, + // the Username field on redis.Options must remain empty ("") rather than + // being set to "". go-redis treats an empty Username as "use the default + // Redis user", which is the correct behavior when no ACL username is + // configured. + opts, err := NewRedisOptions("localhost", "6379", "", "pass", 0, &Config{}) + require.NoError(t, err) + assert.Empty(t, opts.Username) +} + +func TestNewRedisOptionsUsernameSetWhenProvided(t *testing.T) { + // This test verifies the core fix: when a username IS provided, it MUST + // be propagated to redis.Options.Username. Missing this causes go-redis + // to authenticate as the "default" Redis user instead of the intended + // ACL user, resulting in WRONGPASS errors when the default user is + // disabled or has a different password. + opts, err := NewRedisOptions("localhost", "6379", "my-acl-user", "pass", 0, &Config{}) + require.NoError(t, err) + assert.Equal(t, "my-acl-user", opts.Username) +}