From da1d1d52f5760b871b46abf2da06f4701734f651 Mon Sep 17 00:00:00 2001 From: Takashi Arai Date: Tue, 22 Sep 2026 14:01:10 -0700 Subject: [PATCH 1/2] fix(dpop): cache key pair to drop per-request keychain lookup and lock keyPair(forScope:) did a Keychain lookup (SecItemCopyMatching) and took a global exclusive barrier lock on every DPoP proof build, serializing all DPoP-bound requests behind uncached securityd round-trips. Add a process-local key-pair cache keyed by scope: warm hits are served via a non-barrier concurrent read (no Keychain I/O, no lock); the barrier is taken only on the cold load/generate path (double-checked to guarantee a single mint) and on delete, which evicts the cached entry. Key material and wire behavior are unchanged: proofs and headers are byte-identical; signing still runs per request against the Keychain-backed key. iOS counterpart to Android #3045. --- docs/auth/token-lifecycle.md | 12 +++ .../Classes/OAuth/DPoP/DPoPKeyStore.swift | 35 +++++++- .../SFSDKDPoPTests.swift | 82 +++++++++++++++++++ 3 files changed, 125 insertions(+), 4 deletions(-) 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..bb33a629c2 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 @@ -95,6 +112,7 @@ public final class DPoPKeyStore: NSObject { // 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 +135,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 +149,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..69b43275d0 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift +++ b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift @@ -422,6 +422,88 @@ 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 without touching the Keychain, and + /// must still correctly report absence for an uncached/empty scope. + func test_givenCachedKeyPair_whenHasKeyPair_thenTrue_andFalseForEmptyScope() throws { + let scope = "cache-haskey-\(UUID().uuidString)" + defer { DPoPKeyStore.shared.delete(forScope: scope) } + XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: scope)) + _ = try DPoPKeyStore.shared.keyPair(forScope: scope) + XCTAssertTrue(DPoPKeyStore.shared.hasKeyPair(forScope: scope)) + XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: "")) + } + // MARK: - Nonce cache func test_givenScopedNonce_whenSameUrlSameScope_thenReturnsCachedValue() { From 822df13250277a74a551a48823626ab17e26c169 Mon Sep 17 00:00:00 2001 From: Takashi Arai Date: Tue, 22 Sep 2026 14:24:47 -0700 Subject: [PATCH 2/2] fix(dpop): clarify hasKeyPair contract; make cache regression test explicit Address review feedback: - hasKeyPair doc now states key material is reported from the in-process cache OR the Keychain, and notes the out-of-band-deletion window. - Rework the hasKeyPair test into a genuine cache-first regression test: remove the persisted key directly (bypassing delete, so the cache is not evicted), assert presence still holds via the cache, then clear the cache and assert the Keychain fallback reports absence. --- .../Classes/OAuth/DPoP/DPoPKeyStore.swift | 12 ++++++-- .../SFSDKDPoPTests.swift | 28 ++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift index bb33a629c2..570de320da 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift +++ b/libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift @@ -103,9 +103,15 @@ 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) diff --git a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift index 69b43275d0..a5f22f87c4 100644 --- a/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift +++ b/libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift @@ -493,15 +493,35 @@ class SFSDKDPoPTests: XCTestCase { XCTAssertTrue(reloaded === rewarmed, "the reloaded instance must be re-cached for subsequent warm hits") } - /// `hasKeyPair` must short-circuit on a cache hit without touching the Keychain, and - /// must still correctly report absence for an uncached/empty scope. - func test_givenCachedKeyPair_whenHasKeyPair_thenTrue_andFalseForEmptyScope() throws { + /// `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)) - XCTAssertFalse(DPoPKeyStore.shared.hasKeyPair(forScope: "")) + + // 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