OCPBUGS-85485: Append id_token_hint to OIDC logout redirect URL#16548
OCPBUGS-85485: Append id_token_hint to OIDC logout redirect URL#16548Cragsmann wants to merge 4 commits into
Conversation
When Keycloak is the IDP and logoutRedirect is configured, logging out with multiple tabs open causes Keycloak to show a confirmation page (because it cannot identify the session without id_token_hint). While the user sits on that page, another tab can re-authenticate via SSO, corrupting the session state and causing "Logout failed." Construct the logout redirect URL dynamically at logout time by retrieving the id_token from the active session and appending it as the id_token_hint query parameter, per the OIDC RP-Initiated Logout spec. The backend now returns the URL as JSON so the frontend can redirect to it. Falls back to the existing static redirect when no session or no logout URL is configured.
openshift-ci-robot
added
jira/severity-moderate
|
@Cragsmann: This pull request references Jira Issue OCPBUGS-85485, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBackend may return JSON ChangesDynamic OIDC Logout + OAuth2 state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
component/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_oidc_test.go`:
- Around line 791-797: The test currently ignores url.Parse errors when parsing
tt.logoutRedirectOverride into expectedBase; change the code in the test where
expectedBase is set (the url.Parse call for tt.logoutRedirectOverride in
pkg/auth/oauth2/auth_oidc_test.go) to check the returned error and fail fast
(e.g., require.NoError/require.Nil or t.Fatalf) if parsing fails, so malformed
table entries don't produce silent incorrect assertions; then continue using
expectedBase.Host/Path and expectedBase.Query() as before.
In `@pkg/auth/oauth2/auth_oidc.go`:
- Around line 149-157: The logout helper currently calls getLoginState which may
trigger refreshSession and hit the IdP; change it to use a non-refreshing
session lookup (e.g., a new or existing helper like getCachedLoginState /
lookupSessionNoRefresh or directly query the session store) to read the cached
session into ls without invoking refreshSession, then obtain idToken :=
ls.AccessToken() and fall back to returning baseURL if no cached id token
exists; ensure you remove any code path that can call refreshSession from this
logout helper so logout never depends on the token endpoint.
- Around line 132-136: The response writer's JSON encoding call
json.NewEncoder(w).Encode(...) currently ignores errors; update the OIDC logout
handler (the block that checks logoutURL and writes the JSON to w) to capture
the error returned by Encode, log it with contextual info (including logoutURL)
and, if the write failed before any body was sent, return an HTTP error status
(e.g., http.Error with 500) or at least ensure the client sees a failure; do not
swallow write errors so disconnects or write failures are surfaced and logged.
🪄 Autofix (Beta)
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: ca47103b-6a90-4588-a6b7-bdd7b679c1d3
📒 Files selected for processing (3)
frontend/public/module/auth.tspkg/auth/oauth2/auth_oidc.gopkg/auth/oauth2/auth_oidc_test.go
- Remove refresh token from error message to avoid logging sensitive credentials (pre-existing issue on line 108) - Use non-refreshing session lookup (GetSession) instead of getLoginState in logoutRedirectURLWithIDTokenHint to avoid hitting the IdP token endpoint during logout - Handle json.Encode error in logout response - Check url.Parse error in test assertions - Add docstrings for logout and logoutRedirectURLWithIDTokenHint
| return baseURL | ||
| } | ||
|
|
||
| idToken := ls.AccessToken() |
There was a problem hiding this comment.
I might be a little confused here - but isn't the access_token and id_token different? Can id_token_hint param accept access_token if it was used to authenticate?
My understanding was id_token_hint needs to take id_token . Since id_token needs to use OIDC flow would this work in OAuth2 ? I believe the oauth-server or osin when making the request gets responseData which would contain id_token but I believe id_token is never specified as something stored to use in the same way access_token is.
I'm asking because I'm curious as to how console works. I was looking at this work and thought of using id_token_hint but ran across this issue. :)
There was a problem hiding this comment.
Great question! The method name AccessToken() is indeed misleading here. It returns ls.rawToken, and what rawToken contains depends on the auth path:
OIDC path (this fix's scope): When tokenVerifier != nil, rawToken is set to token.Extra("id_token").(string) — the raw id_token JWT from the OIDC provider (loginstate.go L75-88). So AccessToken() actually returns the id_token in this path, which is exactly what id_token_hint needs.
OpenShift OAuth path: When tokenVerifier == nil, rawToken is set to token.AccessToken — the OCP OAuth bearer token (e.g. sha256~...) (loginstate.go L64-72). This is NOT an id_token and can't be used as id_token_hint. But this fix only runs in the oidcAuth.logout() handler, which is never called in the OpenShift auth path — openShiftAuth.logout() has its own separate implementation.
So to directly answer your question: this won't work in pure OAuth2 (OpenShift auth mode) because the id_token isn't stored there. It only works in the OIDC auth path where the console talks directly to an OIDC provider like Keycloak.
There was a problem hiding this comment.
Thanks for getting back to me so quickly. That makes perfect sense. Thanks for the explanation!
|
/retest |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
Could we use a 302 response with a Location header to automatically redirect the browser to the logout URL?
|
@Cragsmann I'm trying to verify by deploying your changes on a cluster and setting up external oidc with keycloak, but I wasn't able to get an env up today. IIRC, the last time I did this I had some help from either QE or someone from the auth team. I feel like we should test this at least once with a real external OIDC setup before merging. |
…re-auth Use a unique cookie per login flow (login-state-<prefix>) instead of a single shared cookie. This prevents concurrent login flows from multiple browser tabs from overwriting each other's CSRF state, which caused invalid_state errors when re-authenticating after logout. Also wraps the frontend handle401 handler in _.once() so only the first 401 per page load triggers the re-auth flow, preventing the redirect counter from being exhausted by concurrent API calls.
|
@TheRealJon regarding redirect to the logout URL. The logout is triggered by a Re: testing on a real cluster — I've been testing this on an OCP cluster with Keycloak as the IDP (external OIDC). The multi-tab logout flow works cleanly now: multiple tabs re-authenticate without any auth errors after logout. Please review my changes and check for possible side effects to the auth flow. So far what I have tested worked fine. Screen.Recording.2026-06-05.at.17.42.55.mov |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cragsmann The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
@Cragsmann: The following tests 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:
When Keycloak is used as the IDP and
logoutRedirectis configured, logout fails when multipletabs are open. The console redirects to Keycloak's
end_session_endpointwithout anid_token_hint,so Keycloak cannot identify the session and shows a "Do you want to log out?" confirmation page.
While the user sits on that page, another tab re-authenticates via SSO (the Keycloak session is
still active), corrupting the session state. When the user finally confirms, Keycloak returns
"Logout failed."
Additionally, when multiple tabs re-authenticate after logout, they race on the shared
login-statecookie — each tab's login flow overwrites the other's CSRF state, causing
invalid_stateerrors.JIRA: https://redhat.atlassian.net/browse/OCPBUGS-85485
Solution description:
Per the OIDC RP-Initiated Logout spec,
passing
id_token_hintlets Keycloak identify the session and skip the confirmation page.Backend (
pkg/auth/oauth2/auth_oidc.go):sessions.GetSession()/AccessToken()id_token_hint,client_id, andpost_logout_redirect_urito the logout redirect URL{"logoutRedirectURL": "..."}) with status 200Backend (
pkg/auth/oauth2/auth.go):login-state-<prefix>) instead of a single sharedlogin-statecookiestate race condition that caused
invalid_stateerrorslogin-statecookie for rolling-update compatibilityConsoleBaseAddressto Config forpost_logout_redirect_urisupportFrontend (
frontend/public/module/auth.ts):POST /api/console/logoutlogoutRedirectURLis present, redirect to it (containsid_token_hint)logoutRedirectfrom SERVER_FLAGS (backward compatible)handle401in_.once()so only the first 401 per page load triggers the re-auth flow,preventing the redirect counter from being exhausted by concurrent API calls
Screenshots / screen recording:
Test setup:
logoutRedirectpointing to Keycloak's OIDC logout endpointTest cases:
logout()andlogoutRedirectURLWithIDTokenHint()covering:Browser conformance:
Summary by CodeRabbit
New Features
Bug Fixes
Tests