fix(security): improve keychain error handling [BCH-01-009]

Add proper error classification to distinguish expected states (item not found) from critical failures (access denied, storage full) and recoverable errors (device locked, auth failed). Changes: - Add KeychainReadResult and KeychainSaveResult enums with error types - Add getIdentityKeyWithResult/saveIdentityKeyWithResult protocol methods - Implement retry logic with exponential backoff for transient errors - Add consistent SecureLogger logging for all keychain operations - Update NoiseEncryptionService identity loading to handle errors gracefully - Update MockKeychain and TrackingMockKeychain with error simulation - Add comprehensive KeychainErrorHandlingTests (18 new tests) Security: Applications can now properly detect and respond to critical keychain failures, improving resilience during device lock states and providing actionable diagnostics for access issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2026-07-26 06:05:20 +00:00 · 2026-01-12 10:53:02 -10:00
parent b84c36c6fa
commit 31be6b83a7
5 changed files with 623 additions and 38 deletions
@@ -10,6 +10,47 @@ import BitLogger
 import Foundation
 import Security

+// MARK: - Keychain Error Types
+// BCH-01-009: Proper error classification to distinguish expected states from critical failures
+
+/// Result of a keychain read operation with proper error classification
+enum KeychainReadResult {
+    case success(Data)
+    case itemNotFound        // Expected: key doesn't exist yet
+    case accessDenied        // Critical: app lacks keychain access
+    case deviceLocked        // Recoverable: device is locked
+    case authenticationFailed // Recoverable: biometric/passcode failed
+    case otherError(OSStatus) // Unexpected error
+
+    var isRecoverableError: Bool {
+        switch self {
+        case .deviceLocked, .authenticationFailed:
+            return true
+        default:
+            return false
+        }
+    }
+}
+
+/// Result of a keychain save operation with proper error classification
+enum KeychainSaveResult {
+    case success
+    case duplicateItem       // Can retry with update
+    case accessDenied        // Critical: app lacks keychain access
+    case deviceLocked        // Recoverable: device is locked
+    case storageFull         // Critical: no space available
+    case otherError(OSStatus)
+
+    var isRecoverableError: Bool {
+        switch self {
+        case .duplicateItem, .deviceLocked:
+            return true
+        default:
+            return false
+        }
+    }
+}
+
 protocol KeychainManagerProtocol {
    func saveIdentityKey(_ keyData: Data, forKey key: String) -> Bool
    func getIdentityKey(forKey key: String) -> Data?
@@ -21,6 +62,12 @@ protocol KeychainManagerProtocol {

    func verifyIdentityKeyExists() -> Bool

+    // BCH-01-009: Methods with proper error classification
+    /// Get identity key with detailed result for error handling
+    func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult
+    /// Save identity key with detailed result for error handling
+    func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult
+
    // MARK: - Generic Data Storage (consolidated from KeychainHelper)
    /// Save data with a custom service name
    func save(key: String, data: Data, service: String, accessible: CFString?)
@@ -54,7 +101,181 @@ final class KeychainManager: KeychainManagerProtocol {
        SecureLogger.logKeyOperation(.delete, keyType: key, success: result)
        return result
    }
-    
+
+    // MARK: - BCH-01-009: Methods with Proper Error Classification
+
+    /// Get identity key with detailed result for proper error handling
+    /// Distinguishes between missing keys (expected) and critical failures
+    func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult {
+        let fullKey = "identity_\(key)"
+        return retrieveDataWithResult(forKey: fullKey)
+    }
+
+    /// Save identity key with detailed result and retry logic for transient errors
+    func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult {
+        let fullKey = "identity_\(key)"
+        return saveDataWithResult(keyData, forKey: fullKey)
+    }
+
+    /// Internal method to save data with detailed result and retry for transient errors
+    private func saveDataWithResult(_ data: Data, forKey key: String, retryCount: Int = 2) -> KeychainSaveResult {
+        // Delete any existing item first to ensure clean state
+        _ = delete(forKey: key)
+
+        // Build base query
+        var base: [String: Any] = [
+            kSecClass as String: kSecClassGenericPassword,
+            kSecAttrAccount as String: key,
+            kSecValueData as String: data,
+            kSecAttrService as String: service,
+            kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked,
+            kSecAttrLabel as String: "bitchat-\(key)"
+        ]
+        #if os(macOS)
+        base[kSecAttrSynchronizable as String] = false
+        #endif
+
+        func attempt(addAccessGroup: Bool) -> OSStatus {
+            var query = base
+            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
+            status = attempt(addAccessGroup: false)
+        }
+        #else
+        let status = attempt(addAccessGroup: false)
+        #endif
+
+        // Classify the result
+        let result = classifySaveStatus(status)
+
+        // Log all outcomes consistently
+        switch result {
+        case .success:
+            SecureLogger.debug("Keychain save succeeded for key: \(key)", category: .keychain)
+        case .duplicateItem:
+            SecureLogger.warning("Keychain save found duplicate for key: \(key)", category: .keychain)
+        case .accessDenied:
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(status)),
+                               context: "Keychain access denied for key: \(key)", category: .keychain)
+        case .deviceLocked:
+            SecureLogger.warning("Device locked during keychain save for key: \(key)", category: .keychain)
+        case .storageFull:
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(status)),
+                               context: "Keychain storage full for key: \(key)", category: .keychain)
+        case .otherError(let code):
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(code)),
+                               context: "Keychain save failed for key: \(key)", category: .keychain)
+        }
+
+        // Retry transient errors with exponential backoff
+        if result.isRecoverableError && retryCount > 0 {
+            let delayMs = UInt32((3 - retryCount) * 100) // 100ms, 200ms backoff
+            usleep(delayMs * 1000)
+            SecureLogger.debug("Retrying keychain save for key: \(key), attempts remaining: \(retryCount)", category: .keychain)
+            return saveDataWithResult(data, forKey: key, retryCount: retryCount - 1)
+        }
+
+        return result
+    }
+
+    /// Internal method to retrieve data with detailed result
+    private func retrieveDataWithResult(forKey key: String) -> KeychainReadResult {
+        let base: [String: Any] = [
+            kSecClass as String: kSecClassGenericPassword,
+            kSecAttrAccount as String: key,
+            kSecAttrService as String: service,
+            kSecReturnData as String: true,
+            kSecMatchLimit as String: kSecMatchLimitOne
+        ]
+
+        var result: AnyObject?
+        func attempt(withAccessGroup: Bool) -> OSStatus {
+            var q = base
+            if withAccessGroup { q[kSecAttrAccessGroup as String] = appGroup }
+            return SecItemCopyMatching(q as CFDictionary, &result)
+        }
+
+        #if os(iOS)
+        var status = attempt(withAccessGroup: true)
+        if status == -34018 { status = attempt(withAccessGroup: false) }
+        #else
+        let status = attempt(withAccessGroup: false)
+        #endif
+
+        // Classify the result
+        let readResult = classifyReadStatus(status, data: result as? Data)
+
+        // Log all outcomes consistently
+        switch readResult {
+        case .success:
+            SecureLogger.debug("Keychain read succeeded for key: \(key)", category: .keychain)
+        case .itemNotFound:
+            // Expected case - no logging needed for missing keys
+            break
+        case .accessDenied:
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(status)),
+                               context: "Keychain access denied for key: \(key)", category: .keychain)
+        case .deviceLocked:
+            SecureLogger.warning("Device locked during keychain read for key: \(key)", category: .keychain)
+        case .authenticationFailed:
+            SecureLogger.warning("Authentication failed for keychain read of key: \(key)", category: .keychain)
+        case .otherError(let code):
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(code)),
+                               context: "Keychain read failed for key: \(key)", category: .keychain)
+        }
+
+        return readResult
+    }
+
+    /// Classify keychain read status into meaningful categories
+    private func classifyReadStatus(_ status: OSStatus, data: Data?) -> KeychainReadResult {
+        switch status {
+        case errSecSuccess:
+            if let data = data {
+                return .success(data)
+            }
+            return .otherError(status)
+        case errSecItemNotFound:
+            return .itemNotFound
+        case errSecInteractionNotAllowed:
+            // Device is locked or in a state that doesn't allow keychain access
+            return .deviceLocked
+        case errSecAuthFailed:
+            return .authenticationFailed
+        case -34018: // errSecMissingEntitlement
+            return .accessDenied
+        case errSecNotAvailable:
+            return .accessDenied
+        default:
+            return .otherError(status)
+        }
+    }
+
+    /// Classify keychain save status into meaningful categories
+    private func classifySaveStatus(_ status: OSStatus) -> KeychainSaveResult {
+        switch status {
+        case errSecSuccess:
+            return .success
+        case errSecDuplicateItem:
+            return .duplicateItem
+        case errSecInteractionNotAllowed:
+            return .deviceLocked
+        case -34018: // errSecMissingEntitlement
+            return .accessDenied
+        case errSecNotAvailable:
+            return .accessDenied
+        case errSecDiskFull:
+            return .storageFull
+        default:
+            return .otherError(status)
+        }
+    }
+
    // MARK: - Generic Operations
    
    private func save(_ value: String, forKey key: String) -> Bool {
@@ -199,64 +199,153 @@ final class NoiseEncryptionService {
    
    init(keychain: KeychainManagerProtocol) {
        self.keychain = keychain
-        
-        // Load or create static identity key (ONLY from keychain)
+
+        // BCH-01-009: Load or create static identity key with proper error handling
        let loadedKey: Curve25519.KeyAgreement.PrivateKey
-        
-        // Try to load from keychain
-        if let identityData = keychain.getIdentityKey(forKey: "noiseStaticKey"),
-           let key = try? Curve25519.KeyAgreement.PrivateKey(rawRepresentation: identityData) {
-            loadedKey = key
-            SecureLogger.logKeyOperation(.load, keyType: "noiseStaticKey", success: true)
-        }
-        // If no identity exists, create new one
-        else {
+
+        // Try to load from keychain with proper error classification
+        let noiseKeyResult = keychain.getIdentityKeyWithResult(forKey: "noiseStaticKey")
+
+        switch noiseKeyResult {
+        case .success(let identityData):
+            if let key = try? Curve25519.KeyAgreement.PrivateKey(rawRepresentation: identityData) {
+                loadedKey = key
+                SecureLogger.logKeyOperation(.load, keyType: "noiseStaticKey", success: true)
+            } else {
+                // Data corrupted, regenerate
+                SecureLogger.warning("Noise static key data corrupted, regenerating", category: .keychain)
+                loadedKey = Self.generateAndSaveNoiseKey(keychain: keychain)
+            }
+
+        case .itemNotFound:
+            // Expected case: no key exists yet, create new one
+            loadedKey = Self.generateAndSaveNoiseKey(keychain: keychain)
+
+        case .accessDenied:
+            // Critical error - log but proceed with ephemeral key (will be lost on restart)
+            SecureLogger.error(NSError(domain: "Keychain", code: -1),
+                               context: "Keychain access denied - using ephemeral identity", category: .keychain)
+            loadedKey = Curve25519.KeyAgreement.PrivateKey()
+
+        case .deviceLocked, .authenticationFailed:
+            // Recoverable error - use ephemeral key and warn
+            SecureLogger.warning("Device locked or auth failed - using ephemeral identity until unlocked", category: .keychain)
+            loadedKey = Curve25519.KeyAgreement.PrivateKey()
+
+        case .otherError(let status):
+            // Unexpected error - log and use ephemeral key
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(status)),
+                               context: "Unexpected keychain error - using ephemeral identity", category: .keychain)
            loadedKey = Curve25519.KeyAgreement.PrivateKey()
-            let keyData = loadedKey.rawRepresentation
-            
-            // Save to keychain
-            let saved = keychain.saveIdentityKey(keyData, forKey: "noiseStaticKey")
-            SecureLogger.logKeyOperation(.create, keyType: "noiseStaticKey", success: saved)
        }
-        
+
        // Now assign the final value
        self.staticIdentityKey = loadedKey
        self.staticIdentityPublicKey = staticIdentityKey.publicKey
-        
-        // Load or create signing key pair
+
+        // BCH-01-009: Load or create signing key pair with proper error handling
        let loadedSigningKey: Curve25519.Signing.PrivateKey
-        
-        // Try to load from keychain
-        if let signingData = keychain.getIdentityKey(forKey: "ed25519SigningKey"),
-           let key = try? Curve25519.Signing.PrivateKey(rawRepresentation: signingData) {
-            loadedSigningKey = key
-            SecureLogger.logKeyOperation(.load, keyType: "ed25519SigningKey", success: true)
-        }
-        // If no signing key exists, create new one
-        else {
+
+        let signingKeyResult = keychain.getIdentityKeyWithResult(forKey: "ed25519SigningKey")
+
+        switch signingKeyResult {
+        case .success(let signingData):
+            if let key = try? Curve25519.Signing.PrivateKey(rawRepresentation: signingData) {
+                loadedSigningKey = key
+                SecureLogger.logKeyOperation(.load, keyType: "ed25519SigningKey", success: true)
+            } else {
+                // Data corrupted, regenerate
+                SecureLogger.warning("Ed25519 signing key data corrupted, regenerating", category: .keychain)
+                loadedSigningKey = Self.generateAndSaveSigningKey(keychain: keychain)
+            }
+
+        case .itemNotFound:
+            // Expected case: no key exists yet, create new one
+            loadedSigningKey = Self.generateAndSaveSigningKey(keychain: keychain)
+
+        case .accessDenied:
+            // Critical error - log but proceed with ephemeral key
+            SecureLogger.error(NSError(domain: "Keychain", code: -1),
+                               context: "Keychain access denied - using ephemeral signing key", category: .keychain)
+            loadedSigningKey = Curve25519.Signing.PrivateKey()
+
+        case .deviceLocked, .authenticationFailed:
+            // Recoverable error - use ephemeral key and warn
+            SecureLogger.warning("Device locked or auth failed - using ephemeral signing key until unlocked", category: .keychain)
+            loadedSigningKey = Curve25519.Signing.PrivateKey()
+
+        case .otherError(let status):
+            // Unexpected error - log and use ephemeral key
+            SecureLogger.error(NSError(domain: "Keychain", code: Int(status)),
+                               context: "Unexpected keychain error - using ephemeral signing key", category: .keychain)
            loadedSigningKey = Curve25519.Signing.PrivateKey()
-            let keyData = loadedSigningKey.rawRepresentation
-            
-            // Save to keychain
-            let saved = keychain.saveIdentityKey(keyData, forKey: "ed25519SigningKey")
-            SecureLogger.logKeyOperation(.create, keyType: "ed25519SigningKey", success: saved)
        }
-        
+
        // Now assign the signing keys
        self.signingKey = loadedSigningKey
        self.signingPublicKey = signingKey.publicKey
-        
+
        // Initialize session manager
        self.sessionManager = NoiseSessionManager(localStaticKey: staticIdentityKey, keychain: keychain)
-        
+
        // Set up session callbacks
        sessionManager.onSessionEstablished = { [weak self] peerID, remoteStaticKey in
            self?.handleSessionEstablished(peerID: peerID, remoteStaticKey: remoteStaticKey)
        }
-        
+
        // Start session maintenance timer
        startRekeyTimer()
    }
+
+    // MARK: - BCH-01-009: Key Generation Helpers with Save Verification
+
+    /// Generate and save a new Noise static key, verifying the save succeeds
+    private static func generateAndSaveNoiseKey(keychain: KeychainManagerProtocol) -> Curve25519.KeyAgreement.PrivateKey {
+        let newKey = Curve25519.KeyAgreement.PrivateKey()
+        let keyData = newKey.rawRepresentation
+
+        // Save to keychain and verify success
+        let saveResult = keychain.saveIdentityKeyWithResult(keyData, forKey: "noiseStaticKey")
+
+        switch saveResult {
+        case .success:
+            SecureLogger.logKeyOperation(.create, keyType: "noiseStaticKey", success: true)
+        case .duplicateItem:
+            // This shouldn't happen since we just tried to load, but handle it
+            SecureLogger.warning("Noise key already exists (race condition?)", category: .keychain)
+        default:
+            // Save failed - log but continue with the key (it will be ephemeral)
+            SecureLogger.error(NSError(domain: "Keychain", code: -1),
+                               context: "Failed to persist noise static key - identity will be lost on restart",
+                               category: .keychain)
+        }
+
+        return newKey
+    }
+
+    /// Generate and save a new Ed25519 signing key, verifying the save succeeds
+    private static func generateAndSaveSigningKey(keychain: KeychainManagerProtocol) -> Curve25519.Signing.PrivateKey {
+        let newKey = Curve25519.Signing.PrivateKey()
+        let keyData = newKey.rawRepresentation
+
+        // Save to keychain and verify success
+        let saveResult = keychain.saveIdentityKeyWithResult(keyData, forKey: "ed25519SigningKey")
+
+        switch saveResult {
+        case .success:
+            SecureLogger.logKeyOperation(.create, keyType: "ed25519SigningKey", success: true)
+        case .duplicateItem:
+            // This shouldn't happen since we just tried to load, but handle it
+            SecureLogger.warning("Signing key already exists (race condition?)", category: .keychain)
+        default:
+            // Save failed - log but continue with the key (it will be ephemeral)
+            SecureLogger.error(NSError(domain: "Keychain", code: -1),
+                               context: "Failed to persist signing key - identity will be lost on restart",
+                               category: .keychain)
+        }
+
+        return newKey
+    }
    
    // MARK: - Public Interface
    
@@ -41,6 +41,19 @@ final class PreviewKeychainManager: KeychainManagerProtocol {
        storage["identity_noiseStaticKey"] != nil
    }

+    // BCH-01-009: New methods with proper error classification
+    func getIdentityKeyWithResult(forKey key: String) -> KeychainReadResult {
+        if let data = storage[key] {
+            return .success(data)
+        }
+        return .itemNotFound
+    }
+
+    func saveIdentityKeyWithResult(_ keyData: Data, forKey key: String) -> KeychainSaveResult {
+        storage[key] = keyData
+        return .success
+    }
+
    // MARK: - Generic Data Storage (consolidated from KeychainHelper)

    func save(key: String, data: Data, service: String, accessible: CFString?) {