test: use openssl_is_fips instead of hasFipsCrypto

[danbev]

Currently, when dynamically linking against a FIPS enabled OpenSSL
library test-process-env-allowed-flags-are-documented will fail with
the following error:

assert.js:89
throw new AssertionError(obj);
^

AssertionError [ERR_ASSERTION]:
The following options are not documented as allowed in NODE_OPTIONS in
/root/node/doc/api/cli.md: --enable-fips --force-fips
at Object.<anonymous>
(/test/parallel/test-process-env-allowed-flags-are-documented.js:82:8)
  at Module._compile (internal/modules/cjs/loader.js:779:30)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:790:10)
  at Module.load (internal/modules/cjs/loader.js:642:32)
  at Function.Module._load (internal/modules/cjs/loader.js:555:12)
  at Function.Module.runMain (internal/modules/cjs/loader.js:842:10)
  at internal/main/run_main_module.js:17:11 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 2,
  expected: 0,
  operator: 'strictEqual'
}

This commit updates the test to use
process.config.variables.openssl_is_fips instead of common.hasFipsCrypto
as hasFipsCrypto only returns true if the OpenSSL library that is
shipped with node was configured with FIPS enabled.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

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

[bnoordhuis]

PR LGTM but you should open an issue to discuss how to detect FIPS-iness at runtime rather than at compile time.

Putting the --enable-fips and --force-fips switches behind a compile-time #if NODE_FIPS_MODE guard is wrong when the dynamically linked openssl is FIPS-capable.

Likewise, crypto.getFips() and crypto.setFips() don't (currently) work right with such builds.

(I don't know of a good way to detect FIPS-iness at runtime but maybe someone else does.)

[bnoordhuis]

Can you add an explaining comment? I would also suggest updating the common.hasFipsCrypto entry in test/common/README.md.

[danbev]

I've added a comment to this line now. About adding a comment to common.hasFipsCrypto in the README, should that be something along the lines of what I added in f1e2dc6?

[richardlau]

The following options are not documented as allowed in NODE_OPTIONS in
/root/node/doc/api/cli.md: --enable-fips --force-fips

While it's entirely possible I made a mistake somewhere in the test, this assertion failure suggests that those command line options are missing from the documentation. Are your /root/node/doc/api/cli.md docs different from those in master? This test is supposed to test for consistency -- that is the options listed in the docs as allowed for NODE_OPTIONS should match those programmattically returned by process.allowedNodeEnvironmentFlags. If the test is correct it is indicating that a Node.js application running on the node binary being tested would be told via process.allowedNodeEnvironmentFlags that --enable-fips and/or --force-fips are valid options.

[danbev]

PR LGTM but you should open an issue to discuss how to detect FIPS-iness at runtime rather than at compile time.

Yeah, I'd like to take a closer look at this and I'm willing to do so but this is my last week before going on PTO so it won't be until I'm back. But if no one has picked up the issue by then I will.

[richardlau]

The following options are not documented as allowed in NODE_OPTIONS in
/root/node/doc/api/cli.md: --enable-fips --force-fips

While it's entirely possible I made a mistake somewhere in the test, this assertion failure suggests that those command line options are missing from the documentation. Are your /root/node/doc/api/cli.md docs different from those in master? This test is supposed to test for consistency -- that is the options listed in the docs as allowed for NODE_OPTIONS should match those programmattically returned by process.allowedNodeEnvironmentFlags. If the test is correct it is indicating that a Node.js application running on the node binary being tested would be told via process.allowedNodeEnvironmentFlags that --enable-fips and/or --force-fips are valid options.

Ah I see it's the filtering, that the test is mistakenly filtering out those two options from the documentation. Does the behaviour change with #28509?

[danbev]

Are your /root/node/doc/api/cli.md docs different from those in master?

I don't think you made a mistake. At the moment there is no FIPS support with the OpenSSL version that is shipped with Node.js so this is not easy to test. And we recently added support to be able to dynamically link to a OpenSSL compatible with FIPS and this is how I discovered this issue.
I'll create an issue like and try to add some more details.

[danbev]

Does the behaviour change with #28509?

No, the same issue remains with that PR.

[danbev]

Likewise, crypto.getFips() and crypto.setFips() don't (currently) work right with such builds.

While looking into creating an issue for this I noticed a mistake I was doing when manually testing crypto.getFips(). I was doing the following:

$ ./node -p "require('crypto').getFips()"
0

And getting the same result for FIPS and non-FIPS builds.
Turns out that I'm an idiot and for a FIPS compatible OpenSSL, FIPS has to be enabled using --enable-fips first:

$ ./node --enable-fips -p "require('crypto').getFips()"
1

Remembering to do this it looks like crypto.getFips() and crypto.setFips() work as expected. Or is there something else here that I'm misssing?

[bnoordhuis]

@danbev I'm thinking of the case where you set fips_mode = yes in openssl.cnf. I don't think Node.js will currently pick that up unless you also pass --enable-fips.

[danbev]

I'm thinking of the case where you set fips_mode = yes in openssl.cnf. I don't think Node.js will currently pick that up unless you also pass --enable-fips.

Ah I see, let me take a look and try that out.

[danbev]

I'm thinking of the case where you set fips_mode = yes in openssl.cnf.

I've tried this but I'm running into an issue that might be specific to the OpenSSL version that we are using . I've created #28531 and I'll follow up on this when I get back from PTO.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jasnell]

One very important challenge with using process.config is that there are userland modules that overwrite it's value. That is, it cannot be relied upon in every case.

[richardlau]

This is the documentation for our test utility functions. Interference from userland modules shouldn't be an issue.

[Trott]

Landed in 6ef4d9e...2111207