[PM-17370] Refactor symmetric keys to use enum (#127)

## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-17370

## 📔 Objective

Symmetric crypto keys (in general) are just random arrays of data - in
contrast to asymmetric keys which require specific structure. Still, it
is cryptographic best practice to split these keys apart entirely, and
not have them confused. An algorithm switch should include a change to a
new key type.

For the upcoming xchacha cipher, we will introduce a symmetric key that
is the same size as the old hmac-less cbc type 0 cipher. In the sdk we
currently just represent this as one key with two byte arrays, "key" and
"mac key" which is confusing and could lead to mis-usage, which can lead
to bugs or even cryptographic flaws.

For this reason, it is paramount to enforce the key types in the type
system. This PR refactors the current key usage to represent symmetric
keys as an enum, with the two types being the current encstring type 0
and type 2, each of which has it's own struct with the data it needs.

In a later PR, the serialization format for symmetric keys will be
adjusted to include the cipher type, so keys are not categorized based
on the size alone.

## ⏰ 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: Daniel García <[email protected]>
Co-authored-by: Oscar Hinton <[email protected]>

Author: 3 people

crates/bitwarden-core/src/auth/access_token.rs

@@ -75,7 +75,7 @@ impl FromStr for AccessToken {
         Ok(AccessToken {
             access_token_id,
             client_secret: client_secret.to_owned(),
-            encryption_key,
+            encryption_key: SymmetricCryptoKey::Aes256CbcHmacKey(encryption_key),
         })
     }
 }

crates/bitwarden-core/src/auth/auth_request.rs

@@ -70,7 +70,7 @@ pub(crate) fn auth_request_decrypt_master_key(
 
     let key = AsymmetricCryptoKey::from_der(&STANDARD.decode(private_key)?)?;
     let mut master_key: Vec<u8> = master_key.decrypt_with_key(&key)?;
-    let master_key = MasterKey::new(SymmetricCryptoKey::try_from(master_key.as_mut_slice())?);
+    let master_key = MasterKey::try_from(master_key.as_mut_slice())?;
 
     Ok(master_key.decrypt_user_key(user_key)?)
 }

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

@@ -177,7 +177,10 @@ pub async fn initialize_user_crypto(
             master_key,
             user_key,
         } => {
-            let master_key = MasterKey::new(SymmetricCryptoKey::try_from(master_key)?);
+            let mut master_key_bytes = STANDARD
+                .decode(master_key)
+                .map_err(|_| CryptoError::InvalidKey)?;
+            let master_key = MasterKey::try_from(master_key_bytes.as_mut_slice())?;
             let user_key: EncString = user_key.parse()?;
 
             client
@@ -484,7 +487,7 @@ pub fn verify_asymmetric_keys(
             .to_public_der()
             .map_err(VerifyError::PublicFailed)?;
 
-        let derived_public_key = STANDARD.encode(&derived_public_key_vec);
+        let derived_public_key = STANDARD.encode(derived_public_key_vec);
 
         if derived_public_key != request.user_public_key {
             return Err(VerifyError::KeyMismatch);

crates/bitwarden-crypto/src/aes.rs

@@ -57,25 +57,8 @@ pub(crate) fn decrypt_aes256_hmac(
     decrypt_aes256(iv, data, key)
 }
 
-/// Encrypt using AES-256 in CBC mode.
-///
-/// Behaves similar to [encrypt_aes256_hmac], but does't generate a MAC.
-///
-/// ## Returns
-///
-/// A AesCbc256_B64 EncString
-#[allow(unused)]
-pub(crate) fn encrypt_aes256(data_dec: &[u8], key: &GenericArray<u8, U32>) -> ([u8; 16], Vec<u8>) {
-    let rng = rand::thread_rng();
-    let (iv, data) = encrypt_aes256_internal(rng, data_dec, key);
-
-    (iv, data)
-}
-
 /// Encrypt using AES-256 in CBC mode with MAC.
 ///
-/// Behaves similar to [encrypt_aes256], but also generate a MAC.
-///
 /// ## Returns
 ///
 /// A AesCbc256_HmacSha256_B64 EncString
@@ -94,7 +77,6 @@ pub(crate) fn encrypt_aes256_hmac(
 /// Encrypt using AES-256 in CBC mode.
 ///
 /// Used internally by:
-/// - [encrypt_aes256]
 /// - [encrypt_aes256_hmac]
 fn encrypt_aes256_internal(
     mut rng: impl rand::RngCore,
@@ -185,15 +167,4 @@ mod tests {
 
         assert_eq!(String::from_utf8(decrypted).unwrap(), "EncryptMe!");
     }
-
-    #[test]
-    fn test_encrypt_decrypt_aes256() {
-        let key = generate_generic_array(0, 1);
-        let data = "EncryptMe!";
-
-        let (iv, encrypted) = encrypt_aes256(data.as_bytes(), &key);
-        let decrypted = decrypt_aes256(&iv, encrypted, &key).unwrap();
-
-        assert_eq!(String::from_utf8(decrypted).unwrap(), "EncryptMe!");
-    }
 }

crates/bitwarden-crypto/src/enc_string/symmetric.rs

@@ -1,14 +1,12 @@
 use std::{fmt::Display, str::FromStr};
 
-use aes::cipher::typenum::U32;
 use base64::{engine::general_purpose::STANDARD, Engine};
-use generic_array::GenericArray;
 use serde::Deserialize;
 
 use super::{check_length, from_b64, from_b64_vec, split_enc_string};
 use crate::{
-    error::{CryptoError, EncStringParseError, Result},
-    KeyDecryptable, KeyEncryptable, SymmetricCryptoKey,
+    error::{CryptoError, EncStringParseError, Result, UnsupportedOperation},
+    Aes256CbcHmacKey, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey,
 };
 
 #[cfg(feature = "wasm")]
@@ -195,10 +193,10 @@ impl serde::Serialize for EncString {
 impl EncString {
     pub(crate) fn encrypt_aes256_hmac(
         data_dec: &[u8],
-        mac_key: &GenericArray<u8, U32>,
-        key: &GenericArray<u8, U32>,
+        key: &Aes256CbcHmacKey,
     ) -> Result<EncString> {
-        let (iv, mac, data) = crate::aes::encrypt_aes256_hmac(data_dec, mac_key, key)?;
+        let (iv, mac, data) =
+            crate::aes::encrypt_aes256_hmac(data_dec, &key.mac_key, &key.enc_key)?;
         Ok(EncString::AesCbc256_HmacSha256_B64 { iv, mac, data })
     }
 
@@ -213,31 +211,26 @@ impl EncString {
 
 impl KeyEncryptable<SymmetricCryptoKey, EncString> for &[u8] {
     fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<EncString> {
-        EncString::encrypt_aes256_hmac(
-            self,
-            key.mac_key.as_ref().ok_or(CryptoError::InvalidMac)?,
-            &key.key,
-        )
+        match key {
+            SymmetricCryptoKey::Aes256CbcHmacKey(key) => EncString::encrypt_aes256_hmac(self, key),
+            SymmetricCryptoKey::Aes256CbcKey(_) => Err(CryptoError::OperationNotSupported(
+                UnsupportedOperation::EncryptionNotImplementedForKey,
+            )),
+        }
     }
 }
 
 impl KeyDecryptable<SymmetricCryptoKey, Vec<u8>> for EncString {
     fn decrypt_with_key(&self, key: &SymmetricCryptoKey) -> Result<Vec<u8>> {
-        match self {
-            EncString::AesCbc256_B64 { iv, data } => {
-                if key.mac_key.is_some() {
-                    return Err(CryptoError::MacNotProvided);
-                }
-
-                let dec = crate::aes::decrypt_aes256(iv, data.clone(), &key.key)?;
-                Ok(dec)
-            }
-            EncString::AesCbc256_HmacSha256_B64 { iv, mac, data } => {
-                let mac_key = key.mac_key.as_ref().ok_or(CryptoError::InvalidMac)?;
-                let dec =
-                    crate::aes::decrypt_aes256_hmac(iv, mac, data.clone(), mac_key, &key.key)?;
-                Ok(dec)
+        match (self, key) {
+            (EncString::AesCbc256_B64 { iv, data }, SymmetricCryptoKey::Aes256CbcKey(key)) => {
+                crate::aes::decrypt_aes256(iv, data.clone(), &key.enc_key)
             }
+            (
+                EncString::AesCbc256_HmacSha256_B64 { iv, mac, data },
+                SymmetricCryptoKey::Aes256CbcHmacKey(key),
+            ) => crate::aes::decrypt_aes256_hmac(iv, mac, data.clone(), &key.mac_key, &key.enc_key),
+            _ => Err(CryptoError::WrongKeyType),
         }
     }
 }
@@ -284,7 +277,7 @@ mod tests {
 
     #[test]
     fn test_enc_string_roundtrip() {
-        let key = derive_symmetric_key("test");
+        let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));
 
         let test_string = "encrypted_test_string";
         let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap();
@@ -295,7 +288,7 @@ mod tests {
 
     #[test]
     fn test_enc_string_ref_roundtrip() {
-        let key = derive_symmetric_key("test");
+        let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));
 
         let test_string = "encrypted_test_string";
         let cipher = test_string.encrypt_with_key(&key).unwrap();
@@ -390,7 +383,7 @@ mod tests {
         assert_eq!(enc_string.enc_type(), 0);
 
         let result: Result<String, CryptoError> = enc_string.decrypt_with_key(&key);
-        assert!(matches!(result, Err(CryptoError::MacNotProvided)));
+        assert!(matches!(result, Err(CryptoError::WrongKeyType)));
     }
 
     #[test]

crates/bitwarden-crypto/src/error.rs

@@ -47,6 +47,18 @@ pub enum CryptoError {
 
     #[error("Number is zero")]
     ZeroNumber,
+
+    #[error("Unsupported operation, {0}")]
+    OperationNotSupported(UnsupportedOperation),
+
+    #[error("Key algorithm does not match encrypted data type")]
+    WrongKeyType,
+}
+
+#[derive(Debug, Error)]
+pub enum UnsupportedOperation {
+    #[error("Encryption is not implemented for key")]
+    EncryptionNotImplementedForKey,
 }
 
 #[derive(Debug, Error)]

crates/bitwarden-crypto/src/keys/device_key.rs

@@ -91,7 +91,7 @@ mod tests {
 
     #[test]
     fn test_trust_device() {
-        let key = derive_symmetric_key("test");
+        let key = SymmetricCryptoKey::Aes256CbcHmacKey(derive_symmetric_key("test"));
 
         let result = DeviceKey::trust_device(&key).unwrap();
 
@@ -103,8 +103,8 @@ mod tests {
             )
             .unwrap();
 
-        assert_eq!(key.key, decrypted.key);
-        assert_eq!(key.mac_key, decrypted.mac_key);
+        assert_eq!(key, decrypted);
+        assert_eq!(key, decrypted);
     }
 
     #[test]
@@ -134,7 +134,6 @@ mod tests {
             .decrypt_user_key(protected_device_private_key, protected_user_key)
             .unwrap();
 
-        assert_eq!(decrypted.key, user_key.key);
-        assert_eq!(decrypted.mac_key, user_key.mac_key);
+        assert_eq!(decrypted, user_key);
     }
 }