From c4053d5100cf149e0807d1c52f308e711522553e Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Sun, 23 Aug 2026 23:39:06 +0200 Subject: [PATCH] Close identity and adoption holes in credential scoping Four issues from an external review of the previous commit. The scope was resolved once at startup, so signing out or switching Apple account while the app kept running left it pinned to the old identity - reads and writes would then reach the previous account's retained credentials, which is the exact leak scoping exists to prevent. CredentialIdentityScope now observes NSUbiquityIdentityDidChange and re-resolves. Legacy deletion authority checked only the instance and the identity, not the account. On an instance with several accounts, deleting any of them satisfied the marker lookup and could retire the legacy item another account had adopted, logging out devices still on the previous release. The marker's accountID is now required to match. A first-time resolution that failed (launched offline, CloudKit unreachable) was never retried, disabling credentials for the rest of the process. resolve() now retries with backoff, and resolveIfNeeded() runs on foreground activation. Adoption re-checked nothing at execution time, so a scoped credential delivered by iCloud Keychain between rendering the offer and tapping it would be overwritten with the older legacy secret. Adoption now re-verifies eligibility and writes with a non-replacing add, leaving a duplicate strictly alone. Adds regression tests for the sibling-account deletion and the adoption race. --- .../AccountKeychainCredentials.swift | 37 +++++- .../CredentialAdoptionMarker.swift | 18 ++- .../Credentials/CredentialIdentityScope.swift | 114 +++++++++++++++--- .../Credentials/KeychainCredentialStore.swift | 19 +++ Yattee/YatteeApp.swift | 12 ++ YatteeTests/AccountCredentialsTests.swift | 45 +++++++ 6 files changed, 215 insertions(+), 30 deletions(-) diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 6308fb37..a34010cc 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -186,20 +186,46 @@ final class AccountKeychainCredentials { @discardableResult func adoptLegacyCredential(for account: Account) -> Bool { guard let scope = currentScope() else { return false } + guard !markers.hasAdopted(instanceID: account.instanceID, scope: scope) else { return false } guard let legacy = legacyCredential(for: account) else { return false } + // Eligibility is re-checked here, not just when the offer was rendered: + // iCloud Keychain may have delivered the real credential in between, and + // replacing it with the older legacy secret would be a silent downgrade. + guard credential(for: account) == nil else { + LoggingService.shared.info( + "Skipping legacy \(credentialNoun) adoption — a scoped credential already arrived", + category: .keychain, + details: "accountID=\(account.id)" + ) + return false + } + let key = KeychainCredentialStore.credentialKey(scope: scope, accountID: account.id) - let status = store.set(legacy, forKey: key, synchronizable: shouldSyncToiCloud) + let status = store.addIfAbsent(legacy, forKey: key, synchronizable: shouldSyncToiCloud) + + // A duplicate means a scoped credential landed between the check above + // and this write — leave it strictly alone, it is newer than the legacy + // secret. + guard status == errSecSuccess else { + LoggingService.shared.error( + "Legacy \(credentialNoun) adoption did not write", + category: .keychain, + details: "accountID=\(account.id), status=\(store.describe(status))" + ) + return false + } // The marker is only written after the scoped copy reads back: a marker // for a credential that never landed would suppress the adoption offer // and strand the user with no way to sign in short of retyping. - guard status == errSecSuccess, store.value(forKey: key) == legacy else { + guard store.value(forKey: key) == legacy else { LoggingService.shared.error( "Legacy \(credentialNoun) adoption failed verification", category: .keychain, - details: "accountID=\(account.id), status=\(store.describe(status))" + details: "accountID=\(account.id)" ) + // Safe to remove: this write is the item that just landed. store.delete(key: key) return false } @@ -229,13 +255,14 @@ final class AccountKeychainCredentials { guard markers.canDeleteLegacyItem( instanceID: account.instanceID, + accountID: account.id, scope: scope, currentLegacyValue: currentValue ) else { LoggingService.shared.info( - "Leaving the legacy \(credentialNoun) in place — not this identity's to delete", + "Leaving the legacy \(credentialNoun) in place — not this account's to delete", category: .keychain, - details: "instanceID=\(account.instanceID)" + details: "instanceID=\(account.instanceID), accountID=\(account.id)" ) return } diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index b61e4886..975ebf51 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -87,23 +87,28 @@ final class CredentialAdoptionMarkerStore { // MARK: - Legacy deletion authority - /// Whether an account deletion may also delete the **unscoped** legacy item. + /// Whether deleting an account may also delete the **unscoped** legacy item. /// - /// Three conditions, all required: + /// Four conditions, all required: /// /// 1. This device has a marker for the instance — no marker means this /// device never adopted anything, so the legacy item is not ours to touch. /// 2. The marker belongs to the current identity — Apple account A's marker /// cannot authorize a deletion while B is signed in. - /// 3. The legacy item still holds the exact secret that was adopted — if it - /// has since been replaced (B signed in again on an older release, and - /// that release rewrote the unscoped key), it is someone else's live - /// login and must survive. + /// 3. The marker belongs to the account being deleted. An instance can hold + /// several accounts, and only the one that actually adopted the legacy + /// secret may retire it — deleting a *sibling* account must leave it + /// alone. + /// 4. The legacy item still holds the exact secret that was adopted — if it + /// has since been replaced (someone signed in again on an older release, + /// and that release rewrote the unscoped key), it is a live login of + /// theirs and must survive. /// /// Deleting a synchronizable item propagates everywhere, so a wrong answer /// here logs other devices out. func canDeleteLegacyItem( instanceID: UUID, + accountID: UUID, scope: String, currentLegacyValue: String? ) -> Bool { @@ -111,6 +116,7 @@ final class CredentialAdoptionMarkerStore { $0.instanceID == instanceID && $0.identityScope == scope }) else { return false } + guard marker.accountID == accountID else { return false } guard let currentLegacyValue else { return false } return fingerprint(of: currentLegacyValue) == marker.secretFingerprint diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 0dcc4398..220dac45 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -58,9 +58,18 @@ final class CredentialIdentityScope { private let defaults: UserDefaults private let resolveUserRecordID: @Sendable () async -> String? + /// Written only on the main actor; read in `deinit`, which runs after the + /// last reference is gone and so cannot race with it. + private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol? private static let cacheKey = "credentialIdentityScopeCache" + /// Attempts made for the Apple account currently being resolved. Reset + /// whenever the identity changes. + private var resolutionAttempts = 0 + private var isResolving = false + private static let maxResolutionAttempts = 4 + /// The resolved scope, or `nil` while unresolved. var current: String? { if case let .resolved(scope) = state { return scope } @@ -76,6 +85,37 @@ final class CredentialIdentityScope { self.defaults = defaults self.resolveUserRecordID = resolveUserRecordID adoptCurrentIdentity() + observeIdentityChanges() + } + + deinit { + if let identityChangeObserver { + NotificationCenter.default.removeObserver(identityChangeObserver) + } + } + + /// Re-scopes as soon as the Apple account changes under a running app. + /// + /// Without this the scope would stay pinned to whichever account was + /// signed in at launch, and a sign-out or switch mid-session would let + /// reads and writes reach the previous account's retained credentials — + /// the exact leak scoping exists to prevent. + private func observeIdentityChanges() { + identityChangeObserver = NotificationCenter.default.addObserver( + forName: .NSUbiquityIdentityDidChange, + object: nil, + queue: .main + ) { [weak self] _ in + MainActor.assumeIsolated { + guard let self else { return } + LoggingService.shared.info( + "Apple account changed — re-resolving the credential identity scope", + category: .keychain + ) + self.adoptCurrentIdentity() + Task { await self.resolve() } + } + } } /// Test seam: a scope pinned up front, never consulting CloudKit. @@ -96,6 +136,9 @@ final class CredentialIdentityScope { /// closed shows up here, before any CloudKit request could report it. func adoptCurrentIdentity() { let current = CloudKitIdentity.ubiquityFingerprint + if current != fingerprint { + resolutionAttempts = 0 + } fingerprint = current guard CloudKitAvailability.isAvailable else { @@ -127,34 +170,67 @@ final class CredentialIdentityScope { /// rather than "not signed in" — callers must not treat that as a missing /// credential. func resolve() async { - guard !isResolved else { return } + guard !isResolved, !isResolving else { return } - // Captured before the await so a switch mid-flight can be detected: - // a record ID fetched under account A must never be cached for B. + isResolving = true + defer { isResolving = false } + + // Captured before the first await so a switch mid-flight can be + // detected: a record ID fetched under account A must never be cached + // for B. let requested = fingerprint - guard let recordID = await resolveUserRecordID() else { return } + while resolutionAttempts < Self.maxResolutionAttempts { + resolutionAttempts += 1 - guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else { - LoggingService.shared.info( - "Discarding credential scope resolved for a previous Apple account", - category: .keychain - ) - return + if let recordID = await resolveUserRecordID() { + guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else { + LoggingService.shared.info( + "Discarding credential scope resolved for a previous Apple account", + category: .keychain + ) + return + } + + if let requested { + cache(recordID, for: requested) + } + state = .resolved(recordID) + + LoggingService.shared.info( + "Resolved credential identity scope", + category: .keychain, + details: "fingerprint=\(requested ?? "none"), attempt=\(resolutionAttempts)" + ) + return + } + + guard resolutionAttempts < Self.maxResolutionAttempts else { break } + + try? await Task.sleep(for: .seconds(pow(2.0, Double(resolutionAttempts)))) + guard fingerprint == requested else { return } } - if let requested { - cache(recordID, for: requested) - } - state = .resolved(recordID) - - LoggingService.shared.info( - "Resolved credential identity scope", - category: .keychain, - details: "fingerprint=\(requested ?? "none")" + // Every credential read is disabled until this succeeds, so failing + // quietly would look exactly like "signed out". Foreground activation + // retries via resolveIfNeeded(). + LoggingService.shared.warning( + "Could not resolve the credential identity scope — credentials stay unavailable until it does", + category: .keychain ) } + /// Retries a resolution that previously ran out of attempts. + /// + /// Called when the app returns to the foreground: a device that launched + /// offline would otherwise keep credentials unavailable for the entire + /// process lifetime, long after connectivity returned. + func resolveIfNeeded() async { + guard !isResolved else { return } + resolutionAttempts = 0 + await resolve() + } + // MARK: - Cache private func cachedScope(for fingerprint: String) -> String? { diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 02d83f52..0f80b55e 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -72,6 +72,25 @@ struct KeychainCredentialStore: Sendable { return value } + /// Writes a value **only if the key is currently empty**, returning + /// `errSecDuplicateItem` otherwise. + /// + /// Used by legacy adoption: between rendering the offer and the user + /// tapping it, iCloud Keychain may deliver the real scoped credential, and + /// a replacing write would silently overwrite it with the older secret. + @discardableResult + func addIfAbsent(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: key, + kSecAttrSynchronizable as String: synchronizable, + kSecValueData as String: Data(value.utf8) + ] + + return SecItemAdd(query as CFDictionary, nil) + } + /// Writes a value, replacing any existing item in either synchronizable form. @discardableResult func set(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { diff --git a/Yattee/YatteeApp.swift b/Yattee/YatteeApp.swift index bc7846c8..d68f10cd 100644 --- a/Yattee/YatteeApp.swift +++ b/Yattee/YatteeApp.swift @@ -288,6 +288,12 @@ struct YatteeApp: App { // Refresh media source password status (Keychain state can change in background) appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus() + // Retry a credential identity scope that never resolved (e.g. + // launched offline) - account credentials stay unavailable until it does + Task { + await appEnvironment.credentialIdentityScope.resolveIfNeeded() + } + // Validate subscription account (auto-correct if invalid) appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded() SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment) @@ -335,6 +341,12 @@ struct YatteeApp: App { // Refresh media source password status (Keychain state can change in background) appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus() + // Retry a credential identity scope that never resolved (e.g. + // launched offline) - account credentials stay unavailable until it does + Task { + await appEnvironment.credentialIdentityScope.resolveIfNeeded() + } + // Validate subscription account (auto-correct if invalid) appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded() SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment) diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index a4bbd453..c471dc46 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -253,6 +253,51 @@ struct LegacyCredentialAdoptionTests { identityA.deleteCredential(for: account) } + @Test("Deleting a sibling account leaves the adopter's legacy item alone") + func legacyDeletionRequiresAccountMatch() { + let service = makeService() + let markers = makeMarkers() + let instanceID = UUID() + let adopter = Account.migrated(from: instanceID) + // A second login on the same instance — a fresh UUID, not the migrated one. + let sibling = Account(instanceID: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let credentials = makeCredentials(service: service, scope: "identity-A", markers: markers) + #expect(credentials.adoptLegacyCredential(for: adopter) == true) + + // Removing the sibling must not retire the secret the migrated account + // adopted — old-release devices are still using it. + credentials.deleteLegacyCredentialIfOwned(for: sibling) + #expect(legacyValue(service: service, instanceID: instanceID) == "legacy-secret") + + credentials.deleteLegacyCredentialIfOwned(for: adopter) + #expect(legacyValue(service: service, instanceID: instanceID) == nil) + + credentials.deleteCredential(for: adopter) + } + + @Test("Adoption does not overwrite a scoped credential that arrived first") + func adoptionDoesNotClobberArrivedCredential() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let credentials = makeCredentials(service: service, scope: "identity-A", markers: makeMarkers()) + // iCloud Keychain delivers the real credential between the offer being + // rendered and the user tapping it. + credentials.setCredential("synced-secret", for: account) + + #expect(credentials.hasAdoptableLegacyCredential(for: account) == false) + #expect(credentials.adoptLegacyCredential(for: account) == false) + #expect(credentials.credential(for: account) == "synced-secret") + + credentials.deleteCredential(for: account) + KeychainCredentialStore(service: service) + .delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + @Test("A legacy item replaced by another login survives the adopter's deletion") func legacyDeletionRequiresFingerprintMatch() { let service = makeService()