[PM-33107] Blob encryption key rotation migration#1188
Conversation
…tation is always V2/blob
shane-melton
requested review from
mzieniukbw and
nick-livefront
and removed request for
a team
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR extends key rotation to upgrade every individual-vault cipher to the sealed-blob format, since rotation always lands the account on V2. The three-case branching in Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn decrypt_for_blob_upgrade( | ||
| cipher: &bitwarden_vault::Cipher, | ||
| current_key: SymmetricKeySlotId, | ||
| new_key: SymmetricKeySlotId, | ||
| ctx: &mut KeyStoreContext<KeySlotIds>, | ||
| ) -> Result<CipherView, DataReencryptionError> { | ||
| if cipher.key.is_some() { | ||
| let mut rewrapped = cipher.clone(); | ||
| rewrapped | ||
| .rewrap_cipher_key(current_key, new_key, ctx) | ||
| .map_err(|_| DataReencryptionError::CipherKeyRewrap)?; | ||
| rewrapped | ||
| .decrypt(ctx, new_key) | ||
| .map_err(|_| DataReencryptionError::Decryption) | ||
| } else { | ||
| cipher | ||
| .decrypt(ctx, current_key) | ||
| .map_err(|_| DataReencryptionError::Decryption) | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: Should the blob upgrade path use strict decryption to avoid silently losing data on per-field decryption errors?
Details
Both branches of decrypt_for_blob_upgrade go through Cipher::decrypt, which routes legacy ciphers through lenient_decrypt_cipher_view. That helper applies .ok().flatten() to each field (login, notes, card, password_history, etc.), so a field that fails to decrypt is silently turned into None and then re-sealed as None in the new blob — permanently discarding the original ciphertext once the rotation payload is uploaded.
For keyless-legacy ciphers this is the same exposure that existed before. The new risk is for keyed-legacy ciphers: previously they only had their cipher key rewrapped (ciphertext preserved bit-for-bit), but they now also go through lenient decrypt + re-seal. A keyed-legacy cipher with a single corrupt field will round-trip into a blob with that field nulled out.
StrictDecrypt<Cipher> already exists in bitwarden-vault for precisely this reason (used by edit/share flows). Is it intentional to keep rotation on the lenient path, or should the rotation crate adopt strict decryption so a single bad field aborts the rotation instead of silently rewriting the cipher?
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## vault/pm-32695/blob-encrypt #1188 +/- ##
===============================================================
+ Coverage 84.75% 84.82% +0.06%
===============================================================
Files 448 448
Lines 60594 60680 +86
===============================================================
+ Hits 51359 51472 +113
+ Misses 9235 9208 -27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shane-melton
changed the title
2 participants
🎟️ Tracking
PM-33107
Stacked on top of #1122 (PM-32695 Cipher blob encryption).
📔 Objective
Make key rotation upgrade every individual-vault cipher to the sealed-blob format. Key rotation always lands the account on the V2 security state, so all individual ciphers must end up blob-encrypted afterward.
reencrypt_ciphersnow handles three cases:Supporting changes:
decrypt_blob_ciphernow takes an explicitwrapping_keyslot instead of always deriving it fromcipher.key_identifier(). During rotation the CEK is wrapped under aLocalslot holding the new user key, so the wrapping key can no longer be assumed to be the user/organization key.EncryptMode::Blobroutes throughencrypt_blob_cipher_with_wrapping_key, respecting the caller-supplied key slot so rotation can target the new-user-key slot.EncryptModeandCipher::is_blob_encrypted()are made public for use by the rotation crate.🚨 Breaking Changes
None for clients.
EncryptModeandCipher::is_blob_encrypted()become public, anddecrypt_blob_ciphergains awrapping_keyparameter (crate-internal).