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.
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.
486054f to
b014f52
Compare
|
History rewritten: 19 commits squashed to 3, and rebased onto current The three commits are feature / tests / docs. Further review rounds will amend these rather than stack more fix commits on top. The three blocking findings — all verified, and all older than the last round.
All five non-blocking items are also fixed, including a Beyond the named findings, I swept the diff for each defect class rather than just the sites reported. That turned up one more instance of the same collapse: the session-listing path built a Verified locally on the rebased tree: full workspace tests, clippy |
Dismissing this as superseded. Re-reviewed at b014f52: all three counts are resolved and I verified them rather than taking the summary on trust - the managed-spawn guard now receives the record's own flag (confirmed load-bearing by reverting the call site in a scratch copy, which makes the new test fail), the reached-but-failed versus never-reached split is restored and pinned by a test asserting the two render differently, and all 23 then-steps were walked for any input under which no assertion runs. A separate, newly-found instance of the same fail-open class is filed as a fresh change request; this older one is retired so only one objection is live.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · b014f52
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: discover GPU machines on a tailnet, provision them over SSH, serve a model there and publish its port, plus a containerised SSH test lane and docs. Needs work — all three counts of the standing objection are genuinely resolved, but the author's own sweep for the "collapse a failure into a weaker state" class missed one instance, and it sits on the same security gate as count 1. Verified: read all three commit messages whole from the raw objects (57/52/28 lines, signatures intact, company identity and DCO sign-off present); ran a scratch-copy revert experiment reverting the apps/rocm/src/main.rs guard call site to the hardcoded false and ran cargo test -p rocm --bin rocm a_managed_spawn_refuses_an_invalid_key_file — it FAILED, so that test is genuinely load-bearing rather than passing either way; confirmed publish_state(..).ok() is gone and its replacement is pinned by a test that asserts the two outcomes render differently; walked all 23 #[then] steps in remote_steps.rs and found no remaining vacuous-pass path; leak scan over the diff clean (no internal hostnames, gateways, cluster names or registry paths; ROCM_TEST_APK_REPOS defaults empty and the documented example uses the public Alpine CDN); no prompt-injection content found. Blocking: 1 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
apps/rocmd/src/lib.rs:3239 — the API-key requirement is silently dropped, and written back to disk, when a record read fails. supervise_service rebuilds the record from scratch (ManagedServiceRecord::new starts requires_api_key false) and restores the flag from disk with load_managed_services(paths).unwrap_or_default(). That call returns Err on a real, informative failure — read_dir, the per-entry ?, or fs::read on any single record file (note it skips unparseable JSON, so Err means an I/O failure, not a corrupt record). .unwrap_or_default() turns that into "no service ever required a key". The next line falls back to key-file presence, which is absent precisely when a service has been stopped — the case the code's own comment three lines above calls out. Both signals then read false, ensure_public_service_has_endpoint_key at :3250 passes, and the service comes back up on a loopback bind with no authentication even though the user launched it with --require-api-key.
Why this blocks rather than being a nit: it is reachable without exotic conditions (a record file deleted by a concurrent rocm services stop between read_dir and fs::read is enough — ENOENT), it fails open on a security gate, and record.write() at :3256 persists the weakened record, so the damage is permanent: every later rocm services restart is disarmed too. That is verbatim the outcome the adjacent comment says must not happen — "rebuilding a record here without restoring it would not just skip the check now — it would write the weakened record back and disarm every later rocm services restart as well." In user-facing terms this is the same sentence as count 1 of the standing objection: a service the user asked to protect starts unprotected. The PR introduces these lines, so it is not pre-existing.
Fix: propagate instead of defaulting — load_managed_services(paths)?. The function already returns Result<()> and uses ? freely a few lines up (paths.ensure()?, fs::create_dir_all(...)?), and failing closed is this file's own stated preference ("an unreachable service is recoverable, an anonymous public one is not"). Add a test that drives the real call site — supervise_service checks the guard before record.write() and before command.spawn(), so the same technique a_managed_spawn_refuses_an_invalid_key_file_on_a_service_that_requires_one uses in the rocm copy works here — asserting both that it refuses and that the on-disk record is not rewritten with requires_api_key: false.
Non-blocking
.github/workflows/ci.yml:669— the new lane is the only job-levelif: needs.changes.outputs.heavy == 'true'in the file; every otherheavygate is step-level, and the file documents at :1066 why job-level gating stalls the merge queue for a required check. Harmless while this lane is not a required check; worth confirming that is the intent before it becomes one.apps/rocm/src/remote/mod.rs:903—stop()collapses a transport error and a non-zero remote exit into one bool, and the bail names no reason, unlike the withdraw branch immediately above which interpolates{error}. Nothing is misreported (unlike count 2), but the remote's own words are available and dropped.apps/rocm/src/remote/mod.rs:839,867— no test ever callsattach()orstop(); onlyrender_stoppedis tested, with hand-picked booleans. Deleting the earlybail!s so the record is removed on an unconfirmed teardown would pass every test in the file.apps/rocmd/src/lib.rs:5228,7571— the guard tests here call the function directly with literals; neither real call site is driven, so a future miswiring in this copy would go undetected. Therocmcopy does it properly and is the model to follow.apps/rocm/src/remote/transport.rs:195— ~14 literal spaces mid-sentence in a user-facing error message ("would be read as an option"), an editing artefact.
On the standing objection
Count 1 — "a service the user asks to protect with an API key can start UNPROTECTED when the key file is empty or unreadable"; originally "ensure_public_service_has_endpoint_key is passed a hardcoded false for requires_api_key in apps/rocm/src/main.rs". RESOLVED. Both call sites (apps/rocm/src/main.rs:5978 and :15818) now pass record.requires_api_key, and key_present is validity-filtered through endpoint_api_key_from_file rather than mere file existence. The requested test exists and I verified it is load-bearing rather than taking the claim on trust: on a scratch copy with the call site reverted to the hardcoded false, a_managed_spawn_refuses_an_invalid_key_file_on_a_service_that_requires_one fails.
Count 2 — "a real remote failure is reported to the user as merely unreachable"; originally "publish_state(...).ok() discards two materially different errors into one". RESOLVED. The .ok() is gone; observe now calls publish::observe(...), which preserves the reached-but-failed versus never-reached split the same function already made forty lines earlier. Pinned by a_remote_that_answers_about_publishing_is_not_reported_as_one_that_was_never_asked, which asserts the remote's own stderr reaches the status line and that the two cases do not render identically — it would fail against the old .ok().
Count 3 — "one end-to-end test passes without checking anything"; originally "the step passes unconditionally unless the output contains a particular phrase, and its partner step carries a matching escape hatch". RESOLVED. Both named steps now assert unconditionally, with inline comments recording why the guard was removed. I walked all 23 #[then] steps individually looking for any input under which no assertion runs — conditional asserts with no else, early returns, silently-falling-through matches, defanging unwrap_or — and found none.
The block is withdrawn on all three counts. It is replaced by the single new blocking finding above.
On the author's sweep claim. The claim that the diff was swept for this defect class beyond the reported sites, finding one further instance, does not hold: apps/rocmd/src/lib.rs:3239 is a fourth instance, in the same subsystem as count 1. A second, milder instance sits at apps/rocm/src/main.rs:18075, where the uninstall plan's remote-session warning is defaulted away on an I/O error — that one mirrors the pre-existing style on the line above it and is informational only, so it is not called out separately.
Why this will recur (and the cheap prevention). The cause is the codebase inviting the wrong conclusion, not reviewer error: load_managed_services(paths).unwrap_or_default() appears twice in this diff with identical shape, once feeding a security gate and once feeding a printed warning, and nothing at the call site distinguishes them. A competent reader sweeping for this class will keep classifying the security-gate use as benign best-effort, exactly as the author's sweep did. The prevention is one line: at :3239, use ? and add a comment saying this read may not be best-effort because its result arms the guard below — sitting next to the comment already there that explains why the flag must be restored at all.
On CI. The check-run conclusions at this head are counts only (failure 2, pending 4, skipped 1, success 21) with no lane names available, so no outcome is attributed to any named job here; the workflow observation above is read from the YAML, and I cannot confirm from this checkout whether any particular lane is red or why.
Check-run conclusions at this head were failure 2, pending 4, skipped 1, success 21 when the review started, and failure 2, pending 3, skipped 1, success 22 when this was filed. The earlier change request on this PR has been dismissed as superseded, so this is the only objection of ours that is live.
Adds `rocm remote`: discover GPU machines on a tailnet, check their health, install what they are missing, serve a model on one, and reach it from any machine on the tailnet. 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. Nothing runs locally, so the endpoint outlives the command that created it and answers from any machine rather than only the one that started it. Two touch points with existing behaviour: - `rocm serve --require-api-key` makes a loopback bind authenticated anyway. Publishing the port makes "loopback means only this machine" false while leaving the bind address unchanged, which would otherwise put an unauthenticated model endpoint on the tailnet. The key travels to the remote on stdin, never in a command line, since both machines expose command arguments in their process tables. - `install.sh` grows download-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. Signing-key selection matches install.sh's own order exactly, and a forwarded key blanks the remote's own path so the two machines cannot end up on different trust roots. `rocm services list --json` is the machine-readable listing the orchestration reads back, applying the same liveness filter as the table. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The unit tests drive a scripted stand-in, which proves the control flow but assumes the real tools behave a certain way. These check that assumption against a real OpenSSH server in a container — no GPU, no ROCm, no tailnet, since the remote's `rocm` and `tailscale` are stand-ins. - `tests/remote-ssh/run.sh` checks the tool contracts: 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` drives 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. - 14 cucumber scenarios in `features/remote.feature`; the six needing a host on the other end of a real SSH connection carry `@requires-docker` and skip with a reason where no container runtime exists. - Both scripts run on a new `remote control channel (containerised)` CI lane, gated on the `heavy` path filter. The Funnel fixtures use 443, not the default tailnet port: Funnel serves only 443, 8443 and 10000, so an AllowFunnel entry on any other port is a document the daemon cannot produce and a test against it proves nothing. `resolve` takes an `Included` struct rather than a row of same-typed bools, so a mis-ordered argument cannot silently change which set runs. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Covers the `rocm remote` surface, and in docs/testing.md how to run the two container-backed lanes — including the ROCM_TEST_APK_REPOS escape hatch a network that intercepts TLS needs to build the fixture image. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
b2c5dd9 to
f24b76d
Compare
f24b76d to
b0657e7
Compare
|
@juhovainio flagging this since the Tightening the chain broke remote serve outright. Fixed by sending the key as a complete line. That is also what gives the short circuit its meaning: a truncated write delivers no newline, so the read fails and nothing starts holding half a key. Without the terminator the chain refuses every start. My first test used closed stdin only, so it passed with the bug present. It now covers three cases (nothing, unterminated, complete line), plus an assertion at the real call site that the stdin actually sent ends with a newline. The other minors are in as well: staging dir 0700, remote-ssh gated on clippy and prek, and the teardown check-then-act window recorded in Worth a re-read of the fix commit if you want it, given it is the credential path. |
|
🔴 Automated review · pr-review-watcher · cc276ef This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryAdds the 🚫 Blocking (must fix before merge)1. 2. 3. Non-blocking
|
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
pr-review-watcher · b0657e7
One blocking item, and it is a one-line fix -- nothing else here gates the merge, and the credential-path work itself holds up under mutation.
The comment that justifies demoting a broken pipe on the remote transport names its tripwire test by a name that does not exist anywhere in the tree. The test that actually pins the property is the one asserting that only a complete key lets the command reach serve. This is the single comment telling a future maintainer that the read-before-serve coupling is enforced by the shell rather than merely asserted in prose -- and it sits on the credential path, in a commit whose whole point is that loosening that chain broke remote serve outright. Someone who greps for the named test finds nothing, and the most available conclusion is that the guard is not real, which is exactly the licence to loosen the chain again.
The guard itself is genuinely there and genuinely tested: loosening the chain, dropping the newline terminator, and reverting the staging-directory command each turn the relevant test red. Only the pointer is wrong. Correcting the referenced name clears this.
Raised as blocking rather than as a note because a wrong cross-reference on a credential path that has already regressed once is the kind of thing nobody returns to after merge.
b0657e7 to
fcee6e7
Compare
|
Fixed in Swept the rest of the remote module for other dangling references while I was in there. One apparent hit, Your reasoning for raising it above a note is fair for this particular comment. It is the only place recording that the read-before-serve coupling is enforced by the shell rather than asserted in prose, it sits on a path that regressed once already, and a dead pointer reads as evidence the guard was never real. Noting the mutation testing specifically, because it is the part that made this review worth reading: loosening the chain, dropping the newline terminator, and reverting the staging-directory command each turning the relevant test red is a check on my work rather than a restatement of it. That is a different thing from reading the test bodies and concluding what they would do. |
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · fcee6e7
This automation 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 — SSH as control channel, with the GPU machine publishing its own loopback-bound model port to the tailnet — plus a new rocm serve --require-api-key flag and split download/install modes in the installer. The work is careful and unusually well-reasoned, but two guarantees stated in prose are not backed by the code or by a test that can fail for them.
Verified: the three mutation claims in the round description all hold — loosening the && chain, dropping the newline terminator, and reverting the staging-directory command each turn the named test red. The comment in transport.rs now names a test that exists and asserts what it promises, and a sweep of every backticked identifier in the remote module found no other dangling reference. The 32 new high-severity "cleartext logging" alerts were read per site: 29 fall inside #[cfg(test)] mod tests (which begins at apps/rocm/src/remote/mod.rs:1177) and are fixture strings — false positives; of the three production sites, :856 (render_status) and :1074 (render_stopped) never receive or emit a credential, only a key-file path — false positives; :489 is a real taint flow but by design, a one-time display of a freshly minted key to the operator who asked for it, with the key also written at mode 0600. No true positive reaches production. The full test suite was not run here, and nothing was exercised against a real tailnet or a real GPU. One claim from a deeper pass was checked and refuted: the container examination fixture omits three Option fields, but it deserializes and the unit test pinning it passes, so that is not a defect.
Checks at review time: 23 success, 2 failure, 3 pending, 1 skipped.
Blocking: 2 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
apps/rocm/src/remote/provision.rs:164 — the test passes with umask 077 removed.
staging_dir_command is (umask 077 && mkdir -p {dir}) && chmod 700 {dir}, and the doc comment above it states the umask half is load-bearing: "umask 077 covers the creation itself — a mode applied afterwards leaves a window at whatever the remote's umask happens to be". Mutating the function to mkdir -p {dir} && chmod 700 {dir} — deleting only that half — leaves the_remote_staging_directory_is_restricted_to_its_owner (line 590) green, 1 passed. The test asserts the mode of the leaf directory only, and the trailing chmod sets the leaf either way. Deleting the chmod half is caught, via the stale-directory case, so it is specifically the branch the comment calls essential that nothing can fail for. This is a credential staging directory on someone else's machine, so the window is the point of the change.
Fix: assert the parent directory's mode as well. mkdir -p creates the parent and the chmod never touches it, so under the harness's umask 022 it is 0o700 with the umask and 0o755 without — confirmed by direct experiment. One extra assertion on the parent makes the umask branch fail-able.
apps/rocm/src/main.rs:6296 — --require-api-key is silently dropped when a managed service is reused.
spawn_managed_engine_child returns ManagedSpawn::AlreadyRunning at the existing_live_managed_service check, which matches on engine and canonical model only and whose sole mismatch guard is the engine recipe. That early return happens before record.requires_api_key = require_api_key (line 6355) and before ensure_public_service_has_endpoint_key(...) (line 6404). So when rocm remote serve reuses a model already served managed on the requested port, the flag is accepted and ignored: the endpoint stays unauthenticated, the caller's freshly minted key is dropped, and render_started (apps/rocm/src/remote/mod.rs:667) nevertheless prints the key alongside "The API key above is what stops anyone else calling it." That sentence is then false about a tailnet-reachable endpoint. Nothing in the changed tests varies require_api_key across the reuse branch — spawn_managed_engine_child_blocks_reuse_with_mismatched_recipe only varies the recipe.
Reachability is narrower than it first looks and worth stating honestly: the default remote port is 11434 while a local rocm serve defaults to 11435, and the publish step discovers by exact port, so an all-defaults collision fails closed with "the remote started no service on port 11434; nothing to publish" rather than publishing. A second rocm remote serve at the same target and port is refused by the session guard. The exposure needs a service already running for that model on the requested port — e.g. someone had run rocm serve MODEL --port 11434 on the box, 11434 being a familiar choice. Narrow, but the failure mode is an unauthenticated model endpoint on the tailnet under an explicit printed assurance that it is protected.
Fix: treat the reuse branch as a decision point rather than a shortcut. Either refuse reuse when require_api_key is set and the existing record has requires_api_key == false (symmetric with the existing recipe-mismatch refusal, and the message can tell the user to stop the existing service), or upgrade the record and re-run ensure_public_service_has_endpoint_key before returning AlreadyRunning. Add a test that reuses a service with requires_api_key == false while requesting the flag, and asserts the call does not return a satisfied AlreadyRunning — that is the assertion that can fail for this defect.
Non-blocking
apps/rocm/src/remote/provision.rs:158— the doc comment sends the reader tothe_staging_path_still_expands_on_the_remote_shellfor the unquoted$HOME, but that test exercises the archive-install command, not this one;$HOMEexpansion is unproven for this function.apps/rocm/src/remote/mod.rs:489— the one by-design credential print leaves the next reader re-triaging 32 alerts; a targeted suppression with a one-line reason at this single site, and nowhere else, makes the triage legible once instead of every round.tests/remote-ssh/run.sh:198— both branches of the batch-mode counterfactual callpass, so it cannot fail yet counts toward the stated 21 checks; make the environment-dependent branch askipso the total reflects what was actually asserted.apps/rocm/src/remote/transport.rs:142— the control-socket path length guard counts the two-byte%Cplaceholder as if it were literal, so it refuses paths that would in fact fit. Over-conservative only.apps/rocm/src/remote/session.rs:61—RemoteSessionRecord::id_forstrips non-alphanumerics, so two distinct all-punctuation peer hosts collapse to the same session id for a given port.
The remote serve command joined the API-key `read`, the export, and `rocm serve` with `;`, so the compound's exit status was whatever `serve` returned. `run_with_piped_io` demotes a broken-pipe write error to a symptom whenever the command also failed, and that is only sound if a failed read cannot be followed by a successful serve. The ordering held by prose, not by the shell: reorder the command and a truncated key would classify as success on the one path guarding the model. `&&` makes the shell hold it instead. Tightening the chain also means the key has to arrive as a complete line. `IFS= read -r` returns non-zero at EOF without a terminator even though it did assign the variable, so the bare key the caller used to send now short-circuits the chain and nothing starts. Send it with a trailing newline, which is also what gives the short circuit meaning: a write truncated part-way delivers no newline, so the read fails and no model comes up holding half a key. Both halves are pinned against a real shell — the command's exit status for the chain, and the recorded stdin at the real call site for the terminator. A string assertion can see neither. Also create the remote staging directory 0700, matching the local `create_restricted_dir`. Its test asserts the parent's mode as well as the leaf's, because `chmod` names the leaf and would set it either way: checking the leaf alone passed with `umask 077` deleted, leaving the half the doc comment calls load-bearing unable to fail. A parent that `mkdir -p` created is covered by the umask and by nothing else. The archive, checksum, and signature sit there before any of them is verified, so a permissive umask on a shared box let another local user read or race them. `umask 077` closes the window at creation and the chmod repairs a directory an earlier run left open. Gate the remote-ssh CI job on clippy and prek and skip it on manual dispatch, as every other heavy job in the workflow already does, so a fast lint failure short-circuits before the containerised build. Record the teardown check-then-act window in `withdraw` as a known limitation: tailscale offers no compare-and-swap on a port, so only a daemon holding the claim could close it, and re-reading state before the exec would narrow the window while implying it was shut. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
`spawn_managed_engine_child` treats a live managed service for the same engine and model as satisfying the request. That early return happens before `record.requires_api_key` is set and before the endpoint key is checked, so `--require-api-key` was accepted and discarded: the caller got a server that never had a key, and no error. `rocm remote serve` is what makes that dangerous. It publishes the port onto the tailnet and prints the key it just minted under "the API key above is what stops anyone else calling it". Reusing an unauthenticated service made that sentence false about an endpoint every machine on the tailnet can reach. Refuse instead of upgrading the record. The engine reads its key once, at launch, so a server already running without one keeps serving anonymously whatever is written afterwards; flipping the flag and writing a key file would leave the record claiming auth that nothing enforces, which is worse than the original bug. The refusal names the service to stop, and is the same shape as the recipe-mismatch refusal beside it. Reachability was narrow: the remote path defaults to a different port than local serving, and publish discovers by exact port, so an all-defaults collision already failed closed. It needed a service already serving that model on the requested port. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
fcee6e7 to
cc276ef
Compare
|
Both blocking findings fixed. The staging assertion in The The parent assertion needed one correction before it worked. Applied to both cases it fails for the wrong reason: the stale case pre-creates its parent inside the test, so
The test drives This one is in its own commit rather than amended into the remediation commit. An unauthenticated-endpoint fix filed under "let the shell enforce the read-before-serve invariant" is not something anyone would find later. On the CodeQL triage. Your per-site reading matches mine and went further than mine did: 29 inside the test module, Not addressed here, and neither is caused by this branch: the Strix Halo WSL2 lane 404s on Verification: |
Superseded by a fresh review at the current head.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · cc276ef
This automation 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 the rocm remote command family — provision a tailnet GPU machine over SSH, start a loopback-bound model on it, publish that port to the tailnet, then status/attach/stop — together with a new rocm serve --require-api-key flag, services list --json, split download/install modes in the installer, a Gherkin feature, and a containerised SSH harness. Outcome: needs work. Both findings raised last round are genuinely fixed, and I confirmed each by mutation on a scratch copy rather than by reading: deleting the new reuse guard makes spawn_managed_engine_child_refuses_to_reuse_an_unauthenticated_service fail, and the reworked staging-directory test is now falsifiable from both directions — dropping umask 077 fails on the parent at 0755, dropping chmod 700 fails on the stale leaf at 0755, which is exactly the half that could not fail before. Beyond that I mutation-tested the publish-state classifier (both the foreign-forward and funnel-allowed arms go red when removed), the && chain and the newline-terminated key (each has its own red test), the peer-ordering parse, the docker skip gate, and the session credential-claim path; I also read the serve, teardown and unwind flows end to end against their callers. Three problems remain, two of them tests that cannot fail for the thing they name, and one correctness gap on the path this round just remediated. Not verified here: the full suite was not run, and nothing was exercised against a real tailnet or real GPU hardware — the container stand-ins are the same ones the change ships. Checks at review time stand at 22 success, 3 failure, 3 pending, 1 skipped; two of the failures are also red on the base branch's own tip and so are not attributable to this change, while the third is a static-analysis lane that is green on the base tip and may be this change's own — I could not confirm which job that is, so treat the attribution as inference, not fact. Blocking: 3 · Non-blocking: 8.
🚫 Blocking (must fix before merge)
1. apps/rocm/src/remote/tailnet.rs:404-469 — the peer-ordering test passes with the sort deleted. parsing_orders_peers_and_strips_the_magicdns_trailing_dot asserts the peer list comes back as ["gpu-box-1", "gpu-box-2", "phone"], and its comment states the property under test: an unsorted list would shuffle between runs. But RawStatus::peers is a BTreeMap, so into_values() already yields entries in key order, and the fixture's keys are nodekey:aaa → gpu-box-1, nodekey:bbb → gpu-box-2, nodekey:ccc → phone. The map's natural order is therefore identical to the sorted order the test asserts, so peers.sort_by at lines 225-230 contributes nothing to the result. Removing that call entirely leaves the whole remote::tailnet test module green — confirmed on a scratch copy, 15 passed. The production sort is correct and wanted; the test simply provides no coverage for it, which is the failure mode this review treats as a hard blocker. Fix: permute the fixture's map keys so insertion order and host order disagree — e.g. key the phone peer nodekey:aaa, gpu-box-1 nodekey:bbb, gpu-box-2 nodekey:ccc — and keep the assertion on ["gpu-box-1", "gpu-box-2", "phone"]. The sort then becomes load-bearing to the assertion. While in there, the secondary dns_name tie-break is likewise unexercised, since no two fixture peers share a host name.
2. apps/rocm/src/remote/mod.rs:345-489 and :667-695 — remote serve prints and stores an API key the endpoint does not accept, when the remote reuses an already-authenticated service. The new guard added this round (apps/rocm/src/main.rs:6302-6331) refuses reuse only when the caller asked for --require-api-key and the running service has none. The symmetric case is untouched: if the running service on that port does require a key, reuse is satisfied, drop_orphaned_endpoint_key_on_already_running discards the key the remote was just handed on stdin, and the engine keeps enforcing the one it was launched with — the same "a running server cannot be given a key it did not start with" reasoning that motivated the refusal. The controlling side never learns this. It minted api_key, wrote it to the session key file, and reaches render_started, which prints api key: {api_key} unconditionally and follows it with "The API key above is what stops anyone else calling it." The remote's stdout is not surfaced — only start.success is consulted at line 406 — so the user is told the command succeeded and handed a credential that will be rejected, with the only stored copy on disk being the wrong one. The local serve path already avoids exactly this, at apps/rocm/src/main.rs:5871-5890, where launched_key is forced to None on already_running precisely so the CLI does not print a key it did not install; the remote path has no equivalent. This is reachable along a route the code itself advertises: rocm remote stop … --force clears the session key and removes the record even when the model could not be stopped (mod.rs:1076-1077), and both non-force failure messages recommend it by name — after which the model is still running and authenticated on that port, and the next rocm remote serve for the same model and port lands in this case. Fix: make the remote path fail closed the way the local path stays honest. The cheapest version with the data already in hand is for serve_with_transport to record a timestamp before exec_with_stdin and require discover_started_service to return a record created after it, bailing with the same shape of message as the reuse refusal (name the service and tell the user to stop it) when the port is held by something this command did not start. Alternatively, have the remote report reuse explicitly so the controlling side can refuse rather than infer. Either way the invariant to establish is that render_started only ever prints a key that the running endpoint actually enforces.
3. tests/remote-ssh/run.sh:185-207 — the batch-mode counterfactual reports pass on every outcome, including the one its comment calls unusable. The comment above it states the purpose plainly: this is "what makes the check above mean anything" — without batch mode, the same account and server should sit waiting for input, which is the hang batch mode exists to prevent. The check runs the command under a pty and expects a timeout (rc=124). When it gets one it passes; when it does not, the else arm also passes, with a message that says the host "does not refuse cleanly". So no result can fail it, and the harness reports a green check for a run that demonstrated nothing. The adjacent branch three lines below already handles the same situation correctly — when script is unavailable the harness prints skip, because the environment cannot show the behaviour. Fix: use skip in the else arm too, so an environment that cannot reproduce the hang is reported as undemonstrated rather than verified. That keeps the check non-flaky (the stated reason for tolerating the other outcome) while making the pass mean what it says.
Non-blocking
apps/rocm/src/remote/mod.rs:636-659—discover_started_servicelists with--alland picks the newest record on the port, so a stopped record created after the live one is a candidate, and ties oncreated_at_unix_msresolve by iteration order rather than by anything meaningful. Filtering to live records, and stating the tie-break assumption, would make the selection say what it means.apps/rocm/src/remote/session.rs:276-278—clear_key's path-traversal guard is not constrained by any test. Removing thevalidate_idcheck leaves all 12 tests in the module green, includingan_unusable_id_cannot_place_a_credential_outside_the_sessions_directory, because that test callsclear_key(&paths, "../../escaped")and asserts nothing afterwards: an unguarded removal of a file that does not exist is indistinguishable from a guarded no-op. Planting a sentinel file at the traversal target and asserting it survives would make the guard testable. The store side of that test was not mutation-tested here, so this observation is scoped to theclear_keyhalf.apps/rocm/src/remote/session.rs:61-77—id_formaps every character outside[A-Za-z0-9-]to-, sogpu.box,gpu_boxandgpu-boxcollapse to one session id on the same port.store_key's exclusive create bounds the damage to a confusing refusal rather than a credential overwrite, but the refusal will name a machine the user is not using. Folding a short hash of the untransformed host into the id would remove the collision..github/workflows/ci.yml:863-886— theremote-sshjob carriesneeds.changes.outputs.heavy == 'true'in its job-levelif:, while every sibling heavy job (build-and-test,test,windows-build-and-test) keeps only the dispatch condition at job level and pushes the heavy filter down to its steps, specifically so the job always reports. The job's own comment says it is "gated like every other heavy job here", which is what sent me looking for a merge-queue hazard that is not there: thechangesjob forces every category true on push and in the queue, so this job always runs where a required check matters. The comment is the problem, not the gating. Naming the difference and why it is safe — one clause — would stop the next reader retracing this.apps/rocm/src/remote/transport.rs:123-145— the control-socket length guard computescandidate.len() + 40 < 100, wherecandidatealready contains the two literal bytes of the%Cplaceholder that the 40-byte expansion replaces. The guard is conservative by two bytes, never optimistic, so it is safe as written; subtracting the placeholder first would make the arithmetic match the comment.tests/e2e-cucumber/features/remote.feature:90-97withtests/e2e-cucumber/tests/e2e/remote_steps.rs:663-668— the endpoint-restore scenario is the one whose premise is that the machine and the CLI have diverged, yet its final assertion reads only the CLI's own printed strings, where its siblings (then_machine_publishing,then_publishing_nothing) assert against real container state. It is not vacuous — "Endpoint re-published" is printed only afterpublish::publishreturnsOk— but atailscale serve status --jsoncheck inthen_endpoint_restored, mirroring the sibling steps, would match the scenario's own premise.apps/rocm/src/remote/mod.rs:1028-1043— when neither the withdraw nor the model stop succeeds and--forcewas not given, the bail reports only the withdraw failure; the stop failure is already known instop_failureat that point and is dropped. Nothing is lost (the session and key are kept either way), but the user retries without knowing both halves failed.apps/rocm/src/main.rs:6413-6425— when a service launched with both--allow-public-bindand--require-api-keylater loses its key, the remediation text names only--require-api-keyfor the relaunch, which would not restore a non-loopback bind. Reachable only through that redundant flag pairing, which the remote path never produces.
The reuse guard added for `rocm serve` covers one direction: a caller asking for a key when the running service has none. The mirror was left open. When the running service does have a key of its own, the remote reuses it, discards the key it was just sent on stdin, and exits 0. The controlling side reads only that exit status, so it publishes the port and prints the key it minted under "the API key above is what stops anyone else calling it" -- a credential the engine will reject, stored as the only copy the user gets. Reachable along a route the code recommends by name: `rocm remote stop --force` clears the session key and drops the record even when the model could not be stopped, and both non-force failures suggest it. The model is still running and authenticated on that port afterwards. Snapshot the live service ids on the port before starting, and refuse when the service discovered afterwards is one of them. Ids rather than timestamps because `created_at_unix_ms` is stamped by the remote's clock, so comparing it against ours would turn ordinary skew between two machines into a spurious refusal or a missed one. The snapshot omits `--all`: only a live service can be reused, and a stopped record sharing the port is not something the remote could hand back instead of a fresh start. Also make two checks able to fail for what they claim. `parsing_orders_peers_and_strips_the_magicdns_trailing_dot` asserted a sorted peer list, but `Peer` deserialises into a `BTreeMap` and the fixture's node keys ran in the same order as its host names, so the sort contributed nothing and deleting it left the module green. The keys are now permuted against the host names. `run.sh`'s batch-mode counterfactual called `pass` on both branches, including the one whose message says the host did not refuse cleanly, so no outcome could fail it. An environment that cannot reproduce the hang now reports `skip`, matching the adjacent branch that already does this when no pty is available. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
|
Re-triaged the 32 CodeQL alerts against the current head rather than from memory, since the line numbers moved with the last few commits. Same 32, same rule ( The test module starts at line 1253, which splits them cleanly: 29 are inside 3 are production, and two of those never touch a credential. One is a real taint flow, and it is the intended behaviour. Line 513, So: 29 test fixtures, 2 non-secret identifiers, 1 deliberate one-time display. False alarms in substance. Two things worth recording so nobody repeats this exercise. First, these alerts do not block anything. Second, the reason all 32 appeared at once is worth knowing. This check reported "No new alerts" two commits earlier, and the only change between those heads was a single comment line. CodeQL's own summary explains it: "Alerts not introduced by this pull request might have been detected because the code changes were too large." The entire The suggestion from the automated round still stands as the better long-term answer: a targeted suppression with a one-line reason at line 513 only, and nowhere else, so the single by-design flow is marked once instead of re-triaged every round. That is a security-annotation change rather than a code change, so I am leaving the call to the repo owner rather than making it here. |
5 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.