Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library #303

RobertDrazkowskiGL · 2020-12-17T11:40:49Z

Definition of new structures and their interfaces.

Known issues

Version mismatch of psa-crypto

There is a version mismatch in a dependent library (psa-crypto) between parsec-interface and parsec on appropriate master/HEAD versions:

parsec-interface depends on git https://github.com/parallaxsecond/rust-psa-crypto#rev18dd4dda96d8b61d2e112b9e6ad83e90fe768d78
parsec depends on depends on official version 6.0.0 from crates.io

The solution is to

assign a new version to psa-crypto (git/hash) as discussed elsewhere
update the package meta-data on crates.io
use this new version in both parsec-interface and parsec

As a workaround this PR modifies parsec/Cargo.toml to have the same psa-crypto as a parsec-interface.

Wrong parsec-interface version number being used

The parsec-interface extension (add an CryptoAuthLib entry to providers' enumeration) was not followed by package version increase nor the package meta-data on crates.io was updated. This PR addressed an issue with parsec-interface version for parsec compilation purposes but there are still issues with e2e-tests.

Issues with e2e-tests

The e2e-tests depend on parsec-client that depends on parsec-interface and it is affected by the previously mentioned versioning issues.

Summary TODO points

assign a new version to psa-crypto and update crate's meta-data on crates.io
update both parsec and parsec-interface to use this new psa-crypto version
assign a new version to parsec-interface and update its metadata on crates.io
update parsec and parsec-client to use this new parsec-interface

Signed-off-by: Robert Drazkowski Robert.Drazkowski@globallogic.com

ionut-arm

I only had a brief look, but it seems good overall, left just one comment below

ionut-arm · 2020-12-17T11:49:44Z

config.toml

+
+[[provider]]
+provider_type = "CryptoAuthLib"
+key_info_manager = "on-disk-manager"


This one isn't really needed like this since this file is mostly an example/for documentation purposes, it's not used anywhere. You can comment them out and add some documentation comments like for PKCS11 or TPM above.

For actually running the service we generally do cargo run --features ... -- -c e2e_tests/provider_cfg/.../config.toml

Thank you. I will do it.

paulhowardarm · 2020-12-17T13:58:07Z

Thanks for raising the PR - it's really exciting to see this stuff starting to arrive! And this is a perfectly-sized first change as well - just introducing the provider without its functions.

Before we can merge this in, I think we'll need to work a bit more on getting the dependency graph consistent. The CI actions are failing in this PR, and I think it's because there is a lack of consistency between how parsec-interface is being consumed into parsec and e2e_tests respectively. It's worth being aware that e2e_tests is an independent crate, even though its source code is nested within parsec. Like parsec, e2e_tests needs to consume parsec-interface, and it does so in a slightly disguised way, via its resurfacing through parsec-client-rust.

Confusing, I know!

The good news is that I think we can resolve the confusion quite easily by following some basic process. I think we need to create tagged releases of everything in the dependency chain, and then consume those releases in the Cargo.toml files instead of having specific Git references.

If we merge this PR as it is, it will cause the CI to fail, so it's worth sorting out ahead of time.

Starting from the ground up, and taking into account our Slack conversation as well as this PR. My proposal would be as follows, although I would also like to take advice on this from other engineers in the community, since I haven't been involved with the tagging/releasing process to date, and I want to make sure that what I'm proposing here is consistent with established practice.

Create a new tag/release on psa-crypto and set it as 0.6.1 since its functionality doesn't seem to have evolved significantly since the last tag.
Change parsec-interface-rs (via separate PR) to consume psa-crypto at 0.6.1 rather than its current git/hash reference.
Create a new tag/release on parsec-interface-rs and set it as 0.22.0.
Change parsec-client-rust (via separate PR) to consume parsec-interface-rs at its new tag level of 0.22.0.
Create a new tag/release on parsec-client-rust and set it as 0.12.0.
Update this PR with a new commit to consume psa-crypto at 0.6.1, parsec-interface-rs at 0.22.0 and parsec-client-rust at 0.12.0.

This should result in a cleanly-updated and consistent dependency graph, with everything consuming official versions rather than referencing commit hashes or head revisions. Referencing head revisions is a particular problem because it could mean that commits to other repos could actually randomly break things.

RobertDrazkowskiGL · 2020-12-17T14:28:46Z

Yes, I fully agree, I was writing very similar proposal in PR top comment.
Is crates.io automatically updated after package version numbers are increased on github?

paulhowardarm · 2020-12-17T16:28:01Z

I'm not sure how the tagging process links with crates.io. Let me put out a query on the community channel and we'll find out. Ideally the process would be written up somewhere.

paulhowardarm

As discussed in the PR and on the Slack channel, we will need to sanitise the dependency graph a little before this can be merged. We are working on a process for this, but it will involve a few PRs and releases in other repos, so please be patient while these propagate through.

RobertDrazkowskiGL · 2021-01-04T15:53:23Z

I have just pushed a newer version with updated dependencies version numbers as well as corrected config.toml contents as requested by Ionuț.

hug-dev

Thanks for the PR 🚀!

Added a few comments, overall this looks good to me to start with!
You might have to fix some lints on the CI to make it work...

About the issues you had, I guess they would be fixed by now? The e2e-tests crate use a git dependency on the Rust client (at master) which now contains the newest interface.

hug-dev · 2021-01-05T10:53:40Z

src/providers/cryptoauthlib/mod.rs

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


Let's not dwell on the past, I guess this should be 2021 now 😉

hug-dev · 2021-01-05T11:00:11Z

src/providers/cryptoauthlib/mod.rs

+            // Assigned UUID for this provider: b8ba81e2-e9f7-4bdd-b096-a29d0019960c
+            uuid: Uuid::parse_str("b8ba81e2-e9f7-4bdd-b096-a29d0019960c").or(Err(ResponseStatus::InvalidEncoding))?,
+            description: String::from("User space hardware provider, utilizing MicrochipTech CryptoAuthentication Library for ATECCx08 chips"),
+            vendor: String::from("Arm"),


Maybe the vendor could be "Microchip Technology "?

As already mentioned on slack - this is a political decision at the moment it is safe to use "Arm" as a vendor.

hug-dev · 2021-01-05T11:02:26Z

config.toml

+# (Required) ATCA device type.
+#   Supported values: "atecc508a", "atecc608a"
+#device_type = "atecc508a"
+# (Required) Interface for ATCA device
+#   Supported values: "i2c"
+#iface_type = "i2c"
+# (Required) Default wake delay for ATCA device
+#wake_delay = 1500
+# (Required) Default number of rx retries for ATCA device
+#rx_retries = 20
+# (Optional - required for i2c) i2c slave addres
+#slave_address = 0xc0
+# (Optional - required for i2c) i2c bus number
+#bus = 1
+# (Optional - required for i2c) i2c bus baud rate
+#baud = 400000


Since for this PR only the key_info_manager field is required, you could remove that for now and add it later on!

e2e_tests/provider_cfg/cryptoauthlib/Dockerfile

ionut-arm · 2021-01-08T15:44:38Z

src/providers/cryptoauthlib/mod.rs

+    }
+}
+
+impl Provide for Provider {


It looks like the list_keys test is failing because it attempts to list the keys for all providers in the service - thus, including yours because you've added it in the configuration - and that tries to call list_keys on this provider, which fails because it uses the default implementation. I think you can just implement it to always return an empty list for now

Thank you for the hint. I can reproduce this (it just failed for me the same way) so I can proceed.

…ation Library. Definition of new structures and their interfaces. Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

…s requested by a commenter. Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

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

RobertDrazkowskiGL · 2021-01-11T14:38:07Z

@paulhowardarm
You have requested a change, which I believe has already been addressed.
The only 'github' dependency is for "parsec-client' where github is updated but crates.io points on previous version.
I hope this is not an issue.

hug-dev

Thanks!! Looks good to me 👍

The only 'github' dependency is for "parsec-client' where github is updated but crates.io points on previous version. I hope this is not an issue.

To me it's fine as it's only used in the e2e_tests crate!

ionut-arm reviewed Dec 17, 2020

View reviewed changes

paulhowardarm self-requested a review December 17, 2020 13:38

paulhowardarm requested changes Dec 17, 2020

View reviewed changes

RobertDrazkowskiGL force-pushed the calib-integration branch from a75083a to 386dcae Compare January 4, 2021 16:07

hug-dev reviewed Jan 5, 2021

View reviewed changes

ionut-arm added ecosystem Issues related to building or improving compatibility layers to enhance Parsec's ecosystem position enhancement New feature or request labels Jan 5, 2021

RobertDrazkowskiGL force-pushed the calib-integration branch from 386dcae to c95818d Compare January 8, 2021 13:40

ionut-arm reviewed Jan 8, 2021

View reviewed changes

RobertDrazkowskiGL added 5 commits January 11, 2021 14:38

Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentic…

a578e4d

…ation Library. Definition of new structures and their interfaces. Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

Dependencies version numbers updated. Configuration file corrected, a…

6172f2e

…s requested by a commenter. Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

Lint corrections and files' clean-up to address review comments.

cd19cda

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

Providers order corrected to match toml files.

d8bf110

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

Implemented list_keys operation.

54f8aa1

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

RobertDrazkowskiGL force-pushed the calib-integration branch from 327c663 to 54f8aa1 Compare January 11, 2021 13:38

RobertDrazkowskiGL requested a review from paulhowardarm January 11, 2021 14:33

hug-dev approved these changes Jan 11, 2021

View reviewed changes

paulhowardarm approved these changes Jan 11, 2021

View reviewed changes

ionut-arm merged commit c67b25a into parallaxsecond:master Jan 11, 2021

ionut-arm mentioned this pull request Jan 11, 2021

Investigate testing of Cryptoauthlib provider #315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library #303

Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library #303

RobertDrazkowskiGL commented Dec 17, 2020 •

edited

Loading

ionut-arm left a comment

ionut-arm Dec 17, 2020

RobertDrazkowskiGL Dec 17, 2020

paulhowardarm commented Dec 17, 2020

RobertDrazkowskiGL commented Dec 17, 2020

paulhowardarm commented Dec 17, 2020

paulhowardarm left a comment

RobertDrazkowskiGL commented Jan 4, 2021

hug-dev left a comment

hug-dev Jan 5, 2021

RobertDrazkowskiGL Jan 8, 2021

hug-dev Jan 5, 2021

RobertDrazkowskiGL Jan 8, 2021

hug-dev Jan 8, 2021

hug-dev Jan 5, 2021

RobertDrazkowskiGL Jan 8, 2021

ionut-arm Jan 8, 2021 •

edited

Loading

RobertDrazkowskiGL Jan 8, 2021

RobertDrazkowskiGL commented Jan 11, 2021

hug-dev left a comment

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

Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library #303

Add new parsec provider using ATECCx08 cryptochip via CryptoAuthentication Library #303

Conversation

RobertDrazkowskiGL commented Dec 17, 2020 • edited Loading

Known issues

Version mismatch of psa-crypto

Wrong parsec-interface version number being used

Issues with e2e-tests

ionut-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhowardarm commented Dec 17, 2020

RobertDrazkowskiGL commented Dec 17, 2020

paulhowardarm commented Dec 17, 2020

paulhowardarm left a comment

Choose a reason for hiding this comment

RobertDrazkowskiGL commented Jan 4, 2021

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

ionut-arm Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertDrazkowskiGL commented Jan 11, 2021

hug-dev left a comment

Choose a reason for hiding this comment

RobertDrazkowskiGL commented Dec 17, 2020 •

edited

Loading

ionut-arm Jan 8, 2021 •

edited

Loading