Repository: nodejs/node
Pull request: src: fix FIPS init error handling

If `--enable-fips` or `--force-fips` fails to be applied during `ProcessFipsOptions()`, the node process might exit with `ExitCode::kNoFailure` because `ERR_GET_REASON(ERR_peek_error())` can return `0` since `ncrypto::testFipsEnabled()` does not populate the OpenSSL error queue.

This is problematic because an exit code of `0` (i.e., `kNoFailure`) indicates successful execution of the `node` command, even though it already failed during initialization. (Error messages also appear to be broken, but fixing that is not as urgent as not pretending that the command succeeded.)

For example, if a user invokes `node` from within a bash script and `NODE_OPTIONS` includes `--force-fips`, the execution of the command will appear to have succeeded even if the actual user script was never executed due to a configuration error.

You can likely test this locally by running

    ./node --enable-fips ; echo $?

with the current `node` binary from the main branch if compiled without support for FIPS.

As confirmed by the `XXX` comment here, which was added three years ago in commit b6971602564fc93c536ad469947536b487c968ea, even if the error queue was populated properly, the OpenSSL reason codes are not really related to the Node.js `ExitCode` enumeration.

This commit changes the initialization logic to exit with `kGenericUserError` in this case, since such errors are most likely due to incorrect configuration. It also modifies the existing test case to actually check the exit code of the process, which would have prevented this recent regression.

Considering that the current behavior is incorrect, I'd prefer to merge this as a bugfix rather than a semver-major change.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: src/node.cc
patch: @@ -1167,10 +1167,7 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
     }
 #endif
     if (!crypto::ProcessFipsOptions()) {
-      // XXX: ERR_GET_REASON does not return something that is
-      // useful as an exit code at all.
-      result->exit_code_ =
-          static_cast<ExitCode>(ERR_GET_REASON(ERR_peek_error()));
+      result->exit_code_ = ExitCode::kGenericUserError;
       result->early_return_ = true;
       result->errors_.emplace_back(
           "OpenSSL error when trying to enable FIPS:\n" +

filename: test/parallel/test-crypto-fips.js
patch: @@ -23,13 +23,16 @@ const FIPS_ENABLE_ERROR_STRING = 'OpenSSL error when trying to enable FIPS:';
 const CNF_FIPS_ON = fixtures.path('openssl_fips_enabled.cnf');
 const CNF_FIPS_OFF = fixtures.path('openssl_fips_disabled.cnf');
 
+const kNoFailure = 0;
+const kGenericUserError = 1;
+
 let num_children_ok = 0;
 
 function sharedOpenSSL() {
   return process.config.variables.node_shared_openssl;
 }
 
-function testHelper(stream, args, expectedOutput, cmd, env) {
+function testHelper(stream, args, expectedStatus, expectedOutput, cmd, env) {
   const fullArgs = args.concat(['-e', `console.log(${cmd})`]);
   const child = spawnSync(process.execPath, fullArgs, {
     cwd: path.dirname(process.execPath),
@@ -56,6 +59,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
         // Normal path where we expect either FIPS enabled or disabled.
         assert.strictEqual(getFipsValue, expectedOutput);
     }
+    assert.strictEqual(child.status, expectedStatus);
     childOk(child);
   }
 
@@ -66,6 +70,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
 testHelper(
   testFipsCrypto() ? 'stdout' : 'stderr',
   ['--enable-fips'],
+  testFipsCrypto() ? kNoFailure : kGenericUserError,
   testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
   'process.versions',
   process.env);
@@ -74,6 +79,7 @@ testHelper(
 testHelper(
   testFipsCrypto() ? 'stdout' : 'stderr',
   ['--force-fips'],
+  testFipsCrypto() ? kNoFailure : kGenericUserError,
   testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
   'process.versions',
   process.env);
@@ -85,6 +91,7 @@ if (!sharedOpenSSL()) {
   testHelper(
     'stdout',
     [],
+    kNoFailure,
     FIPS_DISABLED,
     'require("crypto").getFips()',
     { ...process.env, 'OPENSSL_CONF': ' ' });
@@ -94,6 +101,7 @@ if (!sharedOpenSSL()) {
 testHelper(
   'stderr',
   [],
+  kGenericUserError,
   'Calling crypto.setFips() is not supported in workers',
   'new worker_threads.Worker(\'require("crypto").setFips(true);\', { eval: true })',
   process.env);
@@ -120,6 +128,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
   testHelper(
     'stdout',
     [`--openssl-config=${CNF_FIPS_ON}`],
+    kNoFailure,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
     'require("crypto").getFips()',
     process.env);
@@ -128,6 +137,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
   testHelper(
     'stdout',
     [],
+    kNoFailure,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
     'require("crypto").getFips()',
     Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -136,6 +146,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
   testHelper(
     'stdout',
     [`--openssl-config=${CNF_FIPS_ON}`],
+    kNoFailure,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
     'require("crypto").getFips()',
     Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -149,6 +160,7 @@ if (!hasOpenSSL3) {
   testHelper(
     'stdout',
     [`--openssl-config=${CNF_FIPS_OFF}`],
+    kNoFailure,
     FIPS_DISABLED,
     'require("crypto").getFips()',
     Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -157,20 +169,23 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--enable-fips', `--openssl-config=${CNF_FIPS_OFF}`],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     process.env);
   // --force-fips should take precedence over OpenSSL config file
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--force-fips', `--openssl-config=${CNF_FIPS_OFF}`],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     process.env);
   // --enable-fips should turn FIPS mode on
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--enable-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     process.env);
@@ -179,6 +194,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--force-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     process.env);
@@ -187,6 +203,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--enable-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -195,6 +212,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--force-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").getFips()',
     Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -203,6 +221,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     [],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     '(require("crypto").setFips(true),' +
     'require("crypto").getFips())',
@@ -212,6 +231,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     [],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     '(require("crypto").setFips(true),' +
     'require("crypto").setFips(false),' +
@@ -222,6 +242,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     [`--openssl-config=${CNF_FIPS_OFF}`],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     '(require("crypto").setFips(true),' +
     'require("crypto").getFips())',
@@ -231,6 +252,7 @@ if (!hasOpenSSL3) {
   testHelper(
     'stdout',
     [`--openssl-config=${CNF_FIPS_ON}`],
+    kNoFailure,
     FIPS_DISABLED,
     '(require("crypto").setFips(false),' +
     'require("crypto").getFips())',
@@ -240,6 +262,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--enable-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     '(require("crypto").setFips(false),' +
     'require("crypto").getFips())',
@@ -249,6 +272,7 @@ if (!hasOpenSSL3) {
   testHelper(
     'stderr',
     ['--force-fips'],
+    kGenericUserError,
     testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").setFips(false)',
     process.env);
@@ -257,6 +281,7 @@ if (!hasOpenSSL3) {
   testHelper(
     testFipsCrypto() ? 'stdout' : 'stderr',
     ['--force-fips'],
+    testFipsCrypto() ? kNoFailure : kGenericUserError,
     testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
     '(require("crypto").setFips(true),' +
     'require("crypto").getFips())',
@@ -266,6 +291,7 @@ if (!hasOpenSSL3) {
   testHelper(
     'stderr',
     ['--force-fips', '--enable-fips'],
+    kGenericUserError,
     testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").setFips(false)',
     process.env);
@@ -274,6 +300,7 @@ if (!hasOpenSSL3) {
   testHelper(
     'stderr',
     ['--enable-fips', '--force-fips'],
+    kGenericUserError,
     testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
     'require("crypto").setFips(false)',
     process.env);