test: skip fips tests using OpenSSL config file#13786
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
ContributorAuthor
danbev commented Jun 19, 2017
test/parallel/test-crypto-fips.js Outdated
Member
There was a problem hiding this comment.
I don't understand how this can pass with a fips-less build of openssl. Can you explain?
ContributorAuthor
There was a problem hiding this comment.
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.
ContributorAuthor
danbev commented Jun 19, 2017
test/osx failure looks unrelatednot ok 184 async-hooks/test-callback-error --- duration_ms: 60.55 severity: fail stack: |- timeout ...test/windows fanned looks unrelatedBuilding 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\win2008r2FATAL: java.nio.channels.ClosedChannelExceptionjava.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 #9313Checking ^not okERROR: Build step failed with exceptionjava.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 failureNotifying upstream projects of job completionFinished: FAILURE |
Member
mhdawson commented Jun 20, 2017
How about testing this case: with fips ? Thats the case were I might be more worried that the test change could affect negative tests when fips is enabled. |
ContributorAuthor
danbev commented Jun 21, 2017
I did not think this case would be any different from the |
ContributorAuthor
danbev commented Jun 21, 2017
@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"trueout/Release/node test/parallel/test-crypto-fips.jsSpawned 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=undefinedChild #2 [pid:49487] OK.Spawned child [pid:49488] with cmd 'require("crypto").fips' expect 1 with args '--force-fips' OPENSSL_CONF=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #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=undefinedChild #19 [pid:49504] OK.</details> |
Member
mhdawson commented Jun 21, 2017
@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 ? |
ContributorAuthor
danbev commented Jun 21, 2017
Yeah, I think that would be possible. I'll give this a try tomorrow and run through the various scenarios. |
ContributorAuthor
danbev commented Jun 22, 2017
ContributorAuthor
danbev commented Jun 22, 2017
@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? |
Member
mhdawson left a comment
mhdawson left a comment There was a problem hiding this comment.
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.
danbevforce-pushed the test-crypto-fips-shared branch from 2d18562 to 6315659Comparedanbev changed the title ContributorAuthor
danbev commented Jun 25, 2017
ContributorAuthor
danbev commented Jun 25, 2017
@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 |
mhdawson approved these changes Jun 26, 2017
jasnell approved these changes Jun 26, 2017
danbev added a commit to danbev/node that referenced this pull request Jun 28, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>ContributorAuthor
danbev commented Jun 28, 2017
Landed in 301c68b |
addaleax pushed a commit that referenced this pull request Jun 29, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Merged
This was referenced Sep 5, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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), orvcbuild test(Windows) passesAffected core subsystem(s)
test, crypto