Repository navigation
feat(ssh): integrate SSH terminals and encrypted key relay - #379
Conversation
theogravity
changed the title
| @@ -16,6 +16,7 @@ export const DATA_COMPONENTS = [ | |||
| "vapid.json", | |||
| "peers.json", | |||
| "identities", | |||
| "ssh-relay", | |||
There was a problem hiding this comment.
MAJOR (backup/restore gap). This PR adds ssh-relay to the archive allow-list so the plane's relay identity material is backed up, but the sibling trust file the relay depends on, <dataDir>/ssh-machine-pins.json (pane-runtime machine-pin-store.ts: "The file is <nodeDataDir>/ssh-machine-pins.json (0600)"), is not a DATA_COMPONENTS member. Every comparable trust file is here: node-signing.json, node-encryption.json, and peers.json, which is the channels pin store that the exact same TOFU-plus-quarantine doctrine got.
Failure scenario: an admin restores an instance archive. The relay keypair comes back inside ssh-relay/, the pin store does not. MachinePinStore starts with no pins, and the next relay open TOFU-pins whatever keys the identities table presents, with no challenge and no signal. The byte-equality defense this spec spends §4 on silently resets across a backup/restore cycle, while the identical channels defense (peers.json) survives it. If restore should not carry the store, that asymmetry needs a decision sentence like the archive docs carry elsewhere; if it should (peers.json says it should), the one-word fix is:
| "ssh-relay", | |
| "ssh-relay", | |
| "ssh-machine-pins.json", |
There was a problem hiding this comment.
Not a gap. On the plane the relay pin store is not a data-dir-root file: localRelayIdentityDir() returns join(SUBSHELL_SERVER_DATA_DIR, "ssh-relay"), and machinePinPath puts ssh-machine-pins.json inside it, so the server's copy lives at ssh-relay/ssh-machine-pins.json, which the already-allowlisted ssh-relay component covers. The backup test at services/backups/tests/backups.test.ts:172-182 round-trips exactly that path. Only agent nodes keep the store at their own data-dir root, and those files are outside the instance archive by design (they're not the plane's data). Leaving the allow-list as is.
|
Review finding (repoint timing): the code now hot-applies a server repoint; the security docs still promise the old timing.
"Choosing when is the operator's" is the stated rationale for the old timing: the route's previous docblock named "a machine that vanished mid-request because it silently re-dialed elsewhere" as the surprise the surface exists to remove. An operator who trusts the threat model and treats a repoint as parked until their maintenance window discloses the node key to the named host fifteen seconds after the click. Per the repo's precedence rule (docs/security.md is authoritative, and a code/doc disagreement is a bug worth fixing rather than a choice to make silently), §6 and the rules summary should be corrected in this same change, with the amendment dated the way other reversals are recorded. |
| void sweepSshDirs().catch((err: unknown) => { | ||
| log(`ssh dir sweep failed: ${err instanceof Error ? err.message : String(err)}`); | ||
| }); | ||
| const sshDirSweepTimer = setInterval(() => { |
There was a problem hiding this comment.
MINOR (cleanup leak). sshDirSweepTimer is built beside its two siblings and unref'd like them (line 667), but unlike them it is never torn down: the finally at 1454 clears transferSweepTimer and retentionTimer only. The heartbeat block's own comment (line 397) says the timers are "removed on every exit path"; this one is not.
Failure scenario: daemon() is re-enterable in one process (test seams and the restart fixtures do this), and every exited instance keeps an hourly sweep firing against its stale config.dataDir/ctx.meta - two live sweeps double-deleting ssh dirs off different meta snapshots is only the loud version; the quiet one is just a leaked closure holding the old daemon world alive forever. Prod exit goes through stop()/process exit so the unref keeps that harmless, which is exactly why the inconsistency survives: the file's own doctrine (the maintenance/ssh-enabled memos, the census) is that every armed timer names its disarming line.
| const sshDirSweepTimer = setInterval(() => { | |
| const sshDirSweepTimer = setInterval(() => { | |
| // (finally block gains: clearInterval(sshDirSweepTimer); beside the transferSweepTimer line) |
There was a problem hiding this comment.
Fixed in b9d87a0: the daemon's finally block now clears sshDirSweepTimer beside the transfer and retention sweeps, so a re-entered daemon no longer leaves an exited instance's hourly sweep firing against a stale config.
| void sweepSshDirs().catch((err: unknown) => { | ||
| log(`ssh dir sweep failed: ${err instanceof Error ? err.message : String(err)}`); | ||
| }); | ||
| const sshDirSweepTimer = setInterval(() => { |
There was a problem hiding this comment.
MINOR (cleanup leak). sshDirSweepTimer is built beside its two siblings and unref'd like them (line 667), but unlike them it is never torn down: the finally at lines 1454-1455 clears transferSweepTimer and retentionTimer only. The file's own doctrine around the heartbeat and sweep timers is that everything armed here is removed on every exit path; this timer is the exception.
Failure scenario: daemon() is re-enterable inside one process (the test seams and restart fixtures do exactly this), and each exited instance keeps its hourly sweep firing against a stale config.dataDir/ctx.meta - overlapping sweeps double-deleting from different meta snapshots is the loud version, the quiet version is a leaked closure pinning the whole old-daemon world forever. Production exit goes through stop()/process exit, and the unref keeps that harmless, which is precisely why the omission stays invisible: the fix is one clearInterval(sshDirSweepTimer); line beside the other two in the finally block.
There was a problem hiding this comment.
Duplicate of the sibling comment; same fix landed in b9d87a0.
| // — an agent that predates M2, or one whose block the grammar refused — | ||
| // writes NOTHING: absence is not evidence the machine's keys are gone, | ||
| // and only a fresh true report may move the plane's copy. | ||
| if (event.runtime?.sshFingerprint) { |
There was a problem hiding this comment.
MINOR (efficiency, one change with two costs). Before this PR a heartbeat was {type, ts} and the runtime was collected once at boot (ready) plus after service commands. Now every 15-second beat from every node carries the full runtime block, and on this line the plane rewrites nodes.ssh_fingerprint with a fresh JSON round-trip on every beat that carries one, with no comparison to what the row already holds. On the node side the paired half: apps/node/agent/src/daemon.ts line 1284 fires void refreshRuntime() every beat, which re-runs collectRuntime through queryService, i.e. spawns the host's service-manager query on a 15-second cycle, forever, on every enrolled machine.
Concrete cost: with N nodes this is N DB writes per 15 seconds against SQLite's single writer on top of the existing heartbeat touch, plus N service-manager process spawns per 15 seconds on each machine's fleet, to re-report a value (sshFingerprint) that changes exactly when a pairing is created or repaired, and supervision facts that change when the operator edits the service. One JSON-equality guard here, and a refresh cadence tied to the beats that can actually change a fact (service commands, a re-pair, a config reload), keep the mirror honest without the churn. The heartbeat payload itself also grows into the frame budget relayFrameRefIfOverCap shares with the pane wire.
There was a problem hiding this comment.
Checked against main and this is mostly pre-existing. git diff origin/main...HEAD on daemon.ts shows main's heartbeat already sent {type, ts, ...(runtime ? { runtime } : {})} and already called refreshRuntime() every beat; this PR only merges a cached sshFingerprint into that same runtime field (no new per-beat key read or spawn). And the setSshFingerprint write is in the ready handler (node-ws-handler.ts:897), once per connection and gated on event.runtime?.sshFingerprint; the heartbeat case (1026) only sets conn.agent.runtime in memory and touches lastSeen. So no per-beat SQLite write or per-beat service spawn is introduced here. Leaving as is.
| } | ||
| } | ||
| const rows = await new NodeSharesRepository(db).replaceForNode(gate.row.id, entries, user.id); | ||
| await getRelayBroker().closeUnauthorizedForNode(gate.row.id); |
There was a problem hiding this comment.
MINOR (revoke path conflates authorization with liveness). closeUnauthorizedForNode re-authorizes every live session through gateSshNode, whose blocker table answers NODE_OFFLINE, NODE_PROTOCOL_HELD, and NODE_IN_MAINTENANCE alongside the real SSH_GATE_OFF/access questions. The consequence of running the sweep on EVERY shares replacement: an owner edits shares for an unrelated reason (or adds a grantee, which revokes nobody) while one of the machines in a live pairing has a momentary link blip, and its in-flight SSH handshakes are cut with an access-revoked word that says the wrong thing. The pane survives (the ssh child is already authenticated), but the retry is a fresh launch the user did not ask for.
The rule the relay already honors elsewhere is that a revocation is asserted where the audience is addressable exactly and asked otherwise: here, a session should be closed when its user has actually lost the SSH gate on either row (sshEnabled, launch access, shares), and a machine that is merely offline right now cannot host a live frame anyway, so a blip needs no cut at all. Narrowing authorize here to the authorization blockers (dropping offline/held/maintenance from this one decision) keeps the off-switch instant and stops the accidental tears.
There was a problem hiding this comment.
Fixed. closeUnauthorizedForNode now consults a new authorization-only revoked seam (production: sshAccessRevoked) instead of the full connect gate, so offline/held/maintenance no longer read as a revocation on a shares edit; liveness stays with the link, caps, and pane cuts. Pinned by two tests: a broker-level blip-vs-revocation test and a route-level test that an unchanged shares write with B offline leaves the pairing standing.
| * reaching a future audit row through a field that promises to be a | ||
| * fingerprint. | ||
| */ | ||
| const SSH_FINGERPRINT_RE = /^SHA256:[A-Za-z0-9_-]{1,128}$/; |
There was a problem hiding this comment.
MINOR (two grammars for one canonical value). The same SHA256:-prefixed digest is validated two ways: here /^SHA256:[A-Za-z0-9_-]{1,128}$/ for selections and rosters, and packages/subshell-protocol/src/node-frames.ts:554 /^SHA256:[A-Za-z0-9_-]{43}$/ (report form) for the runtime/mirror block. A SHA-256 base64url digest is exactly 43 characters; the loose spelling accepts 1- to 128-character values of the same alphabet.
Concrete cost: the launch route's fingerprints array passes isSshFingerprints with a value like SHA256:x or a 120-character paste; it is then refused downstream (roster membership, byte-equality) with the same word an honest mismatch gets, because the grammar never told the caller the SHAPE was wrong. And the two constants invite drift: the 43-form is the only one parseNodeSshFingerprintReport will ever accept, so a future producer has two "canonical" validators to choose between and they disagree about length. This mirrors a doctrine the rest of the relay code already applies to itself (ssh-relay.service.ts: "two carriers of one canonical spelling must not be two implementations of it"), and the fix is that: one exported SSH_FINGERPRINT_RE in the 43 form, imported at both sites. If the loose length range is deliberate for some future digest family, the two constants should say so in a comment naming why the API and the report differ.
There was a problem hiding this comment.
Fixed. One canonical value, one predicate: ssh-frames.ts now enforces exactly the 43 chars of a SHA-256 base64url digest, and node-frames.ts imports isSshFingerprint from it instead of carrying a second regex. Test fixtures that leaned on the loose bound were updated to real-width digests, plus an explicit short-digest refusal case.
| ); | ||
| const machines: ({ node: Awaited<ReturnType<typeof toNodeView>>; ownerLabel?: string } & Awaited< | ||
| ReturnType<typeof sshReadiness> | ||
| >)[] = []; |
There was a problem hiding this comment.
MINOR (efficiency, N+1 with a serial walk). findAccessible(user.id) has already resolved the caller's rows; the loop then re-reads every row plus its shares through loadNodeGate, awaits toNodeView (which does its own per-row reads), and awaits sshReadiness(gate), all serially. That is a chain of roughly three round-trips per machine run one after another, and this endpoint is what the SSH wizard's machine step opens on, so its latency is a user's Connect click.
Concrete cost: a fleet of ten machines pays ten serialized gate+view+readiness walks where one batched pass would do (loadNodeGate is over the rows findAccessible already handed back, and sshReadiness mostly reads the live-registry memo and the already-held row). Cheapest honest fix: map the rows concurrently with Promise.all and keep loadNodeGate for the access facts, or extend findAccessible to return the shares it resolved. The shape matches the repo's precedent for exactly this class (GET /api/admin/updates renders its whole node list from batched reads for the same reason).
There was a problem hiding this comment.
Fixed. The two instance-level facts (lockdown, serverAccountEnabled) are now read once via sshPolicyConfig(), and the per-machine gate/view/policy build runs under Promise.all instead of a serial walk, so N machines cost one round of shared facts rather than N serialized chains.
| try { | ||
| // Atomic with enrollment's compare-and-set: never erase its consumed | ||
| // record between a state read and deletion. | ||
| await db.deleteFrom("nodeSetupKeys").where("id", "=", keyId).where("usedAt", "is", null).execute(); |
There was a problem hiding this comment.
MINOR (revocation races an installer that outlives the poll). revokeUnspentKey is atomic against enrollment's compare-and-set, but the read-check-delete cycle still races the installer across the gap between them, and the exec child keeps running when the plane stops watching: ssh_exec's own doctrine is that timed_out "signals nothing" and the command continues. So: a link blip makes sshExecStatus throw exec-lost-* (or the poll deadline passes), enrolledResult finds the key unspent, revokeUnspentKey deletes it, and seconds later the still-running installer reaches the part that spends it and answers the first refusal word ("setup key was rejected"). The user sees "...No machine was enrolled" for a destination that, a beat later, has a complete Subshell install with no node row, which no surface lists.
The refusal sentence is already wrong at the moment it is printed (enrolledResult is one point-in-time read), so the cheap containment is in the message and the audit: name that the installer may still be running and its key has been retired (the remedy is to re-run setup after the machine settles, or check Nodes), and add a late enrolled follow-up the same way the ready-wait path already upgrades a refusal into connected: false after enrolledResult re-checks. An unspent-key delete-on-refusal is still the right default; what must not ship is a confident past tense about an installer the code knows it stopped watching.
There was a problem hiding this comment.
Fixed. The act now tracks an execMayStillRun flag and only revokes an unspent key where the child is proven gone: the terminal ssh_exec_status answer, or a kick that proved nothing started (offline/refused/unsupported). On the two doubt paths (a lost status call, the plane's poll deadline) the key survives its single-use expiry, so a late installer that finishes enrolls successfully instead of half-provisioning against a deleted row. Two regression tests cover both give-ups.
sshDirSweepTimer was armed beside the transfer and retention sweeps but the daemon's finally block only cleared those two. daemon() is re-enterable in one process (test seams, restart fixtures), so each exited instance kept firing its hourly sweep against a stale config, and the leaked closure pinned the old world. Prod exit masked it because the timer is unref'd.
Six findings from the #379 code review, each with a test: - Repoint timing (docs): the agent hot-reloads a `set_server_url` and re-dials on its next heartbeat beat (~15s, no restart), but docs/security.md and the security rules still said "takes effect next start." Updated both to match the shipped behavior. - Shares revocation conflated authorization with liveness. A node edit ran the full connect gate against live relay sessions, so an offline/held/maintenance machine read as "access-revoked" and cut sessions the shares change said nothing about. closeUnauthorizedForNode now consults a new authorization-only `revoked` seam; liveness stays with the link, caps, and pane cuts. - The setup key raced an installer that outlives the poll. The act deleted an unspent key on every give-up, including the two paths where the exec child may still be running (a lost status call, the plane's poll deadline). The key now survives those paths so a late enroll succeeds; it is still revoked when the child is proven gone. - The SSH fingerprint grammar split. ssh-frames.ts accepted 1..128 chars where node-frames.ts demanded exactly 43 (a SHA-256 base64url digest). One canonical value had two disagreeing validators; unified on the strict predicate and node-frames imports it. - Readiness was an N+1 with a serial walk: it re-fetched the instance policy facts and each node's view/gate per row in series. The facts are fetched once and the machines built concurrently. - Backup: the reported ssh-machine-pins.json gap is not real. The plane's MachinePinStore lives under ssh-relay/ (already archive-allowlisted and round-trip-tested); only agents keep it at their data-dir root, outside the instance archive by design.
|
Fixed. docs/security.md §6 and the .claude/rules/security-context.md repoint bullet now state the shipped behavior: the running daemon hot-reloads a |
The arm that actually runs ssh(1) (a launch carrying cmd.ssh) consulted neither the node's fail-closed ssh-enabled mirror nor its maintenance switch, while every other SSH arm gates on both first. It now does: an absent/corrupt mirror or maintenance-on machine refuses the launch (A/B relay-open included) instead of spawning outbound ssh with the account's agent socket exported. gateOpen is one shared helper in ssh-shared.ts now (was duplicated in ssh-aliases/ssh-relay-exec and inlined at three more arms), so the mirror's three-state read has exactly one home. Regression tests: absent mirror refuses an ssh launch by name; maintenance refuses ssh_relay_open for both roles.
- resolve `IdentityAgent unix:/abs` to the socket it names instead of falling through to the env agent (wrong keys on the approved path) - poisonSession now fails the connection whose request is mid-seal, so a forged reply landing in that window tears it down instead of letting the request complete against a refused session - sweepDir unlinks files in place (readdir + unlinkat by name) so the sweep is no longer quadratic from per-entry rmdir+mkdir - the relay-open lifetime is validated before the gate (non-finite JSON numbers like 1e999 previously became an undefined deadline that silently skipped the expiry timer); the sweep cap is now enforced in the runtime Co-Authored-By: Claude <noreply@anthropic.com>
sshAccessRevoked neutralized lockdown and liveness through the policy state, but maintenance is read from the node ROW inside sshPolicy, so a machine slipping into maintenance still made canConnect false and the function returned true. A shares write on the OTHER node then cut a healthy live relay with reason access-revoked (and a misnamed audit row), the exact "a blip is not a revocation" class this round removes. A maintenance-only blocker set no longer revokes; a genuine loss (launch access gone, outgoing SSH off) still does, even alongside maintenance. Also clear sealedInFlight when close() races a resolving seal, so the flag keeps its stated "true while a seal is in flight" invariant. Co-Authored-By: Claude <noreply@anthropic.com>
The "sends the selected key machine" test clicks after renderPanel's fixed 50ms settle, but Start stays disabled until the key-home roster query leaves isPending (rosterReady). On a loaded CI runner that query is still in flight at click time, so the click lands on the disabled gate and sends nothing — the launch body read back undefined and the shard failed green-locally/red-in-CI. Wait for the button to actually enable before clicking; the body assertion is unchanged, so a real launch regression still fails the test. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
SSH terminals now open from the ordinary subshell and workspace forms. Choose the connecting node and destination, then use that node’s keys, selected keys from another accessible node, or a password entered in the terminal. The server can act as either relay endpoint.
This integrates the SSH feature stack as one reviewable unit: #333, #334, #336–#341, and #343–#346. The source branches remain open. It includes the LAN routing changes from #377.
Behavior
Validation
Full workspace build: 35 tasks passed, including mobile and the server SPA.
Type checks: 56 tasks passed. Lint, prose, design, licenses and lockfile checks passed.
Full unit suites: 54 tasks passed using the repository’s test scripts.
SSH browser and integration scenarios passed, including a focused rerun after fixing confirmation/error rendering. Coverage includes Testcontainers password/key login, password retry and non-persistence, saved destinations, shared-pane access, real encrypted relay authentication, unfamiliar destination confirmation, changed-key blocking, revocation and destination enrollment.
New route tests cover account isolation, expired/single-use tokens, permission revocation, alias changes, conflicting trust and cookie-only access. Screenshots checked the confirmation dialog and SSH settings at narrow and wide widths.
Documentation build and export verification passed for all 119 pages. The new guide was checked at 1440px and 390px, with working screenshot zoom and readable content without JavaScript. A repeatable capture script uses two real nodes and a Testcontainers SSH destination.
The screenshot refresh was checked on 11 documentation pages and the marketing site at 1440px and 390px, including image dimensions, zoom, overflow, and documentation without JavaScript.
Automated runtime tests use temporary accounts, databases, SSH fixtures and isolated ports.