Skip to content

crypto: support OPENSSL_CONF again #11006

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

Merged
sam-github merged 2 commits into from
Feb 9, 2017
Merged

crypto: support OPENSSL_CONF again #11006

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
901e926
src: add SafeGetenv() to internal API
sam-github Jan 27, 2017
59afa27
crypto: support OPENSSL_CONF again
sam-github Jan 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
crypto: support OPENSSL_CONF again 
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 <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
@sam-github
sam-github committed Feb 9, 2017
commit 59afa275adc8c91d564bdde92390dd9341c919c8
13 changes: 13 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,18 @@ misformatted, but any errors are otherwise ignored.
Note that neither the well known nor extra certificates are used when the `ca`
options property is explicitly specified for a TLS or HTTPS client or server.

### `OPENSSL_CONF=file`
<!-- YAML
added: REPLACEME
-->

Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with `./configure
\-\-openssl\-fips`.

If the [`--openssl-config`][] command line option is used, the environment
variable is ignored.

### `SSL_CERT_DIR=dir`

If `--use-openssl-ca` is enabled, this overrides and sets OpenSSL's directory
Expand Down Expand Up @@ -421,3 +433,4 @@ equivalent to using the `--redirect-warnings=file` command-line flag.
[debugger]: debugger.html
[REPL]: repl.html
[SlowBuffer]: buffer.html#buffer_class_slowbuffer
[`--openssl-config`]: #cli_openssl_config_file
10 changes: 10 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,16 @@ asynchronous when outputting to a TTY on platforms which support async stdio.
Setting this will void any guarantee that stdio will not be interleaved or
dropped at program exit. \fBAvoid use.\fR

.TP
.BR OPENSSL_CONF = \fIfile\fR
Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with
\fB./configure \-\-openssl\-fips\fR.

If the
\fB\-\-openssl\-config\fR
command line option is used, the environment variable is ignored.

.TP
.BR SSL_CERT_DIR = \fIdir\fR
If \fB\-\-use\-openssl\-ca\fR is enabled, this overrides and sets OpenSSL's directory
Expand Down
14 changes: 10 additions & 4 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
std::string openssl_config; // NOLINT(runtime/string)
#endif // HAVE_OPENSSL

// true if process warnings should be suppressed
Expand Down Expand Up @@ -3561,8 +3561,9 @@ static void PrintHelp() {
" --enable-fips enable FIPS crypto at startup\n"
" --force-fips force FIPS crypto (cannot be disabled)\n"
#endif /* NODE_FIPS_MODE */
" --openssl-config=path load OpenSSL configuration file from\n"
" the specified path\n"
" --openssl-config=file load OpenSSL configuration from the\n"
" specified file (overrides\n"
" OPENSSL_CONF)\n"
#endif /* HAVE_OPENSSL */
#if defined(NODE_HAVE_I18N_SUPPORT)
" --icu-data-dir=dir set ICU data load path to dir\n"
Expand Down Expand Up @@ -3597,6 +3598,8 @@ static void PrintHelp() {
" file\n"
"NODE_REDIRECT_WARNINGS write warnings to path instead of\n"
" stderr\n"
"OPENSSL_CONF load OpenSSL configuration from file\n"
"\n"
"Documentation can be found at https://nodejs.org/\n");
}

Expand Down Expand Up @@ -3746,7 +3749,7 @@ static void ParseArgs(int* argc,
force_fips_crypto = true;
#endif /* NODE_FIPS_MODE */
} else if (strncmp(arg, "--openssl-config=", 17) == 0) {
openssl_config = arg + 17;
openssl_config.assign(arg + 17);
#endif /* HAVE_OPENSSL */
#if defined(NODE_HAVE_I18N_SUPPORT)
} else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
Expand Down Expand Up @@ -4246,6 +4249,9 @@ void Init(int* argc,
if (config_warning_file.empty())
SafeGetenv("NODE_REDIRECT_WARNINGS", &config_warning_file);

if (openssl_config.empty())
SafeGetenv("OPENSSL_CONF", &openssl_config);

// Parse a few arguments which are specific to Node.
int v8_argc;
const char** v8_argv;
Expand Down
4 changes: 2 additions & 2 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5893,14 +5893,14 @@ void InitCryptoOnce() {
OPENSSL_no_config();

// --openssl-config=...
if (openssl_config != nullptr) {
if (!openssl_config.empty()) {
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(),
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 9, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@sam-github sam-github Feb 9, 2017

Choose a reason for hiding this comment

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

#11272

nullptr,
CONF_MFLAGS_DEFAULT_SECTION);
int err = ERR_get_error();
Expand Down
2 changes: 1 addition & 1 deletion src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace node {

// Set in node.cc by ParseArgs with the value of --openssl-config.
// Used in node_crypto.cc when initializing OpenSSL.
extern const char* openssl_config;
extern std::string openssl_config;

// Set in node.cc by ParseArgs when --preserve-symlinks is used.
// Used in node_config.cc to set a constant on process.binding('config')
Expand Down
25 changes: 22 additions & 3 deletions test/parallel/test-crypto-fips.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
env: env
});

console.error('Spawned child [pid:' + child.pid + '] with cmd ' +
cmd + ' and args \'' + args + '\'');
console.error('Spawned child [pid:' + child.pid + '] with cmd \'' +
cmd + '\' expect %j with args \'' + args + '\'' +
' OPENSSL_CONF=%j', expectedOutput, env.OPENSSL_CONF);

function childOk(child) {
console.error('Child #' + ++num_children_ok +
Expand Down Expand Up @@ -92,10 +93,26 @@ testHelper(
compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
'require("crypto").fips',
process.env);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Feb 9, 2017

Choose a reason for hiding this comment

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

Can you add blank lines before and after for legibility?

Copy link
Copy Markdown
Contributor Author

@sam-github sam-github Feb 9, 2017

Choose a reason for hiding this comment

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

done, thanks.

// OPENSSL_CONF should _not_ be able to turn on FIPS mode

// OPENSSL_CONF should be able to turn on FIPS mode
testHelper(
'stdout',
[],
compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
'require("crypto").fips',
addToEnv('OPENSSL_CONF', CNF_FIPS_ON));

// --openssl-config option should override OPENSSL_CONF
testHelper(
'stdout',
[`--openssl-config=${CNF_FIPS_ON}`],
compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
'require("crypto").fips',
addToEnv('OPENSSL_CONF', CNF_FIPS_OFF));

testHelper(
'stdout',
[`--openssl-config=${CNF_FIPS_OFF}`],
FIPS_DISABLED,
'require("crypto").fips',
addToEnv('OPENSSL_CONF', CNF_FIPS_ON));
Expand All @@ -107,6 +124,7 @@ testHelper(
compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING,
'require("crypto").fips',
process.env);

// OPENSSL_CONF should _not_ make a difference to --enable-fips
testHelper(
compiledWithFips() ? 'stdout' : 'stderr',
Expand All @@ -122,6 +140,7 @@ testHelper(
compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING,
'require("crypto").fips',
process.env);

// Using OPENSSL_CONF should not make a difference to --force-fips
testHelper(
compiledWithFips() ? 'stdout' : 'stderr',
Expand Down