Fix silent account-machine connect: zombie relay detection + sealed adoption fallback#884
Merged
Merged
Conversation
…o-end probes JSON control keepalive (DO-answered, unlike edge-terminated native pings), relay self-probe of our own /connect/<machineKey> after control open and every bridge validation, honest relay endpoint publication (drop stale retention on verified failure), truthful sync status + doctor verdicts, and zombie telemetry logs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…routes AccountMachineRow now renders adoption errors (previously computed and silently dropped), a guided "Pair nearby instead" jump into the existing PIN flow, live stage text over the new account:pairMachineProgress IPC channel, and a transient connected-via route/latency line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Machines publish an ed25519 identity key in their directory row and answer a pre-auth account_challenge by signing the client nonce over an ephemeral X25519 exchange. Adoption hellos and hello_ok replies are then sealed with the derived session key, so account adoption can safely run over any route and clients verify the host is genuine before releasing credentials. Plaintext account hellos remain relay-only for legacy compatibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adoption tries the directory-verified relay first and, for hosts that publish an identity key, falls back to sealed adoption over tailnet and LAN routes. Host-identity verification failures abort immediately without dialing further routes; route-by-route stage progress feeds the desktop Connections panel and failures aggregate every attempted route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…PIN fallback iOS mirrors ade-adopt-v1 (X25519+HKDF+ChaChaPoly with Ed25519 host identity verified against the directory pubkey, fail-closed on malformed keys), tries relay → Tailscale → LAN for adoption, and gains stage text, a guided pair-with-PIN fallback, connected-route toast, and accessibility support. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… relay publication Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… downgrade edges - Adopt-challenge issuance: only malformed/anomalous challenges feed the cross-tenant global rate limiter; well-formed signing requests count per-IP only, so a few cheap frames (or one failed multi-route adoption) can no longer lock out account adoption host-wide. - Relay self-probe: a CLOSE_TOO_MANY (at tunnel cap) rejection is proof the control socket is alive and serving clients — treat it as liveness, not a zombie verdict that would terminate control and drop the relay endpoint. - Directory pubkey: preserve a present-but-blank key so it fails closed at verification like iOS, instead of silently downgrading to plaintext relay. - Host challenge decline (cooldown) now surfaces the host's real reason and fails over to the next route instead of a misleading identity-abort. - Publisher uses the shared signing-key filename constant; drop dead keepalive sleep/wake branch; iOS connectedLabel reuses attemptLabel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… re-review Re-review showed the per-IP challenge limiter is degenerate over relay (every relay adopter reaches the host from 127.0.0.1 via the loopback bridge), so a few well-formed relay adoptions could still trip a shared cooldown. Well-formed challenges now feed no limiter at all — signing load is bounded by the per-connection single-active guard and the relay's connection cap — and only malformed/anomalous challenges (genuine abuse) count. Also: a self-probe CLOSE_TOO_MANY (4503) is inconclusive, not proof of health (an attacker could fill all 16 slots to mask a stale control), so it now renders no verdict — the JSON control keepalive independently owns zombie detection regardless of tunnel capacity, and prior publication state is left untouched. Tests updated to assert well-formed challenges are never throttled while malformed floods still are, and the 4503 atCapacity classification. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Document the ade-adopt-v1 sealed handshake and envelope types, the JSON control keepalive + end-to-end self-probe with honest relay publication (pubkey + relayEndToEnd gating, 4503 at-capacity liveness), and the ade doctor relay self-probe verdict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesAccount identity and adoption
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rogress to webclient account surface - .gitleaksignore: the ade-adopt-v1 HKDF session-key vector (derived from public RFC 7748 keys, asserted in code) trips generic-api-key in a doc comment. - webclient adapter completeness test: include the onPairMachineProgress method added to the browser account namespace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review — pushed two CI fixes: gitleaks allowlist for the ade-adopt-v1 RFC-7748-derived test vector, and the webclient account-surface completeness list. No product-code changes since your last pass. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 4826-4832: Update the timestamp validation preceding
Int64(timestampValue) to use a strictly safe upper bound that guarantees
conversion cannot exceed Int64.max, while preserving the existing finite,
integral, and nonnegative checks. Keep the machineName error path unchanged and
ensure malformed out-of-range values throw
AccountAdoptionIdentityVerificationError instead of trapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0312fdda-0aa3-48ff-b606-f237deffd1e0
⛔ Files ignored due to path filters (4)
CHANGELOG.mdis excluded by!*.mddocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**
📒 Files selected for processing (53)
apps/account-directory/README.mdapps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/sync/machineIdentitySigningStore.test.tsapps/ade-cli/src/services/sync/machineIdentitySigningStore.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncLoopbackCollision.test.tsapps/ade-cli/src/services/sync/syncRelaySelfProbe.test.tsapps/ade-cli/src/services/sync/syncRelaySelfProbe.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/services/sync/syncTunnelClientService.test.tsapps/ade-cli/src/services/sync/syncTunnelClientService.tsapps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.test.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/remoteTargets/AccountMachineRow.tsxapps/desktop/src/renderer/components/remoteTargets/PairMachineForm.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/remoteTargets/SavedMachineRow.tsxapps/desktop/src/renderer/components/remoteTargets/remoteTargetListStyles.tsapps/desktop/src/renderer/webclient/adapter/account.tsapps/desktop/src/shared/accountDirectory.test.tsapps/desktop/src/shared/accountDirectory.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/sync/adoptChannelCrypto.test.tsapps/desktop/src/shared/sync/adoptChannelCrypto.tsapps/desktop/src/shared/types/account.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/Services/AccountDirectory.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Account/MobileAccessGateView.swiftapps/ios/ADE/Views/Hub/HubQuickConnect.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swiftapps/ios/ADETests/PairingAndDpopTests.swift
…mestamp Double(Int64.max) rounds up to 2^63, so a `<=` bound admitted a ts that traps in Int64(...), crashing before skew validation on a malformed challenge. Use strict `<`. (CodeRabbit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Applied the strict-bound fix for the Int64(Double) timestamp trap (thanks @coderabbitai). @codex review — only change since last pass is that one-line iOS guard. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- changed Swift hunks ---'
git diff --unified=20 HEAD^ HEAD -- 'apps/ios/**/*.swift'
echo '--- candidate timestamp conversion locations ---'
rg -n -C 8 'Int64\s*\(|Double\s*\(|timestamp|clock.?skew|challenge' apps/ios/ADE/Services/SyncService.swift
echo '--- source outline ---'
ast-grep outline apps/ios/ADE/Services/SyncService.swift --items all --type function --match 'Adopt|adopt|Account|account|Hello|hello'Length of output: 30079 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- adoption challenge test references ---'
rg -n -C 10 'performAccountAdoptionChallenge|account_challenge|timestampMilliseconds|maximumClockSkew|Int64\.max|9223372036854775808' apps/ios/ADETests apps/ios/ADE
echo '--- test declarations near sealed-adoption coverage ---'
ast-grep outline apps/ios/ADETests/PairingAndDpopTests.swift --items all --type function --match 'test|Adopt|adopt|Sealed|sealed'Length of output: 21777
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Account-machine Connect (the PIN-less "adopt a machine from your account" flow) fails silently across the fleet. Diagnosed live against a real MacBook Pro → Mac Studio pair: the row spins "Connecting" for ~1s, returns to "Connect", no error message, ever — while pairing by six-digit PIN works every time. Reverse direction worked, which is what made it look flaky.
Root causes, confirmed against the relay Worker's own Cloudflare logs and the host brain state:
/connect/<machineKey>was rejectedhost_offline. The Studio sat in this state through ~30 silent user retries.AccountMachineRowcomputed the failure and dropped it on the floor (onlySavedMachineRowrendered errors).What
{t:"ping"}→{t:"pong"}keepalive on the control socket (the DO must answer, unlike edge-terminated native pings). Miss → terminate + reconnect within minutes./connect/<machineKey>like a real client after every control open and bridge validation; the relay endpoint is published to the account directory only after that round-trips.ade sync status/ade doctornow report the honest verdict. A4503at-capacity close is treated as liveness, not a false zombie.ade-adopt-v1sealed adoption — machines publish an ed25519 identity key in their directory row; the host signs the adopting client's challenge over an ephemeral X25519 exchange, and the account bearer / DPoP / minted secret travel ChaCha20-Poly1305-sealed. Clients verify host identity before releasing any credential and fail closed (no downgrade to plaintext-on-LAN). This lets adoption fall back relay → Tailscale → LAN on desktop,ade code remote, and iOS. Cross-platform crypto is locked by a golden test vector (TS ↔ Swift, independently reverified).Security
Three independent reviewers (Fable, Opus,
codex) audited the protocol and converged: sound on replay, downgrade/MITM, nonce uniqueness, key parsing, and the transport-gate change (every account-auth check preserved). Two rounds of review-driven hardening are included: the adopt-challenge issuance limiter no longer lets a few cheap frames lock out adoption host-wide (well-formed challenges feed no cross-tenant limiter; over relay every adopter shares the loopback origin), and the self-probe no longer over-claims health at the tunnel cap.One item deliberately deferred to a follow-up (not this PR): pubkey continuity pinning (TOFU) — a directory that drops a previously-published key silently reverts clients to legacy plaintext-over-relay. Present-but-malformed keys already fail closed; only absence downgrades, over the already-trusted relay. Pinning interacts with the machine-trust-reset migration and key rotation, so it wants its own review.
Validation
apps/ade-cli,apps/desktop,apps/account-directory— clean.PairingAndDpopTests19 (full build+test passed), tuiClient 929 — all green.4503at-capacity classification, DoS limiter policy (well-formed never throttled, malformed floods still are), present-but-blank pubkey fail-closed, direct-route impostor abort.ade doctorrelay check), TUI (inherits fallback) — all reconciled.Rollout note
apps/account-directory(thepubkeycolumn is already in its schema) and the tunnel relay need no new deploy for this to function; the directory pubkey publish + sealed adoption activate as machines update. Ship alongside the next desktop release + TestFlight build.🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
The PR hardens account-machine connection and adoption flows.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
What T-Rex did
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client as Desktop / CLI / iOS participant Directory as Account Directory participant Relay as ADE Relay participant Host as Machine Brain Host->>Relay: Open control connection Host->>Relay: Self-dial machine route Relay->>Host: End-to-end probe Host->>Directory: Publish verified routes and Ed25519 public key Client->>Directory: List account machines Directory-->>Client: Routes and host public key loop Relay, Tailscale, then LAN Client->>Host: Account adoption challenge Host-->>Client: Signed ephemeral key exchange Client->>Client: Verify host identity Client->>Host: Sealed account authentication Host-->>Client: Sealed paired credential endReviews (2): Last reviewed commit: "fix(ios): avoid Int64(Double) trap on ma..." | Re-trigger Greptile