diff --git a/pkg/auth/sessions/combined_sessions.go b/pkg/auth/sessions/combined_sessions.go index 8a1aaf03827..18c9f6ac3b1 100644 --- a/pkg/auth/sessions/combined_sessions.go +++ b/pkg/auth/sessions/combined_sessions.go @@ -43,10 +43,35 @@ func NewSessionStore(authnKey, encryptKey []byte, secureCookies bool, cookiePath } } +// expireOldPodCookies expires session cookies from other pods to prevent cookie accumulation +// when users are load-balanced across multiple pods. +func (cs *CombinedSessionStore) expireOldPodCookies(w http.ResponseWriter, r *http.Request) { + currentCookieName := SessionCookieName() + for _, cookie := range r.Cookies() { + // Expire any session cookies that are not for the current pod + if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) && cookie.Name != currentCookieName { + // Must match all attributes of the original cookie for browsers to properly delete it + http.SetCookie(w, &http.Cookie{ + Name: cookie.Name, + Value: "", + Path: cs.clientStore.Options.Path, + MaxAge: -1, + Secure: cs.clientStore.Options.Secure, + HttpOnly: cs.clientStore.Options.HttpOnly, + SameSite: cs.clientStore.Options.SameSite, + }) + } + } +} + func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Request, tokenVerifier IDTokenVerifier, token *oauth2.Token) (*LoginState, error) { cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods before creating new session + // This prevents cookie accumulation when users are load-balanced across multiple pods + cs.expireOldPodCookies(w, r) + ls, err := cs.serverStore.AddSession(tokenVerifier, token) if err != nil { return nil, fmt.Errorf("failed to add session to server store: %w", err) @@ -87,6 +112,11 @@ func (cs *CombinedSessionStore) GetSession(w http.ResponseWriter, r *http.Reques cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods + // This is done here because GetSession is called on /api/* requests where + // session cookies (with Path=/api) are actually sent by the browser + cs.expireOldPodCookies(w, r) + // Get always returns a session, even if empty. clientSession := cs.getCookieSession(r) @@ -136,6 +166,10 @@ func (cs *CombinedSessionStore) UpdateTokens(w http.ResponseWriter, r *http.Requ cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods when refreshing tokens + // This handles the case where a user is load-balanced to a different pod + cs.expireOldPodCookies(w, r) + clientSession := cs.getCookieSession(r) var oldRefreshTokenID string var oldRefreshToken string @@ -193,10 +227,16 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req defer cs.sessionLock.Unlock() for _, cookie := range r.Cookies() { - cookie := cookie if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) { - cookie.MaxAge = -1 - http.SetCookie(w, cookie) + http.SetCookie(w, &http.Cookie{ + Name: cookie.Name, + Value: "", + Path: cs.clientStore.Options.Path, + MaxAge: -1, + Secure: cs.clientStore.Options.Secure, + HttpOnly: cs.clientStore.Options.HttpOnly, + SameSite: cs.clientStore.Options.SameSite, + }) } } diff --git a/pkg/auth/sessions/combined_sessions_test.go b/pkg/auth/sessions/combined_sessions_test.go index 9c1ec5f4563..268d210e03e 100644 --- a/pkg/auth/sessions/combined_sessions_test.go +++ b/pkg/auth/sessions/combined_sessions_test.go @@ -52,7 +52,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { }, testIDToken, ), - origRefreshToken: utilptr.To[string]("orig-refresh-token"), + origRefreshToken: utilptr.To("orig-refresh-token"), wantRefreshToken: "random-token-string", wantRawToken: testIDToken, }, @@ -64,7 +64,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { }, testIDToken, ), - origSessionToken: utilptr.To[string]("orig-session-token"), + origSessionToken: utilptr.To("orig-session-token"), wantRefreshToken: "random-token-string", wantRawToken: testIDToken, }, @@ -93,7 +93,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { &oauth2.Token{}, testIDToken, ), - origRefreshToken: utilptr.To[string]("random-token-string"), + origRefreshToken: utilptr.To("random-token-string"), wantRawToken: testIDToken, }, } @@ -695,3 +695,135 @@ func indexSessionByRefreshToken(serverStore *SessionStore, refreshToken string, serverStore.byRefreshToken[refreshToken] = session return serverStore } + +func TestCombinedSessionStore_AddSession_CleansUpOldPodCookies(t *testing.T) { + testIDToken := createTestIDToken(`{"sub":"user-id-0"}`) + testVerifier := newTestVerifier(`{"sub":"user-id-0"}`) + + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + token := addIDToken( + &oauth2.Token{ + RefreshToken: "new-refresh-token", + }, + testIDToken, + ) + + // Simulate request with old session cookies from different pods + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, err := cs.AddSession(testWriter, req, testVerifier, token) + require.NoError(t, err) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + newSessionCookie := false + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == SessionCookieName() { + require.NotEqual(t, -1, c.MaxAge, "New session cookie should not be expired") + newSessionCookie = true + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") + require.True(t, newSessionCookie, "New session cookie should be created") +} + +func TestCombinedSessionStore_GetSession_CleansUpOldPodCookies(t *testing.T) { + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + // Simulate request with old session cookies from different pods + // This is the primary cleanup path since GetSession is called on /api/* requests + // where session cookies (with Path=/api) are actually sent by the browser + req := httptest.NewRequest(http.MethodGet, "/api/some-endpoint", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, _ = cs.GetSession(testWriter, req) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") +} + +func TestCombinedSessionStore_UpdateTokens_CleansUpOldPodCookies(t *testing.T) { + currentTime := strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10) + testIDToken := createTestIDToken(`{"sub":"user-id-0","exp":` + currentTime + `}`) + testVerifier := newTestVerifier(`{"sub":"user-id-0","exp":` + currentTime + `}`) + + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + token := addIDToken( + &oauth2.Token{ + RefreshToken: "new-refresh-token", + }, + testIDToken, + ) + + // Simulate request with old session cookies from different pods + // This simulates the scenario where a user is load-balanced to a new pod + // and their token is being refreshed + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, err := cs.UpdateTokens(testWriter, req, testVerifier, token) + require.NoError(t, err) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + newSessionCookie := false + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == SessionCookieName() { + require.NotEqual(t, -1, c.MaxAge, "New session cookie should not be expired") + newSessionCookie = true + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") + require.True(t, newSessionCookie, "New session cookie should be created") +}