fix(deploy): make the Linux operator preflight actually detect the role - #472
Conversation
The preflight probed `tailscale serve status` to prove this account may
operate tailscaled. That is a *read*, and tailscaled serves reads to
everyone — only writes are gated on the operator role. So on exactly the
machine whose Serve write is about to be denied, the probe printed
"No serve config", exited 0, and the installer reported
✓ this account may operate tailscaled
before going on to fail at the Serve step — after the build, and after
`current` had already advanced. That late refusal is the thing the check
exists to prevent, so the check could never fire.
Read the role itself instead, from `tailscale debug prefs` (OperatorUser),
compared against the invoking account. `debug` is an unstable CLI surface,
so only a definitive mismatch is fatal: an unreadable or unparseable answer
warns and proceeds, degrading to the old late refusal rather than blocking
an install that would have worked. The `serve status` read still runs
first, since a denial there means something broader is wrong.
Also make the late refusal useful when it is reached: `current` has already
switched and the service is healthy by then, so say the install is complete
but unserved and point at `manage serve`, which adds the HTTPS front door
without a reinstall.
Neither leg is reachable under DORMOUSE_INSTALL_TEST=1, which never
consults Tailscale, so this stays the one preflight rule CI cannot
exercise. Verified by hand against a live tailnet: operator set, operator
belonging to another account, operator unset, `debug prefs` unavailable,
and `debug prefs` returning non-JSON.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J1jDWEh3MTRaFTqmKPoJkK
Deploying mouseterm with
|
| Latest commit: |
f48c07f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5593ff4e.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-linux-installer-operator.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
The mechanism argument holds — serve status is a read, tailscaled serves reads to everyone, so the old probe could never fire. Reading OperatorUser is the right shape, and pointing the late refusal at manage serve matches what cmd_serve actually does (ts serve --bg "$PORT" — the exact write that was denied). Preflight now dies before the build, verified. bash -n, deploy-lint (27 checks), and spec-lint all pass here.
One open question on the unset case, plus three small things. Inline.
The $3 addition isn't reflected in die_needs_operator's own header. Just above the function, the comment reads # One remediation, three call sites. and # $1 = what was refused, $2 = the CLI's own output — there are four call sites now (tailscale status, serve status, the new preflight mismatch, and Serve), and $3 is undocumented. Outside the diff hunk, so I can't suggest it inline; happy to push the comment fix if you want it.
The preflight mismatch reuses $1's "was refused" framing for something that hasn't run yet. That call site renders as `tailscale serve` was refused for this user: tailscaled's operator is unset, not this account (bob). — no Serve command was attempted at preflight. Cosmetic, and it's the one message in this script written to be read under stress, so worth a beat of thought about whether die_needs_operator should take the lead clause rather than assemble it.
Review catch, and it invalidated one row of the previous commit's verification table. `ipn.Prefs.OperatorUser` carries `json:",omitempty"` (confirmed in tailscale v1.102.3), so with no operator set the key is absent from `debug prefs` rather than present and empty. Deciding readability with the same `json_query` call that reads the answer therefore sent the commonest form of this bug — nobody ever ran `tailscale set --operator` — into the lenient warn branch, reopening the exact miss the check exists to close. The earlier "operator unset" test passed only because its stub forced a literal `"OperatorUser": ""` with sed, a shape tailscale never emits. Re-tested against a stub that deletes the key, which is the real shape. Decide readability with `ControlURL`, which has no `omitempty` and is always marshalled; absent-or-empty `OperatorUser` on a blob that parsed is then a definitive unset rather than an unknown. Also from review: - Drop json_query's stderr on this path, and only this path: every other call site reads known-good JSON, whereas here a parse failure is an expected outcome and the runner's stack trace buried the two warnings. - Let `die_needs_operator`'s caller supply the lead clause. One of the four sites has not run anything yet — preflight predicts the refusal — so the hardcoded "was refused" was wrong there; it now reads "will be refused". Header comment corrected to four call sites with `$3` documented. - Fix the spec's stated reason for the CI gap: `DORMOUSE_INSTALL_TEST=1` alone still consults Tailscale; it is the probe's own TEST_MODE gate, plus the injected origin CI pairs with it, that put this out of reach. Re-verified against a live tailnet: operator set, operator belonging to another account, operator key absent, `debug prefs` unavailable, and `debug prefs` returning non-JSON (no stack trace). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1jDWEh3MTRaFTqmKPoJkK
|
Pushed The unset case. You were right to push on it. The stderr and the spec's CI-gap reason applied as suggested; I confirmed Re-verified against a live tailnet, with the unset case now using the real omitted-key shape:
One thing found while tearing the test server down, out of scope here: |
|
Separately, please — and it's bigger than one script: all three installers have it, so it would nearly double this diff in files that Confirmed the mechanism, and it's worse than a backwards order. One detail worth carrying into the issue, because it decides which fix is right. The closing install banner advertises Line anchorsAt
The On the four review points: your replies match what I read in |
|
Filed as #473 — mechanism, the three line anchors, and the reorder-vs-preserve tradeoff, with no fix proposed since it wants an operator-experience call. Added a third option there (advertising Agreed it stays out of this PR: three files that Thanks for the sign-off on the four points — I will leave this here pending the in-flight run on |
dormouse-bot
left a comment
There was a problem hiding this comment.
All four addressed, and the readability/answer split is the right shape — ControlURL decides parseability, absent-or-empty OperatorUser on a parsed blob is a definitive unset. I re-derived the six branches against stub daemons rather than taking the table on trust; all six match what you recorded, including the omitted-key case dying in preflight and the malformed-JSON case warning with no Node stack trace. bash -n, spec-lint (24 specs), deploy-lint (27 checks) and its selftest all pass here.
Also checked the new manage serve remedy is reachable at the point it's printed: BIN_DIR/manage is written at the heredoc well before the Serve step and chmod 0700 before it, and everything after that step is pruning plus the summary banner — so "finish without a reinstall" is accurate, nothing state-bearing is skipped.
On the manage uninstall / manage purge ordering — confirmed, and it's worth its own PR rather than this one. cmd_uninstall prints Use "manage purge" separately to delete those irreversibly. and then rm -rf includes "$ROOT/bin", which is where manage itself lives, so the command the banner just told you to run is gone by the time the banner is on screen. Two details for whoever picks it up: cmd_purge only touches $STATE_DIR and $ROOT/config, both of which uninstall deliberately leaves behind, so the operation is still meaningful after uninstall — it's only the entry point that vanishes. And purge is absent from the installer's closing manage: usage line, so the uninstall banner is currently its only discovery path. Separate concern from the operator preflight, revertible independently, so it belongs in its own PR — happy to open it if you'd like.
|
Thanks — and on the teardown ordering, that is already filed as #473 (posted a couple of minutes before your review landed, so you likely had not seen it). It carries the same two details you named: No fix proposed there, since it is an operator-experience call rather than a mechanical one. If you want to open the PR against that issue, please do. |
|
Opened as #479, against #473. Checks are green on Since you asked for the PR rather than the pick, it makes the pick: preserve The reasoning against the other two is in the PR body, but the short version: reordering to "purge first" strands anyone who already ran Note the triage run's comment on #473 landed a couple of minutes after yours and declined to pick, not having seen the request; #479 supersedes that, and carries its two corrections. Its Windows finding turned out to matter: the delete it suspected of failing against the |
The bug
deploy/local/install-linux.shproved "this account may operate tailscaled" by runningtailscale serve statusand checking its exit status. That is a read, and tailscaled serves reads to everyone — only writes are gated on the operator role.So on exactly the machine whose Serve write is about to be denied, the probe prints
No serve config, exits 0, and the installer reports:…then fails at the Serve step anyway — after the build, and after
currenthas already advanced to the new release. That late refusal is the thing the check exists to prevent, so the check could never fire. Found while followingSELF_HOST.mdon a fresh Linux install, which is also what its troubleshooting section calls the tell:The fix
Read the role itself, from
tailscale debug prefs(OperatorUser), compared against the invoking account.debugis an explicitly unstable CLI surface, so only a definitive mismatch is fatal. An unreadable or unparseable answer warns and proceeds, degrading to the old late refusal rather than blocking an install that would otherwise have worked. Theserve statusread still runs first, since a denial there means something broader than the operator role is wrong.Second, smaller fix: the late refusal is reached with
currentalready switched and the service healthy, so it now reports that the install is complete but unserved and points atmanage serve, which adds the HTTPS front door without a reinstall. (That is the state this bug actually leaves you in.)Verification
No automated coverage is possible:
DORMOUSE_INSTALL_TEST=1never consults Tailscale, so CI cannot reach either leg — which is why this survived. That limitation is now stated in the spec rather than left implicit. Verified by hand against a live tailnet:✓ this account may operate tailscaleddebug prefsunavailabledebug prefsreturns non-JSONspec-lint,deploy-lint+ selftest,loopback-lint,xterm-lintall pass.bash -nclean.Specs
docs/specs/server.mddescribed the broken mechanism verbatim in its Linux trap bullet, so it changed with the code;SELF_HOST.md's troubleshooting entry gained themanage serverecovery path.Possible follow-up (not included)
A
deploy-lintrule asserting the Linux installer readsOperatorUserwould make this regression-proof textually, the way the other 27 rules are. Left out as scope creep — happy to add it.🤖 Generated with Claude Code
https://claude.ai/code/session_01J1jDWEh3MTRaFTqmKPoJkK