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: update tests for OpenSSL 3.0.14 #53373

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Jun 6, 2024

Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old ECONNRESET error on older versions of OpenSSL and the new ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL on newer versions of OpenSSL.

Refs: openssl/openssl#24338


Addresses these failures (Node.js dynamically linked against OpenSSL 3.0.14 -- I first noticed these (and other failures) with OpenSSL 3.2.2):

not ok 1779 parallel/test-http2-https-fallback
  ---
  duration_ms: 164.14700
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/home/nodejs/node/test/parallel/test-http2-https-fallback.js:154:11)
        at TLSSocket.<anonymous> (/home/nodejs/node/test/common/index.js:466:15)
        at TLSSocket.emit (node:events:520:28)
        at emitErrorNT (node:internal/streams/destroy:170:8)
        at emitErrorCloseNT (node:internal/streams/destroy:129:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v23.0.0-pre
  ...
--
not ok 1873 parallel/test-http2-server-unknown-protocol
  ---
  duration_ms: 132.88000
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/home/nodejs/node/test/parallel/test-http2-server-unknown-protocol.js:45:12)
        at TLSSocket.<anonymous> (/home/nodejs/node/test/common/index.js:466:15)
        at TLSSocket.emit (node:events:520:28)
        at emitErrorNT (node:internal/streams/destroy:170:8)
        at emitErrorCloseNT (node:internal/streams/destroy:129:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v23.0.0-pre
  ...
--
not ok 3010 parallel/test-tls-alpn-server-client
  ---
  duration_ms: 156.74900
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected

    + 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL'
    - 'ECONNRESET'
        at TLSSocket.<anonymous> (/home/nodejs/node/test/parallel/test-tls-alpn-server-client.js:195:14)
        at TLSSocket.<anonymous> (/home/nodejs/node/test/common/index.js:466:15)
        at TLSSocket.emit (node:events:520:28)
        at emitErrorNT (node:internal/streams/destroy:170:8)
        at emitErrorCloseNT (node:internal/streams/destroy:129:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL',
      expected: 'ECONNRESET',
      operator: 'strictEqual'
    }

    Node.js v23.0.0-pre
  ...

cc @nodejs/crypto

@richardlau richardlau added test Issues and PRs related to the tests. openssl Issues and PRs related to the OpenSSL dependency. lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x labels Jun 6, 2024
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 6, 2024
@nodejs-github-bot
Copy link
Collaborator

Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessen
Copy link
Member

tniessen commented Jun 7, 2024

@richardlau Is there any chance you could make this conditional based on the version of OpenSSL? Otherwise, we'll likely forget about it and keep the old code here forever.

@richardlau
Copy link
Member Author

@richardlau Is there any chance you could make this conditional based on the version of OpenSSL? Otherwise, we'll likely forget about it and keep the old code here forever.

It's going to be complicated -- there's currently four OpenSSL 3 lines:

  • 3.0.x
  • 3.1.x
  • 3.2.x
  • 3.3.x

and the change is only in the latest release of each of those four lines.

@richardlau richardlau mentioned this pull request Jun 7, 2024
26 tasks
@richardlau
Copy link
Member Author

richardlau commented Jun 10, 2024

@richardlau Is there any chance you could make this conditional based on the version of OpenSSL? Otherwise, we'll likely forget about it and keep the old code here forever.

@tniessen We'd end up with code like this repeated for each of the three tests:

diff --git a/test/parallel/test-http2-https-fallback.js b/test/parallel/test-http2-https-fallback.js
index 8e0612375f4..8a1c06caad4 100644
--- a/test/parallel/test-http2-https-fallback.js
+++ b/test/parallel/test-http2-https-fallback.js
@@ -151,7 +151,14 @@ function onSession(session, next) {
       // Incompatible ALPN TLS client
       tls(Object.assign({ port, ALPNProtocols: ['fake'] }, clientOptions))
         .on('error', common.mustCall((err) => {
-          strictEqual(err.code, 'ECONNRESET');
+          // ECONNRESET prior to OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1.
+          const { OPENSSL_VERSION_NUMBER } = require('node:crypto').constants;
+          const expectedErr = (OPENSSL_VERSION_NUMBER < 0x300000e0 ||
+            (OPENSSL_VERSION_NUMBER >= 0x30100000 && OPENSSL_VERSION_NUMBER < 0x30100060) ||
+            (OPENSSL_VERSION_NUMBER >= 0x30200000 && OPENSSL_VERSION_NUMBER < 0x30200020) ||
+            (OPENSSL_VERSION_NUMBER >= 0x30300000 && OPENSSL_VERSION_NUMBER < 0x30300010)) ?
+            'ECONNRESET' : 'ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL';
+          strictEqual(err.code, expectedErr);
           cleanup();
           testNoALPN();
         }));

If you think this is worth doing I can update this PR, but I think this is harder to read.

@tniessen
Copy link
Member

@richardlau It's entirely up to you :)

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2024
@richardlau
Copy link
Member Author

@richardlau It's entirely up to you :)

In which case I'll land as-is as this has a passing CI, approvals and no blocks 🙂.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit 14863e8 into nodejs:main Jun 10, 2024
51 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 14863e8

@richardlau richardlau deleted the openssl3.0.14tests branch June 10, 2024 18:05
targos pushed a commit that referenced this pull request Jun 20, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@lheckemann
Copy link

Older versions do fail the tests against newer OpenSSL versions, so could this be backported to v18 and v20?

@richardlau
Copy link
Member Author

Older versions do fail the tests against newer OpenSSL versions, so could this be backported to v18 and v20?

Once it's gone out in a current (i.e. Node.js 22) release first.

EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: nodejs#53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: nodejs#53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau added a commit that referenced this pull request Sep 19, 2024
Starting from OpenSSL 3.0.14, 3.1.6, 3.2.2, and 3.3.1, OpenSSL was fixed
to return an error reason string for bad/unknown application protocols.

Update tests to handle both the old `ECONNRESET` error on older versions
of OpenSSL and the new `ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL` on
newer versions of OpenSSL.

Refs: openssl/openssl#24338
PR-URL: #53373
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos removed the lts-watch-v20.x PRs that may need to be released in v20.x label Sep 30, 2024
@aduh95 aduh95 removed the lts-watch-v18.x PRs that may need to be released in v18.x. label Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants