Skip to content

test: Skip weak crypto tests in FIPS mode#3757

Closed
stefanmb wants to merge 2 commits intonodejs:masterfrom
stefanmb:fips-cs6-disable-weak-md5-rc4
Closed

test: Skip weak crypto tests in FIPS mode#3757
stefanmb wants to merge 2 commits intonodejs:masterfrom
stefanmb:fips-cs6-disable-weak-md5-rc4

Conversation

@stefanmb
Copy link
Contributor

@stefanmb stefanmb commented Nov 10, 2015

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 Outdated
Copy link
Member

@indutny indutny Nov 11, 2015

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

@shigeki shigeki Nov 12, 2015

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

@stefanmb stefanmb Nov 12, 2015

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

stefanmb commented Nov 13, 2015

@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 Outdated
Copy link
Member

@indutny indutny Nov 13, 2015

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

@stefanmb stefanmb Nov 13, 2015

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

stefanmb commented Nov 13, 2015

@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

stefanmb commented Nov 16, 2015

@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

stefanmb commented Nov 16, 2015

@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

mhdawson commented Nov 19, 2015

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

@mhdawson
Copy link
Member

mhdawson commented Nov 19, 2015

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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@mhdawson
Copy link
Member

mhdawson commented Nov 19, 2015

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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
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 <fedor@indutny.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
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