GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: update tests for OpenSSL 3.0.14 by richardlau · Pull Request #53373 · nodejs/node

Conversation

@richardlau

@richardlau richardlau commented Jun 6, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
avivkeller reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@richardlau richardlau added test Issues and PRs related to Node.js core tests and test infrastructure. openssl Issues and PRs related to the OpenSSL dependency. lts-watch-v18.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

nodejs-github-bot commented Jun 6, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59681/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Jun 6, 2024
View reviewed changes
@richardlau
test: update tests for OpenSSL 3.0.14
25d62ec 
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
@richardlau
richardlau force-pushed the openssl3.0.14tests branch from fecfde6 to 25d62ec Compare June 7, 2024 11:36
@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59682/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59684/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson approved these changes Jun 7, 2024
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tniessen

tniessen commented Jun 7, 2024

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Jun 7, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau richardlau mentioned this pull request Jun 7, 2024
Test failures with OpenSSL 3.2.2 #53382
Closed
26 tasks done
@richardlau

richardlau commented Jun 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
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.

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Jun 10, 2024

Copy link
Copy Markdown
Member

@richardlau It's entirely up to you :)
richardlau reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau richardlau added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 10, 2024
@richardlau

richardlau commented Jun 10, 2024

Copy link
Copy Markdown
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 🙂.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 10, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 14863e8 into nodejs:main Jun 10, 2024
@nodejs-github-bot

nodejs-github-bot commented Jun 10, 2024

Copy link
Copy Markdown
Collaborator

Landed in 14863e8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@richardlau @targos
test: update tests for OpenSSL 3.0.14
602b9d6 
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@lheckemann

lheckemann commented Jun 20, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lheckemann lheckemann mentioned this pull request Jun 20, 2024
nodejs: run JS test suite as part of the checks NixOS/nixpkgs#313982
Merged
3 of 13 tasks
@richardlau

richardlau commented Jun 20, 2024

Copy link
Copy Markdown
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.
lheckemann reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@richardlau @soophoo
test: update tests for OpenSSL 3.0.14
53d1d6e 
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Jun 25, 2024
v22.4.0 release proposal #53583
Merged
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16
Open
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
test: update tests for OpenSSL 3.0.14
059e47c 
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
v20.16.0 proposal #53945
Merged
@richardlau richardlau mentioned this pull request Aug 21, 2024
Revert "deps: upgrade openssl sources to quictls/openssl-3.0.14+quic1" #54491
Closed
richardlau added a commit that referenced this pull request Sep 19, 2024
@richardlau
test: update tests for OpenSSL 3.0.14
9f521f4 
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos removed the lts-watch-v20.x label Sep 30, 2024
@aduh95 aduh95 removed the lts-watch-v18.x label Nov 2, 2024
@aduh95 aduh95 mentioned this pull request Nov 7, 2024
2024-11-12, Version 18.20.5 'Hydrogen' (LTS) #55768
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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 Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@richardlau @nodejs-github-bot @tniessen @lheckemann @lpinca @mhdawson @targos @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.