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 a PKCS 11 provider #68

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Add a PKCS 11 provider #68

merged 1 commit into from
Nov 19, 2019

Conversation

hug-dev
Copy link
Member

@hug-dev hug-dev commented Nov 15, 2019

This provider will interface with a PKCS 11 dynamic library to
communicate with a PKCS 11 device.
Add PKCS 11 configuration for the user to enter the path of the dynamic
PKCS 11 library to load, the slot number where the device is and the
user pin of that device.
Removes provider-dependant information in the tests: they shoudl be
generic no matter what provider is currently used.
The PKCS 11 provider currently only supports RSA Key pair generation,
importing and exporting public keys, signing and verifying with those
RSA keys.

Co-authored-by: Paul Howard [email protected]
Signed-off-by: Hugues de Valon [email protected]

@hug-dev hug-dev added the enhancement New feature or request label Nov 15, 2019
@hug-dev hug-dev self-assigned this Nov 15, 2019
@hug-dev
Copy link
Member Author

hug-dev commented Nov 15, 2019

Depends on parallaxsecond/parsec-client-test#13 and parallaxsecond/parsec-interface-rs#8

When those two PRs are merged, I will need to push the Cargo.lock file as well.

@hug-dev
Copy link
Member Author

hug-dev commented Nov 15, 2019

Has been tested using all of our tests (normal integration tests, persistence integration tests and stress tests) using the SoftHSMv2 library and the Nitrokey HSM with the OpenSC library.
I am going to create a GitHub issue to test PKCS 11 provider on the CI.

@hug-dev hug-dev mentioned this pull request Nov 15, 2019
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.

Nice! I'll have to spend more time to understand all of this, but looks good!

|| op.key_attributes.algorithm != Algorithm::sign(SignAlgorithm::RsaPkcs1v15Sign, None)
{
error!("The PKCS 11 provider currently only supports creating RSA key pairs for signing and verifying.");
return Err(ResponseStatus::UnsupportedOperation);
Copy link
Member

Choose a reason for hiding this comment

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

Would ResponseStatus::PsaErrorInvalidArgument work better?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a valid option as well. However, those arguments would be valid for another provider for example, it is just that this one does not support them.

Err(ResponseStatus::PsaErrorHardwareFailure)
}
}
.or_else(|err| {
Copy link
Member

Choose a reason for hiding this comment

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

Why not do the processing from this closure in the block after Err(e) on line 559?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would be less convoluted. Will change here and elsewhere if I do the same.

priv_template
.push(CK_ATTRIBUTE::new(pkcs11::types::CKA_TOKEN).with_bool(&pkcs11::types::CK_TRUE));

pub_template
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to pin the public exponent by setting CKA_PUBLIC_EXPONENT (preferably to 65537)? e.g. mbed crypto does it https://github.com/ARMmbed/mbed-crypto/blob/150d5777802834f0b7084fad95b32ebbc1e75d16/include/psa/crypto.h#L3728

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I checked that the implementation I was working on (SoftHSM) does that by default but it would be more correct indeed to fix it to 0x010001. Will change.


let key = self
.find_key(session.session_handle(), key_id, KeyPairType::PublicKey)
.or_else(|err| {
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? You have logging messages before most failures in find_key (apart from line 449), don't think there's a need to log something here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

True!

{
Ok(_) => Ok(ResultAsymVerify {}),
Err(e) => match e {
pkcs11::errors::Error::Pkcs11(ck_rv) if ck_rv == CKR_SIGNATURE_INVALID => {
Copy link
Member

Choose a reason for hiding this comment

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

pkcs11::errors::Error::Pkcs11(CKR_SIGNATURE_INVALID) => { ...

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 tried that before but got an error but now it seems that it works 😕 Will change!

@@ -19,17 +19,23 @@ mod tests {
use parsec_interface::requests::Result;
use std::collections::HashSet;

//TODO: put those two first tests in a separate target which is executed with an
Copy link
Member

Choose a reason for hiding this comment

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

❤️


let mut template: Vec<CK_ATTRIBUTE> = Vec::new();

let key_test: RsaPublicKey = serde_asn1_der::from_bytes(&op.key_data).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this isn't a "test" key here

This provider will interface with a PKCS 11 dynamic library to
communicate with a PKCS 11 device.
Add PKCS 11 configuration for the user to enter the path of the dynamic
PKCS 11 library to load, the slot number where the device is and the
user pin of that device.
Removes provider-dependant information in the tests: they shoudl be
generic no matter what provider is currently used.
The PKCS 11 provider currently only supports RSA Key pair generation,
importing and exporting public keys, signing and verifying with those
RSA keys.

Co-authored-by: Paul Howard <[email protected]>
Signed-off-by: Hugues de Valon <[email protected]>
@hug-dev hug-dev merged commit f8c60c0 into parallaxsecond:master Nov 19, 2019
@hug-dev hug-dev deleted the pkcs11 branch November 19, 2019 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants