Skip to content

Conversation

@sam-github
Copy link
Contributor

@sam-githubsam-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

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 16, 2019
@sam-githubsam-github added the tls Issues and PRs related to the tls subsystem. label Oct 16, 2019
@richardlau
Copy link
Member

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

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

@sam-github
Copy link
ContributorAuthor

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

@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member

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

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

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

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

@codebytere What's your preference?

@nodejs-github-bot
Copy link
Collaborator

@sam-githubsam-githubforce-pushed the modernize-openssl-init branch from 1b8d782 to 21859d4CompareOctober 17, 2019 18:08
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: nodejs#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
@codebytere
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member

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

@sam-github
Copy link
ContributorAuthor

@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
Copy link
ContributorAuthor

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

@sam-github
Copy link
ContributorAuthor

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

Trying again.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 19, 2019

@Trott
Copy link
Member

Landed in 8425183

@TrottTrott closed this Oct 19, 2019
Trott pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
MylesBorins pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
MylesBorins pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Oct 23, 2019
targos pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
targos pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
@targostargos mentioned this pull request Nov 10, 2019
targos pushed 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 <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
@sam-githubsam-github deleted the modernize-openssl-init branch November 28, 2019 22:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loading OpenSSL config files twice

7 participants

@sam-github@richardlau@nodejs-github-bot@codebytere@davidben@Trott@jasnell