fix: add keychain-access-groups entitlement to preserve sessions across updates#562
fix: add keychain-access-groups entitlement to preserve sessions across updates#562
Conversation
…ss updates Add $(TeamIdentifierPrefix)com.browseros to keychain-access-groups in app-entitlements.plist so that Keychain ACLs are tied to the Team ID rather than the exact binary signature. This prevents users from losing all sessions/cookies when BrowserOS is updated via Sparkle, since the new binary's code signature no longer needs to exactly match the old one.
Greptile SummaryThis PR implements a comprehensive, multi-layered strategy to keep the BrowserOS keychain encryption key accessible across app updates and signing-identity changes on macOS. It addresses the root cause (item ACL tied to a specific binary hash) at three independent layers: signing-time (Team ID–pinned designated requirement in Confidence Score: 4/5PR is on the happy path to merge; the previous critical review concern is resolved and the architecture is sound — one minor diagnostic gap remains in SecItemUpdate error handling. The multi-layer keychain migration strategy is well-designed and the previously flagged SecKeychainSetUserInteractionAllowed no-op has been correctly fixed by switching to SecKeychainFindGenericPassword. The only remaining gap is that SecItemUpdate results are silently discarded in both branches of MaybeMigrateKeychainAccess, unlike the identical pattern in sparkle_glue.mm which logs all outcomes. This won't cause data loss or functional regression — the item remains accessible even if the access-group update fails — but will make repeated keychain prompts harder to diagnose. A one-line fix per call site resolves it. packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm — SecItemUpdate status handling at lines 84–85 and 136–137 Important Files Changed
Sequence DiagramsequenceDiagram
participant App as BrowserOS App
participant KR as MaybeMigrateKeychainAccess
participant KP as keychain_password_mac
participant SG as SparkleGlue
participant KC as macOS Keychain
Note over App,KC: Startup (first launch after update from v0.44.0)
App->>KR: MaybeMigrateKeychainAccess()
KR->>KC: SecItemCopyMatching("BrowserOS Safe Storage")
alt Access OK
KC-->>KR: errSecSuccess
KR->>KC: SecItemUpdate → set kSecAttrAccessGroup
KR-->>App: return (migrated)
else Item not found
KC-->>KR: errSecItemNotFound
KR-->>App: return (will be created on first use)
else ACL mismatch (e.g. v0.44.0 → new binary)
KC-->>KR: errSecAuthFailed
KR->>KC: SecKeychainFindGenericPassword() [legacy prompt]
KC-->>App: macOS dialog: "BrowserOS wants keychain access"
App-->>KC: User clicks Allow
KC-->>KR: errSecSuccess (binary added to ACL)
KR->>KC: SecItemUpdate → set kSecAttrAccessGroup
KR-->>App: return (recovered)
end
Note over App,KC: Normal operation — every password read/write
App->>KP: GetOrCreateEncryptionKey()
KP->>KC: AddGenericPassword / FindGenericPassword
KC-->>KP: success
KP->>KC: SecItemUpdate → set kSecAttrAccessGroup (best-effort)
Note over App,KC: Before Sparkle installs update
SG->>SG: showReadyToInstallAndRelaunch
SG->>SG: broadenKeychainACLBeforeUpdate()
SG->>KC: SecCodeCopySelf → get Team ID
SG->>KC: SecItemUpdate → set kSecAttrAccessGroup = TeamID.com.browseros
KC-->>SG: errSecSuccess / logged warning on failure
SG->>App: proceed with update installation
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm
Line: 84-85
Comment:
**`SecItemUpdate` result silently dropped in both migration paths**
The result of `SecItemUpdate` is never checked or logged here (or in the equivalent call at lines 136–137 in the recovery path). If the update fails — e.g., because `errSecItemNotFound` races with first-use creation, or because an entitlement mismatch prevents the write — the item never gets the access group set, and the same ACL-mismatch symptom will recur on every subsequent launch until the migration finally succeeds. The `sparkle_glue.mm` counterpart handles this correctly with a `LOG(WARNING)` on non-`errSecItemNotFound` failures; the same pattern would help here.
```suggestion
OSStatus migrateStatus = SecItemUpdate(
(__bridge CFDictionaryRef)updateQuery,
(__bridge CFDictionaryRef)update);
if (migrateStatus != errSecSuccess &&
migrateStatus != errSecItemNotFound) {
LOG(WARNING) << "browseros: Failed to set keychain access group"
<< " (status=" << migrateStatus << ")";
}
```
The same fix applies to the `SecItemUpdate` call at line 136 in the recovery branch — where the comment even asserts "SecItemUpdate will work because the user just granted access", making a silent failure especially confusing to diagnose.
How can I resolve this? If you propose a fix, please make it concise.Reviews (4): Last reviewed commit: "fix: add missing forward declaration and..." | Re-trigger Greptile |
- Extend keychain_password_mac.mm patch to set kSecAttrAccessGroup on keychain writes and reads, ensuring items are stored under the shared BrowserOS access group rather than tied to a specific binary signature - Pin the designated requirement in macos.py to the Team ID via certificate leaf[subject.OU] instead of wildcard /* exists */ checks, so Keychain ACLs survive across builds with different binary hashes - Add browseros_keychain_recovery.mm that runs on startup to detect broken keychain access (errSecAuthFailed), prompt the user for access, and migrate the item to the BrowserOS access group - Add broadenKeychainACLBeforeUpdate to sparkle_glue.mm that updates the keychain item's access group right before Sparkle replaces the app bundle, so the new binary can read it immediately Resolves: TKT-666, TKT-668, TKT-669, TKT-670
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
...ages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm
Outdated
Show resolved
Hide resolved
SecKeychainSetUserInteractionAllowed only affects the legacy SecKeychain* APIs, not SecItemCopyMatching. Replace the recovery path with SecKeychainFindGenericPassword which actually triggers the macOS system dialog asking the user to grant keychain access and updates the item's ACL to include the new binary.
- Add broadenKeychainACLBeforeUpdate to SparkleGlue forward declaration so BrowserOSUserDriver can call it without -Werror compile failure - Downgrade LOG(INFO) to VLOG(1) for normal keychain access paths that fire on every startup
2 participants
This pull request implements robust support for keychain access group migration on macOS, ensuring that encrypted user data (such as cookies and passwords) remains accessible after app updates or signing identity changes. It introduces logic to migrate the BrowserOS keychain item to a team-based access group, updates signing and entitlement configurations, and adds new code to handle keychain recovery and migration both at startup and during app updates.
Keychain Access Group Migration and Recovery:
browseros_keychain_recovery.handbrowseros_keychain_recovery.mmto check and migrate the BrowserOS Safe Storage keychain item to a team-based access group (<TeamID>.com.browseros), with interactive recovery if access is denied. This ensures future updates or signing identity changes do not break keychain access. [1] [2]keychain_password_mac.mmto always create and migrate the BrowserOS keychain item into the correct access group, both when adding and retrieving passwords, using the current Team ID from the app's code signature. [1] [2]app-entitlements.plistto include the new keychain access group, allowing the app to read/write the migrated keychain item.App Signing and Build Process:
macos.pyto pin the designated requirement to the Team ID, ensuring keychain ACLs survive across builds with different binary hashes. The signing logic now passes the Team ID and logs a warning if it's missing. [1] [2] [3] [4]BUILD.gn) to include the new keychain recovery source files for macOS builds.Update Process Integration:
sparkle_glue.mmso that before an update replaces the app bundle, the keychain item's ACL is broadened to the team-based access group, ensuring the new app version can access the existing encryption key. [1] [2] [3]These changes collectively ensure that BrowserOS users on macOS will not lose access to encrypted data after updates or signing changes, and that keychain items are always accessible to the app as long as it is signed by the same Team ID.