From 6f0a10f43a651d362319887a20ef9b9372ba7570 Mon Sep 17 00:00:00 2001 From: Harley Rossetto Date: Sun, 4 Oct 2026 13:40:03 +1100 Subject: [PATCH 1/3] redis mTLS implementation This commit adds in the ability for relay users to specify client certificates and keys, as well as the ability to provide a Certificate Authority file for verify cert trust against. --- config/config.go | 4 ++ internal/autoconfigcache/redis_store.go | 12 +++--- internal/bigsegments/store_redis.go | 13 ++++--- internal/sdks/data_stores.go | 50 +++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index cb887cae..79e11145 100644 --- a/config/config.go +++ b/config/config.go @@ -257,6 +257,10 @@ type RedisConfig struct { Username string `conf:"REDIS_USERNAME"` Password string `conf:"REDIS_PASSWORD"` + ClientCertificateFile string `conf:"REDIS_CLIENT_CERT_FILE"` + ClientKeyFile string `conf:"REDIS_CLIENT_KEY_FILE"` + CAFile string `conf:"REDIS_CA_FILE"` + AtomicUpsert bool `conf:"REDIS_ATOMIC_UPSERT"` } diff --git a/internal/autoconfigcache/redis_store.go b/internal/autoconfigcache/redis_store.go index 76fafe7c..659dba1e 100644 --- a/internal/autoconfigcache/redis_store.go +++ b/internal/autoconfigcache/redis_store.go @@ -2,7 +2,6 @@ package autoconfigcache import ( "context" - "crypto/tls" "encoding/json" "fmt" "log/slog" @@ -13,6 +12,7 @@ import ( "github.com/launchdarkly/ld-relay/v9/config" "github.com/launchdarkly/ld-relay/v9/internal/autoconfig" "github.com/launchdarkly/ld-relay/v9/internal/envfactory" + "github.com/launchdarkly/ld-relay/v9/internal/sdks" ) type redisStore struct { @@ -43,11 +43,13 @@ func newRedisStore(redisConfig config.RedisConfig, cacheKey string, encKey []byt if redisConfig.Username != "" { uo.Username = redisConfig.Username } - if redisConfig.TLS && uo.TLSConfig == nil { - uo.TLSConfig = &tls.Config{ - ServerName: redisConfig.URL.Get().Hostname(), - MinVersion: tls.VersionTLS12, + if redisConfig.TLS { + tlsConfig, err := sdks.CreateTLSConfig(redisConfig) + if err != nil { + return nil, err } + + uo.TLSConfig = tlsConfig } client := redis.NewUniversalClient(uo) ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/bigsegments/store_redis.go b/internal/bigsegments/store_redis.go index 79e0b946..abcc2c99 100644 --- a/internal/bigsegments/store_redis.go +++ b/internal/bigsegments/store_redis.go @@ -2,7 +2,6 @@ package bigsegments import ( "context" - "crypto/tls" "fmt" "log/slog" "strconv" @@ -72,11 +71,15 @@ func newRedisBigSegmentStore( if redisConfig.Username != "" { opts.Username = redisConfig.Username } - if redisConfig.TLS && opts.TLSConfig == nil { - opts.TLSConfig = &tls.Config{ - ServerName: redisConfig.URL.Get().Hostname(), - MinVersion: tls.VersionTLS12, + // GetRedisBasicProperties rewrites redis: to rediss: when TLS is enabled, so ParseURL has already + // set a default TLSConfig (without our CA/client cert). Override it whenever TLS is enabled. + if redisConfig.TLS { + tlsConfig, err := sdks.CreateTLSConfig(redisConfig) + if err != nil { + return nil, err } + + opts.TLSConfig = tlsConfig } store := redisBigSegmentStore{ diff --git a/internal/sdks/data_stores.go b/internal/sdks/data_stores.go index 2f10c17f..4cb9fa59 100644 --- a/internal/sdks/data_stores.go +++ b/internal/sdks/data_stores.go @@ -2,8 +2,12 @@ package sdks import ( "context" + "crypto/tls" + "crypto/x509" "errors" + "fmt" "log/slog" + "os" "strings" "github.com/launchdarkly/ld-relay/v9/config" @@ -156,6 +160,43 @@ func GetRedisBasicProperties( return } +// CreateTLSConfig creates a TLS configuration for Redis based on the provided RedisConfig. +// It returns nil if TLS is not enabled in the configuration. +// If TLS is enabled, it sets up the TLS configuration with the specified server name, minimum version, +// if a client certificate, key and CA file are provided, it loads them into the TLS configuration. +func CreateTLSConfig(config config.RedisConfig) (*tls.Config, error) { + if !config.TLS { + return nil, nil + } + + tlsConfig := &tls.Config{ + ServerName: config.URL.Get().Hostname(), + MinVersion: tls.VersionTLS12, + } + + if config.ClientCertificateFile != "" && config.ClientKeyFile != "" { + cert, err := tls.LoadX509KeyPair(config.ClientCertificateFile, config.ClientKeyFile) + if err != nil { + return nil, err + } + tlsConfig.Certificates = []tls.Certificate{cert} + } + + if config.CAFile != "" { + caCert, err := os.ReadFile(config.CAFile) + if err != nil { + return nil, err + } + caCertPool := x509.NewCertPool() + if !caCertPool.AppendCertsFromPEM(caCert) { + return nil, fmt.Errorf("failed to append CA certificate") + } + tlsConfig.RootCAs = caCertPool + } + + return tlsConfig, nil +} + // getRedisBuilderOptions returns the parameters that the Redis data store and the Redis big segment // store both use. func getRedisBuilderOptions( @@ -170,6 +211,15 @@ func getRedisBuilderOptions( if allConfig.Redis.Username != "" { dialOptions = append(dialOptions, redigo.DialUsername(allConfig.Redis.Username)) } + + tlsOpts, err := CreateTLSConfig(allConfig.Redis) + // TODO: Should the TLS config be validated as part of validateConfigDatabases? + if err != nil { + panic(err) + } + if tlsOpts != nil { + dialOptions = append(dialOptions, redigo.DialUseTLS(true), redigo.DialTLSConfig(tlsOpts)) + } return } From 9b03cd7efd2b1e4ad6b87690ebf24a22ce4ba86d Mon Sep 17 00:00:00 2001 From: Harley Rossetto Date: Sun, 4 Oct 2026 19:08:05 +1100 Subject: [PATCH 2/3] add redis mtls tests capabilities --- integrationtests/big_segments_test.go | 5 + integrationtests/database_params_test.go | 65 ++++++- integrationtests/database_test.go | 4 + internal/bigsegments/store_redis_mtls_test.go | 69 ++++++++ internal/sdks/tls_config_test.go | 76 ++++++++ internal/sharedtest/mtls.go | 163 ++++++++++++++++++ 6 files changed, 378 insertions(+), 4 deletions(-) create mode 100644 internal/bigsegments/store_redis_mtls_test.go create mode 100644 internal/sdks/tls_config_test.go create mode 100644 internal/sharedtest/mtls.go diff --git a/integrationtests/big_segments_test.go b/integrationtests/big_segments_test.go index ec81712f..def170da 100644 --- a/integrationtests/big_segments_test.go +++ b/integrationtests/big_segments_test.go @@ -84,6 +84,11 @@ func testBigSegments(t *testing.T, manager *integrationTestManager) { // of this part is just to make sure connecting with a password also works doBigSegmentsTestWithPreExistingSegment(t, manager, redisWithPasswordDatabaseTestParams) }) + t.Run("Redis with TLS and client certificate (mTLS)", func(t *testing.T) { + // Big segments use a separate Redis client from the data store, so verify it picks up the + // CA and client certificate too. + doBigSegmentsTestWithPreExistingSegment(t, manager, redisMTLSDatabaseTestParams) + }) t.Run("DynamoDB", func(t *testing.T) { doAll(t, dynamoDBDatabaseTestParams) }) diff --git a/integrationtests/database_params_test.go b/integrationtests/database_params_test.go index f6b7b68c..f6a9c6fe 100644 --- a/integrationtests/database_params_test.go +++ b/integrationtests/database_params_test.go @@ -5,11 +5,14 @@ package integrationtests import ( "encoding/json" "fmt" + "os" + "path/filepath" "reflect" "testing" "github.com/launchdarkly/ld-relay/v9/integrationtests/docker" "github.com/launchdarkly/ld-relay/v9/internal/api" + "github.com/launchdarkly/ld-relay/v9/internal/sharedtest" "github.com/stretchr/testify/require" ) @@ -22,16 +25,23 @@ const ( ) type databaseTestParams struct { - dbImageName string - dbDockerParams []string - hostnamePrefix string + dbImageName string + dbDockerParams []string + hostnamePrefix string + // mountFn, if set, is called with the database container's hostname before the container is + // created, and returns a host directory to mount into it. + mountFn func(t *testing.T, m *integrationTestManager, hostname string) (hostDir, containerDir string, err error) setupFn func(*integrationTestManager, *docker.Container) error envVarsFn func(*docker.Container) map[string]string expectedStatusFn func(*docker.Container) api.DataStoreStatusRep } func (p databaseTestParams) withContainer(t *testing.T, manager *integrationTestManager, action func(*docker.Container)) { - manager.withExtraContainer(t, p.dbImageName, p.dbDockerParams, p.hostnamePrefix, func(dbContainer *docker.Container) { + var mountFn func(string) (string, string, error) + if p.mountFn != nil { + mountFn = func(hostname string) (string, string, error) { return p.mountFn(t, manager, hostname) } + } + manager.withExtraContainer(t, p.dbImageName, p.dbDockerParams, p.hostnamePrefix, mountFn, func(dbContainer *docker.Container) { containersOnNetwork, err := manager.dockerNetwork.GetContainerIDs() require.NoError(t, err) require.Len(t, containersOnNetwork, 1, "database container did not start or did not attach to the test network") @@ -155,6 +165,53 @@ var redisWithACLDatabaseTestParams = databaseTestParams{ }, } +// Redis with TLS and required client certificates. The certificates are generated per test run in a +// subdirectory of the Relay shared directory, so Relay sees them under relayContainerSharedDir and the +// Redis container sees them under /tls. +const ( + redisMTLSSubdir = "redis-mtls" + redisMTLSContainerTLSDir = "/tls" + redisMTLSRelayContainerDir = relayContainerSharedDir + "/" + redisMTLSSubdir +) + +var redisMTLSDatabaseTestParams = databaseTestParams{ + dbImageName: "redis", + dbDockerParams: []string{ + "--port", "0", + "--tls-port", "6379", + "--tls-cert-file", redisMTLSContainerTLSDir + "/server.pem", + "--tls-key-file", redisMTLSContainerTLSDir + "/server.key", + "--tls-ca-cert-file", redisMTLSContainerTLSDir + "/ca.pem", + "--tls-auth-clients", "yes", + }, + hostnamePrefix: "redis", + mountFn: func(t *testing.T, m *integrationTestManager, hostname string) (string, string, error) { + hostDir := filepath.Join(m.relaySharedDir, redisMTLSSubdir) + if err := os.MkdirAll(hostDir, 0o755); err != nil { + return "", "", err + } + // The server certificate must be valid for the container hostname, since Relay verifies it. + sharedtest.NewMTLSFilesInDir(t, hostDir, []string{hostname}, nil) + return hostDir, redisMTLSContainerTLSDir, nil + }, + envVarsFn: func(dbContainer *docker.Container) map[string]string { + return map[string]string{ + "USE_REDIS": "true", + "REDIS_HOST": dbContainer.GetName(), + "REDIS_TLS": "true", + "REDIS_CA_FILE": redisMTLSRelayContainerDir + "/ca.pem", + "REDIS_CLIENT_CERT_FILE": redisMTLSRelayContainerDir + "/client.pem", + "REDIS_CLIENT_KEY_FILE": redisMTLSRelayContainerDir + "/client.key", + } + }, + expectedStatusFn: func(dbContainer *docker.Container) api.DataStoreStatusRep { + return api.DataStoreStatusRep{ + Database: "redis", + DBServer: fmt.Sprintf("rediss://%s:6379", dbContainer.GetName()), + } + }, +} + var consulDatabaseTestParams = databaseTestParams{ dbImageName: "hashicorp/consul", hostnamePrefix: "consul", diff --git a/integrationtests/database_test.go b/integrationtests/database_test.go index 22258a1f..6b4a7379 100644 --- a/integrationtests/database_test.go +++ b/integrationtests/database_test.go @@ -27,6 +27,10 @@ func testDatabaseIntegrations(t *testing.T, manager *integrationTestManager) { doDatabaseTest(t, manager, redisWithACLDatabaseTestParams) }) + t.Run("Redis with TLS and client certificate (mTLS)", func(t *testing.T) { + doDatabaseTest(t, manager, redisMTLSDatabaseTestParams) + }) + t.Run("Consul", func(t *testing.T) { doDatabaseTest(t, manager, consulDatabaseTestParams) }) diff --git a/internal/bigsegments/store_redis_mtls_test.go b/internal/bigsegments/store_redis_mtls_test.go new file mode 100644 index 00000000..ee9a0d91 --- /dev/null +++ b/internal/bigsegments/store_redis_mtls_test.go @@ -0,0 +1,69 @@ +package bigsegments + +import ( + "fmt" + "log/slog" + "testing" + "time" + + "github.com/launchdarkly/ld-relay/v9/config" + "github.com/launchdarkly/ld-relay/v9/internal/sharedtest" + + "github.com/launchdarkly/go-configtypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Regression test: with REDIS_TLS=true the URL is rewritten to rediss://, and the parsed default TLS +// config must not mask the configured CA and client certificate. +func TestRedisBigSegmentStoreMTLS(t *testing.T) { + files := sharedtest.NewMTLSFiles(t) + port, handshakes := sharedtest.StartMTLSPingServer(t, files) + + makeConfig := func(mutate func(*config.RedisConfig)) config.RedisConfig { + url, err := configtypes.NewOptURLAbsoluteFromString(fmt.Sprintf("redis://127.0.0.1:%d", port)) + require.NoError(t, err) + c := config.RedisConfig{URL: url, TLS: true} + mutate(&c) + return c + } + logger := slog.New(slog.DiscardHandler) + + t.Run("succeeds with CA and client cert", func(t *testing.T) { + store, err := newRedisBigSegmentStore(makeConfig(func(c *config.RedisConfig) { + c.CAFile = files.CAFile + c.ClientCertificateFile = files.ClientCertFile + c.ClientKeyFile = files.ClientKeyFile + }), config.EnvConfig{}, true, logger) + require.NoError(t, err) + defer store.Close() + assert.NoError(t, awaitHandshake(t, handshakes)) + }) + + t.Run("fails without the CA", func(t *testing.T) { + _, err := newRedisBigSegmentStore(makeConfig(func(c *config.RedisConfig) { + c.ClientCertificateFile = files.ClientCertFile + c.ClientKeyFile = files.ClientKeyFile + }), config.EnvConfig{}, true, logger) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown authority") + }) + + t.Run("fails without a client cert", func(t *testing.T) { + _, err := newRedisBigSegmentStore(makeConfig(func(c *config.RedisConfig) { + c.CAFile = files.CAFile + }), config.EnvConfig{}, true, logger) + require.Error(t, err) + }) +} + +func awaitHandshake(t *testing.T, ch <-chan error) error { + t.Helper() + select { + case err := <-ch: + return err + case <-time.After(5 * time.Second): + t.Fatal("no TLS handshake observed") + return nil + } +} diff --git a/internal/sdks/tls_config_test.go b/internal/sdks/tls_config_test.go new file mode 100644 index 00000000..eb51c8af --- /dev/null +++ b/internal/sdks/tls_config_test.go @@ -0,0 +1,76 @@ +package sdks + +import ( + "testing" + + "github.com/launchdarkly/ld-relay/v9/config" + "github.com/launchdarkly/ld-relay/v9/internal/sharedtest" + + "github.com/launchdarkly/go-configtypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func redisTLSConfig(t *testing.T, mutate func(*config.RedisConfig)) config.RedisConfig { + url, err := configtypes.NewOptURLAbsoluteFromString("rediss://redishost:6380") + require.NoError(t, err) + c := config.RedisConfig{URL: url, TLS: true} + if mutate != nil { + mutate(&c) + } + return c +} + +func TestCreateTLSConfig(t *testing.T) { + files := sharedtest.NewMTLSFiles(t) + + t.Run("nil when TLS disabled", func(t *testing.T) { + c := redisTLSConfig(t, func(c *config.RedisConfig) { c.TLS = false }) + tc, err := CreateTLSConfig(c) + assert.NoError(t, err) + assert.Nil(t, tc) + }) + + t.Run("defaults", func(t *testing.T) { + tc, err := CreateTLSConfig(redisTLSConfig(t, nil)) + require.NoError(t, err) + assert.Equal(t, "redishost", tc.ServerName) + assert.Nil(t, tc.RootCAs) + assert.Empty(t, tc.Certificates) + }) + + t.Run("client cert, key and CA", func(t *testing.T) { + tc, err := CreateTLSConfig(redisTLSConfig(t, func(c *config.RedisConfig) { + c.ClientCertificateFile = files.ClientCertFile + c.ClientKeyFile = files.ClientKeyFile + c.CAFile = files.CAFile + })) + require.NoError(t, err) + assert.Len(t, tc.Certificates, 1) + assert.NotNil(t, tc.RootCAs) + }) + + t.Run("cert without key is ignored", func(t *testing.T) { + tc, err := CreateTLSConfig(redisTLSConfig(t, func(c *config.RedisConfig) { + c.ClientCertificateFile = files.ClientCertFile + })) + require.NoError(t, err) + assert.Empty(t, tc.Certificates) + }) + + t.Run("errors", func(t *testing.T) { + _, err := CreateTLSConfig(redisTLSConfig(t, func(c *config.RedisConfig) { + c.ClientCertificateFile = "/nonexistent.pem" + c.ClientKeyFile = "/nonexistent.key" + })) + assert.Error(t, err) + + _, err = CreateTLSConfig(redisTLSConfig(t, func(c *config.RedisConfig) { c.CAFile = "/nonexistent.pem" })) + assert.Error(t, err) + + _, err = CreateTLSConfig(redisTLSConfig(t, func(c *config.RedisConfig) { + c.CAFile = files.ClientKeyFile // readable, but not a certificate + })) + assert.Error(t, err) + }) +} diff --git a/internal/sharedtest/mtls.go b/internal/sharedtest/mtls.go new file mode 100644 index 00000000..2fbf61eb --- /dev/null +++ b/internal/sharedtest/mtls.go @@ -0,0 +1,163 @@ +package sharedtest + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// MTLSFiles holds paths to PEM files generated for a throwaway CA, a server cert for 127.0.0.1, and a +// client cert signed by that CA. +type MTLSFiles struct { + CAFile string + ClientCertFile string + ClientKeyFile string + ServerCertFile string + ServerKeyFile string + ServerCert tls.Certificate + CAPool *x509.CertPool +} + +// NewMTLSFiles generates a CA and CA-signed server and client certificates in t.TempDir(). The server +// certificate is valid for 127.0.0.1. +func NewMTLSFiles(t *testing.T) MTLSFiles { + t.Helper() + return NewMTLSFilesInDir(t, t.TempDir(), nil, []net.IP{net.ParseIP("127.0.0.1")}) +} + +// NewMTLSFilesInDir is like NewMTLSFiles but writes the PEM files to dir and issues the server +// certificate for the given DNS names and IP addresses. The files are world-readable so that a +// container running as a different user can read them; use only for tests. +func NewMTLSFilesInDir(t *testing.T, dir string, serverDNSNames []string, serverIPs []net.IP) MTLSFiles { + t.Helper() + + caKey, caCert, caDER := issueCert(t, nil, nil, "test-ca", true, nil, nil) + + srvKey, _, srvDER := issueCert(t, caCert, caKey, "server", false, serverDNSNames, serverIPs) + cliKey, _, cliDER := issueCert(t, caCert, caKey, "client", false, nil, nil) + + srvKeyDER, err := x509.MarshalECPrivateKey(srvKey) + require.NoError(t, err) + cliKeyDER, err := x509.MarshalECPrivateKey(cliKey) + require.NoError(t, err) + + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caDER}) + srvCertPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srvDER}) + srvKeyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: srvKeyDER}) + srvPair, err := tls.X509KeyPair(srvCertPEM, srvKeyPEM) + require.NoError(t, err) + + pool := x509.NewCertPool() + require.True(t, pool.AppendCertsFromPEM(caPEM)) + + write := func(name string, data []byte) string { + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, data, 0o644)) + return p + } + return MTLSFiles{ + CAFile: write("ca.pem", caPEM), + ClientCertFile: write("client.pem", pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cliDER})), + ClientKeyFile: write("client.key", pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: cliKeyDER})), + ServerCertFile: write("server.pem", srvCertPEM), + ServerKeyFile: write("server.key", srvKeyPEM), + ServerCert: srvPair, + CAPool: pool, + } +} + +func issueCert( + t *testing.T, + parent *x509.Certificate, + parentKey *ecdsa.PrivateKey, + cn string, + isCA bool, + dnsNames []string, + ips []net.IP, +) (*ecdsa.PrivateKey, *x509.Certificate, []byte) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + serial, err := rand.Int(rand.Reader, big.NewInt(1<<62)) + require.NoError(t, err) + tmpl := &x509.Certificate{ + SerialNumber: serial, + Subject: pkix.Name{CommonName: cn}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, + BasicConstraintsValid: true, + IsCA: isCA, + DNSNames: dnsNames, + IPAddresses: ips, + } + if isCA { + tmpl.KeyUsage |= x509.KeyUsageCertSign + } + signer, signerKey := tmpl, key + if parent != nil { + signer, signerKey = parent, parentKey + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, signer, &key.PublicKey, signerKey) + require.NoError(t, err) + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + return key, cert, der +} + +// StartMTLSPingServer starts a TLS listener on 127.0.0.1 that requires a client certificate signed by +// files.CAPool and answers every read with a Redis simple-string reply. It returns the listening port +// and a channel that receives one error (nil on success) per handshake attempt. +func StartMTLSPingServer(t *testing.T, files MTLSFiles) (port int, handshakes <-chan error) { + t.Helper() + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{ + Certificates: []tls.Certificate{files.ServerCert}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: files.CAPool, + MinVersion: tls.VersionTLS12, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + + ch := make(chan error, 100) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func(c net.Conn) { + defer c.Close() + err := c.(*tls.Conn).Handshake() + select { + case ch <- err: + default: + } + if err != nil { + return + } + buf := make([]byte, 1024) + for { + if _, err := c.Read(buf); err != nil { + return + } + _, _ = c.Write([]byte("+PONG\r\n")) + } + }(conn) + } + }() + return ln.Addr().(*net.TCPAddr).Port, ch +} From 92c5abf8879c459c5ceadf29e69adc413bf66489 Mon Sep 17 00:00:00 2001 From: Harley Rossetto Date: Sun, 4 Oct 2026 20:23:22 +1100 Subject: [PATCH 3/3] address initial cursor feedback --- config/config.go | 13 ++++ config/config_validation.go | 13 ++++ config/redis_tls_test.go | 15 ++++ config/test_data_configs_invalid_test.go | 17 +++++ config/test_data_configs_valid_test.go | 28 +++++++ internal/autoconfigcache/redis_store.go | 4 +- .../autoconfigcache/redis_store_mtls_test.go | 75 +++++++++++++++++++ internal/bigsegments/store_redis.go | 6 +- internal/sdks/big_segments.go | 5 +- internal/sdks/data_stores.go | 14 ++-- internal/sdks/tls_config_test.go | 36 ++++++++- 11 files changed, 214 insertions(+), 12 deletions(-) create mode 100644 config/redis_tls_test.go create mode 100644 internal/autoconfigcache/redis_store_mtls_test.go diff --git a/config/config.go b/config/config.go index 79e11145..d4565bd1 100644 --- a/config/config.go +++ b/config/config.go @@ -264,6 +264,19 @@ type RedisConfig struct { AtomicUpsert bool `conf:"REDIS_ATOMIC_UPSERT"` } +// TLSEnabled is true if TLS was requested either with the TLS option or with a rediss:// URL. +func (c RedisConfig) TLSEnabled() bool { + if c.TLS { + return true + } + return c.URL.IsDefined() && strings.EqualFold(c.URL.Get().Scheme, "rediss") +} + +// hasTLSFiles is true if any certificate, key or CA file option is set. +func (c RedisConfig) hasTLSFiles() bool { + return c.ClientCertificateFile != "" || c.ClientKeyFile != "" || c.CAFile != "" +} + // ConsulConfig configures the optional Consul integration. // // Consul is enabled if Host is non-empty. diff --git a/config/config_validation.go b/config/config_validation.go index 2f22ea47..21526a1e 100644 --- a/config/config_validation.go +++ b/config/config_validation.go @@ -24,6 +24,7 @@ var ( errOTLPNegativeCardinalityLimit = errors.New("metrics cardinality limit must not be negative; use 0 for no limit (OTEL_METRICS_CARDINALITY_LIMIT)") errRedisURLWithHostAndPort = errors.New("please specify Redis URL or host/port, but not both") + errRedisClientCertWithoutKey = errors.New("REDIS_CLIENT_CERT_FILE and REDIS_CLIENT_KEY_FILE must be specified together") errRedisBadHostname = errors.New("invalid Redis hostname") errConsulTokenAndTokenFile = errors.New("Consul token must be specified as either an inline value or a file, but not both") //nolint:staticcheck errCacheKeyWithoutStore = errors.New("AUTO_CONFIG_CACHE_KEY requires Redis or DynamoDB to be enabled") @@ -32,6 +33,9 @@ var ( errInvalidCredentialCleanupInterval = fmt.Errorf("expired credential cleanup interval must be >= %s", minimumCredentialCleanupInterval) ) +const warnRedisTLSFilesWithoutTLS = "Redis client certificate, key or CA file was set, but TLS is not enabled " + + "(use REDIS_TLS or a rediss:// URL); these settings will be ignored" + const warnMetricsCapacityBelowMinimum = "configured usage metrics event capacity of %d is below the minimum of %d; using %[2]d instead" func warnUnrecognizedSignalExporter(varName, value string) string { @@ -236,6 +240,15 @@ func validateConfigDatabases(result *ct.ValidationResult, c *Config, logger *slo return // no point doing further database config validation if it's in this state } + if c.Redis.URL.IsDefined() { + if (c.Redis.ClientCertificateFile == "") != (c.Redis.ClientKeyFile == "") { + result.AddError(nil, errRedisClientCertWithoutKey) + } + if c.Redis.hasTLSFiles() && !c.Redis.TLSEnabled() { + logger.Warn(warnRedisTLSFilesWithoutTLS) + } + } + if c.Consul.Host != "" { if c.Consul.Token != "" && c.Consul.TokenFile != "" { result.AddError(nil, errConsulTokenAndTokenFile) diff --git a/config/redis_tls_test.go b/config/redis_tls_test.go new file mode 100644 index 00000000..8934397c --- /dev/null +++ b/config/redis_tls_test.go @@ -0,0 +1,15 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRedisConfigTLSEnabled(t *testing.T) { + assert.False(t, RedisConfig{}.TLSEnabled()) + assert.False(t, RedisConfig{URL: newOptURLAbsoluteMustBeValid("redis://host:6379")}.TLSEnabled()) + assert.True(t, RedisConfig{URL: newOptURLAbsoluteMustBeValid("redis://host:6379"), TLS: true}.TLSEnabled()) + assert.True(t, RedisConfig{URL: newOptURLAbsoluteMustBeValid("rediss://host:6379")}.TLSEnabled()) + assert.True(t, RedisConfig{URL: newOptURLAbsoluteMustBeValid("REDISS://host:6379")}.TLSEnabled()) +} diff --git a/config/test_data_configs_invalid_test.go b/config/test_data_configs_invalid_test.go index 3f48dd44..3f6ad1bd 100644 --- a/config/test_data_configs_invalid_test.go +++ b/config/test_data_configs_invalid_test.go @@ -36,6 +36,7 @@ func makeInvalidConfigs() []testDataInvalidConfig { makeInvalidConfigRedisInvalidDockerPort(), makeInvalidConfigRedisConflictingParams(), makeInvalidConfigRedisNoPrefix(), + makeInvalidConfigRedisClientCertWithoutKey(), makeInvalidConfigRedisAutoConfNoPrefix(), makeInvalidConfigConsulNoPrefix(), makeInvalidConfigConsulAutoConfNoPrefix(), @@ -322,6 +323,22 @@ Url = "http://redishost:6400" return c } +func makeInvalidConfigRedisClientCertWithoutKey() testDataInvalidConfig { + c := testDataInvalidConfig{name: "Redis - client certificate without key"} + c.envVarsError = errRedisClientCertWithoutKey.Error() + c.envVars = map[string]string{ + "USE_REDIS": "1", + "REDIS_URL": "rediss://localhost:6379", + "REDIS_CLIENT_CERT_FILE": "/certs/client.pem", + } + c.fileContent = ` +[Redis] +URL = rediss://localhost:6379 +ClientCertificateFile = /certs/client.pem +` + return c +} + func makeInvalidConfigRedisNoPrefix() testDataInvalidConfig { c := testDataInvalidConfig{name: "Redis - multiple environments, prefix not defined"} c.envVarsError = errEnvWithoutDBDisambiguation("env2", false).Error() diff --git a/config/test_data_configs_valid_test.go b/config/test_data_configs_valid_test.go index d14abc09..1e961df9 100644 --- a/config/test_data_configs_valid_test.go +++ b/config/test_data_configs_valid_test.go @@ -83,6 +83,7 @@ func makeValidConfigs() []testDataValidConfig { makeValidConfigOfflineModeWithMonitoringInterval("5m"), makeValidConfigRedisMinimal(), makeValidConfigRedisAll(), + makeValidConfigRedisMTLS(), makeValidConfigRedisURL(), makeValidConfigRedisPortOnly(), makeValidConfigRedisDockerPort(), @@ -475,6 +476,33 @@ AtomicUpsert = true return c } +func makeValidConfigRedisMTLS() testDataValidConfig { + c := testDataValidConfig{name: "Redis - mTLS files"} + c.makeConfig = func(c *Config) { + c.Redis = RedisConfig{ + URL: newOptURLAbsoluteMustBeValid("rediss://redishost:6400"), + ClientCertificateFile: "/certs/client.pem", + ClientKeyFile: "/certs/client.key", + CAFile: "/certs/ca.pem", + } + } + c.envVars = map[string]string{ + "USE_REDIS": "1", + "REDIS_URL": "rediss://redishost:6400", + "REDIS_CLIENT_CERT_FILE": "/certs/client.pem", + "REDIS_CLIENT_KEY_FILE": "/certs/client.key", + "REDIS_CA_FILE": "/certs/ca.pem", + } + c.fileContent = ` +[Redis] +Url = "rediss://redishost:6400" +ClientCertificateFile = "/certs/client.pem" +ClientKeyFile = "/certs/client.key" +CAFile = "/certs/ca.pem" +` + return c +} + func makeValidConfigRedisURL() testDataValidConfig { c := testDataValidConfig{name: "Redis - URL instead of host/port"} c.makeConfig = func(c *Config) { diff --git a/internal/autoconfigcache/redis_store.go b/internal/autoconfigcache/redis_store.go index 659dba1e..77984ed2 100644 --- a/internal/autoconfigcache/redis_store.go +++ b/internal/autoconfigcache/redis_store.go @@ -43,7 +43,9 @@ func newRedisStore(redisConfig config.RedisConfig, cacheKey string, encKey []byt if redisConfig.Username != "" { uo.Username = redisConfig.Username } - if redisConfig.TLS { + // ParseURL sets a default TLSConfig for rediss:// URLs but none for redis://, so apply ours + // whenever TLS is enabled by either the TLS option or the URL scheme. + if redisConfig.TLSEnabled() { tlsConfig, err := sdks.CreateTLSConfig(redisConfig) if err != nil { return nil, err diff --git a/internal/autoconfigcache/redis_store_mtls_test.go b/internal/autoconfigcache/redis_store_mtls_test.go new file mode 100644 index 00000000..931d0b95 --- /dev/null +++ b/internal/autoconfigcache/redis_store_mtls_test.go @@ -0,0 +1,75 @@ +package autoconfigcache + +import ( + "context" + "fmt" + "log/slog" + "testing" + "time" + + "github.com/launchdarkly/ld-relay/v9/config" + "github.com/launchdarkly/ld-relay/v9/internal/sharedtest" + + "github.com/launchdarkly/go-configtypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedisStoreMTLS(t *testing.T) { + files := sharedtest.NewMTLSFiles(t) + port, handshakes := sharedtest.StartMTLSPingServer(t, files) + logger := slog.New(slog.DiscardHandler) + + makeStore := func(t *testing.T, scheme string, mutate func(*config.RedisConfig)) Store { + url, err := configtypes.NewOptURLAbsoluteFromString(fmt.Sprintf("%s://127.0.0.1:%d", scheme, port)) + require.NoError(t, err) + c := config.RedisConfig{URL: url} + mutate(&c) + store, err := newRedisStore(c, "cache-key", make([]byte, 32), logger) + require.NoError(t, err) + t.Cleanup(func() { _ = store.Close() }) + return store + } + withFiles := func(c *config.RedisConfig) { + c.CAFile = files.CAFile + c.ClientCertificateFile = files.ClientCertFile + c.ClientKeyFile = files.ClientKeyFile + } + getAll := func(store Store) error { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := store.GetAll(ctx) + return err + } + awaitHandshake := func(t *testing.T) error { + select { + case err := <-handshakes: + return err + case <-time.After(5 * time.Second): + t.Fatal("no TLS handshake observed") + return nil + } + } + + t.Run("redis URL with TLS option uses CA and client cert", func(t *testing.T) { + store := makeStore(t, "redis", func(c *config.RedisConfig) { c.TLS = true; withFiles(c) }) + _ = getAll(store) + assert.NoError(t, awaitHandshake(t)) + }) + + t.Run("rediss URL uses CA and client cert without the TLS option", func(t *testing.T) { + store := makeStore(t, "rediss", withFiles) + _ = getAll(store) + assert.NoError(t, awaitHandshake(t)) + }) + + t.Run("fails without the CA", func(t *testing.T) { + store := makeStore(t, "rediss", func(c *config.RedisConfig) { + c.ClientCertificateFile = files.ClientCertFile + c.ClientKeyFile = files.ClientKeyFile + }) + err := getAll(store) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown authority") + }) +} diff --git a/internal/bigsegments/store_redis.go b/internal/bigsegments/store_redis.go index abcc2c99..9e0c0015 100644 --- a/internal/bigsegments/store_redis.go +++ b/internal/bigsegments/store_redis.go @@ -71,9 +71,9 @@ func newRedisBigSegmentStore( if redisConfig.Username != "" { opts.Username = redisConfig.Username } - // GetRedisBasicProperties rewrites redis: to rediss: when TLS is enabled, so ParseURL has already - // set a default TLSConfig (without our CA/client cert). Override it whenever TLS is enabled. - if redisConfig.TLS { + // ParseURL sets a default TLSConfig (without our CA/client cert) for rediss:// URLs, and + // GetRedisBasicProperties rewrites redis: to rediss: when TLS is enabled. Override it whenever TLS is enabled. + if redisConfig.TLSEnabled() { tlsConfig, err := sdks.CreateTLSConfig(redisConfig) if err != nil { return nil, err diff --git a/internal/sdks/big_segments.go b/internal/sdks/big_segments.go index 8b70e329..635b2283 100644 --- a/internal/sdks/big_segments.go +++ b/internal/sdks/big_segments.go @@ -25,7 +25,10 @@ func ConfigureBigSegments( var storeFactory subsystems.ComponentConfigurer[subsystems.BigSegmentStore] if allConfig.Redis.URL.IsDefined() { - redisURL, prefix, dialOptions := getRedisBuilderOptions(allConfig, envConfig) + redisURL, prefix, dialOptions, err := getRedisBuilderOptions(allConfig, envConfig) + if err != nil { + return nil, err + } redisBuilder := ldredis.BigSegmentStore(). URL(redisURL). Prefix(prefix). diff --git a/internal/sdks/data_stores.go b/internal/sdks/data_stores.go index 4cb9fa59..a65425f3 100644 --- a/internal/sdks/data_stores.go +++ b/internal/sdks/data_stores.go @@ -60,7 +60,10 @@ func ConfigureDataStore( if allConfig.Redis.URL.IsDefined() { // Our config validation already takes care of normalizing the Redis parameters so that if a // host & port were specified, they are transformed into a URL. - redisURL, prefix, dialOptions := getRedisBuilderOptions(allConfig, envConfig) + redisURL, prefix, dialOptions, err := getRedisBuilderOptions(allConfig, envConfig) + if err != nil { + return nil, DataStoreEnvironmentInfo{}, err + } upsertMode := ldredis.UpsertModeWatch if allConfig.Redis.AtomicUpsert { upsertMode = ldredis.UpsertModeAtomicScript @@ -161,11 +164,11 @@ func GetRedisBasicProperties( } // CreateTLSConfig creates a TLS configuration for Redis based on the provided RedisConfig. -// It returns nil if TLS is not enabled in the configuration. +// It returns nil if TLS is not enabled in the configuration (neither REDIS_TLS nor a rediss:// URL). // If TLS is enabled, it sets up the TLS configuration with the specified server name, minimum version, // if a client certificate, key and CA file are provided, it loads them into the TLS configuration. func CreateTLSConfig(config config.RedisConfig) (*tls.Config, error) { - if !config.TLS { + if !config.TLSEnabled() { return nil, nil } @@ -202,7 +205,7 @@ func CreateTLSConfig(config config.RedisConfig) (*tls.Config, error) { func getRedisBuilderOptions( allConfig config.Config, envConfig config.EnvConfig, -) (redisURL, prefix string, dialOptions []redigo.DialOption) { +) (redisURL, prefix string, dialOptions []redigo.DialOption, err error) { redisURL, prefix = GetRedisBasicProperties(allConfig.Redis, envConfig) if allConfig.Redis.Password != "" { @@ -213,9 +216,8 @@ func getRedisBuilderOptions( } tlsOpts, err := CreateTLSConfig(allConfig.Redis) - // TODO: Should the TLS config be validated as part of validateConfigDatabases? if err != nil { - panic(err) + return "", "", nil, err } if tlsOpts != nil { dialOptions = append(dialOptions, redigo.DialUseTLS(true), redigo.DialTLSConfig(tlsOpts)) diff --git a/internal/sdks/tls_config_test.go b/internal/sdks/tls_config_test.go index eb51c8af..cab533a0 100644 --- a/internal/sdks/tls_config_test.go +++ b/internal/sdks/tls_config_test.go @@ -1,6 +1,7 @@ package sdks import ( + "log/slog" "testing" "github.com/launchdarkly/ld-relay/v9/config" @@ -25,12 +26,33 @@ func TestCreateTLSConfig(t *testing.T) { files := sharedtest.NewMTLSFiles(t) t.Run("nil when TLS disabled", func(t *testing.T) { - c := redisTLSConfig(t, func(c *config.RedisConfig) { c.TLS = false }) + c := redisTLSConfig(t, func(c *config.RedisConfig) { + c.TLS = false + c.URL, _ = configtypes.NewOptURLAbsoluteFromString("redis://redishost:6380") + }) tc, err := CreateTLSConfig(c) assert.NoError(t, err) assert.Nil(t, tc) }) + t.Run("rediss URL enables TLS without the TLS option", func(t *testing.T) { + c := redisTLSConfig(t, func(c *config.RedisConfig) { + c.TLS = false + c.CAFile = files.CAFile + }) + tc, err := CreateTLSConfig(c) + require.NoError(t, err) + require.NotNil(t, tc) + assert.NotNil(t, tc.RootCAs) + }) + + t.Run("redis URL without the TLS option stays plaintext", func(t *testing.T) { + url, _ := configtypes.NewOptURLAbsoluteFromString("redis://redishost:6380") + tc, err := CreateTLSConfig(config.RedisConfig{URL: url, CAFile: files.CAFile}) + assert.NoError(t, err) + assert.Nil(t, tc) + }) + t.Run("defaults", func(t *testing.T) { tc, err := CreateTLSConfig(redisTLSConfig(t, nil)) require.NoError(t, err) @@ -74,3 +96,15 @@ func TestCreateTLSConfig(t *testing.T) { assert.Error(t, err) }) } + +func TestConfigureDataStoreRedisTLSFileErrorIsReturned(t *testing.T) { + url, _ := configtypes.NewOptURLAbsoluteFromString("rediss://redishost:6380") + c := config.Config{Redis: config.RedisConfig{URL: url, CAFile: "/nonexistent.pem"}} + logger := slog.New(slog.DiscardHandler) + + _, _, err := ConfigureDataStore(c, config.EnvConfig{}, logger) + assert.Error(t, err) + + _, err = ConfigureBigSegments(c, config.EnvConfig{}, logger) + assert.Error(t, err) +}