[PM-21862] Provide SDK with account keys #1873

fedemkr · 2025-08-21T20:49:25Z

🎟️ Tracking

📔 Objective

Implement passing AEAD (Authenticated Encryption with Associated Data) keys to the SDK crypto initialization and storing them.

Note

SDK Crypto Re-initialization from the sync profile response keys can't be done yet given we lack the crypto method necessary data to do so, therefore it's going to be done in a future PR when the SDK updates the API to allow a different way to update such keys.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…crypto initialization correctly.

github-actions · 2025-08-21T20:57:31Z

Checkmarx One – Scan Summary & Details – e88fbe74-fade-4f2f-95cc-70bc40a325f2

Great job! No new security vulnerabilities introduced in this pull request

quexten

Nice work!

Some nits, and a question around the keychain. I don't think we need to store the signing key and the security state in the keychain. The security state is signed, by the signing key, and the signing key is encrypted by the userkey, and they are safe to store in the same way as the user private key, which - as far as I can tell - is not in the keychain.

quexten · 2025-08-22T14:09:38Z

BitwardenShared/Core/Platform/Utilities/KeyManagementTypes.swift

+typealias SignedPublicKey = String
+
+/// A public key in base64 encoded SPKI-DER.
+typealias UnsignedPublicKey = [UInt8]


Question: Should this be string? I'm not exactly sure how the conversion from response model to bytes works in swift.

It should work but we could just try it when we have the server part deployed somewhere I can target and check the response. I also could try locally if it's urgent to know.
Additionally, if you have a serialized json response I could use, I can quickly check this in a test.

quexten · 2025-08-22T14:24:34Z

BitwardenShared/Core/Platform/Services/CryptoClient.swift

+                userId: account.profile.userId,
+                kdfParams: account.kdf.sdkKdf,
+                email: account.profile.email,
+                privateKey: encryptionKeys.encryptedPrivateKey,


Nit (non-blocking): This should probably already use the accountKeys.publicKeyEncryptionKeyPair.wrappedPrivateKey if available. These keys are the same, but I imagine the local state will in the future drop encryptionKeys.encryptedPrivateKey?

It's already using that as when initializing the AccountEncryptionKeys I set the encryptedPrivateKey to the one in accountKeys if present, and to privateKey otherwise.
Would we want to maintain them separate for some reason or can I still reuse the same property for both private keys depending on the context?

I think in the medium term we can drop encryptionKeys.encryptedPrivateKey entirely, since it's the same as accountKeys.publicKeyEncryptionKeyPair.wrappedPrivateKey

Yes we can definitely drop in a future PR given that now we'd need to do a migration in order to move the stored encryptionKeys.encryptedPrivateKey to accountKeys.publicKeyEncryptionKeyPair.wrappedPrivateKey and we wouldn't gain much now.

quexten · 2025-08-22T14:25:31Z

BitwardenShared/Core/Platform/Services/KeyConnectorService.swift

