WIP: Address review comments - 1

In a separate commit for better visibility.

Removed addressed TODOs, reworded those that should stay for later (magic numbers),
rebased to the latest "main".

Signed-off-by: alt3r 3go <[email protected]>

Cargo.toml

@@ -91,9 +91,7 @@ default-mechanisms = [
     "tdes",
     "totp",
     "trng",
-    "rsa2k-pkcs",
-    "rsa3k",
-    "rsa4k"
+    "rsa2k"
 ]
 aes256-cbc = []
 chacha8-poly1305 = []
@@ -108,7 +106,7 @@ sha256 = []
 tdes = ["des"]
 totp = ["sha-1"]
 trng = ["sha-1"]
-rsa2k-pkcs = ["rsa"]
+rsa2k = ["rsa"]
 rsa3k = ["rsa"]
 rsa4k = ["rsa"]
 

src/client/mechanisms.rs

@@ -245,7 +245,7 @@ pub trait P256: CryptoClient {
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl<S: Syscall> Rsa2kPkcs for ClientImplementation<S> {}
 
 pub trait Rsa2kPkcs: CryptoClient {

src/config.rs

@@ -44,23 +44,21 @@ cfg_if::cfg_if! {
 }
 pub const MAX_SHORT_DATA_LENGTH: usize = 128;
 
-// TODO:alt3r-3go: Do we want better keylength granularity here?
-#[cfg(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k"))]
+#[cfg(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k"))]
 pub const MAX_SIGNATURE_LENGTH: usize = 512;
-#[cfg(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k"))]
-// TODO:alt3r-3go: We use PKCS#8 DER format, this value was found empirically for 2K keys. Need to generalize.
+#[cfg(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k"))]
+// TODO: We use PKCS#8 DER format, this value was found empirically for 2K keys. Need to generalize.
 pub const MAX_KEY_MATERIAL_LENGTH: usize = 1217;
-#[cfg(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k"))]
-// TODO:alt3r-3go: This is due to the fact that KEY_MATERIAL_LENGTH is now bigger than MESSAGE_LENGTH.
-//                 Double-check this is okay.
+#[cfg(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k"))]
+// This is due to the fact that KEY_MATERIAL_LENGTH is bigger than MESSAGE_LENGTH for RSA.
 pub const MAX_MESSAGE_LENGTH: usize = MAX_KEY_MATERIAL_LENGTH;
 
 
-#[cfg(not(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k")))]
+#[cfg(not(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k")))]
 pub const MAX_SIGNATURE_LENGTH: usize = 72;
-#[cfg(not(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k")))]
+#[cfg(not(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k")))]
 pub const MAX_KEY_MATERIAL_LENGTH: usize = 128;
-#[cfg(not(any(feature = "rsa2k-pkcs", feature = "rsa3k", feature = "rsa4k")))]
+#[cfg(not(any(feature = "rsa2k", feature = "rsa3k", feature = "rsa4k")))]
 pub const MAX_MESSAGE_LENGTH: usize = 1024;
 
 // must be MAX_KEY_MATERIAL_LENGTH + 4

src/mechanisms.rs

@@ -47,7 +47,8 @@ pub struct P256Prehashed {}
 mod p256;
 
 pub struct Rsa2kPkcs {}
-mod rsa2kpkcs;
+// Later on we'll add: "pub struct Rsa2kPss {}" and so on
+mod rsa2k;
 
 pub struct Sha256 {}
 mod sha256;

src/mechanisms/rsa2kpkcs.rs → src/mechanisms/rsa2k.rs

@@ -12,11 +12,7 @@ use crate::error::Error;
 use crate::service::*;
 use crate::types::*;
 
-//TODO:alt3r-3go: sign() and verify() are the only two methods that are actually different between -pkcs and -pss.
-//                Moreover, the key::Kind::Rsa2K could also probably be parametrized, instead of having a dedicated kind
-//                for each. Overall this means the class structure can probably be simplified - need to decide.
-
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl DeriveKey for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -55,7 +51,7 @@ impl DeriveKey for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl DeserializeKey for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -73,7 +69,7 @@ impl DeserializeKey for super::Rsa2kPkcs
         let private_key: RsaPrivateKey = DecodePrivateKey::from_pkcs8_der(&request.serialized_key)
             .map_err(|_| Error::InvalidSerializedKey)?;
 
-        // We store our keys in PKCS#8 DER format as well
+        // We store our keys in PKCS#8 DER format
         let private_key_der = private_key.to_pkcs8_der()
             .expect("Failed to serialize an RSA 2K private key to PKCS#8 DER");
 
@@ -88,7 +84,7 @@ impl DeserializeKey for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl GenerateKey for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -125,7 +121,7 @@ impl GenerateKey for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl SerializeKey for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -140,8 +136,6 @@ impl SerializeKey for super::Rsa2kPkcs
             .material;
 
         let serialized_key = match request.format {
-            // TODO:alt3r-3go: There are "Der" and "Asn1Der" commented out in KeySerialization enum,
-            //                 should those be used instead?
             KeySerialization::Raw => {
                 let mut serialized_key = Message::new();
                 serialized_key.extend_from_slice(&priv_key_der).map_err(|_| Error::InternalError)?;
@@ -155,7 +149,7 @@ impl SerializeKey for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl Exists for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -169,7 +163,7 @@ impl Exists for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl Sign for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -188,12 +182,9 @@ impl Sign for super::Rsa2kPkcs
             .expect("Failed to deserialize an RSA 2K private key from PKCS#8 DER");
 
         // RSA lib takes in a hash value to sign, not raw data.
-        // TODO:alt3r-3go: Do we assume we get digest into this function, or we calculate it ourselves?
-        // use sha2::digest::Digest;
-        // let digest_to_sign: [u8; 32] = sha2::Sha256::digest(&request.message).into();
+        // We assume we get digest into this function, too.
 
-        // TODO:alt3r-3go: There's also .sign_blinded(), which is supposed to protect the private key from timing side channels,
-        //                 but requires an RNG instance - decide if we want to always use it.
+        // TODO: Consider using .sign_blinded(), which is supposed to protect the private key from timing side channels
         use rsa::padding::PaddingScheme;
         use rsa::hash::Hash;
         let native_signature = priv_key
@@ -211,7 +202,7 @@ impl Sign for super::Rsa2kPkcs
     }
 }
 
-#[cfg(feature = "rsa2k-pkcs")]
+#[cfg(feature = "rsa2k")]
 impl Verify for super::Rsa2kPkcs
 {
     #[inline(never)]
@@ -223,7 +214,7 @@ impl Verify for super::Rsa2kPkcs
             return Err(Error::InvalidSerializationFormat);
         }
 
-        // TODO:alt3r-3go: This must not be a hardcoded magic number, need to generalize
+        // TODO: This must not be a hardcoded magic number, convert when a common mechanism is available
         if request.signature.len() != 256 {
             return Err(Error::WrongSignatureLength);
         }
@@ -250,11 +241,11 @@ impl Verify for super::Rsa2kPkcs
     }
 }
 
