Review requested:

• @nodejs/crypto
• @nodejs/net

A few tests will require fixes.... but it would be nice to have feedback especially when these ciphers are set externally to node itself.

I've fixed some of the unit tests now but will finish the rest next week. Mostly it's just involves clamping the maxVersion to TLSv1.2 when there are no TLS 1.3 cipher suites set.

This should now pass CI. Fix in parallel/test-tls-set-ciphers.js is rather ugly but it just re-iterates previous assumptions.

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

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

- Loading data for nodejs/node/pull/43427
✔  Done loading data for nodejs/node/pull/43427
----------------------------------- PR info ------------------------------------
Title      crypto: don't disable TLS 1.3 without suites (#43427)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     AdamMajer:ciphersuites -> nodejs:main
Labels     tls, commit-queue-squash
Commits    1
 - crypto: don't disable TLS 1.3 without suites
Committers 1
 - Adam Majer 
PR-URL: https://github.com/nodejs/node/pull/43427
Fixes: https://github.com/nodejs/node/issues/43419
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43427
Fixes: https://github.com/nodejs/node/issues/43419
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - crypto: don't disable TLS 1.3 without suites
   ℹ  This PR was created on Tue, 14 Jun 2022 15:11:49 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006059888
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006070148
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006073727
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-06-24T23:13:05Z: https://ci.nodejs.org/job/node-test-pull-request/44842/
- Querying data for job/node-test-pull-request/44842/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Landed in 9cde7a0

OpenSSL docs for reference:

• https://www.openssl.org/docs/man3.0/man1/openssl-ciphers.html
• https://www.openssl.org/docs/man3.0/man3/SSL_CTX_set_cipher_list.html

-ciphersuites val Sets the list of TLSv1.3 ciphersuites.
cipherlist A cipher list of TLSv1.2 and below ciphersuites

node --help shows:

  --tls-cipher-list=...       use an alternative default TLS cipher
                              list

Node.js' TLS cipher list covers all TLS versions (in contrary to the OpenSSL definition).

With this PR, if we specify a single TLSv1.2 ciphersuite (regardless whether through --tls-cipher-list or ciphers, all default TLSv1.3 ciphersuites remain enabled.

The change is quite breaking, so I believe this should have been released in the next major release instead.

This is correct - the TLSv1.3 ciphersuites are for TLSv1.3, so the correct way is to set max TLS version to 1.2 if you only want TLS 1.2 used. This fix breaks the assumption that TLSv1.3 should be disabled when cipher suites are not explicitly present because TLSv1.3 has a default set of cipher suites already. This was not the case for TLSv1.2 and prior.

But at the same time, the way that cipher suites were selected prior to this, having disabled TLSv1.3 whenever distros used crypto policies or other OpenSSL based ciphers - this is a bug. For example, setting ciphers list to DEFAULT_SUSE or to PROFILE=SYSTEM and then have TLSv1.3 disabled ... that's a problem.

Personally, I will have to backport this to our nodejs16 irrespective if it gets backported here. I believe that disabling TLSv1.3 by accident is worse than having it enabled by accident 😉

For example, setting ciphers list to DEFAULT_SUSE or to PROFILE=SYSTEM and then have TLSv1.3 disabled ... that's a problem.

That is a separate case and should not (I believe) be confused with providing a single ciphersuite. I think the problem we have here is that a single option is used to control both <TLSv1.3 and TLSv1.3 ciphersuites. OpenSSL separates them and I believe that Node.js should too 🤔

Providing different options now for both would be even larger change. Upstream has provided the 2nd option because they didn't want people to accidentally disable their TLSv1.3 support by not supplying TLS1.3 ciphers ;)

openssl/openssl#11899 (comment)

In Node, the difference between cipher set and cipher suite is whether it uses standard names or not. So, TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384 would be sent to cipher suite function and ECDHE-PSK-AES256-CBC-SHA384 to the cipher set function for TLS 1.2 and below. But both set TLS 1.2 and below ciphers! For a second, I thought this would be a bug but look,

openssl ciphers -v -ciphersuites TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384 NONE
ECDHE-PSK-AES256-CBC-SHA384 TLSv1 Kx=ECDHEPSK Au=PSK  Enc=AES(256)  Mac=SHA384

OpenSSL surprises :-) So, having a single command line parameter for both makes sense, provided that this OpenSSL feature is not a bug 😆

Now, for the possible reason why TLSv1.3 was disabled in node when no ciphersuites were selected, is,

openssl s_client -ciphersuites TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 -connect google.com:443
CONNECTED(00000003)
140005711374144:error:141A90B5:SSL routines:ssl_cipher_list_to_bytes:no ciphers available:ssl/statem/statem_clnt.c:3800:No ciphers enabled for max supported SSL/TLS version

This will work if we explicitly only permit TLSv1.2,

openssl s_client -tls1_2 -ciphersuites TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 -cipher NONE -connect google.com:443
...
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : ECDHE-RSA-AES256-GCM-SHA384
...

And it makes sense because TLS Protocol version negotiation happens prior to cipher negotiation. And this was already found 2 years ago,

adrienverge/openfortivpn#687 (comment)

Of course, the core of the problem is that TLSv1.3 should not be disabled if cipher suites are not specified. This PR fixes this problem and restores upstream behaviour (as per OpenSSL's explicit design decision). This is why it should not be viewed as a major feature change and more of a bug fix.

openssl/openssl#11899 (comment)

Ciphersuites in TLSv1.3 are completely different to TLSv1.2 ciphersuites and are mutually exclusive

Hence the need for two different options.

TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384

It's not a TLSv1.3 ciphersuite.

as per OpenSSL's explicit design decision

Their explicit design decision was to have two different options, not a single one. Despite this PR being a bug fix, it's a major breaking change. I had code that was relying on the previous behavior and it stopped working with this PR.

Sure, but despite what it says, openssl is still setting TLSv1.2 cipher with ciphersuite option, as I demonstrated. 🤷

I had code that was relying on the previous behavior and it stopped working with this PR.

Did you expect that TLSv1.3 would be disabled implicitly if you didn't specify the ciphersuite in a cipher set? That's the only thing that is really changed here. --tls-max-v1.2 should be used instead in these cases.

openssl is still setting TLSv1.2 cipher with ciphersuite option, as I demonstrated

It was just implemented like that:

Under the hood the TLSv1.3 ciphersuites still get added into the same cipher_list as all the others. This is visible to an application if it calls SSL_get_ciphers() et al.

despite what it says

That doesn't mean the option should be misused, no?

Did you expect that TLSv1.3 would be disabled implicitly if you didn't specify the ciphersuite in a cipher set?

Correct. I expected that because Node.js' option manages both <TLSv1.3 and TLSv1.3. Autofilling other ciphers where I explicitly set just one sounds like magic behavior to me.

--tls-max-v1.2 should be used instead in these cases.

That works, however that is a breaking change since it requires a change in my code. To sum up, since this has already landed I'm okay with this change ~ maybe someday we'll have a more strict way to set ciphers :)