diff --git a/pkg/apis/options/sessions.go b/pkg/apis/options/sessions.go index 3b2a4d19..7ff656fe 100644 --- a/pkg/apis/options/sessions.go +++ b/pkg/apis/options/sessions.go @@ -1,12 +1,9 @@ package options -import "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" - // SessionOptions contains configuration options for the SessionStore providers. type SessionOptions struct { - Type string `flag:"session-store-type" cfg:"session_store_type"` - Cipher encryption.Cipher `cfg:",internal"` - Redis RedisStoreOptions `cfg:",squash"` + Type string `flag:"session-store-type" cfg:"session_store_type"` + Redis RedisStoreOptions `cfg:",squash"` } // CookieSessionStoreType is used to indicate the CookieSessionStore should be diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 62f6b348..77dc1504 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -126,9 +126,9 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string, // NewCookieSessionStore initialises a new instance of the SessionStore from // the configuration given -func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) { +func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions, cipher encryption.Cipher) (sessions.SessionStore, error) { return &SessionStore{ - CookieCipher: opts.Cipher, + CookieCipher: cipher, CookieOptions: cookieOpts, }, nil } diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index 2d0fd9b0..36298b9f 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -39,7 +39,7 @@ type SessionStore struct { // NewRedisSessionStore initialises a new instance of the SessionStore from // the configuration given -func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) { +func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions, cipher encryption.Cipher) (sessions.SessionStore, error) { client, err := newRedisCmdable(opts.Redis) if err != nil { return nil, fmt.Errorf("error constructing redis client: %v", err) @@ -47,7 +47,7 @@ func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cook rs := &SessionStore{ Client: client, - CookieCipher: opts.Cipher, + CookieCipher: cipher, CookieOptions: cookieOpts, } return rs, nil diff --git a/pkg/sessions/redis/redis_store_test.go b/pkg/sessions/redis/redis_store_test.go index e7b801dd..70f35f42 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -1,6 +1,7 @@ package redis import ( + "crypto/rand" "net/http" "net/http/httptest" "net/url" @@ -10,11 +11,19 @@ import ( "github.com/alicebob/miniredis/v2" "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestRedisStore(t *testing.T) { + secret := make([]byte, 32) + _, err := rand.Read(secret) + assert.NoError(t, err) + + cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(string(secret))) + assert.NoError(t, err) + t.Run("save session on redis standalone", func(t *testing.T) { redisServer, err := miniredis.Run() require.NoError(t, err) @@ -25,7 +34,7 @@ func TestRedisStore(t *testing.T) { Host: redisServer.Addr(), } opts.Session.Redis.ConnectionURL = redisURL.String() - redisStore, err := NewRedisSessionStore(&opts.Session, &opts.Cookie) + redisStore, err := NewRedisSessionStore(&opts.Session, &opts.Cookie, cipher) require.NoError(t, err) err = redisStore.Save( httptest.NewRecorder(), @@ -49,7 +58,7 @@ func TestRedisStore(t *testing.T) { opts.Session.Redis.SentinelConnectionURLs = []string{sentinelURL.String()} opts.Session.Redis.UseSentinel = true opts.Session.Redis.SentinelMasterName = sentinel.MasterInfo().Name - redisStore, err := NewRedisSessionStore(&opts.Session, &opts.Cookie) + redisStore, err := NewRedisSessionStore(&opts.Session, &opts.Cookie, cipher) require.NoError(t, err) err = redisStore.Save( httptest.NewRecorder(), diff --git a/pkg/sessions/session_store.go b/pkg/sessions/session_store.go index 992d6ded..c0054ba6 100644 --- a/pkg/sessions/session_store.go +++ b/pkg/sessions/session_store.go @@ -5,17 +5,22 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/cookie" "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/redis" ) // NewSessionStore creates a SessionStore from the provided configuration func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) { + cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret)) + if err != nil { + return nil, fmt.Errorf("error initialising cipher: %v", err) + } switch opts.Type { case options.CookieSessionStoreType: - return cookie.NewCookieSessionStore(opts, cookieOpts) + return cookie.NewCookieSessionStore(opts, cookieOpts, cipher) case options.RedisSessionStoreType: - return redis.NewRedisSessionStore(opts, cookieOpts) + return redis.NewRedisSessionStore(opts, cookieOpts, cipher) default: return nil, fmt.Errorf("unknown session store type '%s'", opts.Type) } diff --git a/pkg/sessions/session_store_test.go b/pkg/sessions/session_store_test.go index 1a60aa0d..4287ae4c 100644 --- a/pkg/sessions/session_store_test.go +++ b/pkg/sessions/session_store_test.go @@ -353,24 +353,11 @@ var _ = Describe("NewSessionStore", func() { SameSite: "strict", } - var err error - ss, err = sessions.NewSessionStore(opts, cookieOpts) - Expect(err).ToNot(HaveOccurred()) - }) - - SessionStoreInterfaceTests(persistent) - }) - - Context("with a cipher", func() { - BeforeEach(func() { + // A secret is required but not defaulted secret := make([]byte, 32) _, err := rand.Read(secret) Expect(err).ToNot(HaveOccurred()) cookieOpts.Secret = base64.URLEncoding.EncodeToString(secret) - cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret)) - Expect(err).ToNot(HaveOccurred()) - Expect(cipher).ToNot(BeNil()) - opts.Cipher = cipher ss, err = sessions.NewSessionStore(opts, cookieOpts) Expect(err).ToNot(HaveOccurred()) @@ -384,9 +371,16 @@ var _ = Describe("NewSessionStore", func() { ss = nil opts = &options.SessionOptions{} + // A secret is required to create a Cipher, validation ensures it is the correct + // length before a session store is initialised. + secret := make([]byte, 32) + _, err := rand.Read(secret) + Expect(err).ToNot(HaveOccurred()) + // Set default options in CookieOptions cookieOpts = &options.CookieOptions{ Name: "_oauth2_proxy", + Secret: base64.URLEncoding.EncodeToString(secret), Path: "/", Expire: time.Duration(168) * time.Hour, Refresh: time.Duration(1) * time.Hour, diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 44a3e758..bb8285ca 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -37,8 +37,6 @@ func Validate(o *options.Options) error { } msgs := make([]string, 0) - - var cipher encryption.Cipher if o.Cookie.Secret == "" { msgs = append(msgs, "missing setting: cookie-secret") } else { @@ -60,12 +58,6 @@ func Validate(o *options.Options) error { msgs = append(msgs, fmt.Sprintf("Cookie secret must be 16, 24, or 32 bytes to create an AES cipher. Got %d bytes.%s", len(encryption.SecretBytes(o.Cookie.Secret)), suffix)) - } else { - var err error - cipher, err = encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(o.Cookie.Secret)) - if err != nil { - msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err)) - } } } @@ -218,7 +210,6 @@ func Validate(o *options.Options) error { } msgs = parseProviderInfo(o, msgs) - o.Session.Cipher = cipher sessionStore, err := sessions.NewSessionStore(&o.Session, &o.Cookie) if err != nil { msgs = append(msgs, fmt.Sprintf("error initialising session storage: %v", err))