From d228d5a928176575364e9e1fb4bb4e417c91de3f Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Thu, 14 May 2020 02:16:35 -0700 Subject: [PATCH] Refactor the utils package to other areas (#538) * Refactor the utils package to other areas Move cookieSession functions to cookie session store & align the double implementation of SecretBytes to be united and housed under encryption * Remove unused Provider SessionFromCookie/CookieForSession These implementations aren't used, these are handled in the cookie store. * Add changelog entry for session/utils refactor --- CHANGELOG.md | 2 +- options.go | 32 +++------------------- pkg/encryption/cipher.go | 23 ++++++++++++++++ pkg/sessions/cookie/session_store.go | 15 ++++++++-- pkg/sessions/session_store_test.go | 3 +- pkg/sessions/utils/utils.go | 41 ---------------------------- providers/provider_default.go | 11 -------- providers/providers.go | 3 -- 8 files changed, 41 insertions(+), 89 deletions(-) delete mode 100644 pkg/sessions/utils/utils.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 91a67acf..4c6b2e4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ Use `--prefer-email-to-user` to restore falling back to the Email in these cases. ## Changes since v5.1.1 - +- [#538](https://github.com/oauth2-proxy/oauth2-proxy/pull/538) Refactor sessions/utils.go functionality to other areas (@NickMeves) - [#503](https://github.com/oauth2-proxy/oauth2-proxy/pull/503) Implements --real-client-ip-header option to select the header from which to obtain a proxied client's IP (@Izzette) - [#529](https://github.com/oauth2-proxy/oauth2-proxy/pull/529) Add local test environments for testing changes and new features (@JoelSpeed) - [#537](https://github.com/oauth2-proxy/oauth2-proxy/pull/537) Drop Fallback to Email if User not set (@JoelSpeed) diff --git a/options.go b/options.go index 7220da84..ae6ac03d 100644 --- a/options.go +++ b/options.go @@ -4,7 +4,6 @@ import ( "context" "crypto" "crypto/tls" - "encoding/base64" "fmt" "io/ioutil" "net/http" @@ -386,12 +385,12 @@ func (o *Options) Validate() error { if o.PassAccessToken || o.SetAuthorization || o.PassAuthorization || (o.Cookie.Refresh != time.Duration(0)) { validCookieSecretSize := false for _, i := range []int{16, 24, 32} { - if len(secretBytes(o.Cookie.Secret)) == i { + if len(encryption.SecretBytes(o.Cookie.Secret)) == i { validCookieSecretSize = true } } var decoded bool - if string(secretBytes(o.Cookie.Secret)) != o.Cookie.Secret { + if string(encryption.SecretBytes(o.Cookie.Secret)) != o.Cookie.Secret { decoded = true } if !validCookieSecretSize { @@ -404,10 +403,10 @@ func (o *Options) Validate() error { "to create an AES cipher when "+ "pass_access_token == true or "+ "cookie_refresh != 0, but is %d bytes.%s", - len(secretBytes(o.Cookie.Secret)), suffix)) + len(encryption.SecretBytes(o.Cookie.Secret)), suffix)) } else { var err error - cipher, err = encryption.NewCipher(secretBytes(o.Cookie.Secret)) + cipher, err = encryption.NewCipher(encryption.SecretBytes(o.Cookie.Secret)) if err != nil { msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err)) } @@ -643,29 +642,6 @@ func validateCookieName(o *Options, msgs []string) []string { return msgs } -func addPadding(secret string) string { - padding := len(secret) % 4 - switch padding { - case 1: - return secret + "===" - case 2: - return secret + "==" - case 3: - return secret + "=" - default: - return secret - } -} - -// secretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary -func secretBytes(secret string) []byte { - b, err := base64.URLEncoding.DecodeString(addPadding(secret)) - if err == nil { - return []byte(addPadding(string(b))) - } - return []byte(secret) -} - func setupLogger(o *Options, msgs []string) []string { // Setup the log file if len(o.LoggingFilename) > 0 { diff --git a/pkg/encryption/cipher.go b/pkg/encryption/cipher.go index 9c438bce..05952d8e 100644 --- a/pkg/encryption/cipher.go +++ b/pkg/encryption/cipher.go @@ -17,6 +17,29 @@ import ( "time" ) +// SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary +func SecretBytes(secret string) []byte { + b, err := base64.URLEncoding.DecodeString(addPadding(secret)) + if err == nil { + return []byte(addPadding(string(b))) + } + return []byte(secret) +} + +func addPadding(secret string) string { + padding := len(secret) % 4 + switch padding { + case 1: + return secret + "===" + case 2: + return secret + "==" + case 3: + return secret + "=" + default: + return secret + } +} + // cookies are stored in a 3 part (value + timestamp + signature) to enforce that the values are as originally set. // additionally, the 'value' is encrypted so it's opaque to the browser diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index a01d8050..d97125c3 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -13,7 +13,6 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" "github.com/oauth2-proxy/oauth2-proxy/pkg/logger" - "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/utils" ) const ( @@ -38,7 +37,7 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi if ss.CreatedAt.IsZero() { ss.CreatedAt = time.Now() } - value, err := utils.CookieForSession(ss, s.CookieCipher) + value, err := cookieForSession(ss, s.CookieCipher) if err != nil { return err } @@ -59,7 +58,7 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) { return nil, errors.New("cookie signature not valid") } - session, err := utils.SessionFromCookie(val, s.CookieCipher) + session, err := sessionFromCookie(val, s.CookieCipher) if err != nil { return nil, err } @@ -83,6 +82,16 @@ func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error { return nil } +// cookieForSession serializes a session state for storage in a cookie +func cookieForSession(s *sessions.SessionState, c *encryption.Cipher) (string, error) { + return s.EncodeSessionState(c) +} + +// sessionFromCookie deserializes a session from a cookie value +func sessionFromCookie(v string, c *encryption.Cipher) (s *sessions.SessionState, err error) { + return sessions.DecodeSessionState(v, c) +} + // setSessionCookie adds the user's session cookie to the response func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val string, created time.Time) { for _, c := range s.makeSessionCookie(req, val, created) { diff --git a/pkg/sessions/session_store_test.go b/pkg/sessions/session_store_test.go index ded609ec..68cfd125 100644 --- a/pkg/sessions/session_store_test.go +++ b/pkg/sessions/session_store_test.go @@ -18,7 +18,6 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions" sessionscookie "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/cookie" "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/redis" - "github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -365,7 +364,7 @@ var _ = Describe("NewSessionStore", func() { _, err := rand.Read(secret) Expect(err).ToNot(HaveOccurred()) cookieOpts.Secret = base64.URLEncoding.EncodeToString(secret) - cipher, err := encryption.NewCipher(utils.SecretBytes(cookieOpts.Secret)) + cipher, err := encryption.NewCipher(encryption.SecretBytes(cookieOpts.Secret)) Expect(err).ToNot(HaveOccurred()) Expect(cipher).ToNot(BeNil()) opts.Cipher = cipher diff --git a/pkg/sessions/utils/utils.go b/pkg/sessions/utils/utils.go deleted file mode 100644 index e8de8ae9..00000000 --- a/pkg/sessions/utils/utils.go +++ /dev/null @@ -1,41 +0,0 @@ -package utils - -import ( - "encoding/base64" - - "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" - "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" -) - -// CookieForSession serializes a session state for storage in a cookie -func CookieForSession(s *sessions.SessionState, c *encryption.Cipher) (string, error) { - return s.EncodeSessionState(c) -} - -// SessionFromCookie deserializes a session from a cookie value -func SessionFromCookie(v string, c *encryption.Cipher) (s *sessions.SessionState, err error) { - return sessions.DecodeSessionState(v, c) -} - -// SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary -func SecretBytes(secret string) []byte { - b, err := base64.URLEncoding.DecodeString(addPadding(secret)) - if err == nil { - return []byte(addPadding(string(b))) - } - return []byte(secret) -} - -func addPadding(secret string) string { - padding := len(secret) % 4 - switch padding { - case 1: - return secret + "===" - case 2: - return secret + "==" - case 3: - return secret + "=" - default: - return secret - } -} diff --git a/providers/provider_default.go b/providers/provider_default.go index d90ad164..141261a1 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -14,7 +14,6 @@ import ( "github.com/coreos/go-oidc" "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" - "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" ) var _ Provider = (*ProviderData)(nil) @@ -108,16 +107,6 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string { return a.String() } -// CookieForSession serializes a session state for storage in a cookie -func (p *ProviderData) CookieForSession(s *sessions.SessionState, c *encryption.Cipher) (string, error) { - return s.EncodeSessionState(c) -} - -// SessionFromCookie deserializes a session from a cookie value -func (p *ProviderData) SessionFromCookie(v string, c *encryption.Cipher) (s *sessions.SessionState, err error) { - return sessions.DecodeSessionState(v, c) -} - // GetEmailAddress returns the Account email address func (p *ProviderData) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) { return "", errors.New("not implemented") diff --git a/providers/providers.go b/providers/providers.go index 87ba9103..a52eff90 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -5,7 +5,6 @@ import ( "github.com/coreos/go-oidc" "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions" - "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" ) // Provider represents an upstream identity provider implementation @@ -19,8 +18,6 @@ type Provider interface { ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool GetLoginURL(redirectURI, finalRedirect string) string RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) - SessionFromCookie(string, *encryption.Cipher) (*sessions.SessionState, error) - CookieForSession(*sessions.SessionState, *encryption.Cipher) (string, error) CreateSessionStateFromBearerToken(ctx context.Context, rawIDToken string, idToken *oidc.IDToken) (*sessions.SessionState, error) }