From d838bd1407826556311ca515129dde9d1858a558 Mon Sep 17 00:00:00 2001 From: Unai Arrien Date: Fri, 29 Nov 2024 11:47:12 +0100 Subject: [PATCH] [PLT-1263] Add a cookie-csrf-per-request-limit attribute --- CHANGELOG.md | 6 ++- Jenkinsfile | 1 + docs/docs/configuration/overview.md | 1 + oauthproxy.go | 1 + pkg/apis/options/cookie.go | 49 +++++++++++---------- pkg/cookies/csrf.go | 45 ++++++++++++++++++- pkg/cookies/csrf_per_request_test.go | 65 ++++++++++++++++++++++++++++ 7 files changed, 142 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d29bd0126..b166afef3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ### 7.5.1-0.3.0 (2023-11-24) +* [PLT-1263] Add a cookie-csrf-per-request-limit attribute + +### 7.5.1-0.3.0 (2023-11-24) + * [EOS-12032] Use jwt session store ## Previous development @@ -31,4 +35,4 @@ * Add tenant and groups to userinfo * Add SIS provider and JWT session support -* Adapt repo to Stratio CICD flow \ No newline at end of file +* Adapt repo to Stratio CICD flow diff --git a/Jenkinsfile b/Jenkinsfile index d6aaa538c8..00b0a1f14e 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -8,6 +8,7 @@ hose { ANCHORE_POLICY = "production" VERSIONING_TYPE = 'stratioVersion-3-3' UPSTREAM_VERSION = '7.5.1' + DEPLOYONPRS = true GRYPE_TEST = false DEV = { config -> diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index c555bf1b4c..5e7d8d11c3 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -99,6 +99,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--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 | +| `--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" | | `--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 | diff --git a/oauthproxy.go b/oauthproxy.go index 93fc0d59fb..59c7b6c1f3 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -817,6 +817,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove extraParams, ) + cookies.ClearExtraCsrfCookies(p.CookieOptions, rw, req) if _, err := csrf.SetCookie(rw, req); err != nil { logger.Errorf("Error setting CSRF cookie: %v", err) p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 6917bdc570..6c822f32cd 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -8,17 +8,18 @@ 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"` - CSRFPerRequest bool `flag:"cookie-csrf-per-request" cfg:"cookie_csrf_per_request"` - CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` + 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"` + CSRFPerRequestLimit int `flag:"cookie-csrf-per-request-limit" cfg:"cookie_csrf_per_request_limit"` } func cookieFlagSet() *pflag.FlagSet { @@ -35,22 +36,24 @@ func cookieFlagSet() *pflag.FlagSet { 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") + flagSet.Int("cookie-csrf-per-request-limit", 0, "default value for CSRFPerRequestLimit key") 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: "", - CSRFPerRequest: false, - CSRFExpire: time.Duration(15) * time.Minute, + 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, + CSRFPerRequestLimit: 0, } -} +} \ No newline at end of file diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index 9840b544f0..0c8e1cc780 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" "net/http" + "sort" + "strings" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" @@ -146,6 +148,43 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki return cookie, nil } +func ClearExtraCsrfCookies(opts *options.Cookie, rw http.ResponseWriter, req *http.Request) { + if !opts.CSRFPerRequest || opts.CSRFPerRequestLimit <= 0 { + return + } + + cookies := req.Cookies() + existingCsrfCookies := []*http.Cookie{} + startsWith := fmt.Sprintf("%v_", opts.Name) + for _, cookie := range cookies { + if strings.HasPrefix(cookie.Name, startsWith) && strings.HasSuffix(cookie.Name, "_csrf") { + existingCsrfCookies = append(existingCsrfCookies, cookie) + } + } + + if len(existingCsrfCookies) <= opts.CSRFPerRequestLimit { + return + } + + decodedCookies := []*csrf{} + for _, cookie := range existingCsrfCookies { + decodedCookie, err := decodeCSRFCookie(cookie, opts) + if err != nil { + continue + } + decodedCookies = append(decodedCookies, decodedCookie) + } + + sort.Slice(decodedCookies, func(i, j int) bool { + return decodedCookies[i].time.Now().Before(decodedCookies[j].time.Now()) + }) + + numberToDelete := len(decodedCookies) - opts.CSRFPerRequestLimit + for i := 0; i < numberToDelete; i++ { + decodedCookies[i].ClearCookie(rw, req) + } +} + // ClearCookie removes the CSRF cookie func (c *csrf) ClearCookie(rw http.ResponseWriter, req *http.Request) { http.SetCookie(rw, MakeCookieFromOptions( @@ -177,7 +216,7 @@ func (c *csrf) encodeCookie() (string, error) { // decodeCSRFCookie validates the signature then decrypts and decodes a CSRF // cookie into a CSRF struct func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) { - val, _, ok := encryption.Validate(cookie, opts.Secret, opts.Expire) + val, t, ok := encryption.Validate(cookie, opts.Secret, opts.Expire) if !ok { return nil, errors.New("CSRF cookie failed validation") } @@ -188,7 +227,9 @@ func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) } // Valid cookie, Unmarshal the CSRF - csrf := &csrf{cookieOpts: opts} + clock := clock.Clock{} + clock.Set(t) + csrf := &csrf{cookieOpts: opts, time: clock} err = msgpack.Unmarshal(decrypted, csrf) if err != nil { return nil, fmt.Errorf("error unmarshalling data to CSRF: %v", err) diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index 915dc34687..d32fdb37c5 100644 --- a/pkg/cookies/csrf_per_request_test.go +++ b/pkg/cookies/csrf_per_request_test.go @@ -191,5 +191,70 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { Expect(privateCSRF.cookieName()).To(ContainSubstring(cookieName)) }) }) + Context("CSRF max cookies", func() { + It("disables the 3rd cookie if a limit of 2 is set", func() { + //needs to be now as pkg/encryption/utils.go uses time.Now() + testNow := time.Now() + cookieOpts.CSRFPerRequestLimit = 2 + + publicCSRF1, err := NewCSRF(cookieOpts, "verifier") + Expect(err).ToNot(HaveOccurred()) + privateCSRF1 := publicCSRF1.(*csrf) + privateCSRF1.time.Set(testNow) + + publicCSRF2, err := NewCSRF(cookieOpts, "verifier") + Expect(err).ToNot(HaveOccurred()) + privateCSRF2 := publicCSRF2.(*csrf) + privateCSRF2.time.Set(testNow.Add(time.Minute)) + + publicCSRF3, err := NewCSRF(cookieOpts, "verifier") + Expect(err).ToNot(HaveOccurred()) + privateCSRF3 := publicCSRF3.(*csrf) + privateCSRF3.time.Set(testNow.Add(time.Minute * 2)) + + //for the test we set all the cookies on a single request, but in reality this will be multiple requests after another + cookies := []string{} + 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, + csrf.time.Now(), + ) + cookies = append(cookies, fmt.Sprintf("%v=%v", cookie.Name, cookie.Value)) + } + header := make(map[string][]string, 1) + header["Cookie"] = cookies + req = &http.Request{ + Method: http.MethodGet, + Proto: "HTTP/1.1", + Host: cookieDomain, + + URL: &url.URL{ + Scheme: "https", + Host: cookieDomain, + Path: cookiePath, + }, + Header: header, + } + fmt.Println(req.Cookies()) + rw := httptest.NewRecorder() + ClearExtraCsrfCookies(cookieOpts, rw, req) + + Expect(rw.Header().Values("Set-Cookie")).To(ContainElement( + fmt.Sprintf( + "%s=; Path=%s; Domain=%s; Expires=%s; HttpOnly; Secure", + privateCSRF1.cookieName(), + cookiePath, + cookieDomain, + testCookieExpires(testNow.Add(time.Hour*-1)), + ), + )) + }) + }) }) })