Merge pull request oauth2-proxy#1403 from openstandia/fix-redis-tls

Improve TLS handling for Redis to support non-standalone mode with TLS

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
 

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
 }

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()
+			mr.Close()
+
+			var err error
+			mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}})
 			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())
+		})
+		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()
 
-			certFile, err := os.CreateTemp("", "cert.*.pem")
+			var err error
+			mr, err = miniredis.RunTLS(&tls.Config{Certificates: []tls.Certificate{cert}})
 			Expect(err).ToNot(HaveOccurred())
-			caPath = certFile.Name()
-			_, err = certFile.Write(certData)
-			defer certFile.Close()
+		})
+		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())
 		})