Skip to content

fix: keep tailnet phone sessions across updates - #6660

Merged
bolichen97 merged 1 commit into
mainfrom
codex/tailnet-persist-after-update
Aug 29, 2026
Merged

fix: keep tailnet phone sessions across updates#6660
bolichen97 merged 1 commit into
mainfrom
codex/tailnet-persist-after-update

Conversation

@bolichen97

@bolichen97 bolichen97 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation Phone access configured through Tailscale could stop working after a Kiro Crew update and ask the user to run kirocrew token again. The earlier identity-pinning work in #6485 made restart-persistent sessions possible, but the guided setup did not durably enable all prerequisites, Windows skipped the daemon whois path, and update/restart handling still left boot-bound or unverified sessions behind. ## Why it matters An ordinary user should configure phone access once and keep using the same authenticated session across Nightly updates and gateway restarts. Requiring a fresh terminal token after every update breaks the one-click mobile setup. Persistence also has to remain identity-bound: a Tailscale daemon outage or a second allowlisted node must never be able to claim a surviving cookie. ## What changed (motivation → approach → change) - The owner-only Phone access setup now performs one locked, atomic config mutation that enables Tailscale, enrolls the daemon-reported local login without replacing existing allowlist entries, enables identity trust, and selects restart-persistent QR sessions. - The handler reloads the merged effective config after writing. A config.local.json override that prevents safe persistence now returns 409 config_overlay_conflict with the conflicting fields instead of reporting a false success. A retry also compares the running gateway trust snapshot so a previously written base config still triggers the required restart. - Update/restart completion is fenced by a changed gateway boot ID before the UI publishes or mints a QR. The card exposes setup/restart progress instead of hiding a restart behind a read-only-looking action. - Windows now participates in forwarded-peer tailscale whois resolution. - A persistent QR initially acts as a five-minute enrollment bearer. After the first verified redemption, both access and refresh cookies carry the original peer key inside the existing HMAC-signed payload. Every request, refresh, grace replay, and child credential mint preserves and verifies that exact key and its issuance scope, so a restart can only rehydrate the in-memory pin for the original device—not whichever allowed node arrives first. - require_peer=1 fails closed when identity is unavailable or the signed device key mismatches. Unsafe legacy claimless persistent cookies/refresh chains require one fresh QR scan rather than silently taking ownership after restart. - Documentation and focused backend/frontend regression coverage were updated. ## Tests - py -3.12 -m pytest -q -n 4 test/test_tailnet_mobile.py test/test_token_auth.py test/test_tailnet_peer.py test/test_auth_refresh_handlers_cov80.py — 489 passed, 1 skipped. - Expanded run including refresh-token and delegated-mobile-link suites — 566 passed, 3 skipped. - npx vitest run src/components/TailnetMobileCard.test.tsx — 44 passed. - npx tsc -b and npm run build — production build completed (6,788 modules). - CI-equivalent Black ratchet, isort, scoped flake8, and focused mypy checks — passed. - py -3.12 scripts/docs_lint.py — passed (242 Markdown files). - A broader local backend run completed 67,709 tests successfully with 3,000 skipped and 6 xfailed; its 21 failures were host/environment cases outside this diff (primarily Windows symlink privilege, plus existing broker/PPTX environment cases). CI remains the authoritative full-suite run. ## Manual verification Validated on the affected Windows Nightly host: - Tailscale status exposed both the local login and node identity. - Effective config had identity trust enabled, one allowed login, restart persistence enabled, and the normal until-restart mode enabled. - Restarted the gateway and confirmed the phone reconnected without running kirocrew token again. ## Screenshots / video Why no screenshot: there is no layout or styling change. The existing Phone access card only changes its ready-action copy to “Set up & show QR” and reuses its current inline status treatment while setup/restart is pending; the substantive change is persistence and authentication behavior. ## Related Issues Follow-up to #6485, which introduced the earlier Tailscale identity-pinning fix. No linked issue. ## Checklist - [x] At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...) - [x] Existing tests pass and new tests added for new functionality - [x] Self-review completed; code follows project style guidelines - [x] Documentation updated (if applicable) - [x] No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team August 28, 2026 22:59
@bolichen97
bolichen97 requested a review from a team as a code owner August 28, 2026 22:59
@bolichen97
bolichen97 requested a review from smeyffret August 28, 2026 22:59
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 3423ee43fb1590360839788b2d6e40ba111e919c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound identity-binding design, but QR minting is now fused to the identity-trust mutation with no degraded path when its prerequisites can't be met.

