Merge pull request #656 from grnhse/cookie-splitting-precision

Split cookies more precisely at 4096 bytes

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Changes since v6.0.0
 
+- [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
 - [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
 - [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
 - [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)

pkg/sessions/cookie/session_store.go

@@ -10,15 +10,16 @@ import (
 
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
+	pkgcookies "github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
 )
 
 const (
-	// Cookies are limited to 4kb including the length of the cookie name,
-	// the cookie name can be up to 256 bytes
-	maxCookieLength = 3840
+	// Cookies are limited to 4kb for all parts
+	// including the cookie name, value, attributes; IE (http.cookie).String()
+	// Most browsers' max is 4096 -- but we give ourselves some leeway
+	maxCookieLength = 4000
 )
 
 // Ensure CookieSessionStore implements the interface
@@ -107,14 +108,15 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
 		value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
 	}
 	c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
-	if len(c.Value) > 4096-len(s.CookieOptions.Name) {
+
+	if len(c.String()) > maxCookieLength {
 		return splitCookie(c)
 	}
 	return []*http.Cookie{c}
 }
 
 func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
-	return cookies.MakeCookieFromOptions(
+	return pkgcookies.MakeCookieFromOptions(
 		req,
 		name,
 		value,
@@ -142,23 +144,30 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo
 // it into a slice of cookies which fit within the 4kb cookie limit indexing
 // the cookies from 0
 func splitCookie(c *http.Cookie) []*http.Cookie {
-	logger.Printf("WARNING: Multiple cookies are required for this session as it exceeds the 4kb cookie limit. Please use server side session storage (eg. Redis) instead.")
-	if len(c.Value) < maxCookieLength {
+	if len(c.String()) < maxCookieLength {
 		return []*http.Cookie{c}
 	}
+
+	logger.Printf("WARNING: Multiple cookies are required for this session as it exceeds the 4kb cookie limit. Please use server side session storage (eg. Redis) instead.")
+
 	cookies := []*http.Cookie{}
 	valueBytes := []byte(c.Value)
 	count := 0
 	for len(valueBytes) > 0 {
 		newCookie := copyCookie(c)
-		newCookie.Name = fmt.Sprintf("%s_%d", c.Name, count)
+		newCookie.Name = splitCookieName(c.Name, count)
 		count++
-		if len(valueBytes) < maxCookieLength {
+
 		newCookie.Value = string(valueBytes)
+		cookieLength := len(newCookie.String())
+		if cookieLength <= maxCookieLength {
 			valueBytes = []byte{}
 		} else {
-			newValue := valueBytes[:maxCookieLength]
-			valueBytes = valueBytes[maxCookieLength:]
+			overflow := cookieLength - maxCookieLength
+			valueSize := len(valueBytes) - overflow
+
+			newValue := valueBytes[:valueSize]
+			valueBytes = valueBytes[valueSize:]
 			newCookie.Value = string(newValue)
 		}
 		cookies = append(cookies, newCookie)
@@ -166,6 +175,15 @@ func splitCookie(c *http.Cookie) []*http.Cookie {
 	return cookies
 }
 
+func splitCookieName(name string, count int) string {
+	splitName := fmt.Sprintf("%s_%d", name, count)
+	overflow := len(splitName) - 256
+	if overflow > 0 {
+		splitName = fmt.Sprintf("%s_%d", name[:len(name)-overflow], count)
+	}
+	return splitName
+}
+
 // loadCookie retreieves the sessions state cookie from the http request.
 // If a single cookie is present this will be returned, otherwise it attempts
 // to reconstruct a cookie split up by splitCookie
@@ -179,7 +197,7 @@ func loadCookie(req *http.Request, cookieName string) (*http.Cookie, error) {
 	count := 0
 	for err == nil {
 		var c *http.Cookie
-		c, err = req.Cookie(fmt.Sprintf("%s_%d", cookieName, count))
+		c, err = req.Cookie(splitCookieName(cookieName, count))
 		if err == nil {
 			cookies = append(cookies, c)
 			count++

pkg/sessions/cookie/session_store_test.go

@@ -1,7 +1,10 @@
 package cookie
 
 import (
+	"fmt"
+	mathrand "math/rand"
 	"net/http"
+	"strings"
 	"testing"
 	"time"
 
@@ -49,3 +52,111 @@ func Test_copyCookie(t *testing.T) {
 	got := copyCookie(c)
 	assert.Equal(t, c, got)
 }
+
+func Test_splitCookie(t *testing.T) {
+	testCases := map[string]*http.Cookie{
+		"Short cookie name": {
+			Name:  "short",
+			Value: strings.Repeat("v", 10000),
+		},
+		"Long cookie name": {
+			Name:  strings.Repeat("n", 251),
+			Value: strings.Repeat("a", 10000),
+		},
+		"Max cookie name": {
+			Name:  strings.Repeat("n", 256),
+			Value: strings.Repeat("a", 10000),
+		},
+		"Suffix overflow cookie name": {
+			Name:  strings.Repeat("n", 255),
+			Value: strings.Repeat("a", 10000),
+		},
+		"Double digit suffix cookie name overflow": {
+			Name:  strings.Repeat("n", 253),
+			Value: strings.Repeat("a", 50000),
+		},
+		"With short name and attributes": {
+			Name:     "short",
+			Value:    strings.Repeat("v", 10000),
+			Path:     "/path",
+			Domain:   "x.y.z",
+			Secure:   true,
+			HttpOnly: true,
+			SameSite: http.SameSiteLaxMode,
+		},
+		"With max length name and attributes": {
+			Name:     strings.Repeat("n", 256),
+			Value:    strings.Repeat("v", 10000),
+			Path:     "/path",
+			Domain:   "x.y.z",
+			Secure:   true,
+			HttpOnly: true,
+			SameSite: http.SameSiteLaxMode,
+		},
+	}
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			splitCookies := splitCookie(tc)
+			for i, cookie := range splitCookies {
+				if i < len(splitCookies)-1 {
+					assert.Equal(t, 4000, len(cookie.String()))
+				} else {
+					assert.GreaterOrEqual(t, 4000, len(cookie.String()))
+				}
+			}
+		})
+	}
+}
+
+func Test_splitCookieName(t *testing.T) {
+	testCases := map[string]struct {
+		Name   string
+		Count  int
+		Output string
+	}{
+		"Standard length": {
+			Name:   "IAmSoNormal",
+			Count:  2,
+			Output: "IAmSoNormal_2",
+		},
+		"Max length": {
+			Name:   strings.Repeat("n", 256),
+			Count:  1,
+			Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 254), 1),
+		},
+		"Large count overflow": {
+			Name:   strings.Repeat("n", 253),
+			Count:  1000,
+			Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 251), 1000),
+		},
+	}
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			splitName := splitCookieName(tc.Name, tc.Count)
+			assert.Equal(t, tc.Output, splitName)
+		})
+	}
+}
+
+func Test_splitCookie_joinCookies(t *testing.T) {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+
+	v := make([]byte, 251)
+	for i := range v {
+		v[i] = charset[mathrand.Intn(len(charset))]
+	}
+	value := strings.Repeat(string(v), 1000)
+
+	for _, nameSize := range []int{1, 10, 50, 100, 200, 254} {
+		t.Run(fmt.Sprintf("%d length cookie name", nameSize), func(t *testing.T) {
+			cookie := &http.Cookie{
+				Name:  strings.Repeat("n", nameSize),
+				Value: value,
+			}
+			splitCookies := splitCookie(cookie)
+			joinedCookie, err := joinCookies(splitCookies)
+			assert.NoError(t, err)
+			assert.Equal(t, *cookie, *joinedCookie)
+		})
+	}
+}