Repository navigation
feat(runtime): device-key authentication and per-installation key migration (host) - #27493
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to The change looks mergeable. The two device-key tests should be tightened so they would catch regressions in keyed dispatch and in the identity assigned at registration. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 34 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly identifies the main change: host-side device-key authentication and migration for runtime installations. Description check The description includes all required sections and provides clear ELI5, implementation details, rationale, issue references, testing coverage, compatibility notes, and checklist status. It notes that … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…ockets dispatch without a token; forks keep their Relay binding
…key is never 'revoked' while it exists
…en when the registry file is missing
… dispatcher for runtime and mobile rows
33768a9 to
a2bf119
Compare
|
Review (P0/P1 only): the codex reviewer was unavailable (orchestration consumer_fenced; retried twice), so an Opus security reviewer ran three rounds.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8c97283d-5f00-46f4-89b6-d3c9ffe55ffc
⛔ Files ignored due to path filters (1)
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (1)
src/main/runtime/runtime-rpc-device-key-dispatch.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| const { server, keyed, send } = keyedServer() | ||
| const dispatch = vi.spyOn(server['dispatcher'], 'dispatchStreaming') | ||
| const reply = await send({ id: 'r1', method: 'status.get' }) | ||
| expect(reply).not.toMatchObject({ error: { code: 'unauthorized' } }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that tokenless dispatch succeeds.
A reply with any error code other than unauthorized passes this assertion. The dispatch spy also records failed calls. Assert ok: true so this test fails when status.get cannot complete on a keyed socket.
| const reply: unknown = JSON.parse(replies.at(-1)!) | ||
| expect(reply).toMatchObject({ id: 'register', ok: true, result: { credentialRevision: 0 } }) | ||
| expect( | ||
| registry.listDevices().filter((row) => row.forkedFrom === legacy.deviceId) | ||
| ).toHaveLength(1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the registered device ID and public key.
This test passes if registration returns a different deviceId or stores a different public key. It checks only the revision and the number of forks. Compare reply.result.deviceId with the forked row’s deviceId, and compare that row’s devicePublicKeyB64 with key.publicKey. Based on learnings: identity-sensitive tests must assert the fields that determine the resulting identity, not only adjacent metadata.
Source: Learnings
Upstream now routes project group catalog calls by their owner host (stablyai#27288): createProjectGroup takes the seeding project's host, and rows are adopted through adoptFromEndpoint. Subgroup creation moves to its own createProjectSubgroup action, routed by the parent group's owner like moveProjectGroup, and the update/move helper uses the new owner routing. protocol-version.ts re-exports the attention capabilities with a star, as stablyai#27493 did for the resume ones, to stay within the line limit.
ELI5
A paired phone or computer can now sign in to this desktop with its own private key, instead of a reusable pairing code. A device that already has a code swaps it for a key on its own, the first time an updated version connects. Other devices that share the same code are not affected. Nothing is cut off, and nobody has to re-pair. This PR is the desktop half; the phone, CLI and web halves come next.
What Changed
Before: the only way to authenticate a paired socket was to send the device token inside the encrypted channel. The token was also the stored credential, so anyone who captured it could connect as that device forever.
After (host only; no client sends the new shapes yet):
{type:'e2ee_auth', v:2, transcriptHashB64, deviceId, deviceProofB64, clientCapabilities?}."orca-device-proof/v1\0" + purpose + "\0" + transcriptHash, wheretranscriptHashis this connection's v2 transcript. The transcript contains a fresh host nonce, so a captured proof cannot be replayed on another connection, and no replay store is needed.classifyE2EEAuthis now the single parser. Mixed shapes are rejected, and key auth over legacy framing isbad_auth.orca-devices.jsoncarrycredentialKind:'device-key',devicePublicKeyB64andforkedFrom, and have notoken.pairing.registerDeviceKeyuses the newdevice-selfpermission. Every paired caller holds it, and it is on the phone allowlist.registerand must be over that socket's own v2 transcript.device_revoked {deviceId}: the id is unknown and the registry is readable. This is the only verdict that will let a client delete its key.registry_unavailable: the registry could not be read; the client keeps its key and retries.bad_proof: the signature did not verify.unauthorizedexactly as today.orca-devices.json.corrupt-<ts>) before the first rewrite.registry_unavailable, neverdevice_revoked. That is fail-safe: a really revoked device keeps retrying until the owner deletes the copy.unauthorized, never a revocation verdict.pairing.device-key.v1is advertised. Clients migrate or present a key only when they see it.DeviceEntry.tokenis optional, andTokenDeviceEntrytypes the rows that do have a token (offers, delegated children).replaceRow. Row types moved todevice-entry.ts, and legacy reply senders moved out ofe2ee-channel.ts; both moves are behavior-preserving and keep the files under the size cap.Why
The design splits the pairing token's four jobs (secret, credential, identity, authority). The device-principal groundwork (#27480) separated identity. This PR adds a credential that is not a bearer.
Fork instead of replace: one token is routinely shared by the QR, several browser tabs, or
ORCA_PAIRING_CODEused on several machines. Replacing the row's credential would lock out every other holder.Signing the existing transcript: this reuses the v2 handshake's fresh nonce, instead of adding a challenge round trip or a persisted replay store.
Sealed verdicts: a client must never delete its key because of storage trouble or an older host. Only an authenticated "this device id is gone, and I can read my registry" justifies deletion.
Linked Issue
Part of #17394 and #17398 (host side; closes with the client PRs)
Visual Proof
N/A: no UI change.
Testing
src/main/runtime/rpc/device-key-auth.test.tsruns end to end throughMobileSocketWiringwith real v2 crypto:device_revokedis sent only with a readable registry, otherwiseregistry_unavailable;device-key-fork.test.ts:runtime-rpc-device-key-dispatch.test.ts: a keyed socket dispatches with its device id as client id, and is refused if it carries a token or once its row is revoked.src/shared/device-key-proof.test.ts(domain separation, canonical base64, label sanitizing) andmobile-e2ee-auth-classification.test.ts(every shape, mixtures).Cross-version (
tests/e2e/cross-version-wire/cross-version-device-key-registry.unit.test.ts, released baseline):bad_auth(so a keyed client can show "host downgraded" and keep its key);pnpm tc,pnpm test src/main/runtime src/shared, oxlint,check:code-quality:changed,pnpm lint.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
bad_auth.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)