Add list keys #261

joechrisellis · 2020-10-08T11:21:36Z

Implementation for ListKeys. This won't immediately pass CI because it's pending a version bump on parsec-interface-rs.

hug-dev

Thanks for adding support for this! I added a few comments of another way to do it that fits better (I think) with our current architecture.

hug-dev · 2020-10-08T12:26:43Z

src/providers/core_provider/mod.rs

@@ -73,6 +85,45 @@ impl Provide for CoreProvider {
        })
    }

+    fn list_keys(&self, _op: list_keys::Operation) -> Result<list_keys::Result> {


This operation should only list the keys belonging to a specific ApplicationName, so you should also add this parameter here and decode it from the auth payload in the back end handler (as it is done for crypto operations on keys).

ACK, will add this 😄

hug-dev · 2020-10-08T12:31:19Z

src/providers/mod.rs

    psa_hash_compute, psa_import_key, psa_raw_key_agreement, psa_sign_hash, psa_verify_hash,
 };
 use parsec_interface::requests::{ResponseStatus, Result};

+type KeyInfoStore = Arc<RwLock<dyn ManageKeyInfo + Send + Sync>>;


I personally do not like type aliasing that much... They kind of make it one step harder to find what the type really is and offer no static checks. I have previously made review comments similar like this on other code so we are trying to not use them too much.

I see the advantage of making the interfaces easier to read but it is not worth it to me.

That's fair enough if this is a design choice we're rolling with -- will revert. 🙂

I guess it's a balance between readability and ease of use. It's kinda similar to the reason why auto exists in C++, to make convoluted types opaque.

They kind of make it one step harder to find what the type really is

That's only true if you don't use an IDE 😛 I prefer the aliasing in cases like this where there are a lot of qualifiers and a type name ends up larger than the rest of a function signature, but I'm not that bothered about it one way or another.

hug-dev · 2020-10-08T12:34:53Z

src/providers/mod.rs

+    /// Get the Provider ID the provider.
+    fn provider_id(&self) -> ProviderID;
+
+    /// Get an optional reference to the KeyInfoStore for this provider. Providers that do not
+    /// implement a key store should return None.
+    fn get_key_info_store(&self) -> Option<&KeyInfoStore>;
+


The Provide trait should ideally only contain the actual operations that the providers can perform with the exception of when it would be really hard/unpractical to do otherwise like with the describe function above. But that one should maybe be in another trait!

My point is that, from the CoreProvider, you can directly get this information by adding a field containing a vector of all ManageKeyInfo implementation and use that trait to get the key information. You can add all of them to the CoreProvider during service building.

Gotcha. Will change this too.

This is where a list_keys implementation on every provider would've helped 🤡 CoreProvider would then just aggregate.

Also, I'd rather go down the route of asking the provider to tell you which ManageKeyInfo it's using, because otherwise imagine having two different key info stores and switching between them at some point. The ListKeys would then report keys that you don't have access to - you see them because they're saved in a different key info manager, but the actual provider can't see them.

hug-dev

That looks good! One suggestion to only need one function from the Provide trait.
Also could you add an end-to-end test which creates a few keys, list them and expect the correct result 🎸 ?

hug-dev · 2020-10-09T14:02:07Z

src/key_info_managers/mod.rs

+    /// # Errors
+    ///
+    /// Returns an error as a String if there was a problem accessing the Key Info Manager.
+    fn list_key_info(


I think I would prefer having this method outside of the trait so that it can not be overwritten:

fn list_key_info( &impl ManageKeyInfo, app_name: &ApplicationName, provider_id: ProviderID, )

hug-dev · 2020-10-09T14:08:58Z

src/providers/mod.rs

@@ -132,6 +142,16 @@ pub trait Provide {
        Err(ResponseStatus::PsaErrorNotSupported)
    }

+    /// Lists all keys belonging to the application.
+    fn list_keys(


I think it would be nicer to only have the list_keys method and:

in all the crypto providers make it the same as what find_keys currently does

in the code provider call list_keys on all provider

Although we need to be sure somewhere that ListKeys can not be called directly on the crypto providers. Probably somewhere higher on the stack.

hug-dev · 2020-10-09T14:13:43Z

src/back/backend_handler.rs

@@ -207,6 +207,14 @@ impl BackEndHandler {
                trace!("list_authenticators egress");
                self.result_to_response(NativeResult::ListAuthenticators(result), header)
            }
+            NativeOperation::ListKeys(op_list_keys) => {
+                let app_name =


The check I was talking in the other comment could be here but could also be in the is_capable method where we would check if the request's opcode is a Core or a Crypto operation and based on that check if the provider can do it or not.

hug-dev · 2020-10-14T15:59:13Z

@joechrisellis gave me permission to continue the work on his fork, so I went ahead and addressed the comments of my own review 🤓

ionut-arm · 2020-10-14T16:09:24Z

src/back/backend_handler.rs

    /// - if the provider ID does not match, returns `ResponseStatus::WrongProviderID`
    /// - if the content type does not match, returns `ResponseStatus::ContentTypeNotSupported`
    /// - if the accept type does not match, returns `ResponseStatus::AcceptTypeNotSupported`
    pub fn is_capable(&self, request: &Request) -> Result<()> {
        let header = &request.header;

+        if (self.provider_id == ProviderID::Core) ^ header.opcode.is_core() {


Suggested change

if (self.provider_id == ProviderID::Core) ^ header.opcode.is_core() {

if (self.provider_id == ProviderID::Core) != header.opcode.is_core() {

Seems easier to read/understand

I think though with != it will not go into the if in the following counter-example:

self.provider_id is Provider::Tpm

header.opcode is Opcode::Ping

edit: nvm I am wrong

I will update, your version is definitely better

e2e_tests/tests/all_providers/mod.rs

ionut-arm · 2020-10-15T09:43:13Z

src/key_info_managers/mod.rs

+pub fn list_keys(
+    manager: &dyn ManageKeyInfo,
+    app_name: &ApplicationName,
+    provider_id: ProviderID,
+) -> Result<Vec<parsec_interface::operations::list_keys::KeyInfo>, String> {
+    use parsec_interface::operations::list_keys::KeyInfo;
+
+    let mut keys: Vec<KeyInfo> = Vec::new();
+    let key_triples = manager.get_all(provider_id)?;
+
+    for key_triple in key_triples {
+        if key_triple.app_name() != app_name {
+            continue;
+        }
+
+        let key_info = manager.get(key_triple)?;
+        let key_info = match key_info {
+            Some(key_info) => key_info,
+            _ => continue,
+        };
+
+        keys.push(KeyInfo {
+            provider_id: ProviderID::MbedCrypto,
+            name: key_triple.key_name().to_string(),
+            attributes: key_info.attributes,
+        });
+    }
+
+    Ok(keys)
+}


You can actually include this in the ManageKeyInfo trait, because you only rely on methods on manager that are exposed through that trait. You'd just change manager.get<all> with self.get<all>

The reason why I did not include it is to prevent overwriting this method. I think the result is the same?

to prevent overwriting this method

To prevent overwriting what?

I think the result is the same?

It is, in the end, I just tend to prefer methods to functions when the function is clearly acting on a specific type.

Well I might be wrong but if a method is in a trait then implementation can either not implement this method and use the default implementation, or overwrite it.

Ah, yes, true, but we have control over that. And it might be that that's an advantage, because for a database, for example, we might be able to build a query to get only the entries that match those requirements instead of filtering ourselves.

ionut-arm · 2020-10-15T09:46:35Z

src/providers/tpm/mod.rs

@@ -1,22 +1,23 @@
-// Copyright 2019 Contributors to the Parsec project.
+//KeyInfoStore Copyright 2019 Contributors to the Parsec project.


vim played me like a fool once again

ionut-arm

👍

Co-authored-by: Hugues de Valon <[email protected]> Signed-off-by: Joe Ellis <[email protected]>

joechrisellis force-pushed the add-list-keys branch from 5d095b2 to fc357e5 Compare October 8, 2020 11:22

joechrisellis requested review from hug-dev and ionut-arm October 8, 2020 11:22

hug-dev reviewed Oct 8, 2020

View reviewed changes

joechrisellis force-pushed the add-list-keys branch 2 times, most recently from 6d7b198 to 93465d3 Compare October 9, 2020 10:36

joechrisellis mentioned this pull request Oct 9, 2020

Bugfix: use UnixPeerCredentials instead of PeerCredentials #262

Closed

joechrisellis force-pushed the add-list-keys branch 4 times, most recently from 4432672 to 0e74e42 Compare October 9, 2020 11:42

hug-dev reviewed Oct 9, 2020

View reviewed changes

hug-dev force-pushed the add-list-keys branch from 0e74e42 to 12b983e Compare October 14, 2020 15:58

ionut-arm reviewed Oct 14, 2020

View reviewed changes

hug-dev force-pushed the add-list-keys branch from 12b983e to 1c38ee8 Compare October 14, 2020 16:31

hug-dev mentioned this pull request Oct 14, 2020

Add authentication configuration #273

Merged

hug-dev force-pushed the add-list-keys branch from 1c38ee8 to f694e00 Compare October 14, 2020 17:00

ionut-arm reviewed Oct 15, 2020

View reviewed changes

hug-dev force-pushed the add-list-keys branch from f694e00 to b81ee59 Compare October 15, 2020 10:18

ionut-arm approved these changes Oct 15, 2020

View reviewed changes

Add ListKeys implementation

f58a43f

Co-authored-by: Hugues de Valon <[email protected]> Signed-off-by: Joe Ellis <[email protected]>

hug-dev force-pushed the add-list-keys branch from b81ee59 to f58a43f Compare October 15, 2020 10:36

hug-dev merged commit 48d2f90 into parallaxsecond:master Oct 15, 2020

hug-dev deleted the add-list-keys branch October 15, 2020 11:10

hug-dev mentioned this pull request Jan 8, 2021

Investigate adding ListClients and DeleteClient operations #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list keys #261

Add list keys #261

joechrisellis commented Oct 8, 2020

hug-dev left a comment

hug-dev Oct 8, 2020

joechrisellis Oct 8, 2020

hug-dev Oct 8, 2020

joechrisellis Oct 8, 2020

ionut-arm Oct 8, 2020

hug-dev Oct 8, 2020

joechrisellis Oct 8, 2020

ionut-arm Oct 8, 2020

hug-dev left a comment

hug-dev Oct 9, 2020

hug-dev Oct 9, 2020

hug-dev Oct 9, 2020

hug-dev commented Oct 14, 2020

ionut-arm Oct 14, 2020

hug-dev Oct 14, 2020 •

edited

Loading

hug-dev Oct 14, 2020

ionut-arm Oct 15, 2020

hug-dev Oct 15, 2020

ionut-arm Oct 15, 2020

hug-dev Oct 15, 2020

ionut-arm Oct 15, 2020

ionut-arm Oct 15, 2020

hug-dev Oct 15, 2020

ionut-arm left a comment

	if (self.provider_id == ProviderID::Core) ^ header.opcode.is_core() {
	if (self.provider_id == ProviderID::Core) != header.opcode.is_core() {

		@@ -1,22 +1,23 @@
		// Copyright 2019 Contributors to the Parsec project.
		//KeyInfoStore Copyright 2019 Contributors to the Parsec project.

Add list keys #261

Add list keys #261

Conversation

joechrisellis commented Oct 8, 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

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

hug-dev commented Oct 14, 2020

Choose a reason for hiding this comment

hug-dev Oct 14, 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

ionut-arm left a comment

Choose a reason for hiding this comment

hug-dev Oct 14, 2020 •

edited

Loading