Watch

  • The ready-path "Show QR" (previously a pure mint) now unconditionally runs api.tailnetMobileConfigure() first, which hard-requires probe.login, trust_identity, and an allowlisted login — "daemon_not_ready" / "config_overlay_conflict" return 409 and the mint never runs. Cause → mechanism → consequence: a daemon whose status exposes no readable Self login (e.g. a tag-owned node), or an operator who deliberately pins trust_identity: false in config.local.json, hits the 409 on every click and loses UI phone access entirely, where the old boot-bound token+IP QR worked fine. The only preserved opt-out (qr_session_until_restart=false) still requires identity trust in _effective_mobile_setup. Relatedly, a hand-set trust_identity: false in base config.json is silently flipped back on by every mint, despite the field's own help text ("explicit opt-in, never inferred").

Suggestions

  • On daemon_not_ready / overlay conflict, fall back to the pre-existing boot-bound QR mint with an inline "won't survive restarts" notice instead of hard-failing the whole flow — the server endpoint for it still exists; only the frontend path was removed.

[DESIGN-REVIEWED] 3423ee4

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3423ee43fb1590360839788b2d6e40ba111e919c and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/TailnetMobileCard.tsx:325 -- after resolving an overlay conflict with timed sessions enabled, "restart_required": false leaves stale trust_off state and aborts setup -> Fix: refetch and accept mobile status after every successful configure.

[GPT-REVIEWED] 3423ee4

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3423ee43fb1590360839788b2d6e40ba111e919c: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 3423ee43fb1590360839788b2d6e40ba111e919c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The i18n keys resolve (pre-existing in en.manual.json and all locale catalogs), the 409 error copy is actionable and carries a code field, boot-id fencing fixes a real false-success failure mode, and the pending-state feedback exists. Two genuine UX risks remain.

UX-Verdict: CONCERNS

During the automated restart the card shows the imperative "Restart Kiro Crew to finish" — an instruction to do what the system is already doing.

Watch

  • TailnetMobileCard.tsx:497 reuses restart_gateway_title ("Restart Kiro Crew to finish") as the setupRestarting progress line → a first-time user reads a command, not a status, and may manually restart or assume the click failed while the automated restart is mid-flight. Hits every restart-requiring setup for up to 60s. Fix: a dedicated progressive key, e.g. "Restarting Kiro Crew…".
  • On ready, the button that was pure display ("Show QR code") now runs the full setup and can restart the gateway (configured.restart_requiredapi.restartGateway()), interrupting anything the agent is doing, with no pre-click signal beyond "Set up". Hits each upgraded user exactly once, mid-click, with no exit. Fix: when the click will restart, surface it before restartGateway() fires — even the (corrected) status line appearing before the call, or a restart mention in ready_body.

Suggestions

  • setupErrorMessage appends raw config paths to an already-long backend sentence; putting the field list on its own line (or in <code>) would keep the two-sentence remedy scannable at card width.

[UX-REVIEWED] 3423ee4

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 3423ee43fb1590360839788b2d6e40ba111e919c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: PASS

Every item traces to the reported defect — phone sessions dying at restart — and the fix lands at cause level: the device binding moves from restart-mortal memory into the signed credential.

What this change ships

Intent: FIX — configure phone access once and keep the same identity-bound session across updates, without letting another tailnet node claim the surviving cookie.

  1. Setup click performs one locked, atomic config write (Tailscale on, daemon login enrolled, trust on, persistence on) — justified
  2. QR sessions carry an HMAC-signed original-device key, checked on request, refresh, grace replay, and child mints — justified, cause-level
  3. Pre-fix claimless persistent cookies fail closed; one re-scan required — justified migration
  4. Windows now resolves forwarded peers via tailscale whois like POSIX — justified; RFC OQ4 resolved in-commit
  5. Status exposes boot_id; the UI waits for a changed boot before minting a QR — justified (exiting process answers during its flush window)
  6. Ready-step button relabeled "Show QR code" → "Set up & show QR", running setup first — declared; relabel derives from changed behavior, and the orphaned show_qr key is deleted from all 12 locales (0 leftovers)
  7. 409 config_overlay_conflict names the config.local.json fields blocking persistence — justified (a raw-write success would be false)
  8. Inline restart-progress text during setup — declared
  9. Spec/RFC/guide sync, correcting the stale "per-process HMAC secret" line (persistent token_secret.py predates this PR) — mandated doc sync

Duplication was the real question and it fails to materialize: the only existing mechanism, patchConfig (one dotted path per PATCH, website/src/api/client.ts:2651), cannot do this job — the allowlist value is the daemon's local login, deliberately kept server-side (DaemonProbe.login is never sent to the renderer), and the write must be multi-field atomic so trust can never be enabled with an empty allowlist. Sibling sweep: 14 generate_token call sites; every require_peer-minting path (link exchange, refresh, QR mint, mobile link) threads peer_key, and the remaining sites mint plain link tokens. New helpers have real consumers (peer_pin_key_for_claim 2, required_peer_key_unverified 3, refresh_token_peer_key 1) with none generalized beyond use.

[FIRST-PRINCIPLES-REVIEWED] 3423ee4

