test: skip fips tests using OpenSSL config file #13786

danbev · 2017-06-19T08:48:42Z

The motivation for this commit is that we are building Node with
--shared-openssl and in our case the system OpenSSL version
supports FIPS.

The tests in test-crypto-fips that toggle fips mode on/off using the
config file option might succeed and return 1 instead of an error
being thrown from OpenSSL (which is what happens for a default build
but the error is not processed/displayed in any way at the moment):
openssl config failed: error:060B10A7:digital envelope
routines:ALG_MODULE_INIT:fips mode not supported

Note that this only concerns the test that use the configuration file
option which is different from when calling the fips setter as
the handling of the configuration file is handled by OpenSSL, so it
is not possible for us to try to call the fips setter as that would
throw an error ("Error: Cannot set FIPS mode in a non-FIPS build.").

The suggestion is to skips these tests when --shared-openssl is used.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, crypto

danbev · 2017-06-19T08:49:18Z

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

bnoordhuis · 2017-06-19T09:47:43Z

test/parallel/test-crypto-fips.js

@@ -89,23 +107,23 @@ testHelper(
 testHelper(
  'stdout',
  [`--openssl-config=${CNF_FIPS_ON}`],
-  compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED,
+  FIPS_ENABLED,


I don't understand how this can pass with a fips-less build of openssl. Can you explain?

In the case of a fips-less build OpenSSL will throw an error which is handled and the asserts skipped. Previously this error went undetected an the process returned 0, but when the same test was run with a build having an OpenSSL FIPS compatible version the call will succeed and return 1.

danbev · 2017-06-19T10:03:31Z

test/osx failure looks unrelated

console output:

not ok 184 async-hooks/test-callback-error
  ---
  duration_ms: 60.55
  severity: fail
  stack: |-
    timeout
  ...

test/windows fanned looks unrelated

console output:

Building remotely on test-rackspace-win2008r2-x64-3 (win2008r2-mp win2008r2-mp-vs2013 win2008r2 win-vs2013 win2008r2-vs2013) in workspace c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2008r2
FATAL: java.nio.channels.ClosedChannelException
java.nio.channels.ClosedChannelException
	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:208)
	at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:222)
	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:832)
	at org.jenkinsci.remoting.protocol.FilterLayer.onRecvClosed(FilterLayer.java:287)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.onRecvClosed(SSLEngineFilterLayer.java:181)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.switchToNoSecure(SSLEngineFilterLayer.java:283)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processWrite(SSLEngineFilterLayer.java:503)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processQueuedWrites(SSLEngineFilterLayer.java:248)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doSend(SSLEngineFilterLayer.java:200)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doCloseSend(SSLEngineFilterLayer.java:213)
	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.doCloseSend(ProtocolStack.java:800)
	at org.jenkinsci.remoting.protocol.ApplicationLayer.doCloseWrite(ApplicationLayer.java:173)
	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer$ByteBufferCommandTransport.closeWrite(ChannelApplicationLayer.java:311)
	at hudson.remoting.Channel.close(Channel.java:1295)
	at hudson.slaves.ChannelPinger$1.onDead(ChannelPinger.java:180)
	at hudson.remoting.PingThread.ping(PingThread.java:130)
	at hudson.remoting.PingThread.run(PingThread.java:86)
