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.

Sign up for GitHub

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

Jump to bottom

src: fix dynamically linked OpenSSL version #53456

Closed
wants to merge 4 commits into from
Closed

src: fix dynamically linked OpenSSL version #53456

wants to merge 4 commits into from

Conversation

richardlau
Copy link
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

@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
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));
Copy link
Member Author

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.

@richardlau richardlau added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jun 14, 2024
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. 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
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau richardlau marked this pull request as ready for review June 15, 2024 18:01
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 15, 2024
@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. 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
Copy link
Collaborator

Landed in 26f2cbd...3e7129e

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]>
@richardlau richardlau mentioned this pull request Jun 18, 2024
Open
@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]>
EliphazB pushed a commit to EliphazB/node that referenced this pull request Jun 20, 2024
@richardlau @EliphazB
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]>
EliphazB pushed a commit to EliphazB/node that referenced this pull request Jun 20, 2024
@richardlau @EliphazB
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]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@richardlau @bmeck
src: fix dynamically linked OpenSSL version
32df039 
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]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@richardlau @bmeck
test: check against run-time OpenSSL version
c7cf4e8 
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
Open
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

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several 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.

None yet
4 participants
@richardlau @nodejs-github-bot @lpinca @tniessen