git.iko.soy/typeable/oauth2-proxy
on master help sign in
Split cookies more precisely at 4096 bytes
commitc6f1daba2f789d36a5a700c9e0cbe37b4337cca4 authorNick Meves <nick.meves@greenhouse.io> dateFri 3 Jul 2020, 20:31 (6 years ago) parentc4cf15f
3 files changed+138−12
CHANGELOG.md+1 pkg/sessions/cookie/session_store.go+22−12 pkg/sessions/cookie/session_store_test.go+115
CHANGELOG.md+1
@@ -8,6 +8,7 @@
88
99## Changes since v6.0.0
1010
11+- [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
1112- [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
1213- [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
1314- [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)
pkg/sessions/cookie/session_store.go+22−12
@@ -10,17 +10,11 @@ import (
1010
1111 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
1212 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
13− "github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
13+ pkgcookies "github.com/oauth2-proxy/oauth2-proxy/pkg/cookies"
1414 "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
1515 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
1616)
1717
18−const (
19− // Cookies are limited to 4kb including the length of the cookie name,
20− // the cookie name can be up to 256 bytes
21− maxCookieLength = 3840
22−)
23−
2418// Ensure CookieSessionStore implements the interface
2519var _ sessions.SessionStore = &SessionStore{}
2620
@@ -107,6 +101,9 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
107101 value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
108102 }
109103 c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
104+
105+ // Cookies are limited to 4kb including the length of the cookie name,
106+ // the cookie name can be up to 256 bytes
110107 if len(c.Value) > 4096-len(s.CookieOptions.Name) {
111108 return splitCookie(c)
112109 }
@@ -114,7 +111,7 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
114111}
115112
116113func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
117− return cookies.MakeCookieFromOptions(
114+ return pkgcookies.MakeCookieFromOptions(
118115 req,
119116 name,
120117 value,
@@ -142,17 +139,21 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo
142139// it into a slice of cookies which fit within the 4kb cookie limit indexing
143140// the cookies from 0
144141func splitCookie(c *http.Cookie) []*http.Cookie {
145− 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.")
146− if len(c.Value) < maxCookieLength {
142+ if len(c.Value) < 4096-len(c.Name) {
147143 return []*http.Cookie{c}
148144 }
145+
146+ 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.")
147+
149148 cookies := []*http.Cookie{}
150149 valueBytes := []byte(c.Value)
151150 count := 0
152151 for len(valueBytes) > 0 {
153152 newCookie := copyCookie(c)
154− newCookie.Name = fmt.Sprintf("%s_%d", c.Name, count)
153+ newCookie.Name = splitCookieName(c.Name, count)
155154 count++
155+
156+ maxCookieLength := 4096 - len(newCookie.Name)
156157 if len(valueBytes) < maxCookieLength {
157158 newCookie.Value = string(valueBytes)
158159 valueBytes = []byte{}
@@ -166,6 +167,15 @@ func splitCookie(c *http.Cookie) []*http.Cookie {
166167 return cookies
167168}
168169
170+func splitCookieName(name string, count int) string {
171+ splitName := fmt.Sprintf("%s_%d", name, count)
172+ overflow := len(splitName) - 256
173+ if overflow > 0 {
174+ splitName = fmt.Sprintf("%s_%d", name[:len(name)-overflow], count)
175+ }
176+ return splitName
177+}
178+
169179// loadCookie retreieves the sessions state cookie from the http request.
170180// If a single cookie is present this will be returned, otherwise it attempts
171181// to reconstruct a cookie split up by splitCookie
@@ -179,7 +189,7 @@ func loadCookie(req *http.Request, cookieName string) (*http.Cookie, error) {
179189 count := 0
180190 for err == nil {
181191 var c *http.Cookie
182− c, err = req.Cookie(fmt.Sprintf("%s_%d", cookieName, count))
192+ c, err = req.Cookie(splitCookieName(cookieName, count))
183193 if err == nil {
184194 cookies = append(cookies, c)
185195 count++
pkg/sessions/cookie/session_store_test.go+115
@@ -1,7 +1,10 @@
11package cookie
22
33import (
4+ "fmt"
5+ mathrand "math/rand"
46 "net/http"
7+ "strings"
58 "testing"
69 "time"
710
@@ -49,3 +52,115 @@ func Test_copyCookie(t *testing.T) {
4952 got := copyCookie(c)
5053 assert.Equal(t, c, got)
5154}
55+
56+func Test_splitCookie(t *testing.T) {
57+ testCases := map[string]struct {
58+ Cookie *http.Cookie
59+ ExpectedSizes []int
60+ }{
61+ "Short cookie name": {
62+ Cookie: &http.Cookie{
63+ Name: "short",
64+ Value: strings.Repeat("v", 10000),
65+ },
66+ ExpectedSizes: []int{4089, 4089, 1822},
67+ },
68+ "Long cookie name": {
69+ Cookie: &http.Cookie{
70+ Name: strings.Repeat("n", 251),
71+ Value: strings.Repeat("a", 10000),
72+ },
73+ ExpectedSizes: []int{3843, 3843, 2314},
74+ },
75+ "Max cookie name": {
76+ Cookie: &http.Cookie{
77+ Name: strings.Repeat("n", 256),
78+ Value: strings.Repeat("a", 10000),
79+ },
80+ ExpectedSizes: []int{3840, 3840, 2320},
81+ },
82+ "Suffix overflow cookie name": {
83+ Cookie: &http.Cookie{
84+ Name: strings.Repeat("n", 255),
85+ Value: strings.Repeat("a", 10000),
86+ },
87+ ExpectedSizes: []int{3840, 3840, 2320},
88+ },
89+ "Double digit suffix cookie name results in different sizes": {
90+ Cookie: &http.Cookie{
91+ Name: strings.Repeat("n", 253),
92+ Value: strings.Repeat("a", 50000),
93+ },
94+ ExpectedSizes: []int{3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841, 3841,
95+ 3840, 3840, 3840, 70},
96+ },
97+ "Double digit suffix overflow cookie name results in same sizes": {
98+ Cookie: &http.Cookie{
99+ Name: strings.Repeat("n", 254),
100+ Value: strings.Repeat("a", 50000),
101+ },
102+ ExpectedSizes: []int{3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840, 3840,
103+ 3840, 3840, 3840, 80},
104+ },
105+ }
106+ for testName, tc := range testCases {
107+ t.Run(testName, func(t *testing.T) {
108+ for i, cookie := range splitCookie(tc.Cookie) {
109+ assert.Equal(t, tc.ExpectedSizes[i], len(cookie.Value))
110+ }
111+ })
112+ }
113+}
114+
115+func Test_splitCookieName(t *testing.T) {
116+ testCases := map[string]struct {
117+ Name string
118+ Count int
119+ Output string
120+ }{
121+ "Standard length": {
122+ Name: "IAmSoNormal",
123+ Count: 2,
124+ Output: "IAmSoNormal_2",
125+ },
126+ "Max length": {
127+ Name: strings.Repeat("n", 256),
128+ Count: 1,
129+ Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 254), 1),
130+ },
131+ "Large count overflow": {
132+ Name: strings.Repeat("n", 253),
133+ Count: 1000,
134+ Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 251), 1000),
135+ },
136+ }
137+ for testName, tc := range testCases {
138+ t.Run(testName, func(t *testing.T) {
139+ splitName := splitCookieName(tc.Name, tc.Count)
140+ assert.Equal(t, tc.Output, splitName)
141+ })
142+ }
143+}
144+
145+func Test_splitCookie_joinCookies(t *testing.T) {
146+ const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
147+
148+ v := make([]byte, 251)
149+ for i := range v {
150+ v[i] = charset[mathrand.Intn(len(charset))]
151+ }
152+ value := strings.Repeat(string(v), 1000)
153+
154+ for _, nameSize := range []int{1, 10, 50, 100, 200, 254} {
155+ t.Run(fmt.Sprintf("%d length cookie name", nameSize), func(t *testing.T) {
156+ cookie := &http.Cookie{
157+ Name: strings.Repeat("n", nameSize),
158+ Value: value,
159+ }
160+ splitCookies := splitCookie(cookie)
161+ joinedCookie, err := joinCookies(splitCookies)
162+ assert.NoError(t, err)
163+ assert.Equal(t, *cookie, *joinedCookie)
164+ })
165+ }
166+}