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
base: master
from

Conversation

Projects
None yet
5 participants
@tniessen
Copy link
Member

tniessen commented Mar 10, 2019

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
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved test/parallel/test-crypto-keygen.js

@panva panva referenced this pull request Mar 11, 2019

Open

OKP Support (EdDSA) #12

@tniessen tniessen force-pushed the tniessen:crypto-add-keygen-for-eddsa branch from 1005c22 to 6730f43 Mar 12, 2019

@tniessen tniessen marked this pull request as ready for review Mar 12, 2019

tniessen added some commits Mar 12, 2019

Show resolved Hide resolved lib/internal/crypto/keygen.js Outdated
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 15, 2019

@sam-github
Copy link
Member

sam-github left a comment

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?

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

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 15, 2019

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

This comment has been minimized.

Copy link
Contributor

panva commented Mar 16, 2019

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

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 17, 2019

@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

This comment has been minimized.

Copy link
Member

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
Copy link
Member

sam-github left a comment

👍

@tniessen

This comment has been minimized.

Copy link
Member Author

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 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

This comment has been minimized.

Copy link
Contributor

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.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 18, 2019

what is your preference?

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

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 18, 2019

@tniessen tniessen referenced this pull request Mar 18, 2019

Closed

crypto: add API to retrieve key type OID #26709

4 of 4 tasks complete
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Mar 18, 2019

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

crypto: add support for EdDSA key pair generation
PR-URL: #26554
Refs: #26319
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.