Bound every guest transport against an unresponsive guest - #451
Open
DarkaMaul wants to merge 3 commits into
Open
Bound every guest transport against an unresponsive guest#451DarkaMaul wants to merge 3 commits into
DarkaMaul wants to merge 3 commits into
Conversation
DarkaMaul
marked this pull request as ready for review
September 4, 2026 11:42
The previous commit bounded the `SshTarget`/`SshSession` methods and
stopped there, leaving the other one-shot transports with the same
unbounded hang — and making `coop push`/`pull` worse, since the now-bounded
`which rsync` probe returned `false` for an unresponsive guest and routed
to a tar pipe that hung with no deadline at all.
- `scp_opts` and `rsync_ssh_cmd` were hand-copied option lists that missed
`BatchMode` and `ConnectTimeout`. All three transports now derive from
one `transport_opts`, differing only in the port flag.
- `coop exec`, `coop shell <cmd>`, the tar pipes, and `check_guest_dirty`
go through `SshTarget::command_args`, the single argv seam.
- Split the keepalive by caller: arbitrary-length guest work (installs,
clones, `post_start`) gets the interactive-grade 30/3 (~90s) rather than
a 10s bound an unscheduled sshd can trip mid-install; short retried
probes keep 5/2.
- `SshTarget::probe` returns a third state, so a paused VM no longer
reports as a missing CLI or an image without Secret Service support and
tells the user to rebuild. ssh's own stderr ("Timeout, server not
responding") now survives into the error instead of `/dev/null`.
- Docstrings: `ConnectTimeout` bounds the TCP connect and banner exchange,
not the KEX/userauth that follow; `BatchMode` guards public-key auth
fallback, not key exchange.
Not run: `./tests/run-integration.sh` on either backend, and no test
suspends a VM to observe a real timeout — the unit tests pin the argv, not
the runtime behavior. A suspend-and-assert integration phase is backend-
specific and slow; the bounds are still unexercised outside production.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous two commits bounded the guest transports through two keepalive tiers (~90s for guest work, ~10s for probes) plus four `*_opts`/`*_args` accessors and their `SshSession` mirrors. Two tiers forced the base options to stay `ServerAlive*`-free, since OpenSSH honors the first value of a repeated `-o`, and that in turn needed tests pinning each call site to the right accessor. Every existing consumer already wanted the same 30s/3 bound — `ssh.rs` interactive sessions, the `proxy.rs` reverse tunnel, and the `port_forward.rs` forwarder each appended it by hand. Put that pair in the shared `transport_opts` list instead and the tiers collapse: the three appenders become redundant at identical values, `ssh`, `scp`, and rsync's `-e` all inherit the bound, and no caller can lose it, so the accessors and their pinning tests are unnecessary. Also drops the `GuestProbe` taxonomy that told an unreachable guest apart from a missing feature. That is an error-message change rather than a hang fix, and it lands separately. Boot probes now share the ~90s ceiling rather than getting ~10s. Both probe loops check their deadline after the probe, so a wedged sshd overshoots the boot timeout by up to that much — still bounded, where before this branch it hung indefinitely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarkaMaul
changed the title
hbrodin
approved these changes
Sep 4, 2026
hbrodin
left a comment
hbrodin
left a comment
Collaborator
There was a problem hiding this comment.
Approved with one nonblocking inline suggestion to remove historical test commentary. No functional or security findings survived review. All eight review lenses were covered. Verified SSH option precedence and shortened banner/key-exchange timeouts, and confirmed the new tests fail when each transport bound is removed. CI is green; full Rust tests were not rerun locally, and Lima/Firecracker integration remain unverified.
Comment on lines
+3328
to
+3329
| // Two hand-copied lists is how the last hardening change reached only | ||
| // one of them; they now derive from `transport_opts`. |
Collaborator
There was a problem hiding this comment.
Please remove these two comment lines: “the last hardening change” records development history, while the test name and assertions already explain the current contract. Nonblocking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Guest transports ran with no deadline. When a guest stops answering — a paused
VM, a wedged sshd, a lost TAP device —
ssh,scp, andrsyncblock on adead socket indefinitely, so the lifecycle command that invoked them hangs
until the user interrupts it.
coop execandcoop push/pullhang the sameway.
ssh,scp, and rsync's-ecommand now derive from one list,SshTarget::transport_opts, which carries the bounds:BatchMode=yes— a key the guest rejects fails instead of stalling on apassword prompt where no interactive user exists (
coop upin CI).ConnectTimeout=10— bounds the TCP connect and banner exchange.ServerAliveInterval=30/ServerAliveCountMax=3— bounds the establishedsession, so a guest whose sshd stops answering fails after ~90s.
The three transports previously kept three hand-copied option lists, which is
how the last hardening change reached only one of them. Only the port flag now
differs (
-pvs-P).Why one list rather than a per-caller bound
OpenSSH honors the first value it obtains for a repeated
-o:So a bound cannot be tightened by appending, only by owning the base list.
Three callsites used to append
ServerAliveInterval=30by hand —ssh.rs:keepalive_opts(interactivecoop shell),port_forward.rs(background forwards), and
proxy.rs(the reverse tunnel). All three wantedthe same value the base list now sets, so this removes them: same effective
bound, three fewer places to keep in step.
ServerAliveCountMaxwaspreviously left at OpenSSH's default of 3 in the two tunnels and is now
explicit.
The consequence worth reviewing is that no caller can pick a shorter bound.
That is the point — it is also why the earlier revisions of this branch needed
two option tiers and four accessors to keep the base list probe-free.
Trade-off
Boot readiness probes (
SshTarget::wait_until_ready, the Lima auth loop inlima.rs) share the ~90s ceiling rather than getting a tighter one. Both loopscheck their deadline after the probe, so a sshd that accepts and then goes
silent overshoots the boot timeout by up to that much. Before this branch that
probe hung indefinitely, and
ConnectTimeout=10still covers the ordinary bootcase of nothing listening or no banner.
ConnectTimeoutdoes not bound key exchange or authentication — only connectand banner.
ServerAlive*covers everything after the client loop starts.Testing
cargo fmt -- --check;cargo clippy --all-targets --all-features -- -D warnings;cargo test(1112 passed).-oprecedence claim above against local OpenSSH 10.3p1.options behind every guest command and transfer, so it wants
./tests/run-integration.shon Lima and on a Firecracker host before merge.Two tests cover the change: one asserts all four bounds reach
ssh,scp, andrsync's
-eand that each carries exactly oneServerAlive*pair (a secondpair would be the silently-shadowed failure mode); one asserts
sshandscpoptions differ only in the port flag.
Follow-up, not addressed here
On an unresponsive guest, the bootstrap checks still report "CLI is not
installed — rebuild the image" rather than naming the real cause, because
exec_okfolds "exited non-zero" together with "never answered". Distinguishingthem is an error-message change rather than a hang fix, and lands separately.