[PM-27238] feat: Support v2 encryption on key connector signups by matt-livefront · Pull Request #2648 · bitwarden/ios

matt-livefront · 2026-05-14T18:25:58Z

🎟️ Tracking

📔 Objective

Enables support for V2 encryption on key connector signups. Most of the existing logic that was implemented in the app, including API requests, has been moved into the SDK (via the postKeysForKeyConnectorRegistration(keyConnectorUrl:ssoOrgIdentifier:) function).

These changes are behind the enable-account-encryption-v2-key-connector-registration feature flag.

github-actions · 2026-05-14T18:27:23Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds V2 account encryption support to the Key Connector new-user registration flow, gated behind the enable-account-encryption-v2-key-connector-registration feature flag. The V2 path delegates the multi-step key generation and API posting to the SDK's postKeysForKeyConnectorRegistration function. The vault-unlock step is consolidated into AuthRepository.convertNewUserToKeyConnector (eliminating a redundant unlockVaultWithKeyConnectorKey call in SingleSignOnProcessor), and a loading overlay is now shown during migration. Test coverage spans both V1 (legacy) and V2 paths, including SDK-failure and unlock-failure scenarios.

Code Review Details

No findings.

quexten

@matt-livefront It's fine if that's out of scope for this PR, but the way iOS is storing the account cryptographic state as a custom model which is different from the public API surface of the SDK is weird and seems brittle / likely to introduce bugs. Is it not possible to just store the SDK's model, or even a string serialized version of it?

Specifically, mobile should not care about the internals of that object or individually set fields on it (i guess with the exception of sync, though we should work on moving that part to the SDK too).

quexten · 2026-05-15T04:22:58Z

