fix(remote): show SSH authentication output (#3609)

* fix(remote): show SSH authentication output refs #3606 * test(remote): stabilize SSH authentication output regression refs #3606 --------- Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Co-authored-by: JJ Liebig <jonathan.liebig@gmail.com>
2026-09-22 00:01:06 +00:00 · 2026-09-11 21:12:49 +04:00
parent eba7758ced
commit 1a7c691559
2 changed files with 233 additions and 8 deletions
@@ -4,9 +4,9 @@ use super::{args::*, process::wait_with_output_timeout, restart_policy::*, shell
 use base64::Engine as _;
 use std::collections::BTreeMap;
 use std::fs::{self, File};
-use std::io::{self, IsTerminal, Write as _};
+use std::io::{self, IsTerminal, Read as _, Write as _};
 use std::path::{Path, PathBuf};
-use std::process::{Command, Output, Stdio};
+use std::process::{Child, Command, Output, Stdio};

 use interprocess::local_socket::traits::Listener as _;
 #[cfg(all(test, unix))]
@@ -594,6 +594,13 @@ impl RemoteSsh {
            .stderr(Stdio::piped())
            .spawn()?;

+        if !self.noninteractive {
+            return normalize_remote_output(output_with_forwarded_stderr(
+                child,
+                Some(script.as_bytes()),
+            )?);
+        }
+
        let write_result = if let Some(mut stdin) = child.stdin.take() {
            stdin.write_all(script.as_bytes())
        } else {
@@ -602,11 +609,7 @@ impl RemoteSsh {
                "ssh bootstrap stdin missing",
            ))
        };
-        let output = if self.noninteractive {
-            wait_with_output_timeout(child, NONINTERACTIVE_SSH_COMMAND_TIMEOUT)?
-        } else {
-            child.wait_with_output()?
-        };
+        let output = wait_with_output_timeout(child, NONINTERACTIVE_SSH_COMMAND_TIMEOUT)?;
        write_result?;
        normalize_remote_output(output)
    }
@@ -623,7 +626,7 @@ impl RemoteSsh {
        let output = if self.noninteractive {
            wait_with_output_timeout(command.spawn()?, NONINTERACTIVE_SSH_COMMAND_TIMEOUT)
        } else {
-            command.output()
+            output_with_forwarded_stderr(command.spawn()?, None)
        }?;
        normalize_remote_output(output)
    }
@@ -685,6 +688,55 @@ impl RemoteSsh {
    }
 }

+// Only interactive setup uses this relay. Background probes retain their
+// capture-only timeout path so SSH diagnostics cannot overwrite the active TUI.
+fn output_with_forwarded_stderr(mut child: Child, stdin: Option<&[u8]>) -> io::Result<Output> {
+    let mut child_stderr = child
+        .stderr
+        .take()
+        .ok_or_else(|| io::Error::new(io::ErrorKind::BrokenPipe, "ssh command stderr missing"))?;
+    let stderr_relay = thread::spawn(move || -> io::Result<Vec<u8>> {
+        let mut captured = Vec::new();
+        let mut buffer = [0_u8; 8 * 1024];
+        let mut destination = io::stderr();
+
+        loop {
+            let read = child_stderr.read(&mut buffer)?;
+            if read == 0 {
+                break;
+            }
+            captured.extend_from_slice(&buffer[..read]);
+            if destination.write_all(&buffer[..read]).is_ok() {
+                let _ = destination.flush();
+            }
+        }
+
+        Ok(captured)
+    });
+
+    let write_result = if let Some(bytes) = stdin {
+        if let Some(mut child_stdin) = child.stdin.take() {
+            child_stdin.write_all(bytes)
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::BrokenPipe,
+                "ssh bootstrap stdin missing",
+            ))
+        }
+    } else {
+        Ok(())
+    };
+    let output_result = child.wait_with_output();
+    let stderr_result = stderr_relay
+        .join()
+        .map_err(|_| io::Error::other("ssh stderr relay panicked"))?;
+
+    let mut output = output_result?;
+    write_result?;
+    output.stderr = stderr_result?;
+    Ok(output)
+}
+
 fn normalize_remote_output(mut output: Output) -> io::Result<Output> {
    normalize_remote_stdout(&mut output.stdout, output.status.success())?;
    Ok(output)
@@ -0,0 +1,173 @@
+#![cfg(unix)]
+
+use std::fs;
+use std::io::{BufRead, BufReader};
+use std::os::unix::fs::PermissionsExt;
+use std::os::unix::process::CommandExt;
+use std::path::{Path, PathBuf};
+use std::process::{Child, Command, Stdio};
+use std::sync::mpsc;
+use std::thread;
+use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH};
+
+const CHECK_NOTICE: &str = "# Tailscale SSH requires an additional check.";
+const CHECK_URL: &str = "# To authenticate, visit: https://login.tailscale.com/a/test";
+const LATER_FAILURE: &str = "ssh: later setup probe failed";
+
+struct TestCleanup {
+    temp_dir: PathBuf,
+    child: Option<Child>,
+    reader: Option<thread::JoinHandle<()>>,
+}
+
+impl Drop for TestCleanup {
+    fn drop(&mut self) {
+        if let Some(mut child) = self.child.take() {
+            // The child leads a private process group and has not been reaped.
+            // Kill its fake SSH descendants too, not just the Herdr launcher.
+            // SAFETY: the negative PID targets only this test's process group.
+            unsafe { libc::kill(-(child.id() as libc::pid_t), libc::SIGKILL) };
+            let _ = child.wait();
+        }
+        if let Some(reader) = self.reader.take() {
+            let _ = reader.join();
+        }
+        let _ = fs::remove_dir_all(&self.temp_dir);
+    }
+}
+
+fn wait_for_file(path: &Path, timeout: Duration) {
+    let deadline = Instant::now() + timeout;
+    while !path.exists() {
+        assert!(Instant::now() < deadline, "timed out waiting for fake ssh");
+        thread::sleep(Duration::from_millis(10));
+    }
+}
+
+#[test]
+fn ssh_check_message_is_visible_while_authentication_waits() {
+    check_authentication_output(false);
+    check_authentication_output(true);
+}
+
+fn check_authentication_output(framed_shell: bool) {
+    let nonce = SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .expect("system clock after Unix epoch")
+        .as_nanos();
+    let temp_dir = std::env::temp_dir().join(format!(
+        "herdr-remote-auth-test-{}-{nonce}",
+        std::process::id()
+    ));
+    let mut cleanup = TestCleanup {
+        temp_dir: temp_dir.clone(),
+        child: None,
+        reader: None,
+    };
+    fs::create_dir_all(&temp_dir).expect("create test directory");
+
+    let started_path = temp_dir.join("ssh-started");
+    let approval_path = temp_dir.join("ssh-approved");
+    let advanced_path = temp_dir.join("ssh-advanced");
+    let first_done_path = temp_dir.join("ssh-first-done");
+    let ssh_path = temp_dir.join("ssh");
+    fs::write(
+        &ssh_path,
+        format!(
+            r#"#!/bin/sh
+authenticate() {{
+    : > "$FAKE_SSH_STARTED"
+    printf '%s\n%s\n' '{CHECK_NOTICE}' '{CHECK_URL}' >&2
+    while [ ! -e "$FAKE_SSH_APPROVED" ]; do
+        /bin/sleep 0.01
+    done
+}}
+if [ ! -e "$FAKE_SSH_FIRST_DONE" ]; then
+    : > "$FAKE_SSH_FIRST_DONE"
+    if [ "$FAKE_SSH_FRAMED" = 0 ]; then authenticate; fi
+    /bin/cat >/dev/null
+    printf 'login banner\nherdr-remote-output-ready:1\nLinux\nx86_64\n'
+    exit 0
+fi
+if [ "$FAKE_SSH_FRAMED" = 1 ] && [ ! -e "$FAKE_SSH_STARTED" ]; then
+    authenticate
+fi
+/bin/cat >/dev/null
+: > "$FAKE_SSH_ADVANCED"
+printf '%s\n' '{LATER_FAILURE}' >&2
+exit 255
+"#
+        ),
+    )
+    .expect("write fake ssh");
+    fs::set_permissions(&ssh_path, fs::Permissions::from_mode(0o755))
+        .expect("make fake ssh executable");
+
+    let inherited_path = std::env::var("PATH").unwrap_or_default();
+    let path = format!("{}:{inherited_path}", temp_dir.display());
+    let child = Command::new(env!("CARGO_BIN_EXE_herdr"))
+        .args(["--remote", "check-host"])
+        .env("PATH", path)
+        .env("FAKE_SSH_FRAMED", if framed_shell { "1" } else { "0" })
+        .env("FAKE_SSH_STARTED", &started_path)
+        .env("FAKE_SSH_APPROVED", &approval_path)
+        .env("FAKE_SSH_ADVANCED", &advanced_path)
+        .env("FAKE_SSH_FIRST_DONE", &first_done_path)
+        .env("HERDR_CONFIG_PATH", temp_dir.join("config.toml"))
+        .env_remove("HERDR_ENV")
+        .env_remove("HERDR_SESSION")
+        .env_remove("HERDR_SOCKET_PATH")
+        .env_remove("HERDR_CLIENT_SOCKET_PATH")
+        .env_remove("HERDR_REMOTE_BINARY")
+        .stdin(Stdio::null())
+        .stdout(Stdio::null())
+        .stderr(Stdio::piped())
+        .process_group(0)
+        .spawn()
+        .expect("start remote attach");
+
+    cleanup.child = Some(child);
+    let child = cleanup.child.as_mut().expect("registered child");
+    let stderr = child.stderr.take().expect("remote attach stderr");
+    let (line_tx, line_rx) = mpsc::channel();
+    cleanup.reader = Some(thread::spawn(move || {
+        for line in BufReader::new(stderr).lines() {
+            if line_tx
+                .send(line.expect("read remote attach stderr"))
+                .is_err()
+            {
+                break;
+            }
+        }
+    }));
+
+    wait_for_file(&started_path, Duration::from_secs(2));
+    let notice = line_rx.recv_timeout(Duration::from_secs(2));
+    let url = line_rx.recv_timeout(Duration::from_secs(2));
+    fs::write(&approval_path, b"approved").expect("release fake ssh approval");
+    wait_for_file(&advanced_path, Duration::from_secs(2));
+
+    let status = child.wait().expect("wait for remote attach");
+    cleanup.child = None;
+    cleanup
+        .reader
+        .take()
+        .expect("registered stderr reader")
+        .join()
+        .expect("join stderr reader");
+    let later_lines = line_rx.try_iter().collect::<Vec<_>>();
+
+    assert_eq!(notice.as_deref(), Ok(CHECK_NOTICE));
+    assert_eq!(url.as_deref(), Ok(CHECK_URL));
+    assert!(
+        later_lines.iter().any(|line| line == LATER_FAILURE),
+        "later SSH stderr should also be visible: {later_lines:?}"
+    );
+    assert!(
+        later_lines.iter().any(|line| {
+            line.contains("error: remote binary discovery failed") && line.contains(LATER_FAILURE)
+        }),
+        "failed SSH stderr should remain in the contextual error: {later_lines:?}"
+    );
+    assert!(!status.success());
+}