SDK-478 keychain migration off main thread, one-shot gated, race-safe reads

CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 
+### Fixed
+- [SDK-478] Gated the keychain migration (`migrateKeychainToIsolatedStorage`) as a one-shot via a UserDefaults flag. Prior to this change the migration ran on the caller's thread on every `start()` invocation; for cross-platform bridges (React Native and Flutter), `start()` is called from `DispatchQueue.main.async`, so the migration's `SecItem` syscalls blocked the main thread on every cold start. The migration now runs only when the flag indicates it has not yet completed for this install, so every subsequent cold start (the hot path) skips the syscall cost entirely. Reads of `email`, `userId`, `userIdUnknownUser`, and `authToken` are coordinated against the migration via a concurrent dispatch queue with a barrier write, so callers always observe a consistent state. See [SDK-294] for context on the original isolated-keychain change.
+- Added empty-key guard and tighter `SecItemCopyMatching` status handling on `KeychainWrapper.data(forKey:)` / `set(_:forKey:)` / `removeValue(forKey:)` to prevent edge-case crashes on app launch.
+- Added duration logging to `IterableKeychain.migrateFromLegacy()` so future regressions of this shape surface in dev logs immediately.
+- Added non-success logging to `KeychainWrapper.set(_:forKey:)` and `update(_:forKey:)` so silent `SecItemAdd`/`SecItemUpdate` failures (e.g. entitlement issues) become observable.
+
 ## [6.7.1]
 ### Fixed
 - Fixed the `enableUnknownUserActivation` default value to `false`

swift-sdk/Core/Constants.swift

@@ -96,6 +96,7 @@ enum Const {
         static let visitorConsentTimestamp = "itbl_visitor_consent_timestamp"
         static let isNotificationsEnabled = "itbl_isNotificationsEnabled"
         static let hasStoredNotificationSetting = "itbl_hasStoredNotificationSetting"
+        static let keychainMigrationCompleted = "itbl_keychain_migration_completed"
 
         static let attributionInfoExpiration = 24
     }

swift-sdk/Internal/InternalIterableAPI.swift

@@ -1101,10 +1101,18 @@ final class InternalIterableAPI: NSObject, PushTrackerProtocol, AuthProvider {
     }
 
     private func updateSDKVersion() {
-        // Always attempt keychain migration to handle uninstall/reinstall scenario
-        // where UserDefaults are cleared but keychain persists
-        localStorage.migrateKeychainToIsolatedStorage()
-
+        // Keychain migration is one-shot per install. We gate on a
+        // UserDefaults flag so every subsequent cold start (the hot path)
+        // skips the SecItem syscall cost entirely. On the rare first launch
+        // after an upgrade from a pre-isolated-keychain build we run the
+        // migration synchronously on the calling thread, because
+        // retrieveIdentifierData() runs immediately after this and must
+        // observe the migrated identity. See SDK-478.
+        if !localStorage.keychainMigrationCompleted {
+            localStorage.migrateKeychainToIsolatedStorage()
+            localStorage.keychainMigrationCompleted = true
+        }
+
         if let lastVersion = localStorage.sdkVersion, lastVersion != IterableAPI.sdkVersion {
             performUpgrade(lastVersion: lastVersion, newVersion: IterableAPI.sdkVersion)
         } else {

swift-sdk/Internal/IterableUserDefaults.swift

@@ -177,7 +177,15 @@ class IterableUserDefaults {
             save(bool: newValue, withKey: .hasStoredNotificationSetting)
         }
     }
-
+
+    var keychainMigrationCompleted: Bool {
+        get {
+            bool(withKey: .keychainMigrationCompleted)
+        } set {
+            save(bool: newValue, withKey: .keychainMigrationCompleted)
+        }
+    }
+
     func getAttributionInfo(currentDate: Date) -> IterableAttributionInfo? {
         (try? codable(withKey: .attributionInfo, currentDate: currentDate)) ?? nil
     }
@@ -355,6 +363,7 @@ class IterableUserDefaults {
 
         static let isNotificationsEnabled = UserDefaultsKey(value: Const.UserDefault.isNotificationsEnabled)
         static let hasStoredNotificationSetting = UserDefaultsKey(value: Const.UserDefault.hasStoredNotificationSetting)
+        static let keychainMigrationCompleted = UserDefaultsKey(value: Const.UserDefault.keychainMigrationCompleted)
     }
     private struct Envelope: Codable {
         let payload: Data

swift-sdk/Internal/Utilities/Keychain/IterableKeychain.swift

@@ -14,131 +14,134 @@ class IterableKeychain {
     /// Migrates keychain data from legacy storage to isolated storage
     /// This is needed when upgrading from older SDK versions that used a shared service name
     /// - Returns: true if migration was performed, false otherwise
+    ///
+    /// Runs as a barrier on `coordinatorQueue` so it blocks until in-flight
+    /// reads finish, then blocks subsequent reads / writes until the barrier
+    /// returns. After it returns, reads run fully in parallel. Uses raw
+    /// `wrapper`/`legacyWrapper` access internally to avoid re-entering the
+    /// queue (the public properties also go through the queue and would
+    /// deadlock if used from inside the barrier).
     @discardableResult
     func migrateFromLegacy() -> Bool {
         guard let legacyWrapper = legacyWrapper else {
             return false
         }
 
-        var migrated = false
+        return coordinatorQueue.sync(flags: .barrier) {
+            if migrationAttempted { return false }
+            migrationAttempted = true
 
-        // Migrate email if isolated keychain is empty and legacy has data
-        if email == nil, let legacyEmail = getString(forKey: Const.Keychain.Key.email, from: legacyWrapper) {
-            email = legacyEmail
-            legacyWrapper.removeValue(forKey: Const.Keychain.Key.email)
-            ITBInfo("UPDATED: migrated email from legacy keychain to isolated keychain")
-            migrated = true
-        }
+            var migrated = false
+            let migrationStartedAt = Date()
 
-        // Migrate userId if isolated keychain is empty and legacy has data
-        if userId == nil, let legacyUserId = getString(forKey: Const.Keychain.Key.userId, from: legacyWrapper) {
-            userId = legacyUserId
-            legacyWrapper.removeValue(forKey: Const.Keychain.Key.userId)
-            ITBInfo("UPDATED: migrated userId from legacy keychain to isolated keychain")
-            migrated = true
-        }
+            if rawString(forKey: Const.Keychain.Key.email, from: wrapper) == nil,
+               let legacyEmail = rawString(forKey: Const.Keychain.Key.email, from: legacyWrapper) {
+                rawWrite(string: legacyEmail, forKey: Const.Keychain.Key.email, to: wrapper)
+                legacyWrapper.removeValue(forKey: Const.Keychain.Key.email)
+                ITBInfo("UPDATED: migrated email from legacy keychain to isolated keychain")
+                migrated = true
+            }
 
-        // Migrate userIdUnknownUser if isolated keychain is empty and legacy has data
-        if userIdUnknownUser == nil, let legacyUserIdUnknownUser = getString(forKey: Const.Keychain.Key.userIdUnknownUser, from: legacyWrapper) {
-            userIdUnknownUser = legacyUserIdUnknownUser
-            legacyWrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser)
-            ITBInfo("UPDATED: migrated userIdUnknownUser from legacy keychain to isolated keychain")
-            migrated = true
-        }
+            if rawString(forKey: Const.Keychain.Key.userId, from: wrapper) == nil,
+               let legacyUserId = rawString(forKey: Const.Keychain.Key.userId, from: legacyWrapper) {
+                rawWrite(string: legacyUserId, forKey: Const.Keychain.Key.userId, to: wrapper)
+                legacyWrapper.removeValue(forKey: Const.Keychain.Key.userId)
+                ITBInfo("UPDATED: migrated userId from legacy keychain to isolated keychain")
+                migrated = true
+            }
 
-        // Migrate authToken if isolated keychain is empty and legacy has data
-        if authToken == nil, let legacyAuthToken = getString(forKey: Const.Keychain.Key.authToken, from: legacyWrapper) {
-            authToken = legacyAuthToken
-            legacyWrapper.removeValue(forKey: Const.Keychain.Key.authToken)
-            ITBInfo("UPDATED: migrated authToken from legacy keychain to isolated keychain")
-            migrated = true
-        }
+            if rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: wrapper) == nil,
+               let legacyUserIdUnknownUser = rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: legacyWrapper) {
+                rawWrite(string: legacyUserIdUnknownUser, forKey: Const.Keychain.Key.userIdUnknownUser, to: wrapper)
+                legacyWrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser)
+                ITBInfo("UPDATED: migrated userIdUnknownUser from legacy keychain to isolated keychain")
+                migrated = true
+            }
 
-        return migrated
-    }
+            if rawString(forKey: Const.Keychain.Key.authToken, from: wrapper) == nil,
+               let legacyAuthToken = rawString(forKey: Const.Keychain.Key.authToken, from: legacyWrapper) {
+                rawWrite(string: legacyAuthToken, forKey: Const.Keychain.Key.authToken, to: wrapper)
+                legacyWrapper.removeValue(forKey: Const.Keychain.Key.authToken)
+                ITBInfo("UPDATED: migrated authToken from legacy keychain to isolated keychain")
+                migrated = true
+            }
 
