Added macro calls for sign output size and export key buffer size #31

sbailey-arm · 2020-06-22T10:19:35Z

Signed-off-by: Samuel Bailey [email protected]

hug-dev

Thanks for the PR! It will be extremely helpful. A few comments to make it look nicer ✨

As a test, could you modify the examples of the sign_hash and export_public_key operations to use the macro you just added?

hug-dev · 2020-06-22T10:40:46Z

psa-crypto-sys/src/shim_methods.rs

+    unsafe { psa_crypto_binding::shim_PSA_SIGN_OUTPUT_SIZE(key_type, key_bits, alg) }
+}
+
+pub fn PSA_EXPORT_KEY_OUTPUT_SIZE(key_type: psa_key_type_t, key_bits: usize) -> usize {


Shoudn't it be PSA_EXPORT_PUBLIC_KEY_OUTPUT_SIZE that the macro PSA_KEY_EXPORT_MAX_SIZE is mapping to in the 1.0 version? It seems to be what the doc indicates.

As in, we would call this function to know a sufficient large buffer for psa_export_public_key as opposed to psa_export_key.

I've gone off the API changelog (scroll down to the highlighted bit) which states PSA_KEY_EXPORT_MAX_SIZE() → PSA_EXPORT_KEY_OUTPUT_SIZE(). Though the old macro in Parsec did specifically state public. Not sure why there is an inconsistency?

Ah you are right, my bad! Seems like PSA_EXPORT_KEY_OUTPUT_SIZE works for both public keys and key pairs. I think in Parsec we only support psa_export_public_key in which case we will need a way to get the output size of the public part of a key pair (to have the size only of the public part and not the whole key).

Currently I don't think mbedtls supports PSA_EXPORT_PUBLIC_KEY_OUTPUT_SIZE (at least I can't find it using Githubs search feature). So does that mean we should add psa_export_public_key_output_size to psa-crypto and do some jiggery pokery with PSA_EXPORT_KEY_OUTPUT_SIZE? That way we implement the spec in psa-crypto and Parsec can use it. Or do we have to ensure that Parsec only supplies the public key to export_key_output_size?

Could we do something like (as written in the spec): PSA_EXPORT_KEY_OUTPUT_SIZE(PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(key_type), key_bits)? If PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR is available?

hug-dev · 2020-06-22T11:07:03Z

psa-crypto/src/types/key.rs

