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: modernize DH/ECDH/ECDH-ES #31178

Open
wants to merge 5 commits into
base: master
from

Conversation

@tniessen
Copy link
Member

tniessen commented Jan 3, 2020

This adds support for DH/ECDH/ECDH-ES via the KeyObject API, and should fix #26626. I also added DH support to generateKeyPair, which is a partial solution to #28404. There are still lots of things I need to figure out, but I would like to see if people are okay with this approach.

(Note that the current API does not support "raw" DH keys, only SPKI/PKCS#8 keys are supported as of now. That will likely change via the previously discussed .params / .fields APIs.)

cc @nodejs/crypto @nodejs/security

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
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from e394dd5 to 6261781 Jan 3, 2020
Copy link
Member

sam-github left a comment

LGTM in principle, some comments on the WIP.

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/diffiehellman.js Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
src/node_crypto_groups.h Show resolved Hide resolved
@panva

This comment has been minimized.

Copy link
Contributor

panva commented Jan 3, 2020

I can confirm the missing ECDH-ES JWA algorithm support for x25519 and x448 keys is solved with this (closes #26626).

@tniessen would it be possible to split the change to

  1. add the diffieHellman method only (accepting only current 'ec', 'x25519' and 'x448' key objects)
  2. add the dh KeyObject support in a follow up PR?

Reason I ask is i'd like to see 1) backported to lts/erbium and in order to do that It would likely be easier if the change was as simple as possible.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Jan 3, 2020

@panva Since either the whole PR, or a subset, are semver-minor, there isn't anything blocking backporting to LTS.

@panva

This comment has been minimized.

Copy link
Contributor

panva commented Jan 3, 2020

Great.

Copy link
Member

bnoordhuis left a comment

Overall direction seems okay to me.

src/node_crypto.cc Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from 6261781 to 0dfa9c5 Jan 4, 2020
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 4, 2020

Thank you for the initial round of reviews, I'll try to finish up within the next few days.

Overall direction seems okay to me.

I would happily accept alternative directions! This is the best I came up with so far :)

doc/api/crypto.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch 4 times, most recently from 27d73df to 78043cc Jan 4, 2020
Copy link
Member

lundibundi left a comment

Few nits regarding error handling.

lib/internal/crypto/keygen.js Show resolved Hide resolved
lib/internal/crypto/keygen.js Show resolved Hide resolved
lib/internal/crypto/keygen.js Outdated Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from 78043cc to eb347a5 Jan 5, 2020
@tniessen tniessen mentioned this pull request Jan 8, 2020
8 of 22 tasks complete
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from eb347a5 to bc309fb Jan 8, 2020
@tniessen tniessen marked this pull request as ready for review Jan 8, 2020
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from e98c950 to 4378c9d Jan 11, 2020
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 11, 2020

@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from 4378c9d to 70dc715 Jan 11, 2020
Copy link
Member

BridgeAR left a comment

The JS code is LGTM

lib/internal/crypto/keygen.js Show resolved Hide resolved
test/parallel/test-crypto-dh-stateless.js Outdated Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch 2 times, most recently from 62197f1 to a962a3d Jan 12, 2020
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 18, 2020

src/node_crypto.cc Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from a962a3d to 8aab99c Jan 19, 2020
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 19, 2020

Lots of infrastructure failures, but this error is related and concerning, especially since it disappeared after resuming CI:

out/Release/node[60615]: ../src/node_crypto.cc:7218:
node::AllocatedBuffer node::crypto::StatelessDiffieHellman(node::Environment*, node::crypto::ManagedEVPPKey, node::crypto::ManagedEVPPKey):
Assertion `(out_size) == (result.size())' failed.

Stress test:

  • OK: 990 NOT OK: 10 TOTAL: 1000 (alpine-latest-x64)
  • OK: 992 NOT OK: 8 TOTAL: 1000 (rhel7-s390x)

The problem is that OpenSSL sometimes returns a shorter secret key than the implementation expects. Should be fixed now :)

@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from 336d8e2 to 40dd86d Jan 20, 2020
@nodejs-github-bot

This comment was marked as outdated.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 20, 2020

Thanks @sam-github! Apparently, rem_size is a reserved name under AIX, fixing that now...

tniessen added 4 commits Dec 29, 2019
The new key type 'dh' corresponds to EVP_PKEY_DH.
This allows using the generateKeyPair API for DH instead of the old
stateful DH APIs.
Currently, Node.js has separate (stateful) APIs for DH/ECDH, and no
support for ECDH-ES. This commit adds a single stateless function to
compute the DH/ECDH/ECDH-ES secret based on two KeyObjects.
@tniessen tniessen force-pushed the tniessen:crypto-modernize-dh branch from 40dd86d to d7a6917 Jan 20, 2020
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

test-crypto-keygen and test-crypto-dh-stateless are currently flaky
on ARM CI systems due to their slow CPUs.
@nodejs-github-bot

This comment was marked as outdated.

@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 20, 2020

AIX is happy now, but the tests are timing out on our slow ARM machines. Not sure what the best course of action is, so I temporarily disabled the two test files on ARM systems.

@nodejs-github-bot

This comment has been minimized.

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.