Always available FIPS options by khardix · Pull Request #36341 · nodejs/node · GitHub

This is continuation of #35019, rebased on current master. I have taken it over from @voxik.

Additional changes

fipsMode constant was replaced by (hopefully) internal binding to FIPS_selftest() OpenSSL function.
The binding is called testFipsCrypto() and it simply returns 1 or 0 based on the FIPS status reported by OpenSSL.
The relevant tests were adjusted to rely on this in place of the original constant.

Open problems

There is still the issue of reporting errors in InitCryptoOnce():

/* Override FIPS settings in cnf file, if needed. */
unsigned long err = 0;  // NOLINT(runtime/int)
if (per_process::cli_options->enable_fips_crypto ||
    per_process::cli_options->force_fips_crypto) {
  if (0 == FIPS_mode() && !FIPS_mode_set(1)) {
    err = ERR_get_error();
  }
}
if (0 != err) {
  fprintf(stderr,
          "openssl fips failed: %s\n",
          ERR_error_string(err, nullptr));
  UNREACHABLE();
}

The UNREACHABLE() section is not so unreachable anymore 😉.
Unfortunately, I was not able to figure out better way to report an error – anything similar to return ThrowCryptoError(env, err) requires a reference to the environment, which AFAIK is not available in the InitCryptoOnce().

Any pointers?

Fixes #34903; obsoletes/closes #35019.

Any pointers?

Would something like this work perhaps?

diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
index a238b84e7e..759e829000 100644
--- a/src/crypto/crypto_util.cc
+++ b/src/crypto/crypto_util.cc
@@ -120,10 +120,9 @@ void InitCryptoOnce() {
     }
   }
   if (0 != err) {
-    fprintf(stderr,
-            "openssl fips failed: %s\n",
-            ERR_error_string(err, nullptr));
-    UNREACHABLE();
+    Isolate* isolate = Isolate::GetCurrent();
+    Environment* env = Environment::GetCurrent(isolate);
+    return ThrowCryptoError(env, err, ERR_error_string(err, nullptr));
   }
 
   // Turn off compression. Saves memory and protects against CRIME attacks.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index d25387f142..2ce5f211fb 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -31,6 +31,7 @@ namespace node {
 using v8::Context;
 using v8::Local;
 using v8::Object;
+using v8::TryCatch;
 using v8::Value;
 
 namespace crypto {
@@ -39,10 +40,14 @@ void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
                 void* priv) {
+  Environment* env = Environment::GetCurrent(context);
+  TryCatch try_catch{env->isolate()};
   static uv_once_t init_once = UV_ONCE_INIT;
   uv_once(&init_once, InitCryptoOnce);
-
-  Environment* env = Environment::GetCurrent(context);
+  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+    try_catch.ReThrow();
+    return;
+  }
 
   AES::Initialize(env, target);
   CipherBase::Initialize(env, target);

With this patch and using --enable-fips when there is no fips support available would result in:

$ ./out/Debug/node --enable-fips -p 'crypto.constants.OPENSSL_VERSION_NUMBER'
node:internal/bootstrap/loaders:140
      mod = bindingObj[module] = getInternalBinding(module);
                                 ^

Error: error:0F06D065:common libcrypto routines:FIPS_mode_set:fips mode not supported
    at internalBinding (node:internal/bootstrap/loaders:140:34)
    at node:crypto:50:5
    at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:283:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:312:14)
    at get (node:internal/modules/cjs/helpers:158:21)
    at [eval]:1:1
    at Script.runInThisContext (node:vm:133:18)
    at Object.runInThisContext (node:vm:310:38)
    at node:internal/process/execution:77:19
    at [eval]-wrapper:6:22 {
  library: 'common libcrypto routines',
  function: 'FIPS_mode_set',
  reason: 'fips mode not supported',
  code: 'ERR_OSSL_CRYPTO_FIPS_MODE_NOT_SUPPORTED'
}

@danbev Thanks, the Isolate::GetCurrent(); Environment::GetCurrent(isolate); calls were exactly what I was looking for 🙂

Now I can get a proper look at the failing tests.

This could optionally be shortened to just be:

return ThrowCryptoError(env, err);

I assume you mean the return line can be shortened… otherwise we are back at the start with not having an environment pointer – or do I miss something?