Caused: hudson.remoting.RequestAbortedException
	at hudson.remoting.Request.abort(Request.java:307)
	at hudson.remoting.Channel.terminate(Channel.java:896)
	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:208)
	at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:222)
	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:832)
	at org.jenkinsci.remoting.protocol.FilterLayer.onRecvClosed(FilterLayer.java:287)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.onRecvClosed(SSLEngineFilterLayer.java:181)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.switchToNoSecure(SSLEngineFilterLayer.java:283)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processWrite(SSLEngineFilterLayer.java:503)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processQueuedWrites(SSLEngineFilterLayer.java:248)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doSend(SSLEngineFilterLayer.java:200)
	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doCloseSend(SSLEngineFilterLayer.java:213)
	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.doCloseSend(ProtocolStack.java:800)
	at org.jenkinsci.remoting.protocol.ApplicationLayer.doCloseWrite(ApplicationLayer.java:173)
	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer$ByteBufferCommandTransport.closeWrite(ChannelApplicationLayer.java:311)
	at hudson.remoting.Channel.close(Channel.java:1295)
	at hudson.slaves.ChannelPinger$1.onDead(ChannelPinger.java:180)
	at hudson.remoting.PingThread.ping(PingThread.java:130)
	at hudson.remoting.PingThread.run(PingThread.java:86)
	at ......remote call to JNLP4-connect connection from 104.130.8.178/104.130.8.178:64690(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1545)
	at hudson.remoting.Request.call(Request.java:172)
	at hudson.remoting.Channel.call(Channel.java:829)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:257)
	at com.sun.proxy.$Proxy81.addCredentials(Unknown Source)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl.addCredentials(RemoteGitImpl.java:200)
	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:766)
	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:743)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1101)
	at hudson.scm.SCM.checkout(SCM.java:496)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1281)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:405)
ERROR: Step ‘Publish TAP Results’ failed: no workspace for node-test-binary-windows/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2008r2 #9313
Checking ^not ok
ERROR: Build step failed with exception
java.lang.NullPointerException
	at hudson.plugins.textfinder.TextFinderPublisher.findText(TextFinderPublisher.java:101)
	at hudson.plugins.textfinder.TextFinderPublisher.perform(TextFinderPublisher.java:74)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:720)
	at hudson.model.Build$BuildExecution.post2(Build.java:186)
	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:665)
	at hudson.model.Run.execute(Run.java:1753)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:405)
Build step 'Jenkins Text Finder' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

mhdawson · 2017-06-20T18:30:19Z

How about testing this case:

1. Bundled OpenSSL (none fips)
   $ ./configure && make -j8 test

with fips ? Thats the case were I might be more worried that the test change could affect negative tests when fips is enabled.

danbev · 2017-06-21T05:23:44Z

with fips ? Thats the case were I might be more worried that the test change could affect negative tests when fips is enabled.

I did not think this case would be any different from the Shared OpenSSL with openssl-fips one with regards to the test in question, but let me try this to make sure.

danbev · 2017-06-21T12:29:46Z

@mhdawson I've done a test using the combination mentioned above:

$ ./configure --openssl-fips=/Users/danielbevenius/work/security/build_1_0_2k/ && make -j8
$ out/Release/node -p 'process.versions.openssl'
1.0.2l-fips

$ out/Release/node -p "require('crypto').fips = true"
true

out/Release/node test/parallel/test-crypto-fips.js

