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

[v16.x] update to openssl-1.1.1m+quic #41175

Merged
merged 3 commits into from
Jan 17, 2022

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Dec 14, 2021

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. v16.x labels Dec 14, 2021
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

RSLGTM

@richardlau
Copy link
Member Author

Looks like test.parallel/test-crypto-engine is failing on macOS 😞:
e.g. https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1015/43113/testReport/junit/(root)/test/parallel_test_crypto_engine/

node:internal/crypto/util:111
    throw new ERR_CRYPTO_ENGINE_UNKNOWN(id);
    ^

Error [ERR_CRYPTO_ENGINE_UNKNOWN]: Engine "/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/out/Release/libtest_crypto_engine.dylib" was not found
    at new NodeError (node:internal/errors:371:5)
    at Object.setEngine (node:internal/crypto/util:111:11)
    at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/test/parallel/test-crypto-engine.js:47:12)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_CRYPTO_ENGINE_UNKNOWN'
}

@richardlau richardlau changed the title [v16.x] update to openssl1.1.1m+quic [v16.x] update to openssl-1.1.1m+quic Dec 14, 2021
@richardlau
Copy link
Member Author

richardlau commented Dec 15, 2021

Looks like test.parallel/test-crypto-engine is failing on macOS 😞: e.g. https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1015/43113/testReport/junit/(root)/test/parallel_test_crypto_engine/

node:internal/crypto/util:111
    throw new ERR_CRYPTO_ENGINE_UNKNOWN(id);
    ^

Error [ERR_CRYPTO_ENGINE_UNKNOWN]: Engine "/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/out/Release/libtest_crypto_engine.dylib" was not found
    at new NodeError (node:internal/errors:371:5)
    at Object.setEngine (node:internal/crypto/util:111:11)
    at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/test/parallel/test-crypto-engine.js:47:12)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_CRYPTO_ENGINE_UNKNOWN'
}

Line 47 is the second of

crypto.setEngine(enginePath);
crypto.setEngine(enginePath);

There is reference to fixes in openssl-1.1.1m with dynamic engines in the changelog:

Changes between 1.1.1l and 1.1.1m [14 Dec 2021]
*) Avoid loading of a dynamic engine twice.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

This comment has been minimized.

@richardlau
Copy link
Member Author

Pulled over the same test fix from #41177.

@nodejs-github-bot

This comment has been minimized.

@DerekNonGeneric DerekNonGeneric added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2022
@danielleadams danielleadams force-pushed the v16.x-staging branch 2 times, most recently from f668e82 to 95c19c3 Compare January 10, 2022 23:09
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

Rebased onto current v16.x-staging.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

Rubber Stamp LGTM

Newer versions of OpenSSL now throws an error if an engine is loaded
twice by its absolute path (a second load by its id appears to be okay).

PR-URL: nodejs#41175
Refs: quictls/openssl#68
Refs: https://mta.openssl.org/pipermail/openssl-announce/2021-December/000212.html
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
This updates all sources in deps/openssl/openssl by:
    $ git clone https://github.com/quictls/openssl
    $ cd openssl
    $ git checkout OpenSSL_1_1_1m+quic
    $ cd ../node/deps/openssl
    $ rm -rf openssl
    $ cp -R ../openssl openssl
    $ rm -rf openssl/.git* openssl/.travis*
    $ git add --all openssl
    $ git commit openssl

PR-URL: nodejs#41175
Refs: quictls/openssl#68
Refs: https://mta.openssl.org/pipermail/openssl-announce/2021-December/000212.html
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
    $ make -C deps/openssl/config
    $ git add deps/openssl/config/archs
    $ git add deps/openssl/openssl/include/crypto/bn_conf.h
    $ git add deps/openssl/openssl/include/crypto/dso_conf.h
    $ git add deps/openssl/openssl/include/openssl/opensslconf.h
    $ git commit

PR-URL: nodejs#41175
Refs: quictls/openssl#68
Refs: https://mta.openssl.org/pipermail/openssl-announce/2021-December/000212.html
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@richardlau richardlau merged commit 0466400 into nodejs:v16.x-staging Jan 17, 2022
@richardlau
Copy link
Member Author

Landed in f4493c1...0466400.

@richardlau richardlau deleted the v16.x-openssl1.1.1m+quic branch January 17, 2022 12:57
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants