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.

Sign up for GitHub

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
Merged

feature: Redis server TLS support #104

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c00a4ed
Implemented verification of redis server certificates.
sicet7 Nov 19, 2024
107f23b
Removed "min_version" from configuration as it should always be 1.2
sicet7 Nov 19, 2024
8df4fd1
Added warning when empty cert pool is defaulted too.
sicet7 Nov 19, 2024
2086670
Fixed warning always being produced even without tls enabled.
sicet7 Nov 19, 2024
f656464
Added better error message when PEM parsing fails.
sicet7 Nov 19, 2024
4b64f14
Moved implementation to "tlsconfig.go" also implemented suggestions.
sicet7 Nov 19, 2024
8abbcc6
chore: some refactoring
rustatian Nov 19, 2024
c63b869
Fixed to set "GetClientCertificate" to nil when cert or key not provi…
sicet7 Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions kv/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ 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"`
Copy link
Member

Choose a reason for hiding this comment

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

Should be a pointer: *TLSConfig to distinguish between empty and non set values.

}

type TLSConfig struct {
CaFile string `mapstructure:"ca_file"`
rustatian marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Also, please, follow the standard RR configuration naming for the TLS options, example: https://github.com/roadrunner-server/grpc/blob/master/tests/configs/.rr-grpc-rq-tls-rootca.yaml#L20

}

// InitDefaults initializing fill config with default values
Expand Down
37 changes: 35 additions & 2 deletions kv/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ package kv

import (
"context"
"crypto/tls"
"crypto/x509"
stderr "errors"
"os"
"strings"
"time"
"unsafe"
Expand Down Expand Up @@ -53,7 +56,7 @@ func NewRedisDriver(log *zap.Logger, key string, cfgPlugin Configurer, tracer *s

d.cfg.InitDefaults()

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

if d.cfg.TLSConfig.CaFile != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Check for nil

tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
}
rustatian marked this conversation as resolved.
Show resolved Hide resolved

rootCAs, sysCertErr := x509.SystemCertPool()
if sysCertErr != nil {
rootCAs = x509.NewCertPool()
d.log.Warn("unable to load system certificate pool, using empty pool", zap.Error(sysCertErr))
}

if _, crtExistErr := os.Stat(d.cfg.TLSConfig.CaFile); crtExistErr != nil {
return nil, errors.E(op, crtExistErr)
}

bytes, crtReadErr := os.ReadFile(d.cfg.TLSConfig.CaFile)
if crtReadErr != nil {
return nil, errors.E(op, crtReadErr)
}

if !rootCAs.AppendCertsFromPEM(bytes) {
return nil, errors.E(op, errors.Errorf("failed to parse certificates from PEM file '%s'. Please ensure the file contains valid PEM-encoded certificates", d.cfg.TLSConfig.CaFile))
}

tlsConfig.RootCAs = rootCAs
redisOptions.TLSConfig = tlsConfig
}
Copy link
Member

Choose a reason for hiding this comment

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

It is better to move TLS configuration to the separate package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry i am new to go, when you say new package do you mean in another repo or simply in another directory?? 😅

Copy link
Member

Choose a reason for hiding this comment

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

Sure, np, I'll help you 😃
Just a new file called tlsconfig.go or similar :)


d.universalClient = redis.NewUniversalClient(redisOptions)

err = redisotel.InstrumentMetrics(d.universalClient)
if err != nil {
Expand Down