+
+    /// Sufficient buffer size for a signature using the given key, if the key is supported
+    #[cfg(feature = "with-mbed-crypto")]
+    pub fn sign_output_size(self) -> Result<usize> {


I think the alg parameter of PSA_SIGN_OUTPUT_SIZE macro should not be the same as the permitted_algorithms field of the attributes but should come from as another alg parameter of this function.
My understanding is that the permitted_algorithms field only specify the authorised algorithms (there can be multiple) that these attributes allow as opposed as the specific algorithm to use for a particular operation.
Even if the results will be the same, I think it would be better this way, to add an alg parameter (of type AsymmetricSignature) to this function!

I was wondering about that, there seemed to be no specific algorithm for a given Attributes struct. Looking at the way it used to work, it did use permitted_algorithm, just like I've done (it converted Attributes into psa_key_attributes_t which does attributes.policy.permitted_algorithms.try_into()? to get the algorithm).

Can we not fish out the AsymmetricSignature from self and if it doesn't have one, return the error as it does currently?
Something like:

if let Algorithm::AsymmetricSignature(sign_alg) = self.policy.permitted_algorithms { Ok(psa_crypto_sys::PSA_SIGN_OUTPUT_SIZE( self.key_type.try_into()?, self.bits, sign_alg.into(), )) } else { Err(Error::InvalidArgument) }

I think that before we were doing things wrong, mixing and using the policy algorithm as the algorithm used for any crypto operation. We changed that and added an alg field to specifically split those two semantic units which are the policy algorithms (the algorithms permitted to be used with a key, they can be multiple because you can have SignHash::Any as an hash algorithm) and the specific algorithm used for a crypto operation. This last one, can not be a policy algorithm.

Following the same logic, I think it would be better splitting things here as well, as in not using the policy algorithm as the one to use to check the output size.

Ah ok. So where should the algorithm come from? Should there be another Algorithm attribute in the struct or is it held separately?

I think it should be part of the method's signature!

pub fn sign_output_size(self, alg: AsymmetricSignature) -> Result<usize>

Yeah, I meant where does the caller get the value from 😆. I've got it though, op.alg.

hug-dev · 2020-06-22T11:13:47Z

psa-crypto-sys/src/shim_methods.rs

@@ -171,3 +171,15 @@ pub fn PSA_KEY_TYPE_DH_KEY_PAIR(group: psa_dh_group_t) -> psa_key_type_t {
 pub fn PSA_KEY_TYPE_DH_PUBLIC_KEY(group: psa_dh_group_t) -> psa_key_type_t {
    unsafe { psa_crypto_binding::shim_PSA_KEY_TYPE_DH_PUBLIC_KEY(group) }
 }
+
+pub fn PSA_SIGN_OUTPUT_SIZE(


I am thinking if those macros should be safe or not... The specification says:

If the parameters are not valid, the return value is unspecified.

and I am wondering if unspecified is also unsafe 😋 Maybe it is safer to add unsafe here, ironically

Can't we do enough checks to make it safe? As in, if we have the checks in place to ensure we only ever pass the macros valid arguments, it shouldn't ever return an unspecified value.

Can't we do enough checks to make it safe?

I definitely agree and that is what we are trying to do in the psa-crypto crate. The psa-crypto-sys should only be the thin Rust wrapper to allow calling the C API and can be unsafe.

Ah ok, I see what you mean now. Yes I think they should be marked as unsafe as you suggested in psa-crypto-sys.

hug-dev · 2020-06-22T11:15:02Z

psa-crypto/src/types/key.rs

+    /// Sufficient buffer size for a signature using the given key, if the key is supported
+    #[cfg(feature = "with-mbed-crypto")]
+    pub fn sign_output_size(self) -> Result<usize> {
+        match self.key_type {


I think here you can re-use the compatible_with_alg method of Attributes which implements similar logic and do something like:

self.compatible_with_alg(alg)?; Ok(PSA_SIGN_OUTPUT_SIZE(..))

hug-dev · 2020-06-22T11:23:46Z

psa-crypto/src/types/key.rs

+                self.key_type.try_into()?,
+                self.bits,
+            )),
+            _ => Err(Error::NotSupported),


Maybe Error:InvalidArgument is better here to follow the specs.

hug-dev · 2020-06-22T11:25:07Z

psa-crypto/src/types/key.rs

+    /// Sufficient size for a buffer to export the key, if supported
+    #[cfg(feature = "with-mbed-crypto")]
+    pub fn export_key_output_size(self) -> Result<usize> {
+        match self.key_type {


The specification says that this macro allows for key_type:

key_type A public key or key pair key type.

So I think you can add DH both for key pair and public key as a valid type as well.

Is DH the same as DSA? I've been looking at this to try and make sure its not possible to pass it something invalid.

Is DH the same as DSA?

No, Diffie-Hellman is a type of key exchange, Digital Signature Algorithm is (as its name says) an asymmetric signature algorithm.

For DH public keys, the spec dictates:

For Diffie-Hellman key exchange public keys, with key types for which PSA_KEY_TYPE_IS_DH_PUBLIC_KEY is true, the format is the representation of the public key y = g^x mod p as a big-endian byte string. The length of the byte string is the length of the base prime p in bytes.

Which brings me to another realisation - that method there, export_key_output_size is improperly named, as it refers only to public keys, not to keys in general.

Ah, I see, you're using the spec-provided macros, I think that's alright (although, will those disappear in the future?). But we do have to make sure we document whether this method refers to the full key or just the public part

ionut-arm · 2020-06-22T15:10:57Z

psa-crypto/src/types/key.rs

+            Type::RsaPublicKey
+            | Type::RsaKeyPair
+            | Type::EccPublicKey { .. }
+            | Type::EccKeyPair { .. } => Ok(psa_crypto_sys::PSA_SIGN_OUTPUT_SIZE(


If the parameters are a valid combination that is not supported by the implementation, this macro must return either a sensible size or 0.

Do we want to return that 0 or do we want to convert that case into an error? Surely a size of 0 is not helpful to anyone

You mean checking the return value and return like NotSupported if 0?

Yeah, something like that - but I don't know if that contradicts the spec. Tbh we're returning a Result anyway so it's not a big deviation

hug-dev

Looks perfect to me!

hug-dev · 2020-06-23T10:46:58Z

psa-crypto/src/operations/asym_signature.rs

 ///                          hash_alg: Hash::Sha256.into(),
-///                      },
+///                      };
+/// let buffer_size = attributes.sign_output_size(alg).unwrap();


Thanks for the update of the example 💯

hug-dev · 2020-06-23T10:58:29Z

psa-crypto/src/types/key.rs

+                let pub_type = self.key_type.key_type_public_key_of_key_pair()?;
+                Attributes::export_key_output_size_base(pub_type, self.bits)


That's clean!

hug-dev · 2020-06-23T11:01:45Z

psa-crypto/src/types/key.rs

+    /// Sufficient buffer size for a signature using the given key, if the key is supported
+    #[cfg(feature = "with-mbed-crypto")]
+    pub fn sign_output_size(self, alg: AsymmetricSignature) -> Result<usize> {
+        self.compatible_with_alg(Algorithm::AsymmetricSignature(alg))?;


This is perfectly fine like this (no need to change), just showing for reference that you can also do:

self.compatible_with_alg(alg.into())?;

ionut-arm · 2020-06-23T11:06:59Z

psa-crypto/src/types/key.rs

+        let size =
+            unsafe { psa_crypto_sys::PSA_EXPORT_KEY_OUTPUT_SIZE(key_type.try_into()?, bits) };
+        if size > 0 {
+            Ok(size)
+        } else {
+            Err(Error::NotSupported)
+        }


Same as what Hugues said, this works fine, no need to change, just for reference:

match unsafe { psa_crypto_sys::PSA_EXPORT_KEY_OUTPUT_SIZE(key_type.try_into()?, bits) } { 0 => Err(Error::NotSupported), val => Ok(val), }

Ah yes, I was sure there would be a more succinct way of writing that bit, didn't think of doing it like your example!

hug-dev · 2020-06-23T11:34:46Z

thread 'generate_integration_test' panicked at 'called `Result::unwrap()` on an `Err` value: InvalidHandle', psa-crypto/tests/mod.rs:97:23

I am pretty sure Travis fails because of a data race in psa_close_key that is called simultaneously in our test. I propose to fix the problem in a later PR and force the tests to be single threaded for now.

You can apply the following change to ci.sh:

-RUST_BACKTRACE=1 cargo test
+RUST_BACKTRACE=1 cargo test -- --test-threads=1

Updated ci.sh - tests run in single thread, deletes mbedtls dir if exists Signed-off-by: Samuel Bailey <[email protected]>

sbailey-arm force-pushed the move-output-size-macros branch from 0b4e5f1 to ee7c5bb Compare June 22, 2020 10:24

sbailey-arm marked this pull request as ready for review June 22, 2020 10:32

hug-dev reviewed Jun 22, 2020

View reviewed changes

ionut-arm reviewed Jun 22, 2020

View reviewed changes

sbailey-arm force-pushed the move-output-size-macros branch from ee7c5bb to e8f1258 Compare June 23, 2020 10:01

hug-dev approved these changes Jun 23, 2020

View reviewed changes

ionut-arm approved these changes Jun 23, 2020

View reviewed changes

Added macro calls for sign output size and export key buffer size

1e39ab4

Updated ci.sh - tests run in single thread, deletes mbedtls dir if exists Signed-off-by: Samuel Bailey <[email protected]>

sbailey-arm force-pushed the move-output-size-macros branch from e8f1258 to 1e39ab4 Compare June 23, 2020 11:51

hug-dev approved these changes Jun 23, 2020

View reviewed changes

hug-dev merged commit b9b9881 into parallaxsecond:master Jun 23, 2020

hug-dev mentioned this pull request Jun 23, 2020

Implement macros to have size of output buffers #30

Closed

sbailey-arm mentioned this pull request Jun 24, 2020

Further simplification of the Mbed Crypto provider parallaxsecond/parsec#187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added macro calls for sign output size and export key buffer size #31

Added macro calls for sign output size and export key buffer size #31

sbailey-arm commented Jun 22, 2020

hug-dev left a comment

hug-dev Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020 •

edited

Loading

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020 •

edited

Loading

hug-dev Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

hug-dev Jun 22, 2020

hug-dev Jun 22, 2020

hug-dev Jun 22, 2020

sbailey-arm Jun 22, 2020

ionut-arm Jun 22, 2020

ionut-arm Jun 22, 2020

ionut-arm Jun 22, 2020

hug-dev Jun 22, 2020

ionut-arm Jun 22, 2020

hug-dev left a comment

hug-dev Jun 23, 2020

hug-dev Jun 23, 2020

hug-dev Jun 23, 2020

ionut-arm Jun 23, 2020

sbailey-arm Jun 23, 2020

hug-dev commented Jun 23, 2020 •

edited

Loading

		let pub_type = self.key_type.key_type_public_key_of_key_pair()?;
		Attributes::export_key_output_size_base(pub_type, self.bits)

Added macro calls for sign output size and export key buffer size #31

Added macro calls for sign output size and export key buffer size #31

Conversation

sbailey-arm commented Jun 22, 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

sbailey-arm Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbailey-arm Jun 22, 2020 • edited Loading

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

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

hug-dev commented Jun 23, 2020 • edited Loading

sbailey-arm Jun 22, 2020 •

edited

Loading

sbailey-arm Jun 22, 2020 •

edited

Loading

hug-dev commented Jun 23, 2020 •

edited

Loading