Part of moving Parsec to use psa-crypto #28

sbailey-arm · 2020-06-15T10:14:01Z

Part of Parsec Issue #177
Includes #26

Contains changes required to allow Prasec to use psa-crypto instead of binding directly to mbedtls.

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

…n. Moved some error consts over from bnding. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

…se key handle closing error. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

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

hug-dev

Thanks for this PR! This is very good overall, thanks for taking the time to fix the issue, add examples and tests.
I made a few comments we can always discuss!

hug-dev · 2020-06-15T13:10:51Z

psa-crypto-sys/src/constants.rs

+pub const PSA_KEY_SLOT_COUNT: isize = 32;
+pub const PSA_MAX_PERSISTENT_KEY_IDENTIFIER: psa_key_id_t = 0x3fff_ffff;


These values seem to be Mbed Crypto specific. Ideally this crate is common to all PSA Crypto implementation so it would be better, I think, to declare those values in the code that needs them directly.

Yes, they are Mbed Crypto specific. Should they be moved to parsec?

Yes I think it will be better.

Presumably PSA_MAX_PERSISTENT_KEY_IDENTIFIER will be replaced with something else in PSA Crypto 1.0?

Presumably PSA_MAX_PERSISTENT_KEY_IDENTIFIER will be replaced with something else in PSA Crypto 1.0?

That's a good point, on section 9.1.1, for psa_key_id_t, it defines ranges of key identifiers. For example for application key identifiers, it can range from PSA_KEY_ID_USER_MIN (0x00000001) to PSA_KEY_ID_USER_MAX (0x3fffffff). Which is the same value as the one above 😄! So I guess as PSA_KEY_SLOT_COUNT should be removed, PSA_MAX_PERSISTENT_KEY_IDENTIFIER can be renamed to PSA_KEY_ID_USER_MAX

hug-dev · 2020-06-15T13:13:19Z

psa-crypto-sys/src/lib.rs

-    psa_close_key, psa_crypto_init, psa_destroy_key, psa_export_public_key, psa_generate_key,
-    psa_import_key, psa_key_attributes_t, psa_open_key, psa_reset_key_attributes, psa_sign_hash,
-    psa_verify_hash,
+    mbedtls_psa_crypto_free, psa_close_key, psa_crypto_init, psa_destroy_key,


Same for mbedtls_psa_crypto_free, I don't think we should declare it here.

I'm not sure how we should deal with this. For psa-crypto to be portable to any PSA Crypto implementation, we can't have this mbedtls specific method declared here, but I'm not sure how we remove it from psa-crypto and allow Parsec to call it without binding straight through to mbedtls.

I think this is is used inside Mbed Crypto for testing but should not be required in this crate. If it is, and creating leaks in our applications, we should probably discuss this at a PSA Crypto level.

hug-dev · 2020-06-15T13:34:01Z

psa-crypto/src/operations/key_management.rs

+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {
+    initialized()?;
+    let mut key_attributes = unsafe { psa_crypto_sys::psa_key_attributes_init() };


I think if we follow the specs, the attributes created here might allocate some extra space that needs to be freed with a call to psa_reset_key_attributes (Attributes::reset). I think that might be needed on every path leaving the function once psa_key_attributes_init has been called.

Hence my pet peeve with these cleanup methods.
What happens if both close_handle and psa_reset_key_attributes fail, which value do we return? Maybe we should really return a generic error when some of these "internal" methods fail, that the user should not really know about.

An alternative would be to make a "layered" function, where for each variable that needs cleaning up we wrap everything after it in a function that returns a Result. If the result is Ok, attempt to clean up and if that fails return an error, otherwise return the Ok you got. If what you got was Err, then do the clean up anyway and return the Err you got from the next layer function. If you have two things that you need to clean, unfortunately you'd have 2 "layer" functions. But the benefit is that you're sure that you clean up everything and you return the Err that's most appropriate.

I think psa_reset_key_attributes can't fail, though that doesn't invalidate the general point, just that particular example!

(Also, I think you don't need to call psa_reset_key_attributes if calling psa_key_attributes_init is all you've done with the psa_key_attributes_t, and according to the 1.0 spec there seem to be certain psa_set_key_ functions that you can call without needing to do the reset.)

hug-dev · 2020-06-15T13:40:06Z

psa-crypto/src/operations/key_management.rs

+    if key_lifetime != Lifetime::Volatile {
+        Status::from(unsafe { psa_crypto_sys::psa_close_key(handle) }).to_result()?;
+    }
+    operation_result?;


Do you think that if psa_generate_key or psa_import_key failed you still need to close the key? I would have thought that if those operations fail, they do not open the key.
The documentation of psa_generate_key says:

* \param[out] handle On success, a handle to the newly created key. * \c 0 on failure.

So I think this is fine not passing operation_result to this function (checking in the caller if it failed or not).

I have checked psa_generate_key and psa_import_key - you are correct, a handle should not be created unless the operation succeeds.

hug-dev · 2020-06-15T13:41:55Z

psa-crypto/src/types/key.rs

@@ -12,6 +12,9 @@ use crate::types::status::{Error, Result};
 #[cfg(feature = "with-mbed-crypto")]
 use core::convert::{TryFrom, TryInto};
 use log::error;
+pub use psa_crypto_sys::{
+    psa_key_id_t as key_id_type, PSA_KEY_SLOT_COUNT, PSA_MAX_PERSISTENT_KEY_IDENTIFIER,


Is the renaming of psa_key_id_t required? I think it is easier to understand when you have the original type name but that might be a personal opinion 😋

No, not required. I think I just did it so it was clear in my head when I was making the change, so I didn't trip up with the other reference to the same type from the binding, when I was swapping them over. I agree, it'll be clearer if the original name is kept 😃

hug-dev · 2020-06-15T13:42:41Z

psa-crypto/src/types/status.rs

+            // Commented out as all errors are currently matched against and this causes compilation error
+            /*e => {
                error!("No equivalent of {:?} to a psa_status_t.", e);
                psa_crypto_sys::PSA_ERROR_GENERIC_ERROR
-            }
+            }*/


That's great, thanks for completing this conversion. Feel free to delete that block altogether 👍

Will remove

hug-dev · 2020-06-15T13:43:05Z

psa-crypto/tests/mod.rs

+use psa_crypto::types::key::{Attributes, Lifetime, Policy, Type, UsageFlags};
+
+#[test]
+fn generate_integration_test() {


Nice, thanks for the new integration tests!
Nice to see that the CI passed them as well!

hug-dev · 2020-06-15T13:46:41Z

psa-crypto/src/operations/asym_signature.rs

    key.close_handle(handle)?;
-
+    sign_res?;


That is actually quite clever 😄 Never thought of doing it like that before.

hug-dev · 2020-06-15T13:55:15Z

psa-crypto/src/operations/key_management.rs

+/// let key_attributes = key_management::get_key_attributes(my_key);
+/// ```
+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {


In a way, this function is a contructor of Attributes so I think it could be placed inside the impl Attributes block. In that case, we could rename it, using namespacing. What about something like from_key? It would then be Attributes::from_key(...) to invoke it.
Agree that we would deviate from the PSA Crypto naming but I think it is fine if we make it more Rust-like!

from_key_id - Id is not a key per se and we keep naming things as if it is.

ionut-arm

Thank you for the changes! 👍

ionut-arm · 2020-06-15T14:00:28Z

psa-crypto/src/operations/asym_signature.rs

-
+    .to_result();
    key.close_handle(handle)?;
-
+    sign_res?;


For these cases where you need to do a cleanup operation that may fail (after some other core operation might've failed in the first place) we have to decide: if both fail, which error do we want to be returned? I'd say the error on the "core" operation should take precedence, in which case the two branches (as in, if the core operation failed or not) will be different

Also, I wish we had a better approach to this kind of problem where you have some cleanup to do whenever you exit the function. In this case it's fine since there's not much branching, but in some cases it might end up being a pain having to call that close_handle for each possible path before doing ?.

I originally had it returning the first operation error, then the close handle error but after a brief discussion with @hug-dev, it was changed to how it is now. I think the reason was the PSA API states keys aren't used, IDs are, so it should be getting removed at some point (soon? 😄) anyway.

Yes, I think that makes sense for this case, my comment was more general - we should have a statement somewhere as to how to prioritize these errors throughout the library. I'd even say that this applies to Parsec too. The last error to occur is not necessarily the one we should return.

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

psa-crypto/src/operations/key_management.rs

+/// let key_attributes = key_management::get_key_attributes(my_key);
+/// ```
+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {


from_key_id - Id is not a key per se and we keep naming things as if it is.

ionut-arm · 2020-06-15T14:23:02Z

psa-crypto/src/operations/key_management.rs

+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {
+    initialized()?;
+    let mut key_attributes = unsafe { psa_crypto_sys::psa_key_attributes_init() };


Hence my pet peeve with these cleanup methods.
What happens if both close_handle and psa_reset_key_attributes fail, which value do we return? Maybe we should really return a generic error when some of these "internal" methods fail, that the user should not really know about.

An alternative would be to make a "layered" function, where for each variable that needs cleaning up we wrap everything after it in a function that returns a Result. If the result is Ok, attempt to clean up and if that fails return an error, otherwise return the Ok you got. If what you got was Err, then do the clean up anyway and return the Err you got from the next layer function. If you have two things that you need to clean, unfortunately you'd have 2 "layer" functions. But the benefit is that you're sure that you clean up everything and you return the Err that's most appropriate.

ionut-arm · 2020-06-15T14:24:31Z

Thank you for the changes! 👍

Disregard the approval for now, my mistake 😅

egrimley-arm · 2020-06-15T16:05:08Z

psa-crypto-sys/src/c/shim.h

@@ -83,6 +83,7 @@ psa_algorithm_t shim_get_key_algorithm(const psa_key_attributes_t *attributes);
 psa_key_usage_t
 shim_get_key_usage_flags(const psa_key_attributes_t *attributes);
 psa_key_attributes_t shim_key_attributes_init(void);
+psa_status_t shim_get_key_attributes(psa_key_handle_t key_handle, psa_key_attributes_t *attributes);


It would be nice to tidy this code up a bit while you're at it. How about having the return type consistently on the same line as the function name and putting the functions in alphabetical order as they already mostly are?

Is it possible to add CI linting (potentially clang-format)? Wouldn't take care of the alphabetical order, but it would keep other things tidy. Might be a bit overkill, tho

egrimley-arm · 2020-06-15T16:07:25Z

psa-crypto-sys/src/constants.rs

@@ -25,6 +25,9 @@ pub const PSA_ERROR_HARDWARE_FAILURE: psa_status_t = -147;
 pub const PSA_ERROR_INSUFFICIENT_ENTROPY: psa_status_t = -148;
 pub const PSA_ERROR_INVALID_SIGNATURE: psa_status_t = -149;
 pub const PSA_ERROR_INVALID_PADDING: psa_status_t = -150;
+pub const PSA_ERROR_CORRUPTION_DETECTED: psa_status_t = -151;
+pub const PSA_ERROR_DATA_CORRUPT: psa_status_t = -152;
+pub const PSA_ERROR_DATA_INVALID: psa_status_t = -153;
 pub const PSA_ERROR_INSUFFICIENT_DATA: psa_status_t = -143;


The last two (-143 and -136) seem to be in the wrong place but that's not part of this patch so I shouldn't really complain.

Remind me why these values can't come from the C library somehow? In a previous patch we had something like pub const PSA_SOMETHING: psa_something_t = shim_PSA_SOMETHING; in the Rust code and const psa_something_t shim_PSA_SOMETHING = PSA_SOMETHING; in shim.h. It would be nice to explain in a comment at the top of constants.rs why these values are being repeated like this.

We removed from the shim all the values that are defined by the specification. That way, it allows you to use those value, without having to link with a PSA Crypto implementation (that is the implementation-defined feature). That is useful for Parsec interface where we re-use some of these values but do not want to link to a PSA Crypto implementation.

At least I hope I removed all the duplication, as I did everything manually

I hadn't realised some values are defined by the specification. Should that be mentioned in a comment in constants.rs?

I think that makes sense - we do have a module comment in this file, it might be worth expanding that to explain the logic of what's hidden by implementation-defined - there's a brief explanation here but it should really be in docs.rs too.

egrimley-arm · 2020-06-15T16:24:24Z

psa-crypto-sys/src/constants.rs

+pub const PSA_KEY_SLOT_COUNT: isize = 32;
+pub const PSA_MAX_PERSISTENT_KEY_IDENTIFIER: psa_key_id_t = 0x3fff_ffff;


Presumably PSA_MAX_PERSISTENT_KEY_IDENTIFIER will be replaced with something else in PSA Crypto 1.0?

egrimley-arm · 2020-06-15T16:33:28Z

psa-crypto/src/operations/key_management.rs

+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {
+    initialized()?;
+    let mut key_attributes = unsafe { psa_crypto_sys::psa_key_attributes_init() };


I think psa_reset_key_attributes can't fail, though that doesn't invalidate the general point, just that particular example!

egrimley-arm · 2020-06-15T16:37:30Z

psa-crypto/src/operations/key_management.rs

+
+pub fn get_key_attributes(key: Id) -> Result<Attributes> {
+    initialized()?;
+    let mut key_attributes = unsafe { psa_crypto_sys::psa_key_attributes_init() };


(Also, I think you don't need to call psa_reset_key_attributes if calling psa_key_attributes_init is all you've done with the psa_key_attributes_t, and according to the 1.0 spec there seem to be certain psa_set_key_ functions that you can call without needing to do the reset.)

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

hug-dev

One small last comment but otherwise it looks good for me!

hug-dev · 2020-06-15T18:07:08Z

psa-crypto/src/lib.rs

@@ -43,6 +43,7 @@

 #[cfg(feature = "with-mbed-crypto")]
 pub mod operations;
+#[cfg(feature = "with-mbed-crypto")]


I don't think this cfg gate is needed for that module!

For types? Now get_key_attributes is in Attributes, it needs access to some mbed-crypto specific code (as in, other code behind the cfg gate, like initialized, psa_key_attributes_ini(), etc). Without that, ci.sh gives a load of not found in errors.

Actually, I could make it more local. Adding a gate for use crate::initialized and one for the method.

ionut-arm

I'm ok with the changes as they are! Leaving it to @hug-dev if he thinks any more changes are needed

ionut-arm · 2020-06-15T18:19:34Z

psa-crypto/src/types/key.rs

+    /// //...
+    /// let key_attributes = Attributes::from_key_id(my_key_id);
+    /// ```
+    pub fn from_key_id(key_id: Id) -> Result<Self> {


Would it be worth having a test for this?

As there is an example, let's say it is fine for now but think about adding one in the future 🤡

Make feature gate for Attributes::from_key_id more local Tidy up and organise function and constant declarations Re-add ID min and max range values Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

hug-dev

Looks good to me! Thanks for all the changes

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

hug-dev requested review from hug-dev, ionut-arm and egrimley-arm June 15, 2020 10:15

hug-dev assigned sbailey-arm Jun 15, 2020

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

sbailey-arm marked this pull request as draft June 15, 2020 11:08

sbailey-arm added 5 commits June 15, 2020 12:17

Added get_key_attributes for export.

19b70b0

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

Generate, import and get_key_attributes close key handle on completio…

3d1580e

…n. Moved some error consts over from bnding. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Changed error handling when psa-crypto-sys operation fails - prioriti…

d361726

…se key handle closing error. Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

Added drop method to clean up mbedtls resources.

6fd4469

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

Removed unecessary shim binding for get_key_attributes

fd73ed1

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

sbailey-arm force-pushed the development branch from c6fc46c to fd73ed1 Compare June 15, 2020 11:22

Fixes for CI

d539aed

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

sbailey-arm force-pushed the development branch from 608d9e8 to d539aed Compare June 15, 2020 12:42

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

hug-dev reviewed Jun 15, 2020

View reviewed changes

ionut-arm approved these changes Jun 15, 2020

View reviewed changes

egrimley-arm approved these changes Jun 15, 2020

View reviewed changes

Addressed comments received.

6f34968

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

hug-dev reviewed Jun 15, 2020

View reviewed changes

ionut-arm approved these changes Jun 15, 2020

View reviewed changes

Addresses further comments:

2d7b6ae

Make feature gate for Attributes::from_key_id more local Tidy up and organise function and constant declarations Re-add ID min and max range values Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>

hug-dev approved these changes Jun 16, 2020

View reviewed changes

Fix conflict between generate and import test ranges when running sum…

999187c

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

hug-dev merged commit fdc9cb4 into parallaxsecond:master Jun 16, 2020

sbailey-arm mentioned this pull request Jun 23, 2020

Handling key handle closing is noisy and error prone #34

Closed

		pub const PSA_KEY_SLOT_COUNT: isize = 32;
		pub const PSA_MAX_PERSISTENT_KEY_IDENTIFIER: psa_key_id_t = 0x3fff_ffff;

Part of moving Parsec to use psa-crypto #28

Part of moving Parsec to use psa-crypto #28

Conversation

sbailey-arm commented Jun 15, 2020

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

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

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

ionut-arm commented Jun 15, 2020

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

sbailey-arm Jun 16, 2020 • edited Loading

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

hug-dev left a comment

Choose a reason for hiding this comment

sbailey-arm Jun 16, 2020 •

edited

Loading