Move test client back in the Parsec repo #150

ionut-arm · 2020-04-20T13:55:54Z

This commit moves the test client back in the main Parsec repo and moves
it on top of the BasicClient - the main core client, thus making the
most out of functionality re-use.

Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com

hug-dev

Thanks for this!! A few general questions to start a discussion.

hug-dev · 2020-04-20T13:59:46Z

Cargo.toml

@@ -40,12 +40,13 @@ derivative = "1.0.3"
 version = "3.0.0"

 [dev-dependencies]
-parsec-client-test = { git = "https://github.com/parallaxsecond/parsec-client-test", tag = "0.3.0" }


hug-dev · 2020-04-20T14:01:25Z

Cargo.toml

 num_cpus = "1.10.1"
 picky-asn1-der = "0.2.2"
 picky-asn1 = "0.2.1"
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.8.1"
+parsec-client = { git = "https://github.com/parallaxsecond/parsec-client-rust" }


Let's publish it as a crate! I think it would make sense to do it now and pull it as a normal crates.io dependence! To get rid of the different versions in dependences issue.

I'm sorta relying on the CI to tell me if the client actually works 😅 if it does, will replace!

I see, makes sense!

hug-dev · 2020-04-20T14:06:17Z

tests/per_provider/normal_tests/ping.rs


 #[test]
 fn test_ping() -> Result<()> {
    let mut client = TestClient::new();
-    let version = client.ping(ProviderID::Core)?;
+    let version = client.ping()?;


So much nicer to not have to put the redundant ProviderID::Core

hug-dev · 2020-04-20T14:07:07Z

tests/test_client/abstract_client.rs

@@ -0,0 +1,329 @@
+// Copyright 2019 Contributors to the Parsec project.


2020 I think 😁

Ok, I wasn't sure because I thought I saw you changing some to 2019

I should have changed all ranges 2019 - 2020 to 2019 but kept 2020 to 2020. If not I made a mistake!

hug-dev · 2020-04-20T14:12:06Z

tests/test_client/abstract_client.rs

+
+/// Client structure automatically choosing a provider and high-level operation functions.
+#[derive(Debug)]
+pub struct TestClient {


About naming, I am thinking: this structure TestClient in test_client/abstract_client.rs does not feel totally right. Ideally we should keep the same name of the main structure of a file and its name. Any idea?

Suggestion:

have the folder tests/test_clients (note the s)

inside have Abstract, Stress and Raw structures under abstract.rs, stress.rs and raw.rs. Dropping test_client as it is in the upper module's name 💇‍♂️

abstract is an identifier.

Also, I'm not sure about having the clients named Abstract, Stress etc. - while it is true that they reside within a test_clients module, it's confusing to take Client part out of the structure name, as when you want to use it somewhere you shouldn't need to do let client = test_clients::Stress::new();; even this looks dubious to me - what's Stress exactly? I think the whole idea of namespacing works and is helpful in some circumstances, but is a bit over the top in others.

I'll rename the modules within test_clients, but will keep the Client part of the struct name.

hug-dev · 2020-04-20T14:21:46Z

tests/test_client/abstract_client.rs

+#[derive(Debug)]
+pub struct TestClient {
+    op_client: BasicClient,
+    cached_opcodes: Option<HashMap<ProviderID, HashSet<Opcode>>>,


Is caching opcodes still helpful for our tests? We might only need to know one crypto provider ID.

We might only need to know one crypto provider ID.

And isn't that a very good example where provider discovery happens? 😸 You have only one provider in the system so the test client finds it for you.

Yes sure, the provider discovery is an excellent idea! I meant since we are doing all requests to one single provider, we might not need to cache this HashMap but just be able to do the discovery in the builder and use that as the provider field of the BasicClient?

hug-dev · 2020-04-20T14:22:20Z

tests/test_client/abstract_client.rs

+pub struct TestClient {
+    op_client: BasicClient,
+    cached_opcodes: Option<HashMap<ProviderID, HashSet<Opcode>>>,
+    provider: Option<ProviderID>,


Could this be the same provider as the one in the basic client?

I was thinking about that, but given how the whole provider discovery thing works, this is the way to force a test to use one specific provider, because the flow goes like this:

you ask for an operation to be executed, say, generate key

the client checks to see if you hardcoded a value (this Option<ProviderID>)

if yes, it uses that

if no, it checks the cache for a provider that it can use

Most of our tests are provider-independent so would have to include some sort of "active provider" discovery mechanism. But it's also useful to have the option to force the client to use a particular provider. This can't be done with the implicit_provider of the underlying BasicClient because that one isn't optional, so you wouldn't know whether it was hardcoded by the test or if it was there from the previous request.

Also, I just realised - do we actually have any tests where we attempt to make a request against a provider that isn't active?

this is the way to force a test to use one specific provider

I do not think we are actually using that feature! The only occurence of set_provider in our tests is in basic::wrong_provider_code to test that a crypto operation on core fails. That wouldn't be possible at the BasicClient level anyway because of the new restrictions.

That could be useful in the all_providers tests, if you want to execute requests on multiple providers.

This can't be done with the implicit_provider of the underlying BasicClient because that one isn't optional, so you wouldn't know whether it was hardcoded by the test or if it was there from the previous request.

Ok I think I get you, you can change the provider ID just for one request and then set it to None again so that it uses the one originally selected by the discovery.

Can you do that with a method replace_provider that replace the current implicit_provider with the one given and returns the old one? That way this logic could be done in the test itself when and if it is needed in the future.

The reason I am asking is that if we remove this logic, we can actually use in all of our tests the BasicClient methods directly for most operations and not have this wrapper in the test client. That would make the code much simpler I think.

Also, I just realised - do we actually have any tests where we attempt to make a request against a provider that isn't active?

I don't think so, and it would be good to add!

hug-dev · 2020-04-20T14:22:33Z

tests/test_client/abstract_client.rs

+    op_client: BasicClient,
+    cached_opcodes: Option<HashMap<ProviderID, HashSet<Opcode>>>,
+    provider: Option<ProviderID>,
+    auth: AuthenticationData,


Same question here

I think this one makes sense to remove

hug-dev

Thanks for the changes and new namings!

hug-dev · 2020-04-20T16:40:40Z

README.md

@@ -80,17 +80,47 @@ $ cd parsec
 $ RUST_LOG=info cargo run
 ```

-Parsec Client Libraries can now communicate with the service. For example using the Rust Test client,
+Parsec Client Libraries can now communicate with the service. For example using the Rust client,


Nice! Thanks for handling #137 as part of this

Uhm... Now that we moved the provider out of the constructor for the client and we advise users to go through list_providers, list_opcodes first... I feel like this example is becoming quite heavy... it went from like 10 lines to probably 40-50 if I do the discovery part too... What do? Link to the client documentation once that's out?

That's probably the best thing to do! Even more so because the example we would put here would not be checked for compilation often and might not even work at times 😬

hug-dev · 2020-04-20T16:49:10Z

README.md

+use parsec_client::core::psa_key_attributes::{KeyAttributes, KeyType, KeyPolicy, UsageFlags};
+use sha2::Sha256;
+
+let app_identity_ = AuthenticationData::AppIdentity(String::from("my-app"));


Nitpick: seems like there is an extraneous "_"

hug-dev · 2020-04-20T17:07:42Z

README.md

-let signature = client.sign(key_name,
-                            String::from("Platform AbstRaction for SECurity").into_bytes())
-                      .unwrap();
+client.psa_generate_key(key_name.clone(), key_attrs).unwrap();


These key name cloning are so annoying :/ I am wondering if we should make the functions take a reference and clone when creating the requests. If we do I am not sure if that should be part of this layer or above (in the Rust client).

I know... we can either do that or make a Rust type for key names that we implement Copy for

hug-dev · 2020-04-20T17:12:52Z

tests/per_provider/provider_cfg/tpm/config.toml

@@ -16,4 +16,4 @@ manager_type = "OnDisk"
 provider_type = "Tpm"
 key_id_manager = "on-disk-manager"
 tcti = "mssim"
-owner_hierarchy_auth = "tpm_pass"
+owner_hierarchy_auth = ""


Is that not good anymore?

😅 my mistake, I changed that for local testing, but forgot to reset it

hug-dev · 2020-04-20T17:15:02Z

tests/test_clients/mod.rs

+    /// The implicit provider chosen for servicing cryptographic operations is decided through
+    /// a call to `list_providers`, followed by choosing the first non-Core provider.


hug-dev · 2020-04-20T17:26:25Z

tests/test_clients/mod.rs

+            basic_client: BasicClient::new(
+                AuthenticationData::AppIdentity(String::from("root")),
+                ProviderID::Core,
+            ),


This constructor feels a bit like the chicken and the egg: you need an implicit provider for non-core operations but you can only do that with list_providers and you need the basic client instance for that.
That is a client change I did not notice before but what do you think if we either take the following decisions:

put the first-crypto provider discovery inside the BasicClient constructor. Not so basic now...

remove the provider argument from the constructor and always but Core, with doc comments saying that list_providers is the first thing expected.

remove the provider argument and put an Option<ProviderID> in the BasicCLient structure, initialised at None by the constructor. We also create a new client error which is throwm when no implicit_provider has been selected.

I personally like 3 best.

Maybe we should continue this in the appropriate GitHub issue

We can try and settle it here, as it's not entirely clear if it's something lacking in the BasicClient or here.

3 makes sense, but I'm guessing you wouldn't want to change the implicit_provider back to None. I think that matching could be done efficiently, so I can do that as another PR in the client.

I guess if implicit_provider is private, you would only change it to Some(ProviderID::...) through methods. Also checking that it's not changing it to Core.
One more suggestion to make it even more clear in the client: renaming implicit_provider to implicit_crypto_provider which what it really is!

Those things give me the feeling that there would be even more reasons to separate our structures (opcodes, provide trait, provider ID, response status) between Crypto (PSA) and Core ones.

One more suggestion to make it even more clear in the client: renaming implicit_provider to implicit_crypto_provider which what it really is!

Well... sorta, that implies we'd be checking on the setter if the provider is a crypto one or not - could do! And then we just assume it's a crypto provider.

Those things give me the feeling that there would be even more reasons to separate our structures (opcodes, provide trait, provider ID, response status) between Crypto (PSA) and Core ones.

Not really, or, I don't think it would 😞 That's because you'd still have a big old enum, just that the underlying variants won't be Core, Mbed, Tpm etc, it would be Core, PsaCrypto. It would be easier to match and separate them, though

I think I'll keep it as implicit_provider - there will be operations that are not explicitly crypto but which need a provider defined (the smart defaulting stuff) - and while they're not available now, it doesn't hurt to have them ready. As you were saying, I'd rather give the user an option to set it to whatever they wish, regardless of whether it is allowed for currently supported operations. It feels like more of a problem with our ProviderID definition than with this field

there will be operations that are not explicitly crypto

Those operations will still be aimed at a a crypto provider though, right? They can still change it with the set_crypto_provider_id methods I think.
My suggestion was mainly about removing the provider ID argument for the new method.

I would be happy with no renaming at all but just the implicit_provider being an Option<ProviderID>, the new method setting it at None and crypto methods returning a new error if it is None.
No need to make more checks in the set_provider method for now.

hug-dev · 2020-04-20T17:33:18Z

tests/test_clients/raw_request.rs

@@ -0,0 +1,49 @@
+// Copyright 2019 Contributors to the Parsec project.


I think those copyrights as well should be 2020

hug-dev · 2020-04-20T17:35:58Z

tests/per_provider/persistent_after.rs

@@ -51,6 +52,6 @@ fn should_have_been_deleted() {
        client
            .destroy_key(key_name)
            .expect_err("This key should have been destroyed."),
-        ResponseStatus::PsaErrorDoesNotExist
+        Error::Service(ResponseStatus::PsaErrorDoesNotExist)


If you want, to avoid having to wrap all ResponseStatus in Error::Service, you could make the test client return Result<_, ResponseStatus> panicking if the error is not Error::Service.
I think all of the test client requests should go to the service and back, otherwise we are not really testing the service.

I didn't mind it that much, but I can change it to unwrap the value before returning

This commit moves the test client back in the main Parsec repo and moves it on top of the `BasicClient` - the main core client, thus making the most out of functionality re-use. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

hug-dev

Nice! Let's get it in!

hug-dev · 2020-04-22T11:06:12Z

tests/test_clients/mod.rs

+    pub fn generate_key(&mut self, key_name: String, attributes: KeyAttributes) -> Result<()> {
+        self.basic_client
+            .psa_generate_key(key_name.clone(), attributes)
+            .map_err(convert_error)?;


hug-dev · 2020-04-22T13:49:32Z

Cargo.toml

 num_cpus = "1.10.1"
 picky-asn1-der = "0.2.2"
 picky-asn1 = "0.2.1"
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.8.1"
+parsec-client = "0.1.0"


ionut-arm added the enhancement New feature or request label Apr 20, 2020

ionut-arm added this to the Parsec production ready milestone Apr 20, 2020

ionut-arm force-pushed the test-client branch from 89d06f5 to 88332af Compare April 20, 2020 14:27

hug-dev reviewed Apr 20, 2020

View reviewed changes

ionut-arm force-pushed the test-client branch from 88332af to e7b4e7e Compare April 20, 2020 16:37

hug-dev reviewed Apr 20, 2020

View reviewed changes

hug-dev mentioned this pull request Apr 21, 2020

Refactor to use the new Rust client parallaxsecond/parsec-client-test#20

Closed

ionut-arm force-pushed the test-client branch 5 times, most recently from 7f9a517 to 25036c3 Compare April 22, 2020 11:02

Move test client back in the Parsec repo

6df3a2d

This commit moves the test client back in the main Parsec repo and moves it on top of the `BasicClient` - the main core client, thus making the most out of functionality re-use. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

ionut-arm force-pushed the test-client branch from 25036c3 to 6df3a2d Compare April 22, 2020 13:45

hug-dev approved these changes Apr 22, 2020

View reviewed changes

ionut-arm merged commit ce5494e into parallaxsecond:master Apr 22, 2020

ionut-arm deleted the test-client branch April 22, 2020 14:38

		@@ -0,0 +1,329 @@
		// Copyright 2019 Contributors to the Parsec project.

		/// The implicit provider chosen for servicing cryptographic operations is decided through
		/// a call to `list_providers`, followed by choosing the first non-Core provider.

		@@ -0,0 +1,49 @@
		// Copyright 2019 Contributors to the Parsec project.

Move test client back in the Parsec repo #150

Move test client back in the Parsec repo #150

Conversation

ionut-arm commented Apr 20, 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 Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

ionut-arm Apr 20, 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

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

hug-dev Apr 20, 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 Apr 21, 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

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

hug-dev Apr 20, 2020 •

edited

Loading

ionut-arm Apr 20, 2020 •

edited

Loading

hug-dev Apr 20, 2020 •

edited

Loading

ionut-arm Apr 21, 2020 •

edited

Loading