[PM-40519] feat: Add SDK-backed passkey registration and assertion services to TestHarness - #2945
Open
morganzellers-bw wants to merge 1 commit into
Conversation
github-actions
Bot
added
app:password-manager
This was referenced Aug 7, 2026
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the SDK-backed passkey service layer added to TestHarness: Code Review DetailsNo findings at or above the reporting threshold. |
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 7, 2026 21:56
3b9bd33 to
35f3fab
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pm-40519-testharness-passkeys-storage-foundation #2945 +/- ##
====================================================================================
+ Coverage 79.51% 79.54% +0.02%
====================================================================================
Files 1169 1169
Lines 75095 75095
====================================================================================
+ Hits 59715 59732 +17
+ Misses 15380 15363 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
2 times, most recently
from
August 7, 2026 22:38
7997cbe to
18947e9
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
3 times, most recently
from
August 10, 2026 14:55
fa0cea4 to
725a98c
Compare
morganzellers-bw
changed the title
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
2 times, most recently
from
August 10, 2026 18:46
93583b7 to
8ed6220
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
5 times, most recently
from
August 12, 2026 19:16
9600d27 to
1d7b483
Compare
morganzellers-bw
marked this pull request as ready for review
August 12, 2026 21:59
morganzellers-bw
requested review from
a team and
matt-livefront
as code owners
August 12, 2026 21:59
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
5 times, most recently
from
August 18, 2026 16:43
0c7291a to
19bce81
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 18, 2026 17:16
19bce81 to
73d79b7
Compare
morganzellers-bw
changed the base branch from
main
to
pm-40519-testharness-passkeys-storage-foundation
August 18, 2026 17:22
morganzellers-bw
changed the title
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
2 times, most recently
from
August 19, 2026 14:12
8995452 to
2f1c37d
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 20, 2026 16:47
2f1c37d to
f3e7a0d
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 25, 2026 20:41
f3e7a0d to
83a0072
Compare
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 26, 2026 19:08
83a0072 to
a523e92
Compare
…rvices Adds the Fido2-driving layer on top of the cipher storage foundation: the Fido2CredentialStore/Fido2UserInterface implementations and the PasskeyService that bootstraps a per-session BitwardenSdk.Client and drives its Fido2 authenticator for registration, assertion, and listing registered credentials, without going through the OS's passkey UI or a real vault. Wires PasskeyService into the ServiceContainer.
morganzellers-bw
force-pushed
the
pm-40519-testharness-passkeys-sdk-foundation
branch
from
August 27, 2026 16:38
a523e92 to
c4f80a6
Compare
fedemkr
reviewed
Aug 27, 2026
| self.cipherStorageService = cipherStorageService | ||
| self.platformClientService = platformClientService | ||
| self.vaultClientService = vaultClientService | ||
| ciphers = cipherStorageService.loadCiphers() |
Member
There was a problem hiding this comment.
🤔 I wonder if this is always a fast operation, as this directly impacts on how long takes the ServiceContainer in initialize.
Comment on lines
+157
to
+167
| func test_saveCredential_existingId_replacesExistingCipher() async throws { | ||
| let original = Cipher(cipherView: .fixture(id: "1", name: "Original")) | ||
| try await subject.saveCredential(cred: EncryptionContext(encryptedFor: "1", cipher: original)) | ||
|
|
||
| let updated = Cipher(cipherView: .fixture(id: "1", name: "Updated")) | ||
| try await subject.saveCredential(cred: EncryptionContext(encryptedFor: "1", cipher: updated)) | ||
|
|
||
| let result = try await subject.allCredentials() | ||
| XCTAssertEqual(result.map(\.name), ["Updated"]) | ||
| XCTAssertEqual(cipherStorageService.saveReceivedCiphers, [updated]) | ||
| } |
Member
There was a problem hiding this comment.
🤔 What do you think about adding a new test for parallel saving? So we actually test the actor part ensuring that save calls from multiple threads are safe.
Comment on lines
+72
to
+79
| func pickCredentialForAuthentication(availableCredentials: [CipherView]) async throws -> CipherViewWrapper { | ||
| guard availableCredentials.count == 1, let onlyCredential = availableCredentials.first else { | ||
| throw availableCredentials.isEmpty | ||
| ? PasskeyError.noMatchingCredential | ||
| : PasskeyError.ambiguousCredential | ||
| } | ||
| return CipherViewWrapper(cipher: onlyCredential) | ||
| } |
Member
There was a problem hiding this comment.
🤔 Not sure if this will be expanded on future PRs, but if the user would be selecting a passkey from a list then in here it should fire some kind of message to the UI to select the credential to be returned amongst the available ones.
Comment on lines
+8
to
+13
| // sourcery: AutoMockable | ||
| /// A service that performs passkey registration and authentication directly through | ||
| /// `BitwardenSdk`'s Fido2 client — the same way the main Bitwarden app and its AutoFill | ||
| /// extension do — without going through the OS's passkey UI or a real vault. | ||
| /// | ||
| public protocol PasskeyService: AnyObject { |
Member
There was a problem hiding this comment.
⛏️ We usually put the sourcery inline with the protocol definition.
Suggested change
| // sourcery: AutoMockable | |
| /// A service that performs passkey registration and authentication directly through | |
| /// `BitwardenSdk`'s Fido2 client — the same way the main Bitwarden app and its AutoFill | |
| /// extension do — without going through the OS's passkey UI or a real vault. | |
| /// | |
| public protocol PasskeyService: AnyObject { | |
| /// A service that performs passkey registration and authentication directly through | |
| /// `BitwardenSdk`'s Fido2 client — the same way the main Bitwarden app and its AutoFill | |
| /// extension do — without going through the OS's passkey UI or a real vault. | |
| /// | |
| public protocol PasskeyService: AnyObject { // sourcery: AutoMockable |
Comment on lines
+45
to
+50
| /// A protocol for an object that provides an `PasskeyService`. | ||
| protocol HasPasskeyService { | ||
| /// The service used to perform passkey registration and authentication through the | ||
| /// Bitwarden SDK. | ||
| var passkeyService: PasskeyService { get } | ||
| } |
Member
There was a problem hiding this comment.
🎨 I believe we should use a similar pattern as in the other apps where we have all the Has* protocols under Core -> Platform -> Services -> Services.swift file.
Member
There was a problem hiding this comment.
🤔 I believe we should use the Fido2 name instead of using Passkey to keep being aligned with the SDK. What's your opinion? @bitwarden/team-ios @matt-livefront ?
Comment on lines
+88
to
+95
| init( | ||
| cipherStorageService: CipherStorageService = DefaultCipherStorageService(), | ||
| keychainServiceFacade: KeychainServiceFacade = DefaultKeychainServiceFacade( | ||
| appSecAttrAccessGroup: Bundle.main.groupIdentifier, | ||
| keychainService: DefaultKeychainService(), | ||
| namespacing: .shared, | ||
| ), | ||
| ) { |
Member
There was a problem hiding this comment.
🎨 Could we use the same pattern as in the other apps where the implementations to use are passed in the ServiceContainer instead of inline here.
Comment on lines
+160
to
+241
| // MARK: Private | ||
|
|
||
| /// Loads the synthetic identity persisted from a previous launch, or generates and persists a | ||
| /// new one via the SDK's local-only key-generation primitive if none exists yet. | ||
| private func loadOrCreateIdentity() async throws -> SyntheticIdentity { | ||
| if let identity: SyntheticIdentity = try? await keychainServiceFacade.getValue( | ||
| for: PasskeyKeychainItem.syntheticIdentity, | ||
| ) { | ||
| return identity | ||
| } | ||
|
|
||
| let client = BitwardenSdk.Client(tokenProvider: ClientManagedTokensProvider(), settings: nil) | ||
| let email = "sdk-passkey-playground@bitwarden.com" | ||
| let password = UUID().uuidString | ||
| let keys = try client.auth().makeRegisterKeys( | ||
| email: email, | ||
| password: password, | ||
| kdf: Kdf.pbkdf2(iterations: Self.kdfIterations), | ||
| ) | ||
| let identity = SyntheticIdentity( | ||
| email: email, | ||
| encryptedUserKey: keys.encryptedUserKey, | ||
| kdfIterations: Self.kdfIterations, | ||
| password: password, | ||
| privateKey: keys.keys.private, | ||
| userId: UUID().uuidString, | ||
| ) | ||
| try await keychainServiceFacade.setValue(identity, for: PasskeyKeychainItem.syntheticIdentity) | ||
| // The new identity's key can't decrypt anything persisted under the previous one. | ||
| cipherStorageService.save(ciphers: []) | ||
| return identity | ||
| } | ||
|
|
||
| /// Lazily builds and crypto-initializes the SDK client and credential store for this session, | ||
| /// reusing them across calls so registered credentials remain visible to later assertions. | ||
| /// Caches the bootstrap as a `Task` so concurrent callers await the same work instead of each | ||
| /// racing their own bootstrap. | ||
| private func session() async throws -> (BitwardenSdk.Client, DefaultFido2CredentialStore) { | ||
| if let sessionTask { | ||
| return try await sessionTask.value | ||
| } | ||
|
|
||
| let task = Task { try await makeSession() } | ||
| sessionTask = task | ||
| do { | ||
| return try await task.value | ||
| } catch { | ||
| sessionTask = nil | ||
| throw error | ||
| } | ||
| } | ||
|
|
||
| /// Builds and crypto-initializes the SDK client and credential store for this session. | ||
| private func makeSession() async throws -> (BitwardenSdk.Client, DefaultFido2CredentialStore) { | ||
| let identity = try await loadOrCreateIdentity() | ||
| let client = BitwardenSdk.Client(tokenProvider: ClientManagedTokensProvider(), settings: nil) | ||
| let kdf = Kdf.pbkdf2(iterations: identity.kdfIterations) | ||
| try await client.crypto().initializeUserCrypto( | ||
| req: InitUserCryptoRequest( | ||
| userId: identity.userId, | ||
| kdfParams: kdf, | ||
| email: identity.email, | ||
| accountCryptographicState: .v1(privateKey: identity.privateKey), | ||
| method: .masterPasswordUnlock( | ||
| password: identity.password, | ||
| masterPasswordUnlock: MasterPasswordUnlockData( | ||
| kdf: kdf, | ||
| masterKeyWrappedUserKey: identity.encryptedUserKey, | ||
| salt: identity.email, | ||
| ), | ||
| ), | ||
| upgradeToken: nil, | ||
| ), | ||
| ) | ||
|
|
||
| let credentialStore = DefaultFido2CredentialStore( | ||
| cipherStorageService: cipherStorageService, | ||
| platformClientService: client.platform(), | ||
| vaultClientService: client.vault(), | ||
| ) | ||
| return (client, credentialStore) | ||
| } |
Member
There was a problem hiding this comment.
🎨 I would refactor this to have its own service: ClientService. This is not particular specific for Fido2 operations but of SDK client set up. Moreover we could reuse this when more features are added to the TestHarness app.
Additionally, there are other things we'll have to configure in the future for the SDK clients, like repositories and more stuff so all of that should be encapsulated in its own service.
Finally, it would maintain a similar structure as in the other apps which would be easier to find and maintain.
| "challenge": challenge.base64EncodedString(), | ||
| "origin": "https://\(rpId)", | ||
| ] | ||
| let clientDataJSON = (try? JSONSerialization.data(withJSONObject: clientData, options: [.sortedKeys])) ?? Data() |
Member
There was a problem hiding this comment.
Data which is not the expected hash.
Comment on lines
+63
to
+64
| /// The number of PBKDF2 iterations used to derive this session's synthetic identity's keys. | ||
| private static let kdfIterations: UInt32 = 600_000 |
Member
There was a problem hiding this comment.
⛏️ Extract to a Constants file to keep structure similar to other apps.
Contributor
Author
🤖 Claude Security Code Review 🤖PR: (#2945) - PM-40519 feat: Add SDK-backed passkey registration and assertion services to TestHarness — 2026-08-27 Date: 2026-08-27 Commits reviewed: c419835..c4f80a6 · 2 commits · TestHarnessShared/Core/Autofill/Passkey/
Summary
📝 NotesExpand for details on (14) notes
✅ StrengthsExpand for details on (13) strengths
❌ DismissedExpand for details on (5) dismissed findings
Net result: 0 Blockers, 0 Improvements — nothing here should hold up the PR. The 14 Notes are all low-cost hardening opportunities around the synthetic identity's error handling, keychain namespacing, and defensive serialization; worth a follow-up cleanup pass but not urgent given the complete isolation from production trust boundaries. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-40519
📔 Objective
Adds SDK-backed passkey test scenarios to TestHarness, on top of the cipher storage/identity foundation in #2977.
Core/Autofilllayer, which drivesBitwardenSdk's Fido2 client directly (makeCredential/getAssertion) the same way the main app and AutoFill extension do, instead of delegating to the OS passkey UI.PasskeyServicelazily bootstraps aBitwardenSdk.Clientbacked by the synthetic identity persisted in the Keychain, so registered credentials survive relaunches.DefaultFido2CredentialStore/DefaultFido2UserInterfacesatisfy the SDK'sFido2CredentialStore/Fido2UserInterfaceprotocols.testHarness_passkeys_flow.MP4
🔐 Security Review
Click to reveal
🤖 Claude Security Code Review 🤖
PR: (#2945) - PM-40519 feat: Add SDK-backed passkey registration and assertion services to TestHarness — 2026-08-27
Date: 2026-08-27
Commits reviewed: c419835..c4f80a6 · 2 commits · TestHarnessShared/Core/Autofill/Passkey/
f4b8cb4b6c4f80a68fSummary
TestHarnessShared, a non-shipping internal developer tool with a bundle ID, app group, and keychain access group fully disjoint from the production Password Manager and Authenticator apps — no production trust boundary is touched.EncString-typed ciphertext is ever persisted toUserDefaults; key material stays exclusively in the Keychain.try?that conflates keychain errors, a self-contradictory UV flag, salt/password co-location) — none reach real user data because there is no real account, no network path, and no verifier consuming these ceremonies today.try?inPasskeyService.loadOrCreateIdentity()to only treatkeyNotFoundas "create new" — it's cheap, removes a real failure-mode ambiguity, and is exactly the kind of pattern that could get copy-pasted into shipping code later.📝 Notes
Expand for details on (14) notes
try?on the keychain identity read conflates "not found" with "read failed" (locked device, missing entitlement, decode failure), causing an unconditional overwrite of the identity and a full wipe of stored passkeys on any of those errors.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:165-190isVerificationEnabled()returnsfalsewhile every check-user ceremony unconditionally returnsuserVerified: true, so emitted authenticator data always claims UV was satisfied.TestHarnessShared/Core/Autofill/Passkey/DefaultFido2UserInterface.swift:14, 64, 68-70clientDataJSON), so there's no live exploit path — forward-risk only, and it's documented as intentional.TestHarnessShared/Core/Autofill/Passkey/SyntheticIdentity.swift:21-23,PasskeyService.swift:173,187,PasskeyKeychainItem.swift:14-21clientDataJSONis discarded before reaching the SDK;rpIdis interpolated into anoriginstring with no validation.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:105-114, 123, 143-144clientDataJSONnever leaves the process today, so nothing is forgeable against a real verifier; documented by design, but the unvalidatedrpId→origin interpolation is exactly what would need hardening if this is ever extended to accept external challenges..sharednamespacing and the app-group access group, deviating from the codebase's established.appScopedconvention for private, non-cross-app secrets.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:90-94try?finding above — a future app extension would compute a different, non-entitled group and trigger silent destruction.sdk-passkey-playground@bitwarden.com), identical across every install.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:172, 228clientDataHashsilently falls back toSHA256("")— a fixed, publicly known constant — ifJSONSerializationfails.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:112[String: String]dictionary cannot fail to serialize, but silently substituting a predictable constant for signed input is a smell worth removing (throwsinstead oftry? ?? Data()).rpIdare unbounded (excludeList: nil), and multiple credentials for one RP later throw.ambiguousCredentialduring assertion.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:146, 151,DefaultFido2UserInterface.swift:72-79credentialId/registeredCredentials(), so this is a functionality rough edge rather than a security gap.BitwardenSdk.Client(tokenProvider:settings: nil)defaults to productionidentity.bitwarden.com/api.bitwarden.comrather than an explicit non-production endpoint.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:171, 215TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:74, 197-209, 662-675StoredCipherdrops the SDK'sCipher.data(blob-format) field, andinit?(cipher:)silently returnsnilfor any unrecognized cipher shape viacompactMap, with no error surfaced.TestHarnessShared/Core/Autofill/Passkey/StoredCipher.swift:129, 138-141,CipherStorageService.swift:61name: nil; a future SDK revision to that format would make every cipher fail to reconstruct and silently empty storage on the next save.Constantsfile.TestHarnessShared/Core/Autofill/Passkey/PasskeyService.swift:64TestHarnessShareddoesn't linkBitwardenShared(where the existing constant lives); the durable fix is promoting the shared constant intoBitwardenKit.CipherStorageServicesilently returns[]onJSONDecoderfailure and silently drops the entire save onJSONEncoderfailure.TestHarnessShared/Core/Autofill/Passkey/CipherStorageService.swift:52-64StoredCipher.cipherreconstructs security-relevant fields (reprompt,edit,viewPassword,organizationUseTotp) from hardcoded defaults rather than persisting them; a persist/reload round-trip silently resetsrepromptto.none.TestHarnessShared/Core/Autofill/Passkey/StoredCipher.swift:71-131DefaultFido2UserInterfaceonly ever creates ciphers withreprompt: .none, but the same silent-attribute-loss class as theCipher.datafinding — worth a doc comment so a future cipher shape doesn't lose protection flags on round-trip.✅ Strengths
Expand for details on (13) strengths
UUID().uuidString, a CSPRNG-backed identifier.TestHarnessShared/Core/Autofill/Passkey/(all files)Fido2Credential'skeyValue,credentialId,rpId,userHandle,userName, etc. are allEncString, as isCipher.name/Cipher.key.CipherStorageService.swift:31-65,StoredCipher.swiftEncStringis a naming convention,typealias EncString = String, not a compiler-enforced type.)Package.resolved.project-common.yml(unchanged)BitwardenSdkpackage and an in-repo mocks target.project-bwth.yml:80, 178.ambiguousCredentialrather than defaulting to the first match.DefaultFido2UserInterface.swift:71-79DefaultFido2CredentialStore.swift:71-76project-bwth.yml,TestHarness.entitlements, and the xcconfig files: disjoint bundle ID, app group, and keychain access group from both Password Manager and Authenticator.TestHarness.entitlements,Configs/Common-bwth.xcconfigPasskeyService.swift:64PasskeyService.swift:74,DefaultFido2CredentialStore.swiftCipherStorageServicebeneath both actors is non-Sendable— harmless today sinceUserDefaultsis thread-safe).clientDatais built viaJSONSerialization, not string concatenation, sorpIdcannot alter the JSON structure.PasskeyService.swift:107-112GetAssertionResult) reaches any logging orErrorReportercall site anywhere in the diff.TestHarnessShared/Core/Autofill/Passkey/(all files)ErrorReportercalls at all in the added files.rpIdmatching in credential lookup is the spec-correct authenticator-level WebAuthn behavior, not merely a lucky safe default.DefaultFido2CredentialStore.swift:59UserDefaultsbackup without the matchingThisDeviceOnlyKeychain item correctly discards the now-undecryptable ciphers instead of wedging.PasskeyServiceTests.swift:854-880try?finding above, so narrowing that catch tokeyNotFoundwill preserve this tested behavior while removing the over-trigger.❌ Dismissed
Expand for details on (5) dismissed findings
CipherStorageService.swift:46, 60-64,PasskeyKeychainItem.swift:18SendableDefaultCipherStorageServiceis written from two different actor isolation domains.PasskeyService.swift:189,DefaultFido2CredentialStore.swift:82UserDefaultsis thread-safe; no memory-safety or security consequence.DefaultFido2CredentialStoreinstances could clobber each other's registrations (last-writer-wins).DefaultFido2CredentialStore.swift:43, 70-77sessionTaskcache guarantees one live instance today.findCredentials, only exactrpIdmatch.DefaultFido2CredentialStore.swift:59-62Gemfile.lock:45-46(fastlane transitive dependencies)fastlane/Gemfile.lockbump rather than blocking here.Net result: 0 Blockers, 0 Improvements — nothing here should hold up the PR. The 14 Notes are all low-cost hardening opportunities around the synthetic identity's error handling, keychain namespacing, and defensive serialization; worth a follow-up cleanup pass but not urgent given the complete isolation from production trust boundaries.
Stack: #2977 → #2945 → #2946 → #2947 → #2948