From 093f9da881e8c3974b2df9b69dbbc1f75e03083f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 15 May 2019 16:56:05 +0100 Subject: [PATCH] Move cipher creation to options and away from oauth2_proxy.go --- oauthproxy.go | 11 ----------- oauthproxy_test.go | 4 ++-- options.go | 12 +++++++++--- pkg/apis/options/sessions.go | 12 +++++++----- pkg/sessions/cookie/session_store.go | 13 ++----------- pkg/sessions/session_store.go | 4 +--- pkg/sessions/session_store_test.go | 26 +++++++++----------------- 7 files changed, 30 insertions(+), 52 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index 75827fa..389b2a9 100644 --- a/oauthproxy.go +++ b/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, } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 52f688b..1d09bbb 100644 --- a/oauthproxy_test.go +++ b/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() diff --git a/options.go b/options.go index 50587c1..0460bce 100644 --- a/options.go +++ b/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)) diff --git a/pkg/apis/options/sessions.go b/pkg/apis/options/sessions.go index 2429b7b..7d33f14 100644 --- a/pkg/apis/options/sessions.go +++ b/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{} diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index ec43d73..c40dd23 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/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 } diff --git a/pkg/sessions/session_store.go b/pkg/sessions/session_store.go index 3a81eaa..ec84dd7 100644 --- a/pkg/sessions/session_store.go +++ b/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) } diff --git a/pkg/sessions/session_store_test.go b/pkg/sessions/session_store_test.go index 7c63795..b407d58 100644 --- a/pkg/sessions/session_store_test.go +++ b/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,13 +202,16 @@ 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()) @@ -214,19 +219,6 @@ var _ = Describe("NewSessionStore", func() { 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() {