git.iko.soy/typeable/oauth2-proxy
on master help sign in
Merge pull request #656 from grnhse/cookie-splitting-precision
016f4aaNick Meves
CHANGELOG.md+1
compare with master
unifiedsplit
@@ -8,6 +8,7 @@
8
8
9## Changes since v6.0.0
9## Changes since v6.0.0
10
10
11+- [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
11- [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
12- [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
12- [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
13- [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
13- [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)
14- [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)
pkg/sessions/cookie/session_store.go+32−14
@@ -10,15 +10,16 @@ import (
10
10
11 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
11 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
12 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
12 "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"
14 "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
14 "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
15 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
15 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
16)
16)
17
17
18const (
18const (
19− // Cookies are limited to 4kb including the length of the cookie name,
19+ // Cookies are limited to 4kb for all parts
20− // the cookie name can be up to 256 bytes
20+ // including the cookie name, value, attributes; IE (http.cookie).String()
21+ // Most browsers' max is 4096 -- but we give ourselves some leeway
21− maxCookieLength = 3840
22+ maxCookieLength = 4000
22)
23)
23
24
24// Ensure CookieSessionStore implements the interface
25// Ensure CookieSessionStore implements the interface
@@ -107,14 +108,15 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now ti
107 value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
108 value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
108 }
109 }
109 c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
110 c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
110− if len(c.Value) > 4096-len(s.CookieOptions.Name) {
111+
112+ if len(c.String()) > maxCookieLength {
111 return splitCookie(c)
113 return splitCookie(c)
112 }
114 }
113 return []*http.Cookie{c}
115 return []*http.Cookie{c}
114}
116}
115
117
116func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
118func (s *SessionStore) makeCookie(req *http.Request, name string, value string, expiration time.Duration, now time.Time) *http.Cookie {
117− return cookies.MakeCookieFromOptions(
119+ return pkgcookies.MakeCookieFromOptions(
118 req,
120 req,
119 name,
121 name,
120 value,
122 value,
@@ -142,23 +144,30 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo
142// it into a slice of cookies which fit within the 4kb cookie limit indexing
144// it into a slice of cookies which fit within the 4kb cookie limit indexing
143// the cookies from 0
145// the cookies from 0
144func splitCookie(c *http.Cookie) []*http.Cookie {
146func 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 {
147+ if len(c.String()) < maxCookieLength {
147 return []*http.Cookie{c}
148 return []*http.Cookie{c}
148 }
149 }
150+
151+ 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.")
152+
149 cookies := []*http.Cookie{}
153 cookies := []*http.Cookie{}
150 valueBytes := []byte(c.Value)
154 valueBytes := []byte(c.Value)
151 count := 0
155 count := 0
152 for len(valueBytes) > 0 {
156 for len(valueBytes) > 0 {
153 newCookie := copyCookie(c)
157 newCookie := copyCookie(c)
154− newCookie.Name = fmt.Sprintf("%s_%d", c.Name, count)
158+ newCookie.Name = splitCookieName(c.Name, count)
155 count++
159 count++
160+
161+ newCookie.Value = string(valueBytes)
162+ cookieLength := len(newCookie.String())
156− if len(valueBytes) < maxCookieLength {
163+ if cookieLength <= maxCookieLength {
157− newCookie.Value = string(valueBytes)
158 valueBytes = []byte{}
164 valueBytes = []byte{}
159 } else {
165 } else {
166+ overflow := cookieLength - maxCookieLength
167+ valueSize := len(valueBytes) - overflow
168+
160− newValue := valueBytes[:maxCookieLength]
169+ newValue := valueBytes[:valueSize]
161− valueBytes = valueBytes[maxCookieLength:]
170+ valueBytes = valueBytes[valueSize:]
162 newCookie.Value = string(newValue)
171 newCookie.Value = string(newValue)
163 }
172 }
164 cookies = append(cookies, newCookie)
173 cookies = append(cookies, newCookie)
@@ -166,6 +175,15 @@ func splitCookie(c *http.Cookie) []*http.Cookie {
166 return cookies
175 return cookies
167}
176}
168
177
178+func splitCookieName(name string, count int) string {
179+ splitName := fmt.Sprintf("%s_%d", name, count)
180+ overflow := len(splitName) - 256
181+ if overflow > 0 {
182+ splitName = fmt.Sprintf("%s_%d", name[:len(name)-overflow], count)
183+ }
184+ return splitName
185+}
186+
169// loadCookie retreieves the sessions state cookie from the http request.
187// loadCookie retreieves the sessions state cookie from the http request.
170// If a single cookie is present this will be returned, otherwise it attempts
188// If a single cookie is present this will be returned, otherwise it attempts
171// to reconstruct a cookie split up by splitCookie
189// to reconstruct a cookie split up by splitCookie
@@ -179,7 +197,7 @@ func loadCookie(req *http.Request, cookieName string) (*http.Cookie, error) {
179 count := 0
197 count := 0
180 for err == nil {
198 for err == nil {
181 var c *http.Cookie
199 var c *http.Cookie
182− c, err = req.Cookie(fmt.Sprintf("%s_%d", cookieName, count))
200+ c, err = req.Cookie(splitCookieName(cookieName, count))
183 if err == nil {
201 if err == nil {
184 cookies = append(cookies, c)
202 cookies = append(cookies, c)
185 count++
203 count++
pkg/sessions/cookie/session_store_test.go+111
@@ -1,7 +1,10 @@
1package cookie
1package cookie
2
2
3import (
3import (
4+ "fmt"
5+ mathrand "math/rand"
4 "net/http"
6 "net/http"
7+ "strings"
5 "testing"
8 "testing"
6 "time"
9 "time"
7
10
@@ -49,3 +52,111 @@ func Test_copyCookie(t *testing.T) {
49 got := copyCookie(c)
52 got := copyCookie(c)
50 assert.Equal(t, c, got)
53 assert.Equal(t, c, got)
51}
54}
55+
56+func Test_splitCookie(t *testing.T) {
57+ testCases := map[string]*http.Cookie{
58+ "Short cookie name": {
59+ Name: "short",
60+ Value: strings.Repeat("v", 10000),
61+ },
62+ "Long cookie name": {
63+ Name: strings.Repeat("n", 251),
64+ Value: strings.Repeat("a", 10000),
65+ },
66+ "Max cookie name": {
67+ Name: strings.Repeat("n", 256),
68+ Value: strings.Repeat("a", 10000),
69+ },
70+ "Suffix overflow cookie name": {
71+ Name: strings.Repeat("n", 255),
72+ Value: strings.Repeat("a", 10000),
73+ },
74+ "Double digit suffix cookie name overflow": {
75+ Name: strings.Repeat("n", 253),
76+ Value: strings.Repeat("a", 50000),
77+ },
78+ "With short name and attributes": {
79+ Name: "short",
80+ Value: strings.Repeat("v", 10000),
81+ Path: "/path",
82+ Domain: "x.y.z",
83+ Secure: true,
84+ HttpOnly: true,
85+ SameSite: http.SameSiteLaxMode,
86+ },
87+ "With max length name and attributes": {
88+ Name: strings.Repeat("n", 256),
89+ Value: strings.Repeat("v", 10000),
90+ Path: "/path",
91+ Domain: "x.y.z",
92+ Secure: true,
93+ HttpOnly: true,
94+ SameSite: http.SameSiteLaxMode,
95+ },
96+ }
97+ for testName, tc := range testCases {
98+ t.Run(testName, func(t *testing.T) {
99+ splitCookies := splitCookie(tc)
100+ for i, cookie := range splitCookies {
101+ if i < len(splitCookies)-1 {
102+ assert.Equal(t, 4000, len(cookie.String()))
103+ } else {
104+ assert.GreaterOrEqual(t, 4000, len(cookie.String()))
105+ }
106+ }
107+ })
108+ }
109+}
110+
111+func Test_splitCookieName(t *testing.T) {
112+ testCases := map[string]struct {
113+ Name string
114+ Count int
115+ Output string
116+ }{
117+ "Standard length": {
118+ Name: "IAmSoNormal",
119+ Count: 2,
120+ Output: "IAmSoNormal_2",
121+ },
122+ "Max length": {
123+ Name: strings.Repeat("n", 256),
124+ Count: 1,
125+ Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 254), 1),
126+ },
127+ "Large count overflow": {
128+ Name: strings.Repeat("n", 253),
129+ Count: 1000,
130+ Output: fmt.Sprintf("%s_%d", strings.Repeat("n", 251), 1000),
131+ },
132+ }
133+ for testName, tc := range testCases {
134+ t.Run(testName, func(t *testing.T) {
135+ splitName := splitCookieName(tc.Name, tc.Count)
136+ assert.Equal(t, tc.Output, splitName)
137+ })
138+ }
139+}
140+
141+func Test_splitCookie_joinCookies(t *testing.T) {
142+ const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
143+
144+ v := make([]byte, 251)
145+ for i := range v {
146+ v[i] = charset[mathrand.Intn(len(charset))]
147+ }
148+ value := strings.Repeat(string(v), 1000)
149+
150+ for _, nameSize := range []int{1, 10, 50, 100, 200, 254} {
151+ t.Run(fmt.Sprintf("%d length cookie name", nameSize), func(t *testing.T) {
152+ cookie := &http.Cookie{
153+ Name: strings.Repeat("n", nameSize),
154+ Value: value,
155+ }
156+ splitCookies := splitCookie(cookie)
157+ joinedCookie, err := joinCookies(splitCookies)
158+ assert.NoError(t, err)
159+ assert.Equal(t, *cookie, *joinedCookie)
160+ })
161+ }
162+}