crypto: support OPENSSL_CONF again #11006

sam-github · 2017-01-25T22:17:58Z

A side-effect of https://github.com/nodejs/node-private/pull/82
was to remove support for OPENSSL_CONF, as well as removing the default
read of a configuration file on startup.

Partly revert this, allowing OPENSSL_CONF to be used to specify a
configuration file to read on startup, but do not read a file by
default.

If the --openssl-config command line option is provided, its value is
used, not the OPENSSL_CONF environment variable.

Fix: #10938

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto

sam-github · 2017-01-25T22:18:33Z

@gibfahn @mhdawson @bnoordhuis PTAL

gibfahn · 2017-01-25T22:49:27Z

cc/ @rvagg

bnoordhuis · 2017-01-25T23:55:29Z

../src/node.cc:179:58: error: ‘secure_getenv’ was not declared in this scope
 const char* openssl_config = secure_getenv("OPENSSL_CONF");

Trott · 2017-01-26T01:12:50Z

Mostly cosmetic, but #10948 fixes misaligned options displayed by node.cc file. Might be worth taking a few seconds now to make sure the changes here are compatible with the changes there to avoid a merge conflict or misaligned options in the near future.

mhdawson · 2017-01-26T16:30:44Z

Will take another look once ci failures are resolved, but general concept looks good.

sam-github · 2017-01-27T19:45:02Z

I see, its because I'm not building with FIPS that I didn't notice that.

@gibfahn (or anybody), how do I compile with FIPS?

mhdawson · 2017-01-27T20:11:10Z

Latest ci run: https://ci.nodejs.org/job/node-test-pull-request/6080/

gibfahn · 2017-01-27T20:34:59Z

@sam-github You need to give a path to a fips-compliant openssl version of fips in the configure stage with --openssl-fips=/path/to/openssl, see the ./configure option.

Then when you run you pass --enable-fips or --force-fips

mhdawson

CI green, LGTM from me.

bnoordhuis · 2017-01-28T11:28:46Z

src/node.cc

@@ -176,7 +176,7 @@ bool ssl_openssl_cert_store =
 bool enable_fips_crypto = false;
 bool force_fips_crypto = false;
 # endif  // NODE_FIPS_MODE
-const char* openssl_config = nullptr;
+const char* openssl_config = secure_getenv("OPENSSL_CONF");


This is not safe, modifications to the environment will invalidate the pointer. Call secure_getenv() only when openssl_config is first needed and store a copy, not a pointer.

I see we're also mishandling icu_data_dir that way. I'll file a pull request.

gibfahn · 2017-02-06T23:12:39Z

@sam-github I see that @bnoordhuis has raised #11051, so presumably this PR will be dependent on it?

sam-github · 2017-02-08T20:32:14Z

I figured out how to do a FIPS build locally, and the tests pass for me. Waiting for ci: https://ci.nodejs.org/job/node-test-pull-request/6295/

@bnoordhuis PTAL

bnoordhuis

LGTM with style suggestions.

bnoordhuis · 2017-02-09T06:31:32Z

