[PM-29208] Remove individual cryptographic-key states & migrate key service #18164
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18164 +/- ##
==========================================
+ Coverage 42.28% 42.33% +0.05%
==========================================
Files 3599 3600 +1
Lines 104552 104666 +114
Branches 15782 15811 +29
==========================================
+ Hits 44208 44311 +103
- Misses 58469 58479 +10
- Partials 1875 1876 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ); | ||
| } | ||
|
|
||
| if (tokenResponse.hasMasterKeyEncryptedUserKey()) { |
There was a problem hiding this comment.
Here, the migration is to just set the account cryptographic state conditionally. Private key creation is happening anymore, so there is no need for this special branching logic.
There was a problem hiding this comment.
I think I'm confused here. How will newly JIT provisioned SSO v1 users get key pairs now?
There was a problem hiding this comment.
Newly provisioned JIT TDE users get their keys set in login-decryption-options.component. We currently have the not-yet-rolled-out v2 code (SDK) and the v1 logic. Newly provisioned JIT Password users get their keys set in the setInitialPasswordService. We don't have the V2 (sdk) logic merged yet, but it's in progress.
quexten
force-pushed
the
km/remove-individual-key-states
branch
from
f206a76 to
b3921d5
Compare
quexten
changed the title
quexten
changed the title
quexten
force-pushed
the
km/remove-individual-key-states
branch
from
4425d87 to
dc75f19
Compare
|
Claude finished @quexten's task in 4m 10s —— View job Overall Assessment: REQUEST CHANGES This PR removes four individual cryptographic state keys (privateKey, userSigningKey, userSignedPublicKey, accountSecurityState) and consolidates them into a single accountCryptographicState subscription pattern. The migration is correctly implemented, and the refactoring follows the intended architecture. However, there are critical null safety issues in login strategy implementations. Code Review Details
Migration Quality: The migration correctly removes all four legacy state keys and is properly registered. Test coverage validates the removal logic. Service Injection: The dependency ordering changes are correct - Architecture: The refactoring successfully eliminates redundant state subscriptions and prepares the codebase for SDK service direct subscription to accountCryptographicState. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
This code unconditionally calls .toWrappedAccountCryptographicState() on response.accountKeysResponseModel, but this field can be null according to the type definition in IdentityTokenResponse (line 30: accountKeysResponseModel: PrivateKeysResponseModel | null = null).
When this breaks:
- JIT-provisioned SSO users who don't yet have asymmetric key pairs
- Any scenario where the server doesn't return accountKeys (noted in sync service comment about PM-21768)
Execution path:
- User logs in with password
- Server returns token response with
accountKeysResponseModel = null - Line 156 executes:
response.accountKeysResponseModel.toWrappedAccountCryptographicState() - Throws:
Cannot read property 'toWrappedAccountCryptographicState' of null
Fix required:
Add null check like SSO login strategy does:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}Why SSO strategy is correct: See sso-login.strategy.ts lines 338-348 and test at line 199 which explicitly tests for accountKeysResponseModel = null.
Why this matters: The PR description states this removes the "old path for account private key regen", but the replacement assumes accountKeysResponseModel is always present. Per the sync service comment (line 243), the server doesn't always return this field yet (PM-21768 tracking).
There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
Same null safety issue as password login strategy. This unconditionally accesses response.accountKeysResponseModel.toWrappedAccountCryptographicState() without checking if accountKeysResponseModel is null.
Fix required:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}Reference the SSO login strategy (lines 338-348) for the correct pattern.
There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
| await this.keyService.setPrivateKey( | ||
| response.privateKey ?? (await this.createKeyPairForOldAccount(userId)), | ||
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❌ CRITICAL: Null reference exception when accountKeysResponseModel is null
Details and fix
Same null safety issue. Add null check:
protected override async setAccountCryptographicState(
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
if (response.accountKeysResponseModel) {
await this.accountCryptographicStateService.setAccountCryptographicState(
response.accountKeysResponseModel.toWrappedAccountCryptographicState(),
userId,
);
}
}There was a problem hiding this comment.
This is not correct. Password login strategy never has null account keys, only SSO login does.
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.spec.ts
Outdated
Show resolved
Hide resolved
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.ts
Outdated
Show resolved
Hide resolved
|
Disabling claude since it's just duplicating. |
|
BIT tests are most likely failing due to the outdated server version that BIT is using. I'll check in with autofill about this before merging, but locally everything works. |
Thomas-Avery
left a comment
Thomas-Avery
left a comment
There was a problem hiding this comment.
I like the direction this is headed. Some open concerns/questions from my side to look at.
| await this.accountCryptographicStateService.setAccountCryptographicState( | ||
| response.accountKeysResponseModel.toWrappedAccountCryptographicState(), |
There was a problem hiding this comment.
❓ Has response.accountKeysResponseModel been introduced on the server more than 3 releases ago?
This would break when a newer client targets an older server that doesn't have this property on the IdentityTokenResponse.
If we haven't had 3 releases for the property we should be able to use response.privateKey as a fallback if response.accountKeysResponseModel doesn't exist.
This feedback applies to all login strategies.
There was a problem hiding this comment.
It has been introduced in 2025.11.0. I'm unsure what 3 releases actually means (do patch releases count), but if we target this for merging after the 2026.1.0 rc cut then we should be good, right?
The alternative is re-constructing the accountCryptographicState from the private key in a few places, but I'm unsure if that's actually needed if we are good to remove the previous property soon anyways.
| ); | ||
| } | ||
|
|
||
| if (tokenResponse.hasMasterKeyEncryptedUserKey()) { |
There was a problem hiding this comment.
I think I'm confused here. How will newly JIT provisioned SSO v1 users get key pairs now?
| const key = await helper.getFromUser(userId, userEncryptedPrivateKey); | ||
| if (key != null) { | ||
| await helper.removeFromUser(userId, userEncryptedPrivateKey); | ||
| } | ||
| // Remove userSigningKey | ||
| const signingKey = await helper.getFromUser(userId, userKeyEncryptedSigningKey); | ||
| if (signingKey != null) { | ||
| await helper.removeFromUser(userId, userKeyEncryptedSigningKey); | ||
| } | ||
| // Remove userSignedPublicKey | ||
| const signedPubKey = await helper.getFromUser(userId, userSignedPublicKey); | ||
| if (signedPubKey != null) { | ||
| await helper.removeFromUser(userId, userSignedPublicKey); | ||
| } | ||
| // Remove accountSecurityState | ||
| const accountSecurity = await helper.getFromUser(userId, accountSecurityState); | ||
| if (accountSecurity != null) { | ||
| await helper.removeFromUser(userId, accountSecurityState); | ||
| } |
There was a problem hiding this comment.
Should we be migrating this data into the accountCryptographicState? Feels like there might be some edge cases here were the sync/login won't be able to set the new state after we wipe this out. Offline unlock?
There was a problem hiding this comment.
My assumption was that users that were offline for an extended amount of time would also not have their clients updated and/or have invalid tokens so when they do come online they would not be able to sync.
However, the account cryptographic state service was only merged 2 weeks ago, so I added the migrations now.
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.spec.ts
Outdated
Show resolved
Hide resolved
libs/state/src/state-migrations/migrations/75-remove-user-encrypted-private-key.ts
Show resolved
Hide resolved
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and all feature flags disabled. ✅ Fortunately, these BIT tests have passed! 🎉 |
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and the feature flag configuration used by ✅ Fortunately, these BIT tests have passed! 🎉 |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29208
📔 Objective
This PR removes the following individual cryptographic states:
and replaces all subscriptions to these states, with subscriptions to the account cryptography state.
These temporary mappings can be removed once the SDK service subscribes directly to the account cryptographic state, and their services can be removed.
Please note, this also drops the old path for account private key regen, if an account does not have a private key. This can instead be done in the Data Recovery tool.
📸 Screenshots
⏰ Reminders before review
🦮 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