Add flags to define CSRF cookie expiration time and to allow CSRF cookies per request (#1708)

* Add start of state to CSRF cookie name * Update CHANGELOG.md * Update CHANGELOG.md * Support optional flags * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * Update overview.md Add new CSRF flags * Update overview.md Describe new CSRF flags Co-authored-by: Nuno Borges <Nuno.Borges@ctw.bmwgroup.com>
2026-10-21 10:17:22 +02:00 · 2022-08-31 23:27:56 +01:00
parent f8bd853702
commit a1ff878fdc
6 changed files with 287 additions and 35 deletions
@@ -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,
 	}
 }
@@ -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) {
@@ -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))
+			})
+		})
+	})
+})
@@ -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)),
 					),
 				))
 			})