From c7cca842ae89e6d6c70f44038a54b460ecd844aa Mon Sep 17 00:00:00 2001 From: alt3r 3go Date: Sun, 31 Jul 2022 15:57:39 +0200 Subject: [PATCH] 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 --- Cargo.toml | 6 ++-- src/client/mechanisms.rs | 2 +- src/config.rs | 18 +++++------ src/mechanisms.rs | 3 +- src/mechanisms/{rsa2kpkcs.rs => rsa2k.rs} | 39 +++++++++-------------- src/store/keystore.rs | 6 ++-- src/types.rs | 7 ++-- tests/rsa2kpkcs.rs | 7 +--- 8 files changed, 36 insertions(+), 52 deletions(-) rename src/mechanisms/{rsa2kpkcs.rs => rsa2k.rs} (84%) diff --git a/Cargo.toml b/Cargo.toml index 7db5cb76657..51f12a1b92f 100644 --- 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 index 06baa60a7b8..cc9d835877a 100644 --- 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 Rsa2kPkcs for ClientImplementation {} pub trait Rsa2kPkcs: CryptoClient { diff --git a/src/config.rs b/src/config.rs index a2bace35121..e26bc5b79f4 100644 --- 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 index 2461811d928..9f38d7352a1 100644 --- 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/rsa2k.rs similarity index 84% rename from src/mechanisms/rsa2kpkcs.rs rename to src/mechanisms/rsa2k.rs index 9c4ab6b6e47..6f3e8838f87 100644 --- a/src/mechanisms/rsa2kpkcs.rs +++ b/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 {} diff --git a/src/store/keystore.rs b/src/store/keystore.rs index 020baa6465e..f82113a4546 100644 --- a/src/store/keystore.rs +++ b/src/store/keystore.rs @@ -147,11 +147,9 @@ impl Keystore for ClientKeystore

{ 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 = store::read(self.store, location, &path)?; let key = key::Key::try_deserialize(&bytes)?; diff --git a/src/types.rs b/src/types.rs index 4d28e71821b..ace1d31d354 100644 --- 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; diff --git a/tests/rsa2kpkcs.rs b/tests/rsa2kpkcs.rs index e7b96fc7d21..f4a621d556c 100644 --- a/tests/rsa2kpkcs.rs +++ b/tests/rsa2kpkcs.rs @@ -9,9 +9,7 @@ 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() { @@ -19,7 +17,6 @@ fn rsa2kpkcs_generate_key() { 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)); }) }