From 17fd1619456baaf0b4a79ce1fca2a175937973b8 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Sat, 5 Sep 2026 18:46:13 +0200 Subject: [PATCH] Report shell ACL refusals --- src/daemon.rs | 20 +++++++--- src/main.rs | 34 ++++++++++++----- src/mux.rs | 4 ++ src/shell.rs | 24 +++++++++--- tests/shell.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 20 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 02a7131..39fe3b9 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -4698,6 +4698,16 @@ async fn handle_shell_dial_socket_connection( .await; } Err(error) => { + if mux::is_permanent_stream_denial(&error) { + let message = format!("refused service \"shell\": {error}"); + let _ = shell::serve_shell_failure( + &mut local, + &message, + shell::EXIT_SHELL_DISABLED, + ) + .await; + return Err(error).context("peer refused resumable shell"); + } if tunnel::is_permanent_failure(&error) { return Err(error.context("peer refused resumable shell")); } @@ -4763,7 +4773,7 @@ async fn run_legacy_shell_after_selection( match connected { Ok(connection) => { let (send, recv) = connection.open_bi().await?; - return pipe_unix_iroh(local, send, recv).await; + return pipe_framed_unix_iroh(local, send, recv).await; } Err(error) => { let error = anyhow::Error::new(error); @@ -5185,18 +5195,18 @@ async fn handle_raw_dial_socket_connection( } }; if alpn == exec::EXEC_ALPN { - pipe_exec_unix_iroh(local, stream.send, stream.recv).await?; + pipe_framed_unix_iroh(local, stream.send, stream.recv).await?; } else { pipe_unix_iroh(local, stream.send, stream.recv).await?; } Ok(()) } -/// Keep receiving exec frames after the peer stops its receive direction. +/// Keep receiving framed replies after the peer stops its receive direction. /// /// A policy refusal sends Error and Exit, then closes without reading the -/// command frame. That stopped send direction must not cancel the useful reply. -async fn pipe_exec_unix_iroh( +/// request frames. That stopped send direction must not cancel the useful reply. +async fn pipe_framed_unix_iroh( local: UnixStream, mut send: SendStream, mut recv: RecvStream, diff --git a/src/main.rs b/src/main.rs index 8f0b520..73f18e9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1018,11 +1018,14 @@ async fn main() -> Result<()> { } } Commands::Shell { peer } => { - let socket = match send_control(&home, ControlRequest::Shell { peer }).await? { - ControlResponse::Shell { socket } => socket, - response => bail!("unexpected daemon response: {response:?}"), - }; - let code = run_shell_client(&socket).await?; + let socket = + match send_control(&home, ControlRequest::Shell { peer: peer.clone() }) + .await? + { + ControlResponse::Shell { socket } => socket, + response => bail!("unexpected daemon response: {response:?}"), + }; + let code = run_shell_client(&socket, &peer).await?; std::process::exit(code); } Commands::Exec { peer, cmd } => { @@ -2869,7 +2872,7 @@ async fn wait_for_daemon_ready( } } -async fn run_shell_client(socket: &PathBuf) -> Result { +async fn run_shell_client(socket: &PathBuf, peer: &str) -> Result { let stream = tokio::net::UnixStream::connect(socket).await?; let (mut read, write) = stream.into_split(); let mut signals = ShellSignals::new()?; @@ -2894,7 +2897,7 @@ async fn run_shell_client(socket: &PathBuf) -> Result { let mut stdout = tokio::io::stdout(); let mut stderr = tokio::io::stderr(); - let mut exit_code = 1; + let mut exit_code = None; loop { tokio::select! { @@ -2908,6 +2911,9 @@ async fn run_shell_client(socket: &PathBuf) -> Result { stdout.flush().await?; } ServerFrame::Error(message) => { + stderr + .write_all(format!("fabric: peer {peer:?} ").as_bytes()) + .await?; stderr.write_all(message.as_bytes()).await?; stderr.write_all(b"\n").await?; stderr.flush().await?; @@ -2918,7 +2924,7 @@ async fn run_shell_client(socket: &PathBuf) -> Result { stderr.flush().await?; } ServerFrame::Exit(code) => { - exit_code = normalize_exit_code(code); + exit_code = Some(normalize_exit_code(code)); break; } } @@ -2948,9 +2954,19 @@ async fn run_shell_client(socket: &PathBuf) -> Result { stdin_task.abort(); let _ = stdin_task.await; terminal.restore()?; + if exit_code.is_none() { + stderr + .write_all( + format!( + "fabric: peer {peer:?} closed service \"shell\" before it returned an exit status\n" + ) + .as_bytes(), + ) + .await?; + } stdout.flush().await?; stderr.flush().await?; - Ok(exit_code) + Ok(exit_code.unwrap_or(1)) } /// Drive the client side of a `fabric exec` session over the daemon-provided diff --git a/src/mux.rs b/src/mux.rs index fe86720..dae6ac6 100644 --- a/src/mux.rs +++ b/src/mux.rs @@ -980,6 +980,10 @@ mod tests { )); assert!(is_stream_denied(&acl)); assert!(is_permanent_stream_denial(&acl)); + + let transport = anyhow::anyhow!("connection timed out"); + assert!(!is_stream_denied(&transport)); + assert!(!is_permanent_stream_denial(&transport)); } #[test] diff --git a/src/shell.rs b/src/shell.rs index 408c34c..bdf8faa 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -26,6 +26,7 @@ const SERVER_OUTPUT: u8 = 17; const SERVER_EXIT: u8 = 18; const SERVER_ERROR: u8 = 19; const SERVER_STATUS: u8 = 20; +pub(crate) const EXIT_SHELL_DISABLED: i32 = 126; #[derive(Debug)] pub enum ClientFrame { @@ -46,14 +47,25 @@ pub async fn serve_shell_disabled(send: &mut W) -> Result<()> where W: AsyncWrite + Unpin, { - write_server_frame( + serve_shell_failure( send, - ServerFrame::Error( - "remote shell is disabled; set allow_shell = true in peers.toml".to_string(), - ), + "refused service \"shell\": remote shell is disabled; set allow_shell = true in peers.toml", + EXIT_SHELL_DISABLED, ) - .await?; - write_server_frame(send, ServerFrame::Exit(126)).await + .await +} + +/// Send a complete failure response when a shell session cannot start. +pub(crate) async fn serve_shell_failure( + send: &mut W, + message: &str, + exit_code: i32, +) -> Result<()> +where + W: AsyncWrite + Unpin, +{ + write_server_frame(send, ServerFrame::Error(message.to_string())).await?; + write_server_frame(send, ServerFrame::Exit(exit_code)).await } pub async fn serve_shell_session(recv: &mut R, send: &mut W, peer: &str) -> Result<()> diff --git a/tests/shell.rs b/tests/shell.rs index 43e9c70..c19359e 100644 --- a/tests/shell.rs +++ b/tests/shell.rs @@ -75,6 +75,21 @@ impl ProtocolHandler for LegacyRawShell { } } +#[derive(Debug, Clone)] +struct LegacyDisabledShell; + +impl ProtocolHandler for LegacyDisabledShell { + async fn accept(&self, connection: Connection) -> Result<(), AcceptError> { + let (mut send, _recv) = connection.accept_bi().await?; + shell::serve_shell_disabled(&mut send) + .await + .map_err(|error| AcceptError::from_boxed(error.into_boxed_dyn_error()))?; + send.finish()?; + connection.closed().await; + Ok(()) + } +} + /// The legacy ALPN is a wire contract with every released Fabric: it carries /// one-shot shell framing and never generic tunnel frames. A build that routes /// shell/0 through the tunnel session protocol makes an old peer reject the @@ -134,6 +149,42 @@ async fn new_client_falls_back_to_legacy_raw_shell_zero() -> Result<()> { Ok(()) } +/// A legacy peer can send a refusal and stop reading at the same time. The +/// stopped send direction must not cancel the Error and Exit frames already in +/// the receive direction. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn legacy_shell_refusal_frames_survive_a_stopped_send_direction() -> Result<()> { + let legacy_endpoint = Endpoint::bind(presets::N0).await?; + let legacy = Router::builder(legacy_endpoint) + .accept(shell::SHELL_ALPN, LegacyDisabledShell) + .spawn(); + legacy.endpoint().online().await; + + let client_dir = TempDir::new()?; + let client_home = FabricHome::new(client_dir.path()); + let client = FabricNode::start(client_home.clone()).await?; + trust_peer( + &client_home, + &client, + legacy.endpoint().id(), + Some("legacy-disabled"), + Some(legacy.endpoint().addr()), + ) + .await?; + + let output = run_shell(&client_home, "legacy-disabled", "")?; + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(126), "stderr: {stderr}"); + assert!( + stderr.contains("legacy-disabled") && stderr.contains("shell"), + "the legacy refusal omitted the peer or service: {stderr:?}" + ); + + client.shutdown().await?; + legacy.shutdown().await?; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn unavailable_old_peer_later_falls_back_to_raw_shell_zero() -> Result<()> { let legacy_secret = iroh::SecretKey::generate(); @@ -853,6 +904,54 @@ async fn trusted_peer_without_allow_shell_is_refused() -> Result<()> { Ok(()) } +/// A per-peer ACL refusal happens before the resumable shell session starts. +/// The local daemon must still send shell frames to the CLI. Otherwise the CLI +/// returns status 1 with no reason, even though the peer is reachable. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn shell_names_the_peer_and_service_when_the_peer_acl_refuses_it() -> Result<()> { + let server_dir = TempDir::new()?; + let client_dir = TempDir::new()?; + let server_home = FabricHome::new(server_dir.path()); + let client_home = FabricHome::new(client_dir.path()); + let server = start_shell_server(server_home.clone()).await?; + let client = FabricNode::start(client_home.clone()).await?; + + trust_peer_allowing( + &server_home, + &server, + client.id(), + Some("client"), + Some(client.addr()), + &["echo"], + ) + .await?; + trust_peer( + &client_home, + &client, + server.id(), + Some("server"), + Some(server.addr()), + ) + .await?; + assert_eq!(client.ping("server").await?.bytes, 32); + + let output = run_shell(&client_home, "server", "")?; + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(126), "stderr: {stderr}"); + assert!( + stderr.contains("server"), + "the refusal omitted the peer: {stderr:?}" + ); + assert!( + stderr.contains("shell"), + "the refusal omitted the service: {stderr:?}" + ); + + client.shutdown().await?; + server.shutdown().await?; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn untrusted_peer_is_refused_even_when_shell_is_allowed() -> Result<()> { let node_a_dir = TempDir::new()?;