Add consistency in key creation/deletion #324

hug-dev · 2021-01-21T19:01:28Z

on all providers.
Also make the core operations looping on all providers "best-effort" by
being infallible.

This is mainly for initial review/CI testing. This makes the assumption that this comment is agreed but I can change later on.

Fix #323
Fix #319
Fix #149

on all providers. Also make the core operations looping on all providers "best-effort" by being infallible. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

ionut-arm

Cheers for the changes, it's nice to make the providers more consistent

src/key_info_managers/mod.rs

ionut-arm · 2021-01-22T15:11:42Z

src/key_info_managers/mod.rs

+    ///
+    /// Returns PsaErrorAlreadyExists if the key triple already exists or KeyInfoManagerError for
+    /// another error.
+    pub fn already_exists(&self, key_triple: &KeyTriple) -> Result<(), ResponseStatus> {


I get that the doc above says how this method works, but to me already_exists implies that Ok(()) would mean yes, not no - at least that's what I'd assume it does if I see it used somewhere in a PR. Maybe something like is_unique (or check_unique) matches the return type better? Or return Result<bool, ResponseStatus>?

ah yes you are correct, now that I look at it it's kind of wrong 🤡
I changed the return type following the same kind of convention we use in the interface:

a method starting by is (an interrogation) return just a bool

a method which name is a declaration returns a Result

I can change the name to does_not_already_exist

ionut-arm · 2021-01-22T15:16:41Z

src/providers/core/mod.rs

@@ -99,7 +104,12 @@ impl Provide for Provider {

        let mut clients: Vec<String> = Vec::new();
        for provider in &self.prov_list {
-            let mut result = provider.list_clients(_op)?;
+            let mut result = provider.list_clients(_op).unwrap_or_else(|e| {
+                error!("list_clients failed on provider with {}", e);


Any way to include the provider ID in that error message?

(same for the other methods that you changed)

I could do it with the describe method? I tried that rapidly but felt like it was going too far for this purpose

Yeah, was wondering about that... I'm just thinking that since the operation is unlikely to proper fail and since logs are the only thing that tells you what fails maybe having more specific messages would help

ionut-arm · 2021-01-22T15:26:07Z

src/providers/mbed_crypto/key_management.rs

+                // If this fails, then we will have a zombie key in the Mbed Crypto backend. This
+                // is probably not a big problem apart from memory consumption.


It can still cause some operations to fail: if you restart the service and don't have a higher ID used (the IDs are allocated incrementally, right?) then you'll end up trying to use this one and that will fail, but the next generate/import will be fine, so it just needs a retry.

ah yes you are correct, I guess this is the problem with this method... And will be the same with PKCS11 even though we use a random key ID generator: if it happens that we create the same key ID of a zombie key then the operation will have to be retried. In the worst case, if that happens a lot, one would have to re-try the key generation several times before finding an unallocated key ID.
Should we try to delete the key then, if insert_key_id fails? Here and for the PKCS11 provider.

src/providers/pkcs11/key_metadata.rs

ionut-arm · 2021-01-25T13:58:51Z

Oh, there's one more issue that I forgot to put in the issue or note here:

In psa_destroy_key_internal in PKCS11 you have two destroy operations happening one after the other (because you could have two parts of a key), but currently if the first bit fails you just return - destroying the second part never happens. Now that you destroy the metadata regardless of that error code I think you should attempt to delete the 2nd part as well, regardless of what happens with the first one. I guess if either fails you return an error

hug-dev · 2021-01-25T16:49:19Z

I think you should attempt to delete the 2nd part as well

Yes, makes sense!

* Modifies the name of already_exists to does_not_exist * Delete zombie keys in a best effort way 🧟 * in PKCS11, destroy key, try to destroy both parts of the key pair in a best effort way Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

hug-dev · 2021-01-25T18:01:58Z

Addressed your comments in a new commit for easier review 🧟

ionut-arm

Thanks, looks good, one question but it's just a nit

ionut-arm · 2021-01-26T17:21:11Z

src/providers/pkcs11/key_management.rs

+        if let Err(e) = first_res {
+            Err(e)
+        } else if let Err(e) = second_res {
+            Err(e)
+        } else {
+            Ok(psa_destroy_key::Result {})
+        }


Suggested change

if let Err(e) = first_res {

Err(e)

} else if let Err(e) = second_res {

Err(e)

} else {

Ok(psa_destroy_key::Result {})

}

first_res?;

second_res?;

Ok(psa_destroy_key::Result {})

Would this work?

Oh yes that's nice 👌

hug-dev · 2021-01-26T18:39:57Z

I will also add the algorithm we follow for those things, in high level terms in the Provide trait for generate, import and delete so that future implementors can follow the same recipe.

To make future implementations consistent as well. Also improves code snippet using ?. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

Add consistency in key creation/deletion

610cdc5

on all providers. Also make the core operations looping on all providers "best-effort" by being infallible. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

hug-dev requested a review from ionut-arm January 22, 2021 10:36

hug-dev added the multitenancy Getting Parsec to provide isolated key stores for multiple clients based on an identity mechanism label Jan 22, 2021

ionut-arm reviewed Jan 22, 2021

View reviewed changes

Add changes after review 🧟

25ccdcd

* Modifies the name of already_exists to does_not_exist * Delete zombie keys in a best effort way 🧟 * in PKCS11, destroy key, try to destroy both parts of the key pair in a best effort way Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

ionut-arm approved these changes Jan 26, 2021

View reviewed changes

Add Provide trait info about key creation/deletion

5d61d3c

To make future implementations consistent as well. Also improves code snippet using ?. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>

hug-dev merged commit 4ea43cc into parallaxsecond:master Jan 27, 2021

hug-dev deleted the delete-client-re branch January 27, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add consistency in key creation/deletion #324

Add consistency in key creation/deletion #324

hug-dev commented Jan 21, 2021

ionut-arm left a comment

ionut-arm Jan 22, 2021

hug-dev Jan 25, 2021

ionut-arm Jan 22, 2021

ionut-arm Jan 22, 2021

hug-dev Jan 25, 2021

ionut-arm Jan 25, 2021

ionut-arm Jan 22, 2021

hug-dev Jan 25, 2021

ionut-arm commented Jan 25, 2021

hug-dev commented Jan 25, 2021

hug-dev commented Jan 25, 2021

ionut-arm left a comment

ionut-arm Jan 26, 2021

hug-dev Jan 26, 2021

hug-dev commented Jan 26, 2021

		// If this fails, then we will have a zombie key in the Mbed Crypto backend. This
		// is probably not a big problem apart from memory consumption.

Add consistency in key creation/deletion #324

Add consistency in key creation/deletion #324

Conversation

hug-dev commented Jan 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm commented Jan 25, 2021

hug-dev commented Jan 25, 2021

hug-dev commented Jan 25, 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

hug-dev commented Jan 26, 2021