Skip to content

fix(dpop): cache key pair to drop per-request keychain lookup and lock - #4178

Merged
sfdctaka merged 2 commits into
forcedotcom:devfrom
sfdctaka:fix/dpop-keypair-cache
Sep 22, 2026
Merged

sfdctaka merged 2 commits into
forcedotcom:devfrom
sfdctaka:fix/dpop-keypair-cache

Conversation

@sfdctaka

@sfdctaka sfdctaka commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

DPoPKeyStore.keyPair(forScope:) performed a Keychain lookup (SecItemCopyMatching) and acquired a global exclusive barrier lock on every DPoP proof build (resource + token endpoint). Under DPoP, that serialized all DPoP-bound requests behind uncached securityd round-trips.

This adds a process-local key-pair cache keyed by credential scope:

  • Warm path — served via a non-barrier concurrent read: no Keychain I/O, no exclusive lock.
  • Cold path — the .barrier is taken only to load-or-generate the key pair once, double-checked so concurrent cold callers converge on a single mint.
  • Delete — evicts the cached entry (inside the same barrier) before removing the persistent key, so a later fetch regenerates.
  • hasKeyPair(forScope:) short-circuits on a cache hit.

Key material, proof format, claims, and header-attachment behavior are unchanged; signing still occurs independently for every request (the proof's jti/iat and its ECDSA signature vary per request by design). No public API change; no feature flag (straight revert to roll back).

iOS counterpart to Android #3045.

Verification

  • New unit tests in SFSDKDPoPTests: warm-hit object identity, concurrent convergence (via concurrentPerform, single-mint), delete eviction, in-memory-clear reloads the same persistent key, and a hasKeyPair cache-first regression test (persisted key removed out of band, cache still reports present until cleared).
  • SalesforceSDKCoreTests full scheme passing (iPhone 16, iOS 18.6).
  • docs/auth/token-lifecycle.md updated to document the cache and eviction-on-delete.

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 forcedotcom#3045.
@sfdctaka
sfdctaka marked this pull request as ready for review September 22, 2026 21:02
@sfdctaka
sfdctaka requested a review from wmathurin September 22, 2026 21:02
@github-actions

Copy link
Copy Markdown
TestsPassedSkippedFailed ❌️
AuthFlowTester UI Test Results all1 ran1 ❌
TestResult
AuthFlowTester UI Test Results all
AuthFlowTesterUITests.xctest
LegacyLoginTests.testCAOpaque_DefaultScopes_WebServerFlow()❌ failure

@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.81%. Comparing base (90028da) to head (822df13).
⚠️ Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
...forceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #4178      +/-   ##
==========================================
- Coverage   71.73%   67.81%   -3.92%     
==========================================
  Files         254      254              
  Lines       23027    23040      +13     
==========================================
- Hits        16518    15625     -893     
- Misses       6509     7415     +906     
Components Coverage Δ
Analytics 70.78% <ø> (ø)
Common 71.20% <ø> (ø)
Core 61.70% <94.11%> (-5.86%) ⬇️
SmartStore 73.45% <ø> (ø)
MobileSync 88.84% <ø> (ø)
Files with missing lines Coverage Δ
...forceSDKCore/Classes/OAuth/DPoP/DPoPKeyStore.swift 81.96% <94.11%> (+2.80%) ⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread libs/SalesforceSDKCore/SalesforceSDKCoreTests/SFSDKDPoPTests.swift
@wmathurin

Copy link
Copy Markdown
Contributor

One PR-description correction: “proofs and headers are byte-identical” is not accurate because DPoP proofs intentionally vary through jti, iat, and the ECDSA signature. Could we replace it with something like: “Key material, proof format, claims, and header-attachment behavior are unchanged; signing still occurs independently for every request.”

@github-actions

Copy link
Copy Markdown
TestsPassed ✅SkippedFailed
SalesforceSDKCore iOS ^18 Test Results1050 ran1050 ✅
TestResult
No test annotations available

…plicit

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.
@sfdctaka
sfdctaka merged commit f97217f into forcedotcom:dev Sep 22, 2026
23 of 24 checks passed
@sfdctaka
sfdctaka deleted the fix/dpop-keypair-cache branch September 22, 2026 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants