cmux NEXT: paired-devices.json counts only when it is the list the app saved last

Merged
TE
10d ago
+734-33

What

paired-devices.json is the Mac's list of approved phones, kept in the iPhone host's state folder. From now on it counts only when it is exactly the list this app saved last.

Each save records a SHA-256 digest of the file's bytes in the data-protection Keychain, in the app's own access group: service <bundle id>.iphone-host.pairing-seal, account paired-devices, this device only, never synced.

MobileHostPairedDevicesFile.load checks the digest once, before it decodes anything. Both consumers get only that result:

  • MobileHostLaunch.devicesLine, the keys hostd pins;
  • approversParams, the approver keys acpmux enrolls.

When the file doesn't match the recorded digest, or the digest is missing or unreadable, load returns an empty list for the user, logs one line, and trusts no phone. It never falls back to trusting the file.

This closes the base hole named as landing term 5 of #24233. Before this change, any process running as the same user could rewrite paired-devices.json, swap in an approval key of its own, and acpmux enrolled that key as an approver at the next launch.

How

  • MobileHostPairingSeal.swift (new, in CmuxNextMobile):

    • MobileHostPairingSealStore protocol: digest(), setDigest(_:) and clearDigest().
    • MobileHostKeychainPairingSeal. Every query sets kSecUseDataProtectionKeychain and names no access group, so it uses the app's default group from the signing entitlement. Every query also passes an LAContext with interactionNotAllowed, so it never prompts; loadPaired runs on the main actor. A write updates the item, adds it when missing, and on a duplicate updates again.
    • MobileHostMemoryPairingSeal, the in-memory fake the tests use.

  • The digest: SHA-256 over a domain string, a zero byte, and the file's exact bytes.

  • Why the data-protection Keychain (review round 1): another process can't add, read or change an item in the app's access group, because that takes the app's entitlement. So it can't plant a digest before first use, and it can't match a digest to a list of its own.

  • Why a digest of the current list, not an HMAC key (review round 1): only the list saved last matches. Putting back an older copy (one saved before a phone was removed) trusts no phone.

  • Save order (review rounds 2 and 3):

    1. The old digest is deleted first.
    2. The json is written atomically, even when the Keychain refused the delete: the new list doesn't match the old digest, so the list before it (perhaps with a phone just removed) stops counting too.
    3. The new digest is recorded. When that fails the save throws, and no list is trusted at the next launch.

    From step 1 until step 3 no copy of any list counts. So a crash, or a Keychain that refuses the new digest, trusts nothing. It never trusts an older list put back, for example the one from before a phone was removed.

  • A save that takes trust away (a revoked phone, a phone that signed out, a sign-out of this Mac; review round 3) goes through saveRemovingTrust. It deletes the old digest synchronously on the save queue, after the saves already queued, before it returns, then queues the write and record. So a crash or quit right after a revocation trusts no older list. App quit (IPhoneHostService.stop) waits up to 2 s for the queued saves (MobileHostPairedDevicesFile.flush).

  • Off the main actor (review round 2): the iPhone host is @MainActor, and a Keychain call is synchronous IPC. So loadPaired and savePaired now go through loadInBackground/saveInBackground on one serial queue, which keeps saves and reads in order.

    • Until a launch's or an account change's read comes back, paired is nil, so no phone is trusted, hostd pins nothing, acpmux gets no approvers, and no pairing request is decided (each needs paired).
    • Only the newest read counts (MobileHostPairedLoad, review round 3): every account change starts a new one, so a read from before a sign-out and a sign-in of the same user is dropped. A sign-out while the list is read still clears it.
    • A phone that signs out on the phone, or that the person removes, while the list is read: its links are dropped at once, and it is removed from the list when the list is in, then saved with saveRemovingTrust and pushed (review round 3).
    • A save failure is logged on the queue, as before.

  • No file-keychain fallback. A build without the keychain-access-groups entitlement (an ad hoc signed tagged build) reads no digest, so it keeps no phone across a launch. Signed nightly and release builds have the entitlement; the relay secrets already rely on it (KeychainRelaySecretStore).

  • IPhoneHostService+Pairing.swift: pairedFile passes the Keychain seal. The bundle id is the state folder's parent, the same Application Support/<bundle id> the wiring builds. This is the only change to the pairing service.

Upgrade path (chosen)

A list written before this change has no digest, so it is not trusted, and each paired phone pairs again once through the normal prompt the person approves.

Recording a digest for an unsealed list on first launch would vouch for whatever a same-user process had already written into it, so it can't be shown safe.

This also means a tampered or unsealed list keeps no phone paired, not only no approver. The list's identity keys (what hostd pins) come from the same unverified file as the approval keys, so trusting them would trust the file; the round 1 reviewer agreed. After the re-pair, the phone is both paired and an approver again, as before.

Tests (CmuxNextMobileTests, MobileHostPairingTests, next to theFileBelongsToOneUser)

  • aTamperedListEnrollsNoApproverAfterARelaunch: approve a phone and save. Rewrite the file with another approval key, then load it as the next launch would (new file object, same seal). No approver reaches acpmux and hostd pins nothing.

  • anOlderListPutBackTrustsNoPhone: save with a phone, keep a copy, remove the phone and save, put the copy back. The removed phone is neither an approver nor pinned.

  • aListWithoutItsDigestTrustsNoPhone: no digest (a pre-seal list) and a Keychain that refuses each trust nothing. A save the Keychain refuses throws and leaves no list trusted, and a recorded save trusts the phones again.

  • aRemovalTheKeychainRefusesTrustsNoOlderList: the Keychain refuses the delete and the new record. Removing a phone throws, and at the next launch neither consumer gets the removed phone.

  • aRemovalTakesTheOldDigestAwayBeforeItReturns: a save queued before it holds the queue for 0.3 s (a gated test seal). When saveRemovingTrust returns, the old digest is gone; the new list is then saved.

  • aReadFromBeforeASignOutDoesNotCount and aPhoneForgottenWhileTheListIsReadIsRemovedFromIt: MobileHostPairedLoad's two rules.

  • aRevocationWhoseDigestIsNotRecordedLeavesNoOlderListTrusted: save with a phone and keep a copy. Then remove the phone while the new digest can't be recorded (the crash window, or a refusal), and put the copy back. Neither consumer gets the phone.

  • aBackgroundReadSeesTheSavesQueuedBeforeIt: two background saves, then a background read, which sees the second.

  • theSealIsInTheAppsOwnDataProtectionKeychain: the query uses the data-protection Keychain, is not synchronizable, and names no access group, so it gets the app's entitled default.

    A test that seeds the Keychain from another process can't run in swift test: the unsigned test runner has no keychain-access-groups entitlement, so it can't reach the data-protection Keychain at all. That is the same OS boundary the fix relies on.

  • theFileBelongsToOneUser now uses the fake seal.

