crypto: fix EdDSA support for KeyObject by mscdex · Pull Request #26319 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@mscdex sadly an error occured when I tried to trigger a build :(

@nodejs/crypto

Also, this should be backported if/when #26270 lands.

Can you also check that encryption and decryption work? Search this file for publicEncrypt() and privateDecrypt() for examples.

(We really should have tests that check that all different KeyObject types are accepted wherever a KeyObject is accepted - e.g., sign(), verify(), createHmac(), etc.)

edit: oh, I see you opened #26320 for this?

Correct, these changes were easy/straight-forward enough that I decided to submit a PR for them separately.

Looks good.

The pre-existing switch statement default CHECK() makes me wonder if there are other key types that could be smuggled in to cause abort.

CI: https://ci.nodejs.org/job/node-test-pull-request/21046/

Sorry for chiming in late, I've been away for a couple of days.

Ping @mscdex

CI again: https://ci.nodejs.org/job/node-test-pull-request/21250/

Aren't we just "patching" here? In the future if another key manages to pass openssl but isn't prepared this way we'll crash again.

It's hard to know if this catches all key types that are possible, I don't understand enough of the context this is used, or the OpenSSL APIs.

A check of the OpenSSL headers don't make it easy to see what EVP_PKEY_ types we should handle:

node/deps/openssl/openssl/include/openssl/evp.h

Lines 41 to 64 in b22ee71

    
           # define EVP_PKEY_NONE   NID_undef 
        
           # define EVP_PKEY_RSA    NID_rsaEncryption 
        
           # define EVP_PKEY_RSA2   NID_rsa 
        
           # define EVP_PKEY_RSA_PSS NID_rsassaPss 
        
           # define EVP_PKEY_DSA    NID_dsa 
        
           # define EVP_PKEY_DSA1   NID_dsa_2 
        
           # define EVP_PKEY_DSA2   NID_dsaWithSHA 
        
           # define EVP_PKEY_DSA3   NID_dsaWithSHA1 
        
           # define EVP_PKEY_DSA4   NID_dsaWithSHA1_2 
        
           # define EVP_PKEY_DH     NID_dhKeyAgreement 
        
           # define EVP_PKEY_DHX    NID_dhpublicnumber 
        
           # define EVP_PKEY_EC     NID_X9_62_id_ecPublicKey 
        
           # define EVP_PKEY_SM2    NID_sm2 
        
           # define EVP_PKEY_HMAC   NID_hmac 
        
           # define EVP_PKEY_CMAC   NID_cmac 
        
           # define EVP_PKEY_SCRYPT NID_id_scrypt 
        
           # define EVP_PKEY_TLS1_PRF NID_tls1_prf 
        
           # define EVP_PKEY_HKDF   NID_hkdf 
        
           # define EVP_PKEY_POLY1305 NID_poly1305 
        
           # define EVP_PKEY_SIPHASH NID_siphash 
        
           # define EVP_PKEY_X25519 NID_X25519 
        
           # define EVP_PKEY_ED25519 NID_ED25519 
        
           # define EVP_PKEY_X448 NID_X448 
        
           # define EVP_PKEY_ED448 NID_ED448

I can only find one case statement handling EVP_PKEY_ED448, and it could be taken to imply we are missing a couple PKEY types that we should perhaps handle,EVP_PKEY_RSA_PSS, EVP_PKEY_DH, and the three Gost key types:

node/deps/openssl/openssl/crypto/x509/x509type.c

Lines 33 to 59 in b22ee71

    
               case EVP_PKEY_RSA: 
        
                   ret = EVP_PK_RSA | EVP_PKT_SIGN; 
        
           /*              if (!sign only extension) */ 
        
                   ret |= EVP_PKT_ENC; 
        
                   break; 
        
               case EVP_PKEY_RSA_PSS: 
        
                   ret = EVP_PK_RSA | EVP_PKT_SIGN; 
        
                   break; 
        
               case EVP_PKEY_DSA: 
        
                   ret = EVP_PK_DSA | EVP_PKT_SIGN; 
        
                   break; 
        
               case EVP_PKEY_EC: 
        
                   ret = EVP_PK_EC | EVP_PKT_SIGN | EVP_PKT_EXCH; 
        
                   break; 
        
               case EVP_PKEY_ED448: 
        
               case EVP_PKEY_ED25519: 
        
                   ret = EVP_PKT_SIGN; 
        
                   break; 
        
               case EVP_PKEY_DH: 
        
                   ret = EVP_PK_DH | EVP_PKT_EXCH; 
        
                   break; 
        
               case NID_id_GostR3410_2001: 
        
               case NID_id_GostR3410_2012_256: 
        
               case NID_id_GostR3410_2012_512: 
        
                   ret = EVP_PKT_EXCH | EVP_PKT_SIGN; 
        
                   break; 
        
               default:

@mscdex Do you think its worth adding a couple more case statements to match X509_certificate_type(), or is that fn meant to handle a different situtation?

Its fine to leave that for a follow up PR if it takes more research. It is in my TODO list of things to look at, but that list is already large and growing faster than my free time, so no promises.

@sam-github The only reason I'm adding these values is because ed25519/ed448 support was only added in OpenSSL 1.1.1, which was pulled in not that long ago. I do not know about supporting other types.

@mscdex Understood. If anyone else wants to take a crack at figuring out what other types could show up in the default of the case statement, have at it, but I don't think it should block this PR.

@sam-github I can take a look at that later, this PR should be good to land either way as you said.

I just remembered we have most of our keys in test/fixtures/keys and a Makefile that generates them. We are not exactly consistent about that, there are also some keys in test/fixtures, I am not sure whether that was intentional.

Unsurprisingly, ubuntu1604_sharedlibs_openssl110_x64 fails. How long do we need to support that platform? It should be enough to add a preprocessor #if around these cases since they cannot occur on older OpenSSL builds anyway. The test case will need to be adapted, though.

@sam-github Wherever the conversation is had, we do have a team for distributors (@nodejs/distros) and another for embedders (@nodejs/embedders) so maybe pinging those channels. They are both subteams of @nodejs/delivery-channels (which also includes the version management folks, for stuff like nvm and n). The delivery-channels team has no repository, but they do have a discussion board. Unfortunately, that can't be made public (but can be made viewable by anyone in the @nodejs org and that's hundreds of people, so maybe that's a place to start.).

PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs#26554 Refs: nodejs#26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

I think dont-land-on-v11.x is erroneously applied here. The other key types we landed without this label had their CI pass just fine.

I just landed the commit on v11.x-staging.

PR-URL: #26319 Fixes: #26316 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

FWIW this was labeled as dont-land-on-v11.x because as-is it would mean v11.x would not be able to compile against a shared OpenSSL version prior to 1.1.1 and traditionally node has allowed such situations, so unless that policy has changed, currently the next v11.x release will break for anyone compiling it against OpenSSL 1.1.0 for example.

Its pretty easy to add some conditionals on the key type so that it builds against shared openssl 1.1.0, so doing a backport is very possible if someone wants to.

Honestly I'd much rather have all eddsa-related functionality available within a branch (even if it means it's only available in node v12+) because it's easier to think about, rather than having to a bunch of complicated checks.

What do you mean by "branch"?

I'm not concerned about getting eddsa back into 10.x, if 10.x users want new features they can use 12.x.

However, in my experience, small "we won't backport this because we don't need the feature" changes have big ripples, as backports become increasingly difficult because new code has textual (as opposed to depending on eddsa) dependencies on the non-backported code. Eventually, more and more stuff has to be backported that could have cherry-picked cleanly, or almost cleanly.

I'm not going to push hard for this to be backported, but I will propose a backport as soon as we have a PR that doesn't land because this is missing.

Required checks for backporting are an ifdef around the two new case statements, and some of the test keys have to be skipped. I don't think that is complicated.

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 Feb 26, 2019

mscdex force-pushed the crypto-fix-eddsa-support branch from 0c427fd to 79c3aa6 Compare February 26, 2019 11:47

jasnell approved these changes Feb 26, 2019

View reviewed changes

bnoordhuis reviewed Feb 26, 2019

View reviewed changes

sam-github approved these changes Feb 26, 2019

View reviewed changes

bnoordhuis approved these changes Feb 28, 2019

View reviewed changes

sam-github added the lts-watch-v10.x label Feb 28, 2019

tniessen approved these changes Mar 1, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

panva mentioned this pull request Mar 1, 2019

OKP Support (EdDSA) panva/jose#12

Closed

mscdex force-pushed the crypto-fix-eddsa-support branch 2 times, most recently from d1c5ca1 to e40b0ad Compare March 6, 2019 06:41

mscdex force-pushed the crypto-fix-eddsa-support branch from e40b0ad to 8c139e6 Compare March 6, 2019 06:52

ryzokuken approved these changes Mar 6, 2019

View reviewed changes

tniessen added a commit that referenced this pull request Mar 18, 2019

crypto: add support for EdDSA key pair generation

3a95924

PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

This was referenced Mar 19, 2019

crypto: add support for x25119 and x448 key pair generation #26774

Closed

keyobject.asymmetricKeyType crashes on C++ assertion #26775

Closed

targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019

crypto: add support for EdDSA key pair generation

f4144d3

PR-URL: nodejs#26554 Refs: nodejs#26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

panva mentioned this pull request Mar 28, 2019

v11.13.0 release proposal #26949

Merged

tniessen removed the dont-land-on-v11.x label Mar 28, 2019

targos pushed a commit that referenced this pull request Mar 28, 2019

crypto: add support for EdDSA key pair generation

1191cbe

PR-URL: #26554 Refs: #26319 Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

tniessen mentioned this pull request Mar 29, 2019

Supported asymmetric key types #26996

Closed

16 tasks

targos added the backport-requested-v11.x label Mar 30, 2019

crypto: fix EdDSA support for KeyObject by mscdex · Pull Request #26319 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: fix EdDSA support for KeyObject - #26319

crypto: fix EdDSA support for KeyObject#26319mscdex merged 1 commit intonodejs:masternodejs/node:masterfrom mscdex:crypto-fix-eddsa-supportmscdex/io.js:crypto-fix-eddsa-supportCopy head branch name to clipboard

Conversation

mscdex commented Feb 26, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 26, 2019

Uh oh!

targos commented Feb 26, 2019

Uh oh!

mscdex commented Feb 26, 2019

Uh oh!

bnoordhuis Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 28, 2019

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

mscdex commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panva commented Mar 6, 2019

Uh oh!

sam-github commented Mar 6, 2019

Uh oh!

mscdex commented Mar 6, 2019

Uh oh!

sam-github commented Mar 6, 2019

Uh oh!

tniessen commented Mar 6, 2019

Uh oh!

tniessen commented Mar 9, 2019

Uh oh!

tniessen commented Mar 9, 2019

Uh oh!

Trott commented Mar 16, 2019

Uh oh!

panva commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Mar 28, 2019

Uh oh!

mscdex commented Mar 30, 2019

Uh oh!

sam-github commented Apr 2, 2019

Uh oh!

mscdex commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

sam-github commented Apr 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

crypto: fix EdDSA support for KeyObject#26319
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:crypto-fix-eddsa-support

bnoordhuis Feb 26, 2019 •
edited
Loading

mscdex commented Mar 6, 2019 •
edited
Loading

panva commented Mar 28, 2019 •
edited
Loading

mscdex commented Apr 2, 2019 •
edited
Loading