-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
2c1015f
to
0a7ea1a
Compare
There was a problem hiding this 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
c080b22
to
0430875
Compare
NOTE: I've had to make some changes to |
0430875
to
a852f69
Compare
I've verified this on the Nitrokey HSM2 as well - it doesn't support verification on chip with ECDSA, but signing worked just fine. |
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
520fb28
to
9b22b95
Compare
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>
9b22b95
to
4d2bed5
Compare
@@ -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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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