-    /// Helper to get string from a specific wrapper
-    private func getString(forKey key: String, from wrapper: KeychainWrapper) -> String? {
-        let data = wrapper.data(forKey: key)
-        return data.flatMap { String(data: $0, encoding: .utf8) }
+            let elapsedMs = Int(Date().timeIntervalSince(migrationStartedAt) * 1000)
+            ITBInfo("keychain migrateFromLegacy completed in \(elapsedMs)ms migrated=\(migrated)")
+
+            return migrated
+        }
     }
-    
+
     var email: String? {
-        get {
-            let data = wrapper.data(forKey: Const.Keychain.Key.email)
-
-            return data.flatMap { String(data: $0, encoding: .utf8) }
-        }
-
+        get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.email, from: wrapper) } }
         set {
-            guard let token = newValue,
-                  let data = token.data(using: .utf8) else {
-                wrapper.removeValue(forKey: Const.Keychain.Key.email)
-                return
+            coordinatorQueue.sync(flags: .barrier) {
+                rawWrite(string: newValue, forKey: Const.Keychain.Key.email, to: wrapper)
             }
-
-            wrapper.set(data, forKey: Const.Keychain.Key.email)
         }
     }
-    
+
     var userId: String? {
-        get {
-            let data = wrapper.data(forKey: Const.Keychain.Key.userId)
-
-            return data.flatMap { String(data: $0, encoding: .utf8) }
-        }
-
+        get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.userId, from: wrapper) } }
         set {
-            guard let token = newValue,
-                  let data = token.data(using: .utf8) else {
-                wrapper.removeValue(forKey: Const.Keychain.Key.userId)
-                return
+            coordinatorQueue.sync(flags: .barrier) {
+                rawWrite(string: newValue, forKey: Const.Keychain.Key.userId, to: wrapper)
             }
-
-            wrapper.set(data, forKey: Const.Keychain.Key.userId)
         }
     }
-    
+
     var userIdUnknownUser: String? {
-        get {
-            let data = wrapper.data(forKey: Const.Keychain.Key.userIdUnknownUser)
-
-            return data.flatMap { String(data: $0, encoding: .utf8) }
-        }
-
+        get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.userIdUnknownUser, from: wrapper) } }
         set {
-            guard let token = newValue,
-                  let data = token.data(using: .utf8) else {
-                wrapper.removeValue(forKey: Const.Keychain.Key.userIdUnknownUser)
-                return
+            coordinatorQueue.sync(flags: .barrier) {
+                rawWrite(string: newValue, forKey: Const.Keychain.Key.userIdUnknownUser, to: wrapper)
             }
-
-            wrapper.set(data, forKey: Const.Keychain.Key.userIdUnknownUser)
         }
     }
