Skip to content

Commit 185f6ab

Browse files
ofekshenawaofekshenawa
committed
feat: improve TLS URL parameters with snake_case naming
Building on Ben Weissmann's original implementation, this commit adds: - Snake_case parameter names (addressing reviewer feedback): * tls_cert_file and tls_key_file (instead of TLSCertPEMFile/TLSKeyPEMFile) * tls_min_version and tls_max_version (instead of TLSMinVersion/TLSMaxVersion) * tls_server_name (instead of ServerName) - Improved error messages for better user experience - Updated test cases to use snake_case parameters - Removed redundant tls_insecure_skip_verify (use existing skip_verify) - Enhanced documentation with clear parameter descriptions This addresses all reviewer feedback from PR #2076 while maintaining the core functionality and comprehensive test coverage.
1 parent 2e2225e commit 185f6ab

File tree

2 files changed

+31
-24
lines changed

2 files changed

+31
-24
lines changed

‎options.go‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,10 @@ func NewDialer(opt *Options) func(context.Context, string, string) (net.Conn, er
358358
// names will be treated as unknown parameters
359359
// - unknown parameter names will result in an error
360360
// - use "skip_verify=true" to ignore TLS certificate validation
361+
// - for rediss:// URLs, additional TLS parameters are supported:
362+
// - tls_cert_file and tls_key_file: paths to client certificate and key files
363+
// - tls_min_version and tls_max_version: TLS version constraints (e.g., 771 for TLS 1.2)
364+
// - tls_server_name: override server name for certificate validation
361365
//
362366
// Examples:
363367
//
@@ -577,32 +581,35 @@ func setupConnParams(u *url.URL, o *Options) (*Options, error) {
577581
}
578582

579583
if u.Scheme == "rediss" {
580-
tlsCertPEMFile := q.string("TLSCertPEMFile")
581-
tlsKeyPEMFile := q.string("TLSKeyPEMFile")
584+
tlsCertFile := q.string("tls_cert_file")
585+
tlsKeyFile := q.string("tls_key_file")
582586

583-
if (tlsCertPEMFile == "") != (tlsKeyPEMFile == "") {
584-
return nil, fmt.Errorf("redis: TLSCertPEMFile and TLSKeyPEMFile URL parameters must be both set or both omitted")
587+
if (tlsCertFile == "") != (tlsKeyFile == "") {
588+
return nil, fmt.Errorf("redis: tls_cert_file and tls_key_file URL parameters must be both set or both omitted")
585589
}
586590

587-
if tlsCertPEMFile != "" {
588-
cert, certLoadErr := tls.LoadX509KeyPair(tlsCertPEMFile, tlsKeyPEMFile)
591+
if tlsCertFile != "" {
592+
cert, certLoadErr := tls.LoadX509KeyPair(tlsCertFile, tlsKeyFile)
589593
if certLoadErr != nil {
590-
return nil, fmt.Errorf("redis: Error loading X509 Key Pair: %w", certLoadErr)
594+
return nil, fmt.Errorf("redis: error loading TLS certificate: %w", certLoadErr)
591595
}
592596

593597
o.TLSConfig.Certificates = []tls.Certificate{cert}
594598
}
595599

596-
o.TLSConfig.MinVersion = uint16(q.int("TLSMinVersion"))
597-
o.TLSConfig.MaxVersion = uint16(q.int("TLSMaxVersion"))
598-
o.TLSConfig.InsecureSkipVerify = q.bool("TLSInsecureSkipVerify")
600+
if q.has("tls_min_version") {
601+
o.TLSConfig.MinVersion = uint16(q.int("tls_min_version"))
602+
}
603+
if q.has("tls_max_version") {
604+
o.TLSConfig.MaxVersion = uint16(q.int("tls_max_version"))
605+
}
599606

600-
serverNameOverride := q.string("ServerName")
601-
if serverNameOverride != "" {
607+
tlsServerName := q.string("tls_server_name")
608+
if tlsServerName != "" {
602609
// we explicitly check for this query parameter, so we don't overwrite
603610
// the default server name (the hostname of the Redis server) if it's
604611
// not given
605-
o.TLSConfig.ServerName = serverNameOverride
612+
o.TLSConfig.ServerName = tlsServerName
606613
}
607614
}
608615
if q.err != nil {

‎options_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,27 +50,27 @@ EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
5050
o: &Options{Addr: "12345:6379"},
5151
}, {
5252
url: "rediss://localhost:123",
53-
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost"}},
53+
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12}},
5454
}, {
55-
url: "rediss://localhost:123?ServerName=abc&TLSMinVersion=1&TLSMaxVersion=3&TLSInsecureSkipVerify=true",
55+
url: "rediss://localhost:123?tls_server_name=abc&tls_min_version=1&tls_max_version=3&skip_verify=true",
5656
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "abc", MinVersion: 1, MaxVersion: 3, InsecureSkipVerify: true}},
5757
}, {
58-
url: "rediss://localhost:123?TLSCertPEMFile=./testdata/testcert.pem&TLSKeyPEMFile=./testdata/testkey.pem",
59-
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", Certificates: []tls.Certificate{testCert}}},
58+
url: "rediss://localhost:123?tls_cert_file=./testdata/testcert.pem&tls_key_file=./testdata/testkey.pem",
59+
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12, Certificates: []tls.Certificate{testCert}}},
6060
}, {
61-
url: "rediss://localhost:123?TLSCertPEMFile=./testdata/doesnotexist.pem&TLSKeyPEMFile=./testdata/testkey.pem",
61+
url: "rediss://localhost:123?tls_cert_file=./testdata/doesnotexist.pem&tls_key_file=./testdata/testkey.pem",
6262
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "abc"}},
63-
err: errors.New("redis: Error loading X509 Key Pair: open ./testdata/doesnotexist.pem: no such file or directory"),
63+
err: errors.New("redis: error loading TLS certificate: open ./testdata/doesnotexist.pem: no such file or directory"),
6464
}, {
65-
url: "rediss://localhost:123?TLSCertPEMFile=./testdata/testcert.pem",
65+
url: "rediss://localhost:123?tls_cert_file=./testdata/testcert.pem",
6666
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "abc"}},
67-
err: errors.New("redis: TLSCertPEMFile and TLSKeyPEMFile URL parameters must be both set or both omitted"),
67+
err: errors.New("redis: tls_cert_file and tls_key_file URL parameters must be both set or both omitted"),
6868
}, {
69-
url: "rediss://localhost:123?TLSKeyPEMFile=./testdata/testkey.pem",
70-
err: errors.New("redis: TLSCertPEMFile and TLSKeyPEMFile URL parameters must be both set or both omitted"),
69+
url: "rediss://localhost:123?tls_key_file=./testdata/testkey.pem",
70+
err: errors.New("redis: tls_cert_file and tls_key_file URL parameters must be both set or both omitted"),
7171
}, {
7272
url: "rediss://localhost:123/?skip_verify=true",
73-
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", InsecureSkipVerify: true}},
73+
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12, InsecureSkipVerify: true}},
7474
}, {
7575
url: "redis://:bar@localhost:123",
7676
o: &Options{Addr: "localhost:123", Password: "bar"},

0 commit comments

Comments
 (0)