Skip to content

KM-17456: Fix Mac Catalyst reconnect keychain race #370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kp-diego-trevisan merged 2 commits into from
Jul 24, 2026
Merged

KM-17456: Fix Mac Catalyst reconnect keychain race #370

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fedfda
Fix Mac Catalyst reconnect race by updating keychain password in plac…
kp-diego-trevisan Jul 24, 2026
aa1eafc
Handle errSecDuplicateItem in keychain password add fallback to close…
kp-diego-trevisan Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions LocalPackages/PIALibrary/Sources/PIALibrary/Util/Keychain.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,44 @@ public class Keychain {

/// :nodoc:
public func set(password: String, for username: String) throws {
removePassword(for: username)
guard let data = password.data(using: .utf8) else {
throw KeychainError.add
}

var query = [String: Any]()
setScope(query: &query)
query[kSecClass as String] = kSecClassGenericPassword
query[kSecAttrAccount as String] = username
query[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock
query[kSecValueData as String] = password.data(using: .utf8)

let status = SecItemAdd(query as CFDictionary, nil)
guard (status == errSecSuccess) else {
let attributesToUpdate: [String: Any] = [
kSecValueData as String: data,
kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock
]
let updateStatus = SecItemUpdate(query as CFDictionary, attributesToUpdate as CFDictionary)

switch updateStatus {
case errSecSuccess:
return
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)
switch addStatus {
case errSecSuccess:
return
case errSecDuplicateItem:
// Lost a first-write race — the item now exists, so update it 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
}
default:
throw KeychainError.add
}
Comment on lines +103 to 124

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Expand Down
Loading