[PM-18102] Use opaque autogenerated local key ids

crates/bitwarden-core/src/key_management/crypto.rs

@@ -20,7 +20,7 @@
 
 use crate::{
     client::{encryption_settings::EncryptionSettingsError, LoginMethod, UserLoginMethod},
-    key_management::{AsymmetricKeyId, SigningKeyId, SymmetricKeyId},
+    key_management::{AsymmetricKeyId, SymmetricKeyId},
     Client, NotAuthenticatedError, VaultLockedError, WrongPasswordError,
 };
 
@@ -596,9 +596,8 @@
 
     // Make new keypair and sign the public key with it
     let signature_keypair = SigningKey::make(SignatureAlgorithm::Ed25519);
-    let temporary_signature_keypair_id = SigningKeyId::Local("temporary_key_for_rotation");
-    #[allow(deprecated)]
-    ctx.set_signing_key(temporary_signature_keypair_id, signature_keypair.clone())?;
+    let temporary_signature_keypair_id = ctx.add_local_signing_key(signature_keypair.clone())?;
+
     let signed_public_key = ctx.make_signed_public_key(
         AsymmetricKeyId::UserPrivateKey,
         temporary_signature_keypair_id,

crates/bitwarden-core/src/key_management/mod.rs

@@ -25,21 +25,21 @@ key_ids! {
         User,
         Organization(uuid::Uuid),
         #[local]
-        Local(&'static str),
+        Local(LocalId),
     }
 
     #[asymmetric]
     pub enum AsymmetricKeyId {
         UserPrivateKey,
         #[local]
-        Local(&'static str),
+        Local(LocalId),
     }
 
     #[signing]
     pub enum SigningKeyId {
         UserSigningKey,
         #[local]
-        Local(&'static str),
+        Local(LocalId),
     }
 
     pub KeyIds => SymmetricKeyId, AsymmetricKeyId, SigningKeyId;

crates/bitwarden-crypto/src/lib.rs

@@ -39,7 +39,7 @@ pub use signing::*;
 mod traits;
 mod xchacha20;
 pub use traits::{
-    CompositeEncryptable, Decryptable, IdentifyKey, KeyId, KeyIds, PrimitiveEncryptable,
+    CompositeEncryptable, Decryptable, IdentifyKey, KeyId, KeyIds, LocalId, PrimitiveEncryptable,
 };
 pub use zeroizing_alloc::ZeroAlloc as ZeroizingAllocator;
 

crates/bitwarden-crypto/src/store/context.rs

@@ -10,7 +10,7 @@
 use crate::{
     derive_shareable_key, error::UnsupportedOperation, signing, store::backend::StoreBackend,
     AsymmetricCryptoKey, BitwardenLegacyKeyBytes, ContentFormat, CryptoError, EncString, KeyId,
-    KeyIds, Result, Signature, SignatureAlgorithm, SignedObject, SignedPublicKey,
+    KeyIds, LocalId, Result, Signature, SignatureAlgorithm, SignedObject, SignedPublicKey,
     SignedPublicKeyMessage, SigningKey, SymmetricCryptoKey, UnsignedSharedKey,
 };
 
@@ -37,15 +37,20 @@
 /// #     #[symmetric]
 /// #     pub enum SymmKeyId {
 /// #         User,
-/// #         Local(&'static str),
+/// #         #[local]
+/// #         Local(LocalId),
 /// #     }
 /// #     #[asymmetric]
 /// #     pub enum AsymmKeyId {
 /// #         UserPrivate,
+/// #         #[local]
+/// #         Local(LocalId),
 /// #     }
 /// #     #[signing]
 /// #     pub enum SigningKeyId {
 /// #         UserSigning,
+/// #         #[local]
+/// #         Local(LocalId),
 /// #     }
 /// #     pub Ids => SymmKeyId, AsymmKeyId, SigningKeyId;
 /// # }
@@ -59,11 +64,10 @@
 /// #    }
 /// # }
 ///
-/// const LOCAL_KEY: SymmKeyId = SymmKeyId::Local("local_key_id");
 ///
 /// impl CompositeEncryptable<Ids, SymmKeyId, EncString> for Data {
 ///     fn encrypt_composite(&self, ctx: &mut KeyStoreContext<Ids>, key: SymmKeyId) -> Result<EncString, CryptoError> {
-///         let local_key_id = ctx.unwrap_symmetric_key(key, LOCAL_KEY, &self.key)?;
+///         let local_key_id = ctx.unwrap_symmetric_key(key, &self.key)?;
 ///         self.name.encrypt(ctx, local_key_id)
 ///     }
 /// }
@@ -149,7 +153,6 @@
     pub fn unwrap_symmetric_key(
         &mut self,
         wrapping_key: Ids::Symmetric,
-        new_key_id: Ids::Symmetric,
         wrapped_key: &EncString,
     ) -> Result<Ids::Symmetric> {
         let wrapping_key = self.get_symmetric_key(wrapping_key)?;
@@ -183,6 +186,8 @@
             _ => return Err(CryptoError::InvalidKey),
         };
 
+        let new_key_id = Ids::Symmetric::new_local(LocalId::new());
+
         #[allow(deprecated)]
         self.set_symmetric_key(new_key_id, key)?;
 
@@ -301,20 +306,14 @@
     }
 
     /// Generate a new random symmetric key and store it in the context
-    pub fn generate_symmetric_key(&mut self, key_id: Ids::Symmetric) -> Result<Ids::Symmetric> {
-        let key = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
-        #[allow(deprecated)]
-        self.set_symmetric_key(key_id, key)?;
-        Ok(key_id)
+    pub fn generate_symmetric_key(&mut self) -> Result<Ids::Symmetric> {
+        self.add_local_symmetric_key(SymmetricCryptoKey::make_aes256_cbc_hmac_key())
     }
 
     /// Generate a new signature key using the current default algorithm, and store it in the
     /// context
-    pub fn make_signing_key(&mut self, key_id: Ids::Signing) -> Result<Ids::Signing> {
-        let key = SigningKey::make(SignatureAlgorithm::default_algorithm());
-        #[allow(deprecated)]
-        self.set_signing_key(key_id, key)?;
-        Ok(key_id)
+    pub fn make_signing_key(&mut self) -> Result<Ids::Signing> {
+        self.add_local_signing_key(SigningKey::make(SignatureAlgorithm::default_algorithm()))
     }
 
     /// Derive a shareable key using hkdf from secret and name and store it in the context.
@@ -323,11 +322,11 @@
     /// Bitwarden `clients` repository.
     pub fn derive_shareable_key(
         &mut self,
-        key_id: Ids::Symmetric,
         secret: Zeroizing<[u8; 16]>,
         name: &str,
         info: Option<&str>,
     ) -> Result<Ids::Symmetric> {
+        let key_id = Ids::Symmetric::new_local(LocalId::new());
         #[allow(deprecated)]
         self.set_symmetric_key(
             key_id,
@@ -414,6 +413,13 @@
         Ok(())
     }
 
+    /// Add a new symmetric key to the local context, returning a new unique identifier for it.
+    pub fn add_local_symmetric_key(&mut self, key: SymmetricCryptoKey) -> Result<Ids::Symmetric> {
+        let key_id = Ids::Symmetric::new_local(LocalId::new());
+        self.local_symmetric_keys.upsert(key_id, key);
+        Ok(key_id)
+    }
+
     #[deprecated(note = "This function should ideally never be used outside this crate")]
     #[allow(missing_docs)]
     pub fn set_asymmetric_key(
@@ -443,6 +449,13 @@
         Ok(())
     }
 
+    /// Add a new signing key to the local context, returning a new unique identifier for it.
+    pub fn add_local_signing_key(&mut self, key: SigningKey) -> Result<Ids::Signing> {
+        let key_id = Ids::Signing::new_local(LocalId::new());
+        self.local_signing_keys.upsert(key_id, key);
+        Ok(key_id)
+    }
+
     pub(crate) fn decrypt_data_with_symmetric_key(
         &self,
         key: Ids::Symmetric,
@@ -525,7 +538,7 @@
             KeyStore,
         },
         traits::tests::{TestIds, TestSigningKey, TestSymmKey},
-        CompositeEncryptable, CryptoError, Decryptable, SignatureAlgorithm, SigningKey,
+        CompositeEncryptable, CryptoError, Decryptable, LocalId, SignatureAlgorithm, SigningKey,
         SigningNamespace, SymmetricCryptoKey,
     };
 
@@ -567,19 +580,20 @@
     #[test]
     fn test_key_encryption() {
         let store: KeyStore<TestIds> = KeyStore::default();
+        let local = LocalId::new();
 
         let mut ctx = store.context();
 
         // Generate and insert a key
-        let key_1_id = TestSymmKey::C(1);
+        let key_1_id = TestSymmKey::C(local);
         let key_1 = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
 
         ctx.set_symmetric_key(key_1_id, key_1.clone()).unwrap();
 
         assert!(ctx.has_symmetric_key(key_1_id));
 
         // Generate and insert a new key
-        let key_2_id = TestSymmKey::C(2);
+        let key_2_id = TestSymmKey::C(local);
         let key_2 = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
 
         ctx.set_symmetric_key(key_2_id, key_2.clone()).unwrap();
@@ -590,10 +604,7 @@
         let key_2_enc = ctx.wrap_symmetric_key(key_1_id, key_2_id).unwrap();
 
         // Decrypt the new key with the old key in a different identifier
-        let new_key_id = TestSymmKey::C(3);
-
-        ctx.unwrap_symmetric_key(key_1_id, new_key_id, &key_2_enc)
-            .unwrap();
+        let new_key_id = ctx.unwrap_symmetric_key(key_1_id, &key_2_enc).unwrap();
 
         // Now `key_2_id` and `new_key_id` contain the same key, so we should be able to encrypt
         // with one and decrypt with the other
@@ -646,24 +657,18 @@
             .unwrap();
 
         // Unwrap the keys
-        let unwrapped_key_2 = ctx
-            .unwrap_symmetric_key(key_aes_1_id, key_aes_2_id, &wrapped_key_1_2)
+        let _unwrapped_key_2 = ctx
+            .unwrap_symmetric_key(key_aes_1_id, &wrapped_key_1_2)
             .unwrap();
-        let unwrapped_key_3 = ctx
-            .unwrap_symmetric_key(key_aes_1_id, key_xchacha_3_id, &wrapped_key_1_3)
+        let _unwrapped_key_3 = ctx
+            .unwrap_symmetric_key(key_aes_1_id, &wrapped_key_1_3)
             .unwrap();
-        let unwrapped_key_1 = ctx
-            .unwrap_symmetric_key(key_xchacha_3_id, key_aes_1_id, &wrapped_key_3_1)
+        let _unwrapped_key_1 = ctx
+            .unwrap_symmetric_key(key_xchacha_3_id, &wrapped_key_3_1)
             .unwrap();
-        let unwrapped_key_4 = ctx
-            .unwrap_symmetric_key(key_xchacha_3_id, key_xchacha_4_id, &wrapped_key_3_4)
+        let _unwrapped_key_4 = ctx
+            .unwrap_symmetric_key(key_xchacha_3_id, &wrapped_key_3_4)
             .unwrap();
-
-        // Assert that the unwrapped keys are the same as the original keys
-        assert_eq!(unwrapped_key_2, key_aes_2_id);
-        assert_eq!(unwrapped_key_3, key_xchacha_3_id);
-        assert_eq!(unwrapped_key_1, key_aes_1_id);
-        assert_eq!(unwrapped_key_4, key_xchacha_4_id);
     }
 
     #[test]

crates/bitwarden-crypto/src/store/mod.rs

@@ -52,15 +52,19 @@ pub use context::KeyStoreContext;
 ///     pub enum SymmKeyId {
 ///         User,
 ///         #[local]
-///         Local(&'static str)
+///         Local(LocalId),
 ///     }
 ///     #[asymmetric]
 ///     pub enum AsymmKeyId {
 ///         UserPrivate,
+///         #[local]
+///         Local(LocalId),
 ///     }
 ///     #[signing]
 ///     pub enum SigningKeyId {
 ///        UserSigning,
+///        #[local]
+///        Local(LocalId),
 ///     }
 ///     pub Ids => SymmKeyId, AsymmKeyId, SigningKeyId;
 /// }

crates/bitwarden-crypto/src/traits/key_id.rs

@@ -24,6 +24,9 @@ pub trait KeyId:
     /// Returns whether the key is local to the current context or shared globally by the
     /// key store. See [crate::store::KeyStoreContext] for more information.
     fn is_local(&self) -> bool;
+
+    /// Creates a new unique local key identifier.
+    fn new_local(id: LocalId) -> Self;
 }
 
 /// Represents a set of all the key identifiers that need to be defined to use a key store.
@@ -37,6 +40,17 @@ pub trait KeyIds {
     type Signing: KeyId<KeyValue = SigningKey>;
 }
 
+/// An opaque identifier for a local key. Currently only contains a unique ID, but it can be
+/// extended to contain scope information to allow cleanup on scope exit.
+#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)]
+pub struct LocalId(pub(crate) uuid::Uuid);
+
+impl LocalId {
+    pub(crate) fn new() -> Self {
+        LocalId(uuid::Uuid::new_v4())
+    }
+}
+
 /// Just a small derive_like macro that can be used to generate the key identifier enums.
 /// Example usage:
 /// ```rust
@@ -47,17 +61,21 @@ pub trait KeyIds {
 ///         User,
 ///         Org(uuid::Uuid),
 ///         #[local]
-///         Local(&'static str),
+///         Local(LocalId),
 ///     }
 ///
 ///     #[asymmetric]
 ///     pub enum AsymmKeyId {
 ///         PrivateKey,
+///         #[local]
+///         Local(LocalId),
 ///     }
 ///
 ///     #[signing]
 ///     pub enum SigningKeyId {
 ///        SigningKey,
+///        #[local]
+///        Local(LocalId),
 ///     }
 ///
 ///     pub Ids => SymmKeyId, AsymmKeyId, SigningKeyId;
@@ -76,6 +94,9 @@ macro_rules! key_ids {
     )+
     $ids_vis:vis $ids_name:ident => $symm_name:ident, $asymm_name:ident, $signing_name:ident;
     ) => {
+
+        use $crate::LocalId;
+
         $(
             #[derive(std::fmt::Debug, Clone, Copy, std::hash::Hash, Eq, PartialEq, Ord, PartialOrd)]
             #[allow(missing_docs)]
@@ -93,6 +114,12 @@ macro_rules! key_ids {
                             key_ids!(@variant_value $( $variant_tag )? ),
                     )* }
                 }
+
+                fn new_local(id: LocalId) -> Self {
+                    $(
+                        { key_ids!(@new_local $variant  id $( $variant_tag )? ) }
+                    )*
+                }
             }
         )+
 
@@ -114,27 +141,33 @@ macro_rules! key_ids {
 
     ( @variant_value local ) => { true };
     ( @variant_value ) => { false };
+
+    ( @new_local $variant:ident $id:ident local  ) => { Self::$variant($id) };
+    ( @new_local $variant:ident $id:ident ) => {{}};
 }
 
 #[cfg(test)]
 pub(crate) mod tests {
+
     use crate::{
         traits::tests::{TestAsymmKey, TestSigningKey, TestSymmKey},
-        KeyId,
+        KeyId, LocalId,
     };
 
     #[test]
     fn test_local() {
+        let local = LocalId::new();
+
         assert!(!TestSymmKey::A(0).is_local());
         assert!(!TestSymmKey::B((4, 10)).is_local());
-        assert!(TestSymmKey::C(8).is_local());
+        assert!(TestSymmKey::C(local).is_local());
 
         assert!(!TestAsymmKey::A(0).is_local());
         assert!(!TestAsymmKey::B.is_local());
-        assert!(TestAsymmKey::C("test").is_local());
+        assert!(TestAsymmKey::C(local).is_local());
 
         assert!(!TestSigningKey::A(0).is_local());
         assert!(!TestSigningKey::B.is_local());
-        assert!(TestSigningKey::C("test").is_local());
+        assert!(TestSigningKey::C(local).is_local());
     }
 }