Skip to content

fix(cli): roll back gateway registration when auth fails during gateway add #1538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zanetworker wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(cli): roll back gateway registration when auth fails during gateway add #1538

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 158 additions & 12 deletions crates/openshell-cli/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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(());
Expand Down Expand Up @@ -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(),
Expand All @@ -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());
}
}

Expand Down Expand Up @@ -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}");
Expand Down Expand Up @@ -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"
);
});
}
}
Loading