Skip to content

tls: ciphers allow bang syntax#49712

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
atlowChemi:cipher-suites
Oct 4, 2023
Merged

tls: ciphers allow bang syntax#49712
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
atlowChemi:cipher-suites

Conversation

@atlowChemi
Copy link
Copy Markdown
Member

Fixes: #49699

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Sep 19, 2023
@atlowChemi
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@atlowChemi atlowChemi force-pushed the cipher-suites branch 2 times, most recently from a439f5d to a55d146 Compare September 21, 2023 06:32
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@atlowChemi
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown
Member

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:

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

@mhdawson
Copy link
Copy Markdown
Member

@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?

@atlowChemi atlowChemi added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 30, 2023
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator