crypto: migrate setFipsCrypto to internal/errors by jasnell · Pull Request #16428 · nodejs/node

jasnell · 2017-10-24T05:47:45Z

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

Also only exports the setFipsCrypto and getFipsCrypto methods on process.binding('crypto') if FIPS mode is available.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto (fips)

lpinca · 2017-10-24T12:57:50Z

doc/api/errors.md

I understand the meaning but as a non native speaker this sounds like alien English to me.
Is something like "Used when trying to enable or disable FIPS mode in the crypto module and the --force-fips command-line argument is used" equivalent?

heh... I think I just mangled that sentence a bit ;-)

lpinca · 2017-10-24T14:08:06Z

doc/api/errors.md

This is also a bit hard to grok (at least for me). Feel free to update it :)

jasnell · 2017-10-25T04:36:58Z

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

jasnell · 2017-10-25T04:37:05Z

ping @nodejs/tsc

joyeecheung · 2017-10-25T09:55:42Z

https://ci.nodejs.org/job/node-test-commit-linux-fips/11974/nodes=ubuntu1404-64/console

not ok 316 parallel/test-crypto-fips
  ---
  duration_ms: 2.721
  severity: fail
  stack: |-
    Spawned child [pid:9049] with cmd 'require("crypto").fips' expect 0 with args '' OPENSSL_CONF=""
    Child #1 [pid:9049] OK.
    Spawned child [pid:9055] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF=undefined
    Child #2 [pid:9055] OK.
    Spawned child [pid:9061] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF=undefined
    Child #3 [pid:9061] OK.
    Spawned child [pid:9067] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
    Child #4 [pid:9067] OK.
    Spawned child [pid:9073] with cmd 'require("crypto").fips' expect 1 with args '' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf"
    Child #5 [pid:9073] OK.
    Spawned child [pid:9080] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #6 [pid:9080] OK.
    Spawned child [pid:9086] with cmd 'require("crypto").fips' expect 0 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf"
    Child #7 [pid:9086] OK.
    Spawned child [pid:9092] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips,--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #8 [pid:9092] OK.
    Spawned child [pid:9098] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #9 [pid:9098] OK.
    Spawned child [pid:9104] with cmd 'require("crypto").fips' expect 1 with args '--force-fips,--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #10 [pid:9104] OK.
    Spawned child [pid:9110] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF="/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf"
    Child #11 [pid:9110] OK.
    Spawned child [pid:9116] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '' OPENSSL_CONF=undefined
    Child #12 [pid:9116] OK.
    Spawned child [pid:9122] with cmd '(require("crypto").fips = true,require("crypto").fips = false,require("crypto").fips)' expect 0 with args '' OPENSSL_CONF=undefined
    Child #13 [pid:9122] OK.
    Spawned child [pid:9128] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
    Child #14 [pid:9128] OK.
    Spawned child [pid:9134] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--openssl-config=/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
    Child #15 [pid:9134] OK.
    Spawned child [pid:9140] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--enable-fips' OPENSSL_CONF=undefined
    Child #16 [pid:9140] OK.
    Spawned child [pid:9146] with cmd 'require("crypto").fips = false' expect "Error [ERR_CRYPTO_FIPS_UNAVAILABLE]: Cannot set FIPS mode in a non-FIPS build." with args '--force-fips' OPENSSL_CONF=undefined
    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: false == true
        at responseHandler (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:51:14)
        at testHelper (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:59:3)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-crypto-fips.js:210:1)
        at Module._compile (module.js:596:30)
        at Object.Module._extensions..js (module.js:607:10)
        at Module.load (module.js:515:32)
        at tryModuleLoad (module.js:478:12)
        at Function.Module._load (module.js:470:3)
        at Function.Module.runMain (module.js:637:10)
        at startup (bootstrap_node.js:191:16)
  ...

joyeecheung

Missed some errors in CI

joyeecheung · 2017-10-25T10:04:39Z

test/parallel/test-crypto-fips.js

There should be two flavors of this string, the tests below needs update

jasnell · 2017-10-25T17:16:04Z

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

jasnell · 2017-10-25T17:16:14Z

@joyeecheung .... PTAL

lpinca · 2017-10-25T17:27:01Z

lib/crypto.js

Why the check on fipsForced? Isn't setFipsForced only called when fipsForced is true?

heh, good point ;-) this only needs to check if val is truthy :-)

jasnell · 2017-10-25T18:31:49Z

CI is good on fips now!

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

jasnell · 2017-10-26T21:40:30Z

ping @joyeecheung

mcollina

LGTM

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced PR-URL: #16428 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-10-27T15:18:56Z

Landed in ee76f31

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced PR-URL: nodejs/node#16428 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 24, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 24, 2017

lpinca approved these changes Oct 24, 2017

View reviewed changes

lpinca reviewed Oct 24, 2017

View reviewed changes

doc/api/errors.md Outdated

Copy link

Member

lpinca Oct 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a bit hard to grok (at least for me). Feel free to update it :)

targos approved these changes Oct 25, 2017

View reviewed changes

joyeecheung approved these changes Oct 25, 2017

View reviewed changes

joyeecheung requested changes Oct 25, 2017

View reviewed changes

jasnell force-pushed the crypto-fips-internal-errors branch from ae963ed to 1e4c943 Compare October 25, 2017 17:15

lpinca reviewed Oct 25, 2017

View reviewed changes

jasnell force-pushed the crypto-fips-internal-errors branch from 0391080 to 5438770 Compare October 25, 2017 18:39

crypto: migrate setFipsCrypto to internal/errors

a4e0c01

With the exception of ThrowCryptoError, use internal/errors to report fips unavailable or forced

jasnell force-pushed the crypto-fips-internal-errors branch from 5438770 to a4e0c01 Compare October 26, 2017 21:39

joyeecheung approved these changes Oct 27, 2017

View reviewed changes

mcollina approved these changes Oct 27, 2017

View reviewed changes

jasnell closed this Oct 27, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Uh oh!

Conversation

jasnell commented Oct 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

lpinca Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

joyeecheung commented Oct 25, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

lpinca Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

jasnell commented Oct 26, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lpinca Oct 25, 2017 •

edited

Loading