Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Redis server TLS support #104

Merged
merged 8 commits into from
Nov 20, 2024
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ require (
github.com/roadrunner-server/errors v1.4.1
go.opentelemetry.io/otel/sdk v1.32.0
go.uber.org/zap v1.27.0
golang.org/x/sys v0.27.0
)

require (
Expand All @@ -27,5 +28,4 @@ require (
go.opentelemetry.io/otel/metric v1.32.0 // indirect
go.opentelemetry.io/otel/trace v1.32.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/sys v0.27.0 // indirect
)
4 changes: 4 additions & 0 deletions go.work.sum
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ github.com/spf13/viper v1.17.0 h1:I5txKw7MJasPL/BrfkbA0Jyo/oELqVmux4pR/UxOMfI=
github.com/spf13/viper v1.17.0/go.mod h1:BmMMMLQXSbcHK6KAOiFLz0l5JHrU89OdIRHvsk0+yVI=
github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
github.com/tklauser/go-sysconf v0.3.12 h1:0QaGUFOdQaIVdPgfITYzaTegZvdCjmYO52cSFAEVmqU=
github.com/tklauser/go-sysconf v0.3.12/go.mod h1:Ho14jnntGE1fpdOqQEEaiKRpvIavV0hSfmBq8nJbHYI=
github.com/tklauser/go-sysconf v0.3.14 h1:g5vzr9iPFFz24v2KZXs/pvpvh8/V9Fw6vQK5ZZb78yU=
Expand Down Expand Up @@ -259,6 +260,7 @@ golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/mod v0.19.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY=
golang.org/x/net v0.15.0 h1:ugBLEUaxABaB5AJqW9enI0ACdci2RUd4eP51NTBvuJ8=
golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk=
golang.org/x/net v0.23.0 h1:7EYJ93RZ9vYSZAIb2x3lnuvqO5zneoD6IvWjuhfxjTs=
Expand All @@ -271,6 +273,7 @@ golang.org/x/oauth2 v0.7.0/go.mod h1:hPLQkd9LyjfXTiRohC/41GhcFqxisoUQ99sCUOHO9x4
golang.org/x/oauth2 v0.18.0 h1:09qnuIAgzdx1XplqJvW6CQqMCtGZykZWcXzPMPUusvI=
golang.org/x/oauth2 v0.18.0/go.mod h1:Wf7knwG0MPoWIMMBgFlEaSUDaKskp0dCfrlJRJXbBi8=
golang.org/x/oauth2 v0.20.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI=
golang.org/x/sync v0.9.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 h1:v+OssWQX+hTHEmOBgwxdZxK4zHq3yOs8F9J7mk0PY8E=
golang.org/x/term v0.21.0 h1:WVXCp+/EBEHOj53Rvu+7KiT/iElMrO8ACK16SMZ3jaA=
golang.org/x/term v0.21.0/go.mod h1:ooXLefLobQVslOqselCNF4SxFAaoS6KujMbsGzSDmX0=
Expand All @@ -284,6 +287,7 @@ golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU=
golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA=
golang.org/x/tools v0.22.0/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c=
golang.org/x/tools v0.23.0/go.mod h1:pnu6ufv6vQkll6szChhK3C3L/ruaIv5eBeztNG8wtsI=
golang.org/x/tools v0.27.0/go.mod h1:sUi0ZgbwW9ZPAq26Ekut+weQPR5eIM6GQLQ1Yjm1H0Q=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 h1:H2TDz8ibqkAF6YGhCdN3jS9O0/s90v0rJh3X/OLHEUk=
golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8=
Expand Down
1 change: 1 addition & 0 deletions kv/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type Config struct {
IdleTimeout time.Duration `mapstructure:"idle_timeout"`
IdleCheckFreq time.Duration `mapstructure:"idle_check_freq"`
ReadOnly bool `mapstructure:"read_only"`
TLSConfig *TLSConfig `mapstructure:"tls"`
}

// InitDefaults initializing fill config with default values
Expand Down
6 changes: 6 additions & 0 deletions kv/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ func NewRedisDriver(log *zap.Logger, key string, cfgPlugin Configurer, tracer *s

d.cfg.InitDefaults()

tlsConfig, err := tlsConfig(d.cfg.TLSConfig)
if err != nil {
return nil, errors.E(op, err)
}

d.universalClient = redis.NewUniversalClient(&redis.UniversalOptions{
Addrs: d.cfg.Addrs,
DB: d.cfg.DB,
Expand All @@ -74,6 +79,7 @@ func NewRedisDriver(log *zap.Logger, key string, cfgPlugin Configurer, tracer *s
RouteByLatency: d.cfg.RouteByLatency,
RouteRandomly: d.cfg.RouteRandomly,
MasterName: d.cfg.MasterName,
TLSConfig: tlsConfig,
})

err = redisotel.InstrumentMetrics(d.universalClient)
Expand Down
130 changes: 130 additions & 0 deletions kv/tlsconfig.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package kv

import (
"crypto/tls"
"crypto/x509"
"os"

"golang.org/x/sys/cpu"
)

type TLSConfig struct {
Cert string `mapstructure:"cert"`
Key string `mapstructure:"key"`
RootCa string `mapstructure:"root_ca"`
}
Comment on lines +11 to +15
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Align struct with PR objectives and add documentation

The TLSConfig struct needs to be updated to match the PR objectives and documentation requirements:

  1. Field names should match configuration keys
  2. Missing min_version field
  3. Missing documentation for struct and fields
  4. Unexplained addition of cert and key fields
+// TLSConfig contains TLS/SSL configuration for Redis connections
 type TLSConfig struct {
-    Cert   string `mapstructure:"cert"`
-    Key    string `mapstructure:"key"`
-    RootCa string `mapstructure:"root_ca"`
+    // MinVersion specifies minimum TLS version (1.0, 1.1, 1.2, 1.3)
+    MinVersion string `mapstructure:"min_version"`
+    // CAFile is the path to PEM file containing CA certificates
+    CAFile     string `mapstructure:"ca_file"`
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type TLSConfig struct {
Cert string `mapstructure:"cert"`
Key string `mapstructure:"key"`
RootCa string `mapstructure:"root_ca"`
}
// TLSConfig contains TLS/SSL configuration for Redis connections
type TLSConfig struct {
// MinVersion specifies minimum TLS version (1.0, 1.1, 1.2, 1.3)
MinVersion string `mapstructure:"min_version"`
// CAFile is the path to PEM file containing CA certificates
CAFile string `mapstructure:"ca_file"`
}


func tlsConfig(conf *TLSConfig) (*tls.Config, error) {
if conf == nil {
return nil, nil
}

tlsConfig := defaultTLSConfig(conf)
if conf.RootCa != "" {
// error is always nil here
certPool, err := x509.SystemCertPool()
if err != nil {
// error is always nil here
return nil, err
}

if certPool == nil {
certPool = x509.NewCertPool()
}

// we already checked this file in the config.go
rca, err := os.ReadFile(conf.RootCa)
if err != nil {
return nil, err
}

if ok := certPool.AppendCertsFromPEM(rca); !ok {
return nil, err
}

tlsConfig.RootCAs = certPool
}

if _, crtExistErr := os.Stat(conf.RootCa); crtExistErr != nil {
return nil, crtExistErr
}

return tlsConfig, nil
}
Comment on lines +17 to +53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix critical issues in TLS configuration logic

Several critical issues need to be addressed:

  1. Certificate existence check is performed after reading the file
  2. Undefined error variable used in return statement
  3. Missing TLS version configuration support
  4. Unnecessary error comments
 func tlsConfig(conf *TLSConfig) (*tls.Config, error) {
     if conf == nil {
         return nil, nil
     }
 
+    // Check certificate existence first
+    if conf.CAFile != "" {
+        if _, err := os.Stat(conf.CAFile); err != nil {
+            return nil, fmt.Errorf("ca_file error: %w", err)
+        }
+    }
+
     tlsConfig := defaultTLSConfig(conf)
-    if conf.RootCa != "" {
-        // error is always nil here
+
+    // Configure TLS version
+    if conf.MinVersion != "" {
+        version, err := parseTLSVersion(conf.MinVersion)
+        if err != nil {
+            return nil, err
+        }
+        tlsConfig.MinVersion = version
+    }
+
+    if conf.CAFile != "" {
         certPool, err := x509.SystemCertPool()
         if err != nil {
-            // error is always nil here
             return nil, err
         }
 
@@ -31,12 +31,11 @@
             certPool = x509.NewCertPool()
         }
 
-        // we already checked this file in the config.go
-        rca, err := os.ReadFile(conf.RootCa)
+        rca, err := os.ReadFile(conf.CAFile)
         if err != nil {
             return nil, err
         }
 
         if ok := certPool.AppendCertsFromPEM(rca); !ok {
-            return nil, err
+            return nil, fmt.Errorf("failed to append CA certificate")
         }
 
         tlsConfig.RootCAs = certPool
     }
 
-    if _, crtExistErr := os.Stat(conf.RootCa); crtExistErr != nil {
-        return nil, crtExistErr
-    }
-
     return tlsConfig, nil
 }

Committable suggestion skipped: line range outside the PR's diff.


func defaultTLSConfig(cfg *TLSConfig) *tls.Config {
var topCipherSuites []uint16
var defaultCipherSuitesTLS13 []uint16

hasGCMAsmAMD64 := cpu.X86.HasAES && cpu.X86.HasPCLMULQDQ
hasGCMAsmARM64 := cpu.ARM64.HasAES && cpu.ARM64.HasPMULL
// Keep in sync with crypto/aes/cipher_s390x.go.
hasGCMAsmS390X := cpu.S390X.HasAES && cpu.S390X.HasAESCBC && cpu.S390X.HasAESCTR && (cpu.S390X.HasGHASH || cpu.S390X.HasAESGCM)

hasGCMAsm := hasGCMAsmAMD64 || hasGCMAsmARM64 || hasGCMAsmS390X

if hasGCMAsm {
// If AES-GCM hardware is provided then priorities AES-GCM
// cipher suites.
topCipherSuites = []uint16{
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
}
defaultCipherSuitesTLS13 = []uint16{
tls.TLS_AES_128_GCM_SHA256,
tls.TLS_CHACHA20_POLY1305_SHA256,
tls.TLS_AES_256_GCM_SHA384,
}
} else {
// Without AES-GCM hardware, we put the ChaCha20-Poly1305
// cipher suites first.
topCipherSuites = []uint16{
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
}
defaultCipherSuitesTLS13 = []uint16{
tls.TLS_CHACHA20_POLY1305_SHA256,
tls.TLS_AES_128_GCM_SHA256,
tls.TLS_AES_256_GCM_SHA384,
}
}

defaultCipherSuites := make([]uint16, 0, 22)
defaultCipherSuites = append(defaultCipherSuites, topCipherSuites...)
defaultCipherSuites = append(defaultCipherSuites, defaultCipherSuitesTLS13...)

return &tls.Config{
CurvePreferences: []tls.CurveID{
tls.X25519,
tls.CurveP256,
tls.CurveP384,
tls.CurveP521,
},
GetClientCertificate: getClientCertificate(cfg),
CipherSuites: defaultCipherSuites,
MinVersion: tls.VersionTLS12,
}
}
rustatian marked this conversation as resolved.
Show resolved Hide resolved

// getClientCertificate is used for tls.Config struct field GetClientCertificate and enables re-fetching the client certificates when they expire
func getClientCertificate(cfg *TLSConfig) func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
if cfg == nil || cfg.Cert == "" || cfg.Key == "" {
return nil
}
return func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
cert, err := tls.LoadX509KeyPair(cfg.Cert, cfg.Key)
if err != nil {
return nil, err
}

return &cert, nil
}
}