From bade7461ca294af88555ecfec50ff3389aa32c4f Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 12 Jul 2019 15:39:03 -0700 Subject: [PATCH 1/3] internal/sessions: add cookie chunking --- CHANGELOG.md | 5 +- internal/sessions/cookie_store.go | 69 ++++++++++++++++++++++++- internal/sessions/session_state.go | 5 -- internal/sessions/session_state_test.go | 3 +- 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abaefb71c..872b5a1b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,10 @@ ### New - ### Changed - GRPC Metrics Implementation [GH-218] + - Additional GRPC server metrics and request sizes - Improved GRPC metrics implementation internals - The GRPC method label is now 'grpc_method' and GRPC status is now `grpc_client_status` and `grpc_server_status` @@ -15,10 +15,13 @@ - GRPC version upgraded to v1.22 [GH-219] - HTTP Metrics Implementation [GH-220] + - Support HTTP request sizes on client and server side of proxy - Improved HTTP metrics implementation internals - The HTTP method label is now `http_method`, and HTTP status label is now `http_status` +- Add support for large cookie sessions by chunking. [GH-211] + ## v0.1.0 ### NEW diff --git a/internal/sessions/cookie_store.go b/internal/sessions/cookie_store.go index aeceb07d5..4492312e4 100644 --- a/internal/sessions/cookie_store.go +++ b/internal/sessions/cookie_store.go @@ -9,11 +9,27 @@ import ( "time" "github.com/pomerium/pomerium/internal/cryptutil" + "github.com/pomerium/pomerium/internal/log" ) // ErrInvalidSession is an error for invalid sessions. var ErrInvalidSession = errors.New("internal/sessions: invalid session") +// ChunkedCanaryByte is the byte value used as a canary prefix to distinguish if +// the cookie is multi-part or not. This constant *should not* be valid +// base64. It's important this byte is ASCII to avoid UTF-8 variable sized runes. +// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Directives +const ChunkedCanaryByte byte = '%' + +// MaxChunkSize sets the upper bound on a cookie chunks payload value. +// Note, this should be lower than the actual cookie's max size (4096 bytes) +// which includes metadata. +const MaxChunkSize = 3800 + +// MaxNumChunks limits the number of chunks to iterate through. Conservatively +// set to prevent any abuse. +const MaxNumChunks = 5 + // CSRFStore has the functions for setting, getting, and clearing the CSRF cookie type CSRFStore interface { SetCSRF(http.ResponseWriter, *http.Request, string) @@ -111,6 +127,40 @@ func (s *CookieStore) makeCSRFCookie(req *http.Request, value string, expiration return s.makeCookie(req, s.csrfName(), value, expiration, now) } +func (s *CookieStore) SetCookie(w http.ResponseWriter, cookie *http.Cookie) { + if len(cookie.String()) <= MaxChunkSize { + http.SetCookie(w, cookie) + } else { + chunks := chunk(cookie.Value, MaxChunkSize) + for i, c := range chunks { + // start with a copy of our original cookie + nc := *cookie + if i == 0 { + // if this is the first cookie, add our canary byte + nc.Value = fmt.Sprintf("%s%s", string(ChunkedCanaryByte), c) + } else { + // subsequent parts will be postfixed with their part number + nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i) + nc.Value = fmt.Sprintf("%s", c) + } + log.Info().Interface("new cookie", nc).Msg("SetCookie: chunked") + http.SetCookie(w, &nc) + } + } + +} + +func chunk(s string, size int) []string { + ss := make([]string, 0, len(s)/size+1) + for len(s) > 0 { + if len(s) < size { + size = len(s) + } + ss, s = append(ss, s[:size]), s[size:] + } + return ss +} + // ClearCSRF clears the CSRF cookie from the request func (s *CookieStore) ClearCSRF(w http.ResponseWriter, req *http.Request) { http.SetCookie(w, s.makeCSRFCookie(req, "", time.Hour*-1, time.Now())) @@ -132,7 +182,7 @@ func (s *CookieStore) ClearSession(w http.ResponseWriter, req *http.Request) { } func (s *CookieStore) setSessionCookie(w http.ResponseWriter, req *http.Request, val string) { - http.SetCookie(w, s.makeSessionCookie(req, val, s.CookieExpire, time.Now())) + s.SetCookie(w, s.makeSessionCookie(req, val, s.CookieExpire, time.Now())) } // LoadSession returns a SessionState from the cookie in the request. @@ -141,7 +191,22 @@ func (s *CookieStore) LoadSession(req *http.Request) (*SessionState, error) { if err != nil { return nil, err // http.ErrNoCookie } - session, err := UnmarshalSession(c.Value, s.CookieCipher) + cipherText := c.Value + + // if the first byte is our canary byte, we need to handle the multipart bit + if []byte(c.Value)[0] == ChunkedCanaryByte { + var b strings.Builder + fmt.Fprintf(&b, "%s", cipherText[1:]) + for i := 1; i < MaxNumChunks; i++ { + next, err := req.Cookie(fmt.Sprintf("%s_%d", s.Name, i)) + if err != nil { + break // break if we can't find the next cookie + } + fmt.Fprintf(&b, "%s", next.Value) + } + cipherText = b.String() + } + session, err := UnmarshalSession(cipherText, s.CookieCipher) if err != nil { return nil, ErrInvalidSession } diff --git a/internal/sessions/session_state.go b/internal/sessions/session_state.go index 6750c5dcc..41e9df2b5 100644 --- a/internal/sessions/session_state.go +++ b/internal/sessions/session_state.go @@ -11,8 +11,6 @@ import ( "github.com/pomerium/pomerium/internal/cryptutil" ) -const MaxCookieSize = 4096 - var ( // ErrLifetimeExpired is an error for the lifetime deadline expiring ErrLifetimeExpired = errors.New("user lifetime expired") @@ -93,9 +91,6 @@ func MarshalSession(s *SessionState, c cryptutil.Cipher) (string, error) { if err != nil { return "", err } - if len(v) >= MaxCookieSize { - return "", fmt.Errorf("session too large, got %d bytes", len(v)) - } return v, nil } diff --git a/internal/sessions/session_state_test.go b/internal/sessions/session_state_test.go index 28144fb1e..19277c7c0 100644 --- a/internal/sessions/session_state_test.go +++ b/internal/sessions/session_state_test.go @@ -2,6 +2,7 @@ package sessions import ( "crypto/rand" + "fmt" "reflect" "testing" "time" @@ -157,7 +158,7 @@ func TestMarshalSession(t *testing.T) { wantErr bool }{ {"simple", &SessionState{}, false}, - {"too big", &SessionState{AccessToken: string(hugeString)}, true}, + {"too big", &SessionState{AccessToken: fmt.Sprintf("%x", hugeString)}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 5b2f6ecd2f6439e8d575cf12a4a2871af6d8ae53 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 12 Jul 2019 15:46:05 -0700 Subject: [PATCH 2/3] update tests --- internal/sessions/cookie_store.go | 30 +++++++++++++------------- internal/sessions/cookie_store_test.go | 25 ++++++++------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/internal/sessions/cookie_store.go b/internal/sessions/cookie_store.go index 4492312e4..3d237820f 100644 --- a/internal/sessions/cookie_store.go +++ b/internal/sessions/cookie_store.go @@ -130,22 +130,22 @@ func (s *CookieStore) makeCSRFCookie(req *http.Request, value string, expiration func (s *CookieStore) SetCookie(w http.ResponseWriter, cookie *http.Cookie) { if len(cookie.String()) <= MaxChunkSize { http.SetCookie(w, cookie) - } else { - chunks := chunk(cookie.Value, MaxChunkSize) - for i, c := range chunks { - // start with a copy of our original cookie - nc := *cookie - if i == 0 { - // if this is the first cookie, add our canary byte - nc.Value = fmt.Sprintf("%s%s", string(ChunkedCanaryByte), c) - } else { - // subsequent parts will be postfixed with their part number - nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i) - nc.Value = fmt.Sprintf("%s", c) - } - log.Info().Interface("new cookie", nc).Msg("SetCookie: chunked") - http.SetCookie(w, &nc) + return + } + chunks := chunk(cookie.Value, MaxChunkSize) + for i, c := range chunks { + // start with a copy of our original cookie + nc := *cookie + if i == 0 { + // if this is the first cookie, add our canary byte + nc.Value = fmt.Sprintf("%s%s", string(ChunkedCanaryByte), c) + } else { + // subsequent parts will be postfixed with their part number + nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i) + nc.Value = fmt.Sprintf("%s", c) } + log.Info().Interface("new cookie", nc).Msg("SetCookie: chunked") + http.SetCookie(w, &nc) } } diff --git a/internal/sessions/cookie_store_test.go b/internal/sessions/cookie_store_test.go index 377d34643..44b484400 100644 --- a/internal/sessions/cookie_store_test.go +++ b/internal/sessions/cookie_store_test.go @@ -1,7 +1,9 @@ package sessions import ( + "crypto/rand" "errors" + "fmt" "net/http" "net/http/httptest" "reflect" @@ -204,6 +206,10 @@ func TestCookieStore_SaveSession(t *testing.T) { if err != nil { t.Fatal(err) } + hugeString := make([]byte, 4097) + if _, err := rand.Read(hugeString); err != nil { + t.Fatal(err) + } tests := []struct { name string sessionState *SessionState @@ -211,22 +217,9 @@ func TestCookieStore_SaveSession(t *testing.T) { wantErr bool wantLoadErr bool }{ - {"good", - &SessionState{ - AccessToken: "token1234", - RefreshToken: "refresh4321", - RefreshDeadline: time.Now().Add(1 * time.Hour).Truncate(time.Second).UTC(), - Email: "user@domain.com", - User: "user", - }, cipher, false, false}, - {"bad cipher", - &SessionState{ - AccessToken: "token1234", - RefreshToken: "refresh4321", - RefreshDeadline: time.Now().Add(1 * time.Hour).Truncate(time.Second).UTC(), - Email: "user@domain.com", - User: "user", - }, mockCipher{}, true, true}, + {"good", &SessionState{AccessToken: "token1234", RefreshToken: "refresh4321", RefreshDeadline: time.Now().Add(1 * time.Hour).Truncate(time.Second).UTC(), Email: "user@domain.com", User: "user"}, cipher, false, false}, + {"bad cipher", &SessionState{AccessToken: "token1234", RefreshToken: "refresh4321", RefreshDeadline: time.Now().Add(1 * time.Hour).Truncate(time.Second).UTC(), Email: "user@domain.com", User: "user"}, mockCipher{}, true, true}, + {"huge cookie", &SessionState{AccessToken: fmt.Sprintf("%x", hugeString), RefreshToken: "refresh4321", RefreshDeadline: time.Now().Add(1 * time.Hour).Truncate(time.Second).UTC(), Email: "user@domain.com", User: "user"}, cipher, false, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 62210c7caf379b5f681ec6703d8df3d000c58497 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 12 Jul 2019 15:49:49 -0700 Subject: [PATCH 3/3] make unexported --- internal/sessions/cookie_store.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/sessions/cookie_store.go b/internal/sessions/cookie_store.go index 3d237820f..6922bfa82 100644 --- a/internal/sessions/cookie_store.go +++ b/internal/sessions/cookie_store.go @@ -9,7 +9,6 @@ import ( "time" "github.com/pomerium/pomerium/internal/cryptutil" - "github.com/pomerium/pomerium/internal/log" ) // ErrInvalidSession is an error for invalid sessions. @@ -127,13 +126,12 @@ func (s *CookieStore) makeCSRFCookie(req *http.Request, value string, expiration return s.makeCookie(req, s.csrfName(), value, expiration, now) } -func (s *CookieStore) SetCookie(w http.ResponseWriter, cookie *http.Cookie) { +func (s *CookieStore) setCookie(w http.ResponseWriter, cookie *http.Cookie) { if len(cookie.String()) <= MaxChunkSize { http.SetCookie(w, cookie) return } - chunks := chunk(cookie.Value, MaxChunkSize) - for i, c := range chunks { + for i, c := range chunk(cookie.Value, MaxChunkSize) { // start with a copy of our original cookie nc := *cookie if i == 0 { @@ -144,7 +142,6 @@ func (s *CookieStore) SetCookie(w http.ResponseWriter, cookie *http.Cookie) { nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i) nc.Value = fmt.Sprintf("%s", c) } - log.Info().Interface("new cookie", nc).Msg("SetCookie: chunked") http.SetCookie(w, &nc) } @@ -182,7 +179,7 @@ func (s *CookieStore) ClearSession(w http.ResponseWriter, req *http.Request) { } func (s *CookieStore) setSessionCookie(w http.ResponseWriter, req *http.Request, val string) { - s.SetCookie(w, s.makeSessionCookie(req, val, s.CookieExpire, time.Now())) + s.setCookie(w, s.makeSessionCookie(req, val, s.CookieExpire, time.Now())) } // LoadSession returns a SessionState from the cookie in the request.