Yeah, sorry that is what I meant, just the last line.

Cleaned the history a bit and rebased on master. If the checks pass, this should be ready for review.

CI: https://ci.nodejs.org/job/node-test-pull-request/35059/

CI: https://ci.nodejs.org/job/node-test-pull-request/35107/

CI: https://ci.nodejs.org/job/node-test-pull-request/35353/

@khardix there still seem to be a few related tests that are failing: test.parallel/test-cli-node-print-help
test.parallel/test-process-env-allowed-flags-are-documented

in https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_debug_x64/24501/

Just noting this is still on my backlog, hopefully I can get back to this within a week again.

There is no reason to hide FIPS functionality behind build flags. OpenSSL always provide the information about FIPS availability via `FIPS_mode()` function. This makes the user experience more consistent, because the OpenSSL library is always queried and the `crypto.getFips()` always returns OpenSSL settings. Fixes nodejs#34903

Signed-off-by: Jan Staněk <jstanek@redhat.com>

- The fipsMode constant (defined at compile time) was replaced by the new `TestFipsCrypto()`/`testFipsCrypto()` functions, which rely on the OpenSSL function `FIPS_selftest()`. This results in the FIPS mode being always checked on runtime and being informed purely by the OpenSSL implementation in use.

I'm currently unable to reproduce the test failures locally. I have rebased the changes on current master; let's see what the CI thinks.

@mhdawson Well, now it seems to pass; only macOS ran out of memory. Unless any changes are requested, I'm considering this ready to be merged.

Landed in f392ac0

There is no reason to hide FIPS functionality behind build flags. OpenSSL always provide the information about FIPS availability via `FIPS_mode()` function. This makes the user experience more consistent, because the OpenSSL library is always queried and the `crypto.getFips()` always returns OpenSSL settings. Fixes #34903 PR-URL: #36341 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) make FIPS related options always awailable (Vít Ondruch) #36341 errors: * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362 PR-URL: #37569

nodejs/node#36341

There is no reason to hide FIPS functionality behind build flags. OpenSSL always provide the information about FIPS availability via `FIPS_mode()` function. This makes the user experience more consistent, because the OpenSSL library is always queried and the `crypto.getFips()` always returns OpenSSL settings. Fixes nodejs#34903 Backport-PR-URL: nodejs#40241 PR-URL: nodejs#36341 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

