Make FIPS related options and functionality always available. #35019
Conversation
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
|
Review requested:
|
nodejs-github-bot
added
c++
| @@ -42,9 +42,7 @@ static void Initialize(Local<Object> target, | |||
| READONLY_FALSE_PROPERTY(target, "hasOpenSSL"); | |||
| #endif // HAVE_OPENSSL | |||
|
|
|||
| #ifdef NODE_FIPS_MODE | |||
| READONLY_TRUE_PROPERTY(target, "fipsMode"); | |||
There was a problem hiding this comment.
Should this reflect FIPS_mode() instead of always being set to true?
There was a problem hiding this comment.
That is very good point looking into this. Do you think it would be OK to basically remove the fipsMode flag and its usage in crypto.js?
There was a problem hiding this comment.
Since this is internal it's probably okay to remove, but you'll need to update everywhere that refers to it, i.e. any of the files under lib or test that do:
const { fipsMode } = internalBinding('config');
github-actions
bot
removed
the
request-ci
|
s/awailable/available/ in the commit message |
| @@ -749,7 +749,6 @@ PerProcessOptionsParser::PerProcessOptionsParser( | |||
| &PerProcessOptions::ssl_openssl_cert_store); | |||
| Implies("--use-openssl-ca", "[ssl_openssl_cert_store]"); | |||
| ImpliesNot("--use-bundled-ca", "[ssl_openssl_cert_store]"); | |||
| #if NODE_FIPS_MODE | |||
| AddOption("--enable-fips", | |||
There was a problem hiding this comment.
I think the comment in the related issue
I did not really test the "vanilla" Node build, but from the above combinations
I assume that the --enable-fips option would not exist and without it,
the crypto.getFips() would still return 0, making this case equivalent to point 3.
is not correct. This change means that the option will be available in the "vanilla" node builds, but that it will fail if you try to enable it. That is likely the cause of a number of the test failures seen in the github actions runs.
@voxik, I thought the whole point was that the option would always be available for consistency across binaries, but would only work when either the binary was dynamically linked and on a system where FIPs was supported or at some point in the future if/when Node.js supports a FIPs mode again natively.
If my understanding is correct there needs to be additional documentation explaining that, both so that its clear and so that the tests will pass.
Assuming my understanding is correct
There was a problem hiding this comment.
Yes, that is right. I think that the comment by @richardlau about fipsMode goes closer to the root cause, because there is apparently still enough functionality controlled by build options.
voxik
changed the title
Good catch 🙈 |
addaleax
added
crypto
|
@voxik This needs a rebase and also some tests are failing. |
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
I'm working on the rebase and looking into the tests. Hopefully should have something to add to this within a week. |
|
@khardix thanks for the update |
|
I think this can be closed since #36341 landed. Going to close, please let us know if that is not the case. |
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