@bolichen97
bolichen97 force-pushed the codex/tailnet-persist-after-update branch from 7e7e23c to cba4161 Compare August 28, 2026 23:16
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3423ee43fb1590360839788b2d6e40ba111e919c — this comment is updated in place on each push.

Review details

Based on my independent falsification of the discovery pass's conclusion and my own review of the changed lines, I could not ground any defect meeting the (a)/(b)/(c) bar at 80+.

Key paths I re-derived rather than inherited:

  • generate_token/generate_refresh_token now raise on a peer_key/require_peer mismatch; every changed call site (_caller_bounds, auth_mobile, the middleware exchange and refresh mint, auth_refresh) sets _session_peer_key/carried_peer_key non-empty exactly when require_peer is present, so no reachable call trips the guard.
  • The refresh handler rejects legacy claimless chains (not carried_peer_key) and node-scope mismatches before consuming the jti; _check_pin fails closed (peer is None and requires_peer) so a require_peer cookie/link can never fall back to ip:127.0.0.1.
  • allow_unbound_require_peer_link=not from_cookie only lets the main query-link exchange enroll a first binding against a verified peer; surviving cookies and mixed-internal paths fail closed.
  • _effective_mobile_setup returns success only when the merged (base+overlay) config actually satisfies the identity/persistence fields; the base write is intentionally not rolled back on overlay conflict.

The en setup_action/setting_up keys and any missing-translation concern fall to the i18n gate this pipeline owns deterministically, so they are out of scope here.

No findings.

[OPUS-REVIEWED] 3423ee4

Verdict parsed from the review's SHA-scoped output markers for commit 3423ee43fb1590360839788b2d6e40ba111e919c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3423ee43fb1590360839788b2d6e40ba111e919c: <one-sentence reason>

@bolichen97
bolichen97 force-pushed the codex/tailnet-persist-after-update branch 3 times, most recently from 98340b0 to a54364f Compare August 28, 2026 23:58
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Review follow-up for a54364f3019cd5a29258d291d5db142ca754bb83:

  • Design/UX: the ready action is now explicitly Set up & show QR, shows a disabled Setting up phone access… state, and displays restart feedback while the gateway cycles. A changed boot_id fences continuation before publish/QR.
  • UX/First Principles: config_overlay_conflict.fields is now rendered in the visible error so the operator knows which config.local.json keys to remove. Unused changed/persistent response fields were removed; successful configure returns only restart_required.
  • Timed opt-out: effective qr_session_until_restart=false remains a valid non-persistent success and does not require identity trust.
  • Security: persistent access and refresh credentials now carry the original HMAC-signed peer key. Restart rehydration, refresh/grace replay, rotation, and child mints verify/preserve that exact key and issuance scope; unsafe legacy claimless persistent sessions fail closed and request one re-scan.

Local evidence: focused backend 489 passed / 1 skipped; expanded auth suite 566 passed / 3 skipped; Tailnet card 44 passed; TypeScript, scoped ESLint/flake8, Black/isort, mypy, and docs lint passed. Two independent final-diff security reviews returned PASS.

Configure guided phone access with the daemon-reported Tailscale
identity so scanned sessions can survive gateway restarts safely.

Verify forwarded peers through the Windows Tailscale CLI and wait for
the replacement gateway before minting the QR session.
@bolichen97
bolichen97 force-pushed the codex/tailnet-persist-after-update branch from a54364f to 3423ee4 Compare August 29, 2026 00:26
@bolichen97

Copy link
Copy Markdown
Collaborator Author

CI follow-up on \3423ee43fb1590360839788b2d6e40ba111e919c: removed the obsolete \components.tailnetMobile.show_qr\ entry from the English, pseudolocale, and all shipped locale catalogs after the renamed setup action made it dead. Local verification now passes: \deadKeys.test.ts\ 3/3, \TailnetMobileCard.test.tsx\ 44/44, and all 19 i18n checks with \I18N_BASE_REF=origin/main. The PR remains one commit.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Final CI status for \3423ee43fb1590360839788b2d6e40ba111e919c: all 66 check runs completed with 62 successes, 4 expected skips, 0 pending, and 0 failures. The transient \setup-uv\ timeout was rerun successfully; its backend shard and the dependent coverage combine/gate both passed. Automated GPT 5.6 and Opus 4.8 reviews report no blocking findings. The remaining branch-protection requirement is one human approval.

@bolichen97
bolichen97 enabled auto-merge (squash) August 29, 2026 05:20
@bolichen97
bolichen97 merged commit 11607e0 into main Aug 29, 2026
104 of 106 checks passed
@bolichen97
bolichen97 deleted the codex/tailnet-persist-after-update branch August 29, 2026 06:09
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 29, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #6652 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #6652: CONTINUE_DEVELOPMENT. The motivating pain in Issue #6132 is now largely handled on main, which shrinks the benefit side of the PR's security trade-off. Worth settling on the issue before more code. Files: src/kiro_crew/dashboard/handlers/tailnet_mobile.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants