KM-17456: Fix Mac Catalyst reconnect keychain race - #370
Conversation
…e instead of delete-then-add
|
Claude finished @kp-diego-trevisan's task in 1m 51s —— View job Review — PR #370: Fix Mac Catalyst reconnect keychain race
Verdict: LGTM. This is the correct fix and it's implemented cleanly. Switching I traced the impact to confirm the "stale persistent reference" claim: What I like
Suggestions (all minor / optional)
Threading / memory / security
Suggested test flows
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case errSecItemNotFound: | ||
| // No existing item — add a fresh one. | ||
| query[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock | ||
| query[kSecValueData as String] = data | ||
| let addStatus = SecItemAdd(query as CFDictionary, nil) | ||
| guard addStatus == errSecSuccess else { | ||
| throw KeychainError.add | ||
| } | ||
| default: | ||
| throw KeychainError.add | ||
| } |
There was a problem hiding this comment.
Minor — residual add-path race / errSecDuplicateItem. The SecItemUpdate path is now atomic, which nicely closes the read gap for the common (item-exists) case. But the errSecItemNotFound → SecItemAdd fallback reintroduces a small TOCTOU window: if two callers race on a first-time write, both can see errSecItemNotFound from the update, and the second SecItemAdd returns errSecDuplicateItem, which falls into default: and throws KeychainError.add.
Consider handling the duplicate by retrying the update, so a concurrent first-write resolves instead of failing:
case errSecItemNotFound:
query[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock
query[kSecValueData as String] = data
let addStatus = SecItemAdd(query as CFDictionary, nil)
switch addStatus {
case errSecSuccess:
break
case errSecDuplicateItem:
// Lost an add race — the item now exists, so update in place.
query.removeValue(forKey: kSecAttrAccessible as String)
query.removeValue(forKey: kSecValueData as String)
guard SecItemUpdate(query as CFDictionary, attributesToUpdate as CFDictionary) == errSecSuccess else {
throw KeychainError.add
}
default:
throw KeychainError.add
}This is a genuinely narrow edge and may not be worth it if set(password:) is never called concurrently for a brand-new account — your call. Also worth noting: SecItemUpdate updates all items matching the query, so if duplicate entries for the same account ever existed, the old delete-then-add collapsed them to one, whereas this keeps them all. Not a concern in normal operation.
KB review —
|
… first-write race
|
Claude finished @kp-diego-trevisan's task in 54s —— View job Follow-up review — commit
|
3 participants
What
Changes
Keychain.set(password:for:)to update the keychain item in place viaSecItemUpdate(falling back toSecItemAddonly when the item doesn't exist yet), instead of deleting the item and re-adding it.Why
The previous delete-then-add approach had two problems on the VPN reconnect path:
passwordReferencealready baked into the savedNEVPNProtocolConfiguration. On reconnect the tunnel could no longer resolve the credential → auth failure.SecItemUpdatepreserves item identity, so the reference stays valid.errSecItemNotFound. Updating in place is atomic and removes that window.This is the standard update-in-place pattern used by Apple's own keychain sample code.