[PM-32211] fix private key before key rotation#994
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #994 +/- ##
==========================================
+ Coverage 84.44% 84.48% +0.04%
==========================================
Files 420 423 +3
Lines 51943 52505 +562
==========================================
+ Hits 43861 44358 +497
- Misses 8082 8147 +65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…unction, regeneration without api call for key rotation
🔍 SDK Breaking Change DetectionSDK Version:
| typescript | | android |
|
# Conflicts: # crates/bitwarden-user-crypto-management/src/key_rotation/password_change_and_rotate_user_keys.rs # crates/bitwarden-user-crypto-management/src/key_rotation/rotate_user_keys.rs
|
Howdy @mzieniukbw et. al 👋🏼 I am field testing the new multi-agent code review, I gave it a local run against the PR. The findings are below for your analysis. Please incorporate the findings that are true signals. We are also looking closely into the difference in the way that Claude Code runs the review given different models for pros, cons, etc. We are going to continue to refine the context delivered to Claude during this reviews which may also include information from the Jira ticket itself to help ground Claude in reality. Many Thanks! Opus Code Review: [PM-32211] fix private key before key rotation (#994)Date: 2026-05-07 | Reviewed by: Claude Code | Model: Opus Summary
This PR introduces the SDK-side private key regeneration flow that precedes key rotation. The new Findings
|
| Severity | Count |
|---|---|
| 🛑 Blocker | 2 |
| 3 | |
| ♻️ Refactor | 2 |
The PR introduces a critical bug that defeats its own purpose: users who need private key
regeneration will fail key rotation with the same cryptographic error after regeneration, because
the stale pre-regeneration sync state is passed unchanged to the internal rotation functions. The
new regeneration logic also raises two architectural security concerns: a compromised server can
silently trigger key pair replacement (P01), and regeneration within key rotation fires without any
user-visible signal or consent gate (P05). Two ♻️ Refactor findings address documentation and API
surface concerns.
Findings
🛑 Blockers
Stale sync state passed to rotation after key pair regeneration causes rotation failure
crates/bitwarden-user-crypto-management/src/key_rotation/rotate_user_keys.rs:72 Caught by: Bug
analysis agent
Details
In the new rotate_user_keys public method, sync_current_account_data is called once, then
regenerate_public_key_encryption_key_pair_if_needed_with_ciphers is called with &sync.ciphers,
and finally the same unmodified sync is passed to internal_rotate_user_keys. If regeneration
actually occurred — the intended recovery path for a corrupt or missing private key —
sync.wrapped_account_cryptographic_state still contains the old encrypted private key fetched
from the server before regeneration. The return value of regenerate_...if_needed_with_ciphers
(Option<WrappedAccountCryptographicState>) is discarded via .map_err(...)?. When
internal_rotate_user_keys then calls
rotate_account_cryptographic_state_to_wrapped_model(&sync.wrapped_account_cryptographic_state, ...),
the decryption of the old corrupt private key fails, returning RotateUserKeysError::Crypto. The
regeneration succeeded (new key posted to server, key store slot updated), so the rotation fails on
precisely the user state this PR is designed to fix.
WrappedAccountCryptographicState::get_v1_from_key_store was added in this same PR and is the
correct tool for re-deriving the state from the key store after regeneration — but it is not called
here.
Implicates EK and P06 (minimized impact of security breaches: the recovery mechanism is
silently broken).
Fix: Capture the return value of regenerate_...if_needed_with_ciphers. If Some(new_state) is
returned, set sync.wrapped_account_cryptographic_state = new_state before passing sync to the
internal function:
if let Some(new_state) = self
.regenerate_public_key_encryption_key_pair_if_needed_with_ciphers(&sync.ciphers)
.await
.map_err(|_| RotateUserKeysError::CryptoError)?
{
sync.wrapped_account_cryptographic_state = new_state;
}Same stale sync state bug in password_change_and_rotate_user_keys causes rotation failure after regeneration
crates/bitwarden-user-crypto-management/src/key_rotation/password_change_and_rotate_user_keys.rs:56
Caught by: Bug analysis agent
Details
Identical root cause to the previous finding. In the new password_change_and_rotate_user_keys
public method, sync is fetched once, regeneration is called (return value discarded), then the
stale sync is passed to internal_password_change_and_rotate_user_keys. The internal function
calls
rotate_account_cryptographic_state_to_request_model(&sync.wrapped_account_cryptographic_state, ...),
which attempts to decrypt the old (potentially corrupt) private key from sync. This fails, leaving
users who needed regeneration unable to change their password and rotate keys — the exact scenario
this PR targets.
Implicates EK and P06.
Fix: Same as above — propagate Some(new_state) from regeneration into
sync.wrapped_account_cryptographic_state before calling the internal function.
⚠️ Important
Public CryptoClient methods removed without migration path documentation
crates/bitwarden-core/src/key_management/crypto_client.rs:69 Caught by: Architecture agent
Details
The diff removes two pub fn methods from CryptoClient — make_key_pair and
verify_asymmetric_keys — both compiled into the WASM bindings via the
#[cfg_attr(feature = "wasm", wasm_bindgen)] on the impl block. CLAUDE.md states explicitly:
"Breaking changes: Automated detection via cross-repo workflow. TypeScript compilation tested
againstclientsrepo on PR. Document migration path for clients."
The diff contains no CHANGELOG entry, no deprecation notice prior to removal, and no documentation
of what callers should use instead. The automated cross-repo workflow will catch compilation
failures in clients, but the project rule requires documenting the migration path independently of
whether CI blocks.
Fix: Add a CHANGELOG entry noting these methods were removed as redundant (key pair management
is now handled via regenerate_public_key_encryption_key_pair_if_needed), and document the
migration path for any callers.
Server-controlled data can silently trigger RSA key pair replacement (P01)
crates/bitwarden-user-crypto-management/src/public_key_encryption_key_pair_regeneration/should_regenerate.rs:105
Caught by: Security & logic agent
Details
The check_key_pair function determines whether to regenerate the user's RSA key pair entirely from
data returned by the server (GET /accounts/keys). A malicious or compromised server can trigger
silent key regeneration by returning any of:
(None, None)for public/private keys- A partial key pair (one present, one absent)
- An unparseable
EncStringfor the private key - A public key string that doesn't match the real one on the server
In each case the client silently replaces the user's RSA key pair without user consent, warning, or
fallback check against a locally cached key pair state.
This violates P01 (Servers are Zero Knowledge): the server must not be able to force replacement
of client-side key material. Replacing the RSA key pair invalidates all data previously encrypted to
the user's public key — specifically org-shared vault items, emergency access grants, and trusted
device keys. A compromised server could force every unlocked V1 user to replace their RSA key pair
on the next rotation attempt, breaking all org-shared data access silently.
Implicates EK: encryption key regeneration must not be triggerable by an untrusted party.
Suggested mitigations:
- Before regenerating based on
(None, None), compare against the locally stored
WrappedAccountCryptographicState.private_key— regenerate only if local state is also absent or
locally verifiable as corrupt. - If no local state is available to cross-reference, require explicit user acknowledgement before
replacing a key pair. - At minimum, treat a
(None, None)response from the server skeptically when the user has
previously been initialized with a key pair.
RSA key pair regeneration fires silently within key rotation — no user visibility or consent (P05)
crates/bitwarden-user-crypto-management/src/public_key_encryption_key_pair_regeneration/mod.rs:81
Caught by: Security & logic agent
Details
In both password_change_and_rotate_user_keys and rotate_user_keys,
regenerate_public_key_encryption_key_pair_if_needed_with_ciphers is called silently before
rotation proceeds. If regeneration fires, the user's RSA key pair is replaced without any signal to
the user — no warning, no prompt, no indication in the rotation functions' return values or error
paths.
Replacing the RSA key pair has significant downstream consequences: all existing org-shared vault
items (re-encrypted to the old public key by org admins), emergency access grants, and trusted
device authorizations that reference the old public key are now effectively invalidated. The PR
description acknowledges a coordinated server-side PR must "handle status changes for emergency
access/organizations" — meaning the client silently performs an action whose full consequences
depend on a server-side deployment.
Under P05 (Controlled Access to Vault Data): vault data must be accessible only under the user's
explicit control. Silently replacing the key that org admins used to share data removes the user's
awareness and control over when that action occurs.
Fix: The public regenerate_public_key_encryption_key_pair_if_needed already returns
Option<WrappedAccountCryptographicState> — Some(state) when regeneration was performed. The
rotation callers should propagate this signal to the application layer, so client applications can
notify users that their RSA key pair was regenerated and that org/emergency-access relationships may
need re-establishment.
♻️ Refactor
Doc on _with_ciphers variant contradicts its call sites regarding state persistence
crates/bitwarden-user-crypto-management/src/public_key_encryption_key_pair_regeneration/mod.rs:81
Caught by: Code quality agent
Details
The regenerate_public_key_encryption_key_pair_if_needed_with_ciphers method's docstring inherits
the contract "Callers should persist the returned state to their local account cryptographic state."
However, both call sites in rotate_user_keys.rs and password_change_and_rotate_user_keys.rs drop
the returned Option<WrappedAccountCryptographicState> entirely — intentionally, because
internal_regenerate_public_key_encryption_key_pair already calls persist_private_key to update
the key store in-place, and the subsequent rotation will produce a fresh wrapped state.
This creates a misleading contract for future callers of the pub(crate) method.
Fix: Clarify the docstring to distinguish contexts: the key store is always updated in-place;
the returned Option<WrappedAccountCryptographicState> only needs to be persisted to the account's
durable storage when the caller will not subsequently rotate keys (i.e., the standalone
regenerate_public_key_encryption_key_pair_if_needed path).
Users with no personal ciphers silently skip needed key pair regeneration
crates/bitwarden-user-crypto-management/src/public_key_encryption_key_pair_regeneration/should_regenerate.rs:195
Caught by: Security & logic agent
Details
In is_user_key_valid, when the private key cannot be decrypted and no personal ciphers exist (user
has only org ciphers), the function returns Ok(false) — "do not regenerate." The function cannot
distinguish between "user key is wrong" and "private key is corrupt" without a personal cipher to
test against. The conservative behavior is a warn! + skip.
Consequence: users with a corrupt private key and no personal ciphers will silently proceed into a
failed key rotation with a cryptographic error and no actionable diagnostic. They cannot resolve the
corrupt private key via this flow.
The PR's stated goal is to "fix corrupt private keys before key rotation." Silently skipping
regeneration for this user subset undermines that goal without surfacing any information to the
caller about why.
Fix: Return a distinct error variant (e.g., KeyPairRegenerationError::CannotValidateUserKey)
when the private key is undecryptable and no validation cipher is available. Callers can then
surface a meaningful diagnostic to the user ("your private key appears corrupt but we cannot safely
regenerate it because no personal vault items are available to validate the user key").
Reviewed and Dismissed
🔍 4 initial findings dismissed after validation
make_v1 promoted from #[cfg(test)] to pub(crate) — test factory leaked into production compilation
crates/bitwarden-core/src/key_management/account_cryptographic_state.rs:353 Caught by:
Architecture agent Original severity: ♻️ Refactor Original confidence: 83/100 Dismissed
at: Step 4 validation Dismissed because: The #[cfg(test)] attribute is preserved in the diff
— the change is from fn make_v1 to pub(crate) fn make_v1 while #[cfg(test)] remains on the
function. The method is still only compiled in test builds and does not enter production-compiled
code. The finding's central claim was factually incorrect.
is_v1_symmetric_key added to KeyStoreContext breaks inspection pattern
crates/bitwarden-crypto/src/store/context.rs:400 Caught by: Architecture agent Original
severity: ♻️ Refactor Original confidence: 80/100 Dismissed at: Step 4 validation
Dismissed because: The method is used at four or more call sites across
account_cryptographic_state.rs, regenerate.rs, and should_regenerate.rs. It encapsulates the
V1-specific algorithm comparison in a clearly named predicate, reducing coupling to the internal
SymmetricKeyAlgorithm::Aes256CbcHmac enum variant. A senior engineer would not flag a well-used
helper as refactor-worthy tech debt.
Tests for unimplemented KeyConnector/TDE rotation deleted without replacement
crates/bitwarden-user-crypto-management/src/key_rotation/rotate_user_keys.rs:57 Caught by:
Code quality agent Original severity:
at: Step 5 severity audit Dismissed because:
test_rotate_user_keys_key_connector_returns_unimplemented and
test_rotate_user_keys_tde_returns_unimplemented are present in the post-PR file at lines 272–331,
still calling internal_rotate_user_keys where the guard for these methods remains. No coverage gap
exists.
Sync API failure test deleted after moving sync call to public method
crates/bitwarden-user-crypto-management/src/key_rotation/rotate_user_keys.rs:80 Caught by:
Code quality agent Original severity:
at: Step 5 severity audit Dismissed because: Both tests are present in the post-PR files.
test_rotate_user_keys_api_failure_returns_api_error exists at lines 333–365 in
rotate_user_keys.rs, and
test_password_change_and_rotate_user_keys_sync_api_failure_returns_api_error exists at lines
255–286 in password_change_and_rotate_user_keys.rs. The diff removed old test implementations and
new equivalents were added as part of the refactor.
Code Review Comparison: PR #994 — Opus vs. Sonnet
| Opus | Sonnet | |
|---|---|---|
| 🛑 Blocker | 0 | 2 |
| 7 | 3 | |
| ♻️ Refactor | 2 | 2 |
| Dismissed | 4 | 4 |
Finding Overlap
Both models identified:
- Stale sync state passed to rotation after regeneration (the core bug)
- Server-controlled data can force key pair replacement (P01)
- No personal ciphers → silent skip of needed regeneration
- Silent regeneration with no user consent gate (P05)
Unique to Opus
POST /regenerate-keys succeeds, local persist fails → client desynced from server
(regenerate.rs:55-72)
The two-phase structure (generate → POST → persist) has a gap: if persist_private_key fails after the API call commits, the server has the new key pair but the client key store doesn't. Caller gets a generic CryptoError with no signal to re-sync. Opus proposed a distinct error variant. Sonnet missed this entirely.
is_user_key_valid* function names contradict their return semantics
(should_regenerate.rs:157-230)
Both functions are named is_user_key_valid but return should_regenerate — a decision, not a property. Opus flagged the naming inversion as a correctness trap for future readers. Sonnet missed this entirely.
is_v1_symmetric_key leaks account-layer vocabulary into bitwarden-crypto
(context.rs:665-670)
Opus kept this as a Refactor with substantive reasoning: "V1/V2" are account-state concepts that don't belong in the crypto primitives crate; the README explicitly says to limit public interfaces; and no is_* methods exist anywhere else on KeyStoreContext. Sonnet dismissed it as having "4+ call sites."
Unique to Sonnet
make_key_pair and verify_asymmetric_keys removed without migration path docs
(crypto_client.rs:69)
These were pub fn in a wasm_bindgen impl block. CLAUDE.md requires documenting the migration path for breaking changes. Opus missed this entirely.
Key Disagreements
Stale sync state bugs: Blocker (Sonnet) vs. Important (Opus)
Sonnet classified these as Blockers. Opus classified them as Important with a key nuance: "A retry works (sync now returns the regenerated, decryptable key), but the user receives an error mid-flow after server state was already mutated." The operation is survivable on retry but harmful for the user. Opus's Important rating is more precisely calibrated. Sonnet's Blocker label isn't wrong — the PR's stated goal is defeated on the first call — but Opus's nuance is more useful.
No-personal-ciphers path: Important (Opus) vs. Refactor (Sonnet)
Opus: users with only org ciphers and a corrupt private key silently fail rotation with no recovery path — a correctness violation of the PR's stated goal. Sonnet: "intentional conservative choice," downgraded to Refactor. Opus is correct here. The PR explicitly claims to fix corrupt private keys before rotation; this is a hole in that claim, not stylistic debt.
is_v1_symmetric_key in bitwarden-crypto: Refactor (Opus) vs. dismissed (Sonnet)
Opus's argument (vocabulary leak across architectural layers, README rule, naming convention break) is stronger than Sonnet's dismissal ("4+ call sites make it legitimate"). Call-site count doesn't rebut an architectural boundary violation.
KeyConnector/TDE tests deleted without replacement: Important (Opus) vs. dismissed (Sonnet)
Sonnet's severity audit agent claimed the tests are present in the post-PR file at lines 272–331. Opus kept the finding. This is a factual disagreement — one of them read the post-PR file state incorrectly.
Overall Assessment
Opus produced a materially better review.
It found two real findings Sonnet missed (desync after API commit, naming inversion), gave more precise reasoning on dismissals, correctly rated the no-ciphers path as Important rather than Refactor, and its stale-sync severity calibration is more nuanced. The one finding Sonnet caught that Opus missed (migration docs) is real but lower-stakes than Opus's unique findings.
Sonnet's Blocker labels on the stale-sync bugs are defensible but imprecise. Its dismissal of is_v1_symmetric_key and the no-ciphers downgrade both represent accuracy losses vs. Opus.
The desync finding (Opus only) is the most significant gap: it describes a durable client-server state inconsistency that Sonnet's entire pipeline failed to surface.
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-32211
Clients PR: bitwarden/clients#20392
📔 Objective
Move asymmetric key regeneration (public, private key) into SDK.
A user may have a corrupt private key. It needs to be fixed before they can rotate their keys or upgrade to v2 encryption. For this, we need to invoke the private key regeneration function (this PR) and handle status changes for emergency access/organizations (server PR).
Other changes:
CryptoClient'smake_key_pairandverify_asymmetric_keys🚨 Breaking Changes