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 x25119 and x448 key pair generation #26774

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Mar 19, 2019

This adds support for key pair generation for x25519 and x448 so that #26626 is one step closer.

Refs: #26626 - ECDH with x25519 and x448 keys

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

@nodejs-github-bot 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 Mar 19, 2019
@panva
Copy link
Member Author

panva commented Mar 19, 2019

/cc @sam-github @tniessen

@panva panva marked this pull request as ready for review March 19, 2019 11:34
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR basically LGTM but can you also add regression tests to test/parallel/test-crypto-key-objects.js? Thanks.

@panva
Copy link
Member Author

panva commented Mar 19, 2019

The PR basically LGTM but can you also add regression tests to test/parallel/test-crypto-key-objects.js? Thanks.

indeed. coming up

@tniessen
Copy link
Member

I guess we should be careful with supporting key types, but I don't have much experience around ECDH keys so I hope others do.

@tniessen
Copy link
Member

With this in place i think (changed my mind) all these new keys should all have the same type (e.g. okp)

See #26709 (comment).

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis
Copy link
Member

I guess we should be careful with supporting key types

I don't disagree but since it's mostly transparent to Node.js because it goes through openssl's EVP API, I don't see a problem with supporting them.

@panva
Copy link
Member Author

panva commented Mar 19, 2019

The CI failures don't seem to be related to the PR. Ideas?

@tniessen
Copy link
Member

CI is known to be flaky from time to time, let's see what happens when I resume it: https://ci.nodejs.org/job/node-test-pull-request/21671/

@panva
Copy link
Member Author

panva commented Mar 19, 2019

✅, ping the team?

doc/api/crypto.md Outdated Show resolved Hide resolved
Co-Authored-By: panva <panva.ip@gmail.com>
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the next key type, we are going to need to refactor our docs to have a section on key types, and the invalid messages will have to change to be "not a supported key type" rather than an ever growing list of types. But, that shouldn't block this.

@panva panva changed the title crypto: add support for OKP x25119 and x448 key pair generation crypto: add support for x25119 and x448 key pair generation Mar 20, 2019
@tniessen
Copy link
Member

@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 25, 2019
targos pushed a commit that referenced this pull request Mar 28, 2019
PR-URL: #26774
Refs: #26626
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 28, 2019
Now that support for X25519 and X448 has been added, this function is
not used exclusively for EdDSA keys anymore.

PR-URL: #26900
Refs: #26774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@tniessen tniessen mentioned this pull request Mar 29, 2019
16 tasks
@sam-github
Copy link
Contributor

@targos @BethGriggs This was landed on v11.x-staging, but it doesn't work against openssl 1.1.0, which doesn't support those. I found this while testing #26951 against openssl 1.1.0, you'll find out soon, too. So, I think it needs popping off of v11.x-staging, and to be backported.

@panva
Copy link
Member Author

panva commented Mar 29, 2019

>,< there will be more in that same boat i guess then (e.g. #26319, which i assumed was labeled to not land by accident). I thought CI handles these cases.

@tniessen
Copy link
Member

Oh, right, I forgot about that, too... We are targeting 1.1.1 but still allow linking against 1.1.0 IIRC :(

@sam-github
Copy link
Contributor

when prepping the release, people don't pull a single commit onto v11.x-staging, run full ci, pull another, etc. They do more of a bulk, test, hope its good (it should be), pull a bunch more. Then narrow down problems to single commits if they occur. So, when @BethGriggs started work on 11.x, she would notice this.

Maybe this is enough:

diff --git a/test/parallel/test-crypto-key-objects.js b/test/parallel/test-crypto-key-objects.js
index fb1c3de1d7..f18ad41658 100644
--- a/test/parallel/test-crypto-key-objects.js
+++ b/test/parallel/test-crypto-key-objects.js
@@ -168,6 +168,9 @@ const privatePem = fixtures.readSync('test_rsa_privkey.pem', 'ascii');
   }, /null/);
 }
 
+if (!/1\.1\.1/.test(process.versions.openssl))
+  return common.skip('ec key types added in openssl 1.1.1');
+
 [
   { private: fixtures.readSync('test_ed25519_privkey.pem', 'ascii'),
     public: fixtures.readSync('test_ed25519_pubkey.pem', 'ascii'),

@sam-github
Copy link
Contributor

Or, you could just not backport it. it depends if you are hoping to get it onto an LTS release. It will come in 12.x, but 12.x won't be LTS until the fall.

@targos
Copy link
Member

targos commented Mar 29, 2019

I'm quite sure I ran tests before pushing to staging. Is it supposed to fail?

@sam-github
Copy link
Contributor

@targos the shared_openssl1.1.0 test won't pass, I'm not sure what you ran, was that test part of it?

This is the one:

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_openssl110_x64/11817/

the key types this PR adds support for aren't supported by openssl 1.1.0, so the test has to be skipped when node is built against that openssl version.

@targos
Copy link
Member

targos commented Mar 29, 2019

OK, I removed it from staging

BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
Now that support for X25519 and X448 has been added, this function is
not used exclusively for EdDSA keys anymore.

PR-URL: #26900
Refs: #26774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
tniessen added a commit to tniessen/node that referenced this pull request Aug 31, 2021
panva pushed a commit that referenced this pull request Sep 1, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants