Added the CanDoCrypto operation as well as fixing some of the other test scripts. #522

Kakemone · 2021-09-10T16:03:41Z

Added the internal CanDoCrypto as a new capability_discovery file in the pkcs11 provider and made corresponding changes to the relevent files. Also added the end to end tests for CanDoCrypto and fixed the other tests that were still using the old syntax for the usage flags.

Signed-off-by: Sam Davis sam.davis@arm.com

ionut-arm

The function is quite convoluted - could you break it down into multiple methods/functions, maybe one for each of Use, Generate..., and maybe have a separate function for anything that is common?

ionut-arm · 2021-09-14T13:51:07Z

src/providers/pkcs11/capability_discovery.rs

+ if !(attributes.bits == 192
+ || attributes.bits == 224
+ || attributes.bits == 256
+ || attributes.bits == 384
+ || attributes.bits == 512)


You could probably put these values in an array of constant and perform the check with contains, makes it a bit easier to read

ionut-arm · 2021-09-14T13:52:54Z

src/providers/pkcs11/capability_discovery.rs

+ if attributes.key_type
+ == (Type::EccKeyPair {
+ curve_family: EccFamily::SecpR1,
+ })
+ || attributes.key_type
+ == (Type::EccPublicKey {
+ curve_family: EccFamily::SecpR1,
+ })


This would be more readable with matches

ionut-arm · 2021-09-14T14:27:35Z

src/providers/pkcs11/capability_discovery.rs

+ if std::any::type_name::<Ulong>() == std::any::type_name::<u64>() {
+ if !((attributes.bits as u64) >= (*mechanism_info.min_key_size()).into()
+ && (attributes.bits as u64) <= (*mechanism_info.max_key_size()).into())
+ {
+ return Err(PsaErrorNotSupported);
+ }
+ } else {
+ if !((*mechanism_info.min_key_size() as u64) <= (attributes.bits as u64)
+ && (attributes.bits as u64) <= (*mechanism_info.max_key_size() as u64))
+ {
+ return Err(PsaErrorNotSupported);
+ }
+ }


Just realised that there's a TryFrom<usize> for Ulong implemented that could help here (code here) (?)

I don't think that this will be that much better as <= and >= cannot be applied to Ulongs

There is a From<Ulong> for CK_ULONG implemented, that could help performing the operations on CK_ULONG not caring if that type is u32 or u64?

hug-dev

Thank you for this! It looks as a good start 👌 Added some comments here and there.

hug-dev · 2021-09-20T09:48:04Z

e2e_tests/src/lib.rs

@@ -98,7 +99,7 @@ impl TestClient {
 /// Creates a key with specific attributes.
 pub fn generate_key(&mut self, key_name: String, attributes: Attributes) -> Result<()> {
 self.basic_client
- .psa_generate_key(key_name.clone(), attributes)
+ .psa_generate_key(&key_name.clone(), attributes)


Ideally here we would also change the prototype of the methods taking key name as String to &str as it was done in the client 😬 However that would imply a ton of change in the tests to change everything so probably better not to do it now. It's worth though to open a new issue indicating that this is something to be done!

I changed this as it threw an error when I was compiling

e2e_tests/tests/per_provider/normal_tests/capability_discovery.rs

hug-dev · 2021-09-20T10:18:40Z

e2e_tests/tests/per_provider/normal_tests/capability_discovery.rs

@@ -0,0 +1,371 @@
+// Copyright 2021 Contributors to the Parsec project.


We might want to make capability_discovery an "all-providers" test. It might be nice to test that specific attributes work on one virtual crypto backend (software TPM, softHSM, etc) but not on others. As for CI the backend we use are fixed and versioned we can check what is supported there and make sure we return the correct info in Parsec.

edit: it's fine to keep it like this for now. We might also want to add tests where we first call can_do_crypto and then try to perform the operation with the same attribute if it succeeds.

A good test of this operation is to try to call it in the beginning of some e2e tests with the same attributes to make sure the test will succeed.

e2e_tests/tests/per_provider/normal_tests/capability_discovery.rs

hug-dev · 2021-09-20T12:22:18Z

e2e_tests/tests/per_provider/normal_tests/capability_discovery.rs

+
+ let status = client.can_do_crypto(CheckType::Use, attributes);
+
+ assert_eq!(Ok(()), status)


Same question here the mix between key type (RSA) and alg (ECDSA) is weird 🤔

hug-dev · 2021-09-20T13:02:47Z

src/providers/pkcs11/capability_discovery.rs

+ if std::any::type_name::<Ulong>() == std::any::type_name::<u64>() {
+ if !((attributes.bits as u64) >= (*mechanism_info.min_key_size()).into()
+ && (attributes.bits as u64) <= (*mechanism_info.max_key_size()).into())
+ {
+ return Err(PsaErrorNotSupported);
+ }
+ } else {
+ if !((*mechanism_info.min_key_size() as u64) <= (attributes.bits as u64)
+ && (attributes.bits as u64) <= (*mechanism_info.max_key_size() as u64))
+ {
+ return Err(PsaErrorNotSupported);
+ }
+ }


There is a From<Ulong> for CK_ULONG implemented, that could help performing the operations on CK_ULONG not caring if that type is u32 or u64?

hug-dev · 2021-09-20T13:12:49Z

src/providers/pkcs11/capability_discovery.rs

+ } else if !(attributes.policy.usage_flags.decrypt()
+ || attributes.policy.usage_flags.encrypt()
+ || attributes.policy.usage_flags.sign_hash()
+ || attributes.policy.usage_flags.sign_message()
+ || attributes.policy.usage_flags.verify_hash()
+ || attributes.policy.usage_flags.verify_message())
+ {
+ return Err(PsaErrorNotSupported);
+ }


Not sure if we need to care about the usage_flags here.

hug-dev · 2021-09-20T13:13:20Z

src/providers/pkcs11/capability_discovery.rs

+ let supported_mechanisms: Vec<MechanismType> = self
+ .backend
+ .get_mechanism_list(self.slot_number)
+ .map_err(to_response_status)?;
+ let mechanism = Mechanism::try_from(attributes.policy.permitted_algorithms)
+ .map_err(to_response_status)?;
+ let mechanism_info: MechanismInfo = self
+ .backend
+ .get_mechanism_info(self.slot_number, mechanism.mechanism_type())
+ .map_err(to_response_status)?;
+ if !(supported_mechanisms.contains(&mechanism.mechanism_type())) {
+ return Err(PsaErrorNotSupported);
+ }


That's pretty clean!
I am thinking, maybe we should put that (and the key size check below) inside the use_check function? And replace the content of the if with just a call to use_check?

In the match at the end of the function we can check for CheckType::Use that the algorithm is not None.

hug-dev · 2021-09-20T13:21:20Z

src/providers/pkcs11/capability_discovery.rs

+ Type::RsaKeyPair | Type::RsaPublicKey => (),
+ _ => return Err(PsaErrorNotSupported),
+ }
+ if attributes.policy.permitted_algorithms != Algorithm::None {


In the case it isn't None, maybe it would be good to call: attributes.compatible_with_alg(attributes.policy.permitted_algorithms)?; to make sure the algorithm is compatible with the key type. That will return PsaErrorNotPermitted which can be added as a possible return value from this operation in the contract. See here

hug-dev · 2021-09-22T10:22:43Z

src/providers/pkcs11/capability_discovery.rs

+ {
+ return Err(PsaErrorNotSupported);
+ }
+ attributes.compatible_with_alg(attributes.policy.permitted_algorithms)?;


Can you please make a PR on the book operation saying that NotPermitted is returned in that case if the alg is not compatible?

hug-dev · 2021-09-22T10:30:20Z

src/providers/pkcs11/capability_discovery.rs

+ }))
+ {
+ return Err(PsaErrorNotSupported);
+ }


If attributes.policy.permitted_algorithms is not None, I think we should call use_check from here too. Same for import. I think some tests with non-compatible key type/algorithm are still passing because of that.

