feat(dashboard): accept this machine's own tailnet origin without hand-writing dashboard.url - #1908
Conversation
dd4b0a1 to
4a92d4d
Compare
|
Self-correction before review, pushed as The PR body admitted one unverified claim: this dev desk has no Tailscale, so "the code reads
Replaced the hardcoded suffix with self-consistency against the suffix the daemon reports in the same status output:
This is strictly stronger than the hardcoded version — it checks "is this name on the tailnet we are actually on" instead of "does this name end in a string I guessed" — and it removes the Headscale special case rather than documenting it. The structural half of the validator is unchanged, and it is the part that defends against origin injection: scheme, port, path, userinfo, whitespace, backslash, uppercase, malformed labels and the 253-byte cap are all still rejected. Two other things the upstream read confirmed rather than changed:
Tests grew 47 → 51 and now pin the behaviour that changed: a Still unverified, and unchanged by this: no live tailnet round-trip. That needs a host with root (this one denies |
4a92d4d to
5e6a507
Compare
CodeQL disposition —
|
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Opt-in, validated, fail-silent enrichment at the right layer; the origin stays single-sourced and the change is fully reversible via one config flag. Suggestions
[DESIGN-REVIEWED] 09d312b |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
5e6a507 to
23c7d3d
Compare
Disposition —
|
23c7d3d to
8cdfd0f
Compare
Disposition —
|
…d-writing dashboard.url
8cdfd0f to
09d312b
Compare
Rebase onto
|
| Check | Cause | Fixed by |
|---|---|---|
Frontend Tests |
catalogParity > ja > has no key missing — missing 11 key(s), e.g. components.kiroPrerequisiteGate.copied. This PR changes zero files under website/, so it cannot introduce a locale parity gap; #1905 (docs-only) failed the identical test. |
#1939 64158682 |
Coverage Gate |
Purely derived — the job's own log says Upstream coverage jobs did not succeed (coverage-combine=success, frontend-test=failure) -- failing closed. |
clears with Frontend |
Backend Tests (3.10, 3) / (3.12, 3) |
src/kiro_crew/sandbox.py:2690 ValueError: not enough values to unpack (expected 3, got 2) in the wrap_argv tests — 2-tuple _last_unshare_failure fixtures broken by #1668 |
#1923 e9b8ffd0 |
Dependency Audit |
electron js-yaml override |
#1933 25aa5ab8 |
GPT 5.6 Review |
Not a finding. Per the lane's own guidance I read the job log before calling it infra, and the log ends with > **Incomplete review:** pass(es) 2 did not complete. — pass 1 returned, pass 2 did not, so no [GPT-REVIEWED] marker was emitted and the gate failed closed. No [BLOCK-MERGE] and no finding text anywhere in the 1.4 MB log. |
this rebase re-runs the lane |
Opus 5 Review, Design Review, UX Review and Code Review were all green on
8cdfd0f3, so the round-2 hydration blocker and the round-3 PATH/credential blocker
are both closed.
Verification on 09d312b9
pytest -n0 on test_tailnet_origin.py + test_dashboard_origin.py +
test_config_loader.py + test_config_baseline.py + test_config_schema.py +
test_spawn_audit.py: 439 passed. isort 0, flake8 -j 1 0, mypy 0 (732
files), docs-lint 0, brand gate 0, scrub-lint --no-history 0. One commit, now
based on 64158682.
Phase 2 of the tailnet RFC (
docs/request-for-change/rfc-tailnet-dashboard-access.md§4). Off by default. Does not widen the network bind and does not touch authentication.Problem
tailscale serveis the right way to reach the dashboard from a phone — tailnet-only, TLS, a stable MagicDNS hostname, access governed by tailnet ACLs. Phase 1 (#1761) documented it. But making it work still takes a manual step that fails obscurely when skipped:tailscale serve --bg --https 443 http://127.0.0.1:5476 # ...then open https://desk.tail1a2b3c.ts.net and get a bare 403The
403comes fromcheck_host(), the DNS-rebinding barrier. The origin allowlist is built from loopback plusdashboard.url, so a tailnet name is not in it. The response does not say which allowlist rejected the request or how to extend it — the user has to know to look up their own MagicDNS name and hand-write it intodashboard.url.Why it matters
This is the one remote-access path that is private by construction, and the friction to reach it is "find a hostname you don't know, put it in a config file you haven't opened, and decode a 403 that names nothing". The RFC calls this out as Problem 3.
Fix (symptom → root cause → change)
The symptom is an unexplained
403. The root cause is that the origin set has no way to learn a name the local Tailscale daemon already knows. So the daemon is asked, once, at startup.dashboard/tailnet.py(new) — a read-only interface to the daemon. Two properties carry the whole design:1. Nothing raises. Missing binary, stopped daemon, timeout, non-zero exit, malformed JSON, unexpected schema — every path returns
None. The dashboard must start on a host that has never heard of Tailscale, so this is pure enrichment: it contributes a name or contributes nothing.2. The name is validated before it is returned. It arrives from a subprocess and its destination is the CSRF origin set and the
Hostbarrier, so an unvalidated value is an origin-injection primitive._valid_magicdns_name()is an allowlist, not a denylist — the question is not "does this look dangerous" but "is this provably a bare tailnet hostname":/ : @ ? #, whitespace, backslashevil.example.com,desk.tail.ts.net.evil.com-bad.tail.ts.net,ts.netThe second check is suffix self-consistency, not a hardcoded
.ts.net: the name must sit under the tailnet's ownCurrentTailnet.MagicDNSSuffixas reported by the samestatusoutput. Upstream documents that suffix as tailnet-specific and its own example isuserfoo.tailscale.net, so a hardcoded list would reject legitimate tailnets and would rot as Tailscale adds suffixes. A consequence worth stating plainly: a self-hosted control plane (Headscale) on its own suffix is accepted, with no special case —test_accepts_a_self_hosted_suffixpins that.CurrentTailnetis nil when the node is not connected, which lands here as a missing suffix and is refused: no tailnet means no origin to add.build_allowed_origins()stays a pure function. It gains atailnet_hostparameter, not a lookup — the caller owns the daemon call and the validation. The docstring says so explicitly, so the next reader does not move the subprocess into it and so nothing may pass an unvalidated value in.build_allowed_hosts()is unmodified: theHostallowlist derives from the origin set, so the barrier follows for free. That is the RFC's single-source-of-truth invariant and a test pins it.The daemon call never runs on the event loop.
resolve_tailnet_host()short-circuits before the thread hop when the feature is off, and offloads toasyncio.to_threadwhen it is on.tailscale statusgenuinely blocks while the daemon is coming up, and a multi-second inline stall would freeze every other session and can trip the loop-stall watchdog. A test asserts the probe runs on a non-main thread.Both startup paths wired —
start_dashboardandstart_api_server. Thedashboard_urlre-derivation insidestart_dashboardalso passestailnet_host; without that it would silently drop the tailnet origin for anyone who sets both.What is deliberately NOT here
tailscale serve statusinstead of a config flag) is gated on an open question — whether every widening of the origin allowlist must be explicit config. Only the explicitdashboard.tailscale.enabledflag ships, which the RFC says can land without that answer.is_local_only()still returnsTrue; the gateway still binds loopback.tailscale serveproxies to it.Tests
test/test_tailnet_origin.py, 47 tests, weighted toward the security surface:Noneand none raise — CLI absent,TimeoutExpired,OSErroron exec, non-zero exit, and seven shapes of unusable stdout (empty, garbage, wrong top-level type, noSelf,Selfnot a dict, noDNSName). One test specifically feeds a hostile name through otherwise-valid output: the daemon is trusted to be the daemon, not to be well-behaved.assert_not_called, so the short-circuit can't rot into a wasted thread hop); enabled resolves off the event loop; enabled-but-unresolvable yields"".https://with no port; theHostallowlist follows; coexists withdashboard.url; the loopback floor is untouched. Absence is stated as set arithmetic (contributed - baseline == {…}), which also pins that the opt-in adds exactly one origin and no stray:portvariant.test/test_config_loader.py) —DashboardConfigis built field-by-field, so a nested section nobody wires up is dropped silently: the documentedconfig set dashboard.tailscale.enabled truewould read backFalseand then be rewritten tofalseby the next unrelatedsave(), leaving the feature inert while looking configured. Both halves are pinned, plus malformed-section degradation.Manual verification
N/A on this host, and worth stating plainly: this dev desk has no Tailscale installed, so the happy path is exercised only against a mocked daemon. What that leaves unverified is the real shape of
tailscale status --jsonon a live tailnet — the code readsSelf.DNSName, and if a future CLI moved it the result isNone(feature silently contributes nothing), not a crash. Everything else — the validator, every failure mode, the threading, and the origin/host wiring — is covered above.Gates
pytest436 passed (new file +test_dashboard_origin/test_config_loader/test_config_baseline/test_config_schema/test_spawn_audit),isort,flake8 -j 1,mypy1.14.1 (732 files) — clean.docs-lint,brand-lint,scrub-lintgreen.config-baseline.jsonregenerated; the diff is only the two new entries.test_spawn_audit.pyrequires everysubprocessspawn undersrc/kiro_crewto route throughsandboxed_spawn_argvor be listed inBENIGN_SPAWNSwith a justification.dashboard/tailnet.py::_run_jsonis listed rather than routed, because routing it would make dashboard startup depend on sandbox availability — precisely the failure this module's "nothing raises" property exists to rule out. Being listed is not a free pass, so the spawn is hardened on both axes it could be reached through:/usr/bin,/usr/local/bin,/opt/homebrew/bin, the macOS app bundle, the Windows install dir) and never consultsPATH. APATHlookup made the executable attacker-selectable even though the arguments never were:~/.local/binis both onPATHand agent-writable, so a plantedtailscalewould be executed by the next gateway start, in-process, with the gateway's environment. A non-standard install is therefore not auto-derived and keeps using explicitdashboard.url.env=sandbox.scrub_env()rather than inheritingos.environ— the repo's own scrubber, not a second policy invented here, so it cannot drift and stays cross-OS safe.One fixed argv (
status --json), 3s timeout, no shell, no cwd.TestSpawnHardeningpins all of it, including thatshutil.whichis never called and that the passed env is non-empty (an over-scrub that stripsHOME/SystemRootbreaks the macOS and Windows CLIs).