@@ -131,6 +131,7 @@ extension DefaultKeyConnectorService: KeyConnectorService {

        try await stateService.setAccountEncryptionKeys(
            AccountEncryptionKeys(
+                accountKeys: nil,


Nit (not-blocking): I think we can already store privateKeyResponseModel here by constructing it from the existing private key. That would allow us to drop the encryptedPrivateKey property quicker.

quexten · 2025-08-22T14:29:19Z

BitwardenShared/Core/Auth/Services/KeychainRepository.swift

@@ -8,6 +9,9 @@ enum KeychainItem: Equatable {
    /// The keychain item for a user's access token.
    case accessToken(userId: String)

+    /// The keychain item for a user's account security state.
+    case accountSecurityState(userId: String)


AccountSecurityState and UserSigningKey don't need to be protected by the keychain. As far as I understand, we don't store the userPrivateKey here either. They should be stored the same way as userPrivateKey.

…ion keys update.

codecov · 2025-08-27T16:21:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.65%. Comparing base (f131eb5) to head (269fcc2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1873   +/-   ##
=======================================
  Coverage   88.64%   88.65%           
=======================================
  Files         810      811    +1     
  Lines       51124    51158   +34     
=======================================
+ Hits        45321    45353   +32     
- Misses       5803     5805    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

quexten

Changes look reasonable. My only question is around the type of public key in the state here. I'm not sure if this is intended.

quexten · 2025-09-01T16:15:47Z

BitwardenShared/Core/Vault/Services/API/Fixtures/syncWithAccountKeysV2Profile.json

+    "accountKeys": {
+      "publicKeyEncryptionKeyPair": {
+        "wrappedPrivateKey": "WRAPPED_PRIVATE_KEY",
+        "publicKey": [1, 2, 3, 4, 5],


Question / note: This is a base64 encoded string on the server https://github.com/bitwarden/server/blob/8a39481fec87848e19cdd92d620d3e23b97901cb/src/Api/KeyManagement/Models/Requests/PublicKeyEncryptionKeyPairRequestModel.cs#L9. Is it intended to be an array of bytes here?

Nice catch! You're right, I saw an object in the clients repo with that type and I mistakenly applied it here directly missing it was being converted from a string in the response. Fixed and removed the UnsignedPublicKey typealias as it's no longer use.

…n UnsignedPublicKey and removed this typealias as it's not used in iOS.

matt-livefront · 2025-09-04T21:36:19Z

...hared/Core/Vault/Models/Response/KeyManagement/PublicKeyEncryptionKeyPairResponseModel.swift

+    /// The wrapped private key.
+    let wrappedPrivateKey: WrappedPrivateKey
+
+    /// The public key.
+    let publicKey: String
+
+    /// The signed public key.
+    let signedPublicKey: SignedPublicKey?


⛏️ Do we want to alphabetize these similar to other models?

matt-livefront · 2025-09-04T21:43:33Z

BitwardenShared/Core/Auth/Repositories/AuthRepository.swift

+            let accountKeys = try? await stateService.getAccountEncryptionKeys()
+            accountPrivateKeys = accountKeys?.accountKeys


🤔 Do you think this would end up throwing in the expected use cases? Would it better to use try instead of try? here? If so, we could also move getting the encryption keys outside if the if since we do that in both cases.

matt-livefront · 2025-09-04T21:44:31Z

BitwardenShared/Core/Auth/Repositories/AuthRepository.swift

@@ -604,6 +604,7 @@ extension DefaultAuthRepository: AuthRepository {

        try await stateService.setAccountEncryptionKeys(
            AccountEncryptionKeys(
+                accountKeys: nil,


🤔 Is it possible to construct a PrivateKeysResponseModel here with the keys we have or does the server need to do that? If I'm understanding correctly, I think this is similar to #1873 (comment).

PM-21862 Implemented adding user v2 account keys to be passed to the …

7c853a8

…crypto initialization correctly.

fedemkr requested a review from quexten August 21, 2025 20:49

quexten reviewed Aug 22, 2025

View reviewed changes

fedemkr added 4 commits August 25, 2025 18:09

Merge branch 'main' into PM-21862/provide-signing-keys-sdk

88e369d

PM-21862 Clear unneeded code and updated sync profile account encrypt…

7abc904

…ion keys update.

Merge branch 'main' into PM-21862/provide-signing-keys-sdk

fcdc70f

PM-21862 Fixed tests for account keys and some warnings.

f0d0b82

fedemkr added 2 commits August 27, 2025 17:50

PM-21862 Clear code and added tests

f6efd45

Merge branch 'main' into PM-21862/provide-signing-keys-sdk

f85a034

fedemkr marked this pull request as ready for review August 27, 2025 20:51

fedemkr requested review from matt-livefront and a team as code owners August 27, 2025 20:51

fedemkr requested a review from quexten August 27, 2025 20:51

quexten reviewed Sep 1, 2025

View reviewed changes

fedemkr added 2 commits September 1, 2025 16:44

Merge branch 'main' into PM-21862/provide-signing-keys-sdk

fbe9e25

PM-21862 Fix publicKey type from response to be a String instead of a…

269fcc2

…n UnsignedPublicKey and removed this typealias as it's not used in iOS.

fedemkr requested a review from quexten September 1, 2025 20:25

matt-livefront reviewed Sep 5, 2025

View reviewed changes

		let accountKeys = try? await stateService.getAccountEncryptionKeys()
		accountPrivateKeys = accountKeys?.accountKeys

[PM-21862] Provide SDK with account keys #1873

Are you sure you want to change the base?

[PM-21862] Provide SDK with account keys #1873

Conversation

fedemkr commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

quexten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fedemkr commented Aug 21, 2025 •

edited

Loading

github-actions bot commented Aug 21, 2025 •

edited

Loading

codecov bot commented Aug 27, 2025 •

edited

Loading