Split Encryptable/Decryptable trait into encryption and key retrieval (…

…#297)

## Type of change
```
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```

## Objective
At the moment, the `Encryptable`/`Decryptable` trait handles both
encryption/decryption and key retrieval. The goal of this PR is to split
this trait into two parts, one part that only handles
encryption/decryption (and thus will be able to be moved into a separate
`bitwarden-crypto` crate, if we want), and another part that handles
retrieving the corresponding encryption key for each model object.

## Code changes
What was the `Encryptable`/`Decryptable` trait has been split into
multiple parts:
- `KeyEncryptable`/`KeyDecryptable`: This receives only the encryption
key and handles encryption/decryption only. The name of the interface
was chosen to match the current `EncString::decrypt_with_key`, which
this replaces.
- `LocateKey`, which is implemented by types that know the type of key
they need. For example a `Folder` always uses the user key and a
`Cipher` uses the user key or one of the organization keys depending on
its `organization_id` field.
- `Encryptable`/`Decryptable`, this is now a thin compatibility layer
between the previously mentioned two, to keep the existing API for the
moment and keep the changes in this PR to a minimum. In the future we
might decide that it's not necessary.

With these changes, any models should implement
`KeyEncryptable`/`KeyDecryptable` only, and `LocateKey` if applicable,
but never `Encryptable`/`Decryptable` directly.

Some of these changes will be more fleshed out in the future to avoid
cluttering this PR, so I expect this won't look perfect:
- Both `LocateKey` and `Encryptable` have an interface like `enc:
&EncryptionSettings, org_id: &Option<Uuid>`. The organization parameter
is mostly superfluous now and will be removed in the future: Objects
that implement `LocateKey` do not use it, while objects which don't
implement it should be provided the keys directly.
- `EncString` is implementing `LocateKey` now, which goes against the
explanation above. This is to avoid refactoring some secrets manager
code, which I'll do on a later PR.
- `EncryptionSettings` will change in the future, specially the
`get_key` implementation, to be more generic. It also contains an
`encrypt` function that shouldn't be there, but is used in a couple of
spots of the code base and I'll remove on a later PR.
- `KeyEncryptable`/`KeyDecryptable` only handles `SymmetricCryptoKey`,
this will be made more generic over key type.

crates/bitwarden/src/auth/login/access_token.rs

@@ -8,7 +8,7 @@ use crate::{
         login::{response::two_factor::TwoFactorProviders, PasswordLoginResponse},
     },
     client::{AccessToken, LoginMethod, ServiceAccountLoginMethod},
-    crypto::{EncString, SymmetricCryptoKey},
+    crypto::{EncString, KeyDecryptable, SymmetricCryptoKey},
     error::{Error, Result},
     util::{decode_token, BASE64_ENGINE},
     Client,
@@ -29,7 +29,7 @@ pub(crate) async fn access_token_login(
         // Extract the encrypted payload and use the access token encryption key to decrypt it
         let payload: EncString = r.encrypted_payload.parse()?;
 
-        let decrypted_payload = payload.decrypt_with_key(&access_token.encryption_key)?;
+        let decrypted_payload: Vec<u8> = payload.decrypt_with_key(&access_token.encryption_key)?;
 
         // Once decrypted, we have to JSON decode to extract the organization encryption key
         #[derive(serde::Deserialize)]

crates/bitwarden/src/client/encryption_settings.rs

@@ -4,7 +4,7 @@ use rsa::RsaPrivateKey;
 use uuid::Uuid;
 #[cfg(feature = "internal")]
 use {
-    crate::client::UserLoginMethod,
+    crate::{client::UserLoginMethod, crypto::KeyDecryptable},
     rsa::{pkcs8::DecodePrivateKey, Oaep},
 };
 
@@ -46,7 +46,7 @@ impl EncryptionSettings {
 
                 // Decrypt the private key with the user key
                 let private_key = {
-                    let dec = private_key.decrypt_with_key(&user_key)?;
+                    let dec: Vec<u8> = private_key.decrypt_with_key(&user_key)?;
                     Some(
                         rsa::RsaPrivateKey::from_pkcs8_der(&dec)
                             .map_err(|_| CryptoError::InvalidKey)?,
@@ -98,7 +98,7 @@ impl EncryptionSettings {
         Ok(self)
     }
 
-    fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey> {
+    pub(crate) fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey> {
         // If we don't have a private key set (to decode multiple org keys), we just use the main user key
         if self.private_key.is_none() {
             return Some(&self.user_key);
@@ -110,42 +110,10 @@ impl EncryptionSettings {
         }
     }
 
-    pub(crate) fn decrypt_bytes(
-        &self,
-        cipher: &EncString,
-        org_id: &Option<Uuid>,
-    ) -> Result<Vec<u8>> {
-        let key = self.get_key(org_id).ok_or(CryptoError::NoKeyForOrg)?;
-        cipher.decrypt_with_key(key)
-    }
-
-    pub(crate) fn decrypt(&self, cipher: &EncString, org_id: &Option<Uuid>) -> Result<String> {
-        let dec = self.decrypt_bytes(cipher, org_id)?;
-        String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String.into())
-    }
-
     pub(crate) fn encrypt(&self, data: &[u8], org_id: &Option<Uuid>) -> Result<EncString> {
         let key = self.get_key(org_id).ok_or(CryptoError::NoKeyForOrg)?;
 
         let dec = encrypt_aes256_hmac(data, key.mac_key.ok_or(CryptoError::InvalidMac)?, key.key)?;
         Ok(dec)
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::{EncryptionSettings, SymmetricCryptoKey};
-    use crate::crypto::{Decryptable, Encryptable};
-
-    #[test]
-    fn test_encryption_settings() {
-        let key = SymmetricCryptoKey::generate("test");
-        let settings = EncryptionSettings::new_single_key(key);
-
-        let test_string = "encrypted_test_string".to_string();
-        let cipher = test_string.clone().encrypt(&settings, &None).unwrap();
-
-        let decrypted_str = cipher.decrypt(&settings, &None).unwrap();
-        assert_eq!(decrypted_str, test_string);
-    }
-}

crates/bitwarden/src/crypto/enc_string.rs

@@ -2,15 +2,15 @@ use std::{fmt::Display, str::FromStr};
 
 use base64::Engine;
 use serde::{de::Visitor, Deserialize};
-use uuid::Uuid;
 
 use crate::{
-    client::encryption_settings::EncryptionSettings,
-    crypto::{decrypt_aes256_hmac, Decryptable, Encryptable, SymmetricCryptoKey},
+    crypto::{decrypt_aes256_hmac, SymmetricCryptoKey},
     error::{CryptoError, EncStringParseError, Error, Result},
     util::BASE64_ENGINE,
 };
 
+use super::{KeyDecryptable, KeyEncryptable, LocateKey};
+
 #[derive(Clone)]
 #[allow(unused, non_camel_case_types)]
 pub enum EncString {
@@ -304,8 +304,24 @@ impl EncString {
             EncString::Rsa2048_OaepSha1_HmacSha256_B64 { .. } => 6,
         }
     }
+}
 
-    pub fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<u8>> {
+fn invalid_len_error(expected: usize) -> impl Fn(Vec<u8>) -> EncStringParseError {
+    move |e: Vec<_>| EncStringParseError::InvalidLength {
+        expected,
+        got: e.len(),
+    }
+}
+
+impl LocateKey for EncString {}
+impl KeyEncryptable<EncString> for &[u8] {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<EncString> {
+        super::encrypt_aes256_hmac(self, key.mac_key.ok_or(CryptoError::InvalidMac)?, key.key)
+    }
+}
+
+impl KeyDecryptable<Vec<u8>> for EncString {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<u8>> {
         match self {
             EncString::AesCbc256_HmacSha256_B64 { iv, mac, data } => {
                 let mac_key = key.mac_key.ok_or(CryptoError::InvalidMac)?;
@@ -317,29 +333,36 @@ impl EncString {
     }
 }
 
-fn invalid_len_error(expected: usize) -> impl Fn(Vec<u8>) -> EncStringParseError {
-    move |e: Vec<_>| EncStringParseError::InvalidLength {
-        expected,
-        got: e.len(),
-    }
-}
-
-impl Encryptable<EncString> for String {
-    fn encrypt(self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<EncString> {
-        enc.encrypt(self.as_bytes(), org_id)
+impl KeyEncryptable<EncString> for String {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<EncString> {
+        self.as_bytes().encrypt_with_key(key)
     }
 }
 
-impl Decryptable<String> for EncString {
-    fn decrypt(&self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<String> {
-        enc.decrypt(self, org_id)
+impl KeyDecryptable<String> for EncString {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<String> {
+        let dec: Vec<u8> = self.decrypt_with_key(key)?;
+        String::from_utf8(dec).map_err(|_| CryptoError::InvalidUtf8String.into())
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use crate::crypto::{KeyDecryptable, KeyEncryptable, SymmetricCryptoKey};
+
     use super::EncString;
 
+    #[test]
+    fn test_enc_string_roundtrip() {
+        let key = SymmetricCryptoKey::generate("test");
+
+        let test_string = "encrypted_test_string".to_string();
+        let cipher = test_string.clone().encrypt_with_key(&key).unwrap();
+
+        let decrypted_str: String = cipher.decrypt_with_key(&key).unwrap();
+        assert_eq!(decrypted_str, test_string);
+    }
+
     #[test]
     fn test_enc_string_serialization() {
         #[derive(serde::Serialize, serde::Deserialize)]

crates/bitwarden/src/crypto/encryptable.rs

@@ -2,7 +2,22 @@ use std::{collections::HashMap, hash::Hash};
 
 use uuid::Uuid;
 
-use crate::{client::encryption_settings::EncryptionSettings, error::Result};
+use crate::{
+    client::encryption_settings::EncryptionSettings,
+    error::{Error, Result},
+};
+
+use super::{KeyDecryptable, KeyEncryptable, SymmetricCryptoKey};
+
+pub trait LocateKey {
+    fn locate_key<'a>(
+        &self,
+        enc: &'a EncryptionSettings,
+        org_id: &Option<Uuid>,
+    ) -> Option<&'a SymmetricCryptoKey> {
+        enc.get_key(org_id)
+    }
+}
 
 pub trait Encryptable<Output> {
     fn encrypt(self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output>;
@@ -12,15 +27,17 @@ pub trait Decryptable<Output> {
     fn decrypt(&self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output>;
 }
 
-impl<T: Encryptable<Output>, Output> Encryptable<Option<Output>> for Option<T> {
-    fn encrypt(self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Option<Output>> {
-        self.map(|e| e.encrypt(enc, org_id)).transpose()
+impl<T: KeyEncryptable<Output> + LocateKey, Output> Encryptable<Output> for T {
+    fn encrypt(self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output> {
+        let key = self.locate_key(enc, org_id).ok_or(Error::VaultLocked)?;
+        self.encrypt_with_key(key)
     }
 }
 
-impl<T: Decryptable<Output>, Output> Decryptable<Option<Output>> for Option<T> {
-    fn decrypt(&self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Option<Output>> {
-        self.as_ref().map(|e| e.decrypt(enc, org_id)).transpose()
+impl<T: KeyDecryptable<Output> + LocateKey, Output> Decryptable<Output> for T {
+    fn decrypt(&self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output> {
+        let key = self.locate_key(enc, org_id).ok_or(Error::VaultLocked)?;
+        self.decrypt_with_key(key)
     }
 }
 
@@ -46,7 +63,7 @@ impl<T: Encryptable<Output>, Output, Id: Hash + Eq> Encryptable<HashMap<Id, Outp
     ) -> Result<HashMap<Id, Output>> {
         self.into_iter()
             .map(|(id, e)| Ok((id, e.encrypt(enc, org_id)?)))
-            .collect::<Result<HashMap<_, _>>>()
+            .collect()
     }
 }
 
@@ -60,18 +77,6 @@ impl<T: Decryptable<Output>, Output, Id: Hash + Eq + Copy> Decryptable<HashMap<I
     ) -> Result<HashMap<Id, Output>> {
         self.iter()
             .map(|(id, e)| Ok((*id, e.decrypt(enc, org_id)?)))
-            .collect::<Result<HashMap<_, _>>>()
-    }
-}
-
-impl<T: Encryptable<Output>, Output> Encryptable<Output> for Box<T> {
-    fn encrypt(self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output> {
-        (*self).encrypt(enc, org_id)
-    }
-}
-
-impl<T: Decryptable<Output>, Output> Decryptable<Output> for Box<T> {
-    fn decrypt(&self, enc: &EncryptionSettings, org_id: &Option<Uuid>) -> Result<Output> {
-        (**self).decrypt(enc, org_id)
+            .collect()
     }
 }

crates/bitwarden/src/crypto/key_encryptable.rs

@@ -0,0 +1,69 @@
+use std::{collections::HashMap, hash::Hash};
+
+use crate::error::Result;
+
+use super::SymmetricCryptoKey;
+
+pub trait KeyEncryptable<Output> {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Output>;
+}
+
+pub trait KeyDecryptable<Output> {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Output>;
+}
+
+impl<T: KeyEncryptable<Output>, Output> KeyEncryptable<Option<Output>> for Option<T> {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Option<Output>> {
+        self.map(|e| e.encrypt_with_key(key)).transpose()
+    }
+}
+
+impl<T: KeyDecryptable<Output>, Output> KeyDecryptable<Option<Output>> for Option<T> {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Option<Output>> {
+        self.as_ref().map(|e| e.decrypt_with_key(key)).transpose()
+    }
+}
+
+impl<T: KeyEncryptable<Output>, Output> KeyEncryptable<Output> for Box<T> {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Output> {
+        (*self).encrypt_with_key(key)
+    }
+}
+
+impl<T: KeyDecryptable<Output>, Output> KeyDecryptable<Output> for Box<T> {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Output> {
+        (**self).decrypt_with_key(key)
+    }
+}
+
+impl<T: KeyEncryptable<Output>, Output> KeyEncryptable<Vec<Output>> for Vec<T> {
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Vec<Output>> {
+        self.into_iter().map(|e| e.encrypt_with_key(key)).collect()
+    }
+}
+
+impl<T: KeyDecryptable<Output>, Output> KeyDecryptable<Vec<Output>> for Vec<T> {
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<Output>> {
+        self.iter().map(|e| e.decrypt_with_key(key)).collect()
+    }
+}
+
+impl<T: KeyEncryptable<Output>, Output, Id: Hash + Eq> KeyEncryptable<HashMap<Id, Output>>
+    for HashMap<Id, T>
+{
+    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<HashMap<Id, Output>> {
+        self.into_iter()
+            .map(|(id, e)| Ok((id, e.encrypt_with_key(key)?)))
+            .collect()
+    }
+}
+
+impl<T: KeyDecryptable<Output>, Output, Id: Hash + Eq + Copy> KeyDecryptable<HashMap<Id, Output>>
+    for HashMap<Id, T>
+{
+    fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<HashMap<Id, Output>> {
+        self.iter()
+            .map(|(id, e)| Ok((*id, e.decrypt_with_key(key)?)))
+            .collect()
+    }
+}

crates/bitwarden/src/crypto/master_key.rs

@@ -4,8 +4,8 @@ use rand::Rng;
 use sha2::Digest;
 
 use super::{
-    encrypt_aes256, hkdf_expand, EncString, PbkdfSha256Hmac, SymmetricCryptoKey, UserKey,
-    PBKDF_SHA256_HMAC_OUT_SIZE,
+    encrypt_aes256, hkdf_expand, EncString, KeyDecryptable, PbkdfSha256Hmac, SymmetricCryptoKey,
+    UserKey, PBKDF_SHA256_HMAC_OUT_SIZE,
 };
 use crate::{client::kdf::Kdf, error::Result, util::BASE64_ENGINE};
 
@@ -53,7 +53,7 @@ impl MasterKey {
     pub(crate) fn decrypt_user_key(&self, user_key: EncString) -> Result<SymmetricCryptoKey> {
         let stretched_key = stretch_master_key(self)?;
 
-        let dec = user_key.decrypt_with_key(&stretched_key)?;
+        let dec: Vec<u8> = user_key.decrypt_with_key(&stretched_key)?;
         SymmetricCryptoKey::try_from(dec.as_slice())
     }
 }

crates/bitwarden/src/crypto/mod.rs

@@ -29,7 +29,9 @@ use crate::error::{Error, Result};
 mod enc_string;
 pub use enc_string::EncString;
 mod encryptable;
-pub use encryptable::{Decryptable, Encryptable};
+pub use encryptable::{Decryptable, Encryptable, LocateKey};
+mod key_encryptable;
+pub use key_encryptable::{KeyDecryptable, KeyEncryptable};
 mod aes_ops;
 pub use aes_ops::{decrypt_aes256, decrypt_aes256_hmac, encrypt_aes256, encrypt_aes256_hmac};
 mod symmetric_crypto_key;