Move Parsec over to psa-crypto #186

sbailey-arm · 2020-06-17T12:35:37Z

Note: It currently won't build due to local dependencies. Once psa-crypto and parsec-interface are updated on crates.io, we can update the dependencies in this PR to make it build. Currently this PR is for everyone to see and discuss the changes.

sbailey-arm · 2020-06-17T12:39:06Z

src/providers/mbed_provider/key_management.rs

@@ -13,14 +11,16 @@ use parsec_interface::operations::{
    psa_destroy_key, psa_export_public_key, psa_generate_key, psa_import_key,
 };
 use parsec_interface::requests::{ProviderID, ResponseStatus, Result};
+use psa_crypto::operations::key_management as new_key_management;


I wasn't sure what to do about this rename. I thought leaving it as use psa_crypto::operations::key_management; might get confusing as it is already in a file called key_management.rs. Thoughts? Maybe psa_crypto_key_management?

psa_crypto_key_management

That seems good to me!

ionut-arm

Looks promising! Lots of gone code 🧹

ionut-arm · 2020-06-17T12:37:21Z

build.rs

-
-    Ok(())
-}
+fn main() {}


If this file is empty we might as well delete it!

I think the reason we still have that file is that the OUT_DIR environment variable will only exist if that file exist.

Yeah, I did remove it at first but ran into compilation errors. @hug-dev found this.

ionut-arm · 2020-06-17T12:41:30Z

src/providers/mbed_provider/constants.rs


-use super::psa_crypto_binding::*;
+pub use super::key::{psa_key_id_t, PSA_KEY_ID_USER_MAX, PSA_KEY_ID_USER_MIN};


I think this file can go away too!

Let's 🧹 it!

ionut-arm · 2020-06-17T12:55:48Z

src/providers/mbed_provider/key_management.rs

    {
-        key_id = rand::random::<psa_key_id_t>();
+        key_id = rand::random::<key::psa_key_id_t>();


Looking at the documentation of SmallRng and Rng I think we can replace the comparisons with PSA_KEY_ID_USER_MIN and PSA_KEY_ID_USER_MAX with

let mut rng = rand::rngs::SmallRng::from_entropy(); rng.gen_range(PSA_KEY_ID_USER_MIN, PSA_KEY_ID_USER_MAX); let key_id = rng.gen(); ...

We could also make use of Mbed Crypto and psa_generate_random 🤡 But I would ay it is better to use RNG as the entropy source should be better I believe?

I'm not sure if it matters, the handle value on its own doesn't really represent a crypto liability, we could even use a counter

ionut-arm · 2020-06-17T12:57:40Z

src/providers/mbed_provider/key_management.rs

+                    &mut local_ids_handle,
+                )?;
+                let error = ResponseStatus::from(error);
+                error!("Generate key status: {}", error);


You can/should probably use the new macro we have, format_error! when attempting to log errors from dependency crates - this way the admin can control whether potentially sensitive data is logged or not

Ok, thanks for the heads up. I stuck with error! because that's what was there before but I'll switch it to format_error!.

ionut-arm · 2020-06-17T14:17:09Z

src/providers/mbed_provider/mod.rs

+                    if let Err(error) = psa_crypto::init() {
+                        error!("Error {} when initialising Mbed Crypto library.", error);
+                        return None;
+                    }


What's the purpose of this init if the one on line 77 happens?

I'm not sure why or how that got there! It definitely shouldn't be.

hug-dev

Thanks a lot for the changes!! It looks really nice and clean using the psa-crypto provider!!

I am thinking of a few things as next steps to simplify the provider further:

I think it would be possible to remove the local_ids field and replace it with a AtomicU32 counter?
The key_handle_mutex might not be needed anymore if updating Mbed Crypto solved the thread-safety issues. If it is still needed (Parsec stress tests fail without it), it might be better suited in the psa-crypto crate.
I am not sure if the key_slot_semaphore is needed at all. It limits the number of open key slots at a time but if that is a constraint of Mbed Crypto we might just return the error

hug-dev · 2020-06-17T14:16:18Z

Cargo.toml

