feat(remote): serve a model on a tailnet GPU machine - #333
Conversation
f2dca4f to
78bf68b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
78bf68b to
e186e9a
Compare
|
The red That job landed on which is exactly the three unexpected failures in this run ( The runner is repaired — torch is back to |
7285717 to
24e057b
Compare
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 24e057b
Summary
Adds rocm remote (~7.8k lines, 31 files): provisions the CLI onto a GPU machine over SSH, serves a model there, and publishes the port onto a Tailscale tailnet, plus a containerised SSH test harness and CI lane. Needs work — the security design is genuinely good, but four added tests cannot fail when the code they guard breaks, and one harness line can expose a baked-in password beyond loopback. Verified: the trust model is loopback bind + per-session API key + tailnet-scoped tailscale serve — I confirmed the remote server is pinned to --host 127.0.0.1 --require-api-key (mod.rs:478-493), that funnel is never invoked (only serve --bg --tcp=, publish.rs:176-186), that the key travels on ssh stdin not argv and is stored 0600 with a path-traversal-guarded session id, that shell_quote covers every user value and is proven against a real sh, and that host trust is delegated to the user's own ssh config with BatchMode=yes (fails closed, no silent TOFU) — the README states all of this plainly, so code and documented model match; on the revert question I checked every added test individually and four fail it (below), while the rest are tied to real functions via a ScriptedTransport that hard-errors on unmatched commands; I refuted a reported "remote skips signature verification" concern by reading install.sh (a pinned release key is present, so public_keys is non-empty and the signature gate fires on the remote too); I ran cargo fmt --check (clean) as my one permitted check, so the red check is not formatting — the two plausible candidates I can argue from the source are the unguarded readiness loop in run-e2e.sh:141-144 and the first-ever activation of @requires-docker scenarios on the required e2e lane via E2E_INCLUDE_DOCKER: "1", but I could not read the CI logs and will not call it flake without them. Blocking: 4 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
tests/remote-ssh/run.sh:84-85 — the container is started with docker run -d -p "127.0.0.1:${PORT}:22" ... || docker run -d -p "${PORT}:22" .... The fallback drops the loopback prefix and publishes sshd on all interfaces, and the image ships a real password account (Dockerfile:39-45: a fixed username/password with PasswordAuthentication yes) whose password is committed to this public repository. On any host where the 127.0.0.1: publish form fails, this silently exposes a guessable-password shell on the LAN for the container's lifetime — on a CI runner or a contributor's laptop. run-e2e.sh:84 correctly uses the loopback bind with no fallback. Fix: drop the || fallback so a failed loopback bind is a hard error.
tests/e2e-cucumber/tests/e2e/remote_steps.rs:283-305 (scenario at features/remote.feature:58-64) — remote-09 is titled "Checking a machine's health never installs anything on it", but its When step deliberately uses no container, so ssh fails at the transport layer before remote_doctor reaches bootstrap::locate_cli (the step's own comment says so). then_doctor_installed_nothing accepts "could not reach" as a pass, and then_doctor_points_at_serve wraps its only assertion in if said.contains("only reads"), which never holds here — a permanently dead assertion. If remote_doctor were changed to call ensure_ready_with(...) and silently provision on a health check, this scenario would pass unchanged. Fix: give it a reachable container with no rocm binary so locate_cli's refusal actually fires, and make then_doctor_points_at_serve unconditional.
tests/remote-ssh/run-e2e.sh:195-198 — attach is the one stateful step whose effect is never checked. serve (line 167) and stop (line 206) both cross-check the container's real tailscale serve status --json; attach only asserts the printed strings "Endpoint re-published" and "not restarted". The preceding step withdraws the endpoint out of band, so this is precisely where re-publishing matters — yet an attach that printed those lines without re-publishing would go undetected, because the following stop reports success either way and the final expect_absent '"8000"' passes trivially. Fix: add serve_config="$(in_container tailscale serve status --json)"; expect_contains "the endpoint is back" '"8000"' "${serve_config}" right after the attach call.
apps/rocm/src/remote/mod.rs:1413-1425 — serve_sends_the_key_over_stdin_when_it_starts_the_model never calls serve(). It invokes transport.exec_with_stdin(..., Some("k")) itself, then asserts ScriptedTransport recorded the Some("k") it was just handed — exec_with_stdin pushes stdin unconditionally, so the assertion cannot fail. Its comment claims to guard "the caller actually supplies it", but reverting the real call site at mod.rs:270 from Some(&api_key) to None leaves this green. The command-shape half is already covered by mod.rs:1023. Fix: make serve() accept a &dyn Transport so the real orchestration can be driven through ScriptedTransport, or delete the test rather than leave a false guarantee on the credential path.
Non-blocking
apps/rocm/src/remote/provision.rs:130-131— the comment "the remote can repeat every check this machine made" is overstated:ROCM_CLI_SIGNING_PUBLIC_KEY_PATH/PEMis not forwarded, so an operator using a private-mirror key gets the remote verifying against the pinned production key instead — a hard failure, not a downgrade, but a confusing one. Forward the key vars, or narrow the comment.apps/rocm/src/remote/provision.rs:155—ROCM_CLI_ARCHIVE={remote_dir}/{asset}is the only unquoted interpolation into a remote command in the whole module;assetcomes from parsing the installer'sdownloaded:line. Not exploitable today, but it breaks the otherwise-uniformshell_quotediscipline.apps/rocm/src/remote/transport.rs:24-28andtailnet.rs:248-252— both#[cfg_attr(not(test), allow(dead_code))]comments say "remove this attribute in the change that adds the serve path"; this PR is that change. Leaving them will mask genuinely dead code added later.apps/rocm/src/remote/transport.rs:238-240—ConnectTimeout=10bounds only the handshake; there is noServerAliveInterval/ServerAliveCountMaxand no wall-clock bound onwait_with_output(), so a connection that drops mid-command hangs the CLI indefinitely, including instatus's polling loop.tests/remote-ssh/run-e2e.sh:141-144— the sshd readiness loop falls through after 15s with no success check, unlike the equivalent loop inrun.sh:108-113which hard-fails with a clear message. A slow container start surfaces as a confusing discovery-assertion failure instead; this is my leading in-diff candidate for the red check.
|
Addressed all 4 blocking findings and 4 of 5 non-blocking findings from the automated review; skipping one non-blocking item as a follow-up. Blocking
Non-blocking
Verification
Also replied to and resolved the two CodeQL threads: alerts #778/#779 already report |
9af9830 to
7327243
Compare
|
🔴 Automated review · pr-review-watcher · 486054f This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryAdds 🚫 Blocking (must fix before merge)1. ensure_public_service_has_endpoint_key(host, endpoint_key_file.is_some(), false)?;
It is reachable, not merely theoretical, because the two values are computed by different tests: This also repeats prior finding 1's shape: the comment on the line above ("enforce the invariant here too rather than relying on every future caller having done so") claims the invariant is enforced, and only half of it is. Fix: 2. publish: publish::publish_state(transport, record.tailnet_port, record.remote_port).ok(),
This is the same defect class as prior blocking finding 2, one layer up and still present. The inconsistency is visible within the same function: forty lines earlier the very same code path carefully separates Fix: carry the error rather than dropping it — make the field 3. async fn then_doctor_points_at_serve(world: &mut E2eWorld) {
let said = said(world);
if said.contains("only reads") {
assert!(said.contains("rocm remote serve"), "{said}");
}
}If the output does not contain This is the standing test-vacuity failure mode, and it is in the remediation itself: commit 6f3a81e ( Fix: drop both escape hatches — assert Non-blocking
|
|
Addressed the second review round. Blocking finding 1 — Blocking finding 2 — Non-blocking items — none skipped, all five addressed:
Verification (local, on top of the branch's current merge with
Commits: One open item, not part of this review round's findings: |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · de914c0
This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Adds rocm remote (serve/attach/stop/status/doctor/targets) driving a tailnet GPU machine over SSH, plus a containerised SSH test lane — Needs work. Verified: ran cargo test -p rocm --bin rocm remote:: (126 passed, 0 failed); confirmed both prior blocking findings are genuinely fixed — the Funnel classifier now checks AllowFunnel before the forward lookup and both publish and withdraw bail naming tailscale funnel --tcp=<port> off, and the tailscale prerequisite is hoisted above the install branch with a revert-sensitive test; also confirmed the credential is delivered over stdin (never argv) and the diff carries no internal leaks or injected instructions. Two new defects in the remediation commits, both verified against source. Blocking: 2 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
1. apps/rocm/src/remote/provision.rs:195-218 — signing-key precedence is the inverse of install.sh, and the doc comment claims otherwise.
signing_env_fragment_from matches pem_env first and only falls back to path_env. install.sh:99-110 (resolve_public_keys) does the opposite: it returns ROCM_CLI_SIGNING_PUBLIC_KEY_PATH if set and only falls through to _PEM when it is not. The doc comment at provision.rs:184-186 asserts "an explicit _PEM is forwarded as-is and takes precedence, matching install.sh's own resolution order" — that is factually false, on the selection of a signature trust root. With both variables set locally, a remote provision verifies against a different key than a local install.sh run would; the failure surfaces as "the remote rejected the build we fetched for it", which points at the artifact rather than at the key.
There is a second, sharper edge in the same function: std::env::var(..).ok() yields Some("") for a variable set to the empty string, whereas install.sh's [ -n ... ] treats empty as unset. So ROCM_CLI_SIGNING_PUBLIC_KEY_PEM="" together with a real _PATH makes this code forward an empty _PEM and silently drop the operator's explicit key, and the remote then falls back to the pinned production keys — the operator's chosen trust root is discarded with no diagnostic.
Fix: check path_env before pem_env (or, if the inversion is deliberate, correct the comment and say why), and treat an empty value as unset on both branches. The new test an_explicit_pem_is_forwarded_as_is_and_wins_over_a_path (provision.rs:407-421) currently encodes the wrong order, so it must change with the code — it is why this slipped through. Add a case asserting the order that install.sh actually implements.
2. apps/rocm/src/remote/transport.rs:359-370 — the deadlock fix consults the stdin-writer error before the outcome it now has in hand, discarding ground truth.
wait_with_output() returns first and output already holds ssh's exit code, stdout and stderr. The code then does writer.join()...?? before the SSH_TRANSPORT_FAILURE (255) check at :379, so a write error on the payload aborts the call and throws the captured outcome away. The relevant write error is BrokenPipe: if the child exits and closes stdin before the writer thread is scheduled, write_all gets EPIPE. Moving the write onto a thread widened that window rather than narrowing it — previously the write happened inline immediately after spawn, whereas now the main thread blocks in wait_with_output while the writer waits to be scheduled.
The consequence lands on the one caller that uses this path, serve_with_transport (mod.rs:286): instead of the purpose-built "could not reach {dest} over ssh: {stderr}", an unreachable host can produce "failed to send input to {dest}: Broken pipe", which mod.rs:288-300 then wraps as "lost contact ... so it may or may not be running" and clears the freshly minted key — telling the user the model's state is unknown when the transport in fact reported 255 and nothing started. The call-site comment "this fails on a broken pipe while sending the key ... so the model's state is genuinely unknown from here" was true before the fix and is now stale.
Fix: evaluate the 255 check and build RemoteOutcome from output first; only surface a writer error when the process outcome does not already explain the failure (treat ErrorKind::BrokenPipe as advisory once output is in hand). While there, join the writer on the wait_with_output error path too — today it is dropped and detached.
Non-blocking
apps/rocm/src/remote/transport.rs:201-216and:655-672— the remote-path guard's stated rationale is wrong: the remote argument is built asformat!("{destination}:{remote_path}"), so it can never start with-and scp cannot read it as an option; keep the check but fix the comment and the test comment, or a future reader re-derives the same wrong mechanism.apps/rocm/src/remote/provision.rs:284-294—create_dirfollowed byset_permissions(0o700)leaves a umask window, contradicting the adjacent comment's "0700 keeps the contents unreadable"; this repo already has the atomic pattern inapps/rocm/src/dash.rs:255-266(DirBuilder::new().mode(0o700)), which even documents why..github/workflows/ci.yml:652-691— the newremote-sshjob runscargo build -p rocmwith noactions-rust-lang/setup-rust-toolchainstep and no rust cache, unlike every other cargo job in this workflow; a cold uncached build of this workspace against the 30-minute timeout is a plausible cause of the single failing check, but I am inferring that from the workflow source and cannot confirm it — no lane names were available to me, and the un-merged base may equally explain it.tests/remote-ssh/fake-tailscale.sh— the fake only ever emits{"TCP": ...}, neverAllowFunnel,ForegroundorServices, so the exposure classifier's safety branches (the subject of the prior blocking finding) are proven only againstScriptedTransportfixtures, not against anything shaped like the real daemon.apps/rocm/src/remote/mod.rs:747— theFunnelAllowedstatus line leads with "no", but that state is also reached when our own forward is live (Funnel is checked first and short-circuits); phrase it as an exposure warning rather than a "not published" answer.
Adds `rocm remote`: discover GPU machines on your tailnet, check their health, install what they are missing, serve a model on one, and reach it from any of your machines. SSH is the control channel, not the data path. Everything that inspects or changes the remote goes over SSH; the inference traffic does not. `rocm serve` binds loopback on the GPU machine as it always has, and the machine then tells its own Tailscale daemon to forward a tailnet port to it. So nothing runs locally, the endpoint outlives the command that created it, and it answers from any of the user's machines rather than only the one that started it. That breaks an assumption elsewhere, so this fixes it too. `serve` grants an API key only to non-loopback binds, on the reasoning that loopback means "only this machine can reach it". Publishing the port makes that false while leaving the bind address unchanged, which would put an unauthenticated model endpoint on the tailnet. `serve` gains `--require-api-key` for exactly this: whoever widens the reach asks for the credential. Remote sessions always set it, and the key travels on stdin, never in a command line that both machines expose in their process tables. Two lifecycles are tracked, both on the remote, because either can fail alone: the model server and the publish pointing at it. They are reported as separate facts — a live model with no endpoint is re-published, a dead one has to be restarted, and collapsing them hides which. Teardown is the sharp edge. A publish is configuration rather than a process, so it survives reboots: a forgotten one is a GPU endpoint on the tailnet with nothing tracking it. Withdrawal is confirmed rather than assumed, ownership is established before a port is claimed or released so a session never takes over or tears down another's forward, and a teardown that cannot confirm both halves keeps the session listed rather than dropping the only record of what is still running. `--force` exists for a machine that is gone for good, and is deliberately louder than a clean stop. Provisioning never copies this machine's binary. That only worked when both machines shared an OS and CPU, and when they did not the copy still landed and still looked installed. The remote fetches its own build with the project's installer instead; if it cannot reach the release host, this machine fetches one *for the remote's platform* and pushes it with its checksum and signature so the remote repeats every check. `install.sh` gains the download-only and install-from-archive modes that makes possible — splitting the trust chain across two machines must not shorten it. Installing ROCm is opt-in and gated on the failure catalog: a machine the catalog recognises as needing a person is refused, since the wizard that walks someone through those cannot run over a connection nobody is watching. Passwordless sudo is checked first, because a prompt the control channel will never answer hangs rather than fails. Health checks need almost no new logic: gathering facts already produces a plain snapshot, and scoring it reads nothing else, so the fetch runs on the remote and the scoring here. Suggested fixes are rewritten to name the target — a command repairing a machine you are not sitting at is not one you can paste, and printed bare it invites being run against your own. `rocm services list --json` is added as the machine-readable listing the remote orchestration reads back, applying the same liveness filter as the table so the two cannot disagree about what is running. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
`rocm remote` drives ssh, scp, and the remote machine's own tooling. Unit tests use a scripted stand-in, which proves control flow but assumes those tools behave a certain way — and the assumptions are where this could quietly be wrong. Three layers close that. A container standing in for a GPU machine: real OpenSSH, with `rocm` and `tailscale` as stubs that keep state in files, so publishing and withdrawing are genuinely stateful rather than canned. No GPU, no ROCm, no tailnet needed. `run.sh` checks the tool contracts against it — argument handling, a non-zero remote exit arriving as an exit code, a credential delivered on stdin and absent from the command line, file copy taking its port as -P, batch mode refusing rather than hanging, and that withdrawing a published endpoint actually removes it. Ports are keyed as strings in the serve config, so a parser looking for a number would match nothing and call every live endpoint absent. The batch-mode check has an account that really does ask for a password. With password auth off everywhere the connection failed for lack of a key either way, and the check proved nothing; now batch mode is refused immediately while the same connection without it waits for input nobody will type. `run-e2e.sh` drives the built binary through the whole flow — discover, probe, serve, publish, reconcile status, re-publish after an out-of-band withdrawal, tear down — catching orchestration mistakes a scripted stand-in cannot. It does not prove the endpoint carries traffic; that needs a real two-node tailnet. Behavioural coverage lands where the project requires it. Nine scenarios cover discovery, refusals and the session list with a stand-in for the network client, so they do not depend on the developer's own machine being on one. Five more cover the paths that succeed — serve, status, attach, stop, doctor — which need a host on the other end of a real SSH connection, so a new `@requires-docker` gate stands the container up and skips with a reason where no container runtime exists. Both run on the new containerised CI lane. The stub's `examine` answers with a complete document generated from the type's own defaults and pinned by a unit test. It previously returned a fragment that looked plausible and could never have deserialized; nothing noticed, because no test ran that code against it. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The README gains the command surface and, more importantly, states who can reach a published endpoint. The loopback intuition from local serving does not carry over: the endpoint is reachable by every machine on the tailnet that the tailnet's own rules allow, and the API key is what stops anyone else using it. A reader who assumes otherwise never thinks to check their access rules. Also documents `ROCM_REMOTE_SSH_CONFIG`, since ssh resolves its config from the account database rather than from HOME and there is otherwise no way to point the CLI at a different one. docs/testing.md describes the two container harnesses, what each proves, and what neither does — the endpoint carrying traffic still needs a real two-node tailnet. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
serve_with_transport() takes the transport and paths serve() builds, so a scripted-transport test can drive the exact code path that starts the model and see the same stdin write, instead of only checking exec_with_stdin and remote_serve_command in isolation with nothing pairing them. Forward ROCM_CLI_SIGNING_PUBLIC_KEY_PATH/_PEM to the remote's own install.sh, shell-quoted, so a machine trusting an alternate signing key does not fall back to the pinned production keys and reject an archive this machine already trusted. Drop two dead_code attributes on resolve_peer and the transport module that have had callers since the serve path landed. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
remote-09 asserted a doctor refusal without a live connection, so the refusal it wanted to prove was never actually reached before the command failed for lack of a connection. Build a second stand-in image with no rocm binary at all (Dockerfile's INCLUDE_ROCM_CLI build arg) and give the scenario a Given step that starts it, so the assertion is reached against a machine that is reachable but genuinely missing the CLI. Harden the SSH fixture harness while in there: run.sh no longer falls back to publishing on all interfaces when the loopback bind fails, since the image ships a password account for the batch-mode check and that fallback would put it on the LAN. run-e2e.sh treats a readiness timeout as a failure instead of racing on regardless, and checks that the tailnet publish actually reports the endpoint after attach. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
A test added on main called resolve() with the old positional bool signature. This branch had already refactored resolve() to take an Included struct, so merging main back in left two call sites that no longer matched the function signature, breaking the build. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
serve status --json's AllowFunnel key (keyed host:port) was listed in SERVE_CONFIG_KEYS but never parsed, so a Funnel-enabled port classified as Absent and publish() would complete a public-internet exposure with no warning. Parse it into RawServeConfig and, when set for the target port, return a distinct FunnelAllowed state so both publish() and withdraw() bail loudly naming the exact command to turn it off. Apply the same fix to the Services key, which had the identical blind spot. Comment SERVE_CONFIG_KEYS to distinguish keys we parse from ones that are only there to round-trip the document shape. Correct the module doc's "visible to the whole tailnet, scoped only by its ACLs" claim, which Funnel already contradicts. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
With --install-rocm, the rocm_present branch returned before the tailscale_present bail was reached, so on a machine with neither present, ROCm install would run before the Tailscale check refused the machine. Hoist the tailscale_present check above the rocm_present branch so a machine without Tailscale is always refused first, regardless of what else is missing. Extend the existing a_machine_without_tailscale_is_refused_before_a_model_is_started test with an install_rocm = true, neither-present fixture; it fails without the hoist. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
push_matched_artifact forwarded ROCM_CLI_SIGNING_PUBLIC_KEY_PATH to the remote verbatim, but that path names a file on this machine — the remote shell has nothing at that location, so the variable was set but useless. Read the file locally and forward its content as _PEM instead; an explicit _PEM still wins, matching install.sh's own resolution order. The lookup is split into a pure, dependency-injected helper so both branches are covered by unit tests without touching the real filesystem or environment. Also replace the PID-keyed staging directory with one salted by a nanosecond nonce and created with create_dir (not _all), so a pre-staged or symlinked path at a reused PID can no longer be silently adopted, and restrict it to 0700 so its contents aren't readable by other local users even if the name leaked. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
exec_with_stdin wrote the payload to the child's stdin synchronously before calling wait_with_output. ssh does not have to drain stdin before producing output, so a payload larger than the OS pipe buffer paired with any remote output could deadlock: us blocked writing stdin, ssh blocked writing a full stdout/stderr pipe, and wait_with_output — which would drain both — never reached. Write the payload on its own thread instead, so the main thread reaches wait_with_output immediately; join the writer afterward and propagate a write failure or panic. Also give scp_argv the same leading-`-` rejection guard validate_destination already applies to the ssh destination: scp has the identical option-vs-argument ambiguity for its two path arguments. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
signing_env_fragment_from checked _PEM first and fell back to _PATH.
install.sh's resolve_public_keys does the opposite: _PATH wins, and _PEM
is only consulted when it is unset. The doc comment claimed the two
agreed.
What they disagreed about is the trust root a signature is verified
against, so with both variables set a remote provision accepted a build
that a local install would have rejected. It surfaced as "the remote
rejected the build we fetched for it", which points at the artifact
rather than at the key.
An empty value now counts as unset on both sides too. install.sh tests
these with `[ -n ... ]`, but env::var yields Some("") for `FOO=`, so an
empty _PEM alongside a real _PATH forwarded the empty one and silently
discarded the operator's chosen key.
The test asserting the old order is replaced rather than kept: it is why
this shipped.
Also closes a umask window in the staging directory the signing key is
written into, using the DirBuilder::mode pattern already in dash.rs.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Moving the stdin write onto its own thread fixed a deadlock but left the join before the 255 check, so a write error aborted the call and threw away the outcome already in hand. The write error that matters is EPIPE. ssh exits without reading stdin when it cannot reach the host, which breaks the pipe, so an unreachable host produces two errors describing one event. Consulting the echo first reported "failed to send input" where ssh had plainly said 255, and serve_with_transport wrapped that as "may or may not be running" — telling the user a model might be up on a machine never contacted. The outcome is now built first and the write error consulted after, and demoted to a symptom only when the command itself also failed. A broken pipe on a *successful* command still errors: the payload there is an API key, and a model started without the key meant to guard it is not a success. The writer is also joined on the wait error path, which previously detached the thread. The regression test uses a payload larger than a pipe buffer. A short one lands in the buffer and returns success with no reader, so the race decides whether the bug appears; overflowing makes the EPIPE certain and exercises the overflow the thread exists to survive. Also corrects the scp path guard's rationale — the remote argument is prefixed with the destination, so scp can never read it as an option and that half is a shape check, not a safety one — and rephrases the Funnel status line, which led with "no" to a question it had not looked at. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The container fake only ever emitted {"TCP": ...}, so the exposure
classifier's Funnel branch was proven against hand-written fixtures and
nothing else. A fake that wrote AllowFunnel in the wrong place would
have left the CLI looking correctly cautious while reading nothing.
The fake now understands `funnel --tcp=N on|off` and writes AllowFunnel
where the real daemon does, keyed host:port. run.sh asserts that shape,
and run-e2e.sh drives the binary at an exposed port and checks it
refuses, names the way out, and publishes nothing anyway. The CLI never
runs funnel itself — it only reads the key — so this exists purely to
put the remote in the state that must be refused.
Also installs the pinned Rust toolchain on the remote-ssh job. It was
the one cargo job in the workflow building against whatever the runner
image shipped, uncached, against a 30-minute timeout.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Funnel serves 443, 8443 and 10000 only. The fake accepted `--tcp=8000` and wrote an AllowFunnel entry tailscaled could never emit, so the new coverage proved the classifier reads the right field against a document that cannot occur. The fake now refuses the ports the daemon refuses, and both lanes exercise 443. Worth stating plainly, because the fixture was hiding it: at the default tailnet port of 8000 the Funnel guard can never fire, since Funnel cannot listen there. It is reachable only via `--tailnet-port 443` (or 8443/10000). That is not a hole — a Funnel on 443 does not expose 8000 — but it does mean the guard is dead weight at the default, which is a design question worth its own look. Also from review: - The FunnelAllowed status line had no test at all, despite being rewritten last commit; it now has one, and interpolates the real port instead of printing a literal `<port>` the user has to substitute. - create_restricted_dir's comment claimed a signing key is staged there. It is not — the archive, checksum, signature and installer are, all public. The mode guards integrity, not confidentiality; say so. - The writer-panic arm is only reached when the two earlier returns did not fire. Note that rather than claiming an outcome cannot explain it. - Record that the BrokenPipe demotion is safe because of where remote_serve_command puts its `read`, not because of anything this function enforces. - Add the Funnel checks to the coverage lists in docs/testing.md and the CI job comment, which are maintained by hand. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
… send Forwarding the key was only half of it. install.sh's resolve_public_keys reads ROCM_CLI_SIGNING_PUBLIC_KEY_PATH before _PEM, so a path the remote exports for itself — /etc/environment through pam_env reaches non-interactive sshd sessions — beat the key we had just sent, and the two machines verified against different trust roots. Which is the same divergence the precedence fix closed, arriving from the other side. The provisioning command now sets an empty _PATH alongside the _PEM. install.sh tests it with `[ -n ... ]`, so an explicitly-empty export reads as unset there, and the forwarded key wins. Verified at the shell: with an ambient _PATH=/etc/other.pem exported, the old fragment resolves to /etc/other.pem and the new one to the forwarded key. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Same drift as the earlier pass: this branch changed `resolve` to take an `Included` struct so a mis-ordered bool cannot silently pick a different set, and main has since grown three more call sites using the old six-argument form. Neither side conflicts textually, so it only appears once the two are built together — which is what CI does and a clean local build does not. Reproduced by rebasing onto main: clippy fails with three E0061s at expectation.rs:1148, :1155 and :1162. All three passed `false, false, false`, so `Included::default()` is the equivalent. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
…port move The rewritten comment on create_restricted_dir was still wrong about the mechanism, in the opposite direction from before. There is no download-then-check window in that directory: install.sh's download-only mode fetches and verifies inside its own mktemp -d and only copies the proven artifacts out. Nor can the installer be swapped there — it is rewritten from the compiled-in constant immediately before the push. What 0700 actually buys is the window between the verified artifacts landing and the scp: a second local user could swap the archive together with a matching .sha256, and the remote re-verifies the pair it is given. A signature would stop that, but install.sh only requires one on the release channel with pinned keys, or when a key is named explicitly, so on any other channel the checksum is all that stands there. The "impossible document" argument applied to the shell fake but not to the Rust fixtures, which still keyed AllowFunnel at :8000 and :9000. They now use 443 and 8443, and the render_status test builds a record on 443 — it was asserting a remedy string no user could ever see, since Funnel cannot listen on the default tailnet port. Also: run.sh now asserts the fake's refusal *message* rather than a non-zero exit, since `remote` is an ssh wrapper and ssh fails non-zero for its own reasons; verified it fails when the port gate is removed. The duplicate signing test is folded into the stronger assert_eq above it, and the remaining hand-maintained coverage comment in ci.yml is caught up with docs/testing.md. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The empty _PATH rides along with a forwarded key, so it only ever guarantees that the key we send wins — not that the remote's own path is always switched off. Forward nothing, which is the default, and a _PATH the remote exports for itself still stands while this machine verifies against the pinned keys. Blanking it unconditionally would close that too, but it would also override a remote operator's deliberate mirror-key config in exactly the case where this machine has no opinion. The divergence fails loud rather than trusting the wrong root, so it stays a decision rather than an assumption, and the comment now says so instead of implying otherwise. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
de914c0 to
486054f
Compare
|
Addressed the review on Both blocking findings fixed, each reproduced first.
All five non-blocking items fixed, including the scp guard rationale (the remote argument is prefixed with the destination, so that half is a shape check rather than a safety one) and the Funnel status line. One finding I did not take. The CI lane suggestion assumed the missing toolchain explained the red check. It did not — Beyond the review, worth flagging:
Local: full workspace tests, clippy |
|
CI status: 20 of 21 checks green, including The one red check,
Flagging rather than fixing: a dash regression is unrelated to this PR and belongs in its own change. Happy to pick it up separately if that is useful. |
pr-review-watcher · de914c0 — superseded, withdrawn.
Both blocking findings from that round are genuinely fixed at the current head, and each was re-verified here rather than taken from the summary: the signing-key precedence now matches the install script in both order and empty-value handling, pinned by a test that fails when the order is reverted; and a cleanly failed transport is now reported as a failure rather than as indeterminate, pinned by a regression test whose payload size makes the condition deterministic instead of timing-dependent.
On the earlier CI-lane point: the author is right and that finding was wrong. It asserted a cause for a red check that this automation had no way to confirm, and should have been stated as an inference or not at all.
A fresh review of the current head is posted separately.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 486054f
This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Both findings from the previous round are genuinely fixed, and the earlier change request has been withdrawn. Blocking: 3 · Non-blocking: 5. Three new issues at this head, two of them the same two defect classes one layer away from where they were fixed. Full detail is in the review comment on this PR.
1. The API-key guard is bypassed at one of its four call sites. ensure_public_service_has_endpoint_key is passed a hardcoded false for requires_api_key in apps/rocm/src/main.rs, while the real value is already in scope and the other three call sites pass it. Reachable, because the guard's two inputs are computed by different tests — one by file existence, one by file validity — so an existing-but-invalid key file is exactly the case the new branch was added to catch, and it spawns an unauthenticated listener instead. The comment above the line says the invariant is enforced here; only half of it is. Fix: pass the real value, and add a test for a present-but-empty key file on a service that requires one.
2. A definite remote failure is reported as "the machine could not be asked." In apps/rocm/src/remote/mod.rs, publish_state(...).ok() discards two materially different errors into one: transport failure, and the remote answering with a non-zero exit and its own stderr. The user is told the machine was never reached when it in fact replied with an actionable reason. This is the same class as the finding just fixed in the transport layer, one layer up — and the same function separates reached-but-failed from never-reached forty lines earlier, which is also what the module docs describe. Fix: carry the error instead of dropping it, and pin the distinction with a test.
3. A test step asserts nothing when its guard does not match. In tests/e2e-cucumber/tests/e2e/remote_steps.rs, the step passes unconditionally unless the output contains a particular phrase, and its partner step carries a matching escape hatch. Together the scenario can go green having checked only that the command exited non-zero. The commit in this round that made the container reachable removed the reason those fallbacks existed but left them in place, so they are now dead permissiveness that will hide the coverage disappearing. Fix: drop both escape hatches so an unexpected path fails loudly.
4 participants
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. — n/a, no bug fix; no xfail rows affected.Summary
Adds
rocm remote: discover GPU machines on your tailnet, check their health, install what they are missing, serve a model on one, and reach it from any of your machines.SSH is the control channel, not the data path. Everything that inspects or changes the remote goes over SSH. The inference traffic does not:
rocm servebinds loopback on the GPU machine as it always has, and the machine then tells its own Tailscale daemon to forward a tailnet port to it. Nothing runs locally, so the endpoint outlives the command that created it and answers from any of your machines rather than only the one that started it.Ready for review, not for merge. Two things still need a real tailnet and a real GPU to confirm — see "Not verified" below.
Why this shape
The alternative was a local
ssh -Ltunnel. Publishing from the remote instead means no local process to supervise, no tunnel PID to track, and an endpoint that survives the terminal that made it. The cost is a hard dependency on Tailscale for serving, and an endpoint that is tailnet-wide rather than point-to-point — which is what drove the one change to existing behaviour below.Changes existing behaviour
rocm serve --require-api-key.servegrants an API key only to non-loopback binds, reasoning that loopback means "only this machine can reach it". Publishing the port makes that false while leaving the bind address unchanged — which would put an unauthenticated model endpoint on the tailnet. The new flag makes a loopback bind authenticated anyway; remote sessions always set it. Local serving is unchanged.The key travels to the remote on stdin, never in a command line, since both machines expose command arguments in their process tables.
install.shdownload-only and install-from-archive modes. Provisioning never copies the local binary — that only works when both machines share an OS and CPU, and when they do not the copy still lands and still looks installed. The remote fetches its own build; if it cannot reach the release host, this machine fetches one for the remote's platform and pushes it with its checksum and signature so the remote repeats every check. Splitting the trust chain across two machines must not shorten it.rocm services list --json— the machine-readable listing the remote orchestration reads back, applying the same liveness filter as the table.Non-obvious decisions
install.shexactly. Both resolve_PATHbefore_PEM, and both treat an empty value as unset. What they choose between is the trust root a signature is verified against, so the two disagreeing would let a remote provision accept a build a local install would reject — surfacing as a rejected artifact rather than a key mismatch. A forwarded key also blanks the remote's own_PATH, so a value the far side exports for itself cannot beat the one we sent.ROCM_REMOTE_SSH_CONFIGnames an alternative ssh config.sshresolves~/.ssh/configfrom the account database rather than fromHOME, so there was otherwise no way to point the CLI at a different one. Added while building the end-to-end harness, which could not run without it; independently useful for anyone with a per-project ssh config.Test plan
cargo test --workspace --all-targets,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --check,prek run --all-files,scripts/smoke_local.py— all pass.tests/e2e-cucumber/features/remote.feature. Eight cover discovery, refusals and the session list. Six need a host on the other end of a real SSH connection, so they carry a@requires-dockergate and skip with a reason where no container runtime exists.tests/remote-ssh/run.sh— 21 checks of the tool contracts against a real OpenSSH server in a container: argument handling, exit-code propagation, a credential delivered on stdin and absent from the command line, file copy, batch-mode refusal, the shape Tailscale Funnel takes in the serve config, and that withdrawing a published endpoint actually removes it.tests/remote-ssh/run-e2e.sh— 25 checks driving the built binary through the whole flow: discover, probe, serve, publish, reconcile status, re-publish after an out-of-band withdrawal, tear down, and refuse to publish over a Funnel-exposed port.remote control channel (containerised)CI lane, gated on theheavypath filter.On a network that intercepts TLS, the container lanes need plain-HTTP package mirrors —
docs/testing.mddocuments theROCM_TEST_APK_REPOSescape hatch.Not verified
tailscaleis a stand-in on both sides of every harness, so publish/withdraw are exercised but no inference request crosses a tailnet. Needs a real two-node tailnet.tailscale servecommand surface. Shapes follow Tailscale's documented CLI and theServeConfigstruct, and the parsing contract is pinned against a stateful stub — but nothing here has spoken to a real daemon.rocmis a stub.Open question for review
The Funnel guard is unreachable at the default port. Tailscale Funnel serves only 443, 8443 and 10000. The default tailnet port is 8000, so
PublishState::FunnelAllowed— a state variant, four refusal arms, a status line, six unit tests and two container lanes — can only be reached by someone passing--tailnet-port 443(or 8443/10000). That is not a hole: Funnel exposure is per-port, so a Funnel on 443 does not expose a session published on 8000. But it is a lot of machinery behind an opt-in flag, and Funnel is not mentioned in any user-facing doc. Worth deciding whether to document it, default differently, or drop it.Risk
Medium. The
rocm remotesurface is entirely new and additive. The two touch points with existing behaviour areserve's new opt-in flag (loopback serving is unchanged when it is absent) and the installer's new modes (the existing path is untouched). Reviewers may reasonably want the installer change looked at separately given its place in the signed-release trust chain — happy to split it out.