diff --git a/CHANGELOG.md b/CHANGELOG.md index 52a6210d..85fe35c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,9 +3,15 @@ ## Release Highlights ## Important Notes +- [#1708](https://github.com/oauth2-proxy/oauth2-proxy/pull/1708) Enable different CSRF cookies per request (@miguelborges99) + - Since the CSRF cookie name is now longer it could potentially break long cookie names (around 1000 characters). + - Having a unique CSRF cookie per request can lead to quite a number of cookies, in case an application performs a high number of parallel authentication requests. Each call will redirect to /oauth2/start, if the user is not authenticated, and a new cookie will be set. The successfully authenticated requests will have its CSRF cookies immediatly expired, however the failed ones will mantain its CSRF cookies until they expire (by default in 15 minutes). + - The user may redefine the CSRF cookie expiration time using flag "--cookie-csrf-expire" (e.g. --cookie-csrf-expire=5m). By default, it is 15 minutes, but you can fine tune to your environment. ## Breaking Changes +N/A + ## Changes since v7.3.0 - [#1691](https://github.com/oauth2-proxy/oauth2-proxy/pull/1691) Fix Redis IdleTimeout when Redis timeout option is set to non-zero (@dimss) @@ -17,6 +23,10 @@ - [#1774](https://github.com/oauth2-proxy/oauth2-proxy/pull/1774) Fix vulnerabilities CVE-2022-27191, CVE-2021-44716 and CVE-2022-29526 +- [#1708](https://github.com/oauth2-proxy/oauth2-proxy/pull/1708) Enable different CSRF cookies per request (@miguelborges99) + - Add flag "--cookie-csrf-per-request" which activates an algorithm to name CSRF cookies differently per request. + This feature allows parallel callbacks and by default it is disabled. + - Add flag "--cookie-csrf-expire" to define a different expiration time for the CSRF cookie. By default, it is 15 minutes. # V7.3.0 ## Release Highlights diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index a2772fa4..49303228 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -95,6 +95,8 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--cookie-secret` | string | the seed string for secure cookies (optionally base64 encoded) | | | `--cookie-secure` | bool | set [secure (HTTPS only) cookie flag](https://owasp.org/www-community/controls/SecureFlag) | true | | `--cookie-samesite` | string | set SameSite cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`). | `""` | +| `--cookie-csrf-per-request` | bool | Enable having different CSRF cookies per request, making it possible to have parallel requests. | false | +| `--cookie-csrf-expire` | duration | expire timeframe for CSRF cookie | 15m | | `--custom-templates-dir` | string | path to custom html templates | | | `--custom-sign-in-logo` | string | path or a URL to an custom image for the sign_in page logo. Use \"-\" to disable default logo. | | `--display-htpasswd-form` | bool | display username / password login form if an htpasswd file is provided | true | @@ -595,4 +597,4 @@ http: :::note If you set up your OAuth2 provider to rotate your client secret, you can use the `client-secret-file` option to reload the secret when it is updated. -::: \ No newline at end of file +::: diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 46ae9c08..6917bdc5 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -8,15 +8,17 @@ import ( // 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"` - Path string `flag:"cookie-path" cfg:"cookie_path"` - Expire time.Duration `flag:"cookie-expire" cfg:"cookie_expire"` - Refresh time.Duration `flag:"cookie-refresh" cfg:"cookie_refresh"` - Secure bool `flag:"cookie-secure" cfg:"cookie_secure"` - HTTPOnly bool `flag:"cookie-httponly" cfg:"cookie_httponly"` - SameSite string `flag:"cookie-samesite" cfg:"cookie_samesite"` + Name string `flag:"cookie-name" cfg:"cookie_name"` + Secret string `flag:"cookie-secret" cfg:"cookie_secret"` + Domains []string `flag:"cookie-domain" cfg:"cookie_domains"` + Path string `flag:"cookie-path" cfg:"cookie_path"` + Expire time.Duration `flag:"cookie-expire" cfg:"cookie_expire"` + Refresh time.Duration `flag:"cookie-refresh" cfg:"cookie_refresh"` + Secure bool `flag:"cookie-secure" cfg:"cookie_secure"` + HTTPOnly bool `flag:"cookie-httponly" cfg:"cookie_httponly"` + SameSite string `flag:"cookie-samesite" cfg:"cookie_samesite"` + CSRFPerRequest bool `flag:"cookie-csrf-per-request" cfg:"cookie_csrf_per_request"` + CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` } func cookieFlagSet() *pflag.FlagSet { @@ -31,21 +33,24 @@ func cookieFlagSet() *pflag.FlagSet { 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.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.Duration("cookie-csrf-expire", time.Duration(15)*time.Minute, "expire timeframe for CSRF cookie") 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: "", + Name: "_oauth2_proxy", + Secret: "", + Domains: nil, + Path: "/", + Expire: time.Duration(168) * time.Hour, + Refresh: time.Duration(0), + Secure: true, + HTTPOnly: true, + SameSite: "", + CSRFPerRequest: false, + CSRFExpire: time.Duration(15) * time.Minute, } } diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index d7c0d9a2..48e20923 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -48,6 +48,9 @@ type csrf struct { time clock.Clock } +// csrtStateTrim will indicate the length of the state trimmed for the name of the csrf cookie +const csrfStateLength int = 9 + // NewCSRF creates a CSRF with random nonces func NewCSRF(opts *options.Cookie, codeVerifier string) (CSRF, error) { state, err := encryption.Nonce(32) @@ -70,7 +73,10 @@ func NewCSRF(opts *options.Cookie, codeVerifier string) (CSRF, error) { // LoadCSRFCookie loads a CSRF object from a request's CSRF cookie func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { - cookie, err := req.Cookie(csrfCookieName(opts)) + + cookieName := GenerateCookieName(req, opts) + + cookie, err := req.Cookie(cookieName) if err != nil { return nil, err } @@ -78,6 +84,18 @@ func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { return decodeCSRFCookie(cookie, opts) } +// GenerateCookieName in case cookie options state that CSRF cookie has fixed name then set fixed name, otherwise +// build name based on the state +func GenerateCookieName(req *http.Request, opts *options.Cookie) string { + stateSubstring := "" + if opts.CSRFPerRequest { + // csrfCookieName will include a substring of the state to enable multiple csrf cookies + // in case of parallel requests + stateSubstring = ExtractStateSubstring(req) + } + return csrfCookieName(opts, stateSubstring) +} + func (c *csrf) GetCodeVerifier() string { return c.CodeVerifier } @@ -120,7 +138,7 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki c.cookieName(), encoded, c.cookieOpts, - c.cookieOpts.Expire, + c.cookieOpts.CSRFExpire, c.time.Now(), ) http.SetCookie(rw, cookie) @@ -179,14 +197,35 @@ func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) return csrf, nil } -// cookieName returns the CSRF cookie's name derived from the base -// session cookie name +// cookieName returns the CSRF cookie's name func (c *csrf) cookieName() string { - return csrfCookieName(c.cookieOpts) + stateSubstring := "" + if c.cookieOpts.CSRFPerRequest { + stateSubstring = encryption.HashNonce(c.OAuthState)[0 : csrfStateLength-1] + } + return csrfCookieName(c.cookieOpts, stateSubstring) } -func csrfCookieName(opts *options.Cookie) string { - return fmt.Sprintf("%v_csrf", opts.Name) +func csrfCookieName(opts *options.Cookie, stateSubstring string) string { + if stateSubstring == "" { + return fmt.Sprintf("%v_csrf", opts.Name) + } + return fmt.Sprintf("%v_csrf_%v", opts.Name, stateSubstring) +} + +// ExtractStateSubstring extract the initial state characters, to add it to the CSRF cookie name +func ExtractStateSubstring(req *http.Request) string { + lastChar := csrfStateLength - 1 + stateSubstring := "" + + state := req.URL.Query()["state"] + if state[0] != "" { + state := state[0] + if lastChar <= len(state) { + stateSubstring = state[0:lastChar] + } + } + return stateSubstring } func encrypt(data []byte, opts *options.Cookie) ([]byte, error) { diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go new file mode 100644 index 00000000..915dc346 --- /dev/null +++ b/pkg/cookies/csrf_per_request_test.go @@ -0,0 +1,195 @@ +package cookies + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { + var ( + cookieOpts *options.Cookie + publicCSRF CSRF + privateCSRF *csrf + ) + + BeforeEach(func() { + cookieOpts = &options.Cookie{ + Name: cookieName, + Secret: cookieSecret, + Domains: []string{cookieDomain}, + Path: cookiePath, + Expire: time.Hour, + Secure: true, + HTTPOnly: true, + CSRFPerRequest: true, + CSRFExpire: time.Duration(5) * time.Minute, + } + + var err error + publicCSRF, err = NewCSRF(cookieOpts, "verifier") + Expect(err).ToNot(HaveOccurred()) + + privateCSRF = publicCSRF.(*csrf) + }) + + Context("NewCSRF", func() { + It("makes unique nonces for OAuth and OIDC", func() { + Expect(privateCSRF.OAuthState).ToNot(BeEmpty()) + Expect(privateCSRF.OIDCNonce).ToNot(BeEmpty()) + Expect(privateCSRF.OAuthState).ToNot(Equal(privateCSRF.OIDCNonce)) + Expect(privateCSRF.CodeVerifier).To(Equal("verifier")) + }) + + It("makes unique nonces between multiple CSRFs", func() { + other, err := NewCSRF(cookieOpts, "verifier") + Expect(err).ToNot(HaveOccurred()) + + Expect(privateCSRF.OAuthState).ToNot(Equal(other.(*csrf).OAuthState)) + Expect(privateCSRF.OIDCNonce).ToNot(Equal(other.(*csrf).OIDCNonce)) + Expect(privateCSRF.CodeVerifier).To(Equal("verifier")) + }) + }) + + Context("CheckOAuthState and CheckOIDCNonce", func() { + It("checks that hashed versions match", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + + stateHashed := encryption.HashNonce([]byte(csrfState)) + nonceHashed := encryption.HashNonce([]byte(csrfNonce)) + + Expect(publicCSRF.CheckOAuthState(stateHashed)).To(BeTrue()) + Expect(publicCSRF.CheckOIDCNonce(nonceHashed)).To(BeTrue()) + + Expect(publicCSRF.CheckOAuthState(csrfNonce)).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce(csrfState)).To(BeFalse()) + Expect(publicCSRF.CheckOAuthState(csrfState + csrfNonce)).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce(csrfNonce + csrfState)).To(BeFalse()) + Expect(publicCSRF.CheckOAuthState("")).To(BeFalse()) + Expect(publicCSRF.CheckOIDCNonce("")).To(BeFalse()) + Expect(publicCSRF.GetCodeVerifier()).To(Equal("verifier")) + }) + }) + + Context("SetSessionNonce", func() { + It("sets the session.Nonce", func() { + session := &sessions.SessionState{} + publicCSRF.SetSessionNonce(session) + Expect(session.Nonce).To(Equal(privateCSRF.OIDCNonce)) + }) + }) + + Context("encodeCookie and decodeCSRFCookie", func() { + It("encodes and decodes to the same nonces", func() { + privateCSRF.OAuthState = []byte(csrfState) + privateCSRF.OIDCNonce = []byte(csrfNonce) + + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + cookie := &http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + } + decoded, err := decodeCSRFCookie(cookie, cookieOpts) + Expect(err).ToNot(HaveOccurred()) + + Expect(decoded).ToNot(BeNil()) + Expect(decoded.OAuthState).To(Equal([]byte(csrfState))) + Expect(decoded.OIDCNonce).To(Equal([]byte(csrfNonce))) + }) + + It("signs the encoded cookie value", func() { + encoded, err := privateCSRF.encodeCookie() + Expect(err).ToNot(HaveOccurred()) + + cookie := &http.Cookie{ + Name: privateCSRF.cookieName(), + Value: encoded, + } + + _, _, valid := encryption.Validate(cookie, cookieOpts.Secret, cookieOpts.Expire) + Expect(valid).To(BeTrue()) + }) + }) + + Context("Cookie Management", func() { + var req *http.Request + + testNow := time.Unix(nowEpoch, 0) + + BeforeEach(func() { + privateCSRF.time.Set(testNow) + + req = &http.Request{ + Method: http.MethodGet, + Proto: "HTTP/1.1", + Host: cookieDomain, + + URL: &url.URL{ + Scheme: "https", + Host: cookieDomain, + Path: cookiePath, + }, + } + }) + + AfterEach(func() { + privateCSRF.time.Reset() + }) + + Context("SetCookie", func() { + It("adds the encoded CSRF cookie to a ResponseWriter", func() { + rw := httptest.NewRecorder() + + _, err := publicCSRF.SetCookie(rw, req) + Expect(err).ToNot(HaveOccurred()) + + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf("%s=", privateCSRF.cookieName()), + )) + Expect(rw.Header().Get("Set-Cookie")).To(ContainSubstring( + fmt.Sprintf( + "; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", + cookiePath, + cookieDomain, + testCookieExpires(testNow.Add(cookieOpts.CSRFExpire)), + ), + )) + }) + }) + + Context("ClearCookie", func() { + It("sets a cookie with an empty value in the past", func() { + rw := httptest.NewRecorder() + + publicCSRF.ClearCookie(rw, req) + + Expect(rw.Header().Get("Set-Cookie")).To(Equal( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", + privateCSRF.cookieName(), + cookiePath, + cookieDomain, + testCookieExpires(testNow.Add(time.Hour*-1)), + ), + )) + }) + }) + + Context("cookieName", func() { + It("has the cookie options name as a base", func() { + Expect(privateCSRF.cookieName()).To(ContainSubstring(cookieName)) + }) + }) + }) +}) diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index 69fbfbb3..c4a461f0 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -23,13 +23,14 @@ var _ = Describe("CSRF Cookie Tests", func() { BeforeEach(func() { cookieOpts = &options.Cookie{ - Name: cookieName, - Secret: cookieSecret, - Domains: []string{cookieDomain}, - Path: cookiePath, - Expire: time.Hour, - Secure: true, - HTTPOnly: true, + Name: cookieName, + Secret: cookieSecret, + Domains: []string{cookieDomain}, + Path: cookiePath, + Expire: time.Hour, + Secure: true, + HTTPOnly: true, + CSRFPerRequest: false, } var err error @@ -160,7 +161,7 @@ var _ = Describe("CSRF Cookie Tests", func() { "; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", cookiePath, cookieDomain, - testCookieExpires(testNow.Add(cookieOpts.Expire)), + testCookieExpires(testNow.Add(cookieOpts.CSRFExpire)), ), )) })