diff --git a/CHANGELOG.md b/CHANGELOG.md index c3a95f7e..0e0a4b23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [#3369](https://github.com/oauth2-proxy/oauth2-proxy/pull/3369) fix: use CSRFExpire instead of Expire for CSRF cookie validation (@Br1an67) - [#3365](https://github.com/oauth2-proxy/oauth2-proxy/pull/3365) fix: filter empty strings from allowed groups (@Br1an67) - [#3338](https://github.com/oauth2-proxy/oauth2-proxy/pull/3338) feat: add --config-test flag for validating configuration (@MayorFaj) +- [#3347](https://github.com/oauth2-proxy/oauth2-proxy/pull/3347) feat: add same site option for csrf cookies (@jvnoije) # V7.14.3 diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 7d8a1c09..c225228e 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -120,7 +120,7 @@ Provider specific options can be found on their respective subpages. | flag: `--approval-prompt`
toml: `approval_prompt` | string | OAuth approval_prompt | `"force"` | | flag: `--backend-logout-url`
toml: `backend_logout_url` | string | URL to perform backend logout, if you use `{id_token}` in the url it will be replaced by the actual `id_token` of the user session | | | flag: `--client-id`
toml: `client_id` | string | the OAuth Client ID, e.g. `"123456.apps.googleusercontent.com"` | | -| flag: `--client-secret-file`
toml: `client_secret_file` | string | the file with OAuth Client Secret. The file must contain the secret only, with no trailing newline | | +| flag: `--client-secret-file`
toml: `client_secret_file` | string | the file with OAuth Client Secret. The file must contain the secret only, with no trailing newline | | | flag: `--client-secret`
toml: `client_secret` | string | the OAuth Client Secret | | | flag: `--code-challenge-method`
toml: `code_challenge_method` | string | use PKCE code challenges with the specified method. Either 'plain' or 'S256' (recommended) | | | flag: `--insecure-oidc-allow-unverified-email`
toml: `insecure_oidc_allow_unverified_email` | bool | don't fail if an email address in an id_token is not verified | false | @@ -140,7 +140,7 @@ Provider specific options can be found on their respective subpages. | flag: `--oidc-enabled-signing-alg`
toml: `oidc_enabled_signing_algs` | string \| list | List of allowed JWT signing algorithms. When oidc discovery is enabled, the effective set is the intersection between this list and the provider's discovered supported algorithms. | | | flag: `--profile-url`
toml: `profile_url` | string | Profile access endpoint | | | flag: `--prompt`
toml: `prompt` | string | [OIDC prompt](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest); if present, `approval-prompt` is ignored | `""` | -| flag: `--provider-ca-file`
toml: `provider_ca_files` | string \| list | Paths to CA certificates that should be used when connecting to the provider. If not specified, the default Go trust sources are used instead. | +| flag: `--provider-ca-file`
toml: `provider_ca_files` | string \| list | Paths to CA certificates that should be used when connecting to the provider. If not specified, the default Go trust sources are used instead. | | | flag: `--provider-display-name`
toml: `provider_display_name` | string | Override the provider's name with the given string; used for the sign-in page | (depends on provider) | | flag: `--provider`
toml: `provider` | string | OAuth provider | google | | flag: `--pubjwk-url`
toml: `pubjwk_url` | string | JWK pubkey access endpoint: required by login.gov | | @@ -158,6 +158,7 @@ Provider specific options can be found on their respective subpages. | flag: `--cookie-csrf-expire`
toml: `cookie_csrf_expire` | duration | expire timeframe for CSRF cookie | 15m | | flag: `--cookie-csrf-per-request`
toml:`cookie_csrf_per_request` | bool | Enable having different CSRF cookies per request, making it possible to have parallel requests. | false | | flag: `--cookie-csrf-per-request-limit`
toml: `cookie_csrf_per_request_limit` | int | Sets a limit on the number of CSRF requests cookies that oauth2-proxy will create. The oldest cookie will be removed. Useful if users end up with 431 Request headers too large status codes. Only effective if --cookie-csrf-per-request is true | "infinite" | +| flag: `--cookie-csrf-samesite`
toml: `cookie_csrf_samesite` | string | set SameSite CSRF cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`). When using the default setting, the CSRF cookie samesite value is taken from the session cookie configuration. | `""` | | flag: `--cookie-domain`
toml: `cookie_domains` | string \| list | Optional cookie domains to force cookies to (e.g. `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match). | | | flag: `--cookie-expire`
toml: `cookie_expire` | duration | expire timeframe for cookie. If set to 0, cookie becomes a session-cookie which will expire when the browser is closed. | 168h0m0s | | flag: `--cookie-httponly`
toml: `cookie_httponly` | bool | set HttpOnly cookie flag | true | @@ -166,7 +167,7 @@ Provider specific options can be found on their respective subpages. | flag: `--cookie-refresh`
toml: `cookie_refresh` | duration | refresh the cookie after this duration; `0` to disable; not supported by all providers [^1] | | | flag: `--cookie-samesite`
toml: `cookie_samesite` | string | set SameSite cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`). | `""` | | flag: `--cookie-secret`
toml: `cookie_secret` | string | the seed string for secure cookies (optionally base64 encoded) | | -| flag: `--cookie-secret-file`
toml: `cookie_secret_file` | string | File containing the cookie secret (must be raw binary, exactly 16, 24, or 32 bytes). Use dd if=/dev/urandom bs=32 count=1 > cookie.secret to generate | | +| flag: `--cookie-secret-file`
toml: `cookie_secret_file` | string | File containing the cookie secret (must be raw binary, exactly 16, 24, or 32 bytes). Use dd if=/dev/urandom bs=32 count=1 > cookie.secret to generate | | | flag: `--cookie-secure`
toml: `cookie_secure` | bool | set [secure (HTTPS only) cookie flag](https://owasp.org/www-community/controls/SecureFlag) | true | [^1]: The following providers support `--cookie-refresh`: ADFS, Azure, GitLab, Google, Keycloak and all other Identity Providers which support the full [OIDC specification](https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokens) @@ -212,7 +213,7 @@ Provider specific options can be found on their respective subpages. | Flag / Config Field | Type | Description | Default | | ----------------------------------------------------------------- | ------ | --------------------------------------------------------------------------------------------------------------------------- | ------- | | flag: `--banner`
toml: `banner` | string | custom (html) banner string. Use `"-"` to disable default banner. | | -| flag: `--custom-sign-in-logo`
toml: `custom_sign_in_logo` | string | path or a URL to an custom image for the sign_in page logo. Use `"-"` to disable default logo. | +| flag: `--custom-sign-in-logo`
toml: `custom_sign_in_logo` | string | path or a URL to an custom image for the sign_in page logo. Use `"-"` to disable default logo. | | | flag: `--custom-templates-dir`
toml: `custom_templates_dir` | string | path to custom html templates | | | flag: `--display-htpasswd-form`
toml: `display_htpasswd_form` | bool | display username / password login form if an htpasswd file is provided | true | | flag: `--footer`
toml: `footer` | string | custom (html) footer string. Use `"-"` to disable default footer. (Can be used to obfuscate the version) | | diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 3653b7d0..3dee9505 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -24,6 +24,7 @@ type Cookie struct { CSRFPerRequest bool `flag:"cookie-csrf-per-request" cfg:"cookie_csrf_per_request"` CSRFPerRequestLimit int `flag:"cookie-csrf-per-request-limit" cfg:"cookie_csrf_per_request_limit"` CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` + CSRFSameSite string `flag:"cookie-csrf-samesite" cfg:"cookie_csrf_samesite"` } func cookieFlagSet() *pflag.FlagSet { @@ -42,6 +43,7 @@ func cookieFlagSet() *pflag.FlagSet { flagSet.Bool("cookie-csrf-per-request", false, "When this property is set to true, then the CSRF cookie name is built based on the state and varies per request. If property is set to false, then CSRF cookie has the same name for all requests.") flagSet.Int("cookie-csrf-per-request-limit", 0, "Sets a limit on the number of CSRF requests cookies that oauth2-proxy will create. The oldest cookies will be removed. Useful if users end up with 431 Request headers too large status codes.") flagSet.Duration("cookie-csrf-expire", time.Duration(15)*time.Minute, "expire timeframe for CSRF cookie") + flagSet.String("cookie-csrf-samesite", "", "set SameSite CSRF cookie attribute (ie: \"lax\", \"strict\", \"none\", or \"\"). When using the default setting, the CSRF cookie samesite value is taken from the session cookie configuration.") return flagSet } @@ -61,6 +63,7 @@ func cookieDefaults() Cookie { CSRFPerRequest: false, CSRFPerRequestLimit: 0, CSRFExpire: time.Duration(15) * time.Minute, + CSRFSameSite: "", } } diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 24ae2841..be5ee451 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -7,14 +7,24 @@ import ( "strings" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util" ) +type CookieOptions struct { + Name string + Value string + Domains []string + Expiration time.Duration + SameSite string + Path string + HTTPOnly bool + Secure bool +} + // MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions, // value and creation time -func MakeCookieFromOptions(req *http.Request, name string, value string, opts *options.Cookie, expiration time.Duration) *http.Cookie { +func MakeCookieFromOptions(req *http.Request, opts *CookieOptions) *http.Cookie { domain := GetCookieDomain(req, opts.Domains) // If nothing matches, create the cookie with the shortest domain if domain == "" && len(opts.Domains) > 0 { @@ -26,8 +36,8 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o } c := &http.Cookie{ - Name: name, - Value: value, + Name: opts.Name, + Value: opts.Value, Path: opts.Path, Domain: domain, HttpOnly: opts.HTTPOnly, @@ -35,9 +45,9 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o SameSite: ParseSameSite(opts.SameSite), } - if expiration > time.Duration(0) { - c.MaxAge = int(expiration.Seconds()) - } else if expiration < time.Duration(0) { + if opts.Expiration > time.Duration(0) { + c.MaxAge = int(opts.Expiration.Seconds()) + } else if opts.Expiration < time.Duration(0) { c.MaxAge = -1 } @@ -58,7 +68,7 @@ func GetCookieDomain(req *http.Request, cookieDomains []string) string { return "" } -// Parse a valid http.SameSite value from a user supplied string for use of making cookies. +// ParseSameSite a valid http.SameSite value from a user supplied string for use of making cookies. func ParseSameSite(v string) http.SameSite { switch v { case "lax": diff --git a/pkg/cookies/cookies_suite_test.go b/pkg/cookies/cookies_suite_test.go index f4893cbd..a11dd798 100644 --- a/pkg/cookies/cookies_suite_test.go +++ b/pkg/cookies/cookies_suite_test.go @@ -17,6 +17,10 @@ const ( cookieDomain = "o2p.cookies.test" cookiePath = "/cookie-tests" + sameSiteLax = "lax" + sameSiteStrict = "strict" + sameSiteNone = "none" + nowEpoch = 1609366421 ) diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go index ef0fbd4f..b67f8a69 100644 --- a/pkg/cookies/cookies_test.go +++ b/pkg/cookies/cookies_test.go @@ -5,8 +5,6 @@ import ( "net/http" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" - middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -82,16 +80,12 @@ var _ = Describe("Cookie Tests", func() { Context("MakeCookieFromOptions", func() { type makeCookieFromOptionsTableInput struct { host string - name string - value string - opts options.Cookie - expiration time.Duration + opts CookieOptions now time.Time expectedOutput int } validName := "_oauth2_proxy" - validSecret := "secretthirtytwobytes+abcdefghijk" domains := []string{"www.cookies.test"} now := time.Now() @@ -106,62 +100,50 @@ var _ = Describe("Cookie Tests", func() { ) Expect(err).ToNot(HaveOccurred()) - Expect(MakeCookieFromOptions(req, in.name, in.value, &in.opts, in.expiration).MaxAge).To(Equal(in.expectedOutput)) + Expect(MakeCookieFromOptions(req, &in.opts).MaxAge).To(Equal(in.expectedOutput)) }, Entry("persistent cookie", makeCookieFromOptionsTableInput{ - host: "www.cookies.test", - name: validName, - value: "1", - opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: true, - HTTPOnly: false, - SameSite: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: 15 * time.Minute, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: 15 * time.Minute, now: now, expectedOutput: int((15 * time.Minute).Seconds()), }), Entry("persistent cookie to be cleared", makeCookieFromOptionsTableInput{ - host: "www.cookies.test", - name: validName, - value: "1", - opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: time.Hour * -1, - Refresh: 15 * time.Minute, - Secure: true, - HTTPOnly: false, - SameSite: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: time.Hour * -1, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: time.Hour * -1, now: now, expectedOutput: -1, }), Entry("session cookie", makeCookieFromOptionsTableInput{ - host: "www.cookies.test", - name: validName, - value: "1", - opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: 0, - Refresh: 15 * time.Minute, - Secure: true, - HTTPOnly: false, - SameSite: "", + host: "www.cookies.test", + opts: CookieOptions{ + Name: validName, + Value: "1", + Domains: domains, + Expiration: 0, + SameSite: "", + Path: "", + HTTPOnly: false, + Secure: true, }, - expiration: 0, now: now, expectedOutput: expectedMaxAge, }), diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index 6fc55716..2614f378 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -134,6 +134,15 @@ func (c *csrf) SetSessionNonce(s *sessions.SessionState) { s.Nonce = c.OIDCNonce } +// getCSRFSameSite get the CSRF same site +func getCSRFSameSite(opts *options.Cookie) string { + sameSite := opts.CSRFSameSite + if sameSite == "" { + sameSite = opts.SameSite + } + return sameSite +} + // SetCookie encodes the CSRF to a signed cookie and sets it on the ResponseWriter func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cookie, error) { encoded, err := c.encodeCookie() @@ -141,13 +150,18 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki return nil, err } - cookie := MakeCookieFromOptions( - req, - c.cookieName(), - encoded, - c.cookieOpts, - c.cookieOpts.CSRFExpire, - ) + csrfCookieOptions := &CookieOptions{ + Name: c.cookieName(), + Value: encoded, + Domains: c.cookieOpts.Domains, + Expiration: c.cookieOpts.CSRFExpire, + SameSite: getCSRFSameSite(c.cookieOpts), + Path: c.cookieOpts.Path, + HTTPOnly: c.cookieOpts.HTTPOnly, + Secure: c.cookieOpts.Secure, + } + + cookie := MakeCookieFromOptions(req, csrfCookieOptions) http.SetCookie(rw, cookie) return cookie, nil @@ -197,13 +211,18 @@ func ClearExtraCsrfCookies(opts *options.Cookie, rw http.ResponseWriter, req *ht // ClearCookie removes the CSRF cookie func (c *csrf) ClearCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, MakeCookieFromOptions( - req, - c.cookieName(), - "", - c.cookieOpts, - time.Hour*-1, - )) + csrfCookieOptions := &CookieOptions{ + Name: c.cookieName(), + Value: "", + Domains: c.cookieOpts.Domains, + Expiration: time.Hour * -1, + SameSite: getCSRFSameSite(c.cookieOpts), + Path: c.cookieOpts.Path, + HTTPOnly: c.cookieOpts.HTTPOnly, + Secure: c.cookieOpts.Secure, + } + + http.SetCookie(rw, MakeCookieFromOptions(req, csrfCookieOptions)) } // encodeCookie MessagePack encodes and encrypts the CSRF and then creates a diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index 6a17013b..59ff0a8a 100644 --- a/pkg/cookies/csrf_per_request_test.go +++ b/pkg/cookies/csrf_per_request_test.go @@ -216,13 +216,18 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { for _, csrf := range []*csrf{privateCSRF1, privateCSRF2, privateCSRF3} { encoded, err := csrf.encodeCookie() Expect(err).ToNot(HaveOccurred()) - cookie := MakeCookieFromOptions( - req, - csrf.cookieName(), - encoded, - csrf.cookieOpts, - csrf.cookieOpts.CSRFExpire, - ) + csrfCookieOptions := &CookieOptions{ + Name: csrf.cookieName(), + Value: encoded, + Domains: csrf.cookieOpts.Domains, + Expiration: csrf.cookieOpts.CSRFExpire, + SameSite: getCSRFSameSite(csrf.cookieOpts), + Path: csrf.cookieOpts.Path, + HTTPOnly: csrf.cookieOpts.HTTPOnly, + Secure: csrf.cookieOpts.Secure, + } + + cookie := MakeCookieFromOptions(req, csrfCookieOptions) cookies = append(cookies, fmt.Sprintf("%v=%v", cookie.Name, cookie.Value)) } diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index f791045d..3182f663 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -278,4 +278,263 @@ var _ = Describe("CSRF Cookie Tests", func() { }) }) }) + + Context("Test Cookie SameSite", func() { + var req *http.Request + var cookieOpts *options.Cookie + + testNow := time.Unix(nowEpoch, 0) + + BeforeEach(func() { + // we need to reset the time to ensure the cookie is valid + privateCSRF.clock = time.Now + + req = &http.Request{ + Method: http.MethodGet, + Proto: "HTTP/1.1", + Host: cookieDomain, + + URL: &url.URL{ + Scheme: "https", + Host: cookieDomain, + Path: cookiePath, + }, + } + + cookieOpts = &options.Cookie{ + Name: cookieName, + Secret: cookieSecret, + Domains: []string{cookieDomain}, + Path: cookiePath, + Expire: time.Hour, + Secure: true, + HTTPOnly: true, + CSRFPerRequest: false, + CSRFExpire: time.Hour, + } + }) + + It("Call SetCookie when CSRF SameSite is not defined. Expected result: CSRF cookie SameSite is the same as session cookie.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + _, err := CSRF.SetCookie(rw, req) + + // validate + Expect(err).ToNot(HaveOccurred()) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure; SameSite=Lax", + cookiePath, + cookieDomain, + int(cookieOpts.CSRFExpire.Seconds()), + ), + )) + }) + + It("Call SetCookie when CSRF SameSite is an empty string. Expected result: CSRF cookie SameSite is the same as session cookie.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = "" + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + _, err := CSRF.SetCookie(rw, req) + + // validate + Expect(err).ToNot(HaveOccurred()) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure; SameSite=Lax", + cookiePath, + cookieDomain, + int(cookieOpts.CSRFExpire.Seconds()), + ), + )) + }) + + It("Call SetCookie when CSRF SameSite is 'none'. Expected result: CSRF cookie SameSite is None.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = sameSiteNone + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + _, err := CSRF.SetCookie(rw, req) + + // validate + Expect(err).ToNot(HaveOccurred()) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure; SameSite=None", + cookiePath, + cookieDomain, + int(cookieOpts.CSRFExpire.Seconds()), + ), + )) + }) + + It("Call SetCookie when CSRF SameSite is 'strict'. Expected result: CSRF cookie SameSite is Strict.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = sameSiteStrict + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + _, err := CSRF.SetCookie(rw, req) + + // validate + Expect(err).ToNot(HaveOccurred()) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure; SameSite=Strict", + cookiePath, + cookieDomain, + int(cookieOpts.CSRFExpire.Seconds()), + ), + )) + }) + + It("Call SetCookie when CSRF SameSite is 'lax'. Expected result: CSRF cookie SameSite is Lax.", func() { + // prepare + cookieOpts.SameSite = sameSiteStrict + cookieOpts.CSRFSameSite = sameSiteLax + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + _, err := CSRF.SetCookie(rw, req) + + // validate + Expect(err).ToNot(HaveOccurred()) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Max-Age=%d; HttpOnly; Secure; SameSite=Lax", + cookiePath, + cookieDomain, + int(cookieOpts.CSRFExpire.Seconds()), + ), + )) + }) + + It("Call ClearCookie when CSRF SameSite is not defined. Expected result: CSRF cookie SameSite is the same as session cookie.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + CSRF.ClearCookie(rw, req) + + // validate + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure; SameSite=Lax", + CSRF.(*csrf).cookieName(), + cookiePath, + cookieDomain, + ), + )) + }) + + It("Call ClearCookie when CSRF SameSite is an empty string. Expected result: CSRF cookie SameSite is the same as session cookie.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = "" + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + CSRF.ClearCookie(rw, req) + + // validate + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure; SameSite=Lax", + CSRF.(*csrf).cookieName(), + cookiePath, + cookieDomain, + ), + )) + }) + + It("Call ClearCookie when CSRF SameSite is 'none'. Expected result: CSRF cookie SameSite is None.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = sameSiteNone + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + CSRF.ClearCookie(rw, req) + + // validate + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure; SameSite=None", + CSRF.(*csrf).cookieName(), + cookiePath, + cookieDomain, + ), + )) + }) + + It("Call ClearCookie when CSRF SameSite is 'strict'. Expected result: CSRF cookie SameSite is Strict.", func() { + // prepare + cookieOpts.SameSite = sameSiteLax + cookieOpts.CSRFSameSite = sameSiteStrict + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + CSRF.ClearCookie(rw, req) + + // validate + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure; SameSite=Strict", + CSRF.(*csrf).cookieName(), + cookiePath, + cookieDomain, + ), + )) + }) + + It("Call ClearCookie when CSRF SameSite is 'lax'. Expected result: CSRF cookie SameSite is Lax.", func() { + // prepare + cookieOpts.SameSite = sameSiteStrict + cookieOpts.CSRFSameSite = sameSiteLax + CSRF, _ := NewCSRF(cookieOpts, "verifier") + rw := httptest.NewRecorder() + CSRF.(*csrf).clock = func() time.Time { return testNow } + + // test + CSRF.ClearCookie(rw, req) + + // validate + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure; SameSite=Lax", + CSRF.(*csrf).cookieName(), + cookiePath, + cookieDomain, + ), + )) + }) + }) }) diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 095bc0e7..a4da3734 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -76,7 +76,17 @@ func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error { for _, c := range req.Cookies() { if cookieNameRegex.MatchString(c.Name) { - clearCookie := s.makeCookie(req, c.Name, "", time.Hour*-1) + sessionCookieOptions := &pkgcookies.CookieOptions{ + Name: c.Name, + Value: "", + Domains: s.Cookie.Domains, + Expiration: time.Hour * -1, + SameSite: s.Cookie.SameSite, + Path: s.Cookie.Path, + HTTPOnly: s.Cookie.HTTPOnly, + Secure: s.Cookie.Secure, + } + clearCookie := pkgcookies.MakeCookieFromOptions(req, sessionCookieOptions) http.SetCookie(rw, clearCookie) } @@ -117,7 +127,7 @@ func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Reques return nil } -// makeSessionCookie creates an http.Cookie containing the authenticated user's +// makeSessionCookie creates a http.Cookie containing the authenticated user's // authentication details func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now time.Time) ([]*http.Cookie, error) { strValue := string(value) @@ -132,23 +142,23 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now ti return nil, err } } - c := s.makeCookie(req, s.Cookie.Name, strValue, s.Cookie.Expire) + sessionCookieOptions := &pkgcookies.CookieOptions{ + Name: s.Cookie.Name, + Value: strValue, + Domains: s.Cookie.Domains, + Expiration: s.Cookie.Expire, + SameSite: s.Cookie.SameSite, + Path: s.Cookie.Path, + HTTPOnly: s.Cookie.HTTPOnly, + Secure: s.Cookie.Secure, + } + c := pkgcookies.MakeCookieFromOptions(req, sessionCookieOptions) if len(c.String()) > maxCookieLength { return splitCookie(c), nil } return []*http.Cookie{c}, nil } -func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration) *http.Cookie { - return pkgcookies.MakeCookieFromOptions( - req, - name, - value, - s.Cookie, - expiration, - ) -} - // NewCookieSessionStore initialises a new instance of the SessionStore from // the configuration given func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) { diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 56d6bd9b..c955143a 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -221,13 +221,17 @@ func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *session // clearCookie removes any cookies that would be where this ticket // would set them func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, cookies.MakeCookieFromOptions( - req, - t.options.Name, - "", - t.options, - time.Hour*-1, - )) + cookieOptions := &cookies.CookieOptions{ + Name: t.options.Name, + Value: "", + Domains: t.options.Domains, + Expiration: time.Hour * -1, + SameSite: t.options.SameSite, + Path: t.options.Path, + HTTPOnly: t.options.HTTPOnly, + Secure: t.options.Secure, + } + http.SetCookie(rw, cookies.MakeCookieFromOptions(req, cookieOptions)) } // makeCookie makes a cookie, signing the value if present @@ -244,13 +248,18 @@ func (t *ticket) makeCookie(req *http.Request, value string, expires time.Durati } } - return cookies.MakeCookieFromOptions( - req, - t.options.Name, - value, - t.options, - expires, - ), nil + cookieOptions := &cookies.CookieOptions{ + Name: t.options.Name, + Value: value, + Domains: t.options.Domains, + Expiration: expires, + SameSite: t.options.SameSite, + Path: t.options.Path, + HTTPOnly: t.options.HTTPOnly, + Secure: t.options.Secure, + } + + return cookies.MakeCookieFromOptions(req, cookieOptions), nil } // makeCipher makes a AES-GCM cipher out of the ticket's secret diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 05b67d8d..dd678042 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/pkg/sessions/tests/session_store_tests.go @@ -422,7 +422,17 @@ func SessionStoreInterfaceTests(in *testInput) { broken := "BrokenSessionFromADifferentSessionImplementation" value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now()) Expect(err).ToNot(HaveOccurred()) - cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire) + cookieOptions := &cookiesapi.CookieOptions{ + Name: in.cookieOpts.Name, + Value: value, + Domains: in.cookieOpts.Domains, + Expiration: in.cookieOpts.Expire, + SameSite: in.cookieOpts.SameSite, + Path: in.cookieOpts.Path, + HTTPOnly: in.cookieOpts.HTTPOnly, + Secure: in.cookieOpts.Secure, + } + cookie := cookiesapi.MakeCookieFromOptions(in.request, cookieOptions) in.request.AddCookie(cookie) err = in.ss().Save(in.response, in.request, in.session)