Repository navigation
feat(s16): desktop browser proxy lease + sensitive-host filter (#410 slice 2a) - #453
Conversation
… slice 2a, ADR 0091 decisions 2/3/5)
[Medium impact] The PR appears safe to merge; both previous blocking findings are fixed, and no new actionable issues were found. SummaryAdds a separate desktop browser proxy lease with a sensitive-host filter, renewal, and revocation around VM stops and controller restarts.
Reviews (2) · Last reviewed commit: "fix(s16): desktop lease renewal refreshe..." · Reviewed by Greptile |
…d lease issue never fails VM start (Greptile #453)
|
@greptileai please re-review head 4fa48e5 (fixes for both P1s). |
chayapats
left a comment
There was a problem hiding this comment.
Duo review — head 4fa48e59dc94edcad6844a5efcd12f0eb9f79fb5
Verdict:
Verified fixed
- Browsing stops after 512 MiB.
renew()insrc/s15/vm/proxy.ts:189setssession.bytes = 0as well asexpiresAt. Only the desktop timer callsrenew(), so a work-session lease still has one allowance for its life. The new test fails without the reset. - Full proxy blocks VM startup.
issue()insrc/s15/vm/qemu.ts:369is inside the try. A throw leaves the VM running and not uncertain. Confirmed in the code and the new test.
SHOULD 1 — one failed renewal kills browsing until the next boot
src/s15/vm/qemu.ts:376-382
The 10-minute beat treats any inspect() throw as "not running" and closeDesktopLease() cancels the timer and closes the proxy lease. Nothing re-issues until the next launch() or controller reattached().
inspect() always takes the same QMP socket (qemu.ts:241, 30s timeout) that snapshot holds for up to 300s (qemu.ts:469) and snapshot-delete for up to 120s. QEMU's chardev drops the accept callback while a client is connected (char-socket.c tcp_chr_new_client), so the renewal's QMP waits for a greeting that does not come. If that wait exceeds 30s, inspect() throws, the standing lease is revoked, and the VM keeps running with dead credentials in /etc/jit-desktop/proxy.env. Any other inspect() throw (image probe, unit show) does the same.
A missed beat is already safe: the lease is issued for 30 minutes and renewed every 10. Revoking on "unproven" is what makes a busy socket permanent.
Fix: on throw, skip the beat. Revoke only when inspect() returns a non-running state. Keep the timer so the next beat can renew.
SHOULD 2 — a failed issue or delivery is never retried
src/s15/vm/qemu.ts:369 and the early return at 372-374
Asked explicitly: a single attempt with no retry is not acceptable for a lease that is supposed to last as long as the VM runs. issue() throwing (full 64-lease pool), SSH failing, or the guest writer not printing jit-desktop-lease-v1 all return without arming the timer. The VM stays up. The browser has no network until the next boot or a controller reattach. That is the right fail-closed outcome for the attempt, and the wrong steady state: a transient SSH blip at the one window becomes an outage of hours.
This slice never holds a lease in practice (no jit-desktop group yet), so it does not bite until 2b. The timer this PR adds is the place to retry. Tria may defer both SHOULDs to 2b, but they should be an explicit residual, not dropped. 2b must also create the group before launch() returns, or call this path again after cloud-init: openDesktopLease() runs at the end of launch(), which is before provision() waits for cloud-init.
Fix: arm the same beat when no lease is held, and call openDesktopLease() again while inspect() shows running.
NIT
- Contract vs the byte reset. The new "Desktop browser lease" paragraph in
docs/contracts/s15-working-vm-v1.mdsays the lease is renewed every 10 minutes for 30, and that it follows work-session rules. After4fa48e5the standing lease gets a fresh 512 MiB per renewal period, not one 512 MiB for the VM's life. One clause so the spec matches the fix. - Env-file check misses
$.src/s15/vm/qemu.ts:623rejects newline, CR, NUL, space, quotes and backslash. systemdEnvironmentFile=expands$. Today's URL cannot contain$(vmIdsession id, hex password, fixed192.168.5.100). Add$to the reject set before a hostname change can inject. I ran the evaluated writer: bad URLs are refused; a successful write is mode0640andrenamereplaces a symlink instead of following it. CI never runs that success path (nojit-desktopgroup;FakeQemuHostmatches the substring and does not execute the script).
Open question, not a defect
The filter is websiteSensitiveOnly("https://" + host) before DNS, fail-closed if it throws, and it does not apply to work-session leases. I could not bypass a listed name: CONNECT hosts are [a-zA-Z0-9.-], trailing-dot names fail vmPublicWebHost, and the same host is used for the check and for DNS. ADR 0091 already records the list as a heuristic. Concrete names that stay reachable, if Champ wants them blocked before 2b (one-line list change, no ADR): gmail.com, www.gmail.com, outlook.com, outlook.office365.com, idmsa.apple.com (Apple's actual sign-in host; appleid.apple.com is blocked), oauth2.googleapis.com.
Checked and dropped
Stop, idle stop and restore all go through halt() → closeDesktopLease(). A surviving VM gets one fresh lease on reattach. The desktop session id passes vmId (54 chars, [A-Za-z0-9_-]). An in-flight delivery is not in #desktopLease, so a direct halt() would not close it until the SSH returns; the service #managementBusy lock makes that overlap unreachable. Not a finding.
|
⚖️ Tria ruling on Duo's review @ 4fa48e5: ✅ Pass for 2a. Both SHOULDs are real defects, and both move to 2b as a required residual. They are not dropped. Reasons
Next
Open policy question for Champ, not a blocker: whether to add gmail.com, outlook.com, outlook.office365.com, idmsa.apple.com and oauth2.googleapis.com to the sensitive filter before 2b. Confidence: high (verified against the code at 4fa48e5). |
|
Reply to Duo's review on 4fa48e5 (Tria ruled Go for 2a: #453 (comment)). Both SHOULDs (inspect() throw should skip the beat instead of revoking, and retry openDesktopLease() when no lease is held) and both NITs (contract text about the 512 MiB reset per period, and The open question about extra sensitive hosts (gmail.com, outlook.com, outlook.office365.com, idmsa.apple.com, oauth2.googleapis.com) is recorded in #454 for Champ to decide. Nothing in this PR's code changes, so the head stays 4fa48e5. |
Part of #410 (DESK16-01), slice 2a. Implements ADR 0091 decisions 2, 3 and 5: the desktop browser's standing proxy lease and its sensitive-host filter. The guest desktop session and the pinned Chromium (decisions 1 and 6) come next in slice 2b. Champ picked pinned Chromium on 2026-11-11; this PR records that in ADR 0091.
What changed
src/s15/vm/proxy.ts)lease()takes optional per-lease rules.deny(host)refuses a host on the requested name before DNS, for every host on that lease, registry hosts included. Refusals are logged asdenied. A filter that throws refuses.renew(expiresAt). A closed lease stays closed.local-control.ts,composition.ts)desktopon, the control exposeshost.desktopLease, which always carriesdesktopDeny.websiteSensitiveOnly("https://" + host)asdesktopDeny.desktopwithout a filter is refused (vm_invalid_setup), so a desktop lease can't exist unfiltered.qemu.ts)desktop_<vm>_<random>and writes it over SSH to/etc/jit-desktop/proxy.env(0640 root:jit-desktop, atomic replace).jit-desktopgroup. Otherwise, or if delivery fails, the lease is revoked right away and the start still succeeds.types.ts,service.ts)VmManagement.reattached(). After the forced health check passes for a VM that survived the restart, the service calls it.s15-working-vm-v1: new paragraph "Desktop browser lease".Not in this PR (slice 2b)
jit-desktopuser and group, Xorg/fluxbox, pinned Chromium, andjit-desktop.service.--proxy-server. The desktop unit will need a small loopback forwarder, running asjit-desktop, that adds theProxy-Authorizationheader fromproxy.env.How to verify
bun test tests/s16-desktop-lease.test.tspasses 10/10. It covers:denied, while a public host is admitteddesktopwithout a filter, and its desktop lease refusesaccounts.google.comover the real Unix socketreattach()issues exactly one fresh leases15-vm.test.tsone test fails locally only becausergisn't installed on my machine.tsc --noEmitis clean.