tls: ciphers allow bang syntax

[atlowChemi]

Fixes: #49699

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/net

[atlowChemi]

Still want to add a UT, was not sure exactly what, will have a go at it later today

[bnoordhuis]

IMO, these exceed (by a fairly wide margin) the threshold of what's still legible. I'd break them up in separate statements.

[nodejs-github-bot]

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

[atlowChemi]

@bnoordhuis The CI failed with the following:

not ok 2966 parallel/test-tls-set-ciphers
  ---
  duration_ms: 283.21700
  severity: fail
  exitcode: 1
  stack: |-
    test: AES256-SHA 9 expect U U ERR_INVALID_ARG_TYPE
       (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-tls-set-ciphers.js:121:1)
    client undefined
    server ERR_INVALID_ARG_TYPE
    test: AES256-SHA : expect U U ERR_INVALID_ARG_VALUE
       (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-tls-set-ciphers.js:123:1)
    client undefined
    server ERR_INVALID_ARG_VALUE
    test: TLS_AES_256_GCM_SHA384:!TLS_CHACHA20_POLY1305_SHA256 U expect TLS_AES_256_GCM_SHA384 U U
       (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-tls-set-ciphers.js:88:1)
    node:assert:991
        throw newErr;
        ^
    
    AssertionError [ERR_ASSERTION]: ifError got unwanted exception: error:1426E0B9:SSL routines:ciphersuite_cb:no cipher match
        at /home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-tls-set-ciphers.js:63:12
        at /home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:474:15
        at /home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:474:15
        at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/fixtures/tls-connect.js:78:9)
        at configSecureContext (node:internal/tls/secure-context:234:13)
        at Object.createSecureContext (node:_tls_common:116:3)
        at Object.connect (node:_tls_wrap:1748:48)
        at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/fixtures/tls-connect.js:65:13)
        at Object.onceWrapper (node:events:628:28)
        at Server.emit (node:events:514:28)
        at emitListeningNT (node:net:1906:10)
        at process.processTicksAndRejections (node:internal/process/task_queues:81:21) {
      generatedMessage: false,
      code: 'ERR_ASSERTION',
      actual: Error: error:1426E0B9:SSL routines:ciphersuite_cb:no cipher match
          at configSecureContext (node:internal/tls/secure-context:234:13)
          at Object.createSecureContext (node:_tls_common:116:3)
          at Object.connect (node:_tls_wrap:1748:48)
          at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/fixtures/tls-connect.js:65:13)
          at Object.onceWrapper (node:events:628:28)
          at Server.emit (node:events:514:28)
          at emitListeningNT (node:net:1906:10)
          at process.processTicksAndRejections (node:internal/process/task_queues:81:21) {
        library: 'SSL routines',
        function: 'ciphersuite_cb',
        reason: 'no cipher match',
        code: 'ERR_SSL_NO_CIPHER_MATCH'
      },
      expected: null,
      operator: 'ifError'
    }
    
    Node.js v21.0.0-pre

Do you think we should skip this test-case on a specific platform etc?

[bnoordhuis]

On the one hand, I'd like to better understand why the test fails with openssl 1.1.1. On the other hand, it's EOL and not worth sinking a lot of time in. I've opened nodejs/build#3496 to discuss removing the buildbots.

Aside 1: doc/api/tls.md tells you to consult https://www.openssl.org/docs/man1.1.1/man1/openssl-ciphers.html (why the 1.1.1 version?) for the cipher list syntax. We don't support the full syntax (e.g. + and -) and I don't think it's important that we do but the docs should make it clear only a subset is supported.

Aside 2: test/parallel/test-tls-set-ciphers.js has pretty much the same bug as lib/internal/tls/secure-context.js but the line length rather obscures it:

node/test/parallel/test-tls-set-ciphers.js

Lines 23 to 24 in 9718a94

	if ((typeof ciphers === 'string' || ciphers instanceof String) && ciphers.length > 0 && !ciphers.includes('TLS_'))
	return 'TLSv1.2';

[mhdawson]

@atlowChemi as discussed in nodejs/build#3496 would you mind adding a check against (common.hasOpenSSL3 || common.hasOpenSSL31) so that the failing test would only run only in builds that don't use 1.1.1 for now?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/49712
✔  Done loading data for nodejs/node/pull/49712
----------------------------------- PR info ------------------------------------
Title      tls: ciphers allow bang syntax (#49712)
Author     Chemi Atlow  (@atlowChemi)
Branch     atlowChemi:cipher-suites -> nodejs:main
Labels     tls, needs-ci
Commits    1
 - tls: ciphers allow bang syntax
Committers 1
 - atlowChemi 
PR-URL: https://github.com/nodejs/node/pull/49712
Fixes: https://github.com/nodejs/node/issues/49699
Reviewed-By: Ben Noordhuis 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49712
Fixes: https://github.com/nodejs/node/issues/49699
Reviewed-By: Ben Noordhuis 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - tls: ciphers allow bang syntax
   ℹ  This PR was created on Tue, 19 Sep 2023 07:27:31 GMT
   ✔  Approvals: 1
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/49712#pullrequestreview-1637126331
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-10-03T22:12:16Z: https://ci.nodejs.org/job/node-test-pull-request/54509/
- Querying data for job/node-test-pull-request/54509/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6401257256

[nodejs-github-bot]

Landed in fae1af0