diff --git a/cmd/bridge/config/auth/authoptions.go b/cmd/bridge/config/auth/authoptions.go index 16e182bc7c8..2b681880f93 100644 --- a/cmd/bridge/config/auth/authoptions.go +++ b/cmd/bridge/config/auth/authoptions.go @@ -298,14 +298,15 @@ func (c *completedOptions) getAuthenticator( // Config for logging into console. oidcClientConfig := &oauth2.Config{ - AuthSource: authSource, - IssuerURL: userAuthOIDCIssuerURL.String(), - IssuerCA: c.CAFilePath, - ClientID: c.ClientID, - ClientSecret: oidcClientSecret, - RedirectURL: proxy.SingleJoiningSlash(baseURL.String(), server.AuthLoginCallbackEndpoint), - Scope: scopes, - OCLoginCommand: c.OCLoginCommand, + AuthSource: authSource, + IssuerURL: userAuthOIDCIssuerURL.String(), + IssuerCA: c.CAFilePath, + ClientID: c.ClientID, + ClientSecret: oidcClientSecret, + RedirectURL: proxy.SingleJoiningSlash(baseURL.String(), server.AuthLoginCallbackEndpoint), + ConsoleBaseAddress: baseURL.String(), + Scope: scopes, + OCLoginCommand: c.OCLoginCommand, // Use the k8s CA file for OpenShift OAuth metadata discovery. // This might be different than IssuerCA. diff --git a/frontend/public/module/auth.ts b/frontend/public/module/auth.ts index 325bdaaacb3..32a4cd74342 100644 --- a/frontend/public/module/auth.ts +++ b/frontend/public/module/auth.ts @@ -122,14 +122,25 @@ export const authSvc = { setNext(next); clearLocalStorage(clearLocalStorageKeys); coFetch(logoutURL, { method: 'POST' }) - // eslint-disable-next-line no-console - .catch((e) => console.error('Error logging out', e)) - .then(() => { + .then(async (response) => { + let dynamicLogoutURL: string | undefined; + try { + const data = await response.json(); + dynamicLogoutURL = data?.logoutRedirectURL; + } catch { + // Response may be empty (204) for non-OIDC auth — ignore + } + if (isKubeAdmin) { authSvc.logoutKubeAdmin(); + } else if (dynamicLogoutURL) { + window.location.assign(dynamicLogoutURL); } else { authSvc.logoutRedirect(next); } + }) + .catch(() => { + authSvc.logoutRedirect(next); }); }), @@ -164,8 +175,11 @@ export const authSvc = { } }, - // Handle 401 responses with redirect loop detection - handle401: (next) => { + // Handle 401 responses with redirect loop detection. + // Wrapped in _.once so that only the first 401 per page load triggers the + // flow — the SPA fires many API calls concurrently and each would otherwise + // increment the redirect counter past MAX_AUTH_REDIRECTS within a single load. + handle401: _.once((next) => { const redirectCount = incrementAuthRedirectCount(); // If we've exceeded the max redirects, redirect to the error page @@ -190,7 +204,7 @@ export const authSvc = { // Proceed with normal logout flow authSvc.logout(next); - }, + }), // Reset redirect counter (called on successful k8s requests) resetRedirectCount: () => { diff --git a/pkg/auth/oauth2/auth.go b/pkg/auth/oauth2/auth.go index c644952c36e..9294cbee155 100644 --- a/pkg/auth/oauth2/auth.go +++ b/pkg/auth/oauth2/auth.go @@ -28,6 +28,7 @@ import ( const ( stateCookieName = "login-state" errorOAuth = "oauth_error" + stateLength = 32 // hex-encoded 16 random bytes errorLoginState = "login_state_error" errorCookie = "cookie_error" errorInternal = "internal_error" @@ -107,6 +108,7 @@ type Config struct { LogoutRedirectOverride string // overrides the OIDC provider's front-channel logout URL IssuerCA string RedirectURL string + ConsoleBaseAddress string // used as post_logout_redirect_uri in OIDC RP-Initiated Logout ClientID string ClientSecret string Scope []string @@ -200,6 +202,7 @@ func NewOAuth2Authenticator(ctx context.Context, config *Config) (*OAuth2Authent issuerURL: c.IssuerURL, logoutRedirectOverride: c.LogoutRedirectOverride, clientID: c.ClientID, + consoleBaseAddress: c.ConsoleBaseAddress, cookiePath: c.CookiePath, secureCookies: c.SecureCookies, constructOAuth2Config: a.oauth2ConfigConstructor, @@ -287,10 +290,12 @@ func (a *OAuth2Authenticator) LoginFunc(w http.ResponseWriter, r *http.Request) state := hex.EncodeToString(randData[:]) cookie := http.Cookie{ - Name: stateCookieName, + Name: stateCookieName + "-" + state[:8], Value: state, HttpOnly: true, Secure: a.secureCookies, + Path: "/auth", + MaxAge: 300, } http.SetCookie(w, &cookie) http.Redirect(w, r, a.oauth2Config().AuthCodeURL(state), http.StatusSeeOther) @@ -321,11 +326,20 @@ func (a *OAuth2Authenticator) CallbackFunc(fn func(loginInfo sessions.LoginJSON, return } - cookieState, err := r.Cookie(stateCookieName) - if err != nil { - klog.Errorf("failed to parse state cookie: %v", err) - a.redirectAuthError(w, errorMissingState) - return + // Look up the per-state cookie for this login flow. + var cookieState *http.Cookie + if len(urlState) == stateLength { + cookieState, _ = r.Cookie(stateCookieName + "-" + urlState[:8]) + } + if cookieState == nil { + // Fall back to legacy single cookie for rolling-update compat. + var cookieErr error + cookieState, cookieErr = r.Cookie(stateCookieName) + if cookieErr != nil { + klog.Errorf("failed to parse state cookie: %v", cookieErr) + a.redirectAuthError(w, errorMissingState) + return + } } // Lack of both `error` and `code` indicates some other redirect with no params. @@ -341,10 +355,12 @@ func (a *OAuth2Authenticator) CallbackFunc(fn func(loginInfo sessions.LoginJSON, } if urlState != cookieState.Value { - klog.Error("state in url does not match State cookie") + klog.Errorf("state in url does not match state cookie %s", cookieState.Name) a.redirectAuthError(w, errorInvalidState) return } + http.SetCookie(w, &http.Cookie{Name: cookieState.Name, Path: "/auth", MaxAge: -1}) + ctx := oidc.ClientContext(r.Context(), a.clientFunc()) oauthConfig := a.oauth2Config() token, err := oauthConfig.Exchange(ctx, code) diff --git a/pkg/auth/oauth2/auth_oidc.go b/pkg/auth/oauth2/auth_oidc.go index a580caf8bd2..75f0211e1bd 100644 --- a/pkg/auth/oauth2/auth_oidc.go +++ b/pkg/auth/oauth2/auth_oidc.go @@ -2,13 +2,16 @@ package oauth2 import ( "context" + "encoding/json" "fmt" "net/http" + "net/url" "sync" "time" oidc "github.com/coreos/go-oidc" "golang.org/x/oauth2" + "k8s.io/klog/v2" "github.com/openshift/console/pkg/auth" "github.com/openshift/console/pkg/auth/sessions" @@ -35,6 +38,7 @@ type oidcConfig struct { issuerURL string logoutRedirectOverride string clientID string + consoleBaseAddress string cookiePath string secureCookies bool constructOAuth2Config oauth2ConfigConstructor @@ -102,7 +106,7 @@ func (o *oidcAuth) refreshSession(ctx context.Context, w http.ResponseWriter, r &oauth2.Token{RefreshToken: cookieRefreshToken}, ).Token() if err != nil { - return nil, fmt.Errorf("failed to refresh a token %s: %w", cookieRefreshToken, err) + return nil, fmt.Errorf("failed to refresh a token: %w", err) } ls, err := o.sessions.UpdateTokens(w, r, o.verify, newTokens) @@ -122,11 +126,66 @@ func (o *oidcAuth) DeleteSession(w http.ResponseWriter, r *http.Request) { o.sessions.DeleteSession(w, r) } +// logout handles the OIDC logout flow by constructing a logout redirect URL +// with an id_token_hint appended (when available) and returning it as JSON. func (o *oidcAuth) logout(w http.ResponseWriter, r *http.Request) { + logoutURL := o.logoutRedirectURLWithIDTokenHint(w, r) o.DeleteSession(w, r) + + if logoutURL != "" { + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(map[string]string{ + "logoutRedirectURL": logoutURL, + }); err != nil { + klog.Errorf("failed to write logout redirect response: %v", err) + } + return + } + w.WriteHeader(http.StatusNoContent) } +// logoutRedirectURLWithIDTokenHint builds the logout redirect URL with the +// id_token_hint query parameter appended, per the OIDC RP-Initiated Logout spec. +// Returns empty when the session or id_token is unavailable — the caller should +// fall back to 204 so the frontend uses the static logout redirect instead of +// navigating to the IdP logout endpoint without a hint (which would cause a +// redirect loop via post_logout_redirect_uri). +func (o *oidcAuth) logoutRedirectURLWithIDTokenHint(w http.ResponseWriter, r *http.Request) string { + baseURL := o.LogoutRedirectURL() + if baseURL == "" { + return "" + } + + ls, err := o.sessions.GetSession(w, r) + if err != nil || ls == nil { + klog.V(4).Infof("could not retrieve session for id_token_hint: %v", err) + return "" + } + + idToken := ls.AccessToken() + if idToken == "" { + return "" + } + + parsed, err := url.Parse(baseURL) + if err != nil { + klog.Errorf("failed to parse logout redirect URL: %v", err) + return "" + } + + q := parsed.Query() + q.Set("id_token_hint", idToken) + if !q.Has("client_id") && o.clientID != "" { + q.Set("client_id", o.clientID) + } + if !q.Has("post_logout_redirect_uri") && o.consoleBaseAddress != "" { + q.Set("post_logout_redirect_uri", o.consoleBaseAddress) + } + parsed.RawQuery = q.Encode() + return parsed.String() +} + func (o *oidcAuth) getLoginState(w http.ResponseWriter, r *http.Request) (*sessions.LoginState, error) { ls, err := o.sessions.GetSession(w, r) if err != nil { diff --git a/pkg/auth/oauth2/auth_oidc_test.go b/pkg/auth/oauth2/auth_oidc_test.go index 7a82f87044d..f635ed0fe79 100644 --- a/pkg/auth/oauth2/auth_oidc_test.go +++ b/pkg/auth/oauth2/auth_oidc_test.go @@ -568,6 +568,276 @@ func Test_oidcAuth_getLoginState(t *testing.T) { } } +func Test_oidcAuth_logout(t *testing.T) { + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + + oidcProvider, providerURL, closePort := startMockProvider(t) + defer closePort() + + tests := []struct { + name string + logoutRedirectOverride string + initSession bool + wantStatus int + wantIDTokenHint bool + wantJSON bool + }{ + { + name: "with session and logout redirect, returns JSON with id_token_hint", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=console", + initSession: true, + wantStatus: http.StatusOK, + wantIDTokenHint: true, + wantJSON: true, + }, + { + name: "no logout redirect configured, returns 204", + logoutRedirectOverride: "", + initSession: true, + wantStatus: http.StatusNoContent, + wantIDTokenHint: false, + wantJSON: false, + }, + { + name: "no session, returns 204", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=console", + initSession: false, + wantStatus: http.StatusNoContent, + wantIDTokenHint: false, + wantJSON: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o, err := newOIDCAuth( + context.Background(), + sessions.NewSessionStore(authnKey, encryptionKey, true, "/"), + &oidcConfig{ + getClient: func() *http.Client { + return http.DefaultClient + }, + issuerURL: providerURL.String(), + logoutRedirectOverride: tt.logoutRedirectOverride, + clientID: testClientID, + secureCookies: true, + constructOAuth2Config: testOAuth2ConfigConstructor, + }, + auth.NewMetrics(defaultRestClientConfig), + ) + require.NoError(t, err) + + testCookieFactory := &testCookieFactory{ + cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey), + serverStore: o.sessions.ServerStore(), + tokenVerifier: oidcProvider.verifyIDToken, + signPayload: oidcProvider.signPayload, + } + + if tt.initSession { + token := addIDToken( + &oauth2.Token{RefreshToken: testValidRefreshToken}, + oidcProvider.signPayload(`{"sub":"testuser","exp":`+strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10)+`}`), + ) + req := httptest.NewRequest(http.MethodGet, "/", nil) + writer := httptest.NewRecorder() + ls, err := o.login(writer, req, token) + require.NoError(t, err) + testCookieFactory.WithSessionToken(ls.SessionToken()) + testCookieFactory.WithRefreshToken(testValidRefreshToken) + } + + req := httptest.NewRequest(http.MethodPost, "/api/console/logout", nil) + req = testCookieFactory.Complete(t, req) + + writer := httptest.NewRecorder() + o.logout(writer, req) + + require.Equal(t, tt.wantStatus, writer.Code) + + if tt.wantJSON { + require.Equal(t, "application/json", writer.Header().Get("Content-Type")) + + var resp map[string]string + err := json.NewDecoder(writer.Body).Decode(&resp) + require.NoError(t, err) + + logoutURL := resp["logoutRedirectURL"] + require.NotEmpty(t, logoutURL) + + parsed, err := url.Parse(logoutURL) + require.NoError(t, err) + + if tt.wantIDTokenHint { + hint := parsed.Query().Get("id_token_hint") + require.NotEmpty(t, hint, "expected id_token_hint in logout URL") + parts := strings.Split(hint, ".") + require.Equal(t, 3, len(parts), "id_token_hint should be a valid JWT with 3 parts") + } else { + require.Empty(t, parsed.Query().Get("id_token_hint"), "expected no id_token_hint") + } + + require.Equal(t, "console", parsed.Query().Get("client_id"), "original query params should be preserved") + } + }) + } +} + +func Test_oidcAuth_logoutRedirectURLWithIDTokenHint(t *testing.T) { + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + + oidcProvider, providerURL, closePort := startMockProvider(t) + defer closePort() + + tests := []struct { + name string + logoutRedirectOverride string + consoleBaseAddress string + initSession bool + wantEmpty bool + wantIDTokenHint bool + wantClientID string + wantPostLogoutRedirectURI string + }{ + { + name: "appends id_token_hint when session and redirect URL exist", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=console", + initSession: true, + wantIDTokenHint: true, + wantClientID: "console", + }, + { + name: "returns empty when no redirect URL configured", + logoutRedirectOverride: "", + initSession: true, + wantEmpty: true, + }, + { + name: "returns empty when no session", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=console", + initSession: false, + wantEmpty: true, + }, + { + name: "preserves existing query parameters", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=console&post_logout_redirect_uri=https%3A%2F%2Fconsole.example.com", + initSession: true, + wantIDTokenHint: true, + wantClientID: "console", + wantPostLogoutRedirectURI: "https://console.example.com", + }, + { + name: "adds client_id and post_logout_redirect_uri when not present", + logoutRedirectOverride: "https://keycloak.example.com/logout", + consoleBaseAddress: "https://console.example.com", + initSession: true, + wantIDTokenHint: true, + wantClientID: testClientID, + wantPostLogoutRedirectURI: "https://console.example.com", + }, + { + name: "does not overwrite existing client_id or post_logout_redirect_uri", + logoutRedirectOverride: "https://keycloak.example.com/logout?client_id=original&post_logout_redirect_uri=https%3A%2F%2Foriginal.example.com", + consoleBaseAddress: "https://console.example.com", + initSession: true, + wantIDTokenHint: true, + wantClientID: "original", + wantPostLogoutRedirectURI: "https://original.example.com", + }, + { + name: "returns empty without session even with consoleBaseAddress", + logoutRedirectOverride: "https://keycloak.example.com/logout", + consoleBaseAddress: "https://console.example.com", + initSession: false, + wantEmpty: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o, err := newOIDCAuth( + context.Background(), + sessions.NewSessionStore(authnKey, encryptionKey, true, "/"), + &oidcConfig{ + getClient: func() *http.Client { + return http.DefaultClient + }, + issuerURL: providerURL.String(), + logoutRedirectOverride: tt.logoutRedirectOverride, + clientID: testClientID, + consoleBaseAddress: tt.consoleBaseAddress, + secureCookies: true, + constructOAuth2Config: testOAuth2ConfigConstructor, + }, + auth.NewMetrics(defaultRestClientConfig), + ) + require.NoError(t, err) + + testCookieFactory := &testCookieFactory{ + cookieCodecs: securecookie.CodecsFromPairs(authnKey, encryptionKey), + serverStore: o.sessions.ServerStore(), + tokenVerifier: oidcProvider.verifyIDToken, + signPayload: oidcProvider.signPayload, + } + + if tt.initSession { + token := addIDToken( + &oauth2.Token{RefreshToken: testValidRefreshToken}, + oidcProvider.signPayload(`{"sub":"testuser","exp":`+strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10)+`}`), + ) + req := httptest.NewRequest(http.MethodGet, "/", nil) + writer := httptest.NewRecorder() + ls, err := o.login(writer, req, token) + require.NoError(t, err) + testCookieFactory.WithSessionToken(ls.SessionToken()) + testCookieFactory.WithRefreshToken(testValidRefreshToken) + } + + req := httptest.NewRequest(http.MethodPost, "/api/console/logout", nil) + req = testCookieFactory.Complete(t, req) + + writer := httptest.NewRecorder() + got := o.logoutRedirectURLWithIDTokenHint(writer, req) + + if tt.wantEmpty { + require.Empty(t, got) + return + } + + require.NotEmpty(t, got) + + parsed, err := url.Parse(got) + require.NoError(t, err) + + if tt.wantIDTokenHint { + hint := parsed.Query().Get("id_token_hint") + require.NotEmpty(t, hint, "expected id_token_hint query parameter") + parts := strings.Split(hint, ".") + require.Equal(t, 3, len(parts), "id_token_hint should be a valid JWT") + } else { + require.Empty(t, parsed.Query().Get("id_token_hint")) + } + + if tt.wantClientID != "" { + require.Equal(t, tt.wantClientID, parsed.Query().Get("client_id")) + } + + if tt.wantPostLogoutRedirectURI != "" { + require.Equal(t, tt.wantPostLogoutRedirectURI, parsed.Query().Get("post_logout_redirect_uri")) + } + + if tt.logoutRedirectOverride != "" { + expectedBase, err := url.Parse(tt.logoutRedirectOverride) + require.NoError(t, err) + require.Equal(t, expectedBase.Host, parsed.Host) + require.Equal(t, expectedBase.Path, parsed.Path) + } + }) + } +} + // TODO: should also test concurrent logins func BenchmarkRefreshSession(b *testing.B) { // init the provider