[PM-41073] Remove nested EncStrings on decrypted CipherViews - #1353
[PM-41073] Remove nested EncStrings on decrypted CipherViews#1353nikwithak wants to merge 21 commits into
Conversation
🔍 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. |
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the removal of nested Code Review Details
Broader note (not blocking, no inline comment): this change materially widens the plaintext key surface crossing the WASM/UniFFI boundary — raw CEKs on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| key: attachment.key.clone(), | ||
| #[cfg(feature = "wasm")] | ||
| decrypted_key: None, | ||
| key: None, |
There was a problem hiding this comment.
Details and fix
Previously this branch preserved key: attachment.key.clone() (the wrapped key), so a failure was only about file_name. Now key: None has three downstream effects:
AttachmentView::encryption_version()returnsLegacyNoKeyV1instead ofAttachmentKeyV2.CipherView::to_list_viewcomputeshas_old_attachments = all_attachments().any(|att| att.key.is_none())and chainsattachment_decryption_failures, so any cipher with one failed attachment now reportshas_old_attachments == true.AttachmentFile::decrypttakes theelse(legacy v1) branch and tries to decrypt contents with the user/org key — silently wrong rather than erroring.
Attachment::decrypt can fail on file_name.decrypt while the attachment key unwraps fine, so the key is often still recoverable. Consider unwrapping the key independently of the file name so the failure view keeps a usable key:
Err(e) => {
let key = attachment.key.as_ref().and_then(|k| {
let id = ctx.unwrap_symmetric_key(key, k).ok()?;
#[allow(deprecated)]
Some(ctx.dangerous_get_symmetric_key(id).ok()?.to_base64().to_string())
});
failures.push(AttachmentView { /* ... */ key });
}There was a problem hiding this comment.
Seems reasonable. This is a failed decryption and we cannot reasonable place a key here. Ideally we would have an enum of different struct representations (failed, not failed) but this is OK too.
|
|
||
| /// Temporary, required to support calculating TOTP from CipherListView. | ||
| pub key: Option<EncString>, | ||
| pub key: Option<String>, |
There was a problem hiding this comment.
CipherListView.key now ships the raw content-encryption key to clients but has no remaining consumer.
Details and fix
The removed doc comment said this field existed only "to support calculating TOTP from CipherListView". get_totp_key no longer reads key, and no other code in the workspace reads CipherListView.key (checked totp.rs, totp_client.rs, cipher_client/*, bitwarden-uniffi).
The field is populated in to_list_view and the three *_decrypt_cipher_list_view fns with dangerous_get_symmetric_key(...).to_base64(), so every decrypt_list call now serializes the plaintext CEK for every cipher in the vault across the WASM/UniFFI boundary into a non-zeroizing String.
Since this PR is already a breaking change, removing key from CipherListView avoids the exposure entirely. If a client does still need it, it would be worth calling out explicitly in the breaking-change notes.
| fido2_credentials: self | ||
| .fido2_credentials | ||
| .as_ref() | ||
| .and_then(|c| c.decrypt(ctx, key).ok()), |
There was a problem hiding this comment.
Details and fix
Before this change LoginView.fido2_credentials held the ciphertext, so a credential that failed to decrypt survived a decrypt → edit → encrypt round trip untouched. Now .ok() turns any decryption failure into None, and LoginView::encrypt_composite writes fido2_credentials: None back to the server — the passkey is gone.
This is more destructive than the neighbouring lenient fields: notes/uris/totp failing to decrypt is visible in the UI, whereas a missing passkey is silent. The codebase already tracks this class of failure elsewhere (attachment_decryption_failures, MalformedField).
Consider propagating the error, or surfacing the failure so callers can refuse to overwrite:
fido2_credentials: self
.fido2_credentials
.as_ref()
.map(|c| c.decrypt(ctx, key))
.transpose()?,
| /// Re-encrypt the cipher key(s) using a new wrapping key. | ||
| /// | ||
| /// If the cipher has a cipher key, it will be re-encrypted with the new wrapping key. | ||
| /// Otherwise, the cipher will re-encrypt all attachment keys and FIDO2 credential keys | ||
| /// If any attachment is missing a key, returns an error because the attachment | ||
| /// keys cannot be re-encrypted. | ||
| pub fn reencrypt_cipher_keys( | ||
| &mut self, | ||
| ctx: &mut KeyStoreContext<KeySlotIds>, | ||
| new_wrapping_key: SymmetricKeySlotId, | ||
| _new_wrapping_key: SymmetricKeySlotId, |
There was a problem hiding this comment.
♻️ DEBT: reencrypt_cipher_keys no longer re-encrypts anything and ignores its only parameter.
Details and fix
The body is now just the AttachmentsWithoutKeys validation; _new_wrapping_key is unused. Callers (encrypt_cipher_for_rotation, and any client via move_to_organization) will reasonably assume passing a key does something. Since this PR is already breaking, renaming to something like validate_attachment_keys() and dropping the parameter would make the new "rewrap happens at encrypt time" model explicit.
Same pattern applies to the other placeholder _ctx parameters kept for signature compatibility (totp.rs:88 generate_totp_cipher_view, bitwarden-fido/src/types.rs:73 from_cipher_view, bitwarden-fido/src/lib.rs:78 CipherViewContainer::new) — worth removing in the same breaking pass rather than leaving dead arguments in the public API.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1353 +/- ##
==========================================
- Coverage 86.27% 86.18% -0.09%
==========================================
Files 500 500
Lines 73325 73129 -196
==========================================
- Hits 63259 63028 -231
- Misses 10066 10101 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Draft clients PR Here: bitwarden/clients#22245 (still WIP) |
| ctx: &mut KeyStoreContext<KeySlotIds>, | ||
| wrapping_key: SymmetricKeySlotId, | ||
| ) -> Result<SymmetricKeySlotId, CryptoError> { | ||
| match &self.key { |
There was a problem hiding this comment.
Hopefully can simplify with the follow-up of enabling cipher keys by default.
| // new cipher key happens inside CompositeEncryptable at encrypt time. | ||
| #[allow(deprecated)] | ||
| let raw = ctx.dangerous_get_symmetric_key(new_key)?.clone(); | ||
| self.key = Some(raw.to_base64().to_string()); |
There was a problem hiding this comment.
I think you can just put SymmetricKey instead of string on the FFI struct. It should convert fine, and we made the FFI constructs. That would save you a bunch of conversion.
| let old_key = self.key_identifier(); | ||
|
|
||
| // If any attachment is missing a key we can't reencrypt the attachment keys | ||
| if self.attachments.iter().flatten().any(|a| a.key.is_none()) { |
There was a problem hiding this comment.
Probably want to clean this up at some point, because this does not (in opposition to the naming) actually re-encrypt anything.
… vault/pm-41073
shane-melton
left a comment
shane-melton
left a comment
There was a problem hiding this comment.
Partial review that I'll pick up tomorrow, but things are looking good!
| #[deprecated( | ||
| note = "Use `get_fido2_credentials` instead - Fido2Credentials are no longer encrypted in `CipherView`" | ||
| )] | ||
| pub fn decrypt_fido2_credentials(&self) -> Vec<Fido2CredentialView> { |
There was a problem hiding this comment.
🎨 I think we should be able to just remove this. We're the only consumers of it internally.
| /// The raw private key material for this passkey credential. | ||
| /// Callers that receive `Fido2CredentialView` over a binding boundary should | ||
| /// treat this field with the same care as any private key material. | ||
| pub key_value: String, |
There was a problem hiding this comment.
💭 Hmm, this is makes Fido2CredentialFullView obsolete. We should look at deprecating/removing that view. Maybe not in this PR if it increases the scope too much (IDE is showing around ~44 usages that would need to be updated)
There was a problem hiding this comment.
Good callout - agreed, let's remove it as a follow-up task!
|
@quexten @shane-melton I believe I've addressed all the comments now - ready for another look! |
shane-melton
left a comment
shane-melton
left a comment
There was a problem hiding this comment.
Nice work! I think we're pretty much there, only a few final comments/questions.
| Ok(decrypted) => successes.push(decrypted), | ||
| Err(e) => { | ||
| tracing::warn!(attachment_id = ?attachment.id, error = %e, "Failed to decrypt attachment"); | ||
| let recovered_key = attachment.key.as_ref().and_then(|attachment_key| { |
There was a problem hiding this comment.
👍 Nice, we're still attempting to decrypt the key in case the filename fails above.
| let cipher_key = Cipher::decrypt_cipher_key(ctx, wrapping_key, &view.key)?; | ||
| let blob = CipherBlobLatest::from_cipher_view(view, ctx, cipher_key)?; | ||
| seal_blob_content(blob, cipher_key, ctx) | ||
| let blob = CipherBlobLatest::from_cipher_view(view)?; |
There was a problem hiding this comment.
👍 Glad this is simplified even further and has one fewer level of key shuffling.
| fn seal_blob_content( | ||
| blob: CipherBlobLatest, | ||
| cipher_key: SymmetricKeySlotId, | ||
| wrapping_key: SymmetricKeySlotId, |
There was a problem hiding this comment.
❓ Same here, this should always be a cipher key and renaming so could potentially help avoid confusion?
| }; | ||
|
|
||
| blob.apply_to_cipher_view(&mut view, ctx, cipher_key)?; | ||
| blob.apply_to_cipher_view(&mut view)?; |
There was a problem hiding this comment.
👍 So much nicer not to have to pass a context and key to what was supposed to be a simple mapper. 🎉
| pub archived_date: Option<DateTime<Utc>>, | ||
| pub attachments: Vec<AttachmentView>, | ||
| pub key: Option<EncString>, | ||
| pub key: Option<String>, |
There was a problem hiding this comment.
👍 I think this would be beneficial, ust like you were able to do for the AttachmentView.key
|
|
||
| /// Temporary, required to support calculating TOTP from CipherListView. | ||
| pub key: Option<EncString>, | ||
| pub key: Option<String>, |
There was a problem hiding this comment.
❓ Could we just remove this? Or does it increase the scope? We never encrypt a CipherListView so it should never need the key.
There was a problem hiding this comment.
Good callout - since we no longer need it to decrypt other values, I think we can remove it entirely.
There was a problem hiding this comment.
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
One note: It may be beneficial to split the cipher-key out. Specifically, if the initial theory for the passkey corruption is correct, then this would currently - temporarily - make it worse until the KM side of the key-id tracking is done.
Previously, the encrypted cipher key would fail to decrypt with the wrong user key, and an update was not possible. Now, because the cipher-key is not encrypted at all, thee cipher would be encrypted with the cipher key present for the wrong user-key, so the corruption type would expand to all passkey assertions that happen with a wrong user-key in sdk, rather than just the subset of assertions with a wrong user-key for ciphers without a cipher-key.
Once the KM work is done, this will be prevented either way.
|
Looks like the linter/clippy aren't happy |
|
Still a few more unhappy CI jobs (if you're already on it, please ignore me, I was just watching the branch for my stacked PR) |
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41073
📔 Objective
When decrypting a
Cipherinto aCipherView, we currently leave the following fields in an encrypted state as anEncString, for later decryption. This breaks the contract of theDecryptabletrait, which should leave us with a fully decrypted object. This PR updates those implementations and types to be fully decrypted and exposed when in a decryptedCipherViewstate.Currently in draft, as additional changes may be needed if any issues arise while working on the client PRs.
🚨 Breaking Changes
Yes - this has breaking changes with the SDK, by changing the contracts with the decrypted types. Changes will be needed in all clients currently using the SDK to consume the new approach.
Clients will need to update their usage of Fido2 credentials and TOTP tokens, and can remove the extra step of decrypting them on access, and use the values, which are now decrypted fully, directly. Any clients directly using the cipher key will also need to update that use, as it is no longer wrapped by the user/org key inside a
CipherView.I'll update this with links to those PRs once they are ready.
Clients PR (DRAFT / WIP): bitwarden/clients#22245