[PM-18102] Use opaque autogenerated local key ids #274
Conversation
dani-garcia
changed the title
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #274 +/- ##
==========================================
- Coverage 78.37% 78.36% -0.01%
==========================================
Files 291 291
Lines 29391 29343 -48
==========================================
- Hits 23034 22994 -40
+ Misses 6357 6349 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # crates/bitwarden-crypto/src/store/context.rs # crates/bitwarden-crypto/src/traits/key_id.rs
# Conflicts: # crates/bitwarden-crypto/src/store/context.rs # crates/bitwarden-vault/src/cipher/attachment.rs # crates/bitwarden-wasm-internal/src/pure_crypto.rs
|
# Conflicts: # crates/bitwarden-core/src/key_management/crypto.rs # crates/bitwarden-crypto/src/store/context.rs # crates/bitwarden-vault/src/cipher/cipher.rs # crates/bitwarden-wasm-internal/src/pure_crypto.rs
|
# Conflicts: # crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs
| // New user key | ||
| let user_key = SymmetricCryptoKey::make_xchacha20_poly1305_key(); | ||
| #[allow(deprecated)] | ||
| ctx.set_symmetric_key(temporary_user_key_id, user_key.clone())?; |
There was a problem hiding this comment.
Note that this key was set but never used, so I've removed it here
|
Claude finished @dani-garcia's task —— View job Code Review CompleteThis PR refactors key management to use opaque autogenerated local key identifiers instead of requiring callers to provide KeyIds when adding/decrypting keys. This is a solid improvement that reduces error-proneness and simplifies the codebase. Summary of ChangesThe PR introduces
Net Impact: -41 lines of code, improved safety, better developer ergonomics. Critical Issues
|
|
Noting for reviewers: I'll take over this PR for the time being (https://bitwarden.atlassian.net/browse/PM-18102?focusedCommentId=113387) |
gbubemismith
left a comment
gbubemismith
left a comment
There was a problem hiding this comment.
Vault changes look good
| let new_key_id = Ids::Symmetric::new_local(LocalId::new()); | ||
|
|
||
| #[allow(deprecated)] | ||
| self.set_symmetric_key(new_key_id, key)?; | ||
|
|
||
| // Returning the new key identifier for convenience | ||
| Ok(new_key_id) |
There was a problem hiding this comment.
❓ Are we going to cleanup the deprecated functions with an equivalent add_local_symmetric_key later (same for asymmetric and signing keys) and remove the local key id support from set_symmetric_key function ?
There was a problem hiding this comment.
Yeah, I think that makes sense. Considering this is a fairly small nit I'll leave it as follow-up work.
quexten
dismissed stale reviews from mzieniukbw, gbubemismith, and coroiu
via
93fb29e
|
Had to resolve some merge conflicts. |
|
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
Do we ever need to clear the local keys?
Considering the key ids are randomly generated now and the collision chance is basically zero, I don't think we need to. Dropping the context will also drop the keys. I will defer to @dani-garcia though, maybe he has thought about other use-cases here before. |
…aque autogenerated local key ids (bitwarden/sdk-internal#274)
That's correct security-wise, the current UUIDs basically guarantee no posibility of collision so clearing them is not needed as the context teardown will take care of them all eventually. There's still a small concern that if a lot of operations are done on that context, the keys will keep growing until the context is dropped. For example, if you were decrypting 5K ciphers individually, you might end up with 5K local keys at the end. This is why the sdk-internal/crates/bitwarden-crypto/src/store/mod.rs Lines 337 to 338 in ac71502 Ideally I'd like this to be handled automatically by introducing some scope concept to the keystore context and key identifiers, but I haven't found a satisfying solution just yet. Something like: ctx.scope(|| {
let key = ctx.decrypt_key(somedata);
use_the_key(ctx, key);
// Key gets automatically deleted from the store at the end of this scope
}; |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18102
📔 Objective
Currently any function that adds/decrypts a key needs to provide the KeyId under which it can be stored. This is fairly cumbersome and error prone as it can lead to accidentally using the wrong key.
This PR changes all the functions that store a key in a context to instead generate and return a unique ID, which simplifies the code and makes the code less error prone.
⏰ Reminders before review
team
🦮 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 confirmedissue 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