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: migrate setFipsCrypto to internal/errors #16428

Closed
wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 24, 2017

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
lpinca approved these changes Oct 24, 2017
doc/api/errors.md Outdated
<a id="ERR_CRYPTO_FIPS_FORCED"></a>
### ERR_CRYPTO_FIPS_FORCED

Used when an attempt to made to enable or disable FIPS mode in the `crypto`

This comment has been minimized.

@lpinca

lpinca Oct 24, 2017
Member

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?

This comment has been minimized.

@jasnell

jasnell Oct 24, 2017
Author Member

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

doc/api/errors.md Outdated
<a id="ERR_CRYPTO_FIPS_UNAVAILABLE"></a>
### ERR_CRYPTO_FIPS_UNAVAILABLE

Used when an attempt to made to enable FIPS mode in the `crypto` module using

This comment has been minimized.

@lpinca

lpinca Oct 24, 2017
Member

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

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 25, 2017

@targos
targos approved these changes Oct 25, 2017
@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 25, 2017

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)
  ...
Copy link
Member

@joyeecheung joyeecheung left a comment

Missed some errors in CI

@@ -10,7 +10,9 @@ const fixtures = require('../common/fixtures');

const FIPS_ENABLED = 1;
const FIPS_DISABLED = 0;
const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode';
const FIPS_ERROR_STRING =
'Error [ERR_CRYPTO_FIPS_UNAVAILABLE]: Cannot set FIPS mode in a ' +

This comment has been minimized.

@joyeecheung

joyeecheung Oct 25, 2017
Member

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

@jasnell jasnell force-pushed the jasnell:crypto-fips-internal-errors branch Oct 25, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 25, 2017

@joyeecheung .... PTAL

lib/crypto.js Outdated
@@ -182,7 +182,9 @@ function setFipsDisabled() {
throw new errors.Error('ERR_CRYPTO_FIPS_UNAVAILABLE');
}

function setFipsForced() {
function setFipsForced(val) {
if (val && fipsForced)

This comment has been minimized.

@lpinca

lpinca Oct 25, 2017
Member

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

This comment has been minimized.

@jasnell

jasnell Oct 25, 2017
Author Member

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

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 25, 2017

CI is good on fips now!

@jasnell jasnell force-pushed the jasnell:crypto-fips-internal-errors branch Oct 25, 2017
@jasnell jasnell added this to In Progress in Error Codes Oct 25, 2017
With the exception of ThrowCryptoError, use internal/errors
to report fips unavailable or forced
@jasnell jasnell force-pushed the jasnell:crypto-fips-internal-errors branch to a4e0c01 Oct 26, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 26, 2017

Copy link
Member

@mcollina mcollina left a comment

LGTM

jasnell added a commit that referenced this pull request Oct 27, 2017
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
Copy link
Member Author

@jasnell jasnell commented Oct 27, 2017

Landed in ee76f31

@jasnell jasnell closed this Oct 27, 2017
@targos targos moved this from In Progress to Done in Error Codes Oct 29, 2017
Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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>
Qard added a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.