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

Add CryptoAuthLib to Provider enumeration. #90

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

RobertDrazkowskiGL
Copy link
Contributor

An interface extension: a new Parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library.

Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello 👋

This looks good to me and I am happy to see you adding a new Parsec provider 🚀 Just added a small comment about the versioning, that I think we can do later.

Just to get a global view of what you are going to add, could you please describe your work a bit more here 😃? As in, what do you plan to do in which repos?

From my understanding, you are going to add a Parsec provider to communicate through a Rust wrapper to the C CryptoAuthLib library? Will it target a specific device or all the devices supported by this library? What are your plans for testing inside Parsec?

Also, if you have one other PR on parallaxsecond/parsec, I think it would be nice to push it now as well, that would help to understand the big picture. From that PR, you can fetch this change with:

parsec-interface = { git = "https://github.com/RobertDrazkowskiGL/parsec-interface-rs", branch = "calib-integration" }

Sorry for all the questions! We will merge this, I am just asking on behalf of the Parsec community to know a bit more about what you are doing 👌

PS: ignore the Travis CI for now, we are going to disable it

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "parsec-interface"
version = "0.21.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now, you can keep the 0.21.0 version and we will use a git dependency on the interface from parsec and the Rust client to get the new commits, something like:

parsec-interface = { git = "https://github.com/parallaxsecond/parsec-interface-rs", rev = "commit-sha" }

Then when we decide to release a new version, we will replace that by the new one!

@RobertDrazkowskiGL
Copy link
Contributor Author

Hello
Thank you for your comment.

Hello 👋

This looks good to me and I am happy to see you adding a new Parsec provider 🚀 Just added a small comment about the versioning, that I think we can do later.

It was the very last thing added and obviously the one not needed at the moment.

Just to get a global view of what you are going to add, could you please describe your work a bit more here 😃? As in, what do you plan to do in which repos?

You are right, I should have done it, I just did not know where is the good place for this kind of information.
First things first:

  1. parsec-interface-rs
    To clean-up all other dependencies, obviously ones in parsec itself (the branch is perfectly fine)
  2. rust-cryptoauthlib (https://github.com/RobertDrazkowskiGL/rust-cryptoauthlib)
    As you wrote, it is the thin and safe Rust wrapper for MicrochipTech CryptoAuthentication Library to use their ATECCx08 cryptochip, loosely based on rust-psa-crypto. Right now it is being cleaned-up before first commit (last minute fixes, the most dangerous ones). Not many callbacks are implemented at the moment, just enough to run tests with real hardware. We'll see (hopefully soon) how many Parsec operation can be offloaded to this hardware. Things may vary between different ATECC devices. It was offline analyzed and at the moment it wraps around the so called "basic interface" from version 3.1.0 of the library.
  3. parsec
    The big one and one being split into human readable pieces. It depends "strongly" on the availability of the hardware i.e. if parsec is built and run with "--features=cryptoauthlib-provider", then the supported must be there, otherwise the constructor fails. I hope this will not be a problem.

From my understanding, you are going to add a Parsec provider to communicate through a Rust wrapper to the C CryptoAuthLib library? Will it target a specific device or all the devices supported by this library? What are your plans for testing inside Parsec?

It very much depends on CAL itself, unfortunately. E.g. there are four interfaces implemented (at least I tried to enable four of them) but they cannot be enabled altogether, the compilation fails. Thus only I2C is compiled in at the moment. Same thing applies to hardware (ATECC chips). At the moment we are sure the ATECC508a and ATECC608a can be compiled in at the same time, and we can call appropriate functions... But MicrochipTech announces ATECC608b should be used in new deployments. We'll see.

Also, if you have one other PR on parallaxsecond/parsec, I think it would be nice to push it now as well, that would help to understand the big picture. From that PR, you can fetch this change with:

parsec-interface = { git = "https://github.com/RobertDrazkowskiGL/parsec-interface-rs", branch = "calib-integration" }

Understood. We (there are more people involved) will use it to clean the dependencies. As the whole parsec thing is being split, some initial pieces will come soon.

Sorry for all the questions! We will merge this, I am just asking on behalf of the Parsec community to know a bit more about what you are doing 👌

Then please let me use this as an opportunity to ask my very specific question. Should I simply amend the latest commit by removing the "bumped" version from Cargo.toml?
Thanks
Robert

PS: ignore the Travis CI for now, we are going to disable it

@hug-dev
Copy link
Member

hug-dev commented Dec 14, 2020

Thanks for your response!

I just did not know where is the good place for this kind of information

That's a good point I will note down. As you are really the first contributor adding a Parsec provider, we do not have any special process for this, but we should.
The split you described makes sense and it is also nice that as a result of this, there would be a rust-cryptoauthlib crate available to the community 👍

if parsec is built and run with "--features=cryptoauthlib-provider", then the supported must be there, otherwise the constructor fails. I hope this will not be a problem.

Yep, that's fine! We are doing things similarly with our other providers.

It very much depends on CAL itself, unfortunately. E.g. there are four interfaces implemented (at least I tried to enable four of them) but they cannot be enabled altogether, the compilation fails. Thus only I2C is compiled in at the moment. Same thing applies to hardware (ATECC chips). At the moment we are sure the ATECC508a and ATECC608a can be compiled in at the same time, and we can call appropriate functions... But MicrochipTech announces ATECC608b should be used in new deployments. We'll see.

Thanks for the explanation!

Should I simply amend the latest commit by removing the "bumped" version from Cargo.toml?

Yes absolutely, that would do it!

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Actually, I think it's fine to just merge it.

Copy link
Collaborator

@paulhowardarm paulhowardarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Good to see the first step towards a new provider!

@hug-dev hug-dev merged commit de4cbe6 into parallaxsecond:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants