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

crypto: add support for EdDSA key pair generation #26554

Closed
wants to merge 6 commits into from

Conversation

tniessen
Copy link
Member

This adds support for key pair generation for ed25519 and ed448.

Refs: #26319

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 10, 2019
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-crypto-keygen.js Show resolved Hide resolved
@tniessen tniessen force-pushed the crypto-add-keygen-for-eddsa branch from 1005c22 to 6730f43 Compare March 12, 2019 15:57
@tniessen tniessen marked this pull request as ready for review March 12, 2019 15:58
@tniessen
Copy link
Member Author

Ping @nodejs/crypto.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

The creation of two entirely new key types for 2 specific EC curves makes me nervous. It looks like this is just mirroring OpenSSL's API choice, but I'd like some time to research this. Do you have any idea why these aren't just EVP_PKEY_EC keys?

doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/keygen.js Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@tniessen
Copy link
Member Author

The creation of two entirely new key types for 2 specific EC curves makes me nervous. It looks like this is just mirroring OpenSSL's API choice, but I'd like some time to research this. Do you have any idea why these aren't just EVP_PKEY_EC keys?

I don't know why OpenSSL did it, but to me, it makes sense, since their ASN.1 encoding is different from EC keys and EdDSA curves have a different OID. I think it is similar to RSA vs RSA-PSS (which I plan on adding support for), which are technically both RSA, except that one is encoded differently and contains additional information / restrictions.

@panva
Copy link
Member

panva commented Mar 16, 2019

@tniessen you already did PSS support 2 years ago in #11705

@tniessen
Copy link
Member Author

@panva Yes, #11705 is indeed a working implementation of RSASSA-PSS. RSASSA-PSS works with regular RSA keys, but there is also a separate key type (1.2.840.113549.1.1.10) for RSASSA-PSS which node does not fully support yet.

@sam-github
Copy link
Contributor

https://mta.openssl.org/pipermail/openssl-users/2019-March/010099.html and preceeding emails are informative.

I suspect that 'EdDSA' would be a more reasonable name for the 'type' of this key, with the specific curve name being supplied as a parameter to generateKeys ('ed25519' or 'ed448'), cf. https://tools.ietf.org/html/rfc8410#section-8.

But our API convention follows OpenSSL, so a 'type' X is a 1-1 map to a EVP_PKEY_X, so I understand the naming used here.

Interestingly, the type is close to unused in our API, other than to inform generateKeyPair() what form the options will be. I guess once/if we have a .fields object for keys, then .type would describe the format of the .fields object.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

👍

@tniessen
Copy link
Member Author

@sam-github Thanks for researching this further. I read the emails on the mailing list and I still think that separating EdDSA keys from EC keys makes sense.

I suspect that 'EdDSA' would be a more reasonable name for the 'type' of this key, with the specific curve name being supplied as a parameter to generateKeys ('ed25519' or 'ed448'), cf. tools.ietf.org/html/rfc8410#section-8.

I originally intended the type argument of generateKeyPair to always equal the asymmetricKeyType property, so #26319 kind of affected me there. If we decide to use EdDSA here, we should probably change that property as well.

Based on the section you mentioned, I think it is also defensible to use separate IDs for the curves:

Use the string "EdDSA" when referring to a signing public key or signature when the curve is not known or relevant.

When the curve is known, use a more specific string. For the id-Ed25519 value use the string "Ed25519". For id-Ed448, use "Ed448".

I am not sure whether the curve is relevant or not, and thus not sure which way is better. What do you think after researching this? You already mentioned that you understand why I decided to do it this way, but what is your preference?

@panva
Copy link
Member

panva commented Mar 17, 2019

I am not sure whether the curve is relevant or not

For EdDSA in regards to JOSE

Json Object Encryption - (ECDH-ES) knowing the curve controls which of the X functions needs to be used. #26626

Json Object Signing - the curve or oid tells me the length of the key and therefore i can transform the der formatted r|s signature to the jose format JWS expects, not sure if this is needed for EdDSA tho.

The JWA algorithm name is always called EdDSA and which curve to use is clear from the used key.

@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 17, 2019
@sam-github
Copy link
Contributor

what is your preference?

The way you did it, which is why I approved ;-).

@tniessen
Copy link
Member Author

Thanks for reviewing, landed in 3a95924.

@tniessen tniessen closed this Mar 18, 2019
tniessen added a commit that referenced this pull request Mar 18, 2019
PR-URL: #26554
Refs: #26319
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26554
Refs: nodejs#26319
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
targos pushed a commit that referenced this pull request Mar 28, 2019
PR-URL: #26554
Refs: #26319
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@tniessen tniessen mentioned this pull request Mar 29, 2019
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
6 participants