Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a Parsec Command Line Interface Client #202

Closed
hug-dev opened this issue Jul 10, 2020 · 14 comments
Closed

Create a Parsec Command Line Interface Client #202

hug-dev opened this issue Jul 10, 2020 · 14 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@hug-dev
Copy link
Member

hug-dev commented Jul 10, 2020

At first it could/should be limited to Core Operations and would be a very nice way to check for administrators/clients that Parsec is running, which providers are currently usable, what operations, etc...

Rust is really easy and fast to make CLI app with (see the guide) and since we already have a Rust Client that should be really nice to work with.

@hug-dev hug-dev added the enhancement New feature or request label Jul 10, 2020
@hug-dev hug-dev added the good first issue Good for newcomers label Jul 10, 2020
@joechrisellis
Copy link
Contributor

I'll take a look at this. :)

@joechrisellis
Copy link
Contributor

Hi there,

Will be submitting a pull request for this shortly. Just thought it might be worth writing down a few bits for some context, or for future reference.

  • CLI binary will be called parsec-tool, which seems to be common among utilities that do a similar sort of thing.
  • AFAIK, parsec-client-rust three core operations -- these are ping, list providers, and list opcodes. The initial implementation of the CLI will support these core operations, with hopefully enough flexibility to keep adding new things as we go.
  • Initial implementation does not yet support authentication, but that can be added in the future.
  • The CLI will live inside the parsec-client-rust repository, under a src/bin directory.

If I remember anything else, or if anything changes, I'll edit this comment. 👍

Cheers!

@ionut-arm
Copy link
Member

Do you have a plan for what the output format will be for the data?

@hug-dev
Copy link
Member Author

hug-dev commented Jul 21, 2020

Adding to Ionut's question, what do you think will be the different commands/options to the CLI?

@joechrisellis
Copy link
Contributor

Hi,

The current prototype implementation has a fairly basic output format. For example, outputting the available providers looks like this:

$ parsec-tool listproviders
[INFO] Available providers:
* MbedCrypto
* Core

It might be possible to output more information -- I'll check and see if this is possible.

@hug-dev, here's the current help screen:

$ parsec-tool -h  
parsec-client 0.7.0
Ionut Mihalcea <ionut.mihalcea@arm.com>, Hugues de Valon <hugues.devalon@arm.com>, Joe Ellis <joe.ellis@arm.com>
Parsec Client library for the Rust ecosystem

USAGE:
    parsec-tool <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    help             Prints this message or the help of the given subcommand(s)
    listopcodes      List the opcodes
    listproviders    List the providers
    ping             Ping the client

Note that some subcommands have additional options -- e.g. listopcodes shows:

$ parsec-tools listopcodes -h
parsec-tool-listopcodes 
List the opcodes

USAGE:
    parsec-tool listopcodes -p <provider>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -p <provider>        The provider to get the opcodes for [possible values: core, mbedcrypto, pkcs11, tpm]

I'm thinking about writing up a more formal RFC for this -- do you guys think that's a good idea before we move forward? 😄

@hug-dev
Copy link
Member Author

hug-dev commented Jul 21, 2020

I'm thinking about writing up a more formal RFC for this -- do you guys think that's a good idea before we move forward? 😄

I think for this it's fine not to do a formal RFC!
I have a few comments on the information you gave:

  • for the commands: I suggest using snake case (eg list_providers, mbed_crypto)
  • what happens if no commands are run? Does it show the help? It would be nice to also do a ping by default!
  • for list_opcodes: could it by default show the supported opcodes of all providers available? Also how do you output the opcodes?
  • for list_providers, it would nice indeed to show all the information that is sent back by the service. Since the providers' list is now prioritised it would be nice to have some visual effect on the most prioritary one 👌

Also about the location of parsec-tool binary: is it definitely best to put inside the parsec-client-rust repo? I am thinking, it might not appear as the "official" tool then. Also if it grows too big, it might not fit the code here.

@joechrisellis
Copy link
Contributor

joechrisellis commented Jul 22, 2020

for the commands: I suggest using snake case (eg list_providers, mbed_crypto)

Sure, will do this. 😄

what happens if no commands are run? Does it show the help? It would be nice to also do a ping by default!

At the moment it throws up the help screen. From having a quick search around, it looks like it might be non-trivial to do this with clap. A 'default subcommand' or 'default behaviour' does not appear to be a clap feature. We could, of course, check for an empty std::env::args(), but that starts to feel a little bit hacky...

for list_opcodes: could it by default show the supported opcodes of all providers available? Also how do you output the opcodes?

Yes, that's a good idea. The opcodes are currently outputted like this:

$ parsec-tool listopcodes -p core
[INFO] Available opcodes:
* ListProviders
* ListOpcodes
* Ping

I'll change this to output the opcodes for each provider, though.

for list_providers, it would nice indeed to show all the information that is sent back by the service. Since the providers' list is now prioritised it would be nice to have some visual effect on the most prioritary one 👌

Agreed, I think I mentioned this in the community call. We can display more information than we do currently. Will make sure that this is implemented.

Also about the location of parsec-tool binary: is it definitely best to put inside the parsec-client-rust repo? I am thinking, it might not appear as the "official" tool then. Also if it grows too big, it might not fit the code here.

You're perhaps right. I'm very much indifferent on where this tool ends up -- if you think it's better to be in its own repo then we can do that! I'm more inclined to think that this is the better approach. 😄

@hug-dev
Copy link
Member Author

hug-dev commented Jul 22, 2020

At the moment it throws up the help screen. From having a quick search around, it looks like it might be non-trivial to do this with clap. A 'default subcommand' or 'default behaviour' does not appear to be a clap feature. We could, of course, check for an empty std::env::args(), but that starts to feel a little bit hacky...

Ok no worries, then let's just keep it with the normal, default behaviour 😃!

The opcodes are currently outputted like this:

Looks good! Would be nice to also show the codes in hexadecimal form.

I'm more inclined to think that this is the better approach. 😄

I think it might also be better. I will create another repository with a default template.

@ionut-arm
Copy link
Member

Just out of curiosity, are there any plans for implementing some of the crypto operations too?

Oh, and what error code is returned when something fails - is it the response status codes defined for the wire protocol or something else?

@joechrisellis
Copy link
Contributor

Just out of curiosity, are there any plans for implementing some of the crypto operations too?

I think @hug-dev is the guy for this question! I'm just guessing, but I would imagine so eventually.

Oh, and what error code is returned when something fails - is it the response status codes defined for the wire protocol or something else?

At the moment it's zero for success, and one for failures. We can change this though.

@ionut-arm
Copy link
Member

At the moment it's zero for success, and one for failures. We can change this though

It would be nice to change the failure code to whatever the value of the ResponseStatus is, if it's an error from the service - maybe someone scripting using the client would want to do different things based on that, without needing to parse the printed text.

@hug-dev
Copy link
Member Author

hug-dev commented Jul 22, 2020

Just out of curiosity, are there any plans for implementing some of the crypto operations too?

We can discuss this, I am not sure of which level of abstraction we want to expose for the CLI: do we want it to access to all operations with all possible parameters? For example, we could build the crypto operations on top of a higher abstraction client.
It depends if the tool is going to be used for admin/developpers/debugging in which case allowing all possible operations will make sense or if it is for users in which case we might want to hide some details and make it simpler to use.

For the Core Operations at least, I have created the parsec-tool repository you can submit a PR against.

About directory structure: in order to use docs.rs documentation (if we want to use it), it would be needed to have modules in src/lib.rs. I think a common paradigm is to keep the main.rs pretty thin, with all the logic implemented in modules public from src/lib.rs. Even if the library crate is not supposed to be used, I think it is useful for developpers to have documentation.

@joechrisellis
Copy link
Contributor

Initial pull request submitted here.

@hug-dev
Copy link
Member Author

hug-dev commented Aug 6, 2020

parallaxsecond/parsec-tool#1 has been merged!

@hug-dev hug-dev closed this as completed Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants