mirror of
https://github.com/yattee/yattee.git
synced 2026-10-23 05:02:53 +00:00
Stop blind rollbacks in the Keychain delete paths
Fifth review pass, which prompted measuring whether SecItemDelete can be conditioned on a value. It cannot: a delete issued with a deliberately wrong kSecValueData still returns 0 and removes the item, so there is no compare-and-delete and every check-then-delete is a race by construction. That measurement decides both fixes here. Adoption no longer rolls back its own write when the verification read comes back empty. The rollback could only ever be a blind delete of whatever occupied the key by then - including a credential synced from another device in the gap - and it would propagate. Nothing is deleted; no marker is written, so the account stays signed out and an ordinary sign-in recovers it. Legacy deletion re-reads immediately before deleting and aborts if the value moved, shrinking the window to a single call gap and catching a replacement that landed while the fingerprint was being computed. A residual race remains and is documented at the call site: closing it is not possible with these APIs, and never deleting would leave a credential the user explicitly removed sitting in their iCloud Keychain. The debug provenance report repeated the same nil-token ambiguity the scope logic was fixed for, reporting "no Apple account signed in" when iCloud Drive is merely off. It now reports the token as unavailable and shows CloudKit's account status separately, so a re-run cannot draw a false conclusion from it.
This commit is contained in:
@@ -236,15 +236,20 @@ final class AccountKeychainCredentials {
|
||||
return false
|
||||
}
|
||||
|
||||
// Nothing readable is there: the add reported success but produced
|
||||
// no usable item, so rolling it back destroys nothing and lets the
|
||||
// adoption offer be retried.
|
||||
// Nothing readable is there. Tempting to roll the write back, but a
|
||||
// delete cannot be conditioned on the value that was just read:
|
||||
// `SecItemDelete` ignores `kSecValueData` (measured — a delete
|
||||
// issued with a deliberately wrong value still removes the item),
|
||||
// so the rollback would be a blind delete of whatever occupies the
|
||||
// key by then, including a credential that arrived from another
|
||||
// device in the gap — and it would propagate. Nothing is deleted;
|
||||
// no marker is written, so the account simply stays signed out and
|
||||
// an ordinary sign-in (a replacing write) recovers it.
|
||||
LoggingService.shared.error(
|
||||
"Legacy \(credentialNoun) adoption failed verification",
|
||||
"Legacy \(credentialNoun) adoption failed verification — sign-in required",
|
||||
category: .keychain,
|
||||
details: "accountID=\(account.id)"
|
||||
)
|
||||
store.delete(key: key)
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -287,6 +292,28 @@ final class AccountKeychainCredentials {
|
||||
return
|
||||
}
|
||||
|
||||
// Re-read immediately before deleting. The Keychain offers no
|
||||
// compare-and-delete — `SecItemDelete` ignores `kSecValueData`
|
||||
// (measured) — so the fingerprint check above can only ever validate a
|
||||
// snapshot. Re-reading here shrinks the window to a single call gap and
|
||||
// catches a replacement that landed while the fingerprint was being
|
||||
// computed.
|
||||
//
|
||||
// A residual race remains by construction: if another device running
|
||||
// the previous release writes a new legacy value inside that gap, this
|
||||
// deletes their fresh login and the deletion propagates. Accepted —
|
||||
// closing it is not possible with these APIs, and the alternative
|
||||
// (never deleting) leaves a credential the user explicitly removed
|
||||
// sitting in their iCloud Keychain indefinitely.
|
||||
guard store.value(forKey: legacyKey) == currentValue else {
|
||||
LoggingService.shared.info(
|
||||
"Legacy \(credentialNoun) changed while being retired — leaving it in place",
|
||||
category: .keychain,
|
||||
details: "instanceID=\(account.instanceID)"
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
store.delete(key: legacyKey)
|
||||
markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope)
|
||||
|
||||
|
||||
@@ -70,7 +70,11 @@ enum KeychainProvenanceReportBuilder {
|
||||
/// - label: free-form run label, e.g. "iPhone 15 — signed in as identity B".
|
||||
/// - userRecordID: resolved asynchronously by the caller (network), `nil`
|
||||
/// when unavailable.
|
||||
static func build(label: String, userRecordID: String?) -> String {
|
||||
static func build(
|
||||
label: String,
|
||||
userRecordID: String?,
|
||||
accountAvailability: CloudKitIdentity.AccountAvailability
|
||||
) -> String {
|
||||
let results = KeychainProvenanceProbe.queryAll()
|
||||
|
||||
var lines: [String] = []
|
||||
@@ -82,7 +86,11 @@ enum KeychainProvenanceReportBuilder {
|
||||
|
||||
lines.append("--- Identity ---")
|
||||
lines.append("CloudKit entitlement: \(CloudKitAvailability.isAvailable ? "available" : "missing (sideload)")")
|
||||
lines.append("ubiquityIdentityToken: \(CloudKitIdentity.ubiquityFingerprint.map { "present, fp=\($0)" } ?? "none — no Apple account signed in")")
|
||||
// A nil token does NOT mean signed out — Foundation also returns nil
|
||||
// when iCloud Drive is off, so the report must not assert either way.
|
||||
// CloudKit's account status is what disambiguates.
|
||||
lines.append("ubiquityIdentityToken: \(CloudKitIdentity.ubiquityFingerprint.map { "present, fp=\($0)" } ?? "unavailable (signed out, or iCloud Drive off)")")
|
||||
lines.append("CloudKit account: \(describe(accountAvailability))")
|
||||
lines.append("CloudKit user record: \(userRecordID ?? "unresolved")")
|
||||
lines.append("")
|
||||
|
||||
@@ -128,6 +136,14 @@ enum KeychainProvenanceReportBuilder {
|
||||
accounts.isEmpty ? "none" : accounts.sorted().joined(separator: ", ")
|
||||
}
|
||||
|
||||
private static func describe(_ availability: CloudKitIdentity.AccountAvailability) -> String {
|
||||
switch availability {
|
||||
case .noAccount: "no account signed in"
|
||||
case .available: "signed in"
|
||||
case .indeterminate: "could not be determined"
|
||||
}
|
||||
}
|
||||
|
||||
// MARK: - Environment
|
||||
|
||||
private static var platformDescription: String {
|
||||
|
||||
@@ -21,6 +21,7 @@ import UIKit
|
||||
struct KeychainProvenanceExperimentView: View {
|
||||
@State private var label = ""
|
||||
@State private var userRecordID: String?
|
||||
@State private var accountAvailability: CloudKitIdentity.AccountAvailability = .indeterminate
|
||||
@State private var resolvingUserRecordID = true
|
||||
@State private var reports: [KeychainProvenanceReport] = KeychainProvenanceReportStore.load()
|
||||
@State private var latestStatus: String?
|
||||
@@ -38,6 +39,7 @@ struct KeychainProvenanceExperimentView: View {
|
||||
.navigationBarTitleDisplayMode(.inline)
|
||||
#endif
|
||||
.task {
|
||||
accountAvailability = await CloudKitIdentity.accountAvailability()
|
||||
userRecordID = await CloudKitIdentity.userRecordID()
|
||||
resolvingUserRecordID = false
|
||||
}
|
||||
@@ -78,10 +80,13 @@ struct KeychainProvenanceExperimentView: View {
|
||||
private var identitySection: some View {
|
||||
SettingsFormSection(
|
||||
"Identity",
|
||||
footer: "The fingerprint is an opaque hash of ubiquityIdentityToken. It must differ between accounts A and B — if it does not, the sign-out did not take effect."
|
||||
footer: "The fingerprint is an opaque hash of ubiquityIdentityToken and must differ between accounts A and B — if it does not, the sign-out did not take effect. An unavailable token does not by itself mean signed out: it also goes missing when iCloud Drive is off, which is why the account status is shown separately."
|
||||
) {
|
||||
row("CloudKit entitlement", CloudKitAvailability.isAvailable ? "available" : "missing")
|
||||
row("Apple account", CloudKitIdentity.ubiquityFingerprint.map { "fp=\($0)" } ?? "signed out")
|
||||
// A nil token is ambiguous — it also means iCloud Drive is off —
|
||||
// so the account status is shown beside it rather than inferred.
|
||||
row("Ubiquity token", CloudKitIdentity.ubiquityFingerprint.map { "fp=\($0)" } ?? "unavailable")
|
||||
row("Apple account", accountStatusText)
|
||||
row("User record", resolvingUserRecordID ? "resolving…" : (userRecordID ?? "unresolved"))
|
||||
}
|
||||
}
|
||||
@@ -179,7 +184,11 @@ struct KeychainProvenanceExperimentView: View {
|
||||
}
|
||||
|
||||
private func capture() {
|
||||
let text = KeychainProvenanceReportBuilder.build(label: label, userRecordID: userRecordID)
|
||||
let text = KeychainProvenanceReportBuilder.build(
|
||||
label: label,
|
||||
userRecordID: userRecordID,
|
||||
accountAvailability: accountAvailability
|
||||
)
|
||||
reports = KeychainProvenanceReportStore.append(
|
||||
KeychainProvenanceReport(label: label, text: text)
|
||||
)
|
||||
@@ -195,6 +204,15 @@ struct KeychainProvenanceExperimentView: View {
|
||||
latestStatus = "Captured report \(reports.count)."
|
||||
}
|
||||
|
||||
private var accountStatusText: String {
|
||||
if resolvingUserRecordID { return "checking…" }
|
||||
switch accountAvailability {
|
||||
case .noAccount: return "no account signed in"
|
||||
case .available: return "signed in"
|
||||
case .indeterminate: return "could not be determined"
|
||||
}
|
||||
}
|
||||
|
||||
private func payloadText() -> String {
|
||||
let fingerprint = CloudKitIdentity.ubiquityFingerprint ?? "none"
|
||||
let written = Date().formatted(.iso8601)
|
||||
|
||||
Reference in New Issue
Block a user