Seed initial client #1

ionut-arm · 2020-03-20T11:34:19Z

This commit adds the initial version of the Parsec Rust client.

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

This version of the client is not finished and requires a lot of changes for it to be usable/production-ready. Some of the things that are not done well and will be improved are:

documentation is minimal and needs to be expanded
tests should be implemented, either with a "mock service" or with the real Parsec service
there are still unsafe uses of unwrap, expect ...
there is no configuration of the parameters used
the API of the client was not designed to match any existing "ergonomic design" crypto APIs from the Rust ecosystem
potentially others - if you find this kind of issues that are more structural than syntactic/aesthetic, please add a [FYI] comment and raise an issue for later fixing

Recommended review order:

all the markdown/legal files
src/core/request_handler.rs
src/core/mod.rs
src/auth.rs
src/lib.rs
CI-related files

ionut-arm · 2020-03-20T12:01:12Z

test/ci.sh

@@ -0,0 +1,41 @@
+#!/usr/bin/env bash


The parent folder of this will be renamed to tests to fix the CI workflows

hug-dev

Thanks for fleshing out the Rust client!
I like the idea of the Core client: a basic client that allows to send all possible operations with all parameters, with no "intelligence". That will be the basis for more advanced, abstracted modules. I propose, to make it easier to scale, to put this core client in his own module.

README.md

hug-dev · 2020-03-20T21:53:28Z

src/auth.rs

+
+/// Authentication token
+#[derive(Clone, Debug)]
+pub enum AuthToken {


To match documentation I would name this structure AuthType (or just Authentication) with variants: None, Direct { application_identity: String } and in the future maybe Token(...)

Indeed, didn't realise Token was just the JWT stuff

Reckon we can name it ApplicationIdentity? I think in the direct case, the String could be called name instead

For me the ApplicationIdentity is only the identity of the application in clear text, the name used for namespacing the key names for example. For me, it is directly passed for Direct authentication type and included in the JWT token. This type seems to represent that particular authentication field type and its content?

Cool, will name the field inside DirectAuthentication as identity (documentation names it that too), but I'll try and find a name for this struct - Authentication isn't really correct

Settled on AuthenticationFactor as this seems to be the term for the information provided when authenticating something.

Ergh, or maybe not 😕

src/core/request_handler.rs

hug-dev · 2020-03-20T22:05:34Z

src/core/request_handler.rs

+        stream
+            .set_read_timeout(self.timeout)
+            .expect("Failed to set read timeout for stream");
+        stream
+            .set_write_timeout(self.timeout)
+            .expect("Failed to set write timeout for stream");


Maybe the socket connection + the timeout setting can be done only once when building the RequestHandler? So that after it is faster so send requests.

Not really, this is a problem we encountered last time - one stream = one E2E connection. Once the response is sent, the service closes its side and writing to the stream results in a broken pipe error.

(Also not closing the connection on the service side would probably be a memory leak)

Oh yes sorry you are right! I forgot about that one 😃

src/lib.rs

hug-dev · 2020-03-20T22:15:02Z

src/lib.rs

+    }
+
+    /// generate key
+    pub fn generate_key(


Should we prefix them with psa_ as well?

Don't know what to say. Maybe at this lowest layer it could make sense, but I don't think clients would be particularly interested in the fact that this operation comes from the PSA spec. Might be wrong... I think my point is that this could(/should?) be an abstraction over the things offered by the service. Not sure how it would be settled for a client that supports some provider-specific version of, say, generate_key

Maybe at this lowest layer it could make sense
That is what I was thinking as well: that this low layer (the "core") could be the dumb translation, in client side, of all our operation with all parameters. With that in mind, it would make sense to me to have the psa_ prefix, as those are PsaXXX operations.

I agree that once on higher abstraction layers, we should probably drop the prefix as it would make less sense.

hug-dev · 2020-03-20T22:17:03Z

src/lib.rs

+///
+/// The client exposes low-level functionality for using the Parsec service
+#[derive(Debug)]
+pub struct CoreClient {


As the name of the structure suggests, I feel like it would belong better in the core module. Maybe we should leave src/lib.rs for re-exports, have core for exposing all our operations will all parameters so that it will be easier to expand with another module later which does more of smart-defaulting.

I'd be ok with not implementing anything in lib.rs and just re-exporting from here

hug-dev · 2020-03-20T22:19:09Z

src/core/mod.rs

+/// OperationHandler structure to send a `NativeOperation` and get a `NativeResult`.
+#[derive(Derivative)]
+#[derivative(Debug)]
+pub struct OperationHandler {


If the Core operations currently in src/lib.rs went here, this file could go to src/core/operation_handler.rs for example.

Makes sense!

hug-dev

Sorry for next round of comments. Maybe it would have been easier to sketch the design before and discuss on it 😕

hug-dev · 2020-03-23T11:52:35Z

src/auth.rs

+
+/// Authentication token
+#[derive(Clone, Debug)]
+pub enum AuthToken {


For me the ApplicationIdentity is only the identity of the application in clear text, the name used for namespacing the key names for example. For me, it is directly passed for Direct authentication type and included in the JWT token. This type seems to represent that particular authentication field type and its content?

src/core/request_handler.rs

hug-dev · 2020-03-23T11:55:08Z

src/core/request_handler.rs

+        stream
+            .set_read_timeout(self.timeout)
+            .expect("Failed to set read timeout for stream");
+        stream
+            .set_write_timeout(self.timeout)
+            .expect("Failed to set write timeout for stream");


Oh yes sorry you are right! I forgot about that one 😃

hug-dev · 2020-03-23T11:58:29Z

src/lib.rs

+    }
+
+    /// generate key
+    pub fn generate_key(


Maybe at this lowest layer it could make sense
That is what I was thinking as well: that this low layer (the "core") could be the dumb translation, in client side, of all our operation with all parameters. With that in mind, it would make sense to me to have the psa_ prefix, as those are PsaXXX operations.

I agree that once on higher abstraction layers, we should probably drop the prefix as it would make less sense.

This commit adds the initial version of the Parsec Rust client. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

ionut-arm · 2020-03-23T14:39:42Z

I think most points raised should be covered, apart from the naming of authentication-related stuff.

hug-dev

Thanks for addressing the comments!

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

ionut-arm requested review from hug-dev and anta5010 March 20, 2020 11:34

ionut-arm self-assigned this Mar 20, 2020

ionut-arm commented Mar 20, 2020

View reviewed changes

hug-dev reviewed Mar 20, 2020

View reviewed changes

hug-dev reviewed Mar 23, 2020

View reviewed changes

Seed initial client

fc5e60c

This commit adds the initial version of the Parsec Rust client. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

ionut-arm force-pushed the init branch from 90f64f8 to fc5e60c Compare March 23, 2020 14:38

hug-dev approved these changes Mar 23, 2020

View reviewed changes

ionut-arm merged commit 26d08ee into parallaxsecond:master Mar 24, 2020

ionut-arm deleted the init branch March 24, 2020 09:42

ionut-arm mentioned this pull request Apr 21, 2020

Improve use of unsafe methods #4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seed initial client #1

Seed initial client #1

ionut-arm commented Mar 20, 2020

ionut-arm Mar 20, 2020

hug-dev left a comment

hug-dev Mar 20, 2020

ionut-arm Mar 23, 2020

ionut-arm Mar 23, 2020 •

edited

Loading

hug-dev Mar 23, 2020

ionut-arm Mar 23, 2020

ionut-arm Mar 23, 2020

ionut-arm Mar 23, 2020

hug-dev Mar 20, 2020

ionut-arm Mar 23, 2020

ionut-arm Mar 23, 2020

hug-dev Mar 23, 2020

hug-dev Mar 20, 2020

ionut-arm Mar 23, 2020

hug-dev Mar 23, 2020

hug-dev Mar 20, 2020

ionut-arm Mar 23, 2020

hug-dev Mar 20, 2020

ionut-arm Mar 23, 2020

hug-dev left a comment

hug-dev Mar 23, 2020

hug-dev Mar 23, 2020

hug-dev Mar 23, 2020

ionut-arm commented Mar 23, 2020

hug-dev left a comment

Seed initial client #1

Seed initial client #1

Conversation

ionut-arm commented Mar 20, 2020

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

ionut-arm Mar 23, 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

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

ionut-arm commented Mar 23, 2020

hug-dev left a comment

Choose a reason for hiding this comment

ionut-arm Mar 23, 2020 •

edited

Loading