There is no reason to hide FIPS functionality behind build flags. OpenSSL always provide the information about FIPS availability via `FIPS_mode()` function. This makes the user experience more consistent, because the OpenSSL library is always queried and the `crypto.getFips()` always returns OpenSSL settings. Fixes: #34903 Backport-PR-URL: #40241 PR-URL: #36341 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Notable changes: Corepack: Node.js now includes Corepack, a script that acts as a bridge between Node.js projects and the package managers they are intended to be used with during development. In practical terms, Corepack will let you use Yarn and pnpm without having to install them - just like what currently happens with npm, which is shipped in Node.js by default. Contributed by Maël Nison - #39608 ICU updated: ICU has been updated to 70.1. This updates timezone database to 2021a3, including bringing forward the start for DST for Jordan from March to February. Contributed by Michaël Zasso - #40658 New option to disable loading of native addons: A new command line option `--no-addons` has been added to disallow loading of native addons. Contributed by Dominic Elm - #39977 Updated Root Certificates: Root certificates have been updated to those from Mozilla's Network Security Services 3.71. Contributed by Richard Lau - #40280 Other Notable Changes: crypto: * (SEMVER-MINOR) make FIPS related options always available (Vít Ondruch) #36341 lib: * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433 * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433 module: * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635 src: * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 PR-URL: #41696

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [node](https://github.com/nodejs/node) | stage | minor | `14.18.3-bullseye-slim` -> `14.19.0-bullseye-slim` | --- ### Release Notes <details> <summary>nodejs/node</summary> ### [`v14.19.0`](https://github.com/nodejs/node/releases/v14.19.0) [Compare Source](nodejs/node@v14.18.3...v14.19.0) ##### Notable Changes ##### Corepack Node.js now includes Corepack, a script that acts as a bridge between Node.js projects and the package managers they are intended to be used with during development. In practical terms, **Corepack will let you use Yarn and pnpm without having to install them** - just like what currently happens with npm, which is shipped in Node.js by default. Please head over to the [Corepack documentation page](https://nodejs.org/dist/latest-v14.x/docs/api/corepack.html) for more information on how to use it. Contributed by Maël Nison - [#39608](nodejs/node#39608) ##### ICU updated ICU has been updated to 70.1. This updates timezone database to 2021a3, including bringing forward the start for DST for Jordan from March to February. Contributed by Michaël Zasso - [#40658](nodejs/node#40658) ##### New option to disable loading of native addons A new command line option `--no-addons` has been added to disallow loading of native addons. Contributed by Dominic Elm - [#39977](nodejs/node#39977) ##### Updated Root Certificates Root certificates have been updated to those from Mozilla's Network Security Services 3.71. Contributed by Richard Lau - [#40280](nodejs/node#40280) ##### Other Notable Changes - \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#36341](nodejs/node#36341) - \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#40433](nodejs/node#40433) - \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#40433](nodejs/node#40433) - \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#39635](nodejs/node#39635) - \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#39926](nodejs/node#39926) ##### Commits - \[[`0231ffa501`](nodejs/node@0231ffa501)] - **build**: add `--without-corepack` (Jonah Snider) [#41060](nodejs/node#41060) - \[[`5389b8ab05`](nodejs/node@5389b8ab05)] - **crypto**: update root certificates (Richard Lau) [#40280](nodejs/node#40280) - \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#36341](nodejs/node#36341) - \[[`cd20ecc7cb`](nodejs/node@cd20ecc7cb)] - **deps**: upgrade Corepack to 0.10 (Maël Nison) [#40374](nodejs/node#40374) - \[[`737df75e17`](nodejs/node@737df75e17)] - **(SEMVER-MINOR)** **deps**: add corepack (Maël Nison) [#39608](nodejs/node#39608) - \[[`b85aa5a143`](nodejs/node@b85aa5a143)] - **deps**: upgrade npm to 6.14.16 (Ruy Adorno) [#41603](nodejs/node#41603) - \[[`2755d391a5`](nodejs/node@2755d391a5)] - **deps**: update ICU to 70.1 (Michaël Zasso) [#40658](nodejs/node#40658) - \[[`3089326d89`](nodejs/node@3089326d89)] - **deps**: update archs files for OpenSSL-1.1.1m (Richard Lau) [#41173](nodejs/node#41173) - \[[`59da7c12aa`](nodejs/node@59da7c12aa)] - **deps**: upgrade openssl sources to 1.1.1m (Richard Lau) [#41173](nodejs/node#41173) - \[[`cede1f26f6`](nodejs/node@cede1f26f6)] - **deps**: add -fno-strict-aliasing flag to libuv (Daniel Bevenius) [#40631](nodejs/node#40631) - \[[`4477da858f`](nodejs/node@4477da858f)] - **doc**: fix corepack grammar for `--force` flag (Steven) [#40762](nodejs/node#40762) - \[[`5971d58600`](nodejs/node@5971d58600)] - **doc**: add missing YAML tag in `esm.md` (Antoine du Hamel) [#41516](nodejs/node#41516) - \[[`e903798ae1`](nodejs/node@e903798ae1)] - **doc**: add note regarding unfinished TLA (Antoine du Hamel) [#41434](nodejs/node#41434) - \[[`a90defebcf`](nodejs/node@a90defebcf)] - **esm**: make `process.exit()` default to exit code 0 (Gang Chen) [#41388](nodejs/node#41388) - \[[`fc328f1ab0`](nodejs/node@fc328f1ab0)] - **fs**: nullish coalescing to respect zero positional reads (Omar El-Mihilmy) [#40716](nodejs/node#40716) - \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#40433](nodejs/node#40433) - \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#40433](nodejs/node#40433) - \[[`2c365961d0`](nodejs/node@2c365961d0)] - **module**: support pattern trailers for imports field (Guy Bedford) [#40041](nodejs/node#40041) - \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#39635](nodejs/node#39635) - \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#39926](nodejs/node#39926) - \[[`b050c65885`](nodejs/node@b050c65885)] - **src**: add option to disable loading native addons (Dominic Elm) [#39977](nodejs/node#39977) - \[[`c1695ac68a`](nodejs/node@c1695ac68a)] - **tools**: update certdata.txt (Richard Lau) [#40280](nodejs/node#40280) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/97 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2020

khardix marked this pull request as draft December 1, 2020 13:25

addaleax approved these changes Dec 1, 2020

View reviewed changes

danbev reviewed Dec 15, 2020

View reviewed changes

khardix force-pushed the always-available-fips-options branch 2 times, most recently from 4f5a7de to 6725b14 Compare December 15, 2020 16:31

khardix marked this pull request as ready for review December 15, 2020 16:32

voxik suggested changes Dec 16, 2020

View reviewed changes

Comment thread src/crypto/crypto_util.h Outdated

Comment thread test/parallel/test-crypto-fips.js Outdated

khardix changed the title [WIP] Always available FIPS options Always available FIPS options Dec 18, 2020

PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020

richardlau mentioned this pull request Jan 26, 2021

FIPS 140-2 compliancy using existing certified libraries #37072

Closed

voxik and others added 4 commits February 9, 2021 17:03

Throw JavaScript exception if FIPS mode cannot be enabled

7aefc26

Signed-off-by: Jan Staněk <jstanek@redhat.com>

Comment on the meaning of OPENSSL_FIPS constant

e0380d9

khardix force-pushed the always-available-fips-options branch from b183f96 to e0380d9 Compare February 9, 2021 16:04

mhdawson reviewed Feb 10, 2021

View reviewed changes

Comment thread doc/api/cli.md Outdated

mhdawson added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 10, 2021

mhdawson closed this Feb 25, 2021

targos mentioned this pull request Mar 2, 2021

v15.11.0 release proposal #37569

Merged

mhdawson mentioned this pull request Mar 4, 2021

Make FIPS related options and functionality always available. #35019

Closed

khardix deleted the always-available-fips-options branch March 24, 2021 11:40

codebytere added a commit to electron/electron that referenced this pull request May 20, 2021

crypto: make FIPS related options always available

fe422d2

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request May 20, 2021

crypto: make FIPS related options always available

ba45759

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

crypto: make FIPS related options always available

e04726f

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021

crypto: make FIPS related options always available

f83808e

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021

crypto: make FIPS related options always available

6a36a98

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021

crypto: make FIPS related options always available

b180c9c

nodejs/node#36341

codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021

crypto: make FIPS related options always available

1e8faee

nodejs/node#36341

targos added dont-land-on-v12.x labels Aug 8, 2021

danbev mentioned this pull request Sep 28, 2021

[v14.x backport] crypto: make FIPS related options always awailable #40241

Closed

richardlau added backported-to-v14.x and removed dont-land-on-v14.x labels Nov 25, 2021

richardlau mentioned this pull request Jan 25, 2022

v14.19.0 proposal #41696

Merged

Always available FIPS options by khardix · Pull Request #36341 · nodejs/node · GitHub

Uh oh!

Uh oh!

Always available FIPS options - #36341

Always available FIPS options#36341khardix wants to merge 7 commits intonodejs:masternodejs/node:masterfrom khardix:always-available-fips-optionskhardix/node:always-available-fips-optionsCopy head branch name to clipboard

Conversation

khardix commented Dec 1, 2020

Additional changes

Open problems

Uh oh!

danbev commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khardix commented Dec 14, 2020

Uh oh!

danbev Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

khardix Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

danbev Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

khardix commented Dec 15, 2020

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 24, 2020

Uh oh!

nodejs-github-bot commented Dec 28, 2020

Uh oh!

nodejs-github-bot commented Jan 7, 2021

Uh oh!

mhdawson commented Jan 8, 2021

Uh oh!

khardix commented Jan 20, 2021

Uh oh!

khardix commented Feb 9, 2021

Uh oh!

khardix commented Feb 10, 2021

Uh oh!

Uh oh!

mhdawson commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Always available FIPS options#36341
khardix wants to merge 7 commits into
nodejs:masterfrom
khardix:always-available-fips-options

danbev commented Dec 14, 2020 •
edited
Loading

mhdawson commented Feb 25, 2021 •
edited
Loading