diff options
| -rw-r--r-- | src/cli/machine.rs | 2 | ||||
| -rw-r--r-- | src/remote.rs | 29 | ||||
| -rw-r--r-- | src/remote/attach.rs | 77 | ||||
| -rw-r--r-- | tests/machine_setup.rs | 59 |
4 files changed, 154 insertions, 13 deletions
diff --git a/src/cli/machine.rs b/src/cli/machine.rs index a55e4611..3e93d101 100644 --- a/src/cli/machine.rs +++ b/src/cli/machine.rs @@ -144,7 +144,7 @@ fn add(args: &[String]) -> std::io::Result<i32> { } if let Err(error) = crate::remote::prepare_saved_ssh(target, &session) { eprintln!("error: {error}; machine was not saved"); - crate::remote::print_remote_error_hint(&error, target); + crate::remote::print_saved_ssh_error_hint(&error, target); return Ok(1); } // Setup can wait for human approval. Do not overwrite catalog edits made meanwhile. diff --git a/src/remote.rs b/src/remote.rs index ba935ec4..4ab4d531 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -19,6 +19,16 @@ pub(crate) fn run_remote_client_bridge() -> std::io::Result<()> { )) } +pub(crate) fn print_saved_ssh_error_hint(err: &std::io::Error, target: &str) { + if is_remote_host_key_error(err) { + eprintln!( + "hint: saved machines use strict host-key checking; add the host key to the configured known_hosts file, then retry." + ); + } else { + print_remote_error_hint(err, target); + } +} + pub(crate) fn print_remote_error_hint(err: &std::io::Error, target: &str) { if is_remote_auth_error(err) { eprintln!( @@ -31,6 +41,12 @@ pub(crate) fn print_remote_error_hint(err: &std::io::Error, target: &str) { } } +fn is_remote_host_key_error(err: &std::io::Error) -> bool { + let message = err.to_string().to_ascii_lowercase(); + message.contains("host key verification failed") + || message.contains("remote host identification has changed") +} + fn is_remote_auth_error(err: &std::io::Error) -> bool { let message = err.to_string(); message.contains("Permission denied") @@ -64,6 +80,19 @@ mod tests { use super::*; #[test] + fn remote_host_key_error_matches_ssh_diagnostics() { + for message in [ + "Host key verification failed.", + "REMOTE HOST IDENTIFICATION HAS CHANGED!", + ] { + assert!(is_remote_host_key_error(&std::io::Error::other(message))); + } + assert!(!is_remote_host_key_error(&std::io::Error::other( + "server closed connection" + ))); + } + + #[test] fn remote_auth_error_matches_ssh_auth_denied() { let err = std::io::Error::other( "remote platform detection failed: user@host: Permission denied (publickey).", diff --git a/src/remote/attach.rs b/src/remote/attach.rs index 46c0165d..ac5415fa 100644 --- a/src/remote/attach.rs +++ b/src/remote/attach.rs @@ -13,7 +13,7 @@ use interprocess::TryClone as _; use serde::Deserialize; use std::sync::{ atomic::{AtomicBool, Ordering}, - Arc, + mpsc, Arc, }; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -23,6 +23,8 @@ const BRIDGE_IO_POLL: Duration = Duration::from_millis(1); const BRIDGE_SOCKET_PERMISSION_MODE: u32 = 0o600; const REMOTE_SERVER_SHUTDOWN_CONFIRM_TIMEOUT: Duration = Duration::from_secs(5); const NONINTERACTIVE_SSH_COMMAND_TIMEOUT: Duration = Duration::from_secs(15); +const NONINTERACTIVE_SSH_STDERR_LIMIT: usize = 16 * 1024; +const BRIDGE_FAILURE_REPORT_TIMEOUT: Duration = Duration::from_secs(1); const REMOTE_SERVER_SHUTDOWN_POLL_INTERVAL: Duration = Duration::from_millis(100); const CURRENT_PROTOCOL: u32 = crate::protocol::PROTOCOL_VERSION; const STABLE_UPDATE_MANIFEST_URL: &str = "https://herdr.dev/latest.json"; @@ -1235,7 +1237,7 @@ fn probe_remote_endpoint( remote_herdr: &RemoteHerdr, ) -> io::Result<crate::client::endpoint::EndpointNegotiation> { let path = local_forward_socket_path(ssh.target(), &ssh.session_name); - let _bridge = SshStdioBridge::start( + let bridge = SshStdioBridge::start( ssh.target.clone(), remote_herdr.clone(), path.clone(), @@ -1246,7 +1248,10 @@ fn probe_remote_endpoint( let mut stream = crate::ipc::connect_local_stream(&path)?; // Use the saved client's noninteractive path. This metadata-only attachment never // acquires a surface or sends pane input. - crate::client::probe_endpoint_negotiation(&mut stream) + match crate::client::probe_endpoint_negotiation(&mut stream) { + Ok(negotiation) => Ok(negotiation), + Err(probe_error) => Err(bridge.reported_failure().unwrap_or(probe_error)), + } } #[derive(Debug, Deserialize)] @@ -1780,6 +1785,7 @@ pub(super) struct SshStdioBridge { local_socket: PathBuf, socket_identity: crate::ipc::SocketFileIdentity, should_stop: Arc<AtomicBool>, + failure_rx: mpsc::Receiver<io::Error>, thread: Option<JoinHandle<()>>, } @@ -1811,6 +1817,7 @@ impl SshStdioBridge { let should_stop = Arc::new(AtomicBool::new(false)); let thread_stop = Arc::clone(&should_stop); let thread_ssh_options = ssh_options.cloned(); + let (failure_tx, failure_rx) = mpsc::sync_channel(1); let thread = thread::spawn(move || { while !thread_stop.load(Ordering::Acquire) { match listener.accept() { @@ -1834,6 +1841,8 @@ impl SshStdioBridge { noninteractive, &thread_stop, ) { + let _ = + failure_tx.try_send(io::Error::new(err.kind(), err.to_string())); if noninteractive { tracing::warn!(error = %err, "saved SSH endpoint bridge failed"); } else { @@ -1860,9 +1869,16 @@ impl SshStdioBridge { local_socket, socket_identity, should_stop, + failure_rx, thread: Some(thread), }) } + + fn reported_failure(&self) -> Option<io::Error> { + self.failure_rx + .recv_timeout(BRIDGE_FAILURE_REPORT_TIMEOUT) + .ok() + } } fn prepare_remote_bridge_stream( @@ -2020,7 +2036,7 @@ fn bridge_connection( .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(if noninteractive { - Stdio::null() + Stdio::piped() } else { Stdio::inherit() }); @@ -2036,6 +2052,14 @@ fn bridge_connection( Some(stdout) => stdout, None => return terminate_bridge_child(child, "ssh bridge stdout missing"), }; + let stderr_reader = if noninteractive { + let Some(child_stderr) = child.stderr.take() else { + return terminate_bridge_child(child, "ssh bridge stderr missing"); + }; + Some(thread::spawn(move || capture_ssh_stderr(child_stderr))) + } else { + None + }; let stream_to_child = match stream.try_clone() { Ok(stream) => stream, Err(err) => { @@ -2132,10 +2156,19 @@ fn bridge_connection( let download_result = download .join() .map_err(|_| io::Error::other("remote bridge download worker panicked"))?; + let stderr = match stderr_reader { + Some(reader) => reader + .join() + .map_err(|_| io::Error::other("SSH stderr reader panicked"))??, + None => Vec::new(), + }; let status = status_result?; let stopping = bridge_stop.load(Ordering::Acquire); let client_closed = client_closed.load(Ordering::Acquire); + if child_exited && !status.success() && !stopping && !client_closed { + return Err(ssh_bridge_exit_error(status, &stderr)); + } if !stopping && !client_closed { upload_result.map_err(|err| { io::Error::new(err.kind(), format!("remote bridge upload failed: {err}")) @@ -2148,10 +2181,31 @@ fn bridge_connection( if status.success() || stopping || client_closed { Ok(()) } else { - Err(io::Error::new( - io::ErrorKind::ConnectionAborted, - format!("ssh bridge exited with {status}"), - )) + Err(ssh_bridge_exit_error(status, &stderr)) + } +} + +fn ssh_bridge_exit_error(status: std::process::ExitStatus, stderr: &[u8]) -> io::Error { + let stderr = String::from_utf8_lossy(stderr); + let stderr = stderr.trim(); + let message = if stderr.is_empty() { + format!("ssh bridge exited with {status}") + } else { + format!("remote SSH connection failed: {stderr}") + }; + io::Error::new(io::ErrorKind::ConnectionAborted, message) +} + +fn capture_ssh_stderr(mut stderr: impl io::Read) -> io::Result<Vec<u8>> { + let mut captured = Vec::new(); + let mut buffer = [0_u8; 4 * 1024]; + loop { + let read = stderr.read(&mut buffer)?; + if read == 0 { + return Ok(captured); + } + let remaining = NONINTERACTIVE_SSH_STDERR_LIMIT.saturating_sub(captured.len()); + captured.extend_from_slice(&buffer[..read.min(remaining)]); } } @@ -2703,6 +2757,13 @@ mod tests { } #[test] + fn noninteractive_ssh_stderr_capture_is_bounded() { + let stderr = vec![b'x'; NONINTERACTIVE_SSH_STDERR_LIMIT + 4096]; + let captured = capture_ssh_stderr(stderr.as_slice()).expect("capture stderr"); + assert_eq!(captured.len(), NONINTERACTIVE_SSH_STDERR_LIMIT); + } + + #[test] fn noninteractive_ssh_command_cannot_prompt_or_accept_unknown_hosts() { let ssh = RemoteSsh::new_noninteractive("example".into()); let args = ssh diff --git a/tests/machine_setup.rs b/tests/machine_setup.rs index 752126fc..da763d98 100644 --- a/tests/machine_setup.rs +++ b/tests/machine_setup.rs @@ -11,7 +11,15 @@ use portable_pty::{native_pty_system, CommandBuilder, PtySize}; // No real SSH connection or server is started. Stop at startup after recording setup actions. const SSH: &str = r#"#!/bin/sh -for arg do last=$arg; done +for arg do + last=$arg + if [ "$arg" = 'StrictHostKeyChecking=yes' ]; then strict_host_key_check=yes; fi +done +if [ "$FAKE_STRICT_HOST_KEY_FAILURE" = yes ] && [ "$strict_host_key_check" = yes ]; then + echo 'No RSA host key is known for fake-host and you have requested strict checking.' >&2 + echo 'Host key verification failed.' >&2 + exit 255 +fi if [ "$last" = 'command -v herdr' ]; then echo /home/remote/.local/bin/herdr exit 0 @@ -32,6 +40,8 @@ case "$script" in *'status server --json'*) if [ -f "$FAKE_ROOT/stopped" ]; then echo '{"running":false}' + elif [ "$FAKE_STRICT_HOST_KEY_FAILURE" = yes ]; then + echo '{"running":true,"version":"0.8.2","capabilities":{"live_handoff":true,"detached_server_daemon":true,"endpoint_protocol_generation":1,"surface_interest":true,"health_check":true}}' else echo '{"running":true,"version":"0.8.2","capabilities":{"live_handoff":true,"detached_server_daemon":true}}' fi ;; @@ -49,15 +59,26 @@ struct SetupResult { output: String, actions: String, prompts: usize, + success: bool, } fn setup(installed: &str, answer: &str, handoff: bool) -> SetupResult { + setup_with_strict_host_key_failure(installed, answer, handoff, false) +} + +fn setup_with_strict_host_key_failure( + installed: &str, + answer: &str, + handoff: bool, + strict_host_key_failure: bool, +) -> SetupResult { let root = std::path::PathBuf::from(format!( - "/var/tmp/herdr-machine-setup-{}-{}-{}-{}", + "/var/tmp/herdr-machine-setup-{}-{}-{}-{}-{}", std::process::id(), installed, answer.trim().is_empty(), - handoff + handoff, + strict_host_key_failure )); let app = if cfg!(debug_assertions) { "herdr-dev" @@ -97,6 +118,10 @@ fn setup(installed: &str, answer: &str, handoff: bool) -> SetupResult { command.env("FAKE_ROOT", &root); command.env("FAKE_INSTALLED", installed); command.env( + "FAKE_STRICT_HOST_KEY_FAILURE", + if strict_host_key_failure { "yes" } else { "no" }, + ); + command.env( "FAKE_CLIENT_STATUS", String::from_utf8(status.stdout).unwrap(), ); @@ -151,7 +176,7 @@ fn setup(installed: &str, answer: &str, handoff: bool) -> SetupResult { } } } - child.wait().unwrap(); + let status = child.wait().unwrap(); drop(writer); drop(pair.master); reading.join().unwrap(); @@ -172,10 +197,36 @@ fn setup(installed: &str, answer: &str, handoff: bool) -> SetupResult { output, actions, prompts, + success: status.success(), } } #[test] +fn machine_add_reports_strict_host_key_failure() { + let result = setup_with_strict_host_key_failure("new", "", false, true); + assert!(!result.success, "{}", result.output); + assert_eq!(result.prompts, 0, "{}", result.output); + assert!(result.actions.is_empty(), "{}", result.output); + assert!( + result.output.contains("Host key verification failed"), + "{}", + result.output + ); + assert!( + result + .output + .contains("saved machines use strict host-key checking"), + "{}", + result.output + ); + assert!( + !result.output.contains("lost connection to server"), + "{}", + result.output + ); +} + +#[test] fn machine_setup_old_install_requires_one_explicit_stop_approval() { let result = setup("old", "y\n", false); assert_eq!(result.prompts, 1, "{}", result.output); |