crypto: Improve control of FIPS mode #5181

stefanmb · 2016-02-10T19:12:28Z

In Issue #3819 requests were made to support a FIPS OpenSSL build of Node.js running in a non-FIPS mode. In PR #3820 an attempt was made to introduce this feature.

In this PR I've attempted to merge all requests and discussion from #3819 and #3820.

The following features are introduced:

Default to FIPS off even in FIPS builds: FIPS mode now has to be explicitly enabled by command line argument, OpenSSL config file, or JavaScript API call.
Add JS API to check and control FIPS mode: Allow JavaScript applications to check if they are executing in FIPS mode and to enable/disable FIPS mode if applicable.
Add command line arguments to force FIPS on/off: --[disable|enable]-fips have been added, and they will override all other options.
Respect OPENSSL_CONF variable and read the config: OpenSSL provides a standard mechanism for reading a config file from the "OPENSSL_CONF" environment variable. Load this config file, which permits, among other things, to control FIPS mode.
Add testing for new features: Test combinations of features from 1 and 4.

As always, I'm open to any suggestions and improvements, especially if there is a better way to pass global options such as the enable/disable flags (instead of extern C variables).

These features were added as a result of discussion in #3819 and #3820, please refer to them for background information.

Note also that going forward we will need to run regression testing in FIPS builds twice: once with FIPS enabled at runtime, and once with FIPS disabled.

stefanmb · 2016-02-10T19:14:34Z

@mhdawson @indutny @shigeki @jasnell @lordjabez @ScarletTanager This PR should include suggestions from all of you, any comments are greatly appreciated. Thanks!

mscdex · 2016-02-10T19:24:26Z

Maybe defining a getter/setter simply called fips would be simpler instead of having separate methods?

mhdawson · 2016-02-10T19:38:09Z

Added semver-major tag since it changes default behaviour

indutny · 2016-02-10T19:38:18Z

Sign in to view

@@ -645,6 +647,13 @@ exports.getCurves = function() {
  return filterDuplicates(getCurves());
 };

+exports.hasFipsCrypto = function() {


Nit: function hasFipsCrypto

indutny · 2016-02-10T19:38:22Z

Sign in to view

+  return hasFipsCrypto();
+};
+
+exports.setFipsCrypto = function(mode) {


indutny · 2016-02-10T19:38:36Z

Sign in to view

-#endif
+#if NODE_FIPS_MODE
+    } else if (strcmp(arg, "--disable-fips") == 0) {
+    disable_fips_crypto = true;


Nit: indent.

indutny · 2016-02-10T19:38:39Z

Sign in to view

+    } else if (strcmp(arg, "--disable-fips") == 0) {
+    disable_fips_crypto = true;
+    } else if (strcmp(arg, "--enable-fips") == 0) {
+    enable_fips_crypto = true;


indutny · 2016-02-10T19:39:42Z

Sign in to view

+  bool mode = args[0]->BooleanValue();
+  if (disable_fips_crypto) {
+    return env->ThrowError(
+     "Cannot set FIPS mode, it was forced with --disable-fips at startup.");


Nit: indent, does make cpplint pass?

Sadly yes, reports 0 errors found. I'll fix all this stuff.

Yikes, too bad for it. Thank you! 👍

indutny · 2016-02-10T19:39:45Z

Sign in to view

+     "Cannot set FIPS mode, it was forced with --disable-fips at startup.");
+  } else if (enable_fips_crypto) {
+    return env->ThrowError(
+     "Cannot set FIPS mode, it was forced with --enable-fips at startup.");


indutny · 2016-02-10T19:40:24Z

Sign in to view

@@ -93,7 +93,7 @@ for (var i in TEST_CASES) {

  (function() {
    if (!test.password) return;
-    if (common.hasFipsCrypto) {
+    if (crypto.hasFipsCrypto()) {


Let's make common.hasFipsCrypto a getter instead, should save us from fixing all tests and make them compatible to older node.js versions.

indutny · 2016-02-10T19:40:48Z

Sign in to view

+}
+
+function getResponse(data)
+{


Nit: style, please put brace on the previous line. Does it pass make lint?

It does :( I'll resolve the style problems.

indutny · 2016-02-10T19:41:03Z

Sign in to view

+const FIPS_DISABLED = 0;
+const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode';
+const OPTION_ERROR_STRING = 'bad option';
+const CNF_FIPS_ON = common.fixturesDir + '/openssl_fips_enabled.cnf';


Nit: Let's use path.join() here.

indutny · 2016-02-10T19:41:26Z

Sign in to view

+
+function getResponse(data)
+{
+  return data.toString().replace('\n', '').replace('>', '').trim();


.replace(/\n|>/g, '').trim()?

indutny · 2016-02-10T19:42:00Z

Sign in to view

+}
+
+function childOk(child) {
+  console.log('Child ' +  ++num_children_ok + '/' + num_children_spawned +


It should be either console.error or nothing at all. Doesn't play well with TAP output.

indutny · 2016-02-10T19:42:05Z

Sign in to view

+    env: env
+  });
+
+  console.log('Spawned child [pid:' + child.pid + '] with cmd ' +


indutny · 2016-02-10T19:43:34Z

Sign in to view

+    child.stdin.setEncoding('utf-8');
+    child.stdout.on('data', function(data) {
+      // Prompt and newline may occur in undefined order.
+      const response = getResponse(data);


Perhaps it should wait until end event to accumulate all data? I'm afraid of the situation where stdout could be chunked.

Also, this way it will be possible able to share lots of code between fips and non-fips branches in this function.

indutny · 2016-02-10T19:44:47Z

Sign in to view

+      const response = getResponse(data);
+      if (response.length > 0) {
+        assert.notEqual(-1, response.indexOf(string));
+        childOk(child);


Possibly this will be called several times as mentioned above.

lordjabez · 2016-02-10T19:45:36Z

This is a great change, and addresses all my concerns. Thanks!

indutny · 2016-02-10T19:45:40Z

Overall I have really good impression from the PR. Thank you for your work!

mhdawson · 2016-02-10T19:53:51Z

In terms of the command line options I'm wondering about the case were we want it enabled by default but then the application can turn on/off. I also wonder about the need to prevent an app from turning it on. Maybe the following would make sense ?

--enable-fips - enables FIPs, overrides setting in openssl file
--force-fips - enables FIPs. overrides seeting in openss file, app cannot disable

stefanmb · 2016-02-10T23:54:46Z

@mscdex I've switched to using a "crypto.fips" property as you suggested, thanks!
@indutny Thanks for the super quick feedback. I think I've addressed most of your concerns, I now use the 'end' event to validate output and 'data' event to buffer it. The handler is now shared between the FIPS and non-FIPS test branches. I'll double check the formatting issues.
@mhdawson I will remove --disable-fips and add a --force-fips tomorrow, as per our discussion there is no point in having disable-fips since that's the new default, instead we should provide a way to turn it on by default (--enable-fips) and a way to force it (--force-fips, does not allow it to be turned off).

stefanmb · 2016-02-23T03:06:30Z

@mhdawson @indutny I've updated this PR to resolve issues encountered while attempting to setup the CI:

test/common.js still used the stale API (crypto.hasFipsCrypto instead of fips.crypto)
I've updated the testing to survive a polluted environment (i.e. one where OPENSSL_CONF may already be defined, note that OpenSSL will not allow you to set the FIPS mode to on if it's already on, an exception will be thrown).
During CI for a FIPS build we must now run the test suite twice, once with FIPS enabled and once with FIPS disabled. One way to do so is to use the OPENSSL_CONF environment variable, however this will cause test failures for those tests that use opensslCli from test/common.js because the OPENSSL_CONF variable will affect the standalone OpenSSL utilities. We could filter out the OPENSSL_CONF variable from the environment in common.js, but this does not strike me as a clean solution. The alternative is to pass "--enable-fips" to each test, however there was no obvious way to pass the --enable-fips argument through the "make test-ci" target. The "--special-command" argument comes close, but is only able to suffix or prefix the entire invocation. I have made a separate PR to deal with this problem since it outside the scope of this PR: #5376

mhdawson · 2016-02-25T19:51:04Z

LTGM to latest changes, and successful test run in both fiips/non-fips enabled in updated test here: https://ci.nodejs.org/job/node-test-commit-linux-fips-dawson/

Will sync landing this and updating the test job.

One last regular CI run so that we have a current one: https://ci.nodejs.org/job/node-test-pull-request/1752/

mhdawson · 2016-02-25T20:19:10Z

CI run is green, landing

Default to FIPS off even in FIPS builds. Add JS API to check and control FIPS mode. Add command line arguments to force FIPS on/off. Respect OPENSSL_CONF variable and read the config. Add testing for new features. Fixes: #3819 PR-URL: #5181 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2016-02-25T20:26:23Z

Landed as 7c48cb5

Updated fits test, CI run here to validate test is updated ok: https://ci.nodejs.org/job/node-test-commit/2345/

mhdawson · 2016-02-25T20:54:14Z

CI all good closing

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

This was referenced Feb 10, 2016

Add a check of the OPENSSL_FIPS environment variable around each call… #3820

Closed

Node 4.2: Unable to run Node in non-FIPS mode if compiled with FIPS support #3819

Closed

mscdex added the crypto label Feb 10, 2016

stefanmb force-pushed the stefanmb:fips-switch branch Feb 10, 2016

mhdawson added the semver-major label Feb 10, 2016