-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
1005c22
to
6730f43
Compare
Ping @nodejs/crypto. |
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.
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. |
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. |
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.
👍
@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 originally intended the Based on the section you mentioned, I think it is also defensible to use separate IDs for the curves:
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? |
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. |
The way you did it, which is why I approved ;-). |
@sam-github Thank you! :) |
Thanks for reviewing, landed in 3a95924. |
PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <[email protected]>
PR-URL: nodejs#26554 Refs: nodejs#26319 Reviewed-By: Sam Roberts <[email protected]>
PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <[email protected]>
This adds support for key pair generation for ed25519 and ed448.
Refs: #26319
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes