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 fixes to cryptoauthlib to support Java PKCS11 requirements, to support Greengrass V2 #290

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

JamieHunter
Copy link
Contributor

Please describe the purpose of this pull request

This PR fixes a number of issues that prevents Greengrass V2 using cryptoauthlib as a PKCS11 HSM provider. Note that for anyone browsing this PR, this also depends on a pending PR to add PKCS11 EC support to aws-c-io.

Greengrass V2 uses Java, which demands certain functionality of compliant PKCS11 libraries. In particular:

  1. Java honors the maximum session limits, and needs at least two sessions (really three when taking into account aws-c-io).
  2. Java requires the private key and certificate to have the same ID, which requires ID support
  3. Java queries certain attributes such as CKA_EXTRACTABLE, which are supposed to be visible even on a private key

Additionally aws-c-io takes advantage of the ability to call C_Sign to determine size of buffer. This code path was unimplemented.

Checklist

…fill pulSignatureLen with required buffer length.
…s of a private key.

Provider library is not expected to fail with CKR_ATTRIBUTE_SENSITIVE.
…ast 2 sessions.

When used with Greengrass V2, at least 3 sessions are needed.
static CK_RV pkcs11_config_parse_object(pkcs11_slot_ctx_ptr slot_ctx, char* cfgstr)
{
char * argv[5];
int argc = (int)sizeof(argv);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, fixed bug here, as argc = (int)sizeof(argv) sets 6sizeof(char) and not 6.

@@ -428,8 +428,8 @@ CK_RV pkcs11_token_get_info(CK_SLOT_ID slotID, CK_TOKEN_INFO_PTR pInfo)
pInfo->ulMinPinLen = 0;
pInfo->flags = CKF_RNG;// | CKF_LOGIN_REQUIRED;

pInfo->ulMaxSessionCount = 1;
pInfo->ulMaxRwSessionCount = 1;
pInfo->ulMaxSessionCount = PKCS11_MAX_SESSIONS_ALLOWED;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on bug reports, there is risk that setting this to PKCS11_MAX_SESSIONS_ALLOWED might break something?

@bryan-hunt
Copy link
Contributor

CKA_ID needs to match the subject key id in the certificate

@JamieHunter
Copy link
Contributor Author

CKA_ID needs to match the subject key id in the certificate

Hi Bryan, per v2.4 spec, I read that it's recommended for interoperability, but "Cryptoki does not enforce this association, or even the uniqueness of the key identifier for a given subject". I don't think there's any possible way to enforce this in code, but maybe could be called out in documentation as a recommended practice? Conversely not having a mechanism to set the ID in the config precludes this library working with Java. Of course, I'm open to other suggestions.

@JamieHunter
Copy link
Contributor Author

I have backed out the ID support from this PR

@bryan-hunt bryan-hunt merged commit c6e9d55 into MicrochipTech:main Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants