GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — crypto: modernize DH/ECDH/ECDH-ES by tniessen · Pull Request #31178 · nodejs/node

Conversation

@tniessen

@tniessen tniessen commented Jan 3, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, cryptococo82, panva, and DuBistKomisch reacted with thumbs up emoji
All reactions
  • 👍 4 reactions
@tniessen tniessen added wip Issues and PRs that are still a work in progress. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Jan 3, 2020
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Jan 3, 2020
BridgeAR
BridgeAR reviewed Jan 3, 2020
View reviewed changes
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from e394dd5 to 6261781 Compare January 3, 2020 15:46
sam-github
sam-github reviewed Jan 3, 2020
View reviewed changes

@sam-github sam-github left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM in principle, some comments on the WIP.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/crypto/diffiehellman.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto_groups.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@panva

panva commented Jan 3, 2020

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sam-github

sam-github commented Jan 3, 2020

Copy link
Copy Markdown
Contributor

@panva Since either the whole PR, or a subset, are semver-minor, there isn't anything blocking backporting to LTS.
tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva

panva commented Jan 3, 2020

Copy link
Copy Markdown
Member

Great.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Jan 4, 2020
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

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.

Overall direction seems okay to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from 6261781 to 0dfa9c5 Compare January 4, 2020 14:41
@tniessen

tniessen commented Jan 4, 2020

Copy link
Copy Markdown
Member Author

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Jan 4, 2020
View reviewed changes
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch 4 times, most recently from 27d73df to 78043cc Compare January 5, 2020 01:35
lundibundi
lundibundi reviewed Jan 5, 2020
View reviewed changes

@lundibundi lundibundi left a comment

Copy link
Copy Markdown
Member

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.

Few nits regarding error handling.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/crypto/keygen.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/crypto/keygen.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/crypto/keygen.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from 78043cc to eb347a5 Compare January 5, 2020 17:36
@tniessen tniessen mentioned this pull request Jan 8, 2020
Supported asymmetric key types #26996
Closed
9 of 16 tasks
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from eb347a5 to bc309fb Compare January 8, 2020 14:22
@tniessen
tniessen marked this pull request as ready for review January 8, 2020 14:27
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jan 9, 2020
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from e98c950 to 4378c9d Compare January 11, 2020 20:19
@tniessen

tniessen commented Jan 11, 2020

Copy link
Copy Markdown
Member Author

@nodejs/crypto PTAL.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch from 4378c9d to 70dc715 Compare January 11, 2020 20:33
BridgeAR
BridgeAR reviewed Jan 12, 2020
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

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 JS code is LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/crypto/keygen.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-crypto-dh-stateless.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
tniessen force-pushed the crypto-modernize-dh branch 2 times, most recently from 62197f1 to a962a3d Compare January 15, 2020 21:36
@tniessen

tniessen commented Feb 23, 2020

Copy link
Copy Markdown
Member Author

@mscdex I don't think KeyObjects themselves made it into node v10.x, so this will likely be a large and difficult backport.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BethGriggs BethGriggs added the backport-requested-v10.x label Feb 27, 2020
addaleax pushed a commit that referenced this pull request Mar 11, 2020
@tniessen @addaleax
crypto: make DH error messages consistent
1b9a62c 
Refs: #31178
Refs: #31445

PR-URL: #31873
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@rhenwood-arm rhenwood-arm mentioned this pull request Mar 12, 2020
re-enable crypto dh keygen testing on Arm #32233
Closed
@codebytere codebytere mentioned this pull request Mar 16, 2020
test: add secp224k1 check in crypto-dh-stateless #31715
Closed
2 tasks done
@targos targos removed the lts-watch-v12.x label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen @targos
crypto: add key type 'dh'
0f4946e 
The new key type 'dh' corresponds to EVP_PKEY_DH.

PR-URL: nodejs#31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen @targos
crypto: simplify DH groups
fd6bdfb 
PR-URL: nodejs#31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen @targos
crypto: add DH support to generateKeyPair
5380c33 
This allows using the generateKeyPair API for DH instead of the old
stateful DH APIs.

PR-URL: nodejs#31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen @targos
crypto: add crypto.diffieHellman
17a98cd 
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.

PR-URL: nodejs#31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen @targos
test: skip keygen tests on arm systems
644b3c8 
test-crypto-keygen and test-crypto-dh-stateless are currently flaky
on ARM CI systems due to their slow CPUs.

PR-URL: nodejs#31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen @targos
crypto: add key type 'dh'
9f85585 
The new key type 'dh' corresponds to EVP_PKEY_DH.

PR-URL: #31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen @targos
crypto: simplify DH groups
5dab489 
PR-URL: #31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen @targos
crypto: add DH support to generateKeyPair
1977136 
This allows using the generateKeyPair API for DH instead of the old
stateful DH APIs.

PR-URL: #31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen @targos
crypto: add crypto.diffieHellman
40253cc 
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.

PR-URL: #31178
Reviewed-By: Sam Roberts <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen @targos
test: skip keygen tests on arm systems
733002b 
test-crypto-keygen and test-crypto-dh-stateless are currently flaky
on ARM CI systems due to their slow CPUs.

PR-URL: #31178
Reviewed-By: Sam Roberts <[email protected]>
@targos targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged
@vishalsevani vishalsevani mentioned this pull request Oct 6, 2020
Issue for large number of ssh connections with DH key exchange on CentOS 8.2 mscdex/ssh2#934
Closed
@tniessen tniessen mentioned this pull request Jan 24, 2021
generateKeyPair('ec') should not support NODE-ED* and NODE-X* #37055
Closed
@panva panva mentioned this pull request Feb 3, 2021
crypto: introduce crypto/promises #37218
Closed
8 tasks
@mhdawson

mhdawson commented Mar 17, 2022

Copy link
Copy Markdown
Member

@tniessen I noticed some test excludes in the parallel tests:

[$arch==arm || $arch==arm64]
# https://github.com/nodejs/node/pull/31178
test-crypto-dh-stateless: SKIP
test-crypto-keygen: SKIP

Since this PR is closed I'm wondering if they are still needed and/or there is an open issue that should be listed instead of this one as to why they are excluded?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Mar 17, 2022

Copy link
Copy Markdown
Member Author

@mhdawson These tests were disabled because they kept timing out. I "temporarily disabled" them more than two years ago, see #31178 (comment) 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@addaleax addaleax addaleax left review comments
@BridgeAR BridgeAR BridgeAR left review comments
@panva panva panva approved these changes
+2 more reviewers
@lundibundi lundibundi lundibundi left review comments
@sam-github sam-github sam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

crypto: X25519 and X448 and ECDH

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@tniessen @panva @sam-github @nodejs-github-bot @mscdex @mhdawson @bnoordhuis @addaleax @BridgeAR @lundibundi @targos @BethGriggs
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.