Spawned child [pid:49486] with cmd 'require("crypto").fips' expect 0 with args '' OPENSSL_CONF=""
Child #1 [pid:49486] OK.
Spawned child [pid:49487] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF=undefined
Child #2 [pid:49487] OK.
Spawned child [pid:49488] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF=undefined
Child #3 [pid:49488] OK.
Spawned child [pid:49489] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
Child #4 [pid:49489] OK.
Spawned child [pid:49490] with cmd 'require("crypto").fips' expect 1 with args '' OPENSSL_CONF="/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_enabled.cnf"
Child #5 [pid:49490] OK.
Spawned child [pid:49491] with cmd 'require("crypto").fips' expect 1 with args '--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF="/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf"
Child #6 [pid:49491] OK.
Spawned child [pid:49492] with cmd 'require("crypto").fips' expect 0 with args '--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF="/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_enabled.cnf"
Child #7 [pid:49492] OK.
Spawned child [pid:49493] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips,--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
Child #8 [pid:49493] OK.
Spawned child [pid:49494] with cmd 'require("crypto").fips' expect 1 with args '--enable-fips' OPENSSL_CONF="/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf"
Child #9 [pid:49494] OK.
Spawned child [pid:49495] with cmd 'require("crypto").fips' expect 1 with args '--force-fips,--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
Child #10 [pid:49495] OK.
Spawned child [pid:49496] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF="/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf"
Child #11 [pid:49496] OK.
Spawned child [pid:49497] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '' OPENSSL_CONF=undefined
Child #12 [pid:49497] OK.
Spawned child [pid:49498] with cmd '(require("crypto").fips = true,require("crypto").fips = false,require("crypto").fips)' expect 0 with args '' OPENSSL_CONF=undefined
Child #13 [pid:49498] OK.
Spawned child [pid:49499] with cmd '(require("crypto").fips = true,require("crypto").fips)' expect 1 with args '--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_disabled.cnf' OPENSSL_CONF=undefined
Child #14 [pid:49499] OK.
Spawned child [pid:49500] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--openssl-config=/Users/danielbevenius/work/nodejs/node/test/fixtures/openssl_fips_enabled.cnf' OPENSSL_CONF=undefined
Child #15 [pid:49500] OK.
Spawned child [pid:49501] with cmd '(require("crypto").fips = false,require("crypto").fips)' expect 0 with args '--enable-fips' OPENSSL_CONF=undefined
Child #16 [pid:49501] OK.
Spawned child [pid:49502] with cmd 'require("crypto").fips = false' expect "Error: Cannot set FIPS mode" with args '--force-fips' OPENSSL_CONF=undefined
Child #17 [pid:49502] OK.
Spawned child [pid:49503] with cmd 'require("crypto").fips = false' expect "Error: Cannot set FIPS mode" with args '--force-fips,--enable-fips' OPENSSL_CONF=undefined
Child #18 [pid:49503] OK.
Spawned child [pid:49504] with cmd 'require("crypto").fips = false' expect "Error: Cannot set FIPS mode" with args '--enable-fips,--force-fips' OPENSSL_CONF=undefined
Child #19 [pid:49504] OK.</details>

mhdawson · 2017-06-21T14:30:59Z

@danbev thanks. One more question. I'm wondering if its possible that the change might cause us to miss errors if we have build normally but for some reason fips is not actually enabled when it should be. If so then is it possible to check if Node.js was compiled with --shared-openssl and only enable the change in that case ?

danbev · 2017-06-21T17:41:46Z

I'm wondering if its possible that the change might cause us to miss errors if we have build normally but for some reason fips is not actually enabled when it should be. If so then is it possible to check if Node.js was compiled with --shared-openssl and only enable the change in that case ?

Yeah, I think that would be possible. I'll give this a try tomorrow and run through the various scenarios.

danbev · 2017-06-22T08:12:54Z

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

danbev · 2017-06-22T11:43:14Z

@mhdawson I've skipped the tests that use the configuration file to toggle fips support as I think it is less messy than anything else I could come up with. Do you think this is acceptable?

mhdawson

That seems reasonable to me LGTM.

The motivation for this commit is that we are building Node with --shared-openssl and in our case the system OpenSSL version supports FIPS. The tests in test-crypto-fips that toggle fips mode on/off using the config file option might succeed and return 1 instead of an error being thrown from OpenSSL (which is what happens for a default build but the error is not processed/displayed in any way at the moment): openssl config failed: error:060B10A7:digital envelope routines:ALG_MODULE_INIT:fips mode not supported Note that this only concerns the test that use the configuration file option which is different from when calling the fips setter as the handling of the configuration file is handled by OpenSSL, so it is not possible for us to try to call the fips setter as that would throw an error ("Error: Cannot set FIPS mode in a non-FIPS build."). The suggestion is to skips these tests when --shared-openssl is used.

danbev · 2017-06-25T16:41:33Z

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

danbev · 2017-06-25T17:25:57Z

@mhdawson @bnoordhuis Sorry about the confusion in my original commit. I've tried to clarify this PR now using @mhdawson suggestion. Would you mind taking another look and review? Thanks

