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

PSA_IMPORT_KEY introduction. #399

Merged

Conversation

RobertDrazkowskiGL
Copy link
Contributor

The next operation, with dedicated tests. The largest changes are in tests, the implementation is pretty straightforward.

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

…d e2e-tests.

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>
@hug-dev hug-dev added the platforms Compatibility with different secure services or hardware platforms label Apr 20, 2021
@hug-dev hug-dev self-requested a review April 20, 2021 10:49
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 for adding support for importing keys! Left some comments.

e2e_tests/src/lib.rs Outdated Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/key_operations.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/key_slot.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/mod.rs Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
A new function was added to simplify importing ECC keys.

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
e2e_tests/src/lib.rs Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/key_management.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/key_operations.rs Show resolved Hide resolved
src/providers/cryptoauthlib/key_operations.rs Outdated Show resolved Hide resolved
src/providers/cryptoauthlib/mod.rs Show resolved Hide resolved
e2e_tests/tests/per_provider/normal_tests/import_key.rs Outdated Show resolved Hide resolved
…DER encoded format.

Few tests adaptations.

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 a lot, really great to see the test also working on the Mbed Crypto provider that will help us a lot 👌
Just added one more request and then I am fine on my side.
I will let @ionut-arm give his review also, so that you don't have to push too many time!

if !client.is_operation_supported(Opcode::PsaImportKey) {
return Ok(());
}

client.generate_ecc_key_pair_secpr1_ecdsa_sha256(key_name.clone())?;
let status = client
.import_ecc_public_secp_r1_ecdsa_sha256_key(key_name, KEY_PAIR_ECC.to_vec())
.import_ecc_public_secp_r1_ecdsa_sha256_key(key_name, ECC_KEY_PAIR.to_vec())
Copy link
Member

Choose a reason for hiding this comment

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

I guess it makes no difference here as we check if the key already exists (so that the code does not go as far as parsing it), but I think it should be ECC_PUBLIC_KEY.

// Things do never get better...
// Things never get better...
Copy link
Member

Choose a reason for hiding this comment

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

Some improvement there 😄

@@ -406,5 +416,5 @@ fn import_ecc_private_key() {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Can you also execute this test on the Mbed Crypto provider 😄? That way we will be sure that both the formats for the public key and key pair are correct 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. And it looks like Mbed Crypto uses just the raw private key octet string from the whole structure.

@hug-dev hug-dev requested a review from ionut-arm April 29, 2021 09:54
Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Thanks for the updates - I left a few comments below, overall it looks good

socket_path = "/tmp/parsec.sock"

[authenticator]
auth_type = "Direct"
Copy link
Member

Choose a reason for hiding this comment

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

Are these files supposed to be used for manual testing/playing around with the service locally, or do you intend to present them to admins as a starting place for configuring the service with an ATECC device? If it's the former then you can disregard this comment, but otherwise I'd say auth_type = "UnixPeerCredentials" is a better choice.

Copy link
Contributor Author

@RobertDrazkowskiGL RobertDrazkowskiGL Apr 29, 2021

Choose a reason for hiding this comment

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

So far, we do not have such far-reaching plans. These files are here to make our life easier when we run tests.
And the e2e-tests depend on auth_type = "Direct", therefore I recommend to leave it that way.

@@ -238,6 +320,7 @@ fn check_format_import3() -> Result<()> {
Ok(())
}

#[cfg(not(feature = "cryptoauthlib-provider"))]
#[test]
fn failed_imported_key_should_be_removed() -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of skipping the test completely, any chance you could make public_key and attributes conditional and have it try to import an ECC key for CALib? Or even just modifying the last import call to succeed in the CAL provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>
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!

@hug-dev hug-dev merged commit 1c51f26 into parallaxsecond:main May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platforms Compatibility with different secure services or hardware platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants