Move cipher creation to options and away from oauth2_proxy.go

Author: JoelSpeed

oauthproxy.go

@@ -85,7 +85,6 @@ type OAuthProxy struct {
 	PassAccessToken     bool
 	SetAuthorization    bool
 	PassAuthorization   bool
-	CookieCipher        *cookie.Cipher
 	skipAuthRegex       []string
 	skipAuthPreflight   bool
 	compiledRegex       []*regexp.Regexp
@@ -215,15 +214,6 @@ func NewOAuthProxy(opts *Options, validator func(string) bool) *OAuthProxy {
 
 	logger.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domain:%s path:%s refresh:%s", opts.CookieName, opts.CookieSecure, opts.CookieHTTPOnly, opts.CookieExpire, opts.CookieDomain, opts.CookiePath, refresh)
 
-	var cipher *cookie.Cipher
-	if opts.PassAccessToken || opts.SetAuthorization || opts.PassAuthorization || (opts.CookieRefresh != time.Duration(0)) {
-		var err error
-		cipher, err = cookie.NewCipher(secretBytes(opts.CookieSecret))
-		if err != nil {
-			logger.Fatal("cookie-secret error: ", err)
-		}
-	}
-
 	return &OAuthProxy{
 		CookieName:     opts.CookieName,
 		CSRFCookieName: fmt.Sprintf("%v_%v", opts.CookieName, "csrf"),
@@ -261,7 +251,6 @@ func NewOAuthProxy(opts *Options, validator func(string) bool) *OAuthProxy {
 		SetAuthorization:   opts.SetAuthorization,
 		PassAuthorization:  opts.PassAuthorization,
 		SkipProviderButton: opts.SkipProviderButton,
-		CookieCipher:       cipher,
 		templates:          loadTemplates(opts.CustomTemplatesDir),
 		Footer:             opts.Footer,
 	}

oauthproxy_test.go

@@ -1083,7 +1083,7 @@ func TestClearSplitCookie(t *testing.T) {
 	opts := NewOptions()
 	opts.CookieName = "oauth2"
 	opts.CookieDomain = "abc"
-	store, err := cookie.NewCookieSessionStore(opts.SessionOptions.CookieStoreOptions, &opts.CookieOptions)
+	store, err := cookie.NewCookieSessionStore(&opts.SessionOptions, &opts.CookieOptions)
 	assert.Equal(t, err, nil)
 	p := OAuthProxy{CookieName: opts.CookieName, CookieDomain: opts.CookieDomain, sessionStore: store}
 	var rw = httptest.NewRecorder()
@@ -1112,7 +1112,7 @@ func TestClearSingleCookie(t *testing.T) {
 	opts := NewOptions()
 	opts.CookieName = "oauth2"
 	opts.CookieDomain = "abc"
-	store, err := cookie.NewCookieSessionStore(opts.SessionOptions.CookieStoreOptions, &opts.CookieOptions)
+	store, err := cookie.NewCookieSessionStore(&opts.SessionOptions, &opts.CookieOptions)
 	assert.Equal(t, err, nil)
 	p := OAuthProxy{CookieName: opts.CookieName, CookieDomain: opts.CookieDomain, sessionStore: store}
 	var rw = httptest.NewRecorder()

options.go

@@ -17,6 +17,7 @@ import (
 	oidc "github.com/coreos/go-oidc"
 	"github.com/dgrijalva/jwt-go"
 	"github.com/mbland/hmacauth"
+	"github.com/pusher/oauth2_proxy/cookie"
 	"github.com/pusher/oauth2_proxy/logger"
 	"github.com/pusher/oauth2_proxy/pkg/apis/options"
 	sessionsapi "github.com/pusher/oauth2_proxy/pkg/apis/sessions"
@@ -267,7 +268,8 @@ func (o *Options) Validate() error {
 	}
 	msgs = parseProviderInfo(o, msgs)
 
-	if o.PassAccessToken || (o.CookieRefresh != time.Duration(0)) {
+	var cipher *cookie.Cipher
+	if o.PassAccessToken || o.SetAuthorization || o.PassAuthorization || (o.CookieRefresh != time.Duration(0)) {
 		validCookieSecretSize := false
 		for _, i := range []int{16, 24, 32} {
 			if len(secretBytes(o.CookieSecret)) == i {
@@ -290,11 +292,15 @@ func (o *Options) Validate() error {
 					"cookie_refresh != 0, but is %d bytes.%s",
 				len(secretBytes(o.CookieSecret)), suffix))
 		} else {
-			// Enable encryption in the session store
-			o.EnableCipher = true
+			var err error
+			cipher, err = cookie.NewCipher(secretBytes(o.CookieSecret))
+			if err != nil {
+				msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err))
+			}
 		}
 	}
 
+	o.SessionOptions.Cipher = cipher
 	sessionStore, err := sessions.NewSessionStore(&o.SessionOptions, &o.CookieOptions)
 	if err != nil {
 		msgs = append(msgs, fmt.Sprintf("error initialising session storage: %v", err))

pkg/apis/options/sessions.go

@@ -1,9 +1,13 @@
 package options
 
+import (
+	"github.com/pusher/oauth2_proxy/cookie"
+)
+
 // SessionOptions contains configuration options for the SessionStore providers.
 type SessionOptions struct {
-	Type         string `flag:"session-store-type" cfg:"session_store_type" env:"OAUTH2_PROXY_SESSION_STORE_TYPE"`
-	EnableCipher bool   // Allow the user to choose encryption or not
+	Type   string `flag:"session-store-type" cfg:"session_store_type" env:"OAUTH2_PROXY_SESSION_STORE_TYPE"`
+	Cipher *cookie.Cipher
 	CookieStoreOptions
 }
 
@@ -12,6 +16,4 @@ type SessionOptions struct {
 var CookieSessionStoreType = "cookie"
 
 // CookieStoreOptions contains configuration options for the CookieSessionStore.
-type CookieStoreOptions struct {
-	EnableCipher bool // Allow the user to choose encryption or not
-}
+type CookieStoreOptions struct{}

pkg/sessions/cookie/session_store.go

@@ -118,18 +118,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.CookieStoreOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
-	var cipher *cookie.Cipher
-	if opts.EnableCipher {
-		var err error
-		cipher, err = cookie.NewCipher(utils.SecretBytes(cookieOpts.CookieSecret))
-		if err != nil {
-			return nil, fmt.Errorf("unable to create cipher: %v", err)
-		}
-	}
-
+func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
 	return &SessionStore{
-		CookieCipher:  cipher,
+		CookieCipher:  opts.Cipher,
 		CookieOptions: cookieOpts,
 	}, nil
 }

pkg/sessions/session_store.go

@@ -12,9 +12,7 @@ import (
 func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
 	switch opts.Type {
 	case options.CookieSessionStoreType:
-		// Ensure EnableCipher is propogated from the parent option
-		opts.CookieStoreOptions.EnableCipher = opts.EnableCipher
-		return cookie.NewCookieSessionStore(opts.CookieStoreOptions, cookieOpts)
+		return cookie.NewCookieSessionStore(opts, cookieOpts)
 	default:
 		return nil, fmt.Errorf("unknown session store type '%s'", opts.Type)
 	}

pkg/sessions/session_store_test.go

@@ -12,11 +12,13 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/pusher/oauth2_proxy/cookie"
 	"github.com/pusher/oauth2_proxy/pkg/apis/options"
 	sessionsapi "github.com/pusher/oauth2_proxy/pkg/apis/sessions"
 	"github.com/pusher/oauth2_proxy/pkg/cookies"
 	"github.com/pusher/oauth2_proxy/pkg/sessions"
-	"github.com/pusher/oauth2_proxy/pkg/sessions/cookie"
+	sessionscookie "github.com/pusher/oauth2_proxy/pkg/sessions/cookie"
+	"github.com/pusher/oauth2_proxy/pkg/sessions/utils"
 )
 
 func TestSessionStore(t *testing.T) {
@@ -200,33 +202,23 @@ var _ = Describe("NewSessionStore", func() {
 			SessionStoreInterfaceTests()
 		})
 
-		Context("with encryption enabled", func() {
+		Context("with a cipher", func() {
 			BeforeEach(func() {
 				secret := make([]byte, 32)
 				_, err := rand.Read(secret)
 				Expect(err).ToNot(HaveOccurred())
 				cookieOpts.CookieSecret = base64.URLEncoding.EncodeToString(secret)
-				opts.EnableCipher = true
+				cipher, err := cookie.NewCipher(utils.SecretBytes(cookieOpts.CookieSecret))
+				Expect(err).ToNot(HaveOccurred())
+				Expect(cipher).ToNot(BeNil())
+				opts.Cipher = cipher
 
 				ss, err = sessions.NewSessionStore(opts, cookieOpts)
 				Expect(err).ToNot(HaveOccurred())
 			})
 
 			SessionStoreInterfaceTests()
 		})
-
-		Context("with encryption enabled, but no secret", func() {
-			BeforeEach(func() {
-				opts.EnableCipher = true
-			})
-
-			It("returns an error", func() {
-				ss, err := sessions.NewSessionStore(opts, cookieOpts)
-				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("unable to create cipher: crypto/aes: invalid key size 0"))
-				Expect(ss).To(BeNil())
-			})
-		})
 	}
 
 	BeforeEach(func() {
@@ -264,7 +256,7 @@ var _ = Describe("NewSessionStore", func() {
 		It("creates a cookie.SessionStore", func() {
 			ss, err := sessions.NewSessionStore(opts, cookieOpts)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(ss).To(BeAssignableToTypeOf(&cookie.SessionStore{}))
+			Expect(ss).To(BeAssignableToTypeOf(&sessionscookie.SessionStore{}))
 		})
 
 		Context("the cookie.SessionStore", func() {