test: add secp224k1 check in crypto-dh-stateless #31715
Conversation
danbev
changed the title
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. |
addaleax
added
the
author ready
The test should work as long as both curves are valid EC curves, but they must be distinct.
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 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 I'm also okay with the change as it is, just not a big favor of silently skipping the check :) |
|
@tniessen Thanks for the suggestion, I'll give that a try today. |
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
danbev
force-pushed
the
shared_openssl_check-test-crypto-dh-stateless
branch
from
9fa3759
to
47ef627
Compare
|
Thank you! |
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 PR-URL: #31715 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 9403250. |
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 PR-URL: #31715 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Blocked on #31178, which can likely land in the next minor (this next upcoming one is a patch) |
targos
removed
author ready
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 PR-URL: nodejs#31715 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 PR-URL: #31715 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
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:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes