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: fix EdDSA support for KeyObject #26319

Merged
merged 1 commit into from Mar 12, 2019

Conversation

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2019

Fixes: #26316

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Feb 26, 2019

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

@mscdex mscdex force-pushed the mscdex:crypto-fix-eddsa-support branch from 0c427fd to 79c3aa6 Feb 26, 2019

@targos

This comment has been minimized.

Copy link
Member

targos commented Feb 26, 2019

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Feb 26, 2019

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

assert.strictEqual(key.export(exportOptions), info.public);
});
}
});

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 26, 2019

Member

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?

This comment has been minimized.

@mscdex

mscdex Feb 27, 2019

Author Contributor

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

@sam-github
Copy link
Member

sam-github left a comment

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.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Feb 28, 2019

@tniessen
Copy link
Member

tniessen left a comment

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

Show resolved Hide resolved src/node_crypto.cc

@panva panva referenced this pull request Mar 1, 2019

Open

OKP Support (EdDSA) #12

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 5, 2019

Ping @mscdex

@mscdex mscdex force-pushed the mscdex:crypto-fix-eddsa-support branch 2 times, most recently from d1c5ca1 to e40b0ad Mar 6, 2019

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 6, 2019

@mscdex mscdex force-pushed the mscdex:crypto-fix-eddsa-support branch from e40b0ad to 8c139e6 Mar 6, 2019

@panva

This comment has been minimized.

Copy link
Contributor

panva commented Mar 6, 2019

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.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 6, 2019

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:

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

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.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 6, 2019

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

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 6, 2019

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

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Mar 6, 2019

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

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Mar 9, 2019

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.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Mar 9, 2019

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.

tniessen added a commit to tniessen/node that referenced this pull request Mar 9, 2019

tniessen added a commit to tniessen/node that referenced this pull request Mar 10, 2019

tniessen added a commit to tniessen/node that referenced this pull request Mar 10, 2019

@tniessen

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 11, 2019

In the past, we did (apparently) decide to allow building against ABI compatible versions of the system OpenSSL. The TLS1.3 won't build against OpenSSL 1.1.0, though, so nodejs/master aka 12.x won't support older than its bundled OpenSSL 1.1.1. This PR should be fine (and should pass if rebuilt).

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Mar 11, 2019

Thanks @sam-github, CI is green.

crypto: fix EdDSA support for KeyObject
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>

@mscdex mscdex force-pushed the mscdex:crypto-fix-eddsa-support branch from 8c139e6 to 247c14c Mar 12, 2019

@mscdex mscdex merged commit 247c14c into nodejs:master Mar 12, 2019

@mscdex mscdex deleted the mscdex:crypto-fix-eddsa-support branch Mar 12, 2019

tniessen added a commit to tniessen/node that referenced this pull request Mar 12, 2019

BridgeAR added a commit that referenced this pull request Mar 13, 2019

crypto: fix EdDSA support for KeyObject
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>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

crypto: fix EdDSA support for KeyObject
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>
@refack

This comment has been minimized.

Copy link
Member

refack commented Mar 14, 2019

In the past, we did (apparently) decide to allow building against ABI compatible versions of the system OpenSSL. The TLS1.3 won't build against OpenSSL 1.1.0, though, so nodejs/master aka 12.x won't support older than its bundled OpenSSL 1.1.1. This PR should be fine (and should pass if rebuilt).

IIUC due to this, this PR can't be backported?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 14, 2019

@nodejs/crypto this is currently added to the release proposal. It would be great to get a quick update if this should indeed be excluded.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 14, 2019

@refack it seems like this builds against OpenSSL 1.0.2? So I am surprised that it fails on 1.1.0. But I'll exclude it and see if the build passes afterwards.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 14, 2019

This PR should be backported if and when OpenSSL 1.1.1 gets backported to v10.x as I noted earlier. I believe that's why it was tagged with lts-watch-v10.x.

v11.x already has OpenSSL 1.1.1 merged so there should be no reason for this to not land there as well....

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 14, 2019

It did to build on the CI twice for v11 and the specific build passed without these commits. That's why I backed it out now and to prevent delaying it further, I won't pull it in again for v11.12.0. I guess we should investigate this again for the release afterwards.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

mscdex commented Mar 14, 2019

@BridgeAR do you have a link to the failures?

EDIT: I assume this is solely about compatibility with shared OpenSSL 1.1.0?

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Mar 15, 2019

Yes, this won't build against openssl 1.1.0, which 10.x allows, and I assume most continue to allow.

commit-linux-containered/out/Release/obj.target/node_lib/src/node_report.o.d.raw   -c
../src/node_crypto.cc: In member function 'v8::Local<v8::String> node::crypto::KeyObject::GetAsymmetricKeyType() const':
../src/node_crypto.cc:3437:8: error: 'EVP_PKEY_ED25519' was not declared in this scope
   case EVP_PKEY_ED25519:
        ^
../src/node_crypto.cc:3439:8: error: 'EVP_PKEY_ED448' was not declared in this scope
   case EVP_PKEY_ED448:

Its possible to ifdef this, in which case if people build 10.x against 1.1.0 they will get less features, and the docs will be wrong.

I'm not totally sure how that will fly, but given that TLS1.3 is in the same boat, it isn't supported by openssl 1.1.0 either, this feature isn't alone. 10.x might collect more features over time that don't work (EDIT: don't work when node is built against openssl 1.1.0 instead of 1.1.1).

This means we really, really need to make sure that downstream consumers of 10.x understand that even though it continues to build against a system openssl 1.1.0, node will not be fully functional.

Its enough of a foot gun that maybe we should just flatout remove the ability to build against 1.1.0 once openssl 1.1.1 lands in 10.x, #26270, or once TLS1.3 is backported to 10.x. I can see why linux distributors or node embedders want to provide their own openssl libs, but I don't know if they require the ability to provide one that is too old to support node's documented API.

/cc @nodejs/crypto @nodejs/tsc Where is a good place to discuss the above ----^ ?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 16, 2019

@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.).

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.