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

src: only initialize openssl once #29999

Closed
wants to merge 1 commit into from

Conversation

@sam-github
Copy link
Member

sam-github commented Oct 16, 2019

For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Oct 16, 2019

For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

I've added a dont-land-on-v10.x label based on the above. Feel free to remove if incorrect.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Oct 16, 2019

Is there anyone we should ping from Electron in case this affects their use of BoringSSL?

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Oct 16, 2019

@danbev @codebytere This affects OpenSSL initialization, you might want to take a look.

src/node_crypto.cc Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Oct 17, 2019

OPENSSL_INIT_set_config_filename is incompatible with BoringSSL, as is OPENSSL_INIT_free it seems :( Is there a potential alternative that doesn't use methods not exposed by BoringSSL?

cc @davidben for potential ideas :)

@davidben

This comment has been minimized.

Copy link
Contributor

davidben commented Oct 17, 2019

Stub versions of those functions in BoringSSL would be just fine. We only add compatibility functions as we need them and this is the first time I've ever seen anyone use OPENSSL_INIT_SETTINGS.

If it's off in a corner, clearly a no-op, and doesn't affect anything else, we generally just freely add compatibility functions.

@davidben

This comment has been minimized.

Copy link
Contributor

davidben commented Oct 17, 2019

We would, of course, ignore whatever config file you pass in, but that's totally self-consistent. BoringSSL has zero config file options so we vacuously "parse" the file. :-)

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Oct 17, 2019

I don't mind 1b8d782 if its going to be a while before BoringSSL implements the compat APIs.

@codebytere What's your preference?

@nodejs-github-bot

This comment has been minimized.

@sam-github sam-github force-pushed the sam-github:modernize-openssl-init branch from 1b8d782 to 21859d4 Oct 17, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html
@sam-github sam-github force-pushed the sam-github:modernize-openssl-init branch from 21859d4 to 12cbdcb Oct 17, 2019
@richardlau richardlau mentioned this pull request Oct 17, 2019
2 of 2 tasks complete
@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Oct 17, 2019

@sam-github i can handle this on the BoringSSL side, so all clear 🚀 ty for tagging me in!

@nodejs-github-bot

This comment has been minimized.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Oct 17, 2019

Failing test es-module/test-esm-specifiers looks unrelated?

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Oct 17, 2019

@nodejs/modules-active-members Any comment on above? Is that test flaky? It doesn't have any crypto dependency I can think of, am I missing something?

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Oct 17, 2019

FTR:

=== release test-esm-specifiers ===
272Path: es-module/test-esm-specifiers
273--- stderr ---
274(node:31666) ExperimentalWarning: The ESM module loader is experimental.
275internal/modules/cjs/loader.js:1039
276      internalBinding('errors').triggerUncaughtException(
277                                ^
278
279AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
280
281'esm' !== 'cjs'
282
283    at file:///home/travis/build/nodejs/node/test/es-module/test-esm-specifiers.mjs:17:8
284    at ModuleJob.run (internal/modules/esm/module_job.js:109:37)
285    at async Loader.import (internal/modules/esm/loader.js:132:24) {
286  generatedMessage: true,
287  code: 'ERR_ASSERTION',
288  actual: 'esm',
289  expected: 'cjs',
290  operator: 'strictEqual'
291}
292Command: out/Release/node --experimental-modules --es-module-specifier-resolution=node /home/travis/build/nodejs/node/test/es-module/test-esm-specifiers.mjs
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Oct 17, 2019

Failing test es-module/test-esm-specifiers looks unrelated?

It's probably some interaction between ccache and #29974 landing on master (Travis runs on refs/pull/29999/merge created by GitHub). I'll clear the cache for this PR and restart the Travis job.

Edit: and everything now passed 🎉.

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Oct 18, 2019

Resume failed on test.parallel/test-worker-prof in windows, @nodejs/platform-windows @nodejs/workers

Trying again.

@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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Oct 19, 2019

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 19, 2019

Landed in 8425183

@Trott Trott closed this Oct 19, 2019
Trott added a commit that referenced this pull request Oct 19, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos added a commit that referenced this pull request Nov 8, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
targos added a commit that referenced this pull request Nov 11, 2019
For compatibility with OpenSSL 1.1.0 and 1.0.1 a series of
initialization wrappers were being called, many deprecated, and many
calling each other internally already. Compatibility is unnecessary in
12.x and later, which support only OpenSSL 1.1.1, and the multiple calls
cause the configuration file to be loaded multiple times.

Fixes: #29702

See:
- https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html
- https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

PR-URL: #29999
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@sam-github sam-github deleted the sam-github:modernize-openssl-init branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.