OCPBUGS-71237: Persist console sessions across pod restarts - #16911
OCPBUGS-71237: Persist console sessions across pod restarts#16911jhadvig wants to merge 8 commits into
Conversation
Store the actual encrypted OAuth refresh token in the browser cookie instead of a reference ID that maps to an in-memory store. This allows any console pod to recover a user's session after a restart by decrypting the cookie and exchanging the refresh token with the OAuth server. To enable cross-pod cookie decryption, accept shared encryption keys from files (managed by the console-operator via a session-secret Secret) instead of generating random keys per process. The existing recovery mechanism in getLoginState() already handles the token refresh — the only gap was making the refresh token available from the cookie. Backward compatible: old-format cookies with reference IDs are still accepted via fallback lookup during rolling upgrades. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references Jira Issue OCPBUGS-71237, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOpenShift authentication now accepts paired cookie keys. Session cookies store actual refresh tokens and retain legacy lookup. Recovery cookies restore missing sessions. Playwright tests verify console session persistence after pod recovery and plugin rollout. ChangesOpenShift session persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant Console
participant OpenShiftAuthenticator
participant CombinedSessionStore
participant ConsoleOperator
participant ConsoleDeployment
Playwright->>Console: authenticate and open dashboard
Console->>OpenShiftAuthenticator: request login state
OpenShiftAuthenticator->>CombinedSessionStore: read session or recovery cookie
CombinedSessionStore-->>OpenShiftAuthenticator: return authenticated state
Playwright->>ConsoleDeployment: delete console pods
ConsoleDeployment-->>Console: recover deployment
Playwright->>Console: verify existing session
Playwright->>ConsoleOperator: disable enabled ConsolePlugin
ConsoleOperator->>ConsoleDeployment: trigger rollout
Playwright->>Console: verify existing session after rollout
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/sessions/combined_sessions_test.go`:
- Around line 288-296: Update the legacy-cookie test in
combined_sessions_test.go so it actually exercises legacy resolution through
GetSession: keep the existing byRefreshTokenID setup, add a LoginState entry to
byRefreshToken for "refresh-old", and change the assertion to expect that
session state instead of nil. Use the GetSession path and the existing
testCookies/refresh-old symbols to verify that a legacy cookie resolves
correctly during rolling upgrade.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: efcb9ee2-7afd-4bed-a0f5-e37fcad5a946
📒 Files selected for processing (9)
cmd/bridge/config/session/sessionoptions.gopkg/auth/oauth2/auth.gopkg/auth/oauth2/auth_oidc.gopkg/auth/oauth2/auth_oidc_test.gopkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.gopkg/auth/sessions/combined_sessions_test.gopkg/auth/sessions/loginstate.gopkg/auth/sessions/server_session.go
💤 Files with no reviewable changes (1)
- pkg/auth/sessions/server_session.go
openshift-ci
Bot
added
the
approved
- Remove refresh token value from error log message to prevent credential leakage in pod logs - Revoke refresh token (OAuthAuthorizeToken) at the OAuth server on logout to prevent cookie replay attacks - Raise securecookie MaxLength to 8192 to accommodate large OIDC JWT refresh tokens that exceed the default 4096 limit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/oauth2/auth_openshift.go (1)
75-94: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate configured cookie key lengths before constructing the session store. OpenShift validation checks that both keys are set or both are unset, but it does not validate file contents. Require an authentication key of at least 32 bytes and an encryption key of 16, 24, or 32 bytes; otherwise
NewCookieStorerecords the cipher error and cookie operations fail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/auth/oauth2/auth_openshift.go` around lines 75 - 94, Before constructing the session store in the key-selection flow, validate configured cookie key lengths: require cookieAuthenticationKey to be at least 32 bytes and cookieEncryptionKey to be exactly 16, 24, or 32 bytes. Return a clear validation error for invalid configured values, while preserving random key generation when both keys are unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/oauth2/auth_openshift.go`:
- Around line 216-221: Update the logout handler around
OAuthAuthorizeTokens().Delete so a refresh-token revocation error is returned as
a failure response instead of only being logged; stop the flow before deleting
the local session or returning 204. Preserve the existing successful revocation
behavior, and add a test covering the failed deletion path.
- Around line 216-221: Update logout’s access-token revocation failure path to
clear the session before returning: preserve the revocation error separately,
perform refresh-token cleanup and call sessions.DeleteSession, then return the
original error. Ensure cleanup also runs when OAuthAccessTokens().Delete fails
rather than exiting before the existing logout cleanup flow.
---
Outside diff comments:
In `@pkg/auth/oauth2/auth_openshift.go`:
- Around line 75-94: Before constructing the session store in the key-selection
flow, validate configured cookie key lengths: require cookieAuthenticationKey to
be at least 32 bytes and cookieEncryptionKey to be exactly 16, 24, or 32 bytes.
Return a clear validation error for invalid configured values, while preserving
random key generation when both keys are unset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ace1f7b9-a182-4ba0-a71a-e452c7615b95
📒 Files selected for processing (2)
pkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/sessions/combined_sessions.go
| if refreshToken := ls.RefreshToken(); refreshToken != "" { | ||
| if delErr := oauthClient.OAuthAuthorizeTokens().Delete(ctx, tokenToObjectName(refreshToken), metav1.DeleteOptions{}); delErr != nil { | ||
| klog.V(4).Infof("failed to revoke refresh token on logout: %v", delErr) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant symbols ---'
rg -n -C 12 'func .*DeleteSession|DeleteSession\(|failed to revoke refresh token|OAuthAuthorizeTokens\(\)\.Delete|RefreshToken\(\)' pkg/auth/oauth2 pkg/auth/sessions
printf '%s\n' '--- bounded source slice ---'
sed -n '180,245p' pkg/auth/oauth2/auth_openshift.goRepository: openshift/console
Length of output: 35701
Broken Authentication (CWE-613): Insufficient Session Expiration
Reachability: External · Exploitability: Moderate
Return an error when refresh-token revocation fails.
OAuthAuthorizeTokens().Delete errors are logged, but the handler still deletes the local session and returns 204. A copied refresh-token cookie can remain valid. Return a failure response and add a test for this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/oauth2/auth_openshift.go` around lines 216 - 221, Update the logout
handler around OAuthAuthorizeTokens().Delete so a refresh-token revocation error
is returned as a failure response instead of only being logged; stop the flow
before deleting the local session or returning 204. Preserve the existing
successful revocation behavior, and add a test covering the failed deletion
path.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '180,235p' pkg/auth/oauth2/auth_openshift.go
printf '\n-- related symbols --\n'
rg -n -C 3 'DeleteSession|refreshSession|OAuthAccessTokens\(\)\.Delete|OAuthAuthorizeTokens\(\)\.Delete|RefreshToken\(\)' pkg/auth/oauth2 pkg/auth/sessionsRepository: openshift/console
Length of output: 20240
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/auth/oauth2/auth_openshift.go")
lines = p.read_text().splitlines()
window = "\n".join(lines[177:224])
required = [
"err = oauthClient.OAuthAccessTokens().Delete",
'http.Error(w, "removing the session failed", http.StatusInternalServerError)',
"return",
'if refreshToken := ls.RefreshToken(); refreshToken != "" {',
"o.sessions.DeleteSession(w, r)",
]
positions = {text: window.find(text) for text in required}
print("logout control-flow positions:", positions)
print("access-delete failure returns before refresh cleanup:",
positions[required[1]] < positions[required[3]] and positions[required[2]] < positions[required[3]])
print("access-delete failure returns before local session deletion:",
positions[required[1]] < positions[required[4]] and positions[required[2]] < positions[required[4]])
PY
printf '\n-- session cookie deletion and refresh lookup --\n'
sed -n '88,108p' pkg/auth/sessions/combined_sessions.go
sed -n '219,250p' pkg/auth/sessions/combined_sessions.go
sed -n '252,274p' pkg/auth/oauth2/auth_openshift.goRepository: openshift/console
Length of output: 3274
Broken Authentication (CWE-613): Insufficient Session Expiration
Reachability: External
Clear the session when access-token revocation fails.
When OAuthAccessTokens().Delete returns an error, logout returns before refresh-token cleanup and o.sessions.DeleteSession. The refresh-token cookie and server-side session can then reach refreshSession. Run cleanup on this failure path and preserve the revocation error separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/oauth2/auth_openshift.go` around lines 216 - 221, Update logout’s
access-token revocation failure path to clear the session before returning:
preserve the revocation error separately, perform refresh-token cleanup and call
sessions.DeleteSession, then return the original error. Ensure cleanup also runs
when OAuthAccessTokens().Delete fails rather than exiting before the existing
logout cleanup flow.
There was a problem hiding this comment.
These seem like valid concerns, have you looked into them yet?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- auth OpenShift structure and logout-related tests ---'
ast-grep outline pkg/auth/oauth2/auth_openshift.go --items all --type function --match 'logout|DeleteSession|getLoginState|refreshSession'
ast-grep outline pkg/auth/oauth2/auth_openshift_test.go --items all --type function --match 'logout|Logout|DeleteSession|refresh'
printf '%s\n' '--- session store structure ---'
ast-grep outline pkg/auth/sessions/combined_sessions.go --items all --type function --match 'DeleteSession|GetSession|GetCookieRefreshToken|ClearRecoveryCookie'
printf '%s\n' '--- logout and deletion references with context ---'
rg -n -C 8 -P '\b(?:logout|DeleteSession|OAuthAccessTokens\(\)\.Delete|OAuthAuthorizeTokens\(\)\.Delete|GetCookieRefreshToken)\s*\(' \
pkg/auth/oauth2 pkg/auth/sessions \
-g '*.go'
printf '%s\n' '--- bounded implementation slices ---'
sed -n '180,245p' pkg/auth/oauth2/auth_openshift.go
sed -n '200,300p' pkg/auth/sessions/combined_sessions.goLength of output: 31672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate OpenShift authentication test files ---'
fd -t f -e go . pkg/auth/oauth2 | sort
printf '%s\n' '--- OpenShift auth construction and logout test references ---'
rg -n -C 10 -P '\b(?:newOpenShiftAuth|openShiftAuth|LogoutFunc|func\s+Test.*(?:OpenShift|Logout)|OAuthAuthorizeTokens|OAuthAccessTokens)\b' \
pkg/auth/oauth2 -g '*_test.go'
printf '%s\n' '--- test helpers that provide HTTP/API-client behavior ---'
rg -n -C 8 -P '\b(?:httptest\.NewServer|RoundTripper|Transport:|http\.Client|NewForConfig|issuerURL|testValidRefreshToken)\b' \
pkg/auth/oauth2 -g '*_test.go'
printf '%s\n' '--- complete local deletion and refresh-token lookup behavior ---'
sed -n '120,175p' pkg/auth/sessions/combined_sessions.go
sed -n '220,270p' pkg/auth/sessions/combined_sessions.goLength of output: 1403
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- auth test file structure ---'
ast-grep outline pkg/auth/oauth2/auth_test.go --items all --type function
printf '%s\n' '--- bounded auth test source ---'
sed -n '1,260p' pkg/auth/oauth2/auth_test.go
printf '%s\n' '--- repository-wide OpenShift logout and API client test references ---'
rg -n -C 8 -P '\b(?:newOpenShiftAuth|openShiftAuth|OAuthAuthorizeTokens|OAuthAccessTokens|LogoutFunc|logout)\b' \
-g '*_test.go' . || true
printf '%s\n' '--- HTTP transport fakes in authentication tests ---'
rg -n -C 8 -P '\b(?:RoundTripper|Transport|httptest\.NewServer|http\.Client|NewForConfig)\b' \
pkg/auth -g '*_test.go' || true
printf '%s\n' '--- relevant auth construction ---'
sed -n '240,340p' pkg/auth/oauth2/auth.goLength of output: 37782
@TheRealJon, yes. I reviewed the implementation, the session-store behavior, and the existing tests.
Both findings identify valid defects:
-
If
OAuthAccessTokens().Deletefails,logoutreturns before it callsDeleteSessionandClearRecoveryCookie. The encrypted refresh-token cookie remains available.getLoginStatecan use it to refresh or restore the session. -
If
OAuthAuthorizeTokens().Deletefails, the remote refresh token can remain valid. The current handler logs the error and returns204.
The first finding needs one correction. On a refresh-token revocation failure, the handler must not return before local cleanup. It must expire the local session, refresh-token cookie, and recovery cookie first. It should then return an error response to report that remote revocation failed.
The robust flow is:
- Attempt both remote revocations.
- Record any revocation error.
- Always clear local session state and cookies.
- Return
204only if both remote revocations succeed. - Return
500after local cleanup if either remote revocation fails.
There is no existing OpenShift logout test. pkg/auth/oauth2/auth_test.go only covers authenticator construction. The change should add failure-path tests that assert both the error response and local cookie/session cleanup.
You are interacting with an AI system.
Two test cases: - Session survives console pod deletion and replacement - Session survives plugin toggle (operator-triggered rollout) Both tests log in (kubeadmin by default, htpasswd when env vars are set), trigger a console pod restart, and assert the user is still authenticated without a login redirect. Tagged @slow — pod rollout takes 30-60s per test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/e2e/tests/console/session-persistence.spec.ts (1)
33-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse Playwright
baseURL-resolved routes.Full URLs bypass Playwright route resolution and treat
WEB_CONSOLE_URLas a route prefix. Use leading-slash routes.
frontend/e2e/tests/console/session-persistence.spec.ts#L33-L33: usepage.goto('/dashboards').frontend/e2e/tests/console/session-persistence.spec.ts#L55-L58: usepage.goto('/k8s/cluster/nodes').frontend/e2e/tests/console/session-persistence.spec.ts#L128-L131: usepage.goto('/dashboards').Based on learnings: use
page.goto('/k8s/...')with a leading-slash absolute path in console E2E specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/console/session-persistence.spec.ts` at line 33, Update all three page.goto calls in frontend/e2e/tests/console/session-persistence.spec.ts at lines 33-33, 55-58, and 128-131 to use leading-slash Playwright routes: /dashboards, /k8s/cluster/nodes, and /dashboards respectively, removing baseURL interpolation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/tests/console/session-persistence.spec.ts`:
- Line 14: Update the baseURL initialization in the session persistence test to
remove the page.url() fallback; use WEB_CONSOLE_URL when set, otherwise default
directly to http://localhost:9000 before passing it to performLogin.
- Around line 50-52: Update the console session-persistence test in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 50-52 and
120-124: before deleting console pods, record their UIDs and require Ready
replacement pods with different UIDs; before applying the plugin patch, record
metadata.generation and wait for it to increase before calling
waitForDeploymentReady. Ensure readiness checks cannot succeed against stale
pods or an unchanged deployment.
---
Nitpick comments:
In `@frontend/e2e/tests/console/session-persistence.spec.ts`:
- Line 33: Update all three page.goto calls in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 33-33, 55-58,
and 128-131 to use leading-slash Playwright routes: /dashboards,
/k8s/cluster/nodes, and /dashboards respectively, removing baseURL
interpolation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b35579f3-c0f2-4253-b387-ebc49baf1e8a
📒 Files selected for processing (1)
frontend/e2e/tests/console/session-persistence.spec.ts
| await test.step('Wait for new console pods to be ready', async () => { | ||
| await k8sClient.waitForDeploymentReady(CONSOLE_DEPLOYMENT, CONSOLE_NAMESPACE, 180_000); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the readiness helper and determine whether it checks a target generation
# or newly created pods rather than only current Deployment readiness.
rg -n -C 8 'waitForDeploymentReady|waitFor.*Pod|metadata\.generation|updatedReplicas' frontend/e2eRepository: openshift/console
Length of output: 15116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session-persistence.spec.ts ---'
sed -n '1,145p' frontend/e2e/tests/console/session-persistence.spec.ts
printf '%s\n' '--- waitForDeploymentReady and related client methods ---'
sed -n '580,750p' frontend/e2e/clients/kubernetes-client.ts
printf '%s\n' '--- console pod discovery and deployment patch call sites ---'
rg -n -C 12 'CONSOLE_DEPLOYMENT|CONSOLE_NAMESPACE|list.*Pod|readNamespacedPod|patchNamespacedCustomObject|plugins' frontend/e2e/tests/console/session-persistence.spec.ts frontend/e2e/clients/kubernetes-client.tsRepository: openshift/console
Length of output: 30341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class DeploymentObservation:
desired: int
available: int
updated: int
available_condition: bool
generation: int
pod_uids: tuple[str, ...]
def wait_for_deployment_ready(d: DeploymentObservation) -> bool:
return (
d.available == d.desired
and d.updated == d.desired
and d.available_condition
)
old = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
# A stale observation can pass immediately after deletion is accepted.
stale_after_delete = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
assert wait_for_deployment_ready(stale_after_delete)
# A plugin change can leave the Deployment ready without a new generation.
no_rollout_after_plugin_patch = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
assert wait_for_deployment_ready(no_rollout_after_plugin_patch)
assert no_rollout_after_plugin_patch.generation == old.generation
# The intended checks distinguish both cases.
replacement = DeploymentObservation(2, 2, 2, True, 7, ("new-a", "new-b"))
assert not set(replacement.pod_uids) & set(old.pod_uids)
rollout = DeploymentObservation(2, 2, 2, True, 8, ("new-a", "new-b"))
assert rollout.generation > old.generation
print("waitForDeploymentReady ignores generation and pod UIDs")
print("stale readiness and no-rollout readiness both satisfy its predicate")
PYRepository: openshift/console
Length of output: 278
Gate readiness on the requested disruption.
- Before deleting console pods, record their UIDs and wait for Ready replacement pods with different UIDs.
- Before the plugin patch, record
metadata.generationand wait for it to increase before callingwaitForDeploymentReady. Otherwise, stale readiness can pass without a rollout.
📍 Affects 1 file
frontend/e2e/tests/console/session-persistence.spec.ts#L50-L52(this comment)frontend/e2e/tests/console/session-persistence.spec.ts#L120-L124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/session-persistence.spec.ts` around lines 50 - 52,
Update the console session-persistence test in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 50-52 and
120-124: before deleting console pods, record their UIDs and require Ready
replacement pods with different UIDs; before applying the plugin patch, record
metadata.generation and wait for it to increase before calling
waitForDeploymentReady. Ensure readiness checks cannot succeed against stale
pods or an unchanged deployment.
OpenShift's internal OAuth server does not support refresh tokens, so the refresh-token-in-cookie approach only works for OIDC auth. For OpenShift auth, store the encrypted access token + expiry in a separate recovery cookie (openshift-recovery-token). After pod restart, the backend reads the token from the cookie, validates expiry locally, and creates a new server-side session transparently. No OAuth server interaction or page redirect needed. - SetRecoveryCookie/GetRecoveryCookie/ClearRecoveryCookie methods - recoverSession() called in getLoginState() when session is nil - Recovery cookie cleared on logout and DeleteSession - MaxAge set to match token expiry - Unit tests for recovery cookie lifecycle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/sessions/combined_sessions_test.go`:
- Around line 310-356: Update the recovery-cookie tests around the request
constructions in the relevant subtests to capture the error returned by each
http.NewRequest call and immediately assert it with require.NoError(t, err).
Apply this to every affected request, including req and req2 in the set/get,
empty-request, clear-cookie, and expired-token cases, without changing the test
behavior.
In `@pkg/auth/sessions/combined_sessions.go`:
- Around line 263-285: Handle errors returned by CookieStore.Get in both
SetRecoveryCookie and GetRecoveryCookie. In SetRecoveryCookie, return a wrapped
decode error or initialize a fresh session before mutating and saving; in
GetRecoveryCookie, return the existing empty values with false when decoding
fails, before accessing session values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f56d416b-5576-4e6b-8775-88ccab4263b1
📒 Files selected for processing (4)
pkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.gopkg/auth/sessions/combined_sessions_test.gopkg/auth/sessions/server_session.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/sessions/server_session.go
| func (cs *CombinedSessionStore) SetRecoveryCookie(w http.ResponseWriter, r *http.Request, accessToken string, expiry time.Time) error { | ||
| s, _ := cs.clientStore.Get(r, openshiftRecoveryTokenCookieName) | ||
| s.Values["access-token"] = accessToken | ||
| s.Values["expiry"] = expiry.Unix() | ||
| maxAge := int(time.Until(expiry).Seconds()) | ||
| if maxAge > 0 { | ||
| s.Options.MaxAge = maxAge | ||
| } | ||
| return s.Save(r, w) | ||
| } | ||
|
|
||
| func (cs *CombinedSessionStore) GetRecoveryCookie(r *http.Request) (string, time.Time, bool) { | ||
| s, _ := cs.clientStore.Get(r, openshiftRecoveryTokenCookieName) | ||
| accessToken, ok := s.Values["access-token"].(string) | ||
| if !ok || accessToken == "" { | ||
| return "", time.Time{}, false | ||
| } | ||
| expiryUnix, ok := s.Values["expiry"].(int64) | ||
| if !ok { | ||
| return "", time.Time{}, false | ||
| } | ||
| return accessToken, time.Unix(expiryUnix, 0), true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For github.com/gorilla/sessions v1.4.0, what does CookieStore.Get return when a cookie cannot be decoded or verified?
💡 Result:
For gorilla/sessions v1.4.0, when a cookie cannot be decoded or verified, CookieStore.Get returns a new session object along with a non-nil error [1][2]. Specifically: - The session object returned is a fresh session (i.e., its IsNew field will be true) [1][2]. - The error returned indicates the reason the existing cookie could not be successfully decoded or verified (such as a signature mismatch or expired/invalid data) [3][4]. Because Get always returns a session, even if it cannot decode the provided cookie, it is common practice in many applications to ignore the error returned by Get if you simply intend to proceed with a blank or new session when the existing one is invalid [5][6]. Calling session.Save on this returned session will effectively overwrite the invalid cookie with a new, valid one in the user's browser [6].
Citations:
- 1: https://github.com/gorilla/sessions/blob/v1.4.0/store.go
- 2: https://github.com/gorilla/sessions/blob/main/store.go
- 3: How to handle err from store.Get() gorilla/sessions#179
- 4: Changed auth/encryption key prevents the signed in user from accessing the webpage again gorilla/sessions#249
- 5: https://pkg.go.dev/github.com/gorilla/sessions
- 6: securecookie: the value is not valid gorilla/sessions#16
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '230,310p' pkg/auth/sessions/combined_sessions.go
printf '%s\n' '--- call sites ---'
rg -n -C 3 'GetRecoveryCookie|SetRecoveryCookie' .
printf '%s\n' '--- dependency declarations ---'
rg -n -C 2 'gorilla/sessions|sessions v' go.mod go.sum vendor 2>/dev/null || true
printf '%s\n' '--- CookieStore.Get implementation references ---'
rg -n -C 8 'func \(.*CookieStore.*\) Get|type CookieStore|Decode' $(go env GOPATH 2>/dev/null)/pkg/mod/github.com/gorilla/sessions* 2>/dev/null || trueRepository: openshift/console
Length of output: 21836
🌐 Web query:
github.com/gorilla/sessions v1.4.0 CookieStore.Get source store.go securecookie.Decode error behavior
💡 Result:
In github.com/gorilla/sessions v1.4.0, the CookieStore.Get method behaves as a convenience wrapper around Registry.Get [1], which in turn calls CookieStore.New if a session does not already exist in the registry [1][2]. When CookieStore.New encounters an issue decoding an existing cookie (e.g., due to invalid keys, tampered data, or expired timestamps handled by the securecookie package), it returns a new, empty session along with the error [1]. Key aspects of this behavior include: 1. Always Returns a Session: CookieStore.Get (via New) is designed to always return a *Session object, even if an error occurs during the decoding process [1][3]. This ensures that application logic can proceed with a fresh session if the previous one is unreadable [4]. 2. Error Indication: The error returned indicates that while a cookie was present, it could not be successfully decoded using the configured codecs [1]. 3. Recommended Handling: The official gorilla/sessions documentation and community guidance suggest that while the error can be checked, Get() often returns a valid (though empty or new) session object regardless of the error [3][5]. Developers often choose to ignore the error returned by Get() if they intend to treat a failed decryption as a session reset (i.e., treating the user as unauthenticated) [5]. 4. Underlying Mechanism: The securecookie.Decode method (called within CookieStore.New) performs several validation steps, including checking the length, verifying the MAC (message authentication code), validating timestamps (if configured), and decrypting the payload [6]. Any failure in these steps results in an error [6], which CookieStore bubbles up [1]. Common scenarios where securecookie.Decode returns an error include key rotation mismatches, modified or malformed cookie values, and expired sessions [4][6][7]. Since CookieStore continues by returning a new *Session with IsNew set to true, the application effectively invalidates the unreadable session and starts a new one [1].
Citations:
- 1: https://github.com/gorilla/sessions/blob/v1.4.0/store.go
- 2: https://github.com/gorilla/sessions/blob/v1.4.0/sessions.go
- 3: https://github.com/gorilla/sessions/blob/v1.4.0/doc.go
- 4: Changed auth/encryption key prevents the signed in user from accessing the webpage again gorilla/sessions#249
- 5: https://github.com/gorilla/sessions/blob/main/README.md
- 6: https://github.com/gorilla/securecookie/blob/main/securecookie.go
- 7: How to handle err from store.Get() gorilla/sessions#179
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session store construction and related error handling ---'
rg -n -C 5 'clientStore|NewCookieStore|CookieStore|CombinedSessionStore' pkg/auth/sessions pkg/auth/oauth2 | head -240
printf '%s\n' '--- recovery-cookie tests ---'
sed -n '280,375p' pkg/auth/sessions/combined_sessions_test.go
printf '%s\n' '--- vendor registry behavior ---'
sed -n '70,125p' vendor/github.com/gorilla/sessions/store.go
sed -n '1,115p' vendor/github.com/gorilla/sessions/registry.go 2>/dev/null || true
printf '%s\n' '--- standalone source-level behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path("vendor/github.com/gorilla/sessions/store.go")
text = p.read_text()
start = text.index("func (s *CookieStore) New")
end = text.index("\n}", start) + 2
snippet = text[start:end]
assert "return session, err" in snippet
assert "session.IsNew = true" in snippet
assert "if err == nil" in snippet and "session.IsNew = false" in snippet
print("CookieStore.New returns the allocated session with the decode error and keeps IsNew=true on decode failure.")
print("CookieStore.Get delegates to the registry, which returns that result.")
PYRepository: openshift/console
Length of output: 22748
Handle recovery-cookie decode errors.
CookieStore.Get returns a new session and a non-nil error when decoding fails. Handle this error at both call sites. Return a wrapped error from SetRecoveryCookie, or explicitly create a fresh session before saving. Return false from GetRecoveryCookie when decoding fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/sessions/combined_sessions.go` around lines 263 - 285, Handle errors
returned by CookieStore.Get in both SetRecoveryCookie and GetRecoveryCookie. In
SetRecoveryCookie, return a wrapped decode error or initialize a fresh session
before mutating and saving; in GetRecoveryCookie, return the existing empty
values with false when decoding fails, before accessing session values.
Source: Path instructions
jhadvig
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
- gofmt formatting on 4 Go files - Replace page.waitForTimeout with pod deletion in e2e test to satisfy playwright/no-wait-for-timeout ESLint rule Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
During initial cluster install, the console config may reference session key file paths before the session-secret Secret is created and mounted. For OpenShift auth, treat missing key files as non-fatal (log warning, fall back to random keys) instead of crashing the console process. OIDC auth retains the fatal error since key files are required for that mode. This fixes the bootstrap timing issue where the console deployment fails with ProgressDeadlineExceeded because pods crash before the operator creates the session-secret. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test ResultsAll scenarios verified on a live cluster (GCP) with both console and console-operator PRs deployed together.
Test methodology
Companion operator PR: openshift/console-operator#1204 /verified by @jhadvig |
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
Just following up on the two major coderabbit concerns. Other than that, lgtm.
| if refreshToken := ls.RefreshToken(); refreshToken != "" { | ||
| if delErr := oauthClient.OAuthAuthorizeTokens().Delete(ctx, tokenToObjectName(refreshToken), metav1.DeleteOptions{}); delErr != nil { | ||
| klog.V(4).Infof("failed to revoke refresh token on logout: %v", delErr) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
These seem like valid concerns, have you looked into them yet?
- Move local session cleanup to defer in logout() so cookies are always cleared even when remote token revocation fails (CWE-613) - Remote revocations are now best-effort; logout always returns 204 - Add positive test case for legacy cookie resolution via byRefreshToken - Remove page.url() fallback from e2e test baseURL (returns about:blank) - Add require.NoError to http.NewRequest calls in recovery cookie tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
|
@TheRealJon addressed both CodeRabbit concerns: Concern 1 (access token revocation failure leaves cookies behind) — Valid, fixed. Moved local cleanup ( Concern 2 (refresh token revocation failure) — Not a real issue for OpenShift auth. The internal OAuth server doesn't support Also addressed the three minor nits (legacy cookie positive test, |
Accept optional previous key pair files for cookie decryption. gorilla/securecookie tries each key pair in order — new cookies use the current keys, old cookies are still readable with the previous keys until they expire. - NewSessionStore accepts variadic previousKeyPairs - SessionOptions reads previous key file paths from config - Previous keys are always optional and non-fatal if missing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/test backend |
|
/retest |
|
/test backend |
1 similar comment
|
/test backend |
|
@jhadvig: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Analysis / Root cause:
Console sessions are stored entirely in per-pod process memory. When a pod restarts (upgrades, scaling, OOM, operator reconciliation), all sessions are lost and users must re-authenticate. This affects all users but is most disruptive for clusters using external IdPs (htpasswd, LDAP) where re-authentication requires full credential entry.
Two architectural limitations cause this:
Jira: https://redhat.atlassian.net/browse/OCPBUGS-71237
Related: https://redhat.atlassian.net/browse/OCPBUGS-58468
Companion operator PR: openshift/console-operator#1204
Solution description:
Phase 1: Shared encryption keys
Accept file-based encryption keys (provided by the console-operator via
session-secretSecret) instead of generating random keys per process. Falls back to random generation when no key files are provided (backward compat with older operators).Phase 2: Cookie-based session recovery
For OIDC auth (refresh tokens available):
For OpenShift auth (no refresh tokens — OAuth server doesn't support
refresh_tokengrant):openshift-recovery-tokencookie stores encrypted access token + expiryrecoverSession()reads the token, validates expiry locally, creates a new server-side sessionSecurity hardening
refreshTokenIDfield removedScreenshots / screen recording:
Test setup:
Deploy cluster with htpasswd IdP, login, restart console pods, verify session persists.
Test cases:
Browser conformance:
Additional info:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes