From 94d0c35e6b643cb519ec89709ed047d5d3ed7be4 Mon Sep 17 00:00:00 2001 From: Overlord Date: Fri, 14 Aug 2026 16:54:04 +0200 Subject: [PATCH] Fix SSH issue (-o ClearAllForwardings=yes) in forced flags, harden ssh.rs (block callers own ~/.ssh/config, force every jump to be self-contained); updated test suite --- src/ssh.rs | 162 +++++++++++++++++++++++++++++++++------------ src/supervisor.rs | 12 ++-- tests/live_test.sh | 80 ++++++++++++++++------ 3 files changed, 190 insertions(+), 64 deletions(-) diff --git a/src/ssh.rs b/src/ssh.rs index e3c56d2..f055ac6 100644 --- a/src/ssh.rs +++ b/src/ssh.rs @@ -1,62 +1,107 @@ //! Builds the `ssh` invocation for a profile. +use std::path::PathBuf; +use std::fmt::Write as _; use std::process::{ Stdio, Command }; use crate::profile::Profile; +/// A throwaway `ssh_config` applying the same hardening (and identity, if +/// any) to every host, so `-J`'s inner proxy connection picks it up too +/// instead of falling back to default identities and prompting for +/// host-key confirmation on a `/dev/null`-less stdin. Deleted on drop, so +/// callers just need to keep this alive for as long as the `ssh` process +/// that reads it runs. +pub struct JumpConfig(PathBuf); + +impl JumpConfig +{ + /// Named after the profile and this process's pid rather than + /// something unique per call, so a long-lived supervisor's repeated + /// reconnect attempts overwrite the same file instead of littering a + /// new one on every retry. + fn write(profile: &Profile) -> Option + { + let mut body = String::from("Host *\n"); + for (key, value) in forced_options(profile) { writeln!(body, "\t{key} {value}").ok()?; } + if let Some(identity) = &profile.identity + { + writeln!(body, "\tIdentityFile {identity}").ok()?; + writeln!(body, "\tIdentitiesOnly yes").ok()?; + } + + let path = std::env::temp_dir().join(format!("porthole-{}-{}.sshconfig", profile.name, std::process::id())); + std::fs::write(&path, body).ok()?; + Some(Self(path)) + } + + fn arg(&self) -> &str { self.0.to_str().unwrap_or("/dev/null") } +} + +impl Drop for JumpConfig +{ + fn drop(&mut self) { let _ = std::fs::remove_file(&self.0); } +} + + +// + +/// The `-o` options forced on every invocation, not user-configurable. +/// Shared by `build()` (as command-line `-o key=value` args) and +/// `JumpConfig::write` (as `ssh_config` lines), so the two representations +/// of "what's forced" can't drift apart. +fn forced_options(profile: &Profile) -> [(&'static str, String); 9] +{ + [ + ("BatchMode", "yes".into()), + ("StrictHostKeyChecking", "accept-new".into()), + ("LogLevel", "ERROR".into()), + ("ExitOnForwardFailure", "yes".into()), + ("ConnectTimeout", "10".into()), + ("ServerAliveCountMax", "3".into()), + ("ControlMaster", "no".into()), + ("ControlPath", "none".into()), + ("ServerAliveInterval", profile.keepalive.to_string()), + ] +} + /// Builds the `ssh` command for `profile`, stdio wired for the supervisor /// to capture (stdout/stderr piped so failure text can be classified; -/// stdin from `/dev/null` since porthole never wants a shell). -pub fn build(profile: &Profile) -> Command { +/// stdin from `/dev/null` since porthole never wants a shell). The second +/// return value, when present, must outlive the spawned process: it owns +/// the config file `-F` points at and deletes it on drop. +pub fn build(profile: &Profile) -> (Command, Option) +{ let mut cmd = Command::new("ssh"); cmd.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped()); - // Flags forced on every invocation, not user-configurable. - cmd.args([ - "-o", - "BatchMode=yes", - "-o", - "StrictHostKeyChecking=accept-new", - "-o", - "LogLevel=ERROR", - "-o", - "ExitOnForwardFailure=yes", - "-o", - "ConnectTimeout=10", - "-o", - "ServerAliveCountMax=3", - "-o", - "ControlMaster=no", - "-o", - "ControlPath=none", - "-o", - "ClearAllForwardings=yes", - "-o", - &format!("ServerAliveInterval={}", profile.keepalive), - "-N", - "-T", - ]); - let (jumps, target) = profile.ssh_target(); - if let Some(jumps) = jumps { - cmd.args(["-J", &jumps]); - } + + let jump_config = jumps.is_some().then(|| JumpConfig::write(profile)).flatten(); + let config_arg = jump_config.as_ref().map_or("/dev/null", |c| c.arg()); + + cmd.args(["-F", config_arg]); + for (key, value) in forced_options(profile) { cmd.args(["-o", &format!("{key}={value}")]); } + cmd.args(["-N", "-T"]); + + if let Some(jumps) = jumps { cmd.args(["-J", &jumps]); } cmd.arg("-p").arg(profile.ssh_port.to_string()); - if let Some(identity) = &profile.identity { + if let Some(identity) = &profile.identity + { cmd.arg("-i").arg(identity); cmd.args(["-o", "IdentitiesOnly=yes"]); } - if let Some(user) = &profile.user { - cmd.arg("-l").arg(user); - } + if let Some(user) = &profile.user { cmd.arg("-l").arg(user); } cmd.arg(profile.kind.ssh_flag()).arg(&profile.mapping); cmd.arg(target); - cmd + (cmd, jump_config) } +// + #[cfg(test)] mod tests { use super::*; @@ -75,17 +120,50 @@ mod tests { .unwrap() } + #[test] + fn ignores_the_callers_own_ssh_config() { + let (cmd, _guard) = build(&profile_with(vec!["jumpbox"])); + let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); + assert!(args.windows(2).any(|w| w == ["-F", "/dev/null"])); + } + #[test] fn single_hop_has_no_dash_j() { - let cmd = build(&profile_with(vec!["jumpbox"])); + let (cmd, _guard) = build(&profile_with(vec!["jumpbox"])); let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); assert!(!args.contains(&"-J".to_string())); assert_eq!(args.last(), Some(&"jumpbox".to_string())); } + #[test] + fn multi_hop_writes_a_config_carrying_hardening_and_identity_to_every_hop() { + let p = Profile::new( + "t2".into(), + &ProfileEdits { + local: Some("5432:db.internal:5432".into()), + via: Some(vec!["bastion1".into(), "bastion2".into()]), + identity: Some("/home/me/.ssh/id_ed25519".into()), + ..Default::default() + }, + ) + .unwrap(); + let (cmd, guard) = build(&p); + let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); + let f_idx = args.iter().position(|a| a == "-F").expect("-F present"); + let config_path = &args[f_idx + 1]; + assert_ne!(config_path, "/dev/null"); + let contents = std::fs::read_to_string(config_path).expect("config file should exist"); + assert!(contents.contains("BatchMode yes")); + assert!(contents.contains("StrictHostKeyChecking accept-new")); + assert!(contents.contains("IdentityFile /home/me/.ssh/id_ed25519")); + assert!(contents.contains("IdentitiesOnly yes")); + drop(guard); + assert!(!std::path::Path::new(config_path).exists()); + } + #[test] fn multi_hop_splits_jumps_from_target() { - let cmd = build(&profile_with(vec!["bastion1", "bastion2:2222"])); + let (cmd, _guard) = build(&profile_with(vec!["bastion1", "bastion2:2222"])); let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); let j_idx = args.iter().position(|a| a == "-J").expect("-J present"); assert_eq!(args[j_idx + 1], "bastion1"); @@ -96,7 +174,7 @@ mod tests { fn includes_forward_flag_and_mapping() { let p = profile_with(vec!["jumpbox"]); assert_eq!(p.kind, Kind::Local); - let cmd = build(&p); + let (cmd, _guard) = build(&p); let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); let l_idx = args.iter().position(|a| a == "-L").expect("-L present"); assert_eq!(args[l_idx + 1], "5432:db.internal:5432"); @@ -114,7 +192,8 @@ mod tests { }, ) .unwrap(); - let args: Vec = build(&p).get_args().map(|a| a.to_string_lossy().into_owned()).collect(); + let (cmd, _guard) = build(&p); + let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); let i_idx = args.iter().position(|a| a == "-i").expect("-i present"); assert_eq!(args[i_idx + 1], "/home/me/.ssh/id_ed25519"); assert!(args.windows(2).any(|w| w == ["-o", "IdentitiesOnly=yes"])); @@ -124,7 +203,8 @@ mod tests { fn identities_only_absent_without_identity() { let p = profile_with(vec!["jumpbox"]); assert!(p.identity.is_none()); - let args: Vec = build(&p).get_args().map(|a| a.to_string_lossy().into_owned()).collect(); + let (cmd, _guard) = build(&p); + let args: Vec = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); assert!(!args.windows(2).any(|w| w == ["-o", "IdentitiesOnly=yes"])); } } diff --git a/src/supervisor.rs b/src/supervisor.rs index 54a54b2..941e370 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -57,8 +57,9 @@ enum Outcome { /// Entry point for `porthole __supervise `. This function is the /// supervisor process: it runs until told to stop (SIGTERM/SIGINT) or -/// gives up per §4. -pub fn run(name: &str) -> Result<()> { +/// gives up. +pub fn run(name: &str) -> Result<()> +{ install_signal_handler(); let profile = profile::load(name)?; @@ -154,13 +155,16 @@ fn sleep_or_shutdown(dur: Duration) -> bool /// requested. Marks `inst` as `State::Up` once the process has survived /// `CONNECT_GRACE`. `ssh` does not report "the forward is bound" directly /// without parsing `-v` debug output; a real failure exits near-instantly -/// under `ExitOnForwardFailure=yes` (§3.1), so staying alive past the grace +/// under `ExitOnForwardFailure=yes`, so staying alive past the grace /// window is used as a proxy for connected. fn run_ssh_once(name: &str, profile: &Profile, inst: &mut Instance) -> Outcome { rotate_log_if_large(name); - let mut cmd = ssh::build(profile); + // `_jump_config`, when present, must stay alive for this whole + // function: it owns the config file `-F` points ssh at, and every + // return path below runs the ssh process to completion first. + let (mut cmd, _jump_config) = ssh::build(profile); let mut child = match cmd.spawn() { Ok(c) => c, Err(e) => return Outcome::Failed { class: Class::Unrecognized, message: format!("failed to spawn ssh: {e}") }, diff --git a/tests/live_test.sh b/tests/live_test.sh index 6c5f384..45de1c0 100755 --- a/tests/live_test.sh +++ b/tests/live_test.sh @@ -120,7 +120,16 @@ p_run() { LAST_OUT="$(PORTHOLE_STATE_DIR_OVERRIDE="$1" "$BIN" "${@:2}" 2>&1)"; L # status.rs); polling that key is far more robust than scraping the padded # human-readable field. json_field() { # json_field -> value, empty if absent/no instance - p "$1" status "$2" --json 2>/dev/null | sed -n "s/.*\"$3\": \"\\{0,1\\}\\([^\",]*\\)\"\\{0,1\\},\\{0,1\\}\$/\\1/p" | head -1 + local out v + out="$(p "$1" status "$2" --json 2>/dev/null)" + # Quoted string value first: the closing quote is mandatory here (unlike + # a bare [^",]* class) so a value with an embedded comma - a real ssh + # "Permission denied (publickey,password)." error has one - isn't + # truncated at the first comma instead of its actual end. + v="$(sed -n "s/.*\"$3\": \"\\(.*\\)\",\\{0,1\\}\$/\\1/p" <<<"$out" | head -1)" + if [[ -n "$v" ]]; then echo "$v"; return; fi + # Bare (unquoted) value: number, bool, or null. + sed -n "s/.*\"$3\": \\([^\",]*\\),\\{0,1\\}\$/\\1/p" <<<"$out" | head -1 } wait_for_state() { # wait_for_state [tries, x0.5s] local dir="$1" name="$2" want="$3" tries="${4:-20}" @@ -238,7 +247,7 @@ p_run "$STATE_DIR" status "${NAME}_local"; expect_contains "fresh profile is clo p_run "$STATE_DIR" open "${NAME}_local"; expect_exit "'open ${NAME}_local'" 0 assert_true "'${NAME}_local' reaches state: up" wait_for_state "$STATE_DIR" "${NAME}_local" up 10 -if ssh "${SSH_PROBE_OPTS[@]}" -p "$LOCAL_PORT" localhost true 2>/tmp/porthole_test_probe.$$; then +if ssh "${SSH_PROBE_OPTS[@]}" -i "$IDENTITY" -o IdentitiesOnly=yes -l "$USER_" -p "$LOCAL_PORT" localhost true 2>/tmp/porthole_test_probe.$$; then pass "forwarded port $LOCAL_PORT actually round-trips to the real sshd" else fail "forwarded port $LOCAL_PORT actually round-trips to the real sshd" "$(cat /tmp/porthole_test_probe.$$ 2>/dev/null)" @@ -259,9 +268,15 @@ expect_exit "'add ${NAME}_remote'" 0 p_run "$STATE_DIR" open "${NAME}_remote"; expect_exit "'open ${NAME}_remote'" 0 assert_true "'${NAME}_remote' reaches state: up" wait_for_state "$STATE_DIR" "${NAME}_remote" up 10 -remote_check="$(ssh "${SSH_PROBE_OPTS[@]}" -i "$IDENTITY" "$USER_@$HOST" \ - "ssh -p $REMOTE_PORT -o BatchMode=yes -o ConnectTimeout=5 -o StrictHostKeyChecking=accept-new -o UserKnownHostsFile=/dev/null localhost true && echo REMOTE_FORWARD_OK" 2>&1)" -if [[ "$remote_check" == *REMOTE_FORWARD_OK* ]]; then +# The inner ssh below runs on the remote box itself (reached through the +# outer session), then loops back out through the -R forward to a real +# sshd on this same server - $IDENTITY is a local path and won't exist +# there, so it has no credentials for that final hop. Reaching the real +# sshd and being rejected already proves the forward round-trips; a +# refused/timed-out connection is what would indicate it's actually broken. +remote_check="$(ssh "${SSH_PROBE_OPTS[@]}" -i "$IDENTITY" -o IdentitiesOnly=yes "$USER_@$HOST" \ + "ssh -p $REMOTE_PORT -o BatchMode=yes -o ConnectTimeout=5 -o StrictHostKeyChecking=accept-new -o UserKnownHostsFile=/dev/null localhost true; echo EXIT:\$?" 2>&1)" +if [[ "$remote_check" == *"EXIT:0"* || "$remote_check" == *"Permission denied"* ]]; then pass "remote-bound port $REMOTE_PORT round-trips back out through the tunnel" else fail "remote-bound port $REMOTE_PORT round-trips back out through the tunnel" "$remote_check" @@ -298,19 +313,30 @@ fi # --------------------------------------------------------------------------- section "Phase 6: multi-hop --via (self-jump - see header comment for why)" # --------------------------------------------------------------------------- +# A real ssh refuses a ProxyJump hop that's textually identical to the +# final target ("jumphost loop via ..."), so USER@HOST,USER@HOST can never +# work no matter how porthole builds the command - it's rejected before a +# connection is even attempted. Using the server's IP for the first hop and +# its hostname for the final target is the same physical box (still a real +# 2-hop handshake) but sidesteps that string-identity check. HOP_PORT=28223 -p_run "$STATE_DIR" add "${NAME}_multihop" -l "$HOP_PORT:localhost:22" -i "$IDENTITY" --via "$USER_@$HOST,$USER_@$HOST" -expect_exit "'add ${NAME}_multihop' with a 2-hop --via" 0 -p_run "$STATE_DIR" open "${NAME}_multihop"; expect_exit "'open ${NAME}_multihop'" 0 -assert_true "'${NAME}_multihop' reaches state: up (real -J handshake, twice)" wait_for_state "$STATE_DIR" "${NAME}_multihop" up 30 - -if ssh "${SSH_PROBE_OPTS[@]}" -p "$HOP_PORT" localhost true 2>/tmp/porthole_test_probe.$$; then - pass "forwarded port round-trips through the 2-hop chain" +HOST_IP="$(getent ahostsv4 "$HOST" 2>/dev/null | awk '{print $1; exit}')" +if [[ -z "$HOST_IP" ]]; then + skip "multi-hop phase (could not resolve $HOST to an IP for the loop workaround)" else - fail "forwarded port round-trips through the 2-hop chain" "$(cat /tmp/porthole_test_probe.$$ 2>/dev/null)" + p_run "$STATE_DIR" add "${NAME}_multihop" -l "$HOP_PORT:localhost:22" -i "$IDENTITY" --via "$USER_@$HOST_IP,$USER_@$HOST" + expect_exit "'add ${NAME}_multihop' with a 2-hop --via" 0 + p_run "$STATE_DIR" open "${NAME}_multihop"; expect_exit "'open ${NAME}_multihop'" 0 + assert_true "'${NAME}_multihop' reaches state: up (real -J handshake, twice)" wait_for_state "$STATE_DIR" "${NAME}_multihop" up 30 + + if ssh "${SSH_PROBE_OPTS[@]}" -i "$IDENTITY" -o IdentitiesOnly=yes -l "$USER_" -p "$HOP_PORT" localhost true 2>/tmp/porthole_test_probe.$$; then + pass "forwarded port round-trips through the 2-hop chain" + else + fail "forwarded port round-trips through the 2-hop chain" "$(cat /tmp/porthole_test_probe.$$ 2>/dev/null)" + fi + rm -f "/tmp/porthole_test_probe.$$" 2>/dev/null + p_run "$STATE_DIR" close "${NAME}_multihop"; expect_exit "'close ${NAME}_multihop'" 0 fi -rm -f "/tmp/porthole_test_probe.$$" 2>/dev/null -p_run "$STATE_DIR" close "${NAME}_multihop"; expect_exit "'close ${NAME}_multihop'" 0 # --------------------------------------------------------------------------- section "Phase 7: -u/--user without an embedded via user - real -l flag auth" @@ -349,8 +375,15 @@ expect_exit "'add ${NAME}_deadport' targeting a closed port on the real host" 0 p_run "$STATE_DIR" open "${NAME}_deadport"; expect_exit "'open ${NAME}_deadport' (backgrounds even though the first attempt fails)" 0 assert_true "'${NAME}_deadport' keeps reconnecting rather than giving up" wait_for_state "$STATE_DIR" "${NAME}_deadport" reconnecting 20 rc1="$(json_field "$STATE_DIR" "${NAME}_deadport" reconnect_count)" -sleep 6 -rc2="$(json_field "$STATE_DIR" "${NAME}_deadport" reconnect_count)" +# Port 9 may be silently dropped rather than actively refused, so a single +# attempt can burn the full ConnectTimeout=10s; poll well past worst-case +# instead of a fixed sleep that assumes an instant refusal. +rc2="$rc1" +for _ in $(seq 1 40); do + rc2="$(json_field "$STATE_DIR" "${NAME}_deadport" reconnect_count)" + [[ -n "$rc2" && "$rc2" -gt "${rc1:-0}" ]] && break + sleep 0.5 +done if [[ -n "$rc2" && "$rc2" -gt "${rc1:-0}" ]]; then pass "reconnect_count keeps increasing on a real refused connection ($rc1 -> $rc2)" else @@ -455,8 +488,17 @@ else expect_contains "wipe reports what it did" "Wiped all forwards" p_run "$STATE_DIR" list; expect_contains "'list' is empty after wipe" "No profiles saved." - assert_true "the phase-13 orphan is gone too (kill_orphaned_supervisors)" bash -c \ - "! pgrep -f '__supervise ${NAME}_keep\$' >/dev/null" + # kill_orphaned_supervisors only sends SIGTERM and returns; the orphan's + # own signal handler needs a moment to shut down, so give it a few + # retries rather than checking the instant `wipe` returns. + wait_for_orphan_gone() { + for _ in $(seq 1 10); do + pgrep -f "__supervise ${NAME}_keep\$" >/dev/null || return 0 + sleep 0.3 + done + return 1 + } + assert_true "the phase-13 orphan is gone too (kill_orphaned_supervisors)" wait_for_orphan_gone assert_true "port $KEEP_PORT is no longer listening" wait_port_closed "$KEEP_PORT" 6 fi fi