Calling use_check causes an issue as it checks if at least on of the the relevant usage flags are set which isn't applicable to import and generate so would it work to just run attributes.compatible_with_alg(attributes.policy.permitted_algorithms)?; to check if the type and algorithm match?

Reading the contract we have written now, I think it should be applicable?

For the Generate, Import and Derive check there is:

If the key_algorithm is not None, also perform the Use check.

So are we saying that Generate and Import should have also have at least one of the relevant usage flags set? (And Derive but that is never supported by pkcs11)

I think so, if an Algorithm is set, it means that it should be used for at least something

…est scripts Added the internal CanDoCrypto as a new capability_discovery file in the pkcs11 provider and made corresponding changes to the relevent files. Also added the end to end tests for CanDoCrypto and fixed the other tests that were still using the old syntax for the usage flags. Signed-off-by: Sam Davis <sam.davis@arm.com>

hug-dev

Thanks for the change. I think since this is a development branch, work can continue on it to make things better little by little in further PRs.

It's really hard to cover everything now, I think we will improve it when adding it as a pre-requisite for our other e2e tests.

ionut-arm

Let's merge this as is and we'll make note of ways to improve the code in the original issue!

Kakemone force-pushed the CanDoCrypto_patch branch from ca80d74 to f13e011 Compare September 13, 2021 10:54

ionut-arm changed the base branch from main to can-do-crypto September 13, 2021 11:10

Kakemone force-pushed the CanDoCrypto_patch branch 9 times, most recently from 210e768 to bbfafae Compare September 14, 2021 11:07

ionut-arm reviewed Sep 14, 2021

View reviewed changes

Kakemone force-pushed the CanDoCrypto_patch branch 8 times, most recently from 897ab4c to b0497ef Compare September 15, 2021 15:26

Kakemone requested a review from hug-dev September 16, 2021 09:38

Kakemone force-pushed the CanDoCrypto_patch branch from b0497ef to a5fdc55 Compare September 20, 2021 09:49

hug-dev reviewed Sep 20, 2021

View reviewed changes

Kakemone force-pushed the CanDoCrypto_patch branch 2 times, most recently from b2aecbc to aac48c0 Compare September 22, 2021 08:16

hug-dev reviewed Sep 22, 2021

View reviewed changes

Kakemone force-pushed the CanDoCrypto_patch branch from aac48c0 to 3d97e9d Compare September 23, 2021 08:22

hug-dev approved these changes Sep 23, 2021

View reviewed changes

Kakemone requested a review from ionut-arm September 24, 2021 13:15

ionut-arm approved these changes Sep 28, 2021

View reviewed changes

hug-dev merged commit b751323 into parallaxsecond:can-do-crypto Sep 28, 2021

Kakemone deleted the CanDoCrypto_patch branch August 31, 2022 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the CanDoCrypto operation as well as fixing some of the other test scripts. #522

Added the CanDoCrypto operation as well as fixing some of the other test scripts. #522

Kakemone commented Sep 10, 2021

ionut-arm left a comment

ionut-arm Sep 14, 2021

ionut-arm Sep 14, 2021

ionut-arm Sep 14, 2021

Kakemone Sep 15, 2021

hug-dev Sep 20, 2021

hug-dev left a comment

hug-dev Sep 20, 2021

Kakemone Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 20, 2021

hug-dev Sep 22, 2021

hug-dev Sep 22, 2021

Kakemone Sep 22, 2021

hug-dev Sep 22, 2021

Kakemone Sep 22, 2021

hug-dev Sep 22, 2021

hug-dev left a comment

ionut-arm left a comment

		@@ -0,0 +1,371 @@
		// Copyright 2021 Contributors to the Parsec project.


		let status = client.can_do_crypto(CheckType::Use, attributes);

		assert_eq!(Ok(()), status)

Added the CanDoCrypto operation as well as fixing some of the other test scripts. #522

Added the CanDoCrypto operation as well as fixing some of the other test scripts. #522

Conversation

Kakemone commented Sep 10, 2021

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

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

hug-dev left a comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment