crypto: support OPENSSL_CONF again

sam-github · MylesBorins · commit f8da60fb2142 · 2017-05-17T21:31:58.000-04:00
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>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -328,8 +328,21 @@ malformed, 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.
+
 [emit_warning]: process.html#process_process_emitwarning_warning_name_ctor
 [Buffer]: buffer.html#buffer_buffer
 [debugger]: debugger.html
 [REPL]: repl.html
 [SlowBuffer]: buffer.html#buffer_class_slowbuffer
+[`--openssl-config`]: #cli_openssl_config_file
diff --git a/doc/node.1 b/doc/node.1
@@ -215,6 +215,16 @@ 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.
+
 
 .SH BUGS
 Bugs are tracked in GitHub Issues:
diff --git a/src/node.cc b/src/node.cc
@@ -179,7 +179,7 @@ bool no_deprecation = false;
 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
@@ -3691,7 +3691,7 @@ static void PrintHelp() {
          "  --force-fips          force FIPS crypto (cannot be disabled)\n"
 #endif  /* NODE_FIPS_MODE */
          "  --openssl-config=path load OpenSSL configuration file from the\n"
-         "                        specified path\n"
+         "                        specified file (overrides OPENSSL_CONF)\n"
 #endif /* HAVE_OPENSSL */
 #if defined(NODE_HAVE_I18N_SUPPORT)
          "  --icu-data-dir=dir    set ICU data load path to dir\n"
@@ -3726,6 +3726,7 @@ static void PrintHelp() {
 #endif
          "                        prefixed to the module search path\n"
          "NODE_REPL_HISTORY       path to the persistent REPL history file\n"
+         "OPENSSL_CONF            load OpenSSL configuration from file\n"
          "\n"
          "Documentation can be found at https://nodejs.org/\n");
 }
@@ -3860,7 +3861,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) {
@@ -4355,6 +4356,9 @@ void Init(int* argc,
   V8::SetFlagsFromString(NODE_V8_OPTIONS, sizeof(NODE_V8_OPTIONS) - 1);
 #endif
 
+  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;
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -5904,14 +5904,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(),
         nullptr,
         CONF_MFLAGS_DEFAULT_SECTION);
     int err = ERR_get_error();
diff --git a/src/node_internals.h b/src/node_internals.h
@@ -36,7 +36,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')
diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js
@@ -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 +
@@ -92,10 +93,26 @@ testHelper(
   compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
   'require("crypto").fips',
   process.env);
-// 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));
@@ -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',
@@ -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',
