diff --git a/CHANGELOG.md b/CHANGELOG.md index d9157bde..6d92a1bd 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index e3e18e0e..46ae9c08 100644 --- a/pkg/apis/options/cookie.go +++ b/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: "", + } +} diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 1f113757..52d3cf02 100644 --- a/pkg/apis/options/options.go +++ b/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 diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 5235bb66..f3879587 100644 --- a/pkg/cookies/cookies.go +++ b/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 != "" { diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 833ed729..6fa6b5ea 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/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_ - 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 } diff --git a/pkg/sessions/cookie/session_store_test.go b/pkg/sessions/cookie/session_store_test.go index c67aafbe..a2670c2c 100644 --- a/pkg/sessions/cookie/session_store_test.go +++ b/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) diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index 2b79aebc..a89349e8 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/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() diff --git a/pkg/sessions/redis/session_store_test.go b/pkg/sessions/redis/session_store_test.go index 0fd1796e..78dd111f 100644 --- a/pkg/sessions/redis/session_store_test.go +++ b/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} diff --git a/pkg/sessions/session_store.go b/pkg/sessions/session_store.go index 992d6ded..ba102af8 100644 --- a/pkg/sessions/session_store.go +++ b/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) diff --git a/pkg/sessions/session_store_test.go b/pkg/sessions/session_store_test.go index c472aaaa..37c35888 100644 --- a/pkg/sessions/session_store_test.go +++ b/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: "/", diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 1b8d6e1c..8778efe8 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/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, diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go new file mode 100644 index 00000000..2d5a557a --- /dev/null +++ b/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)), + } +} diff --git a/pkg/validation/cookie_test.go b/pkg/validation/cookie_test.go new file mode 100644 index 00000000..ac2e7951 --- /dev/null +++ b/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]))) + } + }) + } +} diff --git a/pkg/validation/options.go b/pkg/validation/options.go index e7ea18ba..50717b60 100644 --- a/pkg/validation/options.go +++ b/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 diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 3951c1ae..51525555 100644 --- a/pkg/validation/options_test.go +++ b/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"