Implement Eq and PartialEq for Sensitive and their inner types (#747)

## Type of change ``` - [ ] Bug fix - [ ] New feature development - [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective With this, we can remove all the uses of .`expose()` from tests, which makes it much easier to see possible areas of improvement for Sensitive coverage. With this PR we can compare the following: - `Sensitive<V>` == `Sensitive<V>` - `Sensitive<V>` == `V` - `Sensitive<String>` == `&str` - `Sensitive<Vec<u8>>` == `&[u8]` I tried to replace the last two with a generic impl that would just take AsRef<> instead, but couldn't do it without conflicting implementations. Note that the position is important, for example `V` == `Sensitive<V>` will not compile, and I don't think we can implement a blanket `Eq` for a generic `V` without running afoul of the orphan rules.
bitwarden · Apr 29, 2024 · 964dc9d · 964dc9d
1 parent 7bf6ecb
commit 964dc9d
Show file tree

Hide file tree

Showing 19 changed files with 84 additions and 70 deletions.
diff --git a/crates/bitwarden-crypto/src/enc_string/asymmetric.rs b/crates/bitwarden-crypto/src/enc_string/asymmetric.rs
@@ -10,6 +10,7 @@ use crate::{
     error::{CryptoError, EncStringParseError, Result},
     rsa::encrypt_rsa2048_oaep_sha1,
     AsymmetricCryptoKey, AsymmetricEncryptable, DecryptedString, DecryptedVec, KeyDecryptable,
+    SensitiveVec,
 };
 
 // This module is a workaround to avoid deprecated warnings that come from the ZeroizeOnDrop
@@ -146,10 +147,10 @@ impl serde::Serialize for AsymmetricEncString {
 impl AsymmetricEncString {
     /// Encrypt and produce a [AsymmetricEncString::Rsa2048_OaepSha1_B64] variant.
     pub fn encrypt_rsa2048_oaep_sha1(
-        data_dec: &[u8],
+        data_dec: SensitiveVec,
         key: &dyn AsymmetricEncryptable,
     ) -> Result<AsymmetricEncString> {
-        let enc = encrypt_rsa2048_oaep_sha1(key.to_public_key(), data_dec)?;
+        let enc = encrypt_rsa2048_oaep_sha1(key.to_public_key(), data_dec.expose())?;
         Ok(AsymmetricEncString::Rsa2048_OaepSha1_B64 { data: enc })
     }
 
@@ -254,7 +255,7 @@ XKZBokBGnjFnTnKcs7nv/O8=
         assert_eq!(enc_string.enc_type(), 3);
 
         let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res.expose(), "EncryptMe!");
+        assert_eq!(res, "EncryptMe!");
     }
 
     #[test]
@@ -266,7 +267,7 @@ XKZBokBGnjFnTnKcs7nv/O8=
         assert_eq!(enc_string.enc_type(), 4);
 
         let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res.expose(), "EncryptMe!");
+        assert_eq!(res, "EncryptMe!");
     }
 
     #[test]
@@ -278,7 +279,7 @@ XKZBokBGnjFnTnKcs7nv/O8=
         assert_eq!(enc_string.enc_type(), 6);
 
         let res: DecryptedString = enc_string.decrypt_with_key(&private_key).unwrap();
-        assert_eq!(res.expose(), "EncryptMe!");
+        assert_eq!(res, "EncryptMe!");
     }
 
     #[test]

diff --git a/crates/bitwarden-crypto/src/enc_string/symmetric.rs b/crates/bitwarden-crypto/src/enc_string/symmetric.rs
@@ -317,7 +317,7 @@ mod tests {
         let cipher = test_string.to_owned().encrypt_with_key(&key).unwrap();
 
         let decrypted_str: SensitiveString = cipher.decrypt_with_key(&key).unwrap();
-        assert_eq!(decrypted_str.expose(), test_string);
+        assert_eq!(decrypted_str, test_string);
     }
 
     #[test]
@@ -415,7 +415,7 @@ mod tests {
         assert_eq!(enc_string.enc_type(), 0);
 
         let dec_str: SensitiveString = enc_string.decrypt_with_key(&key).unwrap();
-        assert_eq!(dec_str.expose(), "EncryptMe!");
+        assert_eq!(dec_str, "EncryptMe!");
     }
 
     #[test]
