Skip to content

feat(dashboard): opt-in phone session that survives a gateway restart - #6369

Closed
bolichen97 wants to merge 1 commit into
mainfrom
feat/qr-session-persist-across-restart
Closed

feat(dashboard): opt-in phone session that survives a gateway restart#6369
bolichen97 wants to merge 1 commit into
mainfrom
feat/qr-session-persist-across-restart

Conversation

@bolichen97

Copy link
Copy Markdown
Collaborator

Problem / Motivation

A phone signed in by scanning the tailnet QR code is signed out by a gateway restart, and has to be re-scanned. On the insider auto-update channel that is frequent enough to be the dominant reason a working phone session ends.

The boot binding that causes this is deliberate (#5763): a session that ends with the process needs no recorded state to be revoked, which is the right default. What is missing is an opt-in for the user who would rather keep the session and accept what that costs.

Why it matters

Someone using the phone as a second screen re-scans several times a day. There is no way to opt out today, so the only workarounds are to stop restarting the gateway or to keep re-scanning.

What changed (motivation → approach → change)

Goal. One scan lasts until the credential's own lifetime lapses, rather than until this process exits.

Approach, and why this one. The QR mint already chooses between two session shapes. A third is added rather than loosening either existing one, because the choice is a genuine trilemma and the existing two must keep behaving exactly as they do:

shape claims bounded by
timed (opt-out) no_refresh a fixed TTL, no renewal
until-restart (default) boot this process's lifetime
persistent (new, opt-in) require_peer the refresh chain's own 30-day MAX_REFRESH_TTL_SECS

The alternative considered and rejected was reusing the timed shape with a long TTL. That produces a credential with no revocation story at all — nothing to rotate, so nothing that can be denied — whereas the refresh chain already carries per-jti reuse detection and a revocation generation.

Why the identity gate is load-bearing, not bookkeeping. Removing the boot bound removes the only thing that was bounding this session, so something must replace it. Behind tailscale serve every request reaches the gateway from 127.0.0.1, so an address pin would produce ip:127.0.0.1 — a pin every peer on the tailnet satisfies, which is worse than no pin because it reads as one in the audit trail. The replacement is therefore a daemon-verified peer identity, and the setting is hard-gated on dashboard.tailscale.trust_identity plus a non-empty allowed_logins.

What was built.

  • dashboard.qr_session_persist_across_restart, default False, in config/loader.py.
  • The third shape in handlers/tailnet_mobile.py: no boot, no no_refresh, plus a new require_peer claim.
  • Two refusal paths — missing identity trust, and the sibling qr_session_until_restart being off — each logging a WARNING that names the missing prerequisite, and each falling back to the boot-bound shape.
  • The require_peer claim carried through the link→session exchange (token_auth.py) and onto both halves of every rotated pair (handlers/auth_refresh.py); refresh_tokens.py gains the parameter and a refresh_token_requires_peer() accessor whose failure direction is deliberately the opposite of refresh_token_boot's (an undecodable payload answers True, because answering False would let a signed-but-unreadable token skip the check its chain was minted to require).
  • api_auth_refresh refuses to use such a chain while no peer resolves, checked ahead of all three exits that hand back a credential — grace replay, reuse detection, and the mint. Refusing before reuse detection is deliberate: reuse detection revokes the chain, so the other order would let an unverified caller sign a legitimate session out by replaying one consumed token. The refusal does not revoke, because identity resolution fails transiently.
  • The three config reads are independent getattr calls with per-field conservative defaults; coupling them in one try meant a config object missing any one attribute silently discarded the other two, which would remove the existing opt-out.

#6033's caller-bounds cap still wins over the configured shape and caps this one too, which is correct: a credential must not outlive the session that authorized it.

Tests

test/test_tailnet_mobile.py — the persistent shape drops boot and carries require_peer; it is refused without identity trust; it is refused when the timed shape is in force.

test/test_auth_refresh_handlers_cov80.py — an identity-bound chain is refused (and not revoked) when no peer resolves; the same holds on the grace-replay path, with no Set-Cookie on the response; the claim survives rotation and is absent by default; an undecodable payload fails closed.

Four of these are mutation-verified — with the pre-mint check removed the grace-replay test fails assert 200 == 401, i.e. it demonstrably exercises the path that serves a cached credential.

Manual verification

Still required, and it cannot be done in unit coverage: the identity path needs a real tailscaled and a second device. On a tailnet with trust_identity on and allowed_logins set — scan, restart the gateway, confirm the phone stays signed in; then confirm a peer NOT in allowed_logins is refused. Both are unverified on real hardware so far.

Screenshots / video

N/A — no UI change beyond the settings entry's own label and help text, which is config-driven.

Related Issues

Split out of #6194 (no linked issue: reported directly in chat as a phone session going blank after 30–40 minutes; that PR carries the two fixes for the reported symptom).

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated — not done: the remote-and-mobile guide needs a line for the new key and its prerequisites. Deliberately left for the round that settles the peer-binding design below, so the doc is not written twice.
  • No secrets, credentials, or internal references in the diff

Open review finding — why this is a DRAFT

GPT 5.6 raised a BLOCKING finding against this code on #6194 that is not fixed here, and it is legitimate:

Refresh accepts a different allowlisted peer than the session's original peer

_verified_peer() checks that some allowlisted peer resolves, not that it is the same peer the session was established for. With more than one entry in allowed_logins — or, under the default pin_scope: node, a second device belonging to the same login — a stolen refresh cookie is usable by a peer that was never the one that scanned. That is weaker than the codebase's own standard: bind_token_peer pins ordinary sessions to a specific peer_pin_key.

The fix is to carry the pin key rather than a boolean flag and compare it for full-string equality, establishing it where the phone first presents itself — the link→session exchange, not the QR mint, since the QR is minted from the desktop and would bind the wrong peer. That is a third plumbing site for this claim and deserves its own review round rather than a fourth consecutive patch, which is why this PR opens as a draft.

Adds `dashboard.qr_session_persist_across_restart` (default OFF): a scanned
phone session bounded by the refresh chain's own 30-day lifetime instead of by
this process's, so one QR scan is not invalidated by a gateway restart.

Hard-gated on `dashboard.tailscale.trust_identity` plus a non-empty
`allowed_logins`, and on the sibling `qr_session_until_restart` being ON (that
setting is what issues the renewable credential this one removes the restart
bound from). Both refusals log a WARNING naming the missing prerequisite and
fall back to the boot-bound shape.

The gate is not bookkeeping: behind `tailscale serve` every request reaches the
gateway from 127.0.0.1, so without a daemon-verified peer identity the session
is a bearer credential any tailnet peer could replay -- and outliving the
process is exactly what makes that matter. A new carried `require_peer` claim
marks such a chain, and `api_auth_refresh` refuses to USE it (ahead of the
grace-replay, reuse and mint exits alike) while no peer resolves.

Split out of #6194 so the black-screen fix there is not held up by this
feature's remaining design work.
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🔴 BLOCK (blocking)

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

Design-Verdict: BLOCK

The identity bound that justifies removing the boot bound is a boolean, not a pin — any allowlisted peer can renew a stolen phone cookie for 30 days.

Blockers

  • require_peer verifies an identity, not the identity. The persistent shape's entire security argument is "bounded by identity rather than by uptime," but _verified_peer() only checks that some allowlisted peer resolves ("Refresh accepts a different allowlisted peer than the session's original peer" — the author's own open finding). Mechanism: with a second allowed_logins entry, or a second device under the default pin_scope: node, a stolen refresh cookie rotates for 30 days from a peer that never scanned — weaker than the codebase's own bind_token_peer standard, while the audit trail reads as identity-bound. Consequence of shipping first and pinning later: chains minted with the boolean claim live up to 30 days, so the follow-up must either honor the weak shape for a month or invalidate them (re-scan) — the exact harm this PR removes. Fix: carry the peer_pin_key in the claim (established at the link→session exchange) and compare full-string equality in this PR, rather than landing the boolean claim schema and migrating it.

Watch

  • Two booleans where "Survives A Gateway Restart" requires "Lasts Until Restart" to be ON needs a paragraph of help text to deny the contradiction — a sign the config shape (three exclusive session shapes as two interacting flags) will keep confusing operators; acceptable given the existing key, but worth a deliberate decision before the surface ossifies.

[DESIGN-REVIEWED] 172e389

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 172e38986b1a597ce6023899a7ff798985f6ac9f — 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.

All evidence is gathered — contract, intent, patch, and repository verification of the pin-key mechanism and claim-plumbing sites. Final review follows.

First-Principles-Verdict: CONCERNS

The identity bound ships as an any-allowlisted-peer boolean while the repo's own pin-key mechanism is the stronger form — and this diff already touches every site that fix needs.

What this change ships

Intent: keep a scanned phone signed in across gateway restarts, opt-in. ADDITION.

  1. New setting "Phone Sign-In Survives A Gateway Restart", off by default — justified (re-scans several times a day, reported).
  2. Third QR shape: session bounded by the 30-day chain, not the process — justified.
  3. A "peer required" mark carried through the link exchange and every rotation — justified, but weaker than the house standard (see Watch).
  4. Refresh answers 401 (never revokes) when no verified peer resolves, ahead of grace replay and reuse detection — justified.
  5. Flag on without prerequisites falls back loudly to today's behavior — justified.
  6. One unreadable config field no longer discards the other two reads — justified, declared.
  7. refresh_token_requires_peer() accessor failing closed — justified.
  8. Help text explaining why two flags must both be on — declared; cost of item 1's encoding.

Watch

  • The boolean claim vs the pin key. Grepped peer_pin_key|bind_token_peer: 11 hits — ordinary sessions pin to a specific key (token_auth.py:2568), rotations re-pin (auth_refresh.py:615), yet this chain records only require_peer: "1", satisfied by any allowlisted peer. The description defers ("deserves its own review round"), but the diff already opens all three plumbing sites the pin-key form needs (token_auth.py:2524,2718, auth_refresh.py:524, refresh_tokens.py:452), and 30-day chains minted with the boolean will outlive the redesign, forcing grandfathering or migration.
  • Two booleans encode three shapes, leaving one contradictory combination that needs a runtime WARNING plus a help paragraph titled "NOT a contradiction" — permanent config-surface cost for one invalid state.

Subtractions

  • Replace the require_peer: "1" boolean with the establishing peer's peer_pin_key at the same three sites, compared full-string at refresh — same plumbing, deletes the second spelling and the future claim migration.

[FIRST-PRINCIPLES-REVIEWED] 172e389

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 172e38986b1a597ce6023899a7ff798985f6ac9f — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 172e389

Verdict parsed from the review's SHA-scoped output markers for commit 172e38986b1a597ce6023899a7ff798985f6ac9f.

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

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 172e38986b1a597ce6023899a7ff798985f6ac9f.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/auth_refresh.py:444 -- Refresh accepts a different allowlisted peer
if refresh_token_requires_peer(refresh_token) and not await _verified_peer(request):
Stolen refresh cookie -> another allowlisted peer passes _verified_peer -> session rotates and binds to the attacker.
Anchor: backend-security-controls
Fix: Carry the original peer pin and require exact equality.

BLOCKING -- src/kiro_crew/dashboard/handlers/tailnet_mobile.py:714 -- Persistence trusts inactive identity configuration
getattr(_ts, "trust_identity", False) and getattr(_ts, "allowed_logins", None)
Config edited without restart -> stale middleware has identity trust disabled -> persistent session is issued with only the shared loopback pin.
Anchor: backend-security-controls
Fix: Gate persistence on the effective request.app["tailnet_trust"].

[BLOCK-MERGE] 172e389
[GPT-REVIEWED] 172e389
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 172e38986b1a597ce6023899a7ff798985f6ac9f: <one-sentence reason>

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Superseded: the persistent session shape is back in #6194 at head 5dc31cdc841351577ed61f27acd610f1ad5222e8, on the author's decision to ship it in one PR rather than split.

The two BLOCKING findings this PR carried are not fixed — they are overridden on #6194 with that stated plainly in the disposition comment there, so whoever approves is deciding on the accepted risk rather than on a clean bill. The redesign this PR's description sketched (carry the peer pin key on the chain and compare it for equality, establish it at the link→session exchange, and make the gate require a live identity resolution rather than a configured value) is still the correct fix and is now follow-up work rather than a blocker.

Closing to avoid two PRs carrying the same change. The branch feat/qr-session-persist-across-restart is left in place if the redesign wants to start from it.

@bolichen97 bolichen97 closed this Aug 27, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 27, 2026
@bolichen97
bolichen97 deleted the feat/qr-session-persist-across-restart branch September 6, 2026 03:55
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.

1 participant