diff --git a/CHANGELOG.md b/CHANGELOG.md index a9d1f524..72d615e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - [#3072](https://github.com/oauth2-proxy/oauth2-proxy/pull/3072) feat: support for multiple github orgs #3072 (@daniel-mersch) - [#3116](https://github.com/oauth2-proxy/oauth2-proxy/pull/3116) feat: bump to go1.24.5 and full dependency update (@wardviaene / @dolmen) - [#3097](https://github.com/oauth2-proxy/oauth2-proxy/pull/3097) chore(deps): update alpine base image to v3.22.0 +- [#3101](https://github.com/oauth2-proxy/oauth2-proxy/pull/3101) fix: return error for empty Redis URL list (@dgivens) # V7.9.0 diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index e41a1e1f..18d79b80 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -218,6 +218,10 @@ func setupTLSConfig(opts options.RedisStoreOptions, opt *redis.Options) error { // parseRedisURLs parses a list of redis urls and returns a list // 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) { + if len(urls) == 0 { + return nil, nil, fmt.Errorf("unable to parse redis urls: no redis urls provided") + } + addrs := []string{} var redisOptions *redis.Options for _, u := range urls { diff --git a/pkg/sessions/redis/redis_store_test.go b/pkg/sessions/redis/redis_store_test.go index 0c0c5106..1bff6855 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -19,116 +19,41 @@ const ( ) var _ = Describe("Redis SessionStore Tests", func() { - // helper interface to allow us to close client connections - // All non-nil redis clients should implement this - type closer interface { - Close() error - } - - var mr *miniredis.Miniredis - var ss sessionsapi.SessionStore - - BeforeEach(func() { - var err error - mr, err = miniredis.Run() - Expect(err).ToNot(HaveOccurred()) - }) - - AfterEach(func() { - mr.Close() - }) - - JustAfterEach(func() { - // Release any connections immediately after the test ends - if redisManager, ok := ss.(*persistence.Manager); ok { - if redisManager.Store.(*SessionStore).Client != nil { - Expect(redisManager.Store.(*SessionStore).Client.(closer).Close()).To(Succeed()) - } + Describe("Redis SessionStore Creation", func() { + // helper interface to allow us to close client connections + // All non-nil redis clients should implement this + type closer interface { + Close() error } - }) - const redisProtocol = "redis://" - tests.RunSessionStoreTests( - func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - // Set the connection URL - opts.Type = options.RedisSessionStoreType - opts.Redis.ConnectionURL = redisProtocol + mr.Addr() + var mr *miniredis.Miniredis + var ss sessionsapi.SessionStore - // Capture the session store so that we can close the client + BeforeEach(func() { var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) - return ss, err - }, - func(d time.Duration) error { - mr.FastForward(d) - return nil - }, - ) - - Context("with sentinel", func() { - var ms *minisentinel.Sentinel - - BeforeEach(func() { - ms = minisentinel.NewSentinel(mr) - Expect(ms.Start()).To(Succeed()) - }) - - tests.RunSessionStoreTests( - func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - // Set the sentinel connection URL - sentinelAddr := redisProtocol + ms.Addr() - opts.Type = options.RedisSessionStoreType - opts.Redis.SentinelConnectionURLs = []string{sentinelAddr} - opts.Redis.UseSentinel = true - opts.Redis.SentinelMasterName = ms.MasterInfo().Name - - // 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 cluster", func() { - tests.RunSessionStoreTests( - func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - clusterAddr := redisProtocol + mr.Addr() - opts.Type = options.RedisSessionStoreType - opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = 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 a redis password", func() { - BeforeEach(func() { - mr.RequireAuth(redisPassword) + mr, err = miniredis.Run() + Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { - mr.RequireAuth("") + mr.Close() }) + JustAfterEach(func() { + // Release any connections immediately after the test ends + if redisManager, ok := ss.(*persistence.Manager); ok { + if redisManager.Store.(*SessionStore).Client != nil { + Expect(redisManager.Store.(*SessionStore).Client.(closer).Close()).To(Succeed()) + } + } + }) + + const redisProtocol = "redis://" tests.RunSessionStoreTests( func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { // Set the connection URL opts.Type = options.RedisSessionStoreType opts.Redis.ConnectionURL = redisProtocol + mr.Addr() - opts.Redis.Password = redisPassword // Capture the session store so that we can close the client var err error @@ -157,7 +82,6 @@ var _ = Describe("Redis SessionStore Tests", func() { opts.Redis.SentinelConnectionURLs = []string{sentinelAddr} opts.Redis.UseSentinel = true opts.Redis.SentinelMasterName = ms.MasterInfo().Name - opts.Redis.Password = redisPassword // Capture the session store so that we can close the client var err error @@ -178,6 +102,33 @@ var _ = Describe("Redis SessionStore Tests", func() { opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} opts.Redis.UseCluster = 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 a redis password", func() { + BeforeEach(func() { + mr.RequireAuth(redisPassword) + }) + + AfterEach(func() { + mr.RequireAuth("") + }) + + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + // Set the connection URL + opts.Type = options.RedisSessionStoreType + opts.Redis.ConnectionURL = redisProtocol + mr.Addr() opts.Redis.Password = redisPassword // Capture the session store so that we can close the client @@ -190,56 +141,134 @@ var _ = Describe("Redis SessionStore Tests", func() { return nil }, ) + + Context("with sentinel", func() { + var ms *minisentinel.Sentinel + + BeforeEach(func() { + ms = minisentinel.NewSentinel(mr) + Expect(ms.Start()).To(Succeed()) + }) + + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + // Set the sentinel connection URL + sentinelAddr := redisProtocol + ms.Addr() + opts.Type = options.RedisSessionStoreType + opts.Redis.SentinelConnectionURLs = []string{sentinelAddr} + opts.Redis.UseSentinel = true + opts.Redis.SentinelMasterName = ms.MasterInfo().Name + opts.Redis.Password = redisPassword + + // 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 cluster", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + clusterAddr := redisProtocol + mr.Addr() + opts.Type = options.RedisSessionStoreType + opts.Redis.ClusterConnectionURLs = []string{clusterAddr} + opts.Redis.UseCluster = true + opts.Redis.Password = redisPassword + + // 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 a redis username and password", func() { + BeforeEach(func() { + mr.RequireUserAuth(redisUsername, redisPassword) + }) + + AfterEach(func() { + mr.RequireUserAuth("", "") + }) + + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + // Set the connection URL + opts.Type = options.RedisSessionStoreType + opts.Redis.ConnectionURL = "redis://" + redisUsername + "@" + mr.Addr() + opts.Redis.Password = redisPassword + + // 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 cluster", func() { + tests.RunSessionStoreTests( + func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { + clusterAddr := "redis://" + redisUsername + "@" + mr.Addr() + opts.Type = options.RedisSessionStoreType + opts.Redis.ClusterConnectionURLs = []string{clusterAddr} + opts.Redis.UseCluster = true + opts.Redis.Username = redisUsername + opts.Redis.Password = redisPassword + + // 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 a redis username and password", func() { - BeforeEach(func() { - mr.RequireUserAuth(redisUsername, redisPassword) + Describe("Redis URL Parsing", func() { + It("should parse valid redis URL", func() { + addrs, opts, err := parseRedisURLs([]string{"redis://localhost:6379"}) + Expect(err).ToNot(HaveOccurred()) + Expect(addrs).To(Equal([]string{"localhost:6379"})) + Expect(opts).ToNot(BeNil()) + Expect(opts.Addr).To(Equal("localhost:6379")) }) - AfterEach(func() { - mr.RequireUserAuth("", "") + It("should return error for invalid redis URL", func() { + addrs, opts, err := parseRedisURLs([]string{"invalid://url"}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("unable to parse redis url")) + Expect(err.Error()).To(Not(ContainSubstring("no redis urls provided"))) + Expect(addrs).To(BeNil()) + Expect(opts).To(BeNil()) }) - tests.RunSessionStoreTests( - func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - // Set the connection URL - opts.Type = options.RedisSessionStoreType - opts.Redis.ConnectionURL = "redis://" + redisUsername + "@" + mr.Addr() - opts.Redis.Password = redisPassword - - // 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 cluster", func() { - tests.RunSessionStoreTests( - func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - clusterAddr := "redis://" + redisUsername + "@" + mr.Addr() - opts.Type = options.RedisSessionStoreType - opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true - opts.Redis.Username = redisUsername - opts.Redis.Password = redisPassword - - // 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 - }, - ) + It("should return error when no URLs provided", func() { + addrs, opts, err := parseRedisURLs([]string{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("unable to parse redis urls: no redis urls provided")) + Expect(addrs).To(BeNil()) + Expect(opts).To(BeNil()) }) }) })