From 4779a8de7c233777160d7527812ba32730a563a6 Mon Sep 17 00:00:00 2001 From: Adel Zaalouk Date: Sat, 23 May 2026 09:45:56 +0200 Subject: [PATCH] fix(cli): roll back gateway registration when auth fails during gateway add When OIDC or Cloudflare browser auth fails during gateway add, remove the gateway registration and restore the previously active gateway instead of leaving a broken entry on disk. Previously, store_gateway_metadata and save_active_gateway were called before the auth attempt. On failure, the registration persisted with an 'authenticate later' message, causing stale entries to accumulate when users retried with different flags or names. The rollback is skipped when the browser is intentionally suppressed (OPENSHELL_NO_BROWSER=1), since the user intends to authenticate later with gateway login. Fixes #1537 Signed-off-by: Adel Zaalouk --- crates/openshell-cli/src/run.rs | 170 +++++++++++++++++++++++++++++--- 1 file changed, 158 insertions(+), 12 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index f1a44ad31..c4b51f3de 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -941,6 +941,7 @@ pub async fn gateway_add( // OIDC takes precedence over plaintext/mTLS/edge detection — the user // explicitly opted in with --oidc-issuer regardless of scheme. if let Some(issuer) = oidc_issuer { + let previous_active = load_active_gateway(); let metadata = GatewayMetadata { name: name.to_string(), gateway_endpoint: endpoint.clone(), @@ -965,8 +966,10 @@ pub async fn gateway_add( eprintln!(" {} oidc", "Auth:".dimmed()); eprintln!(); + let auth_skipped = is_browser_suppressed(); + // Check for client_credentials env var (CI mode). - if std::env::var("OPENSHELL_OIDC_CLIENT_SECRET").is_ok() { + let auth_ok = if std::env::var("OPENSHELL_OIDC_CLIENT_SECRET").is_ok() { match crate::oidc_auth::oidc_client_credentials_flow( issuer, oidc_client_id, @@ -982,9 +985,11 @@ pub async fn gateway_add( "{} Authenticated via client credentials", "✓".green().bold() ); + true } Err(e) => { eprintln!("{} Authentication failed: {e}", "!".yellow()); + false } } } else { @@ -1000,15 +1005,17 @@ pub async fn gateway_add( Ok(bundle) => { openshell_bootstrap::oidc_token::store_oidc_token(name, &bundle)?; eprintln!("{} Authenticated successfully", "✓".green().bold()); + true } Err(e) => { - eprintln!("{} Authentication skipped: {e}", "!".yellow()); - eprintln!( - " Authenticate later with: {}", - "openshell gateway login".dimmed(), - ); + eprintln!("{} Authentication failed: {e}", "!".yellow()); + false } } + }; + + if !auth_ok && !auth_skipped { + rollback_gateway_registration(name, previous_active.as_deref()); } return Ok(()); @@ -1126,6 +1133,7 @@ pub async fn gateway_add( eprintln!("{} TLS certificates present", "✓".green().bold()); } else { // Cloud (edge-authenticated) gateway. + let previous_active = load_active_gateway(); let metadata = GatewayMetadata { name: name.to_string(), gateway_endpoint: endpoint.clone(), @@ -1146,18 +1154,22 @@ pub async fn gateway_add( eprintln!(" {} cloud", "Type:".dimmed()); eprintln!(); - match crate::auth::browser_auth_flow(&endpoint).await { + let auth_skipped = is_browser_suppressed(); + + let auth_ok = match crate::auth::browser_auth_flow(&endpoint).await { Ok(token) => { openshell_bootstrap::edge_token::store_edge_token(name, &token)?; eprintln!("{} Authenticated successfully", "✓".green().bold()); + true } Err(e) => { - eprintln!("{} Authentication skipped: {e}", "!".yellow()); - eprintln!( - " Authenticate later with: {}", - "openshell gateway login".dimmed(), - ); + eprintln!("{} Authentication failed: {e}", "!".yellow()); + false } + }; + + if !auth_ok && !auth_skipped { + rollback_gateway_registration(name, previous_active.as_deref()); } } @@ -1426,6 +1438,22 @@ async fn gateway_reachable(server: &str, tls: &TlsOptions) -> bool { matches!(http_health_check(server, tls).await, Ok(Some(status)) if status.is_success()) } +fn is_browser_suppressed() -> bool { + std::env::var("OPENSHELL_NO_BROWSER").is_ok_and(|v| v == "1" || v.eq_ignore_ascii_case("true")) +} + +fn rollback_gateway_registration(name: &str, previous_active: Option<&str>) { + remove_gateway_registration(name); + if let Some(prev) = previous_active { + let _ = save_active_gateway(prev); + } + eprintln!( + "{} Registration for '{}' removed. Fix the issue and retry gateway add.", + "!".yellow(), + name, + ); +} + fn remove_gateway_registration(name: &str) { if let Err(err) = openshell_bootstrap::edge_token::remove_edge_token(name) { tracing::debug!("failed to remove edge token: {err}"); @@ -8062,4 +8090,122 @@ mod tests { "host.example.test:443 [L7 rest, allow PUT /v1/example/resource, deny DELETE /v1/example/resource]" ); } + + #[test] + fn gateway_add_oidc_rolls_back_on_auth_failure() { + let _ = rustls::crypto::ring::default_provider().install_default(); + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + with_tmp_xdg(tmpdir.path(), || { + let runtime = tokio::runtime::Runtime::new().expect("create runtime"); + + // Register a working plaintext gateway first so we can verify + // the active gateway is restored after rollback. + runtime.block_on(async { + gateway_add( + "http://127.0.0.1:9999", + Some("existing-gw"), + None, + false, + None, + "openshell-cli", + None, + None, + false, + ) + .await + .expect("register seed gateway"); + }); + assert_eq!(load_active_gateway().as_deref(), Some("existing-gw")); + + // Attempt OIDC gateway add against an unreachable issuer. + // Auth will fail (connection refused), triggering rollback. + runtime.block_on(async { + gateway_add( + "https://gateway.example.com", + Some("oidc-fail"), + None, + false, + Some("http://127.0.0.1:1/realms/nonexistent"), + "openshell-cli", + None, + None, + false, + ) + .await + .expect("gateway_add should not return Err on auth failure"); + }); + + // The failed registration should have been rolled back. + assert!( + load_gateway_metadata("oidc-fail").is_err(), + "failed OIDC gateway should be removed after auth failure" + ); + // The previously active gateway should be restored. + assert_eq!( + load_active_gateway().as_deref(), + Some("existing-gw"), + "active gateway should be restored after rollback" + ); + }); + } + + #[test] + fn gateway_add_cloud_rolls_back_on_auth_failure() { + let _ = rustls::crypto::ring::default_provider().install_default(); + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + with_tmp_xdg(tmpdir.path(), || { + let runtime = tokio::runtime::Runtime::new().expect("create runtime"); + + // Register a working plaintext gateway first. + runtime.block_on(async { + gateway_add( + "http://127.0.0.1:9999", + Some("existing-gw"), + None, + false, + None, + "openshell-cli", + None, + None, + false, + ) + .await + .expect("register seed gateway"); + }); + assert_eq!(load_active_gateway().as_deref(), Some("existing-gw")); + + // Attempt cloud gateway add. The browser flow will fail because + // OPENSHELL_NO_BROWSER is NOT set but the /auth/connect endpoint + // is unreachable (connection refused), so the 120s timeout would + // kick in. To keep the test fast, set OPENSHELL_NO_BROWSER=0 + // (explicitly not suppressed) and use a port that refuses connections. + // The CF auth flow will fail quickly on connection refused. + runtime.block_on(async { + gateway_add( + "https://127.0.0.1:1", + Some("cloud-fail"), + None, + false, + None, + "openshell-cli", + None, + None, + false, + ) + .await + .expect("gateway_add should not return Err on auth failure"); + }); + + // The failed registration should have been rolled back. + assert!( + load_gateway_metadata("cloud-fail").is_err(), + "failed cloud gateway should be removed after auth failure" + ); + assert_eq!( + load_active_gateway().as_deref(), + Some("existing-gw"), + "active gateway should be restored after rollback" + ); + }); + } }