+        switch accountCryptographicState {
+        case let .v1(privateKey):
+            self.init(
+                accountKeys: nil,


AccountKeys should not be nil. It should be populated with a PrivateKeysResponseModel with with signature key pair and security state set to nil, but the publicKeyEncryptionKeyPair present.

Do you know where that could come from in this key connector registration flow? It isn't returned in KeyConnectorRegistrationResult when calling postKeysForKeyConnectorRegistration? Given that the SDK is generating V2 encryption keys, is it possible to end up in this v1 case? Maybe the answer is, as you suggested, to persist WrappedAccountCryptographicState directly, but I was trying to make this work with we we already have in place to avoid additional changes.

As mentioned, persisting the WrappedAccountCryptographicState is the correct solution.

IF needed, then you can make an accountKeys model in the same way you make the V2 model, only leave out the security state and signature key pair, and signed public key.

quexten · 2026-05-15T04:24:39Z

+            self.init(
+                accountKeys: PrivateKeysResponseModel(
+                    publicKeyEncryptionKeyPair: PublicKeyEncryptionKeyPairResponseModel(
+                        publicKey: "", // Not returned by SDK at registration; will populate on next sync.


This feels like it has a high risk of introducing bugs. Can we not just store the accountCryptographicState? That's what the SDK takes for init, and KM expects clients to just store it instead of the response model.

quexten · 2026-05-15T04:24:49Z

+                    ),
+                    signatureKeyPair: SignatureKeyPairResponseModel(
+                        wrappedSigningKey: signingKey,
+                        verifyingKey: "", // Not returned by SDK at registration; will populate on next sync.


Same comment as above, this seems like a high risk.

quexten · 2026-05-15T04:25:30Z

+    ///   - accountCryptographicState: The SDK's wrapped account cryptographic state.
+    ///   - encryptedUserKey: An optional encrypted user key to store alongside the cryptographic state.
+    ///
+    init(accountCryptographicState: WrappedAccountCryptographicState, encryptedUserKey: String? = nil) {


I don't quite understand what we are doing here. We should just store the WrappedAccountCryptographicState, not a response model. On sync, the response model should get converted to WrappedAccountCryptographicState.

matt-livefront · 2026-05-15T15:07:07Z

@matt-livefront It's fine if that's out of scope for this PR, but the way iOS is storing the account cryptographic state as a custom model which is different from the public API surface of the SDK is weird and seems brittle / likely to introduce bugs. Is it not possible to just store the SDK's model, or even a string serialized version of it?

Specifically, mobile should not care about the internals of that object or individually set fields on it (i guess with the exception of sync, though we should work on moving that part to the SDK too).

@quexten Thanks for your feedback! In Swift, the easiest way to support persistence is via Codable. The Swift bindings from the SDK don't include this, and it's a fair bit of extra work for us to implement it manually on our side. So I suspect that's how we ended up in this position; our response models are already Codable (for decoding JSON) so it's straightforward to persist those and then transform into the SDK types as needed. But you make a good point, do you know if the SDK could include the Codable conformance? It seems like it could just be a configuration option, but maybe there's more to it. https://mozilla.github.io/uniffi-rs/latest/swift/configuration.html#:~:text=generate_codable_conformance

quexten · 2026-05-18T06:49:17Z

@matt-livefront I don't see a reason not to enable codable on the SDK. We want to keep the amount mobile implements to a minimum, so if this reduces it I support it. I'm opening a PR to enable it for crypto / core, which should cover anything on the key-management front.

## 🎟️ Tracking  bitwarden/ios#2648 (comment) ## 📔 Objective  As per mobile's request, we are enabling codable for the uniffi bindings so that they can directly serialize the structs they get returned from the SDK without having to maintain a seprate copy. Maintaining a separate copy in the past has lead to bugs / incorrect state being saved (bitwarden/ios#2303). This is safer than mobiles current approach of persisting server request / response models, which do not match what the SDK returns. ## 🚨 Breaking Changes

quexten · 2026-05-18T11:06:21Z

@matt-livefront Merged the PR, codable should now be available. Please note I did not test the UNIFFI build locally. I have a separate PR adding swift integration tests, after which we could add those tests into there. Until then, please LMK if there are any issues on this front.

[PM-27238] feat: Support v2 encryption on key connector signups

173b35e

matt-livefront requested a review from quexten May 14, 2026 18:25

matt-livefront requested a review from a team as a code owner May 14, 2026 18:25

matt-livefront added the ai-review Request a Claude code review label May 14, 2026

github-actions Bot added app:password-manager Bitwarden Password Manager app context t:feature labels May 14, 2026

quexten requested changes May 15, 2026

View reviewed changes

quexten mentioned this pull request May 18, 2026

Enable codable on swift bindings bitwarden/sdk-internal#1096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-27238] feat: Support v2 encryption on key connector signups#2648

[PM-27238] feat: Support v2 encryption on key connector signups#2648
matt-livefront wants to merge 1 commit into
mainfrom
matt/PM-27238-key-connector-v2

matt-livefront commented May 14, 2026 •

edited by atlassian Bot

Loading

Uh oh!

github-actions Bot commented May 14, 2026 •

edited

Loading

Uh oh!

quexten left a comment

Uh oh!

Uh oh!

quexten May 15, 2026

Uh oh!

matt-livefront May 15, 2026

Uh oh!

quexten May 18, 2026

Uh oh!

quexten May 15, 2026

Uh oh!

quexten May 15, 2026

Uh oh!

quexten May 15, 2026

Uh oh!

matt-livefront commented May 15, 2026

Uh oh!

quexten commented May 18, 2026

Uh oh!

quexten commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

matt-livefront commented May 14, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

github-actions Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

quexten May 15, 2026

Choose a reason for hiding this comment

Uh oh!

matt-livefront May 15, 2026

Choose a reason for hiding this comment

Uh oh!

quexten May 18, 2026

Choose a reason for hiding this comment

Uh oh!

quexten May 15, 2026

Choose a reason for hiding this comment

Uh oh!

quexten May 15, 2026

Choose a reason for hiding this comment

Uh oh!

quexten May 15, 2026

Choose a reason for hiding this comment

Uh oh!

matt-livefront commented May 15, 2026

Uh oh!

quexten commented May 18, 2026

Uh oh!

quexten commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matt-livefront commented May 14, 2026 •

edited by atlassian Bot

Loading

github-actions Bot commented May 14, 2026 •

edited

Loading