diff --git a/docs/auth/token-lifecycle.md b/docs/auth/token-lifecycle.md index 576432eebb..5d535c1fb1 100644 --- a/docs/auth/token-lifecycle.md +++ b/docs/auth/token-lifecycle.md @@ -168,6 +168,15 @@ transition, that overload can fall back to credential-scoped key material. This not consult the global preference. Token-endpoint proofs omit `ath`; resource proofs bind the proof to the current access token with `ath`. +`DPoPKeyStore` caches each credential-scoped `SecKey` handle in process memory, keyed by the derived +Keychain key name. The first "load or create the credential-scoped EC P-256 keypair" step for a +scope performs the Keychain lookup (or Secure Enclave key generation on first use) and caches the +resulting handle; every subsequent proof build for that scope reuses the cached handle directly, +skipping the Keychain (`SecItemCopyMatching`/`securityd`) round-trip and the exclusive lock that +previously serialized every DPoP proof build across every credential. Proof signing still happens +per request against the Secure-Enclave/Keychain-backed private key — only the *lookup* is cached, so +no private-key material is exported or held in plaintext. + --- ## 5. DPoP Nonce Lifecycle @@ -198,6 +207,9 @@ refresh, where the token endpoint can provide a fresh nonce before the resource Account deletion clears both the credential-scoped DPoP keypair and cached nonces. Credential migration clears the old credential's DPoP state after the new credential state is established. +Both paths evict the corresponding entry from `DPoPKeyStore`'s in-process key-pair cache before +removing the persisted Keychain entry, so no later lookup for that scope can return a stale handle +to a now-deleted key. --- diff --git a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift index 73f5507892..570de320da 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift +++ b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift @@ -57,6 +57,12 @@ public final class DPoPKeyStore: NSObject { // Use dispatch queue to prevent a potential double create race private let queue = DispatchQueue(label: "com.salesforce.dpop.keystore", attributes: .concurrent) + // Process-local cache of key-pair handles, keyed by the derived Keychain key name. + // The key material never changes for the lifetime of a credential, so once loaded or + // generated, the handle is safe to reuse for the rest of the process's lifetime. + // Accessed only under `queue` — concurrent reads, exclusive (.barrier) writes. + private var keyPairCache: [String: DPoPKeyPair] = [:] + private override init() { super.init() } /// Returns the keypair bound to the given scope identifier, generating it on first call. @@ -66,13 +72,24 @@ public final class DPoPKeyStore: NSObject { throw DPoPKeyStoreError.missingScopeIdentifier } let name = Self.keyName(for: scope) - // In this specific file, only .barrier work runs on this queue, so it's behaviorally - // identical to a serial queue. The concurrent-with-barriers pattern is the standard Swift - // idiom for reader/writer locks — concurrent reads, exclusive write. + + // Warm path: concurrent (non-barrier) read. No Keychain I/O, no exclusive lock. + if let cached = queue.sync(execute: { keyPairCache[name] }) { + return cached + } + + // Cold path: exclusive barrier. Double-check the cache (a concurrent cold caller may + // have populated it while this call was waiting), then load-or-generate exactly once + // and cache the handle. return try queue.sync(flags: .barrier) { + if let cached = keyPairCache[name] { + return cached + } do { let pair = try KeyGenerator.ecKeyPair(name: name) - return DPoPKeyPair(publicKey: pair.publicKey, privateKey: pair.privateKey) + let keyPair = DPoPKeyPair(publicKey: pair.publicKey, privateKey: pair.privateKey) + keyPairCache[name] = keyPair + return keyPair } catch { SFSDKCoreLogger.e(Self.self, message: "DPoP keypair generation failed: \(error.localizedDescription)") throw DPoPKeyStoreError.keyGenerationFailed @@ -86,15 +103,22 @@ public final class DPoPKeyStore: NSObject { return try keyPair(forScope: credentials.identifier) } - /// Returns `true` iff a private key is already present in the Keychain for `scope`. - /// Side-effect-free — never generates a key on miss. Used to gate DPoP proof attachment - /// on the presence of previously-minted key material for the credential. + /// Returns `true` if key material for `scope` is available — from the in-process cache, + /// or (on a cache miss) from the Keychain. Side-effect-free — never generates a key on + /// miss. Used to gate DPoP proof attachment on the presence of previously-minted key + /// material for the credential. + /// + /// Note: a cache hit reports `true` without touching the Keychain, so if the persistent + /// key were removed out of band (i.e. not via `delete(forScope:)`) this can report `true` + /// until the in-process cache is evicted. Every in-SDK deletion routes through + /// `delete(forScope:)`, which evicts the cache, keeping the two views consistent. public func hasKeyPair(forScope scope: String) -> Bool { guard !scope.isEmpty else { return false } let name = Self.keyName(for: scope) // Non-barrier read: safe to run concurrently with other reads. // All mutating methods in this class use .barrier for exclusive access. return queue.sync { + if keyPairCache[name] != nil { return true } // cache-first short-circuit guard let privateTag = try? KeyGenerator.keyTag(name: name, prefix: KeyGenerator.ecPrivateKeyTagPrefix) else { // A throw here isn't necessarily "key absent" — it can be a Keychain access // denied, entitlement, or device-locked error. Returning `false` means @@ -117,6 +141,7 @@ public final class DPoPKeyStore: NSObject { guard !scope.isEmpty else { return } let name = Self.keyName(for: scope) queue.sync(flags: .barrier) { + keyPairCache.removeValue(forKey: name) // evict BEFORE removing the persistent key do { try KeyGenerator.removeECKeyPair(name: name) } catch { @@ -130,6 +155,14 @@ public final class DPoPKeyStore: NSObject { delete(forScope: credentials.identifier) } + /// Clears process-local key-pair handles without deleting their Keychain entries. + /// Test-only (simulates a process restart / cold load). Not `@objc` — invisible to consumers. + internal func clearInMemoryCache() { + queue.sync(flags: .barrier) { + keyPairCache.removeAll() + } + } + // MARK: - Internal helpers static func keyName(for scope: String) -> String { diff --git a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift index e18936c150..a5f22f87c4 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift +++ b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift @@ -422,6 +422,108 @@ class SFSDKDPoPTests: XCTestCase { } } + // MARK: - In-memory key-pair cache + + /// Warm hit must return the exact same cached instance (object identity), not merely + /// an equivalent-but-distinct wrapper — a fresh `DPoPKeyPair` would mean the lookup + /// re-hit the Keychain and re-acquired the barrier lock. + func test_givenCachedKeyPair_whenFetchedAgain_thenSameInstanceReturned() throws { + let scope = "cache-warm-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + let first = try DPoPKeyStore.shared.keyPair(forScope: scope) + let second = try DPoPKeyStore.shared.keyPair(forScope: scope) + XCTAssertTrue(first === second, "warm hit must return the identical cached instance") + } + + /// N concurrent cold callers for a not-yet-cached scope must converge on exactly one + /// generated instance — the barrier's double-check must prevent a double-mint race. + func test_givenConcurrentCallers_whenGenerateOrLoad_thenSameCachedInstance() throws { + let scope = "cache-concurrent-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + let iterations = 50 + var results: [DPoPKeyPair?] = Array(repeating: nil, count: iterations) + let lock = NSLock() + + // `concurrentPerform` genuinely runs the closures in parallel (unlike serial-prone + // `async` dispatch), so multiple cold callers actually race the barrier's double-check. + DispatchQueue.concurrentPerform(iterations: iterations) { i in + if let kp = try? DPoPKeyStore.shared.keyPair(forScope: scope) { + lock.lock(); results[i] = kp; lock.unlock() + } + } + + let resolved = results.compactMap { $0 } + XCTAssertEqual(resolved.count, iterations, "every concurrent caller should get a keypair") + let first = try XCTUnwrap(resolved.first) + for result in resolved.dropFirst() { + XCTAssertTrue(result === first, "all concurrent cold callers must converge on the same cached instance") + } + } + + /// Deleting a scope must evict its cached handle — the next fetch must not resurrect + /// the pre-delete instance, and must mint a genuinely new key pair. + func test_givenCachedKeyPair_whenDeleted_thenNextFetchGeneratesNew() throws { + let scope = "cache-delete-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + let first = try DPoPKeyStore.shared.keyPair(forScope: scope) + DPoPKeyStore.shared.delete(forScope: scope) + let regenerated = try DPoPKeyStore.shared.keyPair(forScope: scope) + XCTAssertFalse(first === regenerated, "delete must evict the cache, not just leave a stale handle live") + let firstJwk = try Encryptor.jwkP256(from: first.publicKey) + let regenJwk = try Encryptor.jwkP256(from: regenerated.publicKey) + XCTAssertNotEqual(firstJwk, regenJwk, "post-delete key pair must have a different public key") + } + + /// Clearing the in-memory cache (simulating a process restart) must reload the same + /// persistent Keychain-backed key on the next fetch, and re-cache it for subsequent + /// warm hits. + func test_givenMemoryCacheCleared_whenFetched_thenSamePersistentKeyReloaded() throws { + let scope = "cache-reset-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + let first = try DPoPKeyStore.shared.keyPair(forScope: scope) + let firstJwk = try Encryptor.jwkP256(from: first.publicKey) + + DPoPKeyStore.shared.clearInMemoryCache() + + let reloaded = try DPoPKeyStore.shared.keyPair(forScope: scope) + let reloadedJwk = try Encryptor.jwkP256(from: reloaded.publicKey) + XCTAssertEqual(firstJwk, reloadedJwk, "cache-reset reload must return the same persistent key") + + let rewarmed = try DPoPKeyStore.shared.keyPair(forScope: scope) + XCTAssertTrue(reloaded === rewarmed, "the reloaded instance must be re-cached for subsequent warm hits") + } + + /// `hasKeyPair` must short-circuit on a cache hit. To prove this is genuinely the + /// cache-first path (and not just the pre-change Keychain lookup — which would also + /// pass while the persisted key exists), remove the persisted key *out of band* via + /// `KeyGenerator`, bypassing `delete(forScope:)` so the in-memory cache is NOT evicted: + /// the cache-first path must still report `true`, whereas the Keychain-only + /// implementation would report `false`. Clearing the cache then falls back to the + /// Keychain and reports the now-absent key. + func test_givenCachedKeyPair_whenPersistentKeyRemovedOutOfBand_thenCacheStillReportsPresentUntilCleared() throws { + let scope = "cache-haskey-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + + // Absent before any key exists; empty scope is always false. + XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: scope)) + XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: "")) + + // Warm the in-process cache. + _ = try DPoPKeyStore.shared.keyPair(forScope: scope) + XCTAssertTrue(DPoPKeyStore.shared.hasKeyPair(forScope: scope)) + + // Remove the persisted key directly, WITHOUT going through delete(forScope:), so the + // cache entry survives. The Keychain-only implementation would now return false. + _ = try KeyGenerator.removeECKeyPair(name: DPoPKeyStore.keyName(for: scope)) + XCTAssertTrue(DPoPKeyStore.shared.hasKeyPair(forScope: scope), + "a cache hit must satisfy hasKeyPair even after the persistent key is gone") + + // Evict the cache: hasKeyPair now falls back to the Keychain and sees the absence. + DPoPKeyStore.shared.clearInMemoryCache() + XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: scope), + "after clearing the cache, hasKeyPair must reflect the now-absent persistent key") + } + // MARK: - Nonce cache func test_givenScopedNonce_whenSameUrlSameScope_thenReturnsCachedValue() {