Move Cipher intialisation to session store initialisation

pkg/apis/options/sessions.go

@@ -1,11 +1,8 @@
 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"`
 }
 

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
 }

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

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(),

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)
 	}

pkg/sessions/session_store_test.go

@@ -353,24 +353,11 @@ var _ = Describe("NewSessionStore", func() {
 					SameSite: "strict",
 				}
 
-				var err error
+				// A secret is required but not defaulted
-				ss, err = sessions.NewSessionStore(opts, cookieOpts)
-				Expect(err).ToNot(HaveOccurred())
-			})
-
-			SessionStoreInterfaceTests(persistent)
-		})
-
-		Context("with a cipher", func() {
-			BeforeEach(func() {
 				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,

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))