[v14.x backport] crypto: make FIPS related options always awailable by danbev · Pull Request #40241 · nodejs/node

danbev · 2021-09-28T09:51:09Z

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

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>

nodejs-github-bot · 2021-09-28T09:52:56Z

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

I made a mistake when resolving the conflict in this test and this commmit fixes it.

mhdawson · 2021-09-28T13:19:08Z

doc/api/cli.md

 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`.
+used to enable FIPS-compliant crypto if Node.js is built


Will ./configure --openssl-fips still be accepted. Want to understand if this could break existing workflows/builds

Yeah, it will still be accepted at configuration time to specify the directory where the FIPS fipscanister.o is located.

@mhdawson The ubi81_sharedlibs_openssl111fips_x64 CI job runs with configure --openssl-is-fips (we didn't update the job when #36341 landed).

Thanks for the quick confirmation.

ah sorry confused --open-is-fips with --openssl-fips configure options.

I don't think --openssl-fips works before on Node.js 14 based on configure.py?

node/configure.py

Lines 1382 to 1383 in 95b9240

if options.openssl_fips or options.openssl_fips == '':

error('FIPS is not supported in this version of Node.js')

(--openssl-is-fips does work and is tested in the CI.)

If I recall correctly it is not possible to enable FIPS with the OpenSSL version that is statically linked with Node.js. This is still the case with upstream/master.
But it is possible to dynamically link to an OpenSSL version that does support FIPS (which is the use case at Red Hat).

@danbev that is my understanding as well.

mhdawson · 2021-09-28T13:59:10Z

src/node_crypto.cc

  settings = nullptr;
 #endif

-#ifdef NODE_FIPS_MODE


There were changes in the original PR that don't seem to have made it into this block. In crypto_util.cc ->

diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 34058d2dcae9..0d44d0bb4e16 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -142,10 +142,9 @@ void InitCryptoOnce() { } } if (0 != err) { - fprintf(stderr, - "openssl fips failed: %s\n", - ERR_error_string(err, nullptr)); - UNREACHABLE(); + auto* isolate = Isolate::GetCurrent(); + auto* env = Environment::GetCurrent(isolate); + return ThrowCryptoError(env, err); }

@danbev was that intentional?

That was not intentional 😞 I'll take a closer look add it tomorrow. Thanks!

I've pushed a commit with the missing code now, please take a look.

This commit adds code that I missed when backporting.

Fix lint issue.

nodejs-github-bot · 2021-09-29T06:04:32Z

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

mhdawson

LGTM

mhdawson · 2021-09-30T22:19:02Z

One more comment there is a typo in the original commit comment. Assume we can fix up when landing.

nodejs-github-bot · 2021-11-24T16:29:23Z

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

nodejs-github-bot · 2021-11-24T22:51:32Z

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

nodejs-github-bot · 2021-11-25T00:26:08Z

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

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>

richardlau · 2021-11-25T12:12:27Z

Landed in 060c962.

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>

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. needs-ci PRs that need a full CI run. v14.x labels Sep 28, 2021

danbev force-pushed the backport-f392ac0bbe-to-v14.x branch from 209db1c to 2b35f7b Compare September 28, 2021 09:52

squash! crypto: make FIPS related options always awailable

9f7f84c

I made a mistake when resolving the conflict in this test and this commmit fixes it.

mhdawson reviewed Sep 28, 2021

View reviewed changes

github-actions bot mentioned this pull request Sep 29, 2021

CI Reliability 2021-09-29 nodejs/reliability#72

Open

18 tasks

danbev added 2 commits September 29, 2021 05:48

squash! crypto: make FIPS related options always awailable

c59618a

This commit adds code that I missed when backporting.

squash! crypto: make FIPS related options always awailable

e942e28

Fix lint issue.

github-actions bot mentioned this pull request Sep 30, 2021

CI Reliability 2021-09-30 nodejs/reliability#73

Open

17 tasks

mhdawson approved these changes Sep 30, 2021

View reviewed changes

danielleadams force-pushed the v14.x-staging branch from 8216d46 to ec3bd72 Compare October 12, 2021 14:30

github-actions bot mentioned this pull request Nov 25, 2021

CI Reliability 2021-11-25 nodejs/reliability#128

Open

38 tasks

richardlau approved these changes Nov 25, 2021

View reviewed changes

richardlau closed this Nov 25, 2021

richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 25, 2021

This was referenced Nov 26, 2021

CI Reliability 2021-11-26 nodejs/reliability#129

Open

CI Reliability 2021-11-27 nodejs/reliability#130

Open

CI Reliability 2021-11-28 nodejs/reliability#131

Open

CI Reliability 2021-11-29 nodejs/reliability#132

Open

	if options.openssl_fips or options.openssl_fips == '':
	error('FIPS is not supported in this version of Node.js')

Uh oh!

Conversation

danbev commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 29, 2021

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 30, 2021

Uh oh!

nodejs-github-bot commented Nov 24, 2021

Uh oh!

nodejs-github-bot commented Nov 24, 2021

Uh oh!

nodejs-github-bot commented Nov 25, 2021

Uh oh!

richardlau commented Nov 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danbev commented Sep 28, 2021 •

edited

Loading

danbev Sep 28, 2021 •

edited

Loading