feat(cookie) csrf per request limit (#3134)

* Allow setting maximum number of csrf cookies, deleting the oldest if necessary * Add a test for multiple CSRF cookies to remove the old cookie * Add docs/changelog * If limit is <=0 do not clear Signed-off-by: test <bert@transtrend.com> * Better docs Co-authored-by: Jan Larwig <jan@larwig.com> * direct check of option value Co-authored-by: Jan Larwig <jan@larwig.com> * direct use of option value Co-authored-by: Jan Larwig <jan@larwig.com> * sort based on clock compare vs time compare Co-authored-by: Jan Larwig <jan@larwig.com> * clock.Clock does not implement Compare, fix csrf cookie extraction after rename Signed-off-by: Bert Helderman <bert@transtrend.com> * Linter fix * add method signature documentation and slight formatting Signed-off-by: Jan Larwig <jan@larwig.com> * fix: test case for csrf cookie limit and flag Signed-off-by: Jan Larwig <jan@larwig.com> --------- Signed-off-by: Bert Helderman <bert@transtrend.com> Signed-off-by: Jan Larwig <jan@larwig.com> Co-authored-by: test <bert@transtrend.com> Co-authored-by: bh-tt <71650427+bh-tt@users.noreply.github.com>
2025-08-08 22:46:33 +02:00 · 2025-07-20 16:44:42 +02:00
parent d5f8507cc8
commit b57c82181d
6 changed files with 186 additions and 56 deletions
--- 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 {
@ -34,6 +35,7 @@ func cookieFlagSet() *pflag.FlagSet {
 	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.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")
 	return flagSet
 }
--- a/pkg/cookies/csrf.go
+++ b/pkg/cookies/csrf.go
@ -4,6 +4,8 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"slices"
+	"strings"
 	"time"

 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
@ -151,6 +153,48 @@ func (c *csrf) SetCookie(rw http.ResponseWriter, req *http.Request) (*http.Cooki
 	return cookie, nil
 }

+// ClearExtraCsrfCookies limits the amount of existing CSRF cookies by deleting
+// an excess of cookies controlled through the option CSRFPerRequestLimit
+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)
+
+	// determine how many csrf cookies we have
+	for _, cookie := range cookies {
+		if strings.HasPrefix(cookie.Name, startsWith) && strings.HasSuffix(cookie.Name, "_csrf") {
+			existingCsrfCookies = append(existingCsrfCookies, cookie)
+		}
+	}
+
+	// short circuit return
+	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)
+	}
+
+	// delete the X oldest cookies
+	slices.SortStableFunc(decodedCookies, func(a, b *csrf) int {
+		return a.time.Now().Compare(b.time.Now())
+	})
+
+	for i := 0; i < len(decodedCookies)-opts.CSRFPerRequestLimit; 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(
@ -181,7 +225,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")
 	}
@ -192,7 +236,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)
--- a/pkg/cookies/csrf_per_request_test.go
+++ b/pkg/cookies/csrf_per_request_test.go
@ -190,5 +190,84 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() {
 				Expect(privateCSRF.cookieName()).To(ContainSubstring(cookieName))
 			})
 		})
+
+		Context("CSRF per request limit", func() {
+			It("clears cookies based on the limit", func() {
+				//needs to be now as pkg/encryption/utils.go uses time.Now()
+				testNow := time.Now()
+				cookieOpts.CSRFPerRequestLimit = 1
+
+				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))
+
+				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,
+					)
+					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,
+				}
+
+				// when setting the limit to one csrf cookie but configuring three csrf cookies
+				// then two cookies should be removed / set to expired on the response
+
+				// for this test case we have set all the cookies on a single request,
+				// but in reality this will be multiple requests after another
+				rw := httptest.NewRecorder()
+				ClearExtraCsrfCookies(cookieOpts, rw, req)
+
+				clearedCookies := rw.Header()["Set-Cookie"]
+				Expect(clearedCookies).To(HaveLen(2))
+				Expect(clearedCookies[0]).To(Equal(
+					fmt.Sprintf(
+						"%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure",
+						privateCSRF1.cookieName(),
+						cookiePath,
+						cookieDomain,
+					),
+				))
+				Expect(clearedCookies[1]).To(Equal(
+					fmt.Sprintf(
+						"%s=; Path=%s; Domain=%s; Max-Age=0; HttpOnly; Secure",
+						privateCSRF2.cookieName(),
+						cookiePath,
+						cookieDomain,
+					),
+				))
+			})
+		})
 	})
 })