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.

Sign up for GitHub

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

Jump to bottom

test: Skip weak crypto tests in FIPS mode #3757

Closed
wants to merge 2 commits into from
Closed

test: Skip weak crypto tests in FIPS mode #3757

wants to merge 2 commits into from

Conversation

stefanmb
Copy link
Contributor

FIPS 140-2 does not permit the use of MD5 and RC4, skip tests that use them in FIPS mode, or substitute with stronger crypto where applicable.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Nov 11, 2015
@stefanmb stefanmb mentioned this pull request Nov 11, 2015
Closed
indutny
indutny reviewed Nov 11, 2015
View reviewed changes
test/parallel/test-crypto-binary-default.js
@@ -323,7 +323,7 @@ var rfc2202_sha1 = [
}
];

for (var i = 0, l = rfc2202_md5.length; i < l; i++) {
for (var i = 0, l = rfc2202_md5.length; i < l && !common.hasFipsCrypto; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

I would rather wrap is with if.

Copy link
Contributor

Choose a reason for hiding this comment

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

me too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@stefanmb stefanmb force-pushed the fips-cs6-disable-weak-md5-rc4 branch from 5e91045 to f42f961 Compare November 12, 2015 23:02
@shigeki
Copy link
Contributor

shigeki commented Nov 13, 2015

@stefanmb I've just updated test-tls-honorcipherorder.js so as to work on only TLS1.2 ciphers to change

DES-CBC-SHA => AES256-SHA256
AES256-SHA => AES128-GCM-SHA256
RC4-SHA => AES128-SHA256
ECDHE-RSA-AES256-SHA => ECDHE-RSA-AES128-GCM-SHA256

Please refer the whole tests at https://gist.github.com/shigeki/f523f894f6d739f59364

All the symmetric cipher is AES so it looks very hard to distinguish them at a glance but it does not matter to test and check the cipher selection.
Only the test5 has a different scenario from the previous one because it need to add a TLS1.2 cipher not included the current default list. I think it is not a realistic test case and should be okay to change it.

@stefanmb stefanmb force-pushed the fips-cs6-disable-weak-md5-rc4 branch 2 times, most recently from f8ea05f to e6b1468 Compare November 13, 2015 20:16
@stefanmb
Copy link
Contributor Author

@shigeki

Thank you! I've included those changes as an additional commit in this PR.

indutny
indutny reviewed Nov 13, 2015
View reviewed changes
test/common.js
@@ -141,6 +141,10 @@ Object.defineProperty(exports, 'hasCrypto', {get: function() {
return process.versions.openssl ? true : false;
}});

Object.defineProperty(exports, 'hasFipsCrypto', {get: function() {
Copy link
Member

Choose a reason for hiding this comment

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

Style, please move get: on a next line and indent everything properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change the line above it too as mine is an exact copy of the existing hasCrypto above it.

@stefanmb stefanmb force-pushed the fips-cs6-disable-weak-md5-rc4 branch 2 times, most recently from e09ee04 to af09eb8 Compare November 13, 2015 20:51
@stefanmb
Copy link
Contributor Author

@indutny Style (and formatting) issues fixed. Thanks!

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@stefanmb ... ok, quite a few CI failures on multiple platforms. The failures look relevant.

@stefanmb
tls: test honor cipher order with TLS1.2
cc835ad 
Modified tests to work in FIPS and non-FIPS mode using TLS1.2 crypto.
@stefanmb stefanmb force-pushed the fips-cs6-disable-weak-md5-rc4 branch from dd5ae5a to cc835ad Compare November 16, 2015 16:56
@stefanmb
Copy link
Contributor Author

@jasnell Oops, there was a typo in one of the tests - I fixed it. Let's try another run.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

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

@stefanmb
Copy link
Contributor Author

@jasnell I see two unrelated failures.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

LGTM
would like @nodejs/crypto to sign off tho

@indutny
Copy link
Member

indutny commented Nov 16, 2015

LGTM

@mhdawson mhdawson self-assigned this Nov 19, 2015
@mhdawson
Copy link
Member

Going to land CI run here: https://ci.nodejs.org/job/node-test-pull-request/787/

@mhdawson
Copy link
Member

No relevant failures, a couple of centos machines had machine related issues and a failure on windows I'm pretty sure I've seen before

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

+1. let's land it!

mhdawson pushed a commit that referenced this pull request Nov 19, 2015
@stefanmb @mhdawson
test: skip/replace weak crypto tests in FIPS mode
e499ea8 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@mhdawson
Copy link
Member

Landed as e499ea8

@mhdawson mhdawson closed this Nov 20, 2015
@Fishrock123 Fishrock123 mentioned this pull request Nov 26, 2015
Closed
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
@stefanmb
test: skip/replace weak crypto tests in FIPS mode
3338fd7 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
@stefanmb @rvagg
test: skip/replace weak crypto tests in FIPS mode
1889593 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
@stefanmb @rvagg
test: skip/replace weak crypto tests in FIPS mode
e129d83 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
This was referenced Dec 8, 2015
Closed
Merged
@jasnell jasnell mentioned this pull request Dec 17, 2015
Closed
jasnell pushed a commit that referenced this pull request Dec 17, 2015
@stefanmb @jasnell
test: skip/replace weak crypto tests in FIPS mode
4a26f74 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
@stefanmb @jasnell
test: skip/replace weak crypto tests in FIPS mode
04d8a65 
FIPS 140-2 does not permit the use of MD5 and RC4, skip or tests
that use them, or substitute with stronger crypto where applicable.

PR-URL: #3757
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mhdawson mhdawson

Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@stefanmb @shigeki @indutny @jasnell @mhdawson @mscdex @MylesBorins