diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a66e8..bc1a08d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,14 @@ ### Fixes +- **Guest transports fail instead of hanging when a VM stops responding** — A + paused VM, a wedged sshd, or a lost TAP device left `ssh`, `scp`, and `rsync` + calls blocked on a dead socket with no deadline, so lifecycle commands, + `coop exec`, and `coop push`/`pull` hung until interrupted. Every transport + now derives from one option list that sets `BatchMode`, a connect timeout, + and a liveness probe, so a guest whose sshd stops answering fails after ~90s + — the bound interactive sessions already had. + - **Install Codex's complete runtime package** (#442) — Recent Codex releases use a companion `codex-code-mode-host` executable, but coop installed only the raw `codex` binary, causing Code Mode to fail closed at startup. Image diff --git a/docs/trust-model.md b/docs/trust-model.md index 2fd27f9..5aabc27 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -132,7 +132,11 @@ user `env_forward` entries, and the VM SSH key. The invariants: - coop connects to the guest with `StrictHostKeyChecking=no`, `UserKnownHostsFile=/dev/null`, `IdentitiesOnly=yes` - (`backend.rs:SshTarget::ssh_opts`, `workspace.rs:ssh_config_block`). This is + (`backend.rs:SshTarget::transport_opts` — the one list `ssh`, `scp`, and + rsync's `-e` all derive from — and `workspace.rs:ssh_config_block`). coop's + own transports add `BatchMode=yes`, so a rejected key fails instead of + falling back to a password prompt; the `~/.ssh/config` block written for the + user's own `ssh coop-` deliberately does not. This is deliberate: guest keys are ephemeral and regenerated per VM, so there is no stable host key to pin. The trade-off is that a MITM on the path to the guest is not detected — acceptable because that path is loopback / a local TAP link diff --git a/src/backend.rs b/src/backend.rs index 9cae856..922334f 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -337,9 +337,31 @@ impl SshTarget { dir.join(format!("coop-{short:08x}.sock")) } - /// SSH options for commands. - pub fn ssh_opts(&self) -> Vec { + /// Options every transport shares, up to the port flag. + /// + /// `ssh`, `scp`, and rsync's `-e` command all take these; only the port + /// flag differs (`-p` vs `-P`), so all three derive from here instead of + /// hand-copying a list that then drifts. + /// + /// The bounds, in the order they take effect. `ConnectTimeout` caps the + /// TCP connect and banner exchange. `BatchMode` refuses password and + /// passphrase prompts, so a key the guest rejects fails instead of + /// blocking on stdin where no interactive user exists (`coop up` in CI). + /// `ServerAlive*` then covers the established session: a paused VM, a + /// wedged sshd, or a lost TAP device otherwise leaves SSH blocked on a + /// dead socket with no deadline. It is answered by sshd itself, not by + /// the remote command, so a silent hour-long install is never at risk — + /// only a guest whose sshd cannot answer for 90s. + fn transport_opts(&self) -> Vec { vec![ + "-o".into(), + "BatchMode=yes".into(), + "-o".into(), + "ConnectTimeout=10".into(), + "-o".into(), + "ServerAliveInterval=30".into(), + "-o".into(), + "ServerAliveCountMax=3".into(), "-o".into(), "StrictHostKeyChecking=no".into(), "-o".into(), @@ -350,11 +372,16 @@ impl SshTarget { "LogLevel=ERROR".into(), "-i".into(), self.key_path.display().to_string(), - "-p".into(), - self.port.to_string(), ] } + /// SSH options for commands. + pub fn ssh_opts(&self) -> Vec { + let mut opts = self.transport_opts(); + opts.extend(["-p".into(), self.port.to_string()]); + opts + } + /// SSH options with connection multiplexing. /// /// Only used during boot probing (`wait_until_ready`) where rapid @@ -376,21 +403,10 @@ impl SshTarget { /// SCP options (uses -P for port instead of -p). pub fn scp_opts(&self) -> Vec { - vec![ - "-q".into(), - "-o".into(), - "StrictHostKeyChecking=no".into(), - "-o".into(), - "UserKnownHostsFile=/dev/null".into(), - "-o".into(), - "IdentitiesOnly=yes".into(), - "-o".into(), - "LogLevel=ERROR".into(), - "-i".into(), - self.key_path.display().to_string(), - "-P".into(), - self.port.to_string(), - ] + let mut opts = vec!["-q".to_string()]; + opts.extend(self.transport_opts()); + opts.extend(["-P".into(), self.port.to_string()]); + opts } /// user@host address string. @@ -529,13 +545,12 @@ impl SshTarget { } /// SSH command string for rsync's -e flag. + /// + /// Derived from [`Self::ssh_opts`] so a transfer inherits the same bounds + /// as any other guest command. rsync splits this string on whitespace, so + /// it stays unquoted — a key path containing spaces has never worked here. pub fn rsync_ssh_cmd(&self) -> String { - format!( - "ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ - -o IdentitiesOnly=yes -o LogLevel=ERROR -i {} -p {}", - self.key_path.display(), - self.port, - ) + format!("ssh {}", self.ssh_opts().join(" ")) } /// Run a command on the guest via SSH and capture stdout. @@ -3267,6 +3282,61 @@ Filesystem 1M-blocks Used Available Use% Mounted on /dev/vda1 20480 3200 16000 17% / "; + fn ssh_test_target() -> SshTarget { + SshTarget { + host: Hostname::new("192.0.2.1").unwrap(), + port: NonZeroU16::new(22).unwrap(), + user: SshUser::new("ubuntu").unwrap(), + key_path: PathBuf::from("/tmp/test-key"), + } + } + + #[test] + fn every_transport_is_bounded_against_a_wedged_guest() { + let target = ssh_test_target(); + let ssh = target.ssh_opts(); + let scp = target.scp_opts(); + let rsync = target.rsync_ssh_cmd(); + + for bound in [ + "BatchMode=yes", + "ConnectTimeout=10", + "ServerAliveInterval=30", + "ServerAliveCountMax=3", + ] { + assert!(ssh.contains(&bound.to_string()), "{bound} missing from ssh"); + assert!(scp.contains(&bound.to_string()), "{bound} missing from scp"); + assert!(rsync.contains(bound), "{bound} missing from rsync -e"); + } + + // Exactly one pair per transport: OpenSSH honors the first value of a + // repeated `-o`, so a caller that appends its own `ServerAlive*` after + // these would be silently ignored rather than tightening the bound. + for (name, opts) in [ + ("ssh", ssh), + ("scp", scp), + ("mux", target.ssh_opts_mux()), + ("rsync", rsync.split(' ').map(str::to_string).collect()), + ] { + let pairs = opts.iter().filter(|o| o.starts_with("ServerAlive")).count(); + assert_eq!(pairs, 2, "{name} must carry one ServerAlive* pair"); + } + } + + #[test] + fn ssh_and_scp_options_differ_only_in_the_port_flag() { + // Two hand-copied lists is how the last hardening change reached only + // one of them; they now derive from `transport_opts`. + let target = ssh_test_target(); + let ssh = target.ssh_opts(); + let scp = target.scp_opts(); + + assert_eq!(scp.first().map(String::as_str), Some("-q")); + assert_eq!(scp[1..scp.len() - 2], ssh[..ssh.len() - 2]); + assert!(scp.ends_with(&["-P".to_string(), "22".to_string()])); + assert!(ssh.ends_with(&["-p".to_string(), "22".to_string()])); + } + #[test] fn boot_preflight_fails_on_missing_config_dir() { // The boot choke point must reject a config whose custom diff --git a/src/port_forward.rs b/src/port_forward.rs index 7fb5471..199c080 100644 --- a/src/port_forward.rs +++ b/src/port_forward.rs @@ -172,8 +172,8 @@ pub fn spawn_ssh_forwards( // ExitOnForwardFailure=yes: refuse to background if a -L fails to // bind — turns races with `check_host_port_collisions` into // loud errors rather than silently lost forwards. - // ServerAliveInterval=30: detect dead VMs so the forwarder doesn't - // hang around eating descriptors after a hard reboot. + // The `ServerAlive*` bound that keeps this tunnel from outliving a dead + // VM comes from `SshTarget::transport_opts` via `ssh_opts` above. args.extend([ "-f".into(), "-N".into(), @@ -186,8 +186,6 @@ pub fn spawn_ssh_forwards( "ControlPersist=yes".into(), "-o".into(), "ExitOnForwardFailure=yes".into(), - "-o".into(), - "ServerAliveInterval=30".into(), ]); let mut spec_log: Vec = Vec::new(); diff --git a/src/proxy.rs b/src/proxy.rs index 39036a5..ee6e8b8 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -379,8 +379,6 @@ fn spawn_reverse_forward(inst: &Instance, name: &str, target: &SshTarget, port: "-T".into(), "-o".into(), "ExitOnForwardFailure=yes".into(), - "-o".into(), - "ServerAliveInterval=30".into(), "-R".into(), format!("127.0.0.1:{port}:127.0.0.1:{port}"), ]); diff --git a/src/ssh.rs b/src/ssh.rs index cac8f05..3e115ae 100644 --- a/src/ssh.rs +++ b/src/ssh.rs @@ -41,22 +41,6 @@ fn guest_term() -> String { } } -/// Keepalive options for interactive sessions. -/// -/// A paused/suspended VM or a dropped local connection leaves SSH -/// blocked on a dead socket indefinitely. With these, SSH sends a probe -/// every 30s and gives up after 3 unanswered probes — so an unreachable -/// session terminates within ~90s instead of hanging. Scoped to -/// interactive use; the short-lived non-interactive paths don't need it. -fn keepalive_opts() -> [String; 4] { - [ - "-o".into(), - "ServerAliveInterval=30".into(), - "-o".into(), - "ServerAliveCountMax=3".into(), - ] -} - /// Force a known OpenSSH escape character for emergency disconnects. /// /// OpenSSH only recognizes the escape at the start of a line, so users @@ -68,7 +52,6 @@ fn escape_opts() -> [String; 2] { fn interactive_ssh_args(session: &SshSession, remote_cmd: String) -> Vec { let mut args = session.ssh_opts(); - args.extend(keepalive_opts()); args.extend(escape_opts()); args.extend(["-t".to_string(), session.target.addr(), remote_cmd]); args @@ -201,19 +184,6 @@ mod tests { assert_eq!(render_remote(&cmd), "cd /workspace && 'echo' 'hi'"); } - #[test] - fn keepalive_opts_set_interval_and_count() { - assert_eq!( - keepalive_opts(), - [ - "-o", - "ServerAliveInterval=30", - "-o", - "ServerAliveCountMax=3", - ], - ); - } - #[test] fn escape_opts_force_tilde_escape() { assert_eq!(escape_opts(), ["-e", "~"]); @@ -232,9 +202,20 @@ mod tests { env: crate::backend::EnvForward::default(), }; + // The `ServerAlive*` pair comes from `SshTarget::transport_opts`, which + // every transport shares; this session must not add a second one, since + // OpenSSH honors the first value of a repeated `-o`. assert_eq!( interactive_ssh_args(&session, "cd /workspace && 'claude' 'agents'".into()), [ + "-o", + "BatchMode=yes", + "-o", + "ConnectTimeout=10", + "-o", + "ServerAliveInterval=30", + "-o", + "ServerAliveCountMax=3", "-o", "StrictHostKeyChecking=no", "-o", @@ -247,10 +228,6 @@ mod tests { "/tmp/coop-test-key", "-p", "1", - "-o", - "ServerAliveInterval=30", - "-o", - "ServerAliveCountMax=3", "-e", "~", "-t",