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

deps,build: add OpenSSL building of legacy module #40466

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Oct 15, 2021

This commit adds a configuration time flag to enable OpenSSL legacy
module to be built.

For example, the following will build the legacy module:

$ ./configure --openssl-legacy-module

To enable the default provider one has currently has to update the
OpenSSL configuration file, openssl.cnf:

[openssl_init]
providers = provider_sect

[provider_sect]
default = default_sect
legacy = legacy_sect

[default_sect]
activate = 1

[legacy_sect]
activate = 1

This module can then be used by specifying the environment variable
OPENSSL_MODULES like this:

$ env OPENSSL_MODULES=$PWD/out/Release/obj.target/deps/openssl/lib/openssl-modules OPENSSL_CONF=out/Release/obj.target/deps/openssl/openssl.cnf ./node -p 'crypto.createHash("md4")'
Hash {
  _options: undefined,
  [Symbol(kHandle)]: Hash {},
  [Symbol(kState)]: { [Symbol(kFinalized)]: false }

Refs: #40455

@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. labels Oct 15, 2021
@targos
Copy link
Member

targos commented Oct 15, 2021

Does it allow to use md4 again by default?

@danbev danbev marked this pull request as draft October 15, 2021 08:57
@danbev
Copy link
Contributor Author

danbev commented Oct 15, 2021

Does it allow to use md4 again by default?

No, but I'm currently looking into enabling/configuring the legacy provider which does support md4. I've not got that part working yet though so I'll convert this into a draft pr.

@tniessen
Copy link
Member

Thank you @danbev!

@danbev danbev force-pushed the legacy_provider_sources branch from e1ef8f3 to 71dca2f Compare October 15, 2021 16:21
@danbev danbev changed the title deps: add missing legacyprov.c source deps,build: add OpenSSL building of legacy module Oct 15, 2021
@nodejs-github-bot
Copy link
Collaborator

@danbev danbev marked this pull request as ready for review October 15, 2021 17:03
@danbev
Copy link
Contributor Author

danbev commented Oct 15, 2021

Does it allow to use md4 again by default?

I'm afraid this does not allow the legacy provider to be enabled by default, instead the user would need to go through the steps above in the PR description.

@@ -27216,8 +27216,8 @@ unless (caller) {
use File::Copy;
use Pod::Usage;

use lib '/home/danielbevenius/work/nodejs/openssl/deps/openssl/openssl/util/perl';
use OpenSSL::fallback '/home/danielbevenius/work/nodejs/openssl/deps/openssl/openssl/external/perl/MODULES.txt';
use lib '/home/danielbevenius/work/nodejs/node/deps/openssl/openssl/util/perl';
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing it does not make sense for this to have danielbevenius in the path. Same for the next line.

Copy link
Member

@mhdawson mhdawson Oct 15, 2021

Choose a reason for hiding this comment

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

Although that may be a more general comment since it's not a change in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken a look at this and I'm not sure how I can prevent this in Perl. I'll take another look but I don't think this is critical as my understanding is that after we generate this file we only use it to parse out values. Still I'd rather fix this but not sure how at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

I assume the paths with be more generic (but still absolute) if regenerating the config via the Docker file?

node/Makefile

Lines 1479 to 1481 in ed01811

gen-openssl: ## Generate platform dependent openssl files (requires docker)
docker build -t node-openssl-builder deps/openssl/config/
$(DOCKER_COMMAND) node-openssl-builder make -C deps/openssl/config

This commit adds a configuration time flag to enable OpenSSL legacy
module to be built.

For example, the following will build the legacy module:

$ ./configure --openssl-legacy-module

To enable the default provider one has currently has to update the
OpenSSL configuration file, openssl.cnf:

[openssl_init]
providers = provider_sect

[provider_sect]
default = default_sect
legacy = legacy_sect

[default_sect]
activate = 1

[legacy_sect]
activate = 1

This module can then be used by specifying the environment variable
OPENSSL_MODULES like this:

$ env OPENSSL_MODULES= \
$PWD/out/Release/obj.target/deps/openssl/lib/openssl-modules \
OPENSSL_CONF=out/Release/obj.target/deps/openssl/openssl.cnf \
./node -p 'crypto.createHash("md4")'
Hash {
  _options: undefined,
  [Symbol(kHandle)]: Hash {},
  [Symbol(kState)]: { [Symbol(kFinalized)]: false }

Refs: nodejs#40455
This commit regenerates the OpenSSL architecture files after the update
in Commmit 04326f3 ("deps: add missing
legacyprov.c source").
@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2021

#40478 has been opened and contains a suggestion by @richardlau to statically link the legacy provider and allow it to be enabled by a command line flag instead of dynamically linking which is done in this pull request.

@danbev
Copy link
Contributor Author

danbev commented Oct 18, 2021

I'm closing this in favor of #40478.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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