diff --git a/src/remote/attach.rs b/src/remote/attach.rs index 042676d0..4f8d2b7c 100644 --- a/src/remote/attach.rs +++ b/src/remote/attach.rs @@ -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 { + 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> { + 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 { normalize_remote_stdout(&mut output.stdout, output.status.success())?; Ok(output) diff --git a/tests/remote_attach.rs b/tests/remote_attach.rs new file mode 100644 index 00000000..b99e4335 --- /dev/null +++ b/tests/remote_attach.rs @@ -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, + reader: Option>, +} + +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::>(); + + 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()); +}