Activity

1. Fishrock123 commented on Nov 11, 2015

   

   Fishrock123

   on Nov 11, 2015

   Contributor

   More actions

   @nodejs/crypto
2. Fishrock123

   added

   cryptoIssues and PRs related to the crypto subsystem.Issues and PRs related to the crypto subsystem.

   on Nov 11, 2015
3. Fishrock123 commented on Nov 11, 2015

   

   Fishrock123

   on Nov 11, 2015

   Contributor

   More actions

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

   

   stefanmb

   on Nov 11, 2015

   ContributorAuthor

   More actions

   @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.
5. bnoordhuis commented on Nov 11, 2015

   

   bnoordhuis

   on Nov 11, 2015

   Member

   More actions

   I think it's fine, maybe something for next time.
6. stefanmb commented on Nov 11, 2015

   

   stefanmb

   on Nov 11, 2015

   ContributorAuthor

   More actions

   @indutny @shigeki
7. indutny commented on Nov 11, 2015

   

   indutny

   on Nov 11, 2015

   Member

   More actions

   Reviewed, thanks for these! Please let me know when you'll fix the nits ;)
8. shigeki commented on Nov 12, 2015

   

   shigeki

   on Nov 12, 2015

   Contributor

   More actions

   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
9. Nibbler999 commented on Nov 12, 2015

   

   Nibbler999

   on Nov 12, 2015

   More actions

   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 ?
10. shigeki commented on Nov 13, 2015

    

    shigeki

    on Nov 13, 2015

    Contributor

    More actions

    @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.
11. stefanmb commented on Nov 13, 2015

    

    stefanmb

    on Nov 13, 2015

    ContributorAuthor

    More actions

    @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!
12. mhdawson commented on Nov 23, 2015

    

    mhdawson

    on Nov 23, 2015

    Member

    More actions

    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
13. mhdawson

    closed this as completedon Nov 23, 2015