crypto: only try to set FIPS mode if different #12210

gibfahn · 2017-04-04T14:40:36Z

Turning FIPS mode on (or off) when it's already on (or off) should be a
no-op, not an error.

Fixes: #11849

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)

Patch shamelessly stolen from @bnoordhuis here. PR'd to remind myself to test this properly.

jasnell · 2017-04-04T16:01:24Z

src/node_crypto.cc

+  const bool enabled = FIPS_mode();
+  const bool enable = args[0]->BooleanValue();
+  if (enable == enabled)
+    return;  // No action needed.
  if (force_fips_crypto) {


Given this change, should we also only throw if attempting to turn FIPS off when force_fips_crypto is on? Turning it on should likely just non-op

@jasnell if force_fips_crypto is on, then FIPS_mode() should already be enabled right? So doesn't the if (enable == enabled) branch already cover that?

good point. ok

richardlau

Needs tests (marking so this doesn't land without them).

mhdawson

LGTM

BridgeAR · 2017-08-26T12:21:25Z

@gibfahn would you be so kind and add some tests for this?

BridgeAR · 2017-09-20T00:33:08Z

Ping @gibfahn

BridgeAR · 2017-09-24T17:23:17Z

Closing due to long inactivity. @gibfahn please reopen if you still want to pursue this.

gibfahn · 2017-09-24T19:50:01Z

Note to self, if a PR is closed, don't push to the branch before reopening the PR. If you do it's undoable as long as you force-push back to the commit hash the branch pointed to when the PR was closed.

gibfahn · 2017-09-24T19:51:09Z

Added a test to confirm that enabling fips when --force-fips is set is a no-op. There are already tests covering everything else AFAICT.

PTAL.

BridgeAR · 2017-09-24T20:00:43Z

test/parallel/test-crypto-fips.js

+  ['--force-fips'],
+  compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING,
+  '(require("crypto").fips = true,' +
+  'require("crypto").fips)',


I personally would use a single line for this argument as all other arguments are also on a single line but that is just taste and does not have to be addressed.

I copied this from a test further up. There are a few things I'd change about this test file, but I thought I'd try to keep this diff clean.

gibfahn · 2017-09-25T13:40:56Z

@richardlau does this LGTY now?

gibfahn · 2017-09-26T12:25:04Z

CI: https://ci.nodejs.org/job/node-test-commit/12618/

@nodejs/crypto I'd appreciate a quick look if you have time.

gibfahn · 2017-09-26T18:51:49Z

CI has these failures (I think all unrelated):

sequential/test-benchmark-buffer on fedora24

not ok 1743 sequential/test-benchmark-buffer
  ---
  duration_ms: 3.848
  severity: fail
  stack: |-
    
    buffers/buffer-base64-decode-wrapped.js
    buffers/buffer-base64-decode-wrapped.js n=1: 8.777361625508389
    
    buffers/buffer-base64-decode.js
    buffers/buffer-base64-decode.js n=1: 10.883984248741532
    
    buffers/buffer-base64-encode.js
    buffers/buffer-base64-encode.js n=1 len=2: 6,010.662916013007
    
    buffers/buffer-bytelength.js
    buffers/buffer-bytelength.js n=1 len=2 encoding="utf8": 6,914.0036229378975
    
    buffers/buffer-compare-instance-method.js
    buffers/buffer-compare-instance-method.js millions=1 args=1 size=1: 6.75575174739039
    
    buffers/buffer-compare-offset.js
    buffers/buffer-compare-offset.js millions=1 size=1 method="": 20.340541756904937
    
    buffers/buffer-compare.js
    buffers/buffer-compare.js millions=1 size=1: 7.3398795370162535
    
    buffers/buffer-concat.js
    buffers/buffer-concat.js n=1 withTotalLength=0 pieceSize=1 pieces=1: 427.32997394996477
    
    buffers/buffer-creation.js
    buffers/buffer-creation.js n=1 len=2 type="": 1,094.8405638428903
    
    buffers/buffer-from.js
    buffers/buffer-from.js n=1 len=2 source="array": 422.5531846565868
    
    buffers/buffer-hex.js
    buffers/buffer-hex.js n=1 len=2: 1,095.9312456573725
    
    buffers/buffer-indexof-number.js
    /home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/benchmark/common.js:202
          throw new Error('insufficient clock precision for short benchmark');
          ^
    
    Error: insufficient clock precision for short benchmark
        at Benchmark.end (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/benchmark/common.js:202:13)
        at main (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/benchmark/buffers/buffer-indexof-number.js:22:9)
        at Benchmark.process.nextTick (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/benchmark/common.js:34:28)
        at _combinedTickCallback (internal/process/next_tick.js:131:7)
        at process._tickCallback (internal/process/next_tick.js:180:9)
        at Function.Module.runMain (module.js:643:11)
        at startup (bootstrap_node.js:187:16)
        at bootstrap_node.js:607:3
    assert.js:44
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.child.on (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/test/common/benchmark.js:25:12)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:210:12)
  ...

parallel/test-http-writable-true-after-close on centos5-64

not ok 755 parallel/test-http-writable-true-after-close
  ---
  duration_ms: 0.214
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/test/common/index.js:482:10)
        at ClientRequest.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/test/parallel/test-http-writable-true-after-close.js:24:28)
        at ClientRequest.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-64/test/common/index.js:514:15)
        at Object.onceWrapper (events.js:316:30)
        at emitOne (events.js:115:13)
        at ClientRequest.emit (events.js:210:7)
        at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:564:21)
        at HTTPParser.parserOnHeadersComplete (_http_common.js:116:23)
        at Socket.socketOnData (_http_client.js:453:20)
  ...

