[PM-31114] Introduce local user data key, set it to state on crypto init#806
[PM-31114] Introduce local user data key, set it to state on crypto init#806mzieniukbw wants to merge 7 commits intomainfrom
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
- Coverage 81.77% 81.74% -0.03%
==========================================
Files 344 346 +2
Lines 41116 41207 +91
==========================================
+ Hits 33623 33686 +63
- Misses 7493 7521 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # crates/bitwarden-core/src/client/encryption_settings.rs # crates/bitwarden-core/src/key_management/crypto.rs
|
| ); | ||
|
|
||
| drop(_span_guard); | ||
| { |
There was a problem hiding this comment.
Do we have a reason for this block scope?
There was a problem hiding this comment.
The span guard does not have Send trait and can't be used with in async context - hence we have to drop it before first await in this function. This is also the reason why i moved all async code beneath that synchronous code block.
Do you want me to add a comment for clarity ? We could also simply make it a separate function.
There was a problem hiding this comment.
Ah, makes sense. A drop(_span_guard); should also work, and is possibly easier to read, but it's unfortunate that the span requires so much manual work here..
There was a problem hiding this comment.
You may have an easier time removing the span and instead using #[tracing::instrument] on the function definition, which should also define and manage a span for you.
| #[cfg_attr(feature = "dangerous-crypto-debug", instrument(err))] | ||
| #[cfg_attr(not(feature = "dangerous-crypto-debug"), instrument(skip_all, err))] | ||
| pub fn encrypt_with_user_key( | ||
| &self, |
There was a problem hiding this comment.
Question: Can we make this work on key context instead of taking a key reference?
| pub fn encrypt_with_user_key( | ||
| &self, | ||
| user_key: &SymmetricCryptoKey, | ||
| ) -> crate::error::Result<EncString> { |
There was a problem hiding this comment.
Nit: Just use Result<EncString, CryptoError> or import crate::error::Result
| encrypted_key: EncString, | ||
| } | ||
|
|
||
| bitwarden_state::register_repository_item!(String => LocalUserDataKeyState, "LocalUserDataKey"); |
There was a problem hiding this comment.
Did we want to make this sdk managed? If so this is the right way to do it: https://github.com/bitwarden/sdk-internal/tree/main/crates/bitwarden-state#settings
| /// | ||
| /// Used for backwards compatibility: data previously encrypted with the user key can be | ||
| /// decrypted by the reconstructed `SymmetricCryptoKey`. | ||
| DerivedFromUserKey(BitwardenLegacyKeyBytes), |
There was a problem hiding this comment.
I'm not sure I understand the reason why this is an enum. If we have an encrypted local user data key (encstring) do we still store the information about whether it was a migrated user key?
Besides this, I'd recommend not making another decrypted copy of keys. We generally want keys to live on the key context, so the struct here should be the wrapped version, with functions to set it to the key context. I.e:
pub struct WrappedLocalUserDataKey(EncString);
impl WrappedLocalUserDataKey {
pub fn from_user_key(user_key_id: [...], ctx: [...])
pub fn set_to_context(user_key_id: [...], local_user_key_id: [...], ctx: [...])
}There was a problem hiding this comment.
You are right, we are storing this in storage, hence it have to be wrapped, not decrypted as symmetric key. I will correct this.
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31114
📔 Objective
Introduced
LocalUserDataKey, which will be used to decouple the direct use of user key for encryption in tool's code (like encrypted forwarders, password history).This will allow for user key rotation without logout, since we just have to re-encrypt the local user data key with the new user key, without re-encrypting all other data.
On user crypto init, we create new
LocalUserDataKeykey, and set it to state, if not already set. For backwards compatibility the local user data key is user key wrapper user key - this allows us to use it as drop in replacement in all client's, which currently encrypt with user key directly. Later, when we start rolling out the encryption v2 (which comes with key rotation in background), the new user key will differ from the one used as local user data key, retaining backwards compatibility, without having to migrate user over.In the future, the
LocalUserDataKeywill likely be generated.🚨 Breaking Changes
Expected to fail to compile, until the new repository is registered in clients, see https://bitwarden.atlassian.net/browse/PM-31111