From c2f7598b3569526fc3fdcdb3ef36ae67a0c5770e Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 16 Oct 2026 12:22:15 -0400 Subject: [PATCH] feat(redisconn): add CACertFile to TLSConfig Consumers that configure Redis TLS from files had to read and validate the CA bundle themselves before populating CACert. Add CACertFile so BuildTLSConfig loads it, rejecting missing, empty, and malformed files instead of falling back to system roots. CACert and CACertFile are mutually exclusive. Config.Validate still avoids filesystem access; the file is read when the TLS configuration is built. InsecureSkipVerify and CACertFile gain json/yaml tags so the type can be used in serialized configuration. Co-Authored-By: Claude Sonnet 5.5 --- redisconn/client.go | 26 +++++++++++++++++++++-- redisconn/client_test.go | 46 ++++++++++++++++++++++++++++++++++++++++ redisconn/config.go | 19 +++++++++++++++-- 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/redisconn/client.go b/redisconn/client.go index b0f9d7b..8d07d1c 100644 --- a/redisconn/client.go +++ b/redisconn/client.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "net" + "os" "slices" "time" @@ -51,9 +52,13 @@ func BuildTLSConfig(cfg *TLSConfig) (*tls.Config, error) { MinVersion: tls.VersionTLS12, InsecureSkipVerify: cfg.InsecureSkipVerify, //nolint:gosec // explicitly configurable } - if len(cfg.CACert) > 0 { + caCert, err := loadCACert(cfg) + if err != nil { + return nil, err + } + if len(caCert) > 0 { pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(cfg.CACert) { + if !pool.AppendCertsFromPEM(caCert) { return nil, errors.New("redis: failed to parse CA certificate PEM data") } tlsCfg.RootCAs = pool @@ -68,6 +73,23 @@ func BuildTLSConfig(cfg *TLSConfig) (*tls.Config, error) { return tlsCfg, nil } +// loadCACert returns the configured CA bundle: CACertFile's contents when set, +// otherwise CACert. An unreadable or empty CACertFile is an error so a +// misconfigured path cannot silently select system roots. +func loadCACert(cfg *TLSConfig) ([]byte, error) { + if cfg.CACertFile == "" { + return cfg.CACert, nil + } + data, err := os.ReadFile(cfg.CACertFile) // #nosec G304 -- path comes from trusted configuration + if err != nil { + return nil, fmt.Errorf("redis: failed to read CA certificate file %q: %w", cfg.CACertFile, err) + } + if len(data) == 0 { + return nil, fmt.Errorf("redis: CA certificate file %q is empty", cfg.CACertFile) + } + return data, nil +} + func buildClient(cfg *Config) (goredis.UniversalClient, error) { switch { case cfg.SentinelConfig != nil: diff --git a/redisconn/client_test.go b/redisconn/client_test.go index 47ad7b8..ff0dec7 100644 --- a/redisconn/client_test.go +++ b/redisconn/client_test.go @@ -14,6 +14,8 @@ import ( "encoding/pem" "math/big" "net" + "os" + "path/filepath" "runtime" "strings" "testing" @@ -294,6 +296,50 @@ func TestBuildTLSConfig(t *testing.T) { } } +func TestBuildTLSConfigCACertFile(t *testing.T) { + t.Parallel() + caPEM, _, _ := testCertificate(t) + dir := t.TempDir() + write := func(name string, data []byte) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + return path + } + valid := write("valid.pem", caPEM) + tests := []struct { + name string + cfg *TLSConfig + want string + }{ + {"valid file", &TLSConfig{CACertFile: valid}, ""}, + {"missing file", &TLSConfig{CACertFile: filepath.Join(dir, "missing.pem")}, "failed to read CA certificate file"}, + {"empty file", &TLSConfig{CACertFile: write("empty.pem", nil)}, "is empty"}, + {"malformed file", &TLSConfig{CACertFile: write("bad.pem", []byte("bad"))}, "failed to parse CA"}, + {"file and bytes", &TLSConfig{CACert: caPEM, CACertFile: valid}, "mutually exclusive"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := BuildTLSConfig(tt.cfg) + if tt.want != "" { + if err == nil || !contains(err.Error(), tt.want) { + t.Fatalf("error = %v", err) + } + return + } + if err != nil { + t.Fatal(err) + } + if got.RootCAs == nil { + t.Fatal("CA file did not replace system roots") + } + }) + } +} + func TestSentinelDynamicProviderStaysOnDataNodeOptions(t *testing.T) { t.Parallel() provider := func(context.Context) (string, string, error) { return "user", "token", nil } diff --git a/redisconn/config.go b/redisconn/config.go index d3e76e3..4cfbfd3 100644 --- a/redisconn/config.go +++ b/redisconn/config.go @@ -80,9 +80,16 @@ type SentinelConfig struct { // TLSConfig configures server verification, custom roots, and mutual TLS. type TLSConfig struct { // InsecureSkipVerify disables server certificate verification. - InsecureSkipVerify bool + InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty" yaml:"insecureSkipVerify,omitempty"` // CACert is an optional PEM CA bundle; empty uses system roots. + // It is mutually exclusive with CACertFile. CACert []byte + // CACertFile is an optional path to a PEM CA bundle, read when the TLS + // configuration is built. When set, the bundle replaces the system roots. + // A missing, empty, or malformed file is an error; it never falls back to + // system roots or plaintext. The file is read once, so changes require a + // new client. It is mutually exclusive with CACert. + CACertFile string `json:"caCertFile,omitempty" yaml:"caCertFile,omitempty"` // ClientCert and ClientKey are a PEM mTLS certificate pair. ClientCert []byte ClientKey []byte @@ -153,10 +160,18 @@ func validateDynamicAuth(c *Config) error { return nil } +// validateTLSConfig checks the configuration without touching the filesystem; +// CACertFile is read and checked by BuildTLSConfig. func validateTLSConfig(cfg *TLSConfig) error { - if cfg != nil && (len(cfg.ClientCert) == 0) != (len(cfg.ClientKey) == 0) { + if cfg == nil { + return nil + } + if (len(cfg.ClientCert) == 0) != (len(cfg.ClientKey) == 0) { return errors.New("client certificate and key must be provided together") } + if len(cfg.CACert) > 0 && cfg.CACertFile != "" { + return errors.New("CACert and CACertFile are mutually exclusive") + } return nil }