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

@@ -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<Self>
+    {
+        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).
+/// stdin from `/dev/null` since porthole never wants a shell). The second
-pub fn build(profile: &Profile) -> Command {
+/// 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<JumpConfig>)
+{
     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 {
+    if let Some(user) = &profile.user { cmd.arg("-l").arg(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<String> = 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<String> = 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<String> = 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<String> = 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<String> = 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<String> = build(&p).get_args().map(|a| a.to_string_lossy().into_owned()).collect();
+        let (cmd, _guard) = build(&p);
+        let args: Vec<String> = 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<String> = build(&p).get_args().map(|a| a.to_string_lossy().into_owned()).collect();
+        let (cmd, _guard) = build(&p);
+        let args: Vec<String> = cmd.get_args().map(|a| a.to_string_lossy().into_owned()).collect();
         assert!(!args.windows(2).any(|w| w == ["-o", "IdentitiesOnly=yes"]));
     }
 }

src/supervisor.rs

@@ -57,8 +57,9 @@ enum Outcome {
 
 /// Entry point for `porthole __supervise <name>`. This function is the
 /// supervisor process: it runs until told to stop (SIGTERM/SIGINT) or
-/// gives up per §4.
+/// gives up.
-pub fn run(name: &str) -> Result<()> {
+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}") },

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 <state-dir> <name> <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 <state-dir> <name> <want-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" \
+# The inner ssh below runs on the remote box itself (reached through the
-  "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)"
+# outer session), then loops back out through the -R forward to a real
-if [[ "$remote_check" == *REMOTE_FORWARD_OK* ]]; then
+# 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"
+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
+  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[@]}" -p "$HOP_PORT" localhost true 2>/tmp/porthole_test_probe.$$; then
+  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
 
 # ---------------------------------------------------------------------------
 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
+# 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 \
+    # kill_orphaned_supervisors only sends SIGTERM and returns; the orphan's
-      "! pgrep -f '__supervise ${NAME}_keep\$' >/dev/null"
+    # 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