-    
+
     var authToken: String? {
-        get {
-            let data = wrapper.data(forKey: Const.Keychain.Key.authToken)
-
-            return data.flatMap { String(data: $0, encoding: .utf8) }
-        }
-
+        get { coordinatorQueue.sync { rawString(forKey: Const.Keychain.Key.authToken, from: wrapper) } }
         set {
-            guard let token = newValue,
-                  let data = token.data(using: .utf8) else {
-                wrapper.removeValue(forKey: Const.Keychain.Key.authToken)
-                return
+            coordinatorQueue.sync(flags: .barrier) {
+                rawWrite(string: newValue, forKey: Const.Keychain.Key.authToken, to: wrapper)
             }
-
-            wrapper.set(data, forKey: Const.Keychain.Key.authToken)
         }
     }
-    
+
     // MARK: - PRIVATE/INTERNAL
 
     private let wrapper: KeychainWrapper
     private let legacyWrapper: KeychainWrapper?
+
+    /// Guards against re-running the migration body within a single
+    /// process lifetime if `migrateFromLegacy()` is invoked more than once
+    /// (e.g. a second `start()` call before the UserDefaults flag is
+    /// observed). Read/write only inside the `coordinatorQueue` barrier.
+    private var migrationAttempted = false
+
+    /// Concurrent queue used to serialize the migration (which runs as a
+    /// barrier) against the property reads/writes. Reads run fully in
+    /// parallel after migration completes. See SDK-478.
+    private let coordinatorQueue = DispatchQueue(label: "com.iterable.IterableKeychain.coordinator",
+                                                 attributes: .concurrent)
+
+    /// Raw read - bypasses the coordinator queue. Only safe to call from
+    /// inside a `coordinatorQueue.sync { }` block.
+    private func rawString(forKey key: String, from wrapper: KeychainWrapper) -> String? {
+        let data = wrapper.data(forKey: key)
+        return data.flatMap { String(data: $0, encoding: .utf8) }
+    }
+
+    /// Raw write - bypasses the coordinator queue. Only safe to call from
+    /// inside a `coordinatorQueue.sync(flags: .barrier) { }` block.
+    private func rawWrite(string: String?, forKey key: String, to wrapper: KeychainWrapper) {
+        guard let value = string, let data = value.data(using: .utf8) else {
+            wrapper.removeValue(forKey: key)
+            return
+        }
+        wrapper.set(data, forKey: key)
+    }
 
     private func encodeJsonPayload(_ json: [AnyHashable: Any]?) -> Data? {
         guard let json = json, JSONSerialization.isValidJSONObject(json) else {

swift-sdk/Internal/Utilities/Keychain/KeychainWrapper.swift

@@ -30,47 +30,65 @@ class KeychainWrapper {
 
     @discardableResult
     func set(_ value: Data, forKey key: String) -> Bool {
+        guard !key.isEmpty else { return false }
+
         var keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key)
-        
+
         keychainQueryDictionary[SecValueData] = value
-        
+
         // Assign default protection - Protect the keychain entry so it's only valid when the device is unlocked
         keychainQueryDictionary[SecAttrAccessible] = SecAttrAccessibleWhenUnlocked
-        
+
         let status: OSStatus = SecItemAdd(keychainQueryDictionary as CFDictionary, nil)
-        
+
         if status == errSecSuccess {
             return true
         } else if status == errSecDuplicateItem {
             return update(value, forKey: key)
         } else {
+            // Surface silent failure paths (e.g. errSecMissingEntitlement in
+            // misconfigured Expo apps) so they become observable in dev logs.
+            // See SDK-478.
+            ITBError("keychain SecItemAdd failed for key=\(key) status=\(status)")
             return false
         }
     }
-    
+
     func data(forKey key: String) -> Data? {
+        guard !key.isEmpty else { return nil }
+
         var keychainQueryDictionary = setupKeychainQueryDictionary(forKey: key)
-        
+
         // Limit search results to one
         keychainQueryDictionary[SecMatchLimit] = SecMatchLimitOne
-        
+
         // Specify we want Data/CFData returned
         keychainQueryDictionary[SecReturnData] = CFBooleanTrue
-        
+
         // Search
         var result: AnyObject?
         let status = SecItemCopyMatching(keychainQueryDictionary as CFDictionary, &result)
-
-        return status == noErr ? result as? Data : nil
+
+        // Treat anything other than noErr / errSecItemNotFound as a benign
+        // miss rather than passing weird statuses through; this prevents
+        // downstream crashes when callers force-unwrap the returned data.
+        // See SDK-478 / b521d533.
+        guard status == noErr || status == errSecItemNotFound else {
+            return nil
+        }
+
+        return result as? Data
     }
-    
+
     @discardableResult
     func removeValue(forKey key: String) -> Bool {
+        guard !key.isEmpty else { return false }
+
         let keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key)
-        
+
         // Delete
         let status: OSStatus = SecItemDelete(keychainQueryDictionary as CFDictionary)
-        
+
         return status == errSecSuccess
     }
 
@@ -109,10 +127,14 @@ class KeychainWrapper {
     private func update(_ value: Data, forKey key: String) -> Bool {
         let keychainQueryDictionary: [String: Any] = setupKeychainQueryDictionary(forKey: key)
         let updateDictionary = [SecValueData: value]
-        
+
         // Update
         let status: OSStatus = SecItemUpdate(keychainQueryDictionary as CFDictionary, updateDictionary as CFDictionary)
-
+
+        if status != errSecSuccess {
+            ITBError("keychain SecItemUpdate failed for key=\(key) status=\(status)")
+        }
+
         return status == errSecSuccess
     }