src/node_crypto.cc

    OPENSSL_load_builtin_modules();
 #ifndef OPENSSL_NO_ENGINE
    ENGINE_load_builtin_engines();
 #endif
    ERR_clear_error();
    CONF_modules_load_file(
-        openssl_config,
+        openssl_config.c_str(),


You can use .data() in C++11, it's interchangeable with .c_str() now (and one keystroke shorter.)

bnoordhuis · 2017-02-09T06:33:21Z

test/parallel/test-crypto-fips.js

@@ -92,10 +93,24 @@ testHelper(
  compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
  'require("crypto").fips',
  process.env);
-// OPENSSL_CONF should _not_ be able to turn on FIPS mode


Can you add blank lines before and after for legibility?

done, thanks.

Allow it to be used anywhere in src/ that env variables with security implications are accessed.

sam-github · 2017-02-09T20:24:57Z

ci: https://ci.nodejs.org/job/node-test-pull-request/6324/

A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: nodejs#10938 PR-URL: nodejs#11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [nodejs#11489](nodejs#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [nodejs#11006](nodejs#11006) * src: adding support for trace-event tracing (misterpoe) [nodejs#11106](nodejs#11106) PR-URL: nodejs#11553

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [#11288](nodejs/node#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [#11562](nodejs/node#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [#11275](nodejs/node#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [#11005](nodejs/node#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [#11236](nodejs/node#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [#11489](nodejs/node#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [#11006](nodejs/node#11006) * src: adding support for trace-event tracing (misterpoe) [#11106](nodejs/node#11106) PR-URL: nodejs/node#11553 Signed-off-by: Ilkka Myller <[email protected]>

Allow it to be used anywhere in src/ that env variables with security implications are accessed. PR-URL: nodejs#11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: nodejs#10938 PR-URL: nodejs#11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Allow it to be used anywhere in src/ that env variables with security implications are accessed. PR-URL: #11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: #10938 PR-URL: #11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Allow it to be used anywhere in src/ that env variables with security implications are accessed. PR-URL: #11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: #10938 PR-URL: #11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059

A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: nodejs/node#10938 PR-URL: nodejs/node#11006 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) nodejs/node#10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - ability to select cert store at runtime (Adam Majer) nodejs/node#8334 - Use system CAs instead of using bundled ones (Adam Majer) nodejs/node#8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 - adding support for OPENSSL_CONF again (Sam Roberts) nodejs/node#11006 - make LazyTransform compabile with Streams1 (Matteo Collina) nodejs/node#12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) nodejs/node#11094 - upgrade libuv to 1.10.2 (cjihrig) nodejs/node#10717 - upgrade libuv to 1.10.1 (cjihrig) nodejs/node#9647 - upgrade libuv to 1.10.0 (cjihrig) nodejs/node#9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) nodejs/node#9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) nodejs/node#10842 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - support "--" after "-e" as end-of-options (John Barboza) nodejs/node#10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) nodejs/node#11005 - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 PR-URL: nodejs/node#13059

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v7.x labels Jan 25, 2017

sam-github mentioned this pull request Jan 25, 2017

Allow enabling FIPS mode from an environment variable #10938

Closed

sam-github force-pushed the ossl-cfg-from-env branch from 1760079 to 57d5fe0 Compare January 25, 2017 22:27

nodejs-github-bot added the dont-land-on-v7.x label Jan 25, 2017

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jan 26, 2017

mscdex removed the dont-land-on-v7.x label Jan 27, 2017

sam-github force-pushed the ossl-cfg-from-env branch from 57d5fe0 to efff1cf Compare January 27, 2017 19:52

mhdawson approved these changes Jan 27, 2017

View reviewed changes

bnoordhuis requested changes Jan 28, 2017

View reviewed changes

sam-github force-pushed the ossl-cfg-from-env branch from efff1cf to 1661a7d Compare February 8, 2017 18:53

sam-github mentioned this pull request Feb 8, 2017

src: make copies of startup environment variables #11051

Closed

sam-github force-pushed the ossl-cfg-from-env branch from 1661a7d to 223d032 Compare February 8, 2017 20:29

bnoordhuis approved these changes Feb 9, 2017

View reviewed changes

src: add SafeGetenv() to internal API

901e926

Allow it to be used anywhere in src/ that env variables with security implications are accessed.

sam-github force-pushed the ossl-cfg-from-env branch from 223d032 to f440e1d Compare February 9, 2017 20:19

sam-github force-pushed the ossl-cfg-from-env branch from f440e1d to 59afa27 Compare February 9, 2017 21:04

sam-github merged commit 59afa27 into nodejs:master Feb 9, 2017

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: support OPENSSL_CONF again #11006

crypto: support OPENSSL_CONF again #11006

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

gibfahn commented Jan 25, 2017

bnoordhuis commented Jan 25, 2017

Trott commented Jan 26, 2017 •

edited

Loading

mhdawson commented Jan 26, 2017

sam-github commented Jan 27, 2017

mhdawson commented Jan 27, 2017

gibfahn commented Jan 27, 2017 •

edited

Loading

mhdawson left a comment

bnoordhuis Jan 28, 2017

gibfahn commented Feb 6, 2017

sam-github commented Feb 8, 2017

bnoordhuis left a comment

bnoordhuis Feb 9, 2017

sam-github Feb 9, 2017

bnoordhuis Feb 9, 2017

sam-github Feb 9, 2017

sam-github commented Feb 9, 2017

crypto: support OPENSSL_CONF again #11006

crypto: support OPENSSL_CONF again #11006

Conversation

sam-github commented Jan 25, 2017

Checklist

Affected core subsystem(s)

sam-github commented Jan 25, 2017

gibfahn commented Jan 25, 2017

bnoordhuis commented Jan 25, 2017

Trott commented Jan 26, 2017 • edited Loading

mhdawson commented Jan 26, 2017

sam-github commented Jan 27, 2017

mhdawson commented Jan 27, 2017

gibfahn commented Jan 27, 2017 • edited Loading

mhdawson left a comment

Choose a reason for hiding this comment

bnoordhuis Jan 28, 2017

Choose a reason for hiding this comment

gibfahn commented Feb 6, 2017

sam-github commented Feb 8, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Feb 9, 2017

Choose a reason for hiding this comment

sam-github Feb 9, 2017

Choose a reason for hiding this comment

bnoordhuis Feb 9, 2017

Choose a reason for hiding this comment

sam-github Feb 9, 2017

Choose a reason for hiding this comment

sam-github commented Feb 9, 2017

Trott commented Jan 26, 2017 •

edited

Loading

gibfahn commented Jan 27, 2017 •

edited

Loading