From 5f2472b6c464d40fc9b5dba6474e8e95e03f3283 Mon Sep 17 00:00:00 2001 From: Pete Date: Sun, 19 May 2024 09:56:10 +0100 Subject: [PATCH 1/4] Add new SecureStore keystore errors --- Sources/SecureStorage/SecureStorageError.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Sources/SecureStorage/SecureStorageError.swift b/Sources/SecureStorage/SecureStorageError.swift index 1d482cf06..ce88bf065 100644 --- a/Sources/SecureStorage/SecureStorageError.swift +++ b/Sources/SecureStorage/SecureStorageError.swift @@ -44,6 +44,8 @@ public enum SecureStorageError: Error { case secError(status: Int32) case generalCryptoError case encodingFailed + case keystoreReadError(status: Int32) + case keystoreUpdateError(status: Int32) } extension SecureStorageError: CustomNSError { @@ -66,6 +68,8 @@ extension SecureStorageError: CustomNSError { case .secError: return 11 case .generalCryptoError: return 12 case .encodingFailed: return 13 + case .keystoreReadError: return 14 + case .keystoreUpdateError: return 15 } } @@ -85,7 +89,7 @@ extension SecureStorageError: CustomNSError { errorUserInfo["SQLiteResultCode"] = NSNumber(value: sqliteError.resultCode.rawValue) errorUserInfo["SQLiteExtendedResultCode"] = NSNumber(value: sqliteError.extendedResultCode.rawValue) } - case .keystoreError(status: let code): + case .keystoreError(status: let code), .keystoreReadError(status: let code), .keystoreUpdateError(status: let code): errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreError", code: Int(code), userInfo: nil) case .secError(status: let code): errorUserInfo["NSUnderlyingError"] = NSError(domain: "secError", code: Int(code), userInfo: nil) From 2dc0f007e0fb7c28fc18eda93a0697e47db70fbb Mon Sep 17 00:00:00 2001 From: Pete Date: Sun, 19 May 2024 10:13:42 +0100 Subject: [PATCH 2/4] Add update method to KeychainService protocol. Add keychainAccessibilityValue to SecureStorageKeyStoreProvider protocol with default implementation. Add AutofillKeystore test --- .../SecureStorageKeyStoreProvider.swift | 12 +++++++++++- .../AutofillKeyStoreProviderTests.swift | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift b/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift index f86c330c8..b8c318a8e 100644 --- a/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift +++ b/Sources/SecureStorage/SecureStorageKeyStoreProvider.swift @@ -23,6 +23,7 @@ public protocol KeychainService { func itemMatching(_ query: [String: Any], _ result: UnsafeMutablePointer?) -> OSStatus func add(_ attributes: [String: Any], _ result: UnsafeMutablePointer?) -> OSStatus func delete(_ query: [String: Any]) -> OSStatus + func update(_ query: [String: Any], _ attributesToUpdate: [String: Any]) -> OSStatus } public extension KeychainService { @@ -37,6 +38,10 @@ public extension KeychainService { func delete(_ query: [String: Any]) -> OSStatus { SecItemDelete(query as CFDictionary) } + + func update(_ query: [String: Any], _ attributesToUpdate: [String: Any]) -> OSStatus { + SecItemUpdate(query as CFDictionary, attributesToUpdate as CFDictionary) + } } public struct DefaultKeychainService: KeychainService { @@ -56,6 +61,7 @@ public protocol SecureStorageKeyStoreProvider { var l1KeyEntryName: String { get } var l2KeyEntryName: String { get } var keychainServiceName: String { get } + var keychainAccessibilityValue: String { get } func storeGeneratedPassword(_ password: Data) throws func generatedPassword() throws -> Data? @@ -79,6 +85,10 @@ public extension SecureStorageKeyStoreProvider { DefaultKeychainService() } + var keychainAccessibilityValue: String { + kSecAttrAccessibleWhenUnlocked as String + } + func generatedPassword() throws -> Data? { return try readData(named: generatedPasswordEntryName, serviceName: keychainServiceName) } @@ -144,7 +154,7 @@ public extension SecureStorageKeyStoreProvider { var query = attributesForEntry(named: name, serviceName: serviceName) query[kSecAttrService as String] = serviceName - query[kSecAttrAccessible as String] = kSecAttrAccessibleWhenUnlocked + query[kSecAttrAccessible as String] = keychainAccessibilityValue query[kSecValueData as String] = base64Data let status = keychainService.add(query, nil) diff --git a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift index ab609054c..f675a6638 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift @@ -128,4 +128,23 @@ final class AutofillKeyStoreProviderTests: XCTestCase { XCTAssertEqual(keychainService.latestAddQuery[kSecAttrService as String] as! String, AutofillKeyStoreProvider.Constants.v3ServiceName) } } + + func testWhenAddData_correctKeychainAccessibilityValueIsUsed() throws { + try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in + // Given + let originalString = "Mock Keychain data!" + let data = originalString.data(using: .utf8)! + let encodedString = data.base64EncodedString() + let mockData = encodedString.data(using: .utf8)! + let keychainService = MockKeychainService() + let sut = AutofillKeyStoreProvider(keychainService: keychainService) + + // When + _ = try sut.writeData(mockData, named: entry.keychainIdentifier, serviceName: sut.keychainServiceName) + + // Then + XCTAssertEqual(keychainService.addCallCount, 1) + XCTAssertEqual(keychainService.latestAddQuery[kSecAttrAccessible as String] as! String, kSecAttrAccessibleWhenUnlocked as String) + } + } } From 5371f01926ae156eebe9a6fd6a72e3df2f178471 Mon Sep 17 00:00:00 2001 From: Pete Date: Sun, 19 May 2024 10:29:14 +0100 Subject: [PATCH 3/4] Rename test to match method being tested --- .../SecureVault/AutofillKeyStoreProviderTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift index f675a6638..89bce30c3 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/AutofillKeyStoreProviderTests.swift @@ -129,7 +129,7 @@ final class AutofillKeyStoreProviderTests: XCTestCase { } } - func testWhenAddData_correctKeychainAccessibilityValueIsUsed() throws { + func testWhenWriteData_correctKeychainAccessibilityValueIsUsed() throws { try AutofillKeyStoreProvider.EntryName.allCases.forEach { entry in // Given let originalString = "Mock Keychain data!" From 92e6183f3a6593db23efdf36461a597bce2578a3 Mon Sep 17 00:00:00 2001 From: Pete Date: Mon, 20 May 2024 12:34:39 +0100 Subject: [PATCH 4/4] Update keystore NSUnderlyingError info --- Sources/SecureStorage/SecureStorageError.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Sources/SecureStorage/SecureStorageError.swift b/Sources/SecureStorage/SecureStorageError.swift index ce88bf065..390e111b4 100644 --- a/Sources/SecureStorage/SecureStorageError.swift +++ b/Sources/SecureStorage/SecureStorageError.swift @@ -89,8 +89,12 @@ extension SecureStorageError: CustomNSError { errorUserInfo["SQLiteResultCode"] = NSNumber(value: sqliteError.resultCode.rawValue) errorUserInfo["SQLiteExtendedResultCode"] = NSNumber(value: sqliteError.extendedResultCode.rawValue) } - case .keystoreError(status: let code), .keystoreReadError(status: let code), .keystoreUpdateError(status: let code): + case .keystoreError(status: let code): errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreError", code: Int(code), userInfo: nil) + case .keystoreReadError(status: let code): + errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreReadError", code: Int(code), userInfo: nil) + case .keystoreUpdateError(status: let code): + errorUserInfo["NSUnderlyingError"] = NSError(domain: "keystoreUpdateError", code: Int(code), userInfo: nil) case .secError(status: let code): errorUserInfo["NSUnderlyingError"] = NSError(domain: "secError", code: Int(code), userInfo: nil) case .authRequired, .invalidPassword, .noL1Key, .noL2Key, .duplicateRecord, .generalCryptoError, .encodingFailed: