Skip to content

src: fix FIPS init error handling #58379

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tniessen
Copy link
Member

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 b697160, 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.

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.

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 b697160, even if the error
queue was populated properly, the OpenSSL reason codes are not really
related to the Node.js `ExitCode` enumeration.
@tniessen tniessen added the openssl Issues and PRs related to the OpenSSL dependency. label May 18, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 18, 2025
Copy link

codecov bot commented May 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (9eb9c26) to head (04b98a7).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58379      +/-   ##
==========================================
+ Coverage   90.23%   90.25%   +0.01%     
==========================================
  Files         633      633              
  Lines      186871   186880       +9     
  Branches    36687    36687              
==========================================
+ Hits       168626   168661      +35     
+ Misses      11046    11025      -21     
+ Partials     7199     7194       -5     
Files with missing lines Coverage Δ
src/node.cc 73.34% <100.00%> (-0.04%) ⬇️

... and 49 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@tniessen
Copy link
Member Author

Only CI faiure is due to #58353.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@gurgunday gurgunday added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2025
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants