[PLT-1263] Add a cookie-csrf-per-request-limit attribute

Author: unai-ttxu

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 
+* Adapt repo to Stratio CICD flow

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 ->

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 |

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())

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,
 	}
-}
+}

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.Contains(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)

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)),
+					),
+				))
+			})
+		})
 	})
 })