Harden Keychain access-group usage: avoid -34018 without entitlements; fallback to no access group on macOS/simulator; retry without group on iOS

bitchat/Services/KeychainManager.swift

@@ -79,38 +79,44 @@ class KeychainManager {
         // Delete any existing item first to ensure clean state
         _ = delete(forKey: key)
         
-        // Build query with all necessary attributes for sandboxed apps
+        // Build base query
-        var query: [String: Any] = [
+        var base: [String: Any] = [
             kSecClass as String: kSecClassGenericPassword,
             kSecAttrAccount as String: key,
             kSecValueData as String: data,
             kSecAttrService as String: service,
-            // Important for sandboxed apps: make it accessible when unlocked
+            kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked,
-            kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked
+            kSecAttrLabel as String: "bitchat-\(key)"
         ]
-        
-        // Add a label for easier debugging
-        query[kSecAttrLabel as String] = "bitchat-\(key)"
-        
-        // Always use app group when available for consistent behavior
-        // This ensures keychain items are properly isolated to our app
-        query[kSecAttrAccessGroup as String] = appGroup
-        
-        // For sandboxed macOS apps, we need to ensure the item is NOT synchronized
         #if os(macOS)
-        query[kSecAttrSynchronizable as String] = false
+        base[kSecAttrSynchronizable as String] = false
         #endif
-        
+
-        let status = SecItemAdd(query as CFDictionary, nil)
+        // Try with access group where it is expected to work (iOS app builds)
-        
+        var triedWithoutGroup = false
-        if status == errSecSuccess {
+        func attempt(addAccessGroup: Bool) -> OSStatus {
-            return true
+            var query = base
-        } else if status == -34018 {
+            if addAccessGroup { query[kSecAttrAccessGroup as String] = appGroup }
+            return SecItemAdd(query as CFDictionary, nil)
+        }
+
+        #if os(iOS)
+        var status = attempt(addAccessGroup: true)
+        if status == -34018 { // Missing entitlement, retry without access group
+            triedWithoutGroup = true
+            status = attempt(addAccessGroup: false)
+        }
+        #else
+        // On macOS dev/simulator default to no access group to avoid -34018
+        let status = attempt(addAccessGroup: false)
+        #endif
+
+        if status == errSecSuccess { return true }
+        if status == -34018 && !triedWithoutGroup {
             SecureLogger.logError(NSError(domain: "Keychain", code: -34018), context: "Missing keychain entitlement", category: SecureLogger.keychain)
         } else if status != errSecDuplicateItem {
             SecureLogger.logError(NSError(domain: "Keychain", code: Int(status)), context: "Error saving to keychain", category: SecureLogger.keychain)
         }
-        
         return false
     }
     
@@ -120,41 +126,56 @@ class KeychainManager {
     }
     
     private func retrieveData(forKey key: String) -> Data? {
-        var query: [String: Any] = [
+        // Base query
+        var base: [String: Any] = [
             kSecClass as String: kSecClassGenericPassword,
             kSecAttrAccount as String: key,
             kSecAttrService as String: service,
             kSecReturnData as String: true,
             kSecMatchLimit as String: kSecMatchLimitOne
         ]
-        
+
-        // Always use app group for consistent behavior
-        query[kSecAttrAccessGroup as String] = appGroup
-        
         var result: AnyObject?
-        let status = SecItemCopyMatching(query as CFDictionary, &result)
+        func attempt(withAccessGroup: Bool) -> OSStatus {
-        
+            var q = base
-        if status == errSecSuccess {
+            if withAccessGroup { q[kSecAttrAccessGroup as String] = appGroup }
-            return result as? Data
+            return SecItemCopyMatching(q as CFDictionary, &result)
-        } else if status == -34018 {
+        }
+
+        #if os(iOS)
+        var status = attempt(withAccessGroup: true)
+        if status == -34018 { status = attempt(withAccessGroup: false) }
+        #else
+        let status = attempt(withAccessGroup: false)
+        #endif
+
+        if status == errSecSuccess { return result as? Data }
+        if status == -34018 {
             SecureLogger.logError(NSError(domain: "Keychain", code: -34018), context: "Missing keychain entitlement", category: SecureLogger.keychain)
         }
-        
         return nil
     }
     
     private func delete(forKey key: String) -> Bool {
-        // Build basic query
+        // Base delete query
-        var query: [String: Any] = [
+        var base: [String: Any] = [
             kSecClass as String: kSecClassGenericPassword,
             kSecAttrAccount as String: key,
             kSecAttrService as String: service
         ]
-        
+
-        // Always use app group for consistent behavior
+        func attempt(withAccessGroup: Bool) -> OSStatus {
-        query[kSecAttrAccessGroup as String] = appGroup
+            var q = base
-        
+            if withAccessGroup { q[kSecAttrAccessGroup as String] = appGroup }
-        let status = SecItemDelete(query as CFDictionary)
+            return SecItemDelete(q as CFDictionary)
+        }
+
+        #if os(iOS)
+        var status = attempt(withAccessGroup: true)
+        if status == -34018 { status = attempt(withAccessGroup: false) }
+        #else
+        let status = attempt(withAccessGroup: false)
+        #endif
         return status == errSecSuccess || status == errSecItemNotFound
     }
     
@@ -313,4 +334,4 @@ class KeychainManager {
         let key = "identity_noiseStaticKey"
         return retrieveData(forKey: key) != nil
     }
-}
+}