FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

crypto: migrate setFipsCrypto to internal/errors - #16428

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:crypto-fips-internal-errors
ConversationCommits (1)ChecksFiles changed
Closed

crypto: migrate setFipsCrypto to internal/errors#16428
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:crypto-fips-internal-errors

Conversation

jasnell commented Oct 24, 2017

Copy link
Copy Markdown
Member

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)

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
Comment thread doc/api/errors.md Outdated

lpinca Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

jasnell Oct 24, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lpinca reviewed Oct 24, 2017
View reviewed changes
Comment thread doc/api/errors.md Outdated

lpinca Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

ping @nodejs/tsc

joyeecheung commented Oct 25, 2017

Copy link
Copy Markdown
Member

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 requested changes Oct 25, 2017
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Missed some errors in CI

Comment thread test/parallel/test-crypto-fips.js Outdated

joyeecheung Oct 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

@joyeecheung .... PTAL

lpinca reviewed Oct 25, 2017
View reviewed changes
Comment thread lib/crypto.js Outdated

lpinca Oct 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell Oct 25, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Oct 25, 2017

Copy link
Copy Markdown
Member Author

CI is good on fips now!

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

jasnell commented Oct 26, 2017

Copy link
Copy Markdown
Member Author

ping @joyeecheung

mcollina approved these changes Oct 27, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jasnell added a commit that referenced this pull request Oct 27, 2017
crypto: migrate setFipsCrypto to internal/errors
ee76f31 
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 commented Oct 27, 2017

Copy link
Copy Markdown
Member Author

Landed in ee76f31

jasnell closed this Oct 27, 2017
jasnell mentioned this pull request Oct 30, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
crypto: migrate setFipsCrypto to internal/errors
19dc5c8 
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 pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
crypto: migrate setFipsCrypto to internal/errors
106f1a8 
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 pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
crypto: migrate setFipsCrypto to internal/errors
902b129 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
mcollina mcollina approved these changes
joyeecheung joyeecheung approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL