fix: show ssh errors during machine setup (#3733)

refs #3731

Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>

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.

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")
@@ -63,6 +79,19 @@ fn shell_quote(value: &str) -> String {
 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(

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)]);
     }
 }
 
@@ -2702,6 +2756,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());

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"
@@ -96,6 +117,10 @@ fn setup(installed: &str, answer: &str, handoff: bool) -> SetupResult {
     command.env("XDG_RUNTIME_DIR", &root);
     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,9 +197,35 @@ 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);