Repository navigation
crypto: Resolve FIPS test failures and API issues #3760
Description
Activity
@nodejs/crypto
@stefanmb heya, could those PRs just be done as separate commits in less (one?) pull request(s)? That would help a lot! :)
@Fishrock123 That's what I did at first, but I was afraid that some of the commits might be more controversial than others and I did not want to bog down all the trivial changes.
I can, of course, put them together in one PR if you'd prefer.
I think it's fine, maybe something for next time.
Reviewed, thanks for these! Please let me know when you'll fix the nits ;)
Thanks for your great work to have FIPS compliance in Node.
Before putting comments to each PR, I have two comments on overall PRs in genral.- The changes to add
hasFipsCryptoare included in different commits of several PRs so that it causes conflicts in merging. I think it is better to be separated commit to easy to be cherry picked.
diff --git a/test/common.js b/test/common.js index eb802cc..8cfb268 100644 --- a/test/common.js +++ b/test/common.js @@ -141,6 +141,10 @@ Object.defineProperty(exports, 'hasCrypto', {get: function() { return process.versions.openssl ? true : false; }}); +Object.defineProperty(exports, 'hasFipsCrypto', {get: function() { + return process.versions.openssl && process.versions.openssl.endsWith("-fips"); +}}); + if (exports.isWindows) { exports.PIPE = '\\\\.\\pipe\\libuv-test'; } else {
- There are 19 lint errors. Please check with
make jslint
- The changes to add
Checking for -fips in the version string is not a good way to detect FIPS mode, neither is OPENSSL_FIPS. See #3077. Can you instead use process.config.variables.openssl_fips and NODE_FIPS_MODE ?
@Nibbler999 Thanks. I did not know it and was surprised at even OPENSSL_FIPS not working.
@stefanmb We have to change all OPENSSL_FIPS into NODE_FIPS_MODE.
All the linter errors are fixed, sorry about that - I completely forgot to run it, won't happen again!
I've also changed the code to use NODE_FIPS_MODE. I split out the common.js change into a separate commit that is shared by the PRs and changed its implementation to match @Nibbler999's suggestion.
Thank you everyone for your suggestions!
All PR's have landed. Next step is to add build to CI that build/test in FIPs mode, opened this issue to track that: nodejs/build#264
Issue Overview
The issue of FIPS-compliant encryption in Node.js has been brought up before and support for compiling with FIPS-compliant OpenSSL was added. The FIPS build instructions have recently been modified to insure compliance with FIPS 140-2 requirements.
However, running the Node.js test suite (“tools/test.py --verbose”) with the FIPS-compliant OpenSSL crypto module produces a large number of test failures. These failures are a significant roadblock to adoption of Node.js in real life applications requiring FIPS compliance, because it is unclear to prospective users if Node.js can actually work correctly in this mode.
I have spent some time debugging the test failures and have produced a series of pull requests to address them. These pull requests are split up under several “themes” below.
Pull Requests
Documentation Update
#3752 (Landed)
Error Checking
#3753 (Landed)
FIPS-incompatible API
#3754 (Landed)
TLS Wrap
#3755 (Landed)
OpenSSL Known Bug Workaround
#3756 (Landed)
Avoid use of disallowed Crypto (e.g. MD5, RC4)
#3757 (Landed)
Boost Strength of “Arbitrary” Crypto
#3758 (Landed)
Update Test Fixtures with FIPS Compatible Crypto
#3759 (Landed)