Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: Resolve FIPS test failures and API issues #3760

Closed
stefanmb opened this issue Nov 11, 2015 · 11 comments
Closed

crypto: Resolve FIPS test failures and API issues #3760

stefanmb opened this issue Nov 11, 2015 · 11 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@stefanmb
Copy link
Contributor

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)

@Fishrock123
Copy link
Contributor

@nodejs/crypto

@Fishrock123 Fishrock123 added the crypto Issues and PRs related to the crypto subsystem. label Nov 11, 2015
@Fishrock123
Copy link
Contributor

@stefanmb heya, could those PRs just be done as separate commits in less (one?) pull request(s)? That would help a lot! :)

@stefanmb
Copy link
Contributor Author

@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.

@bnoordhuis
Copy link
Member

I think it's fine, maybe something for next time.

@stefanmb
Copy link
Contributor Author

@indutny @shigeki

@indutny
Copy link
Member

indutny commented Nov 11, 2015

Reviewed, thanks for these! Please let me know when you'll fix the nits ;)

@shigeki
Copy link
Contributor

shigeki commented Nov 12, 2015

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 hasFipsCrypto are 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

@Nibbler999
Copy link
Contributor

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 ?

@shigeki
Copy link
Contributor

shigeki commented Nov 13, 2015

@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.

@stefanmb
Copy link
Contributor Author

@shigeki @Nibbler999 @indutny

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!

@mhdawson
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants