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 secret key generation and key wrapping functions #38

Merged
merged 7 commits into from
Sep 3, 2021

Conversation

wiktor-k
Copy link
Collaborator

@wiktor-k wiktor-k commented Aug 5, 2021

Hi folks 👋

If you don't mind I'll add the wrap/unwrap feature. The end goal is to securely transfer private keys between tokens or between airgapped machine and a token.

I start modestly with wrapper for C_WrapKey. Apparently during work it seems like wrapping private keys with private keys is not possible but among the combinations allowed with the spec is wrapping secret keys with private keys so I also added C_GenerateKey for secret (symmetric) 3DES keys and some tests.

Suggestions welcome!

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@hug-dev hug-dev self-requested a review August 5, 2021 10:52
@hug-dev hug-dev added the enhancement New feature or request label Aug 5, 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.

Looks good, nothing to say! Thanks for the test as well.

Just a general note that we are under a code freeze period until the Parsec release is finalized. That means that we will only be able to merge this PR in a few weeks latest. Would that be ok for you?

About the wrapping feature: I guess that then the key is unwrapped with the RSA private key?

@hug-dev hug-dev requested a review from ionut-arm August 5, 2021 13:29
@wiktor-k
Copy link
Collaborator Author

wiktor-k commented Aug 5, 2021

That means that we will only be able to merge this PR in a few weeks latest. Would that be ok for you?

Yep, that's fine with me but if you don't mind I'd still file the unwrap PR later. Or just keep adding commits to this one? (it shouldn't be that big). It's just if I don't create a PR now I'll probably forget about it :)

About the wrapping feature: I guess that then the key is unwrapped with the RSA private key?

Yes, I was planning to work on that next and the unwrap test would show what's going on (or I'll just extend the wrap test 🤔).

@hug-dev
Copy link
Member

hug-dev commented Aug 6, 2021

Yep, that's fine with me but if you don't mind I'd still file the unwrap PR later. Or just keep adding commits to this one? (it shouldn't be that big). It's just if I don't create a PR now I'll probably forget about it :)

Yes sure adding commits to this one is totally fine! We can do the review now and merge it later 👌

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.

LGTM! 🚀

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k
Copy link
Collaborator Author

wiktor-k commented Aug 9, 2021

I've added the unwrap function and tweaked the integration test to check if the unwrapped key works. This uncovered that I need DES3 key type and a symmetric encryption method. Phew! 😅

@ionut-arm
Copy link
Member

ionut-arm commented Aug 9, 2021

Hmm, I'm wondering if we should consider adding #[deprecated] (docs here) on the DES/3DES stuff, much in the same vein as we did for the PSA Crypto stuff. Although I think that attribute is more about Rust features rather than the abstract stuff it represents.

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 test! I am ok with the changes.

&[1, 2, 3, 4, 5, 6, 7, 8],
)
.unwrap();
assert_eq!(encrypted_with_original, encrypted_with_unwrapped);
Copy link
Member

Choose a reason for hiding this comment

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

That's cool!

@hug-dev
Copy link
Member

hug-dev commented Aug 9, 2021

Hmm, I'm wondering if we should consider adding #[deprecated] (docs here) on the DES/3DES stuff, much in the same vein as we did for the PSA Crypto stuff. Although I think that attribute is more about Rust features rather than the abstract stuff it represents.

Yes, that's a good point... PSA Crypto note it as "strongly deprecated" here 😬
Maybe this vision of "choosing" for the user what is deprecated or not should only be for the Parsec/PSA Crypto related crate?

@wiktor-k
Copy link
Collaborator Author

wiktor-k commented Aug 9, 2021

I'm okay with choosing another good symmetric cipher that's supported by softhsm and some hardware vendor and removing DES3. To be honest I was more interested in the wrap/unwrap and didn't check thoroughly what symmetric ciphers are supported (e.g. Yubikey doesn't list any symmetric algos via pkcs11-tool --list-mechanisms but AET cards do so I need to check out with some sample code).

(And if Wrap worked with two asymmetric keys I wouldn't bother with symmetric ones but apparently this is not supported in the spec 🤷 )

So... all in all it's good that this PR will take some time as we can iron out the details :) Just give me some time to test it all.

For curiosity sake that's what I get for my cards:

$ pkcs11-tool --module /usr/lib/libykcs11.so --list-mechanisms # Yubikey
Using slot 0 with a present token (0x0)
Supported mechanisms:
  RSA-PKCS-KEY-PAIR-GEN, keySize={1024,2048}, hw, generate_key_pair
  RSA-PKCS, keySize={1024,2048}, hw, encrypt, decrypt, sign, verify
  RSA-PKCS-PSS, keySize={1024,2048}, hw, sign, verify
  RSA-PKCS-OAEP, keySize={1024,2048}, hw, encrypt, decrypt
  RSA-X-509, keySize={1024,2048}, hw, encrypt, decrypt, sign, verify
  SHA1-RSA-PKCS, keySize={1024,2048}, hw, sign, verify
  SHA256-RSA-PKCS, keySize={1024,2048}, hw, sign, verify
  SHA384-RSA-PKCS, keySize={1024,2048}, hw, sign, verify
  SHA512-RSA-PKCS, keySize={1024,2048}, hw, sign, verify
  SHA1-RSA-PKCS-PSS, keySize={1024,2048}, hw, sign, verify
  SHA256-RSA-PKCS-PSS, keySize={1024,2048}, hw, sign, verify
  SHA384-RSA-PKCS-PSS, keySize={1024,2048}, hw, sign, verify
  SHA512-RSA-PKCS-PSS, keySize={1024,2048}, hw, sign, verify
  ECDSA-KEY-PAIR-GEN, keySize={256,384}, hw, generate_key_pair
  ECDSA, keySize={256,384}, hw, sign, verify
  ECDSA-SHA1, keySize={256,384}, hw, sign, verify
  ECDSA-SHA224, keySize={256,384}, hw, sign, verify
  ECDSA-SHA256, keySize={256,384}, hw, sign, verify
  ECDSA-SHA384, keySize={256,384}, hw, sign, verify
  ECDH1-DERIVE, keySize={256,384}, hw, derive
  SHA-1, digest
  SHA256, digest
  SHA384, digest
  SHA512, digest
$ pkcs11-tool --module /usr/lib/libaetpkss.so --list-mechanisms # AET card
Using slot 0 with a present token (0xcd01)
Supported mechanisms:
  RSA-PKCS, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  RSA-X-509, keySize={768,1024}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  RSA-PKCS-OAEP, keySize={768,2048}, hw, encrypt, decrypt, wrap, unwrap
  MD5-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  SHA1-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  SHA224-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  SHA256-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  SHA384-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  SHA512-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  RIPEMD160-RSA-PKCS, keySize={768,2048}, hw, sign, verify
  mechtype-0x80000003, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000004, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000005, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000006, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000007, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000008, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  mechtype-0x80000009, keySize={768,2048}, hw, encrypt, decrypt, sign, sign_recover, verify, verify_recover, wrap, unwrap
  SHA1-RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  SHA224-RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  SHA256-RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  SHA384-RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  SHA512-RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  RSA-PKCS-PSS, keySize={768,2048}, hw, sign, verify
  RSA-PKCS-KEY-PAIR-GEN, keySize={768,2048}, hw, generate_key_pair
  RC2-KEY-GEN, keySize={8,1024}, generate
  RC2-ECB, keySize={1,1024}, encrypt, decrypt, wrap, unwrap
  RC2-CBC, keySize={1,1024}, encrypt, decrypt, wrap, unwrap
  RC2-CBC-PAD, keySize={1,1024}, encrypt, decrypt, wrap, unwrap
  RC4-KEY-GEN, keySize={8,2048}, generate
  RC4, keySize={8,2048}, encrypt, decrypt, wrap, unwrap
  DES-KEY-GEN, generate
  DES2-KEY-GEN, generate
  DES3-KEY-GEN, generate
  DES-ECB, encrypt, decrypt, wrap, unwrap
  DES-CBC, encrypt, decrypt, wrap, unwrap
  DES3-ECB, encrypt, decrypt, wrap, unwrap
  DES3-CBC, encrypt, decrypt, wrap, unwrap
  DES-CBC-PAD, encrypt, decrypt, wrap, unwrap
  DES3-CBC-PAD, hw, encrypt, decrypt, wrap, unwrap
  MD5, digest
  SHA-1, digest
  SHA224, digest
  SHA256, digest
  SHA384, digest
  SHA512, digest
  RIPEMD160, digest

@ionut-arm
Copy link
Member

Oh, I was by no means implying we shouldn't support it - the more of the spec we cover the better! It was mostly a note because I remembered we did that in psa-crypto. We could, as @hug-dev was saying, leave it as is since the spec itself does not mark anything as deprecated. It just seems in line with the general goal of pushing users towards safety.

AES would be the usual go-to cipher, but if your tokens don't support it then we can stick with DES. If someone requires AES they can raise an issue or submit a PR.

@wiktor-k
Copy link
Collaborator Author

Understood! 👍 How about I extend the docs for DES and point the document referenced by @hug-dev? (Maybe it's just me but I like to keep everything close to the code). And then I guess this PR is complete (and ready for the next "merge window" ;) ).

@hug-dev
Copy link
Member

hug-dev commented Aug 10, 2021

How about I extend the docs for DES and point the document referenced by @hug-dev?

Yes I think that would be good! Maybe another "official" document would be good instead of the PSA Crypto one. Maybe this NIST document?

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@ionut-arm ionut-arm merged commit 5194254 into parallaxsecond:main Sep 3, 2021
@wiktor-k wiktor-k deleted the add-wrap branch September 3, 2021 08:58
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.

3 participants