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 import and export support for ECC for PKCS11 #452

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

ionut-arm
Copy link
Member

This commit adds support for importing and exporting public EC keys in
the PKCS11 provider. E2E tests are also enabled, and cross-provider
tests for ECC keys have been added.

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

@ionut-arm ionut-arm added enhancement New feature or request ecosystem Issues related to building or improving compatibility layers to enhance Parsec's ecosystem position labels Jun 24, 2021
@ionut-arm ionut-arm added this to the Parsec Release 0.8.0 milestone Jun 24, 2021
@ionut-arm ionut-arm requested a review from hug-dev June 24, 2021 15:12
@ionut-arm ionut-arm self-assigned this Jun 24, 2021
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 👏 I have a few questions

e2e_tests/tests/all_providers/cross.rs Outdated Show resolved Hide resolved
src/providers/pkcs11/key_management.rs Outdated Show resolved Hide resolved
src/providers/pkcs11/key_management.rs Outdated Show resolved Hide resolved
src/providers/pkcs11/key_management.rs Show resolved Hide resolved
@ionut-arm ionut-arm force-pushed the imp_exp_ecc_pkcs11 branch 2 times, most recently from c080b22 to 0430875 Compare June 25, 2021 13:04
@ionut-arm
Copy link
Member Author

NOTE: I've had to make some changes to sign and verify too, as it seems using the ASN.1 DigestInfo structure for ECDSA is a no-no, and the token just assumed that was part of the hash as well. I've also enabled more tests to verify these formatting compatibilities.

@ionut-arm ionut-arm marked this pull request as ready for review June 28, 2021 16:15
@ionut-arm
Copy link
Member Author

I've verified this on the Nitrokey HSM2 as well - it doesn't support verification on chip with ECDSA, but signing worked just fine.

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.

Nice and clean ⭐

@@ -32,10 +33,18 @@ impl Provider {
let key = self.find_key(&session, key_id, KeyPairType::PrivateKey)?;
info!("Located signing key.");

let hash = match key_attributes.key_type {
// For RSA signatures we need to format the hash into a DigestInfo structure
Type::RsaKeyPair => utils::digest_info(op.alg, op.hash.to_vec())?.into(),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe off topic, but that reminds me that we don't perform any kind of verification of the hash parameter? That it has the correct length when it is defined, or is already in the DigestInfo form for the Raw version?

Copy link
Member Author

Choose a reason for hiding this comment

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

That should fall under parallaxsecond/parsec-interface-rs#107 , but yes, something we need to consider at some point.

Comment on lines +297 to +301
// The format expected by PKCS11 is an ASN.1 OctetString containing the
// data that the PSA Crypto interface specifies.
// See ECPoint in [SEC1](https://www.secg.org/sec1-v2.pdf). PKCS11 mandates using
// [ANSI X9.62 ECPoint](https://cryptsoft.com/pkcs11doc/v220/group__SEC__12__3__3__ECDSA__PUBLIC__KEY__OBJECTS.html),
// however SEC1 is an equivalent spec.
Copy link
Member

Choose a reason for hiding this comment

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

👌

@ionut-arm ionut-arm force-pushed the imp_exp_ecc_pkcs11 branch 2 times, most recently from 520fb28 to 9b22b95 Compare June 30, 2021 16:36
This commit adds support for importing and exporting public EC keys in
the PKCS11 provider. E2E tests are also enabled, and cross-provider
tests for ECC keys have been added.

Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
@@ -1,14 +1,13 @@
// Copyright 2019 Contributors to the Parsec project.
// SPDX-License-Identifier: Apache-2.0
#[cfg(any(feature = "mbed-crypto-provider", feature = "cryptoauthlib-provider"))]
#![allow(unused_imports, unused)]
Copy link
Member Author

Choose a reason for hiding this comment

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

I've swapped the explicit cfg for features tagged on inports and constants with this because it was starting to be a mess of figuring out what's used where, and trying to align across tens of tests that are supported by different providers.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's totally fine for tests

@ionut-arm ionut-arm merged commit 3dd2ae2 into parallaxsecond:main Jul 1, 2021
@ionut-arm ionut-arm deleted the imp_exp_ecc_pkcs11 branch July 1, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Issues related to building or improving compatibility layers to enhance Parsec's ecosystem position enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants