diff --git a/CHANGELOG.md b/CHANGELOG.md index 33db5210..d9157bde 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 00fb02b4..32a71e56 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -10,17 +10,11 @@ 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 -) - // Ensure CookieSessionStore implements the interface var _ sessions.SessionStore = &SessionStore{} @@ -107,6 +101,9 @@ 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) + + // Cookies are limited to 4kb including the length of the cookie name, + // the cookie name can be up to 256 bytes if len(c.Value) > 4096-len(s.CookieOptions.Name) { return splitCookie(c) } @@ -114,7 +111,7 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti } 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,17 +139,21 @@ 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.Value) < 4096-len(c.Name) { 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++ + + maxCookieLength := 4096 - len(newCookie.Name) if len(valueBytes) < maxCookieLength { newCookie.Value = string(valueBytes) valueBytes = []byte{} @@ -166,6 +167,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 +189,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++ diff --git a/pkg/sessions/cookie/session_store_test.go b/pkg/sessions/cookie/session_store_test.go index 1465d6ec..f29edd16 100644 --- a/pkg/sessions/cookie/session_store_test.go +++ b/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,115 @@ func Test_copyCookie(t *testing.T) { got := copyCookie(c) assert.Equal(t, c, got) } + +func Test_splitCookie(t *testing.T) { + testCases := map[string]struct { + Cookie *http.Cookie + ExpectedSizes []int + }{ + "Short cookie name": { + Cookie: &http.Cookie{ + Name: "short", + Value: strings.Repeat("v", 10000), + }, + ExpectedSizes: []int{4089, 4089, 1822}, + }, + "Long cookie name": { + Cookie: &http.Cookie{ + Name: strings.Repeat("n", 251), + Value: strings.Repeat("a", 10000), + }, + ExpectedSizes: []int{3843, 3843, 2314}, + }, + "Max cookie name": { + Cookie: &http.Cookie{ + Name: strings.Repeat("n", 256), + Value: strings.Repeat("a", 10000), + }, + ExpectedSizes: []int{3840, 3840, 2320}, + }, + "Suffix overflow cookie name": { + Cookie: &http.Cookie{ + Name: strings.Repeat("n", 255), + Value: strings.Repeat("a", 10000), + }, + ExpectedSizes: []int{3840, 3840, 2320}, + }, + "Double digit suffix cookie name results in different sizes": { + Cookie: &http.Cookie{ + Name: strings.Repeat("n", 253), + Value: strings.Repeat("a", 50000), + }, + ExpectedSizes: []int{3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, + 3840, 3840, 3840, 70}, + }, + "Double digit suffix overflow cookie name results in same sizes": { + Cookie: &http.Cookie{ + Name: strings.Repeat("n", 254), + Value: strings.Repeat("a", 50000), + }, + ExpectedSizes: []int{3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, + 3840, 3840, 3840, 80}, + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + for i, cookie := range splitCookie(tc.Cookie) { + assert.Equal(t, tc.ExpectedSizes[i], len(cookie.Value)) + } + }) + } +} + +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) + }) + } +}