Skip to content

fix(cli): fail closed when OIDC refresh fails #2817

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
jhjaggars wants to merge 1 commit into
base: main
Choose a base branch
from
+163 −34
Open

fix(cli): fail closed when OIDC refresh fails #2817

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion .agents/skills/openshell-cli/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name>`; `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.

Expand Down
52 changes: 26 additions & 26 deletions crates/openshell-cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,11 @@ fn resolve_gateway_name(gateway_flag: &Option<String>) -> Option<String> {
/// 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,
Expand Down Expand Up @@ -206,11 +209,8 @@ fn apply_auth_with_status(tls: &mut TlsOptions, gateway_name: &str) -> Option<St
}
Err(e) => {
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}`"
))
}
}
Expand Down Expand Up @@ -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()?;
Expand Down Expand Up @@ -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?;
}

Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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 } => {
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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 { .. }
Expand Down Expand Up @@ -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 } => {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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?;
}
Expand All @@ -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).
Expand Down Expand Up @@ -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"));
});
Expand Down
141 changes: 135 additions & 6 deletions crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/gateway-auth.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name>`.
3. The CLI connects to the gateway and attaches `authorization: Bearer <token>` 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.
Expand Down
Loading