@@ -40,6 +40,7 @@ derivative = "2.1.1"
 version = "3.0.0"
 hex = "0.4.2"
 picky = "5.0.0"
+psa-crypto = { path = "../rust-psa-crypto/psa-crypto", default-features = false, features = ["with-mbed-crypto"] }


I think this should be an optional feature (like tss-esapi for example), that is only activated if the mbed-crypto-provider feature is selected.

hug-dev · 2020-06-17T14:22:51Z

build.rs

-
-    Ok(())
-}
+fn main() {}


I think the reason we still have that file is that the OUT_DIR environment variable will only exist if that file exist.

hug-dev · 2020-06-17T14:24:10Z

src/providers/mbed_provider/mod.rs

@@ -28,17 +26,13 @@ use uuid::Uuid;
    trivial_casts
 )]
 #[allow(clippy::all)]
-mod psa_crypto_binding {


I think you can also remove all the allow above that were targeting this psa_crypto_binding mod specifically.

And even the one above, at line 21 😋 If we keep it, I am afraid it will apply on to the next mod, which might hides somes lints for that module!

hug-dev · 2020-06-17T14:26:10Z

src/providers/mbed_provider/mod.rs

@@ -120,13 +119,12 @@ impl MbedProvider {
                        // Safety: safe because:
                        // * the Mbed Crypto library has been initialized
                        // * this code is executed only by the main thread
-                        match unsafe { KeyHandle::open(key_id) } {
+                        let pc_key_id = key::Id::from_persistent_key_id(key_id);


You can remove the // Safety comments above as the call is now safe 💯

Same everywhere an unsafe block is removed!

hug-dev · 2020-06-17T14:29:50Z

src/providers/mbed_provider/utils.rs

+pub fn psa_asymmetric_sign_output_size(key_attrs: &key::Attributes) -> Result<usize> {
+    match key_attrs.key_type {
+        Type::RsaKeyPair => Ok(bits_to_bytes!(key_attrs.bits)),
+        Type::EccKeyPair { .. } => Ok(bits_to_bytes!(key_attrs.bits) * 2),
        _ => Err(ResponseStatus::PsaErrorNotSupported),
    }
 }

 /// Compute the size of the public key material to be exported, given the attributes of the key.
 /// Implementing `PSA_KEY_EXPORT_MAX_SIZE` for public keys only, as defined in `crypto_sizes.h` (Mbed Crypto).
-pub fn psa_export_public_key_size(key_attrs: &psa_key_attributes_t) -> Result<usize> {
+pub fn psa_export_public_key_size(key_attrs: &key::Attributes) -> Result<usize> {
    macro_rules! export_asn1_int_max_size {
        ($size:expr) => {
            ($size) / 8 + 5
        };
    };

-    match key_attrs.core.type_ {
-        PSA_KEY_TYPE_RSA_PUBLIC_KEY | PSA_KEY_TYPE_RSA_KEYPAIR => Ok(usize::from(
-            export_asn1_int_max_size!(key_attrs.core.bits) + 11,
-        )),
+    match key_attrs.key_type {
+        Type::RsaPublicKey | Type::RsaKeyPair => Ok(export_asn1_int_max_size!(key_attrs.bits) + 11),
        _ => Err(ResponseStatus::PsaErrorNotSupported),
    }
 }


That is fine like this at the moment for this PR but I think it would be really good to implement an abstraction in psa-crypto for the macros: PSA_SIGN_OUTPUT_SIZE and PSA_EXPORT_PUBLIC_KEY_OUTPUT_SIZE and use it instead of redifining them here!

In which case after that, this file can be deleted!

hug-dev · 2020-06-17T14:30:45Z

src/providers/mbed_provider/constants.rs


-use super::psa_crypto_binding::*;
+pub use super::key::{psa_key_id_t, PSA_KEY_ID_USER_MAX, PSA_KEY_ID_USER_MIN};


Let's 🧹 it!

hug-dev · 2020-06-17T14:40:38Z

src/providers/mbed_provider/asym_sign.rs

+                let mut res = psa_sign_hash::Result {
+                    signature: Vec::new(),
+                };
+                res.signature.resize(size, 0);
+                res.signature.copy_from_slice(&signature[0..size]);
+                Ok(res)


I think you can also just move signature into the Result structure:

signature.resize(size, 0); Ok(psa_sign_hash::Result { signature, })

A bit more efficient as well as you don't need the copy!

hug-dev · 2020-06-17T14:43:43Z

src/providers/mbed_provider/key_management.rs

@@ -13,14 +11,16 @@ use parsec_interface::operations::{
    psa_destroy_key, psa_export_public_key, psa_generate_key, psa_import_key,
 };
 use parsec_interface::requests::{ProviderID, ResponseStatus, Result};
+use psa_crypto::operations::key_management as new_key_management;


psa_crypto_key_management

That seems good to me!

hug-dev · 2020-06-17T14:48:02Z

src/providers/mbed_provider/key_management.rs

    {
-        key_id = rand::random::<psa_key_id_t>();
+        key_id = rand::random::<key::psa_key_id_t>();


We could also make use of Mbed Crypto and psa_generate_random 🤡 But I would ay it is better to use RNG as the entropy source should be better I believe?

hug-dev

That looks good to me! A few things I think you forgot (maybe a few Safety as well) but it is all ok. I will publish the 0.2.0 version of psa-crypto so that the PR can pass.

hug-dev · 2020-06-18T11:34:24Z

src/providers/mbed_provider/key_management.rs

-        key_id = rand::random::<psa_key_id_t>();
+) -> Result<key::psa_key_id_t> {
+    let mut rng = SmallRng::from_entropy();
+    let mut key_id = rng.gen_range(key::PSA_KEY_ID_USER_MIN, key::PSA_KEY_ID_USER_MAX);


gen_range might be exclusive of its high parameter whereas (to be confirmed) PSA_KEY_ID_USER_MAX is inclusive. It might then need to be key::PSA_KEY_ID_USER_MAX + 1 for the second parameter.

ah yes, my bad. I'd expect PSA_KEY_ID_USER_MAX to be inclusive.

hug-dev · 2020-06-18T11:36:03Z

src/providers/mbed_provider/mod.rs

@@ -28,17 +26,13 @@ use uuid::Uuid;
    trivial_casts
 )]
 #[allow(clippy::all)]
-mod psa_crypto_binding {


And even the one above, at line 21 😋 If we keep it, I am afraid it will apply on to the next mod, which might hides somes lints for that module!

sbailey-arm · 2020-06-18T12:27:02Z

(maybe a few Safety as well) but it is all ok.

I did find one more Safety comment that shouldn't be there but I've left the destroy one in on purpose as it still uses an unsafe block. Should that comment also be removed?

hug-dev · 2020-06-18T12:33:38Z

(maybe a few Safety as well) but it is all ok.
I did find one more Safety comment that shouldn't be there but I've left the destroy one in on purpose as it still uses an unsafe block. Should that comment also be removed?

Oh, my bad then, if it is only on destroy, that is fine!

hug-dev · 2020-06-18T12:41:21Z

Versions 0.2.0 of psa-crypto and 0.16.0 of parsec-interface have just been published on crates.io so you can now use it in this PR!

sbailey-arm · 2020-06-18T13:05:33Z

I am thinking of a few things as next steps to simplify the provider further:
* I think it would be possible to remove the `local_ids` field and replace it with a `AtomicU32` counter?

We could do. How would we deal with holes when keys are destroyed? E.g. Allocate keys 1 to 100, then destroy 34, 67, 78 and 91.

* The `key_handle_mutex` might not be needed anymore if updating Mbed Crypto solved the thread-safety issues. If it is still needed (Parsec stress tests fail without it), it might be better suited in the `psa-crypto` crate.

This is the issue for that and its still open so I think we'll hit issues. I can try testing it without though, and if it causes a problem, look into moving it to psa-crypto.

* I am not sure if the `key_slot_semaphore` is needed at all. It limits the number of open key slots at a time but if that is a constraint of Mbed Crypto we might just return the error

stress_test and normal_tests pass after removing the semaphore guard request in each operation method so it looks like we can remove it.

hug-dev · 2020-06-18T13:22:44Z

I would prefer to keep the changes of this PR as they are and tackle those different simplifications in future issues/PR.s, if you do not mind. I can open a new issue for those points and we can move the dicsussion there?

sbailey-arm · 2020-06-18T13:31:42Z

I would prefer to keep the changes of this PR as they are and tackle those different simplifications in future issues/PR.s, if you do not mind. I can open a new issue for those points and we can move the dicsussion there?

Sound good!

…test Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Parsec no longer calls mbedtls directly, it goes through the psa-crypto Rust interface to the library Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

…o psa_crypto_binding Now Parsec calls psa-crypto when initialising and dropping an mbedcrypto provider. Also Parsec no longer has references for the bdinging to the mbedtls C lib. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Parsec now uses psa-crypto instead of interfacing directly with the C library. Therefore, the build files are no longer needed in Parsec. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Rename use statement in key_management Removed mbed_crypto constants and relocated contents Modified random number generation for key IDs Moved from error! to format_error! for dependency crate errors Removed redundant init psa-crypto crate optional Removed uneccessary safety comment Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Remove redundant comments Fix index MAX_ID for key ID gen Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

hug-dev

Thanks a lot for the changes!

sbailey-arm commented Jun 17, 2020

View reviewed changes

hug-dev linked an issue Jun 17, 2020 that may be closed by this pull request

Use psa-crypto crate in the Mbed Crypto Provider #177

Closed

hug-dev requested review from ionut-arm, hug-dev and egrimley-arm June 17, 2020 14:14

hug-dev assigned sbailey-arm Jun 17, 2020

hug-dev added the enhancement New feature or request label Jun 17, 2020

hug-dev added this to In progress in Parsec via automation Jun 17, 2020

ionut-arm reviewed Jun 17, 2020

View reviewed changes

hug-dev reviewed Jun 17, 2020

View reviewed changes

hug-dev reviewed Jun 18, 2020

View reviewed changes

hug-dev mentioned this pull request Jun 18, 2020

Further simplification of the Mbed Crypto provider #187

Closed

sbailey-arm added 8 commits June 19, 2020 12:27

Fixed PKCS#11 provieder failing failed_created_key_should_be_removed …

d426bd7

…test Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Move calls from C library to psa-crypto

7b2ca72

Parsec no longer calls mbedtls directly, it goes through the psa-crypto Rust interface to the library Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Removed unecessary build files

683f52d

Parsec now uses psa-crypto instead of interfacing directly with the C library. Therefore, the build files are no longer needed in Parsec. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Tweaks to pass CI tests

e1ab2b7

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Update psa_crypto and parsec-interface dependencies

d5b4be4

Remove redundant comments Fix index MAX_ID for key ID gen Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Added cmake to dockerfile for mbed_crypto and all-providers

186980a

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

sbailey-arm force-pushed the development branch from 92e48b7 to 186980a Compare June 19, 2020 11:52

sbailey-arm marked this pull request as ready for review June 19, 2020 12:12

hug-dev approved these changes Jun 19, 2020

View reviewed changes

egrimley-arm approved these changes Jun 22, 2020

View reviewed changes

hug-dev merged commit fbb239b into parallaxsecond:master Jun 22, 2020

Parsec automation moved this from In progress to Done Jun 22, 2020


		use super::psa_crypto_binding::*;
		pub use super::key::{psa_key_id_t, PSA_KEY_ID_USER_MAX, PSA_KEY_ID_USER_MIN};

Move Parsec over to psa-crypto #186

Move Parsec over to psa-crypto #186

Conversation

sbailey-arm commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm commented Jun 18, 2020 • edited

hug-dev commented Jun 18, 2020

hug-dev commented Jun 18, 2020

sbailey-arm commented Jun 18, 2020

hug-dev commented Jun 18, 2020

sbailey-arm commented Jun 18, 2020

hug-dev left a comment

Choose a reason for hiding this comment

sbailey-arm commented Jun 18, 2020 •

edited