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

test: fixtures key/certs cause failures when openssl security level > 1 #27862

Open
reasonablytall opened this issue May 24, 2019 · 13 comments
Open
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.

Comments

@reasonablytall
Copy link
Contributor

  • Version: v13.0.0-pre 5b8df5e
  • Platform: Linux alexaub.svl.corp.google.com 4.19.37-2rodete1-amd64 deps: update openssl to 1.0.1j #1 SMP Debian 4.19.37-2rodete1 (2019-05-15 > 2018) x86_64 GNU/Linux
  • Subsystem: test

In Debian testing, the latest openssl sets the default required security level to 2 in /etc/ssl/openssl.cnf (link). This requires (among other things, described here) that certs/keys be at least of a certain length depending on cipher.

Some of the keys under test/fixtures/ don't satisfy seclevel 2, and cause tests to fail when node's openssl reads that openssl.cnf.

Here's the output of python tools/test.py -J -p tap --logfile=test.log. 180 tests fail due to errors like ERR_SSL_EE_KEY_TOO_SMALL.

The issue can be avoided by changing the line CipherString = DEFAULT@SECLEVEL=2 to CipherString = DEFAULT@SECLEVEL=1 in /etc/ssl/openssl.cnf. However beware that this is globally reducing the required security on your machine.

I figure that this issue can be solved by regenerating the troublesome keys/certs with a greater size -- probably 2048 bits for RSA/DSA/DH and 256 for ECC. I've been looking into this, and I would be happy to take a crack at it! The changes would be similar to #3759, and I think that it would also be good to move the certs under test/fixtures/ to be under test/fixtures/keys/ and generate them in the Makefile.

@reasonablytall reasonablytall changed the title test: fixtures key/certs cause failures when openssl security level > 2 test: fixtures key/certs cause failures when openssl security level > 1 May 24, 2019
@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels May 25, 2019
@bnoordhuis
Copy link
Member

I think at least some of the tests check that things like 1024 bits RSA still work. Your proposal would invalidate those and I don't think that's something we'll accept, you'd have to float a downstream patch for them. Everything else is fair game though.

@sam-github
Copy link
Contributor

It looks like you could add SSL_CTX_get_security_level() support, and PR changes to the tests to understand whether they should pass or fail. Or even, assuming the error code was specific enough, add support to the tests to detect based on error code when failure is allowed due to the global security level test.

@bnoordhuis
Copy link
Member

#27962 landed so I think this can be closed out now? Please reopen if I'm mistaken.

@reasonablytall
Copy link
Contributor Author

reasonablytall commented Jul 3, 2019

#27962 unfortunately does not solve this issue because it only brings the test certs under a makefile -- it doesn't change their generation parameters to be secure enough. Changing those generation parameters should be simpler now with #27962, and I'm still interested in working on this issue, but I don't have a prediction for when I can.

@bnoordhuis
Copy link
Member

No problem, I'll reopen.

@bnoordhuis bnoordhuis reopened this Jul 3, 2019
@BobCochran
Copy link

I compiled 12.13.0 from source code on a fresh install of Debian 10.1 with all OS updates applied. When running make test-only I received 149 errors similar to this one:

Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
at Object.createSecureContext (_tls_common.js:131:17)
at Server.setSecureContext (_tls_wrap.js:1155:27)
at Server (_tls_wrap.js:1033:8)
at new Server (https.js:65:14)
at Object.createServer (https.js:89:10)
at serverKeepAliveTimeoutWithPipeline (/home/bobdeb101/node-v12.13.0/test/sequential/test-https-server-keep-alive-timeout.js:32:24)
at run (/home/bobdeb101/node-v12.13.0/test/sequential/test-https-server-keep-alive-timeout.js:28:11)
at processTicksAndRejections (internal/process/task_queues.js:75:11) {
library: 'SSL routines',
function: 'SSL_CTX_use_certificate',
reason: 'ee key too small',
code: 'ERR_SSL_EE_KEY_TOO_SMALL'
}

CipherString sets a SECLEVEL of 2:

$ egrep 'CipherString' /etc/ssl/openssl.cnf CipherString = DEFAULT@SECLEVEL=2

Kernel details:

4.19.0-6-amd64 #1 SMP Debian 4.19.67-2+deb10u1 (2019-09-20) x86_64 GNU/Linux

If I can help resolve this, please get in touch with me. I'm a software developer, but not a node.js developer and I have no qualifications in C++ development, nor in openssl security contexts. I'm commenting mainly to document this issue occurs in Debian 10.1.

I recently compiled 12.13.0 under Ubuntu 18.04.3 LTS and did not get similar errors from 'make test-only'.

@sam-github
Copy link
Contributor

node tests use old and unrecommended crypto, on purpose, they are still supported even if not recommended. Could you try with OPENSSL_CONF=/dev/null make test?

@andriy-simonov
Copy link

I have the same problem with versions 12.13.1 and 13.2.0 on Debian. OPENSSL_CONF=/dev/null make test actually allows the tests to pass. Is it a valid way to run the test, is it possible to miss any important cases with this configuration?

@bnoordhuis
Copy link
Member

@andriy-simonov Yes, that's a valid way to test - it tests openssl in its default configuration (what 99% of all deployments probably use.)

@MylesBorins
Copy link
Contributor

I'm running into this issue right now when building / testing on crostini (pixelbook). If this testing issue is widespread on mdoern versions of ubuntu (which have that security setting) should we perhaps consider changing some of the internals of what we do with make so that folks on linux can build / test without having to dig up this thread / set env vars?

@sam-github
Copy link
Contributor

Could do so, but it could go awry, making things work for test, but then when people actually try it they don't work.

Fwiw, I run ubuntu 19.10, and haven't run into any issues related to this. Maybe I will when I update to 20.10, but I'm not going to update to a 1 day old OS. :-)

@bnoordhuis
Copy link
Member

@sam-github If you mean 20.04, I upgraded yesterday and things work okay. :-)

@richardlau
Copy link
Member

We've seen this on the CI on freshly installed Debian 10 and Fedora 34, which both set security level 2 and Node.js is picking up their openssl.cnf. I've had to put scripts in our Ansible playbooks to drop the security level back down for tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

7 participants