refactor(crypto): replace legacy aes.rs with hazmat primitives - #1282
Conversation
| impl Aes256CbcHmacKey { | ||
| /// Returns the 64-byte composite key (`enc_key || mac_key`) in the layout expected by the | ||
| /// [`Aes256CbcHmacSha256`](crate::hazmat::symmetric_encryption::Aes256CbcHmacSha256) cipher. | ||
| pub(crate) fn to_composite_key(&self) -> Zeroizing<[u8; 64]> { |
There was a problem hiding this comment.
Future PR will probably replace the internal representation to not carry two distinct subkeys to eliminate copying here.
🔍 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1282 +/- ##
==========================================
- Coverage 85.79% 85.78% -0.02%
==========================================
Files 487 489 +2
Lines 69999 70054 +55
==========================================
+ Hits 60059 60096 +37
- Misses 9940 9958 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR removes the legacy Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| EncString::Aes256Cbc_B64 { iv, ref data } => { | ||
| let legacy_key = self.0.clone(); | ||
| crate::aes::decrypt_aes256(&iv, data.clone(), &legacy_key) | ||
| Aes256Cbc::decrypt(&iv, data, &(*self.0).into()) |
There was a problem hiding this comment.
🎨 SUGGESTED: (*self.0).into() materializes an unzeroized [u8; 32] copy of the key on the stack.
Details and fix
(*self.0).into() (and the equivalent (**key).into() in master_key.rs:176) copies the 32-byte key onto the stack as a bare [u8; 32] that is not zeroized on drop. The previous aes.rs path cloned the heap-allocated Pin<Box<Array<u8, U32>>>, which the ZeroizingAllocator wiped on drop.
The to_composite_key() helper added in this same PR deliberately returns Zeroizing<[u8; 64]> for exactly this reason, so wrapping these legacy-CBC copies for consistency would keep the zeroization guarantee uniform:
Aes256Cbc::decrypt(&iv, data, &Zeroizing::new((*self.0).into()))Low severity — the exposure is transient and decrypt-only — but it restores the zeroization behavior the old code had and matches the pattern used elsewhere in this PR.
c2ed0f5 to
c4f76af
Compare
Route all AES-256-CBC and AES-256-CBC-HMAC-SHA256 operations through the hazmat::symmetric_encryption primitives and delete the private aes module; the on-wire format is unchanged. IV generation for the HMAC path moves into EncString::encrypt_aes256_hmac, and the 64-byte composite key is assembled via a new Aes256CbcHmacKey::to_composite_key helper.
c4f76af to
18c4f36
Compare
|
Given we have good coverage no AES primitives; merging. |
… replace legacy aes.rs with hazmat primitives (bitwarden/sdk-internal#1282)
https://bitwarden.atlassian.net/browse/PM-40756
Replace the legacy aes implementation with the hazmat module based implementations.