Runs on the fleet (cmux-ci run ... package-test-lane.sh suite Packages/macOS/CmuxNext MobileHostPairingTests,IPhoneHostChiefTests,MobilePageProviderTests):

  • Red at 8563d86e47 (the tests and new types over base's load and save; step f061b2498d7930ccc7d11249). Four tests fail:

    • aTamperedListEnrollsNoApproverAfterARelaunch;
    • anOlderListPutBackTrustsNoPhone;
    • aListWithoutItsDigestTrustsNoPhone;
    • aRevocationWhoseDigestIsNotRecordedLeavesNoOlderListTrusted.

  • Green at 6b9ee28db7 (step 1026d19b078a7216789225e0): all 15 tests pass in MobileHostPairingTests and MobileHostMacIdentityTests. The pairing service's suites pass too: IPhoneHostChiefTests 2/2 and MobilePageProviderTests 7/7.

  • Review round 3: red at f52eaf26ed (the new tests, with stubs that behave as the reviewed code; step 7cf12f05b43add39870a0614): all five new or changed tests fail, 7 issues. The fix at 36887c1496 (step 5ef5872d41d3d7411d420711) failed only aRevocationWhoseDigestIsNotRecordedLeavesNoOlderListTrusted, which expected the save not to throw; af7be1e4a1 changes it to expect keychainRefused and keeps its trust checks. Green at af7be1e4a1 (step c27a391ffd485317944ca721: MobileHostPairingTests, MobileHostMacIdentityTests, IPhoneHostChiefTests, MobilePageProviderTests all pass).

  • Review round 4 (five findings on af7be1e4a1, fixed at 45a9dc7f28):

    1. Clear and record both refused leave the old digest readable, so the old list put back re-trusts a revoked phone. A refused clear is now tried again before the write and once more after a refused record. In this run the replaced list's digest is withdrawn: no list a removal replaced counts again in this process, even if its exact bytes are put back and read again. Test: aRevokedListPutBackIsNotTrustedAgainInThisRun.
    2. A phone forgotten while the list is read was ignored when the read returned an empty or untrusted list. MobileHostPairedLoad.finish now reports any forgotten phone, so the list is always saved as a removal then. Test: aPhoneForgottenWhileAnEmptyListIsReadStillTakesTrustAway.
    3. The main thread waited (queue.sync in a removal, a semaphore on quit). A removal no longer waits on the caller's thread: on the save queue, after the saves before it, it withdraws the old digest, clears it and writes and records the new list. The person's new list is the one in force from the call on. The quit flush stays bounded at 2 s and runs only from applicationWillTerminate, inside allowBlocking(.terminate) (the threading model's reviewed termination allowance), through the blockingWait seam. Test: aRemovalNeverWaitsOnTheCallersThread; aRemovalTakesTheOldDigestAwayBeforeItReturns became aRemovalTakesTheOldDigestAwayAfterTheSavesBeforeIt.
    4. The queue was not in the threading model. com.cmuxterm.mobile.paired-devices is registered in plans/cmux-next/threading-model.md (utility, serial, its file and Keychain calls block only it), and the queue now states qos: .utility. scripts/cmux-next/guardrails.py over the PR's range: ok (run 40396478744). On the previous head it refused task-priority, raw-blocking and thread-doc (run 40396603380); raw-blocking no longer fires because the file reads and writes run on that registered queue.
    5. An unawaited empty devices update could reach a restarted hostd after the old list. Base has the same unordered hop (Task { await supervisor.setDevices(list) }). This PR's sign-out and revocation depend on it, so the paired-list updates are now one ordered chain (devicesUpdate), and a hostd start waits for the newest update before it starts, so hostd always starts with the newest list. The supervisor's cached line is that newest list (empty after a sign-out), so it needs no separate clear.

  • Review round 5 (four findings on 45a9dc7f28, fixed at f57c4b8e04; 6dcefc0d04 before them only adds the reviewed // concurrency-allow: comment at the quit flush's semaphore, which the concurrency check requires):

    1. A phone removed while the list is read was only saved once the read was back, so a quit's flush could pass first. A removal during a read now queues withdrawTrust() at once: on the save queue, right after the read, the stored list stops counting in this run and its digest is cleared, so the flush covers it. Test: aPhoneForgottenDuringAReadLosesTrustBeforeTheQuitFlushReturns.
    2. A hostd restart could hand over a list an update still on its way would replace. MobileHostSupervisor.setNewestDevices stores the newest devices line at once, on the caller's thread; every hostd start and restart hands over that line, and the hop to a running hostd sends the newest. The ordered update chain is gone. Test: aNewHostdGetsTheNewestPairedListAtOnce.
    3. The set of withdrawn digests grew with every removal. A run now keeps one digest per file: the newest list it saved. Test: theRunsRecordOfReplacedListsStaysBounded (40 rounds, one digest).
    4. Approving new keys for a paired phone did not withdraw the list with the old keys. Every save now makes the list it replaces stop counting in this run, a key replacement included; saveRemovingTrust folded into saveInBackground. Test: aReplacedKeyPutBackIsNotTrustedAgainInThisRun.
    • Red at 55d15a647f (the new tests, with stubs that behave as the reviewed code; step 3881eab9d92add60fcd25111): all four new tests fail, 7 issues. Green at f57c4b8e04 (step 6bb84b559d107a9380766067): MobileHostPairingTests, MobileHostMacIdentityTests, MobileHostTests, IPhoneHostChiefTests, MobilePageProviderTests all pass. check-concurrency and guardrails.py over the range: ok (run 40414817960).

Residual risks

  • Ad hoc signed builds keep no paired phone across a launch (no entitlement, no fallback).
  • A pairing request that arrives before a launch's read comes back is dropped, as base already drops one that arrives with no list loaded (ask needs paired). The read is one Keychain lookup and one small file read.
  • A removal no longer waits on the main thread; the app's quit waits up to 2 s for queued saves, under the reviewed terminate allowance.
  • A crash between a removal's click and the save queue's turn (the queue holds only earlier saves: one file write and a few Keychain calls each) keeps the list from before the click, as any write would. The main thread may not wait on Keychain IPC, so the click itself cannot make the removal durable.
  • Across a relaunch, a Keychain that refuses every write of the app's own item (the delete twice and the new record) leaves the old digest in place. The app's own Keychain item is the only store a same-user process cannot rewrite, so nothing durable can record that revocation. If a process then puts back the old list's exact bytes, that list counts again after a relaunch. In the same run it does not count (finding 1). The app logs a fault when this happens.
  • A process that runs as the app itself, or with its entitlement, can write the digest; that is outside this threat model.

No capture: no UI changes. Phones paired before this change see the existing pairing prompt once.

Overlap: #25437 (HOLD, a different change: a local Allow gate for Phone Access) touches the same two source files. Whichever lands second rebases.

Scope: only MobileHostPairedDevices.swift, the new seal file, the tests, and in the pairing service: the pairedFile line, loadPaired's read and savePaired's write off the main actor, revokePhone while the list is read, the pairedLoad field and the bounded flush in stop() (IPhoneHostService.swift), and in MobileHostSupervisor the newest devices line kept at once (setNewestDevices). No other pairing code changed.

Review: a fresh Codex gpt-6-astra xhigh review at f57c4b8e04222cc203904d5986af00e16dc31604 said SHIP (review round 6, after the round 5 fixes above).

Agent session: agent session

šŸ¤– Generated with Claude Code