The motivation for this commit is that we are building Node with --shared-openssl and in our case the system OpenSSL version supports FIPS. The tests in test-crypto-fips that toggle fips mode on/off using the config file option might succeed and return 1 instead of an error being thrown from OpenSSL (which is what happens for a default build but the error is not processed/displayed in any way at the moment): openssl config failed: error:060B10A7:digital envelope routines:ALG_MODULE_INIT:fips mode not supported Note that this only concerns the test that use the configuration file option which is different from when calling the fips setter as the handling of the configuration file is handled by OpenSSL, so it is not possible for us to try to call the fips setter as that would throw an error ("Error: Cannot set FIPS mode in a non-FIPS build."). The suggestion is to skips these tests when --shared-openssl is used. PR-URL: nodejs#13786 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danbev · 2017-06-28T05:22:40Z

Landed in 301c68b

The motivation for this commit is that we are building Node with --shared-openssl and in our case the system OpenSSL version supports FIPS. The tests in test-crypto-fips that toggle fips mode on/off using the config file option might succeed and return 1 instead of an error being thrown from OpenSSL (which is what happens for a default build but the error is not processed/displayed in any way at the moment): openssl config failed: error:060B10A7:digital envelope routines:ALG_MODULE_INIT:fips mode not supported Note that this only concerns the test that use the configuration file option which is different from when calling the fips setter as the handling of the configuration file is handled by OpenSSL, so it is not possible for us to try to call the fips setter as that would throw an error ("Error: Cannot set FIPS mode in a non-FIPS build."). The suggestion is to skips these tests when --shared-openssl is used. PR-URL: #13786 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 19, 2017

danbev added the crypto Issues and PRs related to the crypto subsystem. label Jun 19, 2017

bnoordhuis reviewed Jun 19, 2017

View reviewed changes

mhdawson reviewed Jun 23, 2017

View reviewed changes

danbev force-pushed the test-crypto-fips-shared branch from 2d18562 to 6315659 Compare June 25, 2017 16:39

danbev changed the title ~~test: support shared openssl with fips~~ test: skip fips tests using OpenSSL config file Jun 25, 2017

mhdawson approved these changes Jun 26, 2017

View reviewed changes

jasnell approved these changes Jun 26, 2017

View reviewed changes

danbev closed this Jun 28, 2017

danbev deleted the test-crypto-fips-shared branch June 28, 2017 05:23

addaleax mentioned this pull request Jun 29, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the land-on-v6.x label Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: skip fips tests using OpenSSL config file #13786

test: skip fips tests using OpenSSL config file #13786

danbev commented Jun 19, 2017 •

edited

Loading

danbev commented Jun 19, 2017

bnoordhuis Jun 19, 2017

danbev Jun 19, 2017 •

edited

Loading

danbev commented Jun 19, 2017

mhdawson commented Jun 20, 2017

danbev commented Jun 21, 2017

danbev commented Jun 21, 2017

mhdawson commented Jun 21, 2017

danbev commented Jun 21, 2017

danbev commented Jun 22, 2017

danbev commented Jun 22, 2017

mhdawson left a comment

danbev commented Jun 25, 2017

danbev commented Jun 25, 2017

danbev commented Jun 28, 2017

test: skip fips tests using OpenSSL config file #13786

test: skip fips tests using OpenSSL config file #13786

Conversation

danbev commented Jun 19, 2017 • edited Loading

Checklist

Affected core subsystem(s)

danbev commented Jun 19, 2017

bnoordhuis Jun 19, 2017

Choose a reason for hiding this comment

danbev Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

danbev commented Jun 19, 2017

mhdawson commented Jun 20, 2017

danbev commented Jun 21, 2017

danbev commented Jun 21, 2017

mhdawson commented Jun 21, 2017

danbev commented Jun 21, 2017

danbev commented Jun 22, 2017

danbev commented Jun 22, 2017

mhdawson left a comment

Choose a reason for hiding this comment

danbev commented Jun 25, 2017

danbev commented Jun 25, 2017

danbev commented Jun 28, 2017

danbev commented Jun 19, 2017 •

edited

Loading

danbev Jun 19, 2017 •

edited

Loading