-#[cfg(not(feature = "rsa2k-pkcs"))]
+#[cfg(not(feature = "rsa2k"))]
 impl DeriveKey for super::Rsa2kPkcs {}
-#[cfg(not(feature = "rsa2k-pkcs"))]
+#[cfg(not(feature = "rsa2k"))]
 impl GenerateKey for super::Rsa2kPkcs {}
-#[cfg(not(feature = "rsa2k-pkcs"))]
+#[cfg(not(feature = "rsa2k"))]
 impl Sign for super::Rsa2kPkcs {}
-#[cfg(not(feature = "rsa2k-pkcs"))]
+#[cfg(not(feature = "rsa2k"))]
 impl Verify for super::Rsa2kPkcs {}

src/store/keystore.rs

@@ -147,11 +147,9 @@ impl<P: Platform> Keystore for ClientKeystore<P> {
 
         let location = self.location(secrecy, id).ok_or(Error::NoSuchKey)?;
 
-        //TODO:alt3r-3go: This should better be defined in some way, instead of hardcoding.
-        //                I've tried referring to MAX_SERIALIZED_KEY_LENGTH, is this a good idea?
-        #[cfg(not(feature = "rsa2k-pkcs"))]
+        #[cfg(not(feature = "rsa2k"))]
         let bytes: Bytes<128> = store::read(self.store, location, &path)?;
-        #[cfg(feature = "rsa2k-pkcs")]
+        #[cfg(feature = "rsa2k")]
         let bytes: Bytes<MAX_SERIALIZED_KEY_LENGTH> = store::read(self.store, location, &path)?;
 
         let key = key::Key::try_deserialize(&bytes)?;

src/types.rs

@@ -509,8 +509,11 @@ pub enum Mechanism {
     Trng,
     X255,
     Rsa2kPkcs,
-    Rsa3k,
-    Rsa4k,
+    Rsa2kPss,
+    Rsa3kPkcs,
+    Rsa3kPss,
+    Rsa4kPkcs,
+    Rsa4kPss,
 }
 
 pub type LongData = Bytes<MAX_LONG_DATA_LENGTH>;

tests/rsa2kpkcs.rs

@@ -9,17 +9,14 @@ use trussed::types::KeySerialization;
 use trussed::types::Location::*;
 use trussed::types::StorageAttributes;
 
-// TODO:alt3r-3go: Looks like the test infra is not supposed to be used with several tests like below -
-//                 right now it randomly fails with SIGSERV on either of the two, when run together,
-//                 but never when run separately. Need to investigate and fix.
+// Tests below can be run on a PC using the "virt" feature
 
 #[test]
 fn rsa2kpkcs_generate_key() {
     client::get(|client| {
         let sk = syscall!(client.generate_rsa2kpkcs_private_key(Internal)).key;
 
         // This assumes we don't ever get a key with ID 0
-        // TODO:alt3r-3go: make sure the above always holds or find a better way to check for success
         assert_ne!(sk, KeyId::from_special(0));
     })
 }
@@ -31,7 +28,6 @@ fn rsa2kpkcs_derive_key() {
         let pk = syscall!(client.derive_rsa2kpkcs_public_key(sk, Volatile)).key;
 
         // This assumes we don't ever get a key with ID 0
-        // TODO:alt3r-3go: make sure the above always holds or find a better way to check for success
         assert_ne!(pk, KeyId::from_special(0));
     })
 }
@@ -67,7 +63,6 @@ fn rsa2kpkcs_deserialize_key() {
         let deserialized_key_id = syscall!(client.deserialize_rsa2kpkcs_key(&serialized_key, KeySerialization::Raw, location)).key;
 
         // This assumes we don't ever get a key with ID 0
-        // TODO:alt3r-3go: make sure the above always holds or find a better way to check for success
         assert_ne!(deserialized_key_id, KeyId::from_special(0));
     })
 }