From 285c65a2d4e9117a25361db2010e15a66b808233 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 25 May 2020 12:01:16 +0100 Subject: [PATCH] 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"