diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b78ba44..ea3c1790 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ ## [Unreleased] +#### Fixed +- Fixed permanent authentication failure after iCloud device migration caused by Secure Enclave key mismatch [SDKS-5172] + #### Added - Routed FIDO ceremony logs through the workflow logger by adding a `logger:` parameter to `Fido.register` and `Fido.authenticate`. The DaVinci collectors and Journey callbacks pass the workflow's configured logger so FIDO ceremony state transitions and errors emit through the same logger as the surrounding flow [SDKS-4924] diff --git a/Oidc/Oidc/OidcClient.swift b/Oidc/Oidc/OidcClient.swift index 0f643d1f..5fc8e2f4 100644 --- a/Oidc/Oidc/OidcClient.swift +++ b/Oidc/Oidc/OidcClient.swift @@ -11,8 +11,9 @@ import Foundation import PingLogger -import PingOrchestrate import PingNetwork +import PingOrchestrate +import PingStorage /// Class representing an OpenID Connect client. /// - Property pkce: PKCE object used for the Authorization call. @@ -115,23 +116,44 @@ public class OidcClient { config.logger.i("Getting access token") do { - if let cached = try await config.storage.get() { - if !cached.isExpired(threshold: config.refreshThreshold) { - config.logger.i("Token is not expired. Returning cached token.") - return .success(cached) - } - config.logger.i("Token is expired. Attempting to refresh.") - if let cachedefreshToken = cached.refreshToken { - do { - let refreshedToken = try await refreshToken(cachedefreshToken) - return .success(refreshedToken) - } catch { - config.logger.e("Failed to refresh token. Revoking token and re-authenticating.", error: error) - await revoke(cached) + do { + if let cached = try await config.storage.get() { + if !cached.isExpired(threshold: config.refreshThreshold) { + config.logger.i("Token is not expired. Returning cached token.") + return .success(cached) } + config.logger.i("Token is expired. Attempting to refresh.") + if let cachedefreshToken = cached.refreshToken { + do { + let refreshedToken = try await refreshToken(cachedefreshToken) + return .success(refreshedToken) + } catch { + config.logger.e("Failed to refresh token. Revoking token and re-authenticating.", error: error) + await revoke(cached) + } + } + } + } catch let error where error is EncryptorError || error is DecodingError { + // The cached bytes are unreadable — either the Secure Enclave key did not + // migrate with an iCloud/Quick Start device transfer (EncryptorError), or the + // stored payload is corrupt / from an incompatible version (DecodingError). + // Clear the unreadable token and fall through to re-authenticate. Other errors + // (e.g. errSecInteractionNotAllowed while the device is locked) are transient + // and must NOT delete a potentially valid token — they propagate to the outer + // catch unchanged. + config.logger.w("Cached token is unreadable (device migration or corrupt payload). Clearing corrupted token and re-authenticating.", error: error) + do { + try await config.storage.delete() + } catch { + // If we cannot clear the corrupted token, re-authenticating would persist a + // new token over (or alongside) the unreadable one and the next token() call + // would loop on the same failure. Surface a real error instead of looping. + config.logger.e("Failed to clear unreadable token. Aborting to avoid a silent retry loop.", error: error) + return .failure(OidcError.authorizeError(cause: error)) } + // fall through to re-authenticate below } - + // Authenticate the user guard let agent = config.agent else { return .failure(OidcError.authorizeError(message: "Agent not configured")) @@ -191,7 +213,21 @@ public class OidcClient { private func revoke(_ token: Token? = nil) async { var accessToken = token if accessToken == nil { - accessToken = try? await config.storage.get() + do { + accessToken = try await config.storage.get() + } catch where error is EncryptorError || error is DecodingError { + // The stored token is permanently unreadable (Secure Enclave key did not migrate, + // or the payload is corrupt). There is nothing to revoke on the server, so just + // clear the dead entry. + config.logger.w("Stored token is unreadable. Clearing corrupted token.", error: error) + try? await config.storage.delete() + return + } catch { + // Transient failure (e.g. errSecInteractionNotAllowed while the device is locked). + // The token may well be valid — do NOT delete it. Skip this revoke attempt. + config.logger.w("Failed to read token for revocation. Leaving stored token intact.", error: error) + return + } } if let token = accessToken { do { diff --git a/Oidc/OidcTests/OidcClientTests.swift b/Oidc/OidcTests/OidcClientTests.swift index 92d72f8b..b3c34f9d 100644 --- a/Oidc/OidcTests/OidcClientTests.swift +++ b/Oidc/OidcTests/OidcClientTests.swift @@ -318,6 +318,92 @@ final class OidcClientTests: XCTestCase { XCTAssertTrue(revokeCalled, "The /revoke endpoint was not called.") } + // TestRailCase — SDKS-5172 recovery path: corrupted token triggers delete + re-auth + func testTokenRecoveryOnDecryptionFailure() async throws { + // Arrange: storage that throws EncryptorError.failedToDecrypt on the first get() + let throwingStorage = ThrowingMockStorage() + await throwingStorage.throwingMock.set(throwOnGet: true) + oidcClientConfig.storage = throwingStorage + + // MockURLProtocol is already configured in setUp() to return valid discovery + token + // responses, so re-authentication will succeed after the corrupted token is cleared. + + // Act + let result = await oidcClient.token() + + // Assert: re-authentication succeeded + switch result { + case .success(let token): + XCTAssertEqual(token.accessToken, "Dummy AccessToken") + case .failure(let error): + XCTFail("Expected success after recovery but got failure: \(error)") + } + + // Assert: the corrupted token was deleted before re-authenticating + let deleted = await throwingStorage.throwingMock.deleteWasCalled + XCTAssertTrue(deleted, "delete() should have been called to clear the corrupted token") + } + + // TestRailCase — SDKS-5172: if delete() fails during recovery, token() must surface a + // failure rather than re-authenticating and looping forever on the same corrupted token. + func testTokenRecoveryFailsWhenDeleteFails() async throws { + let throwingStorage = ThrowingMockStorage() + await throwingStorage.throwingMock.set(throwOnGet: true) // EncryptorError on get() + await throwingStorage.throwingMock.set(throwOnDelete: true) // delete() cannot clear it + oidcClientConfig.storage = throwingStorage + + let result = await oidcClient.token() + + switch result { + case .success: + XCTFail("Expected failure when the corrupted token cannot be cleared") + case .failure(let error): + // Should be the authorizeError wrapping the delete failure, not a silent re-auth. + if case .authorizeError = error { + // expected + } else { + XCTFail("Expected .authorizeError but got \(error)") + } + } + } + + // TestRailCase — SDKS-5172: a non-EncryptorError/non-DecodingError from get() is transient + // and must NOT trigger recovery (no delete, no re-auth) — it surfaces as a failure with the + // stored token left intact. + func testTokenDoesNotRecoverOnTransientStorageError() async throws { + let throwingStorage = ThrowingMockStorage() + await throwingStorage.throwingMock.set(getError: TransientStorageError.interactionNotAllowed) + await throwingStorage.throwingMock.set(throwOnGet: true) + oidcClientConfig.storage = throwingStorage + + let result = await oidcClient.token() + + switch result { + case .success: + XCTFail("Expected failure on a transient storage error") + case .failure: + break + } + + // The transient error must not have deleted the (potentially valid) token. + let deleted = await throwingStorage.throwingMock.deleteWasCalled + XCTAssertFalse(deleted, "delete() must not be called on a transient storage error") + } + + // TestRailCase — SDKS-5172: revoke() with a transient read error must leave the stored token + // intact (it may be valid) rather than deleting it. + func testRevokeLeavesTokenIntactOnTransientStorageError() async throws { + let throwingStorage = ThrowingMockStorage() + await throwingStorage.throwingMock.set(getError: TransientStorageError.interactionNotAllowed) + await throwingStorage.throwingMock.set(throwOnGet: true) + oidcClientConfig.storage = throwingStorage + + await oidcClient.revoke() + + let deleted = await throwingStorage.throwingMock.deleteWasCalled + XCTAssertFalse(deleted, "revoke() must not delete the token on a transient storage error") + } + private func makeClient(config: HttpClientConfig = HttpClientConfig()) -> URLSessionHttpClient { let sessionConfig = URLSessionConfiguration.ephemeral sessionConfig.protocolClasses = [MockURLProtocol.self] diff --git a/Oidc/OidcTests/mock/MockStorage.swift b/Oidc/OidcTests/mock/MockStorage.swift index 0195c358..5a47847d 100644 --- a/Oidc/OidcTests/mock/MockStorage.swift +++ b/Oidc/OidcTests/mock/MockStorage.swift @@ -2,7 +2,7 @@ // MockStorage.swift // OidcTests // -// Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved. +// Copyright (c) 2024 - 2026 Ping Identity Corporation. All rights reserved. // // This software may be modified and distributed under the terms // of the MIT license. See the LICENSE file for details. @@ -14,15 +14,15 @@ import PingStorage public actor Mock: Storage { private var data: T? - + public func save(item: T) async throws { data = item } - + public func get() async throws -> T? { return data } - + public func delete() async throws { data = nil } @@ -34,3 +34,80 @@ public class MockStorage: StorageDelegate, @unchecked S } } +// MARK: - ThrowingMock + +/// A simple error used to simulate a transient (non-`EncryptorError`) storage failure. +public enum TransientStorageError: Error, Sendable { + case interactionNotAllowed +} + +/// A configurable actor-based Storage that can simulate decryption and delete failures. +/// Used to test recovery paths where a cached token is unreadable (e.g. after device migration) +/// and the negative paths where the failure is transient or `delete()` itself fails. +public actor ThrowingMock: Storage { + private var data: T? + + /// When `true`, `get()` throws `getError` to simulate a failure reading the cached token. + public var throwOnGet: Bool = false + + /// The error thrown by `get()` when `throwOnGet` is `true`. Defaults to a decryption failure, + /// matching the Secure Enclave key-mismatch scenario after device migration. + public var getError: Error = EncryptorError.failedToDecrypt + + /// When `true`, `delete()` throws `KeychainError.unableToDelete` to simulate a keychain that + /// cannot clear the corrupted item (e.g. errSecInteractionNotAllowed while the device is locked). + public var throwOnDelete: Bool = false + + /// Set to `true` when `delete()` is called; allows test assertions on cleanup behaviour. + public var deleteWasCalled: Bool = false + + public func save(item: T) async throws { + data = item + } + + public func get() async throws -> T? { + if throwOnGet { + throw getError + } + return data + } + + public func delete() async throws { + deleteWasCalled = true + if throwOnDelete { + throw KeychainError.unableToDelete + } + throwOnGet = false + data = nil + } + + /// Convenience setter so tests can configure `throwOnGet` from outside the actor. + public func set(throwOnGet value: Bool) { + throwOnGet = value + } + + /// Convenience setter so tests can configure `throwOnDelete` from outside the actor. + public func set(throwOnDelete value: Bool) { + throwOnDelete = value + } + + /// Convenience setter so tests can configure the error thrown by `get()`. + public func set(getError value: Error) { + getError = value + } +} + +/// `ThrowingMockStorage` wraps `ThrowingMock` inside a `StorageDelegate` using `.NO_CACHE`, +/// mirroring the shape of `MockStorage`. Exposes the underlying `ThrowingMock` so tests can +/// inspect `deleteWasCalled` and toggle `throwOnGet`. +public class ThrowingMockStorage: StorageDelegate, @unchecked Sendable { + /// The underlying actor; use `await throwingMock.` to read test state. + public let throwingMock: ThrowingMock + + public init() { + let mock = ThrowingMock() + self.throwingMock = mock + super.init(delegate: mock, cacheStrategy: .NO_CACHE) + } +} + diff --git a/Storage/Storage/KeychainStorage.swift b/Storage/Storage/KeychainStorage.swift index 3e73ef8b..1634544f 100644 --- a/Storage/Storage/KeychainStorage.swift +++ b/Storage/Storage/KeychainStorage.swift @@ -30,18 +30,38 @@ public actor Keychain: Storage { /// - Parameter item: The item to save. public func save(item: T) async throws { let data = try JSONEncoder().encode(item) - var query = [ + + // Encrypt BEFORE deleting the existing item. If encryption fails (e.g. the Secure + // Enclave key is transiently unavailable under device lock or memory pressure), this + // throws here and the previously stored token is left untouched — never leaving the + // keychain slot empty. + let encrypted = try await encryptor.encrypt(data: data) + + // Delete using primary key only (class + account + service) — no accessibility filter + // so ANY pre-existing item is removed regardless of its stored accessibility class. + // This prevents errSecDuplicateItem when upgrading from an older SDK version that + // stored items with the default kSecAttrAccessibleWhenUnlocked accessibility. + let deleteQuery = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: account, + kSecAttrService as String: service + ] as [String: Any] + SecItemDelete(deleteQuery as CFDictionary) + + // Add the fresh item with device-only accessibility to prevent iCloud/iTunes backup + // migration. kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly is chosen over + // kSecAttrAccessibleWhenUnlockedThisDeviceOnly to allow background token refresh + // (Background App Refresh, silent push) after first device unlock, matching the + // accessibility used by SecuredKey for SE-backed keys. + let addQuery = [ kSecClass as String: kSecClassGenericPassword, kSecAttrAccount as String: account, kSecAttrService as String: service, - kSecValueData as String: data + kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, + kSecValueData as String: encrypted ] as [String: Any] - - query[kSecValueData as String] = try await encryptor.encrypt(data: data) - - SecItemDelete(query as CFDictionary) // Remove any existing item - let status = SecItemAdd(query as CFDictionary, nil) - + + let status = SecItemAdd(addQuery as CFDictionary, nil) guard status == errSecSuccess else { throw KeychainError.unableToSave } diff --git a/Storage/StorageTests/KeychainStorageTests.swift b/Storage/StorageTests/KeychainStorageTests.swift index 73c61d11..f35b4fa0 100644 --- a/Storage/StorageTests/KeychainStorageTests.swift +++ b/Storage/StorageTests/KeychainStorageTests.swift @@ -2,7 +2,7 @@ // KeychainStorageTests.swift // StorageTests // -// Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved. +// Copyright (c) 2024 - 2026 Ping Identity Corporation. All rights reserved. // // This software may be modified and distributed under the terms // of the MIT license. See the LICENSE file for details. @@ -14,19 +14,30 @@ import XCTest final class KeychainStorageTests: XCTestCase { private var keychainStorage: KeychainStorage! - + + // The account and service match exactly what KeychainStorage uses internally. + private let account = "testAccount" + private let service = "com.pingidentity.keychainService" + override func setUp() { super.setUp() // By default the KeychainStorage does not use encryption - keychainStorage = KeychainStorage(account: "testAccount") + keychainStorage = KeychainStorage(account: account) } - + override func tearDown() async throws { - try? await keychainStorage.delete() + // Clean up any keychain item regardless of accessibility attribute so subsequent + // tests always start from a clean slate. + let cleanupQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: account, + kSecAttrService as String: service + ] + SecItemDelete(cleanupQuery as CFDictionary) keychainStorage = nil try await super.tearDown() } - + // TestRailCase(24703) func testSaveItem() async throws { let item = TestItem(id: 1, name: "Test") @@ -34,7 +45,7 @@ final class KeychainStorageTests: XCTestCase { let retrievedItem = try await keychainStorage.get() XCTAssertEqual(retrievedItem, item) } - + // TestRailCase(24704) func testGetItem() async throws { let item = TestItem(id: 1, name: "Test") @@ -42,7 +53,7 @@ final class KeychainStorageTests: XCTestCase { let retrievedItem = try await keychainStorage.get() XCTAssertEqual(retrievedItem, item) } - + // TestRailCase(24705) func testDeleteItem() async throws { let item = TestItem(id: 1, name: "Test") @@ -51,4 +62,98 @@ final class KeychainStorageTests: XCTestCase { let retrievedItem = try await keychainStorage.get() XCTAssertNil(retrievedItem) } + + // SDKS-5172 — verify saved items carry device-only accessibility + func testSavedItemHasDeviceOnlyAccessibility() async throws { + let item = TestItem(id: 1, name: "Test") + try await keychainStorage.save(item: item) + + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: account, + kSecAttrService as String: service, + kSecReturnAttributes as String: true, + kSecMatchLimit as String: kSecMatchLimitOne + ] + var result: CFTypeRef? + let status = SecItemCopyMatching(query as CFDictionary, &result) + XCTAssertEqual(status, errSecSuccess) + let attrs = result as? [String: Any] + let accessible = attrs?[kSecAttrAccessible as String] as? String + XCTAssertEqual(accessible, kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly as String) + } + + // SDKS-5172 — verify upgrade path: item stored with old accessibility is replaced cleanly + func testUpgradePathDoesNotThrowDuplicateItem() async throws { + // Pre-populate keychain with old-style accessibility (simulating a token stored + // by an earlier SDK version that used kSecAttrAccessibleWhenUnlocked by default). + let oldData = try JSONEncoder().encode(TestItem(id: 99, name: "OldToken")) + let oldQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: account, + kSecAttrService as String: service, + kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked, + kSecValueData as String: oldData + ] + let insertStatus = SecItemAdd(oldQuery as CFDictionary, nil) + XCTAssertEqual(insertStatus, errSecSuccess, "Pre-condition: old item should insert cleanly") + + // Save via SDK — should delete the old item and add a new one without throwing. + let newItem = TestItem(id: 1, name: "NewToken") + do { + try await keychainStorage.save(item: newItem) + } catch { + XCTFail("save() should not throw when replacing an item with mismatched accessibility: \(error)") + } + + // Verify the new item carries device-only accessibility. + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrAccount as String: account, + kSecAttrService as String: service, + kSecReturnAttributes as String: true, + kSecMatchLimit as String: kSecMatchLimitOne + ] + var result: CFTypeRef? + let status = SecItemCopyMatching(query as CFDictionary, &result) + XCTAssertEqual(status, errSecSuccess) + let attrs = result as? [String: Any] + let accessible = attrs?[kSecAttrAccessible as String] as? String + XCTAssertEqual(accessible, kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly as String) + } + + // SDKS-5172 — verify the existing token survives a failed save() (encryption error). + // save() must encrypt BEFORE deleting the previous item, so a transient encrypt failure + // never leaves the keychain slot empty (data-loss regression guard). + func testExistingTokenSurvivesFailedSave() async throws { + // First, store a good token using a non-throwing encryptor. + let goodStorage = KeychainStorage(account: account) + let original = TestItem(id: 1, name: "Original") + try await goodStorage.save(item: original) + + // Now attempt to save a new token through an encryptor that always fails. + let failingStorage = KeychainStorage(account: account, encryptor: FailingEncryptor()) + do { + try await failingStorage.save(item: TestItem(id: 2, name: "New")) + XCTFail("save() should throw when encryption fails") + } catch { + // Expected — encryption failed. + } + + // The original token must still be retrievable (it was never deleted). + let retrieved = try await goodStorage.get() + XCTAssertEqual(retrieved, original, "Existing token must survive a failed save()") + } +} + +/// An `Encryptor` that always throws on encrypt — used to verify save() does not destroy the +/// existing keychain item when encryption fails. +private struct FailingEncryptor: Encryptor { + func encrypt(data: Data) async throws -> Data { + throw EncryptorError.failedToEncrypt + } + + func decrypt(data: Data) async throws -> Data { + throw EncryptorError.failedToDecrypt + } }