/cc @sam-github @tniessen

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

indeed. coming up

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.

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

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.

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

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/

✅, ping the team?

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

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

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

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

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'),

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.

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

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

OK, I removed it from staging