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]>
trussed-dev · Jul 31, 2022 · 16003b6 · 16003b6
1 parent c360dce
commit 16003b6
Show file tree

Hide file tree

Showing 8 changed files with 21 additions and 288 deletions.
diff --git a/Cargo.toml b/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"]
 

diff --git a/src/client/mechanisms.rs b/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 {

diff --git a/src/config.rs b/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

diff --git a/src/mechanisms.rs b/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;

diff --git a/src/mechanisms/rsa2kpkcs.rs b/src/mechanisms/rsa2kpkcs.rs
diff --git a/src/store/keystore.rs b/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)?;

diff --git a/src/types.rs b/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>;

diff --git a/tests/rsa2kpkcs.rs b/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));
     })
 }