parallel/test-http-writable-true-after-close on centos5-32

not ok 755 parallel/test-http-writable-true-after-close
  ---
  duration_ms: 0.286
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
        at Object.exports.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/common/index.js:482:10)
        at ClientRequest.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-http-writable-true-after-close.js:24:28)
        at ClientRequest.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/common/index.js:514:15)
        at Object.onceWrapper (events.js:316:30)
        at emitOne (events.js:115:13)
        at ClientRequest.emit (events.js:210:7)
        at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:564:21)
        at HTTPParser.parserOnHeadersComplete (_http_common.js:116:23)
        at Socket.socketOnData (_http_client.js:453:20)
  ...

inspector/test-bindings on win2016

not ok 442 inspector/test-bindings
  ---
  duration_ms: 0.286
  severity: fail
  stack: |-
    Expecting warning to be emitted
    (node:7428) Error: We expect this
    c:\workspace\node-test-binary-windows\COMPILED_BY\vs2017\RUNNER\win2016\RUN_SUBSET\1\test\common\index.js:789
                 (err) => process.nextTick(() => { throw err; }));
                                                   ^
    
    AssertionError [ERR_ASSERTION]: [ 'Iteration 1 variable: i expected: 1 actual: 0',
      'Iteration 2 variable: i expected: 2 actual: 1',
      'Iteration 2 variable: a deepStrictEqual []
        at testSampleDebugSession (c:\workspace\node-test-binary-windows\COMPILED_BY\vs2017\RUNNER\win2016\RUN_SUBSET\1\test\inspector\test-bindings.js:104:10)
        at doTests (c:\workspace\node-test-binary-windows\COMPILED_BY\vs2017\RUNNER\win2016\RUN_SUBSET\1\test\inspector\test-bindings.js:131:3)
        at <anonymous>
        at process._tickCallback (internal/process/next_tick.js:188:7)
        at Function.Module.runMain (module.js:643:11)
        at startup (bootstrap_node.js:187:16)
        at bootstrap_node.js:607:3
  ...

Turning FIPS mode on (or off) when it's already on (or off) should be a no-op, not an error. PR-URL: nodejs#12210 Fixes: nodejs#11849 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Turning FIPS mode on (or off) when it's already on (or off) should be a no-op, not an error. PR-URL: #12210 Fixes: #11849 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Turning FIPS mode on (or off) when it's already on (or off) should be a no-op, not an error. PR-URL: nodejs/node#12210 Fixes: nodejs/node#11849 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

Turning FIPS mode on (or off) when it's already on (or off) should be a no-op, not an error. PR-URL: #12210 Fixes: #11849 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-10-17T18:51:34Z

@gibfahn I've backported to v6.x, can you test this out once the r.c. drops

Turning FIPS mode on (or off) when it's already on (or off) should be a no-op, not an error. PR-URL: #12210 Fixes: #11849 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

gibfahn added crypto Issues and PRs related to the crypto subsystem. wip Issues and PRs that are still a work in progress. labels Apr 4, 2017

gibfahn mentioned this pull request Apr 4, 2017

using --enable-fips or --force-fips with crypto.fips=1 fails #11849

Closed

jasnell reviewed Apr 4, 2017

View reviewed changes

richardlau requested changes Apr 4, 2017

View reviewed changes

mhdawson approved these changes Apr 4, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

gibfahn self-assigned this Sep 2, 2017

BridgeAR added the stalled Issues and PRs that are stalled. label Sep 12, 2017

gibfahn force-pushed the crypto-fips branch from 167bde0 to e93588b Compare September 22, 2017 16:43

BridgeAR closed this Sep 24, 2017

gibfahn reopened this Sep 24, 2017

gibfahn force-pushed the crypto-fips branch from e93588b to dadc0ea Compare September 24, 2017 19:49

gibfahn removed wip Issues and PRs that are still a work in progress. stalled Issues and PRs that are stalled. labels Sep 24, 2017

BridgeAR approved these changes Sep 24, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

richardlau approved these changes Sep 26, 2017

View reviewed changes

gibfahn force-pushed the crypto-fips branch from dadc0ea to 6f1c8c8 Compare September 26, 2017 22:48

gibfahn force-pushed the crypto-fips branch from 6f1c8c8 to 0919dff Compare September 26, 2017 22:49

gibfahn merged commit 0919dff into nodejs:master Sep 26, 2017

gibfahn deleted the crypto-fips branch September 26, 2017 22:51

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: only try to set FIPS mode if different #12210

crypto: only try to set FIPS mode if different #12210

gibfahn commented Apr 4, 2017

jasnell Apr 4, 2017

gibfahn Apr 4, 2017 •

edited

Loading

jasnell Apr 4, 2017

richardlau left a comment

mhdawson left a comment

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 20, 2017

BridgeAR commented Sep 24, 2017

gibfahn commented Sep 24, 2017

gibfahn commented Sep 24, 2017

BridgeAR Sep 24, 2017

gibfahn Sep 24, 2017

gibfahn commented Sep 25, 2017

gibfahn commented Sep 26, 2017

gibfahn commented Sep 26, 2017

MylesBorins commented Oct 17, 2017

crypto: only try to set FIPS mode if different #12210

crypto: only try to set FIPS mode if different #12210

Conversation

gibfahn commented Apr 4, 2017

Checklist

Affected core subsystem(s)

jasnell Apr 4, 2017

Choose a reason for hiding this comment

gibfahn Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell Apr 4, 2017

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 20, 2017

BridgeAR commented Sep 24, 2017

gibfahn commented Sep 24, 2017

gibfahn commented Sep 24, 2017

BridgeAR Sep 24, 2017

Choose a reason for hiding this comment

gibfahn Sep 24, 2017

Choose a reason for hiding this comment

gibfahn commented Sep 25, 2017

gibfahn commented Sep 26, 2017

gibfahn commented Sep 26, 2017

MylesBorins commented Oct 17, 2017

gibfahn Apr 4, 2017 •

edited

Loading