Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
crypto: add support for EdDSA key pair generation#26554
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Mar 10, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1005c22 to 6730f43CompareUh oh!
There was an error while loading. Please reload this page.
tniessen commented Mar 15, 2019
Ping @nodejs/crypto. |
sam-github left a comment
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?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tniessen commented Mar 15, 2019
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 commented Mar 16, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
tniessen commented Mar 17, 2019
sam-github commented Mar 17, 2019
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. |
sam-github left a comment
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.
👍
tniessen commented Mar 17, 2019
@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? |
panva commented Mar 17, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
sam-github commented Mar 18, 2019
The way you did it, which is why I approved ;-). |
tniessen commented Mar 18, 2019
@sam-github Thank you! :) |
tniessen commented Mar 18, 2019
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