From 7eb3a4fbd5ed424eccfcd6988126ee3e52185d61 Mon Sep 17 00:00:00 2001 From: Hiroyuki Wada Date: Wed, 13 Oct 2021 00:19:30 +0900 Subject: [PATCH] Improve TLS handling for Redis to support non-standalone mode with TLS --- CHANGELOG.md | 1 + pkg/sessions/redis/redis_store.go | 46 +++++-- pkg/sessions/redis/redis_store_test.go | 177 ++++++++++++++++++++----- 3 files changed, 181 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bf7aee4..cc5d215d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ - [#997](https://github.com/oauth2-proxy/oauth2-proxy/pull/997) Allow passing the raw url path when proxying upstream requests - e.g. /%2F/ (@FStelzer) - [#1147](https://github.com/oauth2-proxy/oauth2-proxy/pull/1147) Multiarch support for docker image (@goshlanguage) - [#1296](https://github.com/oauth2-proxy/oauth2-proxy/pull/1296) Fixed `panic` when connecting to Redis with TLS (@mstrzele) +- [#1403](https://github.com/oauth2-proxy/oauth2-proxy/pull/1403) Improve TLS handling for Redis to support non-standalone mode with TLS (@wadahiro) # V7.1.3 diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index a90fe494..f1c798fa 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -89,28 +89,40 @@ func NewRedisClient(opts options.RedisStoreOptions) (Client, error) { // buildSentinelClient makes a redis.Client that connects to Redis Sentinel // for Primary/Replica Redis node coordination func buildSentinelClient(opts options.RedisStoreOptions) (Client, error) { - addrs, err := parseRedisURLs(opts.SentinelConnectionURLs) + addrs, opt, err := parseRedisURLs(opts.SentinelConnectionURLs) if err != nil { return nil, fmt.Errorf("could not parse redis urls: %v", err) } + + if err := setupTLSConfig(opts, opt); err != nil { + return nil, err + } + client := redis.NewFailoverClient(&redis.FailoverOptions{ MasterName: opts.SentinelMasterName, SentinelAddrs: addrs, SentinelPassword: opts.SentinelPassword, Password: opts.Password, + TLSConfig: opt.TLSConfig, }) return newClient(client), nil } // buildClusterClient makes a redis.Client that is Redis Cluster aware func buildClusterClient(opts options.RedisStoreOptions) (Client, error) { - addrs, err := parseRedisURLs(opts.ClusterConnectionURLs) + addrs, opt, err := parseRedisURLs(opts.ClusterConnectionURLs) if err != nil { return nil, fmt.Errorf("could not parse redis urls: %v", err) } + + if err := setupTLSConfig(opts, opt); err != nil { + return nil, err + } + client := redis.NewClusterClient(&redis.ClusterOptions{ - Addrs: addrs, - Password: opts.Password, + Addrs: addrs, + Password: opts.Password, + TLSConfig: opt.TLSConfig, }) return newClusterClient(client), nil } @@ -127,6 +139,16 @@ func buildStandaloneClient(opts options.RedisStoreOptions) (Client, error) { opt.Password = opts.Password } + if err := setupTLSConfig(opts, opt); err != nil { + return nil, err + } + + client := redis.NewClient(opt) + return newClient(client), nil +} + +// setupTLSConfig sets the TLSConfig if the TLS option is given in redis.Options +func setupTLSConfig(opts options.RedisStoreOptions, opt *redis.Options) error { if opts.InsecureSkipTLSVerify { if opt.TLSConfig == nil { /* #nosec */ @@ -146,7 +168,7 @@ func buildStandaloneClient(opts options.RedisStoreOptions) (Client, error) { } certs, err := ioutil.ReadFile(opts.CAPath) if err != nil { - return nil, fmt.Errorf("failed to load %q, %v", opts.CAPath, err) + return fmt.Errorf("failed to load %q, %v", opts.CAPath, err) } // Append our cert to the system pool @@ -161,21 +183,21 @@ func buildStandaloneClient(opts options.RedisStoreOptions) (Client, error) { opt.TLSConfig.RootCAs = rootCAs } - - client := redis.NewClient(opt) - return newClient(client), nil + return nil } // parseRedisURLs parses a list of redis urls and returns a list -// of addresses in the form of host:port that can be used to connect to Redis -func parseRedisURLs(urls []string) ([]string, error) { +// of addresses in the form of host:port and redis.Options that can be used to connect to Redis +func parseRedisURLs(urls []string) ([]string, *redis.Options, error) { addrs := []string{} + var redisOptions *redis.Options for _, u := range urls { parsedURL, err := redis.ParseURL(u) if err != nil { - return nil, fmt.Errorf("unable to parse redis url: %v", err) + return nil, nil, fmt.Errorf("unable to parse redis url: %v", err) } addrs = append(addrs, parsedURL.Addr) + redisOptions = parsedURL } - return addrs, nil + return addrs, redisOptions, nil } diff --git a/pkg/sessions/redis/redis_store_test.go b/pkg/sessions/redis/redis_store_test.go index acaba855..f5ced7e2 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -47,6 +47,35 @@ func TestSessionStore(t *testing.T) { RunSpecs(t, "Redis SessionStore") } +var ( + cert tls.Certificate + caPath string +) + +var _ = BeforeSuite(func() { + var err error + certBytes, keyBytes, err := util.GenerateCert() + Expect(err).ToNot(HaveOccurred()) + certOut := new(bytes.Buffer) + Expect(pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: certBytes})).To(Succeed()) + certData := certOut.Bytes() + keyOut := new(bytes.Buffer) + Expect(pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: keyBytes})).To(Succeed()) + cert, err = tls.X509KeyPair(certData, keyOut.Bytes()) + Expect(err).ToNot(HaveOccurred()) + + certFile, err := os.CreateTemp("", "cert.*.pem") + Expect(err).ToNot(HaveOccurred()) + caPath = certFile.Name() + _, err = certFile.Write(certData) + defer certFile.Close() + Expect(err).ToNot(HaveOccurred()) +}) + +var _ = AfterSuite(func() { + Expect(os.Remove(caPath)).ToNot(HaveOccurred()) +}) + var _ = Describe("Redis SessionStore Tests", func() { // helper interface to allow us to close client connections // All non-nil redis clients should implement this @@ -229,36 +258,130 @@ var _ = Describe("Redis SessionStore Tests", func() { }) }) - Context("with custom CA path", func() { - var caPath string - + Context("with TLS connection", func() { BeforeEach(func() { - certBytes, keyBytes, err := util.GenerateCert() - Expect(err).ToNot(HaveOccurred()) - certOut := new(bytes.Buffer) - Expect(pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: certBytes})).To(Succeed()) - certData := certOut.Bytes() - keyOut := new(bytes.Buffer) - Expect(pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: keyBytes})).To(Succeed()) - cert, err := tls.X509KeyPair(certData, keyOut.Bytes()) - Expect(err).ToNot(HaveOccurred()) - - certFile, err := os.CreateTemp("", "cert.*.pem") - Expect(err).ToNot(HaveOccurred()) - caPath = certFile.Name() - _, err = certFile.Write(certData) - defer certFile.Close() - Expect(err).ToNot(HaveOccurred()) - mr.Close() + var err error + mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mr.Close() + + var err error + mr, err = miniredis.Run() + Expect(err).ToNot(HaveOccurred()) + }) + + Context("with standalone", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + // Set the connection URL + opts.Type = options.RedisSessionStoreType + opts.Redis.ConnectionURL = "rediss://" + mr.Addr() + opts.Redis.CAPath = caPath + + // Capture the session store so that we can close the client + ss, err := NewRedisSessionStore(opts, cookieOpts) + return ss, err + }, + func(d time.Duration) error { + mr.FastForward(d) + return nil + }, + ) + }) + + Context("with cluster", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + clusterAddr := "rediss://" + mr.Addr() + opts.Type = options.RedisSessionStoreType + opts.Redis.ClusterConnectionURLs = []string{clusterAddr} + opts.Redis.UseCluster = true + opts.Redis.CAPath = caPath + + // Capture the session store so that we can close the client + var err error + ss, err = NewRedisSessionStore(opts, cookieOpts) + return ss, err + }, + func(d time.Duration) error { + mr.FastForward(d) + return nil + }, + ) + }) + }) + + Context("with insecure TLS connection", func() { + BeforeEach(func() { + mr.Close() + + var err error + mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mr.Close() + + var err error + mr, err = miniredis.Run() + Expect(err).ToNot(HaveOccurred()) + }) + + Context("with standalone", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + // Set the connection URL + opts.Type = options.RedisSessionStoreType + opts.Redis.ConnectionURL = "rediss://" + mr.Addr() + opts.Redis.InsecureSkipTLSVerify = true + + // Capture the session store so that we can close the client + ss, err := NewRedisSessionStore(opts, cookieOpts) + return ss, err + }, + func(d time.Duration) error { + mr.FastForward(d) + return nil + }, + ) + }) + + Context("with cluster", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + clusterAddr := "rediss://" + mr.Addr() + opts.Type = options.RedisSessionStoreType + opts.Redis.ClusterConnectionURLs = []string{clusterAddr} + opts.Redis.UseCluster = true + opts.Redis.InsecureSkipTLSVerify = true + + // Capture the session store so that we can close the client + var err error + ss, err = NewRedisSessionStore(opts, cookieOpts) + return ss, err + }, + func(d time.Duration) error { + mr.FastForward(d) + return nil + }, + ) + }) + }) + + Context("with custom CA path", func() { + BeforeEach(func() { + mr.Close() + + var err error mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}}) Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { - Expect(os.Remove(caPath)).ToNot(HaveOccurred()) - mr.Close() var err error @@ -287,17 +410,9 @@ var _ = Describe("Redis SessionStore Tests", func() { Context("with insecure TLS connection", func() { BeforeEach(func() { - certBytes, keyBytes, err := util.GenerateCert() - Expect(err).ToNot(HaveOccurred()) - certOut := new(bytes.Buffer) - Expect(pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: certBytes})).To(Succeed()) - keyOut := new(bytes.Buffer) - Expect(pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: keyBytes})).To(Succeed()) - cert, err := tls.X509KeyPair(certOut.Bytes(), keyOut.Bytes()) - Expect(err).ToNot(HaveOccurred()) - mr.Close() + var err error mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}}) Expect(err).ToNot(HaveOccurred()) })