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

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

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

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 57d5fe0Compare 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 efff1cfCompare 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 1661a7dCompare 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 223d032Compare 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 f440e1dCompare February 9, 2017 20:19

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

sam-github merged commit 59afa27 into nodejs:masterFeb 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

Uh oh!

crypto: support OPENSSL_CONF again#11006

crypto: support OPENSSL_CONF again #11006

Uh oh!

Conversation

sam-github commented Jan 25, 2017

Checklist

Affected core subsystem(s)

Uh oh!

sam-github commented Jan 25, 2017

Uh oh!

gibfahn commented Jan 25, 2017

Uh oh!

bnoordhuis commented Jan 25, 2017

Uh oh!

Trott commented Jan 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Jan 26, 2017

Uh oh!

sam-github commented Jan 27, 2017

Uh oh!

mhdawson commented Jan 27, 2017

Uh oh!

gibfahn commented Jan 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuisJan 28, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Feb 6, 2017

Uh oh!

sam-github commented Feb 8, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuisFeb 9, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubFeb 9, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuisFeb 9, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubFeb 9, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Trott commented Jan 26, 2017•
edited
Loading

gibfahn commented Jan 27, 2017•
edited
Loading