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

Loading OpenSSL config files twice #29702

Closed
OYTIS opened this issue Sep 25, 2019 · 2 comments
Closed

Loading OpenSSL config files twice #29702

OYTIS opened this issue Sep 25, 2019 · 2 comments

Comments

@OYTIS
Copy link
Contributor

OYTIS commented Sep 25, 2019 •
edited
Loading

Problem

When OPENSSL_CONF environment variable is set, config files are loaded twice.

Reason

InitCryptoOnce function in node_crypto.cc first calls SSL_load_error_strings() which is a macro for OPENSSL_init_ssl() which in turn will at some point will call CONF_modules_load_file() for the file pointed to by OPENSSL_CONF environment variable. Then in the same function (InitCryptoOnce()) CONF_modules_load_file() will be called again for the file pointed to by openssl_config command line option.

But upon nodejs initialization, if openssl_config is not given, and if OPENSSL_CONF is set, openssl_config is set to the value in OPENSSL_CONF. As a result CONF_modules_load_file() will be called twice.

Impact

Normally none, except for cases when the config is not idempotent, as is the case e.g. when dynamic engines need to be loaded. In which case it is also not a huge deal, but the user will see a scary error message:

openssl config failed: error:26078067:engine routines:engine_list_add:conflicting engine id

Not sure what would be the best fix, probably it's also not too urgent.
@sam-github
Copy link
Contributor

Background: https://mta.openssl.org/pipermail/openssl-users/2019-October/011303.html

Node.js didn't used to load a conf file. Then it added specific support to load it if the OPENSSL_CONF env var was there, and now that is likely not necessary.

Also, looking at above, its likely that Node.js no longer needs to be doing (itself) one time init of libssl, and Node.js master does not support openssl < 1.1.1, so we can delete code specific to older openssls.

All of which is to say that it looks like some thinking about how things should work now needs doing, and possibly is mostly deleting of code.

I'll take a look.

@sam-github
Copy link
Contributor

AFAICT, we are just calling the same init functions over and over via various deprecated API compatibility wrappers. I've a fix building, I'll PR it once it passes locally.

@sam-github sam-github mentioned this issue Oct 16, 2019
Closed
3 tasks
sam-github added a commit to sam-github/node that referenced this issue Oct 17, 2019
@sam-github
src: only initialize openssl once
12cbdcb 
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
@Trott Trott closed this as completed in 8425183 Oct 19, 2019
MylesBorins pushed a commit that referenced this issue Oct 23, 2019
@sam-github @MylesBorins
src: initialize openssl only once
d16d673 
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 pushed a commit that referenced this issue Oct 23, 2019
@sam-github @MylesBorins
src: initialize openssl only once
b019ccd 
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 pushed a commit that referenced this issue Nov 8, 2019
@sam-github @targos
src: initialize openssl only once
8d8ef16 
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 pushed a commit that referenced this issue Nov 10, 2019
@sam-github @targos
src: initialize openssl only once
70c46d1 
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 pushed a commit that referenced this issue Nov 11, 2019
@sam-github @targos
src: initialize openssl only once
aa0aacb 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: only initialize openssl once sam-github/node
2 participants
@sam-github @OYTIS