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 — src: fix dynamically linked OpenSSL version by richardlau · Pull Request #53456 · nodejs/node

Conversation

@richardlau

@richardlau richardlau commented Jun 14, 2024

Copy link
Copy Markdown
Member

Some fixes for dynamically linked OpenSSL (i.e. configure --shared-openssl):

  • src: fix dynamically linked OpenSSL version
    Report the version of OpenSSL that Node.js is running with instead
    of the version of OpenSSL that Node.js was compiled against.
  • test: check against run-time OpenSSL version
    Update common.hasOpenSSL3* to check against the run-time version of
    OpenSSL instead of the version of OpenSSL that Node.js was compiled
    against.
    Add a generalized common.hasOpenSSL() so we do not need to keep adding
    new checks for each new major/minor of OpenSSL.

There's a secondary issue, where all of process.versions is currently captured in the snapshot and thus reflects the snapshot-time values rather than run-time ones. That is being addressed separately by #53444.

cc @nodejs/crypto

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
richardlau added 2 commits June 14, 2024 16:51
@richardlau
src: fix dynamically linked OpenSSL version
f132c85 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.
@richardlau
test: check against run-time OpenSSL version
d71c5ab 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 14, 2024
richardlau
richardlau commented Jun 14, 2024
View reviewed changes
Comment thread test/parallel/test-crypto-dh.js Outdated
Comment on lines +89 to +90
const v = crypto.constants.OPENSSL_VERSION_NUMBER;
const hasOpenSSL3WithNewErrorMessage = (v >= 0x300000c0 && v <= 0x30100000) || (v >= 0x30100040 && v <= 0x30200000);
const hasOpenSSL3WithNewErrorMessage = (common.hasOpenSSL(3, 0, 12) && !common.hasOpenSSL(3, 1, 0)) ||
(common.hasOpenSSL(3, 1, 4) && !common.hasOpenSSL(3, 2, 0));

@richardlau richardlau Jun 14, 2024

Copy link
Copy Markdown
Member Author

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.

I've translated the existing checks, but there's actually an issue here which is that OpenSSL 3.2 and 3.3 have the new error message but fail the hasOpenSSL3WithNewErrorMessage check. I'm thinking this could be explicitly fixed in a follow up PR, but let me know if you prefer me to include it here since I'm changing the check anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca reviewed Jun 14, 2024
View reviewed changes
Comment thread test/parallel/test-crypto-dh.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@richardlau
fixup! test: check against run-time OpenSSL version
067a462
@richardlau richardlau added the commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. label Jun 14, 2024
lpinca
lpinca approved these changes Jun 14, 2024
View reviewed changes
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 14, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 14, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@richardlau

This comment was marked as outdated.

Sign in to view
@richardlau

This comment was marked as outdated.

Sign in to view
@richardlau
richardlau marked this pull request as draft June 15, 2024 13:49
@richardlau
fixup! test: check against run-time OpenSSL version
3f69079
@nodejs-github-bot

nodejs-github-bot commented Jun 15, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau
richardlau marked this pull request as ready for review June 15, 2024 18:01
@nodejs-github-bot

nodejs-github-bot commented Jun 15, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Jun 15, 2024
View reviewed changes
@richardlau richardlau added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 15, 2024
tniessen
tniessen approved these changes Jun 18, 2024
View reviewed changes
@richardlau richardlau added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 18, 2024
nodejs-github-bot pushed a commit that referenced this pull request Jun 18, 2024
@richardlau @nodejs-github-bot
src: fix dynamically linked OpenSSL version
5909cf3 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@nodejs-github-bot

nodejs-github-bot commented Jun 18, 2024

Copy link
Copy Markdown
Collaborator

Landed in 26f2cbd...3e7129e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nodejs-github-bot pushed a commit that referenced this pull request Jun 18, 2024
@richardlau @nodejs-github-bot
test: check against run-time OpenSSL version
3e7129e 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@nodejs-github-bot nodejs-github-bot closed this Jun 18, 2024
@richardlau richardlau mentioned this pull request Jun 18, 2024
test: fix OpenSSL version checks #53503
Merged
@richardlau
richardlau deleted the opensslver branch June 18, 2024 22:48
targos pushed a commit that referenced this pull request Jun 20, 2024
@richardlau @targos
src: fix dynamically linked OpenSSL version
e16a04e 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Jun 20, 2024
@richardlau @targos
test: check against run-time OpenSSL version
513e6aa 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@richardlau @soophoo
src: fix dynamically linked OpenSSL version
4c6df95 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@richardlau @soophoo
test: check against run-time OpenSSL version
eb3aa37 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[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
src: fix dynamically linked OpenSSL version
a0879ad 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@richardlau @marco-ippolito
test: check against run-time OpenSSL version
54e0ba8 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: #53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
v20.16.0 proposal #53945
Merged
@codebytere codebytere mentioned this pull request Jul 25, 2024
src: account for OpenSSL unexpected version #54038
Merged
@richardlau richardlau mentioned this pull request Aug 28, 2024
test: fix test-tls-client-auth test for OpenSSL32 #54610
Closed
richardlau added a commit to aduh95/node that referenced this pull request Sep 27, 2024
@richardlau
src: fix dynamically linked OpenSSL version
6615fe5 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
richardlau added a commit to aduh95/node that referenced this pull request Sep 27, 2024
@richardlau
test: check against run-time OpenSSL version
cc3cdf7 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@aduh95 aduh95 mentioned this pull request Nov 7, 2024
2024-11-12, Version 18.20.5 'Hydrogen' (LTS) #55768
Merged
@richardlau richardlau mentioned this pull request Dec 6, 2024
test: make test-crypto-hash compatible with OpenSSL > 3.4.0 #56160
Closed
@aduh95 aduh95 mentioned this pull request Dec 7, 2024
test: remove hasOpenSSL3x utils #56164
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
@tniessen tniessen tniessen approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. needs-ci PRs that need a full CI run.

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.

4 participants

@richardlau @nodejs-github-bot @lpinca @tniessen
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.