test: add secp224k1 check in crypto-dh-stateless

[danbev]

This commit adds a check to test-crypto-dh-statless to avoid an error if
the curve secp224k1 is not present. This could occur if node was
configured with shared-openssl.

The use case for this was building on RHEL 8.1 which only has the
following curves:

$ openssl ecparam -list_curves
secp224r1 : NIST/SECG curve over a 224 bit prime field
secp256k1 : SECG curve over a 256 bit prime field
secp384r1 : NIST/SECG curve over a 384 bit prime field
secp521r1 : NIST/SECG curve over a 521 bit prime field
prime256v1: X9.62/SECG curve over a 256 bit prime field

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[tniessen]

LGTM, but wouldn't it be easier to switch to a different curve that's available on all systems?

[danbev]

LGTM, but wouldn't it be easier to switch to a different curve that's available on all systems?

I did not think about changing the curve as I thought that was something that was being tested specifically. I don't think we can know which curves will be available on all systems and they might change over time. Even ones that exist now might be removed later. I'm not saying either way is better and I'm happy as long as the test does not fail for us.

Kind of related to this is that I'm going to open an issue in the build working group suggesting we add UBI 8.1 (Universal Base Image which is based on RHEL) build to the CI system which would dynamically link to the system OpenSSL. This would allow us to test FIPS which is not done at the moment but also find issues like this one early. I'll link this issue to it.

[mhdawson]

LGTM

[nodejs-github-bot]

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

[tniessen]

I did not think about changing the curve as I thought that was something that was being tested specifically.

The test should work as long as both curves are valid EC curves, but they must be distinct.

I don't think we can know which curves will be available on all systems and they might change over time. Even ones that exist now might be removed later.

Exactly, that's my problem: If secp224k1 is removed on one or more platforms, we don't have test coverage on those anymore, and we wouldn't know, because the test is skipped silently. But we also cannot cannot just pick any curve from crypto.getCurves(), because some might be aliases for secp256k1.

From what I can tell, OpenSSL supports at least secp224r1, secp384r1, and secp521r1, even in FIPS mode (openssl/openssl#9081). The easiest fix might be to just use secp224r1, which will likely be available everywhere, and if it isn't, we can revisit this. But if we don't want to risk limiting the test to one specific curve, something like this should also work:

const not256k1 = crypto.getCurves().find(c => /^sec.*(224|384|512)/.test(c));
assert.throws(() => {
  test(crypto.generateKeyPairSync('ec', { namedCurve: 'secp256k1' }),
       crypto.generateKeyPairSync('ec', { namedCurve: not256k1 }));
}, {
  name: 'Error',
  code: 'ERR_OSSL_EVP_DIFFERENT_PARAMETERS'
});

2 line change instead of 20 lines :)

I think it's fair to assume that any OpenSSL implementation will support at least one sec* curve that isn't 256 bits. We don't need to check if not256k1 === undefined because generateKeyPairSync will throw an error in that case.

I'm also okay with the change as it is, just not a big favor of silently skipping the check :)

[danbev]

@tniessen Thanks for the suggestion, I'll give that a try today.

[nodejs-github-bot]

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

[tniessen]

Thank you!

[danbev]

Landed in 9403250.

[codebytere]

Blocked on #31178, which can likely land in the next minor (this next upcoming one is a patch)