Skip to content

Commit be62fbb

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #15631 from TheRealJon/OCPBUGS-62142
OCPBUGS-62142: Fix cookie size limit error with large OIDC refresh tokens
2 parents 645d5a6 + 5d33468 commit be62fbb

File tree

5 files changed

+195
-62
lines changed

5 files changed

+195
-62
lines changed

‎pkg/auth/oauth2/auth_oidc_test.go‎

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,10 @@ func Test_oidcAuth_refreshSession(t *testing.T) {
360360
name: "session exists with different refresh token - session tokens match short-circuit to prevent multiple refreshes",
361361
initSessions: func(s *sessions.CombinedSessionStore) (initSessionToken string) {
362362
testCookieFactory := &testCookieFactory{
363-
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
363+
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
364+
serverStore: s.ServerStore(),
365+
tokenVerifier: oidcProvider.verifyIDToken,
366+
signPayload: oidcProvider.signPayload,
364367
}
365368
// inject the refresh token into the refresh token cache
366369
testCookieFactory.WithRefreshToken("test-original-refresh-token")
@@ -406,7 +409,10 @@ func Test_oidcAuth_refreshSession(t *testing.T) {
406409
require.NoError(t, err)
407410

408411
testCookieFactory := &testCookieFactory{
409-
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
412+
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
413+
serverStore: o.sessions.ServerStore(),
414+
tokenVerifier: oidcProvider.verifyIDToken,
415+
signPayload: oidcProvider.signPayload,
410416
}
411417
if len(tt.cookieRefreshToken) > 0 {
412418
testCookieFactory.WithRefreshToken(tt.cookieRefreshToken)
@@ -530,7 +536,10 @@ func Test_oidcAuth_getLoginState(t *testing.T) {
530536
require.NoError(t, err)
531537

532538
testCookieFactory := &testCookieFactory{
533-
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
539+
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
540+
serverStore: o.sessions.ServerStore(),
541+
tokenVerifier: oidcProvider.verifyIDToken,
542+
signPayload: oidcProvider.signPayload,
534543
}
535544
if len(tt.cookieRefreshToken) > 0 {
536545
testCookieFactory.WithRefreshToken(tt.cookieRefreshToken)
@@ -609,8 +618,11 @@ func BenchmarkRefreshSession(b *testing.B) {
609618
<-startChan
610619
refreshToken := testValidRefreshToken + strconv.Itoa(tokenSuffix)
611620
testCookieFactory := &testCookieFactory{
612-
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
613-
refreshToken: &refreshToken,
621+
cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey),
622+
refreshToken: &refreshToken,
623+
serverStore: o.sessions.ServerStore(),
624+
tokenVerifier: oidcProvider.verifyIDToken,
625+
signPayload: oidcProvider.signPayload,
614626
}
615627
req := httptest.NewRequest("GET", "/", nil)
616628
testCookieFactory.Complete(b, req)
@@ -687,10 +699,14 @@ func testOAuth2ConfigConstructor(endpointConfig oauth2.Endpoint) *oauth2.Config
687699
}
688700

689701
type testCookieFactory struct {
690-
cookieCodecs []securecookie.Codec
691-
sessionToken *string
692-
refreshToken *string
693-
customCookies map[string]map[interface{}]interface{}
702+
cookieCodecs []securecookie.Codec
703+
sessionToken *string
704+
refreshToken *string
705+
refreshTokenID *string
706+
customCookies map[string]map[interface{}]interface{}
707+
serverStore *sessions.SessionStore // needed to set up refresh token ID mapping
708+
tokenVerifier sessions.IDTokenVerifier
709+
signPayload func(string) string // function to sign an ID token payload
694710
}
695711

696712
func (f *testCookieFactory) WithSessionToken(sessionToken string) *testCookieFactory {
@@ -720,9 +736,26 @@ func (f *testCookieFactory) Complete(t testing.TB, req *http.Request) *http.Requ
720736
f.cookieCodecs)
721737
}
722738
if f.refreshToken != nil {
739+
var id string
740+
if f.serverStore != nil && f.tokenVerifier != nil && f.signPayload != nil {
741+
// Use AddSession to properly set up the state
742+
token := addIDToken(
743+
&oauth2.Token{RefreshToken: *f.refreshToken},
744+
f.signPayload(`{"sub":"testuser","exp":`+strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10)+`}`),
745+
)
746+
loginState, err := f.serverStore.AddSession(f.tokenVerifier, token)
747+
require.NoError(t, err)
748+
id = loginState.RefreshTokenID()
749+
} else {
750+
// Fallback to generating a random ID
751+
id = randomString(32)
752+
}
753+
f.refreshTokenID = &id
754+
755+
// Store only the ID in the cookie
723756
attachCookieOrDie(t, req, "openshift-refresh-token",
724757
map[interface{}]interface{}{
725-
"refresh-token": f.refreshToken,
758+
"refresh-token-id": id,
726759
},
727760
f.cookieCodecs)
728761
}

‎pkg/auth/sessions/combined_sessions.go‎

Lines changed: 60 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Reques
5454

5555
clientSession := cs.getCookieSession(r)
5656
clientSession.sessionToken.Values["session-token"] = ls.sessionToken
57-
clientSession.refreshToken.Values["refresh-token"] = ls.refreshToken
57+
// Store only the small reference ID in the cookie, not the full refresh token
58+
clientSession.refreshToken.Values["refresh-token-id"] = ls.refreshTokenID
5859

5960
return ls, clientSession.save(r, w)
6061
}
@@ -89,12 +90,19 @@ func (cs *CombinedSessionStore) GetSession(w http.ResponseWriter, r *http.Reques
8990
// Get always returns a session, even if empty.
9091
clientSession := cs.getCookieSession(r)
9192

92-
var sessionToken, refreshToken string
93+
var (
94+
sessionToken string
95+
refreshToken string
96+
)
97+
9398
if sessionTokenIface, ok := clientSession.sessionToken.Values["session-token"]; ok {
9499
sessionToken = sessionTokenIface.(string)
95100
}
96-
if refreshTokenIface, ok := clientSession.refreshToken.Values["refresh-token"]; ok {
97-
refreshToken = refreshTokenIface.(string)
101+
if refreshTokenID, ok := clientSession.refreshToken.Values["refresh-token-id"]; ok {
102+
// Look up the actual refresh token from the ID
103+
if actualToken, exists := cs.serverStore.byRefreshTokenID[refreshTokenID.(string)]; exists {
104+
refreshToken = actualToken
105+
}
98106
}
99107

100108
loginState := cs.serverStore.GetSession(sessionToken, refreshToken)
@@ -104,16 +112,23 @@ func (cs *CombinedSessionStore) GetSession(w http.ResponseWriter, r *http.Reques
104112
func (cs *CombinedSessionStore) GetCookieRefreshToken(r *http.Request) string {
105113
// Get always returns a session, even if empty.
106114
clientSession, _ := cs.clientStore.Get(r, openshiftRefreshTokenCookieName)
107-
if refreshToken, ok := clientSession.Values["refresh-token"].(string); ok {
108-
return refreshToken
115+
if refreshTokenID, ok := clientSession.Values["refresh-token-id"].(string); ok {
116+
// Look up the actual refresh token using the ID
117+
if actualToken, exists := cs.serverStore.byRefreshTokenID[refreshTokenID]; exists {
118+
return actualToken
119+
}
109120
}
110121
return ""
111122
}
112123

113124
func (cs *CombinedSessionStore) UpdateCookieRefreshToken(w http.ResponseWriter, r *http.Request, refreshToken string) error {
114-
// no need to lock here since there shouldn't be any races around the client session
125+
// Generate a new ID for the refresh token
126+
newID := RandomString(32)
127+
cs.serverStore.byRefreshTokenID[newID] = refreshToken
128+
129+
// Store the ID in the cookie, not the full token
115130
clientSession, _ := cs.clientStore.Get(r, openshiftRefreshTokenCookieName)
116-
clientSession.Values["refresh-token"] = refreshToken
131+
clientSession.Values["refresh-token-id"] = newID
117132
return clientSession.Save(r, w)
118133
}
119134

@@ -122,13 +137,25 @@ func (cs *CombinedSessionStore) UpdateTokens(w http.ResponseWriter, r *http.Requ
122137
defer cs.sessionLock.Unlock()
123138

124139
clientSession := cs.getCookieSession(r)
140+
var oldRefreshTokenID string
125141
var oldRefreshToken string
126-
if oldToken, ok := clientSession.refreshToken.Values["refresh-token"]; ok {
127-
oldRefreshToken = oldToken.(string)
142+
if oldID, ok := clientSession.refreshToken.Values["refresh-token-id"]; ok {
143+
oldRefreshTokenID = oldID.(string)
144+
// Look up the actual refresh token from the ID
145+
if actualToken, exists := cs.serverStore.byRefreshTokenID[oldRefreshTokenID]; exists {
146+
oldRefreshToken = actualToken
147+
}
128148
}
129149

130-
refreshToken := tokenResponse.RefreshToken
131-
clientSession.refreshToken.Values["refresh-token"] = refreshToken
150+
// Generate a new ID for the new refresh token
151+
newRefreshTokenID := RandomString(32)
152+
newRefreshToken := tokenResponse.RefreshToken
153+
if newRefreshToken != "" {
154+
cs.serverStore.byRefreshTokenID[newRefreshTokenID] = newRefreshToken
155+
}
156+
157+
// Store the new ID in the cookie
158+
clientSession.refreshToken.Values["refresh-token-id"] = newRefreshTokenID
132159

133160
var loginState *LoginState
134161
sessionToken, ok := clientSession.sessionToken.Values["session-token"]
@@ -142,16 +169,22 @@ func (cs *CombinedSessionStore) UpdateTokens(w http.ResponseWriter, r *http.Requ
142169
return nil, fmt.Errorf("failed to add session to server store: %w", err)
143170
}
144171
clientSession.sessionToken.Values["session-token"] = loginState.sessionToken
172+
// AddSession already generated an ID, so update the cookie with it
173+
clientSession.refreshToken.Values["refresh-token-id"] = loginState.refreshTokenID
145174
} else {
146175
// loginState is a pointer to the cache so this effectively mutates it for everyone
147176
if err := loginState.UpdateTokens(tokenVerifier, tokenResponse); err != nil {
148177
return nil, err
149178
}
179+
// Update the ID in the LoginState
180+
loginState.refreshTokenID = newRefreshTokenID
150181
}
151182

152183
// index by the old refresh token so that any follow-up requests that arrived
153184
// before their cookie was updated with an actual session can still find the login state
154-
cs.serverStore.byRefreshToken[oldRefreshToken] = loginState
185+
if oldRefreshToken != "" {
186+
cs.serverStore.byRefreshToken[oldRefreshToken] = loginState
187+
}
155188
return loginState, clientSession.save(r, w)
156189
}
157190

@@ -168,8 +201,14 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req
168201
}
169202

170203
cookieSession := cs.getCookieSession(r)
171-
if refreshToken, ok := cookieSession.refreshToken.Values["refresh-token"]; ok {
172-
cs.serverStore.DeleteByRefreshToken(refreshToken.(string))
204+
if refreshTokenID, ok := cookieSession.refreshToken.Values["refresh-token-id"]; ok {
205+
refreshTokenIDStr := refreshTokenID.(string)
206+
// Look up the actual refresh token from the ID and delete
207+
if actualToken, exists := cs.serverStore.byRefreshTokenID[refreshTokenIDStr]; exists {
208+
cs.serverStore.DeleteByRefreshToken(actualToken)
209+
// Clean up the ID mapping
210+
delete(cs.serverStore.byRefreshTokenID, refreshTokenIDStr)
211+
}
173212
}
174213

175214
if sessionToken, ok := cookieSession.sessionToken.Values["session-token"]; ok {
@@ -185,3 +224,9 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req
185224

186225
return nil
187226
}
227+
228+
// ServerStore returns the underlying server session store.
229+
// This is primarily used for testing purposes.
230+
func (cs *CombinedSessionStore) ServerStore() *SessionStore {
231+
return cs.serverStore
232+
}

‎pkg/auth/sessions/combined_sessions_test.go‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,13 @@ func TestCombinedSessionStore_AddSession(t *testing.T) {
166166
refreshFound = true
167167
gotRefresh := make(map[interface{}]interface{})
168168
require.NoError(t, securecookie.DecodeMulti(openshiftRefreshTokenCookieName, c.Value, &gotRefresh, cookieCodecs...))
169-
if gotRefresh["refresh-token"] != tt.wantRefreshToken {
170-
t.Errorf("wanted refresh cookie to be %q, got %q", tt.wantRefreshToken, c.Value)
169+
// The cookie now contains an ID, not the actual refresh token
170+
refreshTokenID := gotRefresh["refresh-token-id"].(string)
171+
actualRefreshToken := cs.serverStore.byRefreshTokenID[refreshTokenID]
172+
if actualRefreshToken != tt.wantRefreshToken {
173+
t.Errorf("wanted refresh token to be %q, got %q (via ID %q)", tt.wantRefreshToken, actualRefreshToken, refreshTokenID)
171174
}
172175
}
173-
174176
}
175177

176178
require.True(t, sessionFound, "session cookie not found")
@@ -239,6 +241,7 @@ func TestCombinedSessionStore_GetSession(t *testing.T) {
239241

240242
testCookies := &testCookieFactory{
241243
cookieCodecs: cookieCodecs,
244+
serverStore: cs.serverStore,
242245
}
243246

244247
req, err := http.NewRequest(http.MethodGet, "/", nil)
@@ -367,7 +370,7 @@ func TestCombinedSessionStore_UpdateTokens(t *testing.T) {
367370
req, err := http.NewRequest(http.MethodGet, "/", nil)
368371
require.NoError(t, err)
369372

370-
testCookieFactory := &testCookieFactory{cookieCodecs: cookieCodecs}
373+
testCookieFactory := &testCookieFactory{cookieCodecs: cookieCodecs, serverStore: cs.serverStore}
371374
if len(tt.currentSessionToken) > 0 {
372375
testCookieFactory.WithSessionToken(tt.currentSessionToken)
373376
}
@@ -557,6 +560,7 @@ func TestCombinedSessionStore_DeleteSession(t *testing.T) {
557560

558561
if tt.cookieStore != nil {
559562
tt.cookieStore.cookieCodecs = cookieCodecs
563+
tt.cookieStore.serverStore = cs.serverStore
560564
req = tt.cookieStore.Complete(t, req)
561565
}
562566

@@ -619,10 +623,12 @@ func TestCombinedSessionStore_DeleteSession(t *testing.T) {
619623
}
620624

621625
type testCookieFactory struct {
622-
cookieCodecs []securecookie.Codec
623-
sessionToken *string
624-
refreshToken *string
625-
customCookies map[string]map[interface{}]interface{}
626+
cookieCodecs []securecookie.Codec
627+
sessionToken *string
628+
refreshToken *string
629+
refreshTokenID *string
630+
customCookies map[string]map[interface{}]interface{}
631+
serverStore *SessionStore // needed to set up refresh token ID mapping
626632
}
627633

628634
func (f *testCookieFactory) WithSessionToken(sessionToken string) *testCookieFactory {
@@ -652,9 +658,17 @@ func (f *testCookieFactory) Complete(t *testing.T, req *http.Request) *http.Requ
652658
f.cookieCodecs)
653659
}
654660
if f.refreshToken != nil {
661+
// Generate an ID for the refresh token and store the mapping
662+
id := randomString(32)
663+
if f.serverStore != nil {
664+
f.serverStore.byRefreshTokenID[id] = *f.refreshToken
665+
}
666+
f.refreshTokenID = &id
667+
668+
// Store only the ID in the cookie
655669
attachCookieOrDie(t, req, openshiftRefreshTokenCookieName,
656670
map[interface{}]interface{}{
657-
"refresh-token": f.refreshToken,
671+
"refresh-token-id": id,
658672
},
659673
f.cookieCodecs)
660674
}

‎pkg/auth/sessions/loginstate.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,16 @@ type IDTokenVerifier func(context.Context, string) (*oidc.IDToken, error)
2121
// and should be safe to send as a non-http-only cookie.
2222
type LoginState struct {
2323
// IMPORTANT: if adding any ref type, change the DeepCopy() implementation
24-
userID string
25-
name string
26-
email string
27-
exp time.Time
28-
rotateAt time.Time // 80% of token's lifetime
29-
now nowFunc
30-
sessionToken string
31-
rawToken string
32-
refreshToken string
24+
userID string
25+
name string
26+
email string
27+
exp time.Time
28+
rotateAt time.Time // 80% of token's lifetime
29+
now nowFunc
30+
sessionToken string
31+
rawToken string
32+
refreshToken string
33+
refreshTokenID string // Small reference ID for the refresh token (stored in cookie)
3334
}
3435

3536
type LoginJSON struct {
@@ -164,6 +165,10 @@ func (ls *LoginState) RefreshToken() string {
164165
return ls.refreshToken
165166
}
166167

168+
func (ls *LoginState) RefreshTokenID() string {
169+
return ls.refreshTokenID
170+
}
171+
167172
func (ls *LoginState) IsExpired() bool {
168173
return ls.now().After(ls.exp)
169174
}

0 commit comments

Comments
 (0)