Merge pull request #576 from oauth2-proxy/cookie-validation

Separate Cookie validation

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Changes since v6.0.0
 
+- [#576](https://github.com/oauth2-proxy/oauth2-proxy/pull/576) Separate Cookie validation out of main options validation (@JoelSpeed)
 - [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
 - [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
 - [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)

pkg/apis/options/cookie.go

@@ -1,9 +1,13 @@
 package options
 
-import "time"
+import (
+	"time"
 
-// CookieOptions contains configuration options relating to Cookie configuration
-type CookieOptions struct {
+	"github.com/spf13/pflag"
+)
+
+// Cookie contains configuration options relating to Cookie configuration
+type Cookie struct {
 	Name     string        `flag:"cookie-name" cfg:"cookie_name"`
 	Secret   string        `flag:"cookie-secret" cfg:"cookie_secret"`
 	Domains  []string      `flag:"cookie-domain" cfg:"cookie_domains"`
@@ -14,3 +18,34 @@ type CookieOptions struct {
 	HTTPOnly bool          `flag:"cookie-httponly" cfg:"cookie_httponly"`
 	SameSite string        `flag:"cookie-samesite" cfg:"cookie_samesite"`
 }
+
+func cookieFlagSet() *pflag.FlagSet {
+	flagSet := pflag.NewFlagSet("cookie", pflag.ExitOnError)
+
+	flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates")
+	flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)")
+	flagSet.StringSlice("cookie-domain", []string{}, "Optional cookie domains to force cookies to (ie: `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match).")
+	flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*")
+	flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie")
+	flagSet.Duration("cookie-refresh", time.Duration(0), "refresh the cookie after this duration; 0 to disable")
+	flagSet.Bool("cookie-secure", true, "set secure (HTTPS) cookie flag")
+	flagSet.Bool("cookie-httponly", true, "set HttpOnly cookie flag")
+	flagSet.String("cookie-samesite", "", "set SameSite cookie attribute (ie: \"lax\", \"strict\", \"none\", or \"\"). ")
+
+	return flagSet
+}
+
+// cookieDefaults creates a Cookie populating each field with its default value
+func cookieDefaults() Cookie {
+	return Cookie{
+		Name:     "_oauth2_proxy",
+		Secret:   "",
+		Domains:  nil,
+		Path:     "/",
+		Expire:   time.Duration(168) * time.Hour,
+		Refresh:  time.Duration(0),
+		Secure:   true,
+		HTTPOnly: true,
+		SameSite: "",
+	}
+}

pkg/apis/options/options.go

@@ -59,7 +59,7 @@ type Options struct {
 	Banner                   string   `flag:"banner" cfg:"banner"`
 	Footer                   string   `flag:"footer" cfg:"footer"`
 
-	Cookie  CookieOptions  `cfg:",squash"`
+	Cookie  Cookie         `cfg:",squash"`
 	Session SessionOptions `cfg:",squash"`
 	Logging Logging        `cfg:",squash"`
 
@@ -153,14 +153,7 @@ func NewOptions() *Options {
 		RealClientIPHeader:  "X-Real-IP",
 		ForceHTTPS:          false,
 		DisplayHtpasswdForm: true,
-		Cookie: CookieOptions{
-			Name:     "_oauth2_proxy",
-			Secure:   true,
-			HTTPOnly: true,
-			Expire:   time.Duration(168) * time.Hour,
-			Refresh:  time.Duration(0),
-			Path:     "/",
-		},
+		Cookie:              cookieDefaults(),
 		Session: SessionOptions{
 			Type: "cookie",
 		},
@@ -245,17 +238,6 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.String("ping-path", "/ping", "the ping endpoint that can be used for basic health checks")
 	flagSet.String("ping-user-agent", "", "special User-Agent that will be used for basic health checks")
 	flagSet.Bool("proxy-websockets", true, "enables WebSocket proxying")
-
-	flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates")
-	flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)")
-	flagSet.StringSlice("cookie-domain", []string{}, "Optional cookie domains to force cookies to (ie: `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match).")
-	flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*")
-	flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie")
-	flagSet.Duration("cookie-refresh", time.Duration(0), "refresh the cookie after this duration; 0 to disable")
-	flagSet.Bool("cookie-secure", true, "set secure (HTTPS) cookie flag")
-	flagSet.Bool("cookie-httponly", true, "set HttpOnly cookie flag")
-	flagSet.String("cookie-samesite", "", "set SameSite cookie attribute (ie: \"lax\", \"strict\", \"none\", or \"\"). ")
-
 	flagSet.String("session-store-type", "cookie", "the session storage provider to use")
 	flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://HOST[:PORT])")
 	flagSet.Bool("redis-use-sentinel", false, "Connect to redis via sentinels. Must set --redis-sentinel-master-name and --redis-sentinel-connection-urls to use this feature")
@@ -292,6 +274,7 @@ func NewFlagSet() *pflag.FlagSet {
 
 	flagSet.String("user-id-claim", "email", "which claim contains the user ID")
 
+	flagSet.AddFlagSet(cookieFlagSet())
 	flagSet.AddFlagSet(loggingFlagSet())
 
 	return flagSet

pkg/cookies/cookies.go

@@ -38,7 +38,7 @@ func MakeCookie(req *http.Request, name string, value string, path string, domai
 
 // MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions,
 // value and creation time
-func MakeCookieFromOptions(req *http.Request, name string, value string, cookieOpts *options.CookieOptions, expiration time.Duration, now time.Time) *http.Cookie {
+func MakeCookieFromOptions(req *http.Request, name string, value string, cookieOpts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie {
 	domain := GetCookieDomain(req, cookieOpts.Domains)
 
 	if domain != "" {

pkg/sessions/cookie/session_store.go

@@ -28,8 +28,8 @@ var _ sessions.SessionStore = &SessionStore{}
 // SessionStore is an implementation of the sessions.SessionStore
 // interface that stores sessions in client side cookies
 type SessionStore struct {
-	CookieOptions *options.CookieOptions
-	CookieCipher  encryption.Cipher
+	Cookie       *options.Cookie
+	CookieCipher encryption.Cipher
 }
 
 // Save takes a sessions.SessionState and stores the information from it
@@ -50,12 +50,12 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi
 // Load reads sessions.SessionState information from Cookies within the
 // HTTP request object
 func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
-	c, err := loadCookie(req, s.CookieOptions.Name)
+	c, err := loadCookie(req, s.Cookie.Name)
 	if err != nil {
 		// always http.ErrNoCookie
-		return nil, fmt.Errorf("cookie %q not present", s.CookieOptions.Name)
+		return nil, fmt.Errorf("cookie %q not present", s.Cookie.Name)
 	}
-	val, _, ok := encryption.Validate(c, s.CookieOptions.Secret, s.CookieOptions.Expire)
+	val, _, ok := encryption.Validate(c, s.Cookie.Secret, s.Cookie.Expire)
 	if !ok {
 		return nil, errors.New("cookie signature not valid")
 	}
@@ -71,7 +71,7 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
 // clear the session
 func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error {
 	// matches CookieName, CookieName_<number>
-	var cookieNameRegex = regexp.MustCompile(fmt.Sprintf("^%s(_\\d+)?$", s.CookieOptions.Name))
+	var cookieNameRegex = regexp.MustCompile(fmt.Sprintf("^%s(_\\d+)?$", s.Cookie.Name))
 
 	for _, c := range req.Cookies() {
 		if cookieNameRegex.MatchString(c.Name) {
@@ -105,9 +105,9 @@ func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Reques
 // authentication details
 func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now time.Time) []*http.Cookie {
 	if value != "" {
-		value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
+		value = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, []byte(value), now)
 	}
-	c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
+	c := s.makeCookie(req, s.Cookie.Name, value, s.Cookie.Expire, now)
 
 	if len(c.String()) > maxCookieLength {
 		return splitCookie(c)
@@ -120,7 +120,7 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string,
 		req,
 		name,
 		value,
-		s.CookieOptions,
+		s.Cookie,
 		expiration,
 		now,
 	)
@@ -128,15 +128,15 @@ 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.Cookie) (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)
 	}
 
 	return &SessionStore{
-		CookieCipher:  cipher,
-		CookieOptions: cookieOpts,
+		CookieCipher: cipher,
+		Cookie:       cookieOpts,
 	}, nil
 }
 

pkg/sessions/cookie/session_store_test.go

@@ -25,7 +25,7 @@ func TestSessionStore(t *testing.T) {
 
 var _ = Describe("Cookie SessionStore Tests", func() {
 	tests.RunSessionStoreTests(
-		func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
+		func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
 			// Set the connection URL
 			opts.Type = options.CookieSessionStoreType
 			return NewCookieSessionStore(opts, cookieOpts)

pkg/sessions/redis/redis_store.go

@@ -32,14 +32,14 @@ type TicketData struct {
 // SessionStore is an implementation of the sessions.SessionStore
 // interface that stores sessions in redis
 type SessionStore struct {
-	CookieCipher  encryption.Cipher
-	CookieOptions *options.CookieOptions
-	Client        Client
+	CookieCipher encryption.Cipher
+	Cookie       *options.Cookie
+	Client       Client
 }
 
 // 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.Cookie) (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)
@@ -51,9 +51,9 @@ func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cook
 	}
 
 	rs := &SessionStore{
-		Client:        client,
-		CookieCipher:  cipher,
-		CookieOptions: cookieOpts,
+		Client:       client,
+		CookieCipher: cipher,
+		Cookie:       cookieOpts,
 	}
 	return rs, nil
 
@@ -145,13 +145,13 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
 
 	// Old sessions that we are refreshing would have a request cookie
 	// New sessions don't, so we ignore the error. storeValue will check requestCookie
-	requestCookie, _ := req.Cookie(store.CookieOptions.Name)
+	requestCookie, _ := req.Cookie(store.Cookie.Name)
 	value, err := s.EncodeSessionState(store.CookieCipher)
 	if err != nil {
 		return err
 	}
 	ctx := req.Context()
-	ticketString, err := store.storeValue(ctx, value, store.CookieOptions.Expire, requestCookie)
+	ticketString, err := store.storeValue(ctx, value, store.Cookie.Expire, requestCookie)
 	if err != nil {
 		return err
 	}
@@ -159,7 +159,7 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
 	ticketCookie := store.makeCookie(
 		req,
 		ticketString,
-		store.CookieOptions.Expire,
+		store.Cookie.Expire,
 		*s.CreatedAt,
 	)
 
@@ -170,12 +170,12 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
 // Load reads sessions.SessionState information from a ticket
 // cookie within the HTTP request object
 func (store *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
-	requestCookie, err := req.Cookie(store.CookieOptions.Name)
+	requestCookie, err := req.Cookie(store.Cookie.Name)
 	if err != nil {
 		return nil, fmt.Errorf("error loading session: %s", err)
 	}
 
-	val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
+	val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
 	if !ok {
 		return nil, fmt.Errorf("cookie signature not valid")
 	}
@@ -189,12 +189,12 @@ func (store *SessionStore) Load(req *http.Request) (*sessions.SessionState, erro
 
 // loadSessionFromString loads the session based on the ticket value
 func (store *SessionStore) loadSessionFromString(ctx context.Context, value string) (*sessions.SessionState, error) {
-	ticket, err := decodeTicket(store.CookieOptions.Name, value)
+	ticket, err := decodeTicket(store.Cookie.Name, value)
 	if err != nil {
 		return nil, err
 	}
 
-	resultBytes, err := store.Client.Get(ctx, ticket.asHandle(store.CookieOptions.Name))
+	resultBytes, err := store.Client.Get(ctx, ticket.asHandle(store.Cookie.Name))
 	if err != nil {
 		return nil, err
 	}
@@ -227,7 +227,7 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
 	http.SetCookie(rw, clearCookie)
 
 	// If there was an existing cookie we should clear the session in redis
-	requestCookie, err := req.Cookie(store.CookieOptions.Name)
+	requestCookie, err := req.Cookie(store.Cookie.Name)
 	if err != nil && err == http.ErrNoCookie {
 		// No existing cookie so can't clear redis
 		return nil
@@ -235,17 +235,17 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
 		return fmt.Errorf("error retrieving cookie: %v", err)
 	}
 
-	val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
+	val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
 	if !ok {
 		return fmt.Errorf("cookie signature not valid")
 	}
 
 	// We only return an error if we had an issue with redis
 	// If there's an issue decoding the ticket, ignore it
-	ticket, _ := decodeTicket(store.CookieOptions.Name, string(val))
+	ticket, _ := decodeTicket(store.Cookie.Name, string(val))
 	if ticket != nil {
 		ctx := req.Context()
-		err := store.Client.Del(ctx, ticket.asHandle(store.CookieOptions.Name))
+		err := store.Client.Del(ctx, ticket.asHandle(store.Cookie.Name))
 		if err != nil {
 			return fmt.Errorf("error clearing cookie from redis: %s", err)
 		}
@@ -256,13 +256,13 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
 // makeCookie makes a cookie, signing the value if present
 func (store *SessionStore) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie {
 	if value != "" {
-		value = encryption.SignedValue(store.CookieOptions.Secret, store.CookieOptions.Name, []byte(value), now)
+		value = encryption.SignedValue(store.Cookie.Secret, store.Cookie.Name, []byte(value), now)
 	}
 	return cookies.MakeCookieFromOptions(
 		req,
-		store.CookieOptions.Name,
+		store.Cookie.Name,
 		value,
-		store.CookieOptions,
+		store.Cookie,
 		expires,
 		now,
 	)
@@ -284,12 +284,12 @@ func (store *SessionStore) storeValue(ctx context.Context, value string, expirat
 	stream := cipher.NewCFBEncrypter(block, ticket.Secret)
 	stream.XORKeyStream(ciphertext, []byte(value))
 
-	handle := ticket.asHandle(store.CookieOptions.Name)
+	handle := ticket.asHandle(store.Cookie.Name)
 	err = store.Client.Set(ctx, handle, ciphertext, expiration)
 	if err != nil {
 		return "", err
 	}
-	return ticket.encodeTicket(store.CookieOptions.Name), nil
+	return ticket.encodeTicket(store.Cookie.Name), nil
 }
 
 // getTicket retrieves an existing ticket from the cookie if present,
@@ -300,14 +300,14 @@ func (store *SessionStore) getTicket(requestCookie *http.Cookie) (*TicketData, e
 	}
 
 	// An existing cookie exists, try to retrieve the ticket
-	val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
+	val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
 	if !ok {
 		// Cookie is invalid, create a new ticket
 		return newTicket()
 	}
 
 	// Valid cookie, decode the ticket
-	ticket, err := decodeTicket(store.CookieOptions.Name, string(val))
+	ticket, err := decodeTicket(store.Cookie.Name, string(val))
 	if err != nil {
 		// If we can't decode the ticket we have to create a new one
 		return newTicket()

pkg/sessions/redis/session_store_test.go

@@ -58,7 +58,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
 	})
 
 	tests.RunSessionStoreTests(
-		func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
+		func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
 			// Set the connection URL
 			opts.Type = options.RedisSessionStoreType
 			opts.Redis.ConnectionURL = "redis://" + mr.Addr()
@@ -87,7 +87,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
 		})
 
 		tests.RunSessionStoreTests(
-			func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
+			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
 				// Set the sentinel connection URL
 				sentinelAddr := "redis://" + ms.Addr()
 				opts.Type = options.RedisSessionStoreType
@@ -109,7 +109,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
 
 	Context("with cluster", func() {
 		tests.RunSessionStoreTests(
-			func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
+			func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
 				clusterAddr := "redis://" + mr.Addr()
 				opts.Type = options.RedisSessionStoreType
 				opts.Redis.ClusterConnectionURLs = []string{clusterAddr}

pkg/sessions/session_store.go

@@ -10,7 +10,7 @@ import (
 )
 
 // NewSessionStore creates a SessionStore from the provided configuration
-func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
+func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) {
 	switch opts.Type {
 	case options.CookieSessionStoreType:
 		return cookie.NewCookieSessionStore(opts, cookieOpts)

pkg/sessions/session_store_test.go

@@ -24,7 +24,7 @@ func TestSessionStore(t *testing.T) {
 
 var _ = Describe("NewSessionStore", func() {
 	var opts *options.SessionOptions
-	var cookieOpts *options.CookieOptions
+	var cookieOpts *options.Cookie
 
 	BeforeEach(func() {
 		opts = &options.SessionOptions{}
@@ -36,7 +36,7 @@ var _ = Describe("NewSessionStore", func() {
 		Expect(err).ToNot(HaveOccurred())
 
 		// Set default options in CookieOptions
-		cookieOpts = &options.CookieOptions{
+		cookieOpts = &options.Cookie{
 			Name:     "_oauth2_proxy",
 			Secret:   base64.URLEncoding.EncodeToString(secret),
 			Path:     "/",

pkg/sessions/tests/session_store_tests.go

@@ -21,7 +21,7 @@ import (
 // Ginkgo has unpacked the tests.
 // Interfaces have to be wrapped in closures otherwise nil pointers are thrown.
 type testInput struct {
-	cookieOpts            *options.CookieOptions
+	cookieOpts            *options.Cookie
 	ss                    sessionStoreFunc
 	session               *sessionsapi.SessionState
 	request               *http.Request
@@ -38,7 +38,7 @@ type PersistentStoreFastForwardFunc func(time.Duration) error
 
 // NewSessionStoreFunc allows any session store implementation to configure their
 // own session store before each test.
-type NewSessionStoreFunc func(sessionOpts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error)
+type NewSessionStoreFunc func(sessionOpts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error)
 
 func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward PersistentStoreFastForwardFunc) {
 	Describe("Session Store Suite", func() {
@@ -62,7 +62,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
 			Expect(err).ToNot(HaveOccurred())
 
 			// Set default options in CookieOptions
-			cookieOpts := &options.CookieOptions{
+			cookieOpts := &options.Cookie{
 				Name:     "_oauth2_proxy",
 				Path:     "/",
 				Expire:   time.Duration(168) * time.Hour,
@@ -111,7 +111,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
 
 		Context("with non-default options", func() {
 			BeforeEach(func() {
-				input.cookieOpts = &options.CookieOptions{
+				input.cookieOpts = &options.Cookie{
 					Name:     "_cookie_name",
 					Path:     "/path",
 					Expire:   time.Duration(72) * time.Hour,

pkg/validation/cookie.go

@@ -0,0 +1,68 @@
+package validation
+
+import (
+	"fmt"
+	"net/http"
+	"sort"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
+)
+
+func validateCookie(o options.Cookie) []string {
+	msgs := validateCookieSecret(o.Secret)
+
+	if o.Refresh >= o.Expire {
+		msgs = append(msgs, fmt.Sprintf(
+			"cookie_refresh (%q) must be less than cookie_expire (%q)",
+			o.Refresh.String(),
+			o.Expire.String()))
+	}
+
+	switch o.SameSite {
+	case "", "none", "lax", "strict":
+	default:
+		msgs = append(msgs, fmt.Sprintf("cookie_samesite (%q) must be one of ['', 'lax', 'strict', 'none']", o.SameSite))
+	}
+
+	// Sort cookie domains by length, so that we try longer (and more specific) domains first
+	sort.Slice(o.Domains, func(i, j int) bool {
+		return len(o.Domains[i]) > len(o.Domains[j])
+	})
+
+	msgs = append(msgs, validateCookieName(o.Name)...)
+	return msgs
+}
+
+func validateCookieName(name string) []string {
+	msgs := []string{}
+
+	cookie := &http.Cookie{Name: name}
+	if cookie.String() == "" {
+		msgs = append(msgs, fmt.Sprintf("invalid cookie name: %q", name))
+	}
+
+	if len(name) > 256 {
+		msgs = append(msgs, fmt.Sprintf("cookie name should be under 256 characters: cookie name is %d characters", len(name)))
+	}
+	return msgs
+}
+
+func validateCookieSecret(secret string) []string {
+	if secret == "" {
+		return []string{"missing setting: cookie-secret"}
+	}
+
+	secretBytes := encryption.SecretBytes(secret)
+	// Check if the secret is a valid length
+	switch len(secretBytes) {
+	case 16, 24, 32:
+		// Valid secret size found
+		return []string{}
+	}
+	// Invalid secret size found, return a message
+	return []string{fmt.Sprintf(
+		"cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes",
+		len(secretBytes)),
+	}
+}

pkg/validation/cookie_test.go

@@ -0,0 +1,273 @@
+package validation
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
+	. "github.com/onsi/gomega"
+)
+
+func TestValidateCookie(t *testing.T) {
+	alphabet := "abcdefghijklmnopqrstuvwxyz"
+
+	validName := "_oauth2_proxy"
+	invalidName := "_oauth2;proxy" // Separater character not allowed
+	// 10 times the alphabet should be longer than 256 characters
+	longName := strings.Repeat(alphabet, 10)
+	validSecret := "secretthirtytwobytes+abcdefghijk"
+	invalidSecret := "abcdef"                                          // 6 bytes is not a valid size
+	validBase64Secret := "c2VjcmV0dGhpcnR5dHdvYnl0ZXMrYWJjZGVmZ2hpams" // Base64 encoding of "secretthirtytwobytes+abcdefghijk"
+	invalidBase64Secret := "YWJjZGVmCg"                                // Base64 encoding of "abcdef"
+	emptyDomains := []string{}
+	domains := []string{
+		"a.localhost",
+		"ba.localhost",
+		"ca.localhost",
+		"cba.localhost",
+		"a.cba.localhost",
+	}
+
+	invalidNameMsg := "invalid cookie name: \"_oauth2;proxy\""
+	longNameMsg := "cookie name should be under 256 characters: cookie name is 260 characters"
+	missingSecretMsg := "missing setting: cookie-secret"
+	invalidSecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 6 bytes"
+	invalidBase64SecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 10 bytes"
+	refreshLongerThanExpireMsg := "cookie_refresh (\"1h0m0s\") must be less than cookie_expire (\"15m0s\")"
+	invalidSameSiteMsg := "cookie_samesite (\"invalid\") must be one of ['', 'lax', 'strict', 'none']"
+
+	testCases := []struct {
+		name       string
+		cookie     options.Cookie
+		errStrings []string
+	}{
+		{
+			name: "with valid configuration",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  domains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{},
+		},
+		{
+			name: "with no cookie secret",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   "",
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				missingSecretMsg,
+			},
+		},
+		{
+			name: "with an invalid cookie secret",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   invalidSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				invalidSecretMsg,
+			},
+		},
+		{
+			name: "with a valid Base64 secret",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validBase64Secret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{},
+		},
+		{
+			name: "with an invalid Base64 secret",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   invalidBase64Secret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				invalidBase64SecretMsg,
+			},
+		},
+		{
+			name: "with an invalid name",
+			cookie: options.Cookie{
+				Name:     invalidName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				invalidNameMsg,
+			},
+		},
+		{
+			name: "with a name that is too long",
+			cookie: options.Cookie{
+				Name:     longName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				longNameMsg,
+			},
+		},
+		{
+			name: "with refresh longer than expire",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   15 * time.Minute,
+				Refresh:  time.Hour,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "",
+			},
+			errStrings: []string{
+				refreshLongerThanExpireMsg,
+			},
+		},
+		{
+			name: "with samesite \"none\"",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "none",
+			},
+			errStrings: []string{},
+		},
+		{
+			name: "with samesite \"lax\"",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "none",
+			},
+			errStrings: []string{},
+		},
+		{
+			name: "with samesite \"strict\"",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "none",
+			},
+			errStrings: []string{},
+		},
+		{
+			name: "with samesite \"invalid\"",
+			cookie: options.Cookie{
+				Name:     validName,
+				Secret:   validSecret,
+				Domains:  emptyDomains,
+				Path:     "",
+				Expire:   time.Hour,
+				Refresh:  15 * time.Minute,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "invalid",
+			},
+			errStrings: []string{
+				invalidSameSiteMsg,
+			},
+		},
+		{
+			name: "with a combination of configuration errors",
+			cookie: options.Cookie{
+				Name:     invalidName,
+				Secret:   invalidSecret,
+				Domains:  domains,
+				Path:     "",
+				Expire:   15 * time.Minute,
+				Refresh:  time.Hour,
+				Secure:   true,
+				HTTPOnly: false,
+				SameSite: "invalid",
+			},
+			errStrings: []string{
+				invalidNameMsg,
+				invalidSecretMsg,
+				refreshLongerThanExpireMsg,
+				invalidSameSiteMsg,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			errStrings := validateCookie(tc.cookie)
+			g := NewWithT(t)
+
+			g.Expect(errStrings).To(ConsistOf(tc.errStrings))
+			// Check domains were sorted to the right lengths
+			for i := 0; i < len(tc.cookie.Domains)-1; i++ {
+				g.Expect(len(tc.cookie.Domains[i])).To(BeNumerically(">=", len(tc.cookie.Domains[i+1])))
+			}
+		})
+	}
+}

pkg/validation/options.go

@@ -10,14 +10,12 @@ import (
 	"net/url"
 	"os"
 	"regexp"
-	"sort"
 	"strings"
 
 	"github.com/coreos/go-oidc"
 	"github.com/dgrijalva/jwt-go"
 	"github.com/mbland/hmacauth"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/ip"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/requests"
@@ -28,7 +26,8 @@ import (
 // Validate checks that required options are set and validates those that they
 // are of the correct format
 func Validate(o *options.Options) error {
-	msgs := make([]string, 0)
+	msgs := validateCookie(o.Cookie)
+
 	if o.SSLInsecureSkipVerify {
 		insecureTransport := &http.Transport{
 			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
@@ -49,30 +48,6 @@ func Validate(o *options.Options) error {
 		}
 	}
 
-	if o.Cookie.Secret == "" {
-		msgs = append(msgs, "missing setting: cookie-secret")
-	} else {
-		validCookieSecretSize := false
-		for _, i := range []int{16, 24, 32} {
-			if len(encryption.SecretBytes(o.Cookie.Secret)) == i {
-				validCookieSecretSize = true
-			}
-		}
-		var decoded bool
-		if string(encryption.SecretBytes(o.Cookie.Secret)) != o.Cookie.Secret {
-			decoded = true
-		}
-		if !validCookieSecretSize {
-			var suffix string
-			if decoded {
-				suffix = " note: cookie secret was base64 decoded"
-			}
-			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))
-		}
-	}
-
 	if o.ClientID == "" {
 		msgs = append(msgs, "missing setting: client-id")
 	}
@@ -222,14 +197,6 @@ func Validate(o *options.Options) error {
 	}
 	msgs = parseProviderInfo(o, msgs)
 
-	if o.Cookie.Refresh >= o.Cookie.Expire {
-		msgs = append(msgs, fmt.Sprintf(
-			"cookie_refresh (%s) must be less than "+
-				"cookie_expire (%s)",
-			o.Cookie.Refresh.String(),
-			o.Cookie.Expire.String()))
-	}
-
 	if len(o.GoogleGroups) > 0 || o.GoogleAdminEmail != "" || o.GoogleServiceAccountJSON != "" {
 		if len(o.GoogleGroups) < 1 {
 			msgs = append(msgs, "missing setting: google-group")
@@ -242,20 +209,7 @@ func Validate(o *options.Options) error {
 		}
 	}
 
-	switch o.Cookie.SameSite {
-	case "", "none", "lax", "strict":
-	default:
-		msgs = append(msgs, fmt.Sprintf("cookie_samesite (%s) must be one of ['', 'lax', 'strict', 'none']", o.Cookie.SameSite))
-	}
-
-	// Sort cookie domains by length, so that we try longer (and more specific)
-	// domains first
-	sort.Slice(o.Cookie.Domains, func(i, j int) bool {
-		return len(o.Cookie.Domains[i]) > len(o.Cookie.Domains[j])
-	})
-
 	msgs = parseSignatureKey(o, msgs)
-	msgs = validateCookieName(o, msgs)
 	msgs = configureLogger(o.Logging, msgs)
 
 	if o.ReverseProxy {
@@ -442,14 +396,6 @@ func newVerifierFromJwtIssuer(jwtIssuer jwtIssuer) (*oidc.IDTokenVerifier, error
 	return verifier, nil
 }
 
-func validateCookieName(o *options.Options, msgs []string) []string {
-	cookie := &http.Cookie{Name: o.Cookie.Name}
-	if cookie.String() == "" {
-		return append(msgs, fmt.Sprintf("invalid cookie name: %q", o.Cookie.Name))
-	}
-	return msgs
-}
-
 // jwtIssuer hold parsed JWT issuer info that's used to construct a verifier.
 type jwtIssuer struct {
 	issuerURI string

pkg/validation/options_test.go

@@ -2,7 +2,6 @@ package validation
 
 import (
 	"crypto"
-	"fmt"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -291,20 +290,6 @@ func TestValidateSignatureKeyUnsupportedAlgorithm(t *testing.T) {
 		"  unsupported signature hash algorithm: "+o.SignatureKey)
 }
 
-func TestValidateCookie(t *testing.T) {
-	o := testOptions()
-	o.Cookie.Name = "_valid_cookie_name"
-	assert.Equal(t, nil, Validate(o))
-}
-
-func TestValidateCookieBadName(t *testing.T) {
-	o := testOptions()
-	o.Cookie.Name = "_bad_cookie_name{}"
-	err := Validate(o)
-	assert.Equal(t, err.Error(), "invalid configuration:\n"+
-		fmt.Sprintf("  invalid cookie name: %q", o.Cookie.Name))
-}
-
 func TestSkipOIDCDiscovery(t *testing.T) {
 	o := testOptions()
 	o.ProviderType = "oidc"