[PM-18102] Use opaque autogenerated local key ids (#274)

## 🎟️ 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

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+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 confirmed
  issue 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

---------

Co-authored-by: Bernd Schoolmann <[email protected]>

Author: dani-garcia

crates/bitwarden-core/src/client/internal.rs

@@ -358,10 +358,9 @@ impl InternalClient {
         let decrypted_user_key = {
             // Note: This block ensures ctx is dropped. Otherwise it would cause a deadlock when
             // initializing the user crypto
-            use crate::key_management::SymmetricKeyId;
             let ctx = &mut self.key_store.context_mut();
             let decrypted_user_key_id = pin_protected_user_key_envelope
-                .unseal(SymmetricKeyId::Local("tmp_unlock_pin"), &pin, ctx)
+                .unseal(&pin, ctx)
                 .map_err(|_| EncryptionSettingsError::WrongPin)?;
 
             // Allowing deprecated here, until a refactor to pass the Local key ids to

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

@@ -724,8 +724,6 @@ pub(crate) fn make_v2_keys_for_v1_user(
     let key_store = client.internal.get_key_store();
     let mut ctx = key_store.context();
 
-    let temporary_user_key_id = SymmetricKeyId::Local("temporary_user_key");
-    let temporary_signing_key_id = SigningKeyId::Local("temporary_signing_key");
     // Re-use existing private key
     let private_key_id = AsymmetricKeyId::UserPrivateKey;
 
@@ -751,13 +749,10 @@ pub(crate) fn make_v2_keys_for_v1_user(
 
     // New user key
     let user_key = SymmetricCryptoKey::make_xchacha20_poly1305_key();
-    #[allow(deprecated)]
-    ctx.set_symmetric_key(temporary_user_key_id, user_key.clone())?;
 
     // New signing key
     let signing_key = SigningKey::make(SignatureAlgorithm::Ed25519);
-    #[allow(deprecated)]
-    ctx.set_signing_key(temporary_signing_key_id, signing_key.clone())?;
+    let temporary_signing_key_id = ctx.add_local_signing_key(signing_key.clone())?;
 
     // Sign existing public key
     let signed_public_key = ctx.make_signed_public_key(private_key_id, temporary_signing_key_id)?;

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

@@ -131,8 +131,7 @@ impl CryptoClient {
         envelope: PasswordProtectedKeyEnvelope,
     ) -> Result<Vec<u8>, CryptoClientError> {
         let mut ctx = self.client.internal.get_key_store().context_mut();
-        let key_slot = SymmetricKeyId::Local("unseal_password_protected_key_envelope");
-        envelope.unseal(key_slot, pin.as_str(), &mut ctx)?;
+        let key_slot = envelope.unseal(pin.as_str(), &mut ctx)?;
         #[allow(deprecated)]
         let key = ctx.dangerous_get_symmetric_key(key_slot)?;
         Ok(key.to_encoded().to_vec())

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

@@ -47,21 +47,21 @@ key_ids! {
         User,
         Organization(OrganizationId),
         #[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-core/src/key_management/security_state.rs

@@ -151,7 +151,7 @@ mod tests {
     use bitwarden_crypto::{KeyStore, SignatureAlgorithm, SigningKey};
 
     use super::*;
-    use crate::key_management::{KeyIds, SigningKeyId};
+    use crate::key_management::KeyIds;
 
     #[test]
     fn test_security_state_signing() {
@@ -161,12 +161,8 @@ mod tests {
         let user_id = UserId::new_v4();
         let security_state = SecurityState::initialize_for_user(user_id);
         let signing_key = SigningKey::make(SignatureAlgorithm::Ed25519);
-        #[allow(deprecated)]
-        ctx.set_signing_key(SigningKeyId::Local(""), signing_key.clone())
-            .unwrap();
-        let signed_security_state = security_state
-            .sign(SigningKeyId::Local(""), &mut ctx)
-            .unwrap();
+        let key = ctx.add_local_signing_key(signing_key.clone()).unwrap();
+        let signed_security_state = security_state.sign(key, &mut ctx).unwrap();
 
         let verifying_key = signing_key.to_verifying_key();
         let verified_security_state = signed_security_state

crates/bitwarden-crypto/examples/protect_key_with_password.rs

@@ -20,9 +20,7 @@ fn main() {
 
     // Alice has a vault protected with a symmetric key. She wants the symmetric key protected with
     // a PIN.
-    let vault_key = ctx
-        .generate_symmetric_key(ExampleSymmetricKey::VaultKey)
-        .expect("Generating vault key should work");
+    let vault_key = ctx.generate_symmetric_key();
 
     // Seal the key with the PIN
     // The KDF settings are chosen for you, and do not need to be separately tracked or synced
@@ -43,7 +41,7 @@ fn main() {
         )
         .expect("Deserializing envelope should work");
     deserialized
-        .unseal(ExampleSymmetricKey::VaultKey, pin, &mut ctx)
+        .unseal(pin, &mut ctx)
         .expect("Unsealing should work");
 
     // Alice wants to change her password; also her KDF settings are below the minimums.
@@ -54,15 +52,14 @@ fn main() {
     disk.save("vault_key_envelope", (&envelope).into());
 
     // Alice wants to change the protected key. This requires creating a new envelope
-    ctx.generate_symmetric_key(ExampleSymmetricKey::VaultKey)
-        .expect("Generating vault key should work");
-    let envelope = PasswordProtectedKeyEnvelope::seal(ExampleSymmetricKey::VaultKey, "0000", &ctx)
-        .expect("Sealing should work");
+    let vault_key = ctx.generate_symmetric_key();
+    let envelope =
+        PasswordProtectedKeyEnvelope::seal(vault_key, "0000", &ctx).expect("Sealing should work");
     disk.save("vault_key_envelope", (&envelope).into());
 
     // Alice tries the password but it is wrong
     assert!(matches!(
-        envelope.unseal(ExampleSymmetricKey::VaultKey, "9999", &mut ctx),
+        envelope.unseal("9999", &mut ctx),
         Err(PasswordProtectedKeyEnvelopeError::WrongPassword)
     ));
 }
@@ -91,17 +88,21 @@ key_ids! {
     #[symmetric]
     pub enum ExampleSymmetricKey {
         #[local]
-        VaultKey
+        VaultKey(LocalId)
     }
 
     #[asymmetric]
     pub enum ExampleAsymmetricKey {
         Key(u8),
+        #[local]
+        Local(LocalId)
     }
 
     #[signing]
     pub enum ExampleSigningKey {
         Key(u8),
+        #[local]
+        Local(LocalId)
     }
 
    pub ExampleIds => ExampleSymmetricKey, ExampleAsymmetricKey, ExampleSigningKey;

crates/bitwarden-crypto/src/lib.rs

@@ -42,7 +42,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/safe/password_protected_key_envelope.rs

@@ -139,15 +139,11 @@ impl<Ids: KeyIds> PasswordProtectedKeyEnvelope<Ids> {
     /// context.
     pub fn unseal(
         &self,
-        target_keyslot: Ids::Symmetric,
         password: &str,
         ctx: &mut KeyStoreContext<Ids>,
     ) -> Result<Ids::Symmetric, PasswordProtectedKeyEnvelopeError> {
         let key = self.unseal_ref(password)?;
-        #[allow(deprecated)]
-        ctx.set_symmetric_key(target_keyslot, key)
-            .map_err(|_| PasswordProtectedKeyEnvelopeError::KeyStore)?;
-        Ok(target_keyslot)
+        Ok(ctx.add_local_symmetric_key(key))
     }
 
     fn unseal_ref(
@@ -501,12 +497,12 @@ mod tests {
         let envelope =
             PasswordProtectedKeyEnvelope::try_from(&TESTVECTOR_COSEKEY_ENVELOPE.to_vec())
                 .expect("Key envelope should be valid");
-        envelope
-            .unseal(TestSymmKey::A(0), TESTVECTOR_PASSWORD, &mut ctx)
+        let key = envelope
+            .unseal(TESTVECTOR_PASSWORD, &mut ctx)
             .expect("Unsealing should succeed");
         #[allow(deprecated)]
         let unsealed_key = ctx
-            .dangerous_get_symmetric_key(TestSymmKey::A(0))
+            .dangerous_get_symmetric_key(key)
             .expect("Key should exist in the key store");
         assert_eq!(
             unsealed_key.to_encoded().to_vec(),
@@ -521,12 +517,12 @@ mod tests {
         let envelope =
             PasswordProtectedKeyEnvelope::try_from(&TESTVECTOR_LEGACYKEY_ENVELOPE.to_vec())
                 .expect("Key envelope should be valid");
-        envelope
-            .unseal(TestSymmKey::A(0), TESTVECTOR_PASSWORD, &mut ctx)
+        let key = envelope
+            .unseal(TESTVECTOR_PASSWORD, &mut ctx)
             .expect("Unsealing should succeed");
         #[allow(deprecated)]
         let unsealed_key = ctx
-            .dangerous_get_symmetric_key(TestSymmKey::A(0))
+            .dangerous_get_symmetric_key(key)
             .expect("Key should exist in the key store");
         assert_eq!(
             unsealed_key.to_encoded().to_vec(),
@@ -549,14 +545,12 @@ mod tests {
         // Unseal the key from the envelope
         let deserialized: PasswordProtectedKeyEnvelope<TestIds> =
             PasswordProtectedKeyEnvelope::try_from(&serialized).unwrap();
-        deserialized
-            .unseal(TestSymmKey::A(1), password, &mut ctx)
-            .unwrap();
+        let key = deserialized.unseal(password, &mut ctx).unwrap();
 
         // Verify that the unsealed key matches the original key
         #[allow(deprecated)]
         let unsealed_key = ctx
-            .dangerous_get_symmetric_key(TestSymmKey::A(1))
+            .dangerous_get_symmetric_key(key)
             .expect("Key should exist in the key store");
 
         #[allow(deprecated)]
@@ -571,7 +565,7 @@ mod tests {
     fn test_make_envelope_legacy_key() {
         let key_store = KeyStore::<TestIds>::default();
         let mut ctx: KeyStoreContext<'_, TestIds> = key_store.context_mut();
-        let test_key = ctx.generate_symmetric_key(TestSymmKey::A(0)).unwrap();
+        let test_key = ctx.generate_symmetric_key();
 
         let password = "test_password";
 
@@ -582,14 +576,12 @@ mod tests {
         // Unseal the key from the envelope
         let deserialized: PasswordProtectedKeyEnvelope<TestIds> =
             PasswordProtectedKeyEnvelope::try_from(&serialized).unwrap();
-        deserialized
-            .unseal(TestSymmKey::A(1), password, &mut ctx)
-            .unwrap();
+        let key = deserialized.unseal(password, &mut ctx).unwrap();
 
         // Verify that the unsealed key matches the original key
         #[allow(deprecated)]
         let unsealed_key = ctx
-            .dangerous_get_symmetric_key(TestSymmKey::A(1))
+            .dangerous_get_symmetric_key(key)
             .expect("Key should exist in the key store");
 
         #[allow(deprecated)]
@@ -638,7 +630,7 @@ mod tests {
         let deserialized: PasswordProtectedKeyEnvelope<TestIds> =
             PasswordProtectedKeyEnvelope::try_from(&(&envelope).into()).unwrap();
         assert!(matches!(
-            deserialized.unseal(TestSymmKey::A(1), wrong_password, &mut ctx),
+            deserialized.unseal(wrong_password, &mut ctx),
             Err(PasswordProtectedKeyEnvelopeError::WrongPassword)
         ));
     }