@@ -428,7 +428,7 @@ mod tests {
         assert_eq!(enc_string.enc_type(), 1);
 
         let dec_str: SensitiveString = enc_string.decrypt_with_key(&key).unwrap();
-        assert_eq!(dec_str.expose(), "EncryptMe!");
+        assert_eq!(dec_str, "EncryptMe!");
     }
 
     #[test]

diff --git a/crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs b/crates/bitwarden-crypto/src/keys/asymmetric_crypto_key.rs
@@ -172,8 +172,8 @@ DnqOsltgPomWZ7xVfMkm9niL2OA=
         assert_eq!(pem_key.key, der_key.key);
 
         // Check that the keys can be converted back to DER
-        assert_eq!(&der_key.to_der().unwrap(), der_key_vec.expose());
-        assert_eq!(&pem_key.to_der().unwrap(), der_key_vec.expose());
+        assert_eq!(der_key_vec, der_key.to_der().unwrap().as_slice());
+        assert_eq!(der_key_vec, pem_key.to_der().unwrap().as_slice());
     }
 
     #[test]
@@ -224,12 +224,12 @@ DnqOsltgPomWZ7xVfMkm9niL2OA=
         let private_key = AsymmetricCryptoKey::from_der(private_key).unwrap();
         let public_key = AsymmetricPublicCryptoKey::from_der(public_key).unwrap();
 
-        let plaintext = "Hello, world!";
+        let plaintext = SensitiveString::test("Hello, world!");
         let encrypted =
-            AsymmetricEncString::encrypt_rsa2048_oaep_sha1(plaintext.as_bytes(), &public_key)
+            AsymmetricEncString::encrypt_rsa2048_oaep_sha1(plaintext.clone().into(), &public_key)
                 .unwrap();
         let decrypted: DecryptedString = encrypted.decrypt_with_key(&private_key).unwrap();
 
-        assert_eq!(plaintext, decrypted.expose());
+        assert_eq!(plaintext, decrypted);
     }
 }
diff --git a/crates/bitwarden-crypto/src/keys/device_key.rs b/crates/bitwarden-crypto/src/keys/device_key.rs
@@ -38,7 +38,7 @@ impl DeviceKey {
         let data = user_key.to_vec();
 
         let protected_user_key =
-            AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data.expose(), &device_private_key)?;
+            AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, &device_private_key)?;
 
         let protected_device_public_key = device_private_key
             .to_public_der()?

diff --git a/crates/bitwarden-crypto/src/keys/master_key.rs b/crates/bitwarden-crypto/src/keys/master_key.rs
@@ -190,11 +190,10 @@ mod tests {
         let master_key = MasterKey::derive(&password, salt, &kdf).unwrap();
 
         assert_eq!(
-            "ZF6HjxUTSyBHsC+HXSOhZoXN+UuMnygV5YkWXCY4VmM=",
             master_key
                 .derive_master_key_hash(&password, HashPurpose::ServerAuthorization)
-                .unwrap()
-                .expose(),
+                .unwrap(),
+            "ZF6HjxUTSyBHsC+HXSOhZoXN+UuMnygV5YkWXCY4VmM=",
         );
     }
 
@@ -211,11 +210,10 @@ mod tests {
         let master_key = MasterKey::derive(&password, salt, &kdf).unwrap();
 
         assert_eq!(
-            "PR6UjYmjmppTYcdyTiNbAhPJuQQOmynKbdEl1oyi/iQ=",
             master_key
                 .derive_master_key_hash(&password, HashPurpose::ServerAuthorization)
-                .unwrap()
-                .expose(),
+                .unwrap(),
+            "PR6UjYmjmppTYcdyTiNbAhPJuQQOmynKbdEl1oyi/iQ=",
         );
     }
 

diff --git a/crates/bitwarden-crypto/src/keys/shareable_key.rs b/crates/bitwarden-crypto/src/keys/shareable_key.rs
@@ -48,13 +48,13 @@ mod tests {
             "test_key",
             None,
         );
-        assert_eq!(key.to_base64().expose(), "4PV6+PcmF2w7YHRatvyMcVQtI7zvCyssv/wFWmzjiH6Iv9altjmDkuBD1aagLVaLezbthbSe+ktR+U6qswxNnQ==");
+        assert_eq!(key.to_base64(), "4PV6+PcmF2w7YHRatvyMcVQtI7zvCyssv/wFWmzjiH6Iv9altjmDkuBD1aagLVaLezbthbSe+ktR+U6qswxNnQ==");
 
         let key = derive_shareable_key(
             Sensitive::new(Box::new(*b"67t9b5g67$%Dh89n")),
             "test_key",
             Some("test"),
         );
-        assert_eq!(key.to_base64().expose(), "F9jVQmrACGx9VUPjuzfMYDjr726JtL300Y3Yg+VYUnVQtQ1s8oImJ5xtp1KALC9h2nav04++1LDW4iFD+infng==");
+        assert_eq!(key.to_base64(), "F9jVQmrACGx9VUPjuzfMYDjr726JtL300Y3Yg+VYUnVQtQ1s8oImJ5xtp1KALC9h2nav04++1LDW4iFD+infng==");
     }
 }
diff --git a/crates/bitwarden-crypto/src/lib.rs b/crates/bitwarden-crypto/src/lib.rs
@@ -23,7 +23,7 @@
 //!   let encrypted = data.clone().encrypt_with_key(&key)?;
 //!   let decrypted: DecryptedString = encrypted.decrypt_with_key(&key)?;
 //!
-//!   assert_eq!(&data, decrypted.expose());
+//!   assert_eq!(decrypted, data);
 //!   Ok(())
 //! }
 //! ```

diff --git a/crates/bitwarden-crypto/src/sensitive/sensitive.rs b/crates/bitwarden-crypto/src/sensitive/sensitive.rs
@@ -17,7 +17,7 @@ use crate::CryptoError;
 ///
 /// Internally [`Sensitive`] contains a [`Box`] which ensures the value is placed on the heap. It
 /// implements the [`Drop`] trait which calls `zeroize` on the inner value.
-#[derive(PartialEq, Clone, Zeroize, ZeroizeOnDrop)]
+#[derive(Eq, Clone, Zeroize, ZeroizeOnDrop)]
 pub struct Sensitive<V: Zeroize> {
     pub(super) value: Box<V>,
 }
@@ -151,6 +151,27 @@ impl<V: Zeroize + Serialize> fmt::Debug for Sensitive<V> {
     }
 }
 
+impl<V: Zeroize + PartialEq<V>> PartialEq<Sensitive<V>> for Sensitive<V> {
+    fn eq(&self, other: &Self) -> bool {
+        self.value.eq(&other.value)
+    }
+}
+impl<V: Zeroize + PartialEq<V>> PartialEq<V> for Sensitive<V> {
+    fn eq(&self, other: &V) -> bool {
+        self.value.as_ref().eq(other)
+    }
+}
+impl PartialEq<&str> for SensitiveString {
+    fn eq(&self, other: &&str) -> bool {
+        self.value.as_ref().eq(other)
+    }
+}
+impl PartialEq<&[u8]> for SensitiveVec {
+    fn eq(&self, other: &&[u8]) -> bool {
+        self.value.as_ref().eq(other)
+    }
+}
+
 /// Unfortunately once we serialize a `SensitiveString` we can't control the future memory.
 impl<V: Zeroize + Serialize> Serialize for Sensitive<V> {
     fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {

diff --git a/crates/bitwarden/src/auth/access_token.rs b/crates/bitwarden/src/auth/access_token.rs
@@ -79,7 +79,7 @@ mod tests {
             "ec2c1d46-6a4b-4751-a310-af9601317f2d"
         );
         assert_eq!(token.client_secret, "C2IgxjjLF7qSshsbwe8JGcbM075YXw");
-        assert_eq!(token.encryption_key.to_base64().expose(), "H9/oIRLtL9nGCQOVDjSMoEbJsjWXSOCb3qeyDt6ckzS3FhyboEDWyTP/CQfbIszNmAVg2ExFganG1FVFGXO/Jg==");
+        assert_eq!(token.encryption_key.to_base64(), "H9/oIRLtL9nGCQOVDjSMoEbJsjWXSOCb3qeyDt6ckzS3FhyboEDWyTP/CQfbIszNmAVg2ExFganG1FVFGXO/Jg==");
     }
 
     #[test]

diff --git a/crates/bitwarden/src/auth/auth_request.rs b/crates/bitwarden/src/auth/auth_request.rs
@@ -98,7 +98,7 @@ pub(crate) fn approve_auth_request(
     let key = enc.get_key(&None).ok_or(Error::VaultLocked)?;
 
     Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1(
-        key.to_vec().expose(),
+        key.to_vec(),
         &public_key,
     )?)
 }
@@ -107,22 +107,23 @@ pub(crate) fn approve_auth_request(
 fn test_auth_request() {
     let request = new_auth_request("[email protected]").unwrap();
 
-    let secret: &[u8] = &[
+    let secret = bitwarden_crypto::SensitiveVec::test(&[
         111, 32, 97, 169, 4, 241, 174, 74, 239, 206, 113, 86, 174, 68, 216, 238, 52, 85, 156, 27,
         134, 149, 54, 55, 91, 147, 45, 130, 131, 237, 51, 31, 191, 106, 155, 14, 160, 82, 47, 40,
         96, 31, 114, 127, 212, 187, 167, 110, 205, 116, 198, 243, 218, 72, 137, 53, 248, 43, 255,
         67, 35, 61, 245, 93,
-    ];
+    ]);
 
     let private_key = SensitiveString::new(Box::new(request.private_key.clone()));
     let private_key =
         AsymmetricCryptoKey::from_der(private_key.decode_base64(STANDARD).unwrap()).unwrap();
 
-    let encrypted = AsymmetricEncString::encrypt_rsa2048_oaep_sha1(secret, &private_key).unwrap();
+    let encrypted =
+        AsymmetricEncString::encrypt_rsa2048_oaep_sha1(secret.clone(), &private_key).unwrap();
 
     let decrypted = auth_request_decrypt_user_key(request.private_key, encrypted).unwrap();
 
-    assert_eq!(decrypted.to_vec().expose(), secret);
+    assert_eq!(decrypted.to_vec(), secret);
 }
 
 #[cfg(test)]
@@ -171,11 +172,12 @@ mod tests {
         let dec = auth_request_decrypt_user_key(private_key.to_owned(), enc_user_key).unwrap();
 
         assert_eq!(
-            dec.to_vec().expose(),
-            &[
+            dec.to_vec(),
+            [
                 201, 37, 234, 213, 21, 75, 40, 70, 149, 213, 234, 16, 19, 251, 162, 245, 161, 74,
                 34, 245, 211, 151, 211, 192, 95, 10, 117, 50, 88, 223, 23, 157
             ]
+            .as_slice()
         );
     }
 
@@ -190,13 +192,14 @@ mod tests {
                 .unwrap();
 
         assert_eq!(
-            dec.to_vec().expose(),
-            &[
+            dec.to_vec(),
+            [
                 109, 128, 172, 147, 206, 123, 134, 95, 16, 36, 155, 113, 201, 18, 186, 230, 216,
                 212, 173, 188, 74, 11, 134, 131, 137, 242, 105, 178, 105, 126, 52, 139, 248, 91,
                 215, 21, 128, 91, 226, 222, 165, 67, 251, 34, 83, 81, 77, 147, 225, 76, 13, 41,
                 102, 45, 183, 218, 106, 89, 254, 208, 251, 101, 130, 10,
             ]
+            .as_slice()
         );
     }
 

diff --git a/crates/bitwarden/src/auth/mod.rs b/crates/bitwarden/src/auth/mod.rs
@@ -59,9 +59,6 @@ mod tests {
 
         let result = determine_password_hash(email, &kdf, &password, purpose).unwrap();
 
-        assert_eq!(
-            result.expose(),
-            "7kTqkF1pY/3JeOu73N9kR99fDDe9O1JOZaVc7KH3lsU="
-        );
+        assert_eq!(result, "7kTqkF1pY/3JeOu73N9kR99fDDe9O1JOZaVc7KH3lsU=");
     }
 }
diff --git a/crates/bitwarden/src/auth/password/validate.rs b/crates/bitwarden/src/auth/password/validate.rs
@@ -29,7 +29,7 @@ pub(crate) fn validate_password(
                     HashPurpose::LocalAuthorization,
                 )?;
 
-                Ok(hash.expose() == password_hash.expose())
+                Ok(hash == password_hash)
             }
         }
     } else {
@@ -146,10 +146,7 @@ mod tests {
         )
         .unwrap();
 
-        assert_eq!(
-            result.expose(),
-            "aOvkBXFhSdgrBWR3hZCMRoML9+h5yRblU3lFphCdkeA="
-        );
+        assert_eq!(result, "aOvkBXFhSdgrBWR3hZCMRoML9+h5yRblU3lFphCdkeA=");
         assert!(validate_password(&client, password.try_into().unwrap(), result).unwrap())
     }
 

diff --git a/crates/bitwarden/src/auth/tde.rs b/crates/bitwarden/src/auth/tde.rs
@@ -25,7 +25,7 @@ pub(super) fn make_register_tde_keys(
     let key_pair = user_key.make_key_pair()?;
 
     let admin_reset =
-        AsymmetricEncString::encrypt_rsa2048_oaep_sha1(user_key.0.to_vec().expose(), &public_key)?;
+        AsymmetricEncString::encrypt_rsa2048_oaep_sha1(user_key.0.to_vec(), &public_key)?;
 
     let device_key = if remember_device {
         Some(DeviceKey::trust_device(&user_key.0)?)

diff --git a/crates/bitwarden/src/mobile/crypto.rs b/crates/bitwarden/src/mobile/crypto.rs
@@ -315,7 +315,7 @@ pub(super) fn enroll_admin_password_reset(
     let key = enc.get_key(&None).ok_or(Error::VaultLocked)?;
 
     Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1(
-        key.to_vec().expose(),
+        key.to_vec(),
         &public_key,
     )?)
 }
@@ -381,10 +381,7 @@ mod tests {
             .await
             .unwrap();
 
-        assert_eq!(
-            new_hash.expose(),
-            new_password_response.password_hash.expose()
-        );
+        assert_eq!(new_hash, new_password_response.password_hash);
 
         assert_eq!(
             client
@@ -540,6 +537,6 @@ mod tests {
             .unwrap()
             .get_key(&None)
             .unwrap();
-        assert_eq!(decrypted.expose(), expected.to_vec().expose());
+        assert_eq!(decrypted, expected.to_vec());
     }
 }
diff --git a/crates/bitwarden/src/tool/exporters/mod.rs b/crates/bitwarden/src/tool/exporters/mod.rs
@@ -227,7 +227,7 @@ mod tests {
             f.id,
             "fd411a1a-fec8-4070-985d-0e6560860e69".parse().unwrap()
         );
-        assert_eq!(f.name.expose(), "test_name");
+        assert_eq!(f.name, "test_name");
     }
 
     #[test]
@@ -274,7 +274,7 @@ mod tests {
             "fd411a1a-fec8-4070-985d-0e6560860e69".parse().unwrap()
         );
         assert_eq!(cipher.folder_id, None);
-        assert_eq!(cipher.name.expose(), "My login");
+        assert_eq!(cipher.name, "My login");
         assert_eq!(cipher.notes, None);
         assert!(!cipher.favorite);
         assert_eq!(cipher.reprompt, 0);
@@ -290,8 +290,8 @@ mod tests {
         assert_eq!(cipher.deleted_date, None);
 
         if let bitwarden_exporters::CipherType::Login(l) = cipher.r#type {
-            assert_eq!(l.username.unwrap().expose(), "test_username");
-            assert_eq!(l.password.unwrap().expose(), "test_password");
+            assert_eq!(l.username.unwrap(), "test_username");
+            assert_eq!(l.password.unwrap(), "test_password");
             assert!(l.login_uris.is_empty());
             assert_eq!(l.totp, None);
         } else {

diff --git a/crates/bitwarden/src/vault/cipher/attachment.rs b/crates/bitwarden/src/vault/cipher/attachment.rs
@@ -200,6 +200,6 @@ mod tests {
         .decrypt_with_key(&user_key)
         .unwrap();
 
-        assert_eq!(dec.expose(), &original);
+        assert_eq!(dec, original);
     }
 }