From b3ba2594c696b338fcb5ad1a0bd4aa86f84eb7ce Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sat, 23 May 2020 17:04:32 +0100 Subject: [PATCH 1/6] Create Cookie FlagSet and Defaults --- pkg/apis/options/cookie.go | 37 ++++++++++++++++++++++++++++++++++++- pkg/apis/options/options.go | 21 ++------------------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index e3e18e0e..6a9b1ca6 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -1,6 +1,10 @@ package options -import "time" +import ( + "time" + + "github.com/spf13/pflag" +) // CookieOptions contains configuration options relating to Cookie configuration type CookieOptions struct { @@ -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 +} + +// defaultCookieOptions creates a CookieOptions populating each field with its default value +func defaultCookieOptions() CookieOptions { + return CookieOptions{ + 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..e933ca16 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -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: defaultCookieOptions(), 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 From 900061b88a59dcd79b462efb3ba57b6ca21b1663 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sun, 24 May 2020 11:16:45 +0100 Subject: [PATCH 2/6] Move CookieOptions validation to it's own file --- pkg/validation/cookie.go | 69 +++++++++++++++++++++++++++++++++++++++ pkg/validation/options.go | 58 ++------------------------------ 2 files changed, 71 insertions(+), 56 deletions(-) create mode 100644 pkg/validation/cookie.go diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go new file mode 100644 index 00000000..c3cc44aa --- /dev/null +++ b/pkg/validation/cookie.go @@ -0,0 +1,69 @@ +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 validateCookieOptions(o options.CookieOptions) []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 { + cookie := &http.Cookie{Name: name} + if cookie.String() == "" { + return []string{fmt.Sprintf("invalid cookie name: %q", name)} + } + return []string{} +} + +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 + + // If the secretBytes is different to the raw secret, it was decoded from Base64 + // Add a note to the error message + var decodedSuffix string + if string(secretBytes) != secret { + decodedSuffix = " note: cookie secret was base64 decoded" + } + + return []string{fmt.Sprintf( + "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes.%s", + len(secretBytes), decodedSuffix)} +} diff --git a/pkg/validation/options.go b/pkg/validation/options.go index e7ea18ba..d7d4270b 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 := validateCookieOptions(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 From 285c65a2d4e9117a25361db2010e15a66b808233 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 25 May 2020 12:01:16 +0100 Subject: [PATCH 3/6] Add tests for cookie validation This also removes the check for the decoded from the valid secret size check. The code was unreachable because encryption.SecretBytes will only return the decoded secret if it was the right length after decoding. --- pkg/validation/cookie.go | 13 +- pkg/validation/cookie_test.go | 250 +++++++++++++++++++++++++++++++++ pkg/validation/options_test.go | 15 -- 3 files changed, 253 insertions(+), 25 deletions(-) create mode 100644 pkg/validation/cookie_test.go diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go index c3cc44aa..e5f50246 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -55,15 +55,8 @@ func validateCookieSecret(secret string) []string { return []string{} } // Invalid secret size found, return a message - - // If the secretBytes is different to the raw secret, it was decoded from Base64 - // Add a note to the error message - var decodedSuffix string - if string(secretBytes) != secret { - decodedSuffix = " note: cookie secret was base64 decoded" - } - return []string{fmt.Sprintf( - "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes.%s", - len(secretBytes), decodedSuffix)} + "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..af4cd492 --- /dev/null +++ b/pkg/validation/cookie_test.go @@ -0,0 +1,250 @@ +package validation + +import ( + "testing" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options" + . "github.com/onsi/gomega" +) + +func TestValidateCookie(t *testing.T) { + validName := "_oauth2_proxy" + invalidName := "_oauth2;proxy" // Separater character not allowed + 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\"" + 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.CookieOptions + errStrings []string + }{ + { + name: "with valid configuration", + cookie: options.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + Name: invalidName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Refresh: 15 * time.Minute, + Secure: true, + HTTPOnly: false, + SameSite: "", + }, + errStrings: []string{ + invalidNameMsg, + }, + }, + { + name: "with refresh longer than expire", + cookie: options.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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.CookieOptions{ + 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 := validateCookieOptions(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_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" From 211fd3a010f86d3a4bfc64788bdd29a2cb7c7c3d Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 25 May 2020 12:43:24 +0100 Subject: [PATCH 4/6] Rename CookieOptions to Cookie --- pkg/apis/options/cookie.go | 10 ++--- pkg/apis/options/options.go | 4 +- pkg/cookies/cookies.go | 2 +- pkg/sessions/cookie/session_store.go | 24 +++++------ pkg/sessions/cookie/session_store_test.go | 2 +- pkg/sessions/redis/redis_store.go | 50 +++++++++++------------ pkg/sessions/redis/session_store_test.go | 6 +-- pkg/sessions/session_store.go | 2 +- pkg/sessions/session_store_test.go | 4 +- pkg/sessions/tests/session_store_tests.go | 8 ++-- pkg/validation/cookie.go | 2 +- pkg/validation/cookie_test.go | 28 ++++++------- pkg/validation/options.go | 2 +- 13 files changed, 72 insertions(+), 72 deletions(-) diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 6a9b1ca6..46ae9c08 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -6,8 +6,8 @@ import ( "github.com/spf13/pflag" ) -// CookieOptions contains configuration options relating to Cookie configuration -type CookieOptions struct { +// 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"` @@ -35,9 +35,9 @@ func cookieFlagSet() *pflag.FlagSet { return flagSet } -// defaultCookieOptions creates a CookieOptions populating each field with its default value -func defaultCookieOptions() CookieOptions { - return CookieOptions{ +// cookieDefaults creates a Cookie populating each field with its default value +func cookieDefaults() Cookie { + return Cookie{ Name: "_oauth2_proxy", Secret: "", Domains: nil, diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index e933ca16..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,7 +153,7 @@ func NewOptions() *Options { RealClientIPHeader: "X-Real-IP", ForceHTTPS: false, DisplayHtpasswdForm: true, - Cookie: defaultCookieOptions(), + Cookie: cookieDefaults(), Session: SessionOptions{ Type: "cookie", }, 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 index e5f50246..db86c0f3 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -9,7 +9,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption" ) -func validateCookieOptions(o options.CookieOptions) []string { +func validateCookie(o options.Cookie) []string { msgs := validateCookieSecret(o.Secret) if o.Refresh >= o.Expire { diff --git a/pkg/validation/cookie_test.go b/pkg/validation/cookie_test.go index af4cd492..26ad0d45 100644 --- a/pkg/validation/cookie_test.go +++ b/pkg/validation/cookie_test.go @@ -33,12 +33,12 @@ func TestValidateCookie(t *testing.T) { testCases := []struct { name string - cookie options.CookieOptions + cookie options.Cookie errStrings []string }{ { name: "with valid configuration", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: domains, @@ -53,7 +53,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with no cookie secret", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: "", Domains: emptyDomains, @@ -70,7 +70,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with an invalid cookie secret", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: invalidSecret, Domains: emptyDomains, @@ -87,7 +87,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with a valid Base64 secret", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validBase64Secret, Domains: emptyDomains, @@ -102,7 +102,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with an invalid Base64 secret", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: invalidBase64Secret, Domains: emptyDomains, @@ -119,7 +119,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with an invalid name", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: invalidName, Secret: validSecret, Domains: emptyDomains, @@ -136,7 +136,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with refresh longer than expire", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: emptyDomains, @@ -153,7 +153,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with samesite \"none\"", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: emptyDomains, @@ -168,7 +168,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with samesite \"lax\"", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: emptyDomains, @@ -183,7 +183,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with samesite \"strict\"", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: emptyDomains, @@ -198,7 +198,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with samesite \"invalid\"", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: validName, Secret: validSecret, Domains: emptyDomains, @@ -215,7 +215,7 @@ func TestValidateCookie(t *testing.T) { }, { name: "with a combination of configuration errors", - cookie: options.CookieOptions{ + cookie: options.Cookie{ Name: invalidName, Secret: invalidSecret, Domains: domains, @@ -237,7 +237,7 @@ func TestValidateCookie(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - errStrings := validateCookieOptions(tc.cookie) + errStrings := validateCookie(tc.cookie) g := NewWithT(t) g.Expect(errStrings).To(ConsistOf(tc.errStrings)) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index d7d4270b..50717b60 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -26,7 +26,7 @@ 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 := validateCookieOptions(o.Cookie) + msgs := validateCookie(o.Cookie) if o.SSLInsecureSkipVerify { insecureTransport := &http.Transport{ From eb933cc3f493b9e26bd45ca9c1c4803916482857 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sun, 31 May 2020 15:17:51 +0100 Subject: [PATCH 5/6] Add changelog entry for cookie validation separation --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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) From 3e13f3197f0628110f525e8b454e7f1d142452a3 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Sat, 4 Jul 2020 16:08:12 +0100 Subject: [PATCH 6/6] Ensure that cookie names over 256 characters are rejected by validation --- pkg/validation/cookie.go | 10 ++++++++-- pkg/validation/cookie_test.go | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go index db86c0f3..2d5a557a 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -35,11 +35,17 @@ func validateCookie(o options.Cookie) []string { } func validateCookieName(name string) []string { + msgs := []string{} + cookie := &http.Cookie{Name: name} if cookie.String() == "" { - return []string{fmt.Sprintf("invalid cookie name: %q", name)} + msgs = append(msgs, fmt.Sprintf("invalid cookie name: %q", name)) } - return []string{} + + 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 { diff --git a/pkg/validation/cookie_test.go b/pkg/validation/cookie_test.go index 26ad0d45..ac2e7951 100644 --- a/pkg/validation/cookie_test.go +++ b/pkg/validation/cookie_test.go @@ -1,6 +1,7 @@ package validation import ( + "strings" "testing" "time" @@ -9,8 +10,12 @@ import ( ) 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" @@ -25,6 +30,7 @@ func TestValidateCookie(t *testing.T) { } 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" @@ -134,6 +140,23 @@ func TestValidateCookie(t *testing.T) { 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{