diff --git a/.agents/skills/openshell-cli/SKILL.md b/.agents/skills/openshell-cli/SKILL.md index 1cee3d5c37..c90dc28f78 100644 --- a/.agents/skills/openshell-cli/SKILL.md +++ b/.agents/skills/openshell-cli/SKILL.md @@ -627,7 +627,7 @@ openshell gateway add https://gateway.example.com --name production openshell gateway remove local ``` -`https://` registrations default to edge authentication. Use `gateway login` and `gateway logout` to refresh or clear stored authentication. For an OIDC gateway, supply `--oidc-issuer` and, when needed, `--oidc-client-id`, `--oidc-audience`, and `--oidc-scopes`. For remote mTLS gateways, use `--remote USER@HOST` or an `ssh://` endpoint. +`https://` registrations default to edge authentication. Use `gateway login` and `gateway logout` to refresh or clear stored authentication. For an OIDC gateway, supply `--oidc-issuer` and, when needed, `--oidc-client-id`, `--oidc-audience`, and `--oidc-scopes`. If automatic OIDC refresh fails, protected commands stop before sending an RPC and direct the user to run `openshell gateway login `; `openshell status` still reports gateway reachability and authentication separately. For remote mTLS gateways, use `--remote USER@HOST` or an `ssh://` endpoint. For one-off automation, `--gateway-endpoint URL` connects directly without stored metadata. Limit `--gateway-insecure` to explicitly trusted development endpoints. diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index fc7728c15d..40fa22a913 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -156,8 +156,11 @@ fn resolve_gateway_name(gateway_flag: &Option) -> Option { /// Handles Cloudflare Access and OIDC auth modes by loading the stored token /// and setting it on `TlsOptions`. For OIDC, automatically refreshes the token /// if it's near expiry. -fn apply_auth(tls: &mut TlsOptions, gateway_name: &str) { - let _ = apply_auth_with_status(tls, gateway_name); +fn apply_auth(tls: &mut TlsOptions, gateway_name: &str) -> Result<()> { + if let Some(error) = apply_auth_with_status(tls, gateway_name) { + return Err(miette::miette!(error)); + } + Ok(()) } /// Apply stored authentication and return a user-facing preparation failure, @@ -206,11 +209,8 @@ fn apply_auth_with_status(tls: &mut TlsOptions, gateway_name: &str) -> Option { tracing::warn!("OIDC token refresh failed: {e}"); - // Use the expired token anyway — server will reject it - // with a clear error prompting re-login. - tls.oidc_token = Some(bundle.access_token); Some(format!( - "OIDC token refresh failed; run `openshell gateway login {gateway_name}`" + "OIDC token refresh failed: {e}\nrun `openshell gateway login {gateway_name}`" )) } } @@ -2315,7 +2315,7 @@ async fn run_async() -> Result<()> { GatewayCommands::Info { output } => { if let Ok(ctx) = resolve_gateway(&cli.gateway, &cli.gateway_endpoint) { let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; run::gateway_info(&ctx.name, &ctx.endpoint, &tls, output.as_str()).await?; } else { run::gateway_info_not_configured()?; @@ -2383,7 +2383,7 @@ async fn run_async() -> Result<()> { Some(Commands::Whoami { output }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; run::whoami(&ctx.endpoint, &tls, output.as_str()).await?; } @@ -2471,7 +2471,7 @@ async fn run_async() -> Result<()> { } => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?; let local = local.unwrap_or_else(|| target_port.to_string()); run::service_forward_tcp( @@ -2493,7 +2493,7 @@ async fn run_async() -> Result<()> { let spec = openshell_core::forward::ForwardSpec::parse(&port)?; let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?; run::sandbox_forward( &ctx.endpoint, @@ -2524,7 +2524,7 @@ async fn run_async() -> Result<()> { }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match command { ServiceCommands::Expose { sandbox, @@ -2584,7 +2584,7 @@ async fn run_async() -> Result<()> { }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?; run::sandbox_logs( &ctx.endpoint, @@ -2608,7 +2608,7 @@ async fn run_async() -> Result<()> { }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match policy_cmd { PolicyCommands::Set { name, @@ -2748,7 +2748,7 @@ async fn run_async() -> Result<()> { }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match settings_cmd { SettingsCommands::Get { name, global, json } => { @@ -2827,7 +2827,7 @@ async fn run_async() -> Result<()> { }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match draft_cmd { DraftCommands::Get { name, status } => { let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?; @@ -2902,7 +2902,7 @@ async fn run_async() -> Result<()> { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match command { InferenceCommands::Set { provider, @@ -3051,7 +3051,7 @@ async fn run_async() -> Result<()> { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; Box::pin(run::sandbox_create( endpoint, &ctx.name, @@ -3089,7 +3089,7 @@ async fn run_async() -> Result<()> { } => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let local = std::path::Path::new(&local_path); run::sandbox_upload( &ctx.endpoint, @@ -3109,7 +3109,7 @@ async fn run_async() -> Result<()> { } => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let local_dest = dest.as_deref().unwrap_or("."); eprintln!("Downloading sandbox:{sandbox_path} -> {local_dest}"); run::sandbox_sync_down( @@ -3127,7 +3127,7 @@ async fn run_async() -> Result<()> { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match other { SandboxCommands::Create { .. } | SandboxCommands::Upload { .. } @@ -3290,7 +3290,7 @@ async fn run_async() -> Result<()> { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match command { WorkspaceCommands::Create { name, labels } => { @@ -3348,7 +3348,7 @@ async fn run_async() -> Result<()> { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let endpoint = &ctx.endpoint; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; match command { ProviderCommands::Create { @@ -3551,7 +3551,7 @@ async fn run_async() -> Result<()> { Some(Commands::Term { theme }) => { let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?; let mut tls = tls.with_gateway_name(&ctx.name); - apply_auth(&mut tls, &ctx.name); + apply_auth(&mut tls, &ctx.name)?; let channel = openshell_cli::tls::build_channel(&ctx.endpoint, &tls).await?; let interceptor = openshell_core::auth::EdgeAuthInterceptor::new( tls.oidc_token.as_deref(), @@ -3595,7 +3595,7 @@ async fn run_async() -> Result<()> { None => tls, }; if let Some(ref g) = gateway_name_opt { - apply_auth(&mut effective_tls, g); + apply_auth(&mut effective_tls, g)?; } run::sandbox_ssh_proxy(&gw, &sid, &tok, &effective_tls).await?; } @@ -3614,7 +3614,7 @@ async fn run_async() -> Result<()> { meta.gateway_endpoint }; let mut tls = tls.with_gateway_name(&g); - apply_auth(&mut tls, &g); + apply_auth(&mut tls, &g)?; run::sandbox_ssh_proxy_by_name(&endpoint, &n, &tls, &cli.workspace).await?; } // Legacy name mode with --server only (no --gateway-name). @@ -4300,7 +4300,7 @@ mod tests { store_edge_token("edge-gateway", "token-123").unwrap(); let mut tls = TlsOptions::default(); - apply_auth(&mut tls, "edge-gateway"); + apply_auth(&mut tls, "edge-gateway").unwrap(); assert_eq!(tls.edge_token.as_deref(), Some("token-123")); }); diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 102cde3714..15b5d30800 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -36,6 +36,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::time::{Duration, Instant}; use tempfile::TempDir; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::TcpListener; use tokio::sync::{Mutex, mpsc}; use tokio_stream::wrappers::TcpListenerStream; @@ -1926,18 +1927,20 @@ async fn sandbox_create_env_rejects_invalid_key_name() { ); } -async fn run_cli_sandbox_create( - server: &TestServer, - name: &str, - extra_args: &[&str], -) -> std::process::Output { - let xdg_dir = tempfile::tempdir().unwrap(); +fn prepare_cli_xdg(server: &TestServer, xdg_dir: &TempDir) { let tls_dir = xdg_dir.path().join("openshell/gateways/openshell/mtls"); fs::create_dir_all(&tls_dir).unwrap(); for filename in ["ca.crt", "tls.crt", "tls.key"] { fs::copy(server.dir.path().join(filename), tls_dir.join(filename)).unwrap(); } +} +async fn run_cli_sandbox_create_with_xdg( + server: &TestServer, + xdg_dir: &TempDir, + name: &str, + extra_args: &[&str], +) -> std::process::Output { let mut cmd = tokio::process::Command::new(env!("CARGO_BIN_EXE_openshell")); for (key, _) in std::env::vars().filter(|(k, _)| k.starts_with("OPENSHELL_")) { cmd.env_remove(&key); @@ -1963,6 +1966,132 @@ async fn run_cli_sandbox_create( .unwrap() } +async fn run_cli_sandbox_create( + server: &TestServer, + name: &str, + extra_args: &[&str], +) -> std::process::Output { + let xdg_dir = tempfile::tempdir().unwrap(); + prepare_cli_xdg(server, &xdg_dir); + run_cli_sandbox_create_with_xdg(server, &xdg_dir, name, extra_args).await +} + +async fn run_rejected_oidc_refresh_server() -> String { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let issuer = format!("http://{}", listener.local_addr().unwrap()); + let task_issuer = issuer.clone(); + + tokio::spawn(async move { + loop { + let Ok((mut stream, _)) = listener.accept().await else { + return; + }; + let issuer = task_issuer.clone(); + tokio::spawn(async move { + let mut request = vec![0; 8192]; + let Ok(read) = stream.read(&mut request).await else { + return; + }; + let request = String::from_utf8_lossy(&request[..read]); + let (status, body) = + if request.starts_with("GET /.well-known/openid-configuration ") { + ( + "200 OK", + serde_json::json!({ + "issuer": issuer, + "authorization_endpoint": format!("{issuer}/authorize"), + "token_endpoint": format!("{issuer}/token"), + }) + .to_string(), + ) + } else if request.starts_with("POST /token ") { + ( + "400 Bad Request", + serde_json::json!({ + "error": "invalid_grant", + "error_description": "Token is not active", + }) + .to_string(), + ) + } else { + ("404 Not Found", "{}".to_string()) + }; + let response = format!( + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len(), + ); + let _ = stream.write_all(response.as_bytes()).await; + }); + } + }); + + issuer +} + +/// Models the gateway's JWT-expiration leeway by using a test gateway that +/// still accepts the stale bearer. Before the CLI failed closed, the rejected +/// refresh was only logged and `CreateSandbox` reached this gateway anyway. +#[tokio::test] +async fn sandbox_create_fails_before_mutation_when_expired_oidc_refresh_is_rejected() { + let server = run_server().await; + let issuer = run_rejected_oidc_refresh_server().await; + let xdg_dir = tempfile::tempdir().unwrap(); + prepare_cli_xdg(&server, &xdg_dir); + + let gateway_dir = xdg_dir.path().join("openshell/gateways/openshell"); + fs::write( + gateway_dir.join("metadata.json"), + serde_json::to_vec_pretty(&openshell_bootstrap::GatewayMetadata { + name: "openshell".to_string(), + gateway_endpoint: server.endpoint.clone(), + auth_mode: Some("oidc".to_string()), + oidc_issuer: Some(issuer.clone()), + oidc_client_id: Some("openshell-cli".to_string()), + ..Default::default() + }) + .unwrap(), + ) + .unwrap(); + fs::write( + gateway_dir.join("oidc_token.json"), + serde_json::to_vec_pretty(&openshell_bootstrap::oidc_token::OidcTokenBundle { + access_token: "expired-access-token".to_string(), + refresh_token: Some("inactive-refresh-token".to_string()), + expires_at: Some(0), + issuer, + client_id: "openshell-cli".to_string(), + }) + .unwrap(), + ) + .unwrap(); + + let result = run_cli_sandbox_create_with_xdg( + &server, + &xdg_dir, + "must-not-be-created", + &["--output=json"], + ) + .await; + + assert!( + !result.status.success(), + "refresh failure must abort create" + ); + let stderr = String::from_utf8_lossy(&result.stderr); + assert!( + stderr.contains("invalid_grant"), + "unexpected error: {stderr}" + ); + assert!( + stderr.contains("openshell gateway login openshell"), + "error should explain how to re-authenticate: {stderr}" + ); + assert!( + create_requests(&server).await.is_empty(), + "CreateSandbox must not be called after OIDC refresh fails" + ); +} + #[tokio::test] async fn sandbox_create_json_stdout_is_parseable() { let server = run_server().await; diff --git a/docs/reference/gateway-auth.mdx b/docs/reference/gateway-auth.mdx index 311038480c..879263b6c7 100644 --- a/docs/reference/gateway-auth.mdx +++ b/docs/reference/gateway-auth.mdx @@ -125,7 +125,7 @@ When you register or log in to an OIDC gateway, the CLI uses the Authorization C The connection flow: 1. The CLI loads the stored OIDC token bundle. -2. If the access token is expired and a refresh token is available, the CLI refreshes it with the OIDC scopes saved in the gateway metadata. +2. If the access token is expired or near expiry, the CLI refreshes it with the OIDC scopes saved in the gateway metadata. If refresh fails, the command stops before sending a protected request and directs you to run `openshell gateway login `. 3. The CLI connects to the gateway and attaches `authorization: Bearer ` metadata to each gRPC request. 4. The gateway validates the JWT signature, issuer, audience, expiration, and key ID against the issuer's JWKS. 5. The gateway extracts roles and optional scopes from the configured claim paths.