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.

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

test: increase crypto strength for FIPS standard #3758

Conversation

stefanmb
Copy link
Contributor

In many test cases arbitrary crypto is chosen, for example a key length of 256 bits may be selected, or a prime number of 768 bits length. Some of these choices are not compatible with FIPS, in these cases I’ve opted to boost the cryptography level to a minimum supported level in FIPS. For a discussion on “equivalent” crypto strength across different algorithms see Section 5.6.1 of SP 800-57.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Nov 11, 2015
@@ -141,6 +141,10 @@ Object.defineProperty(exports, 'hasCrypto', {get: function() {
return process.versions.openssl ? true : false;
}});

Object.defineProperty(exports, 'hasFipsCrypto', {get: function() {
return process.versions.openssl && process.versions.openssl.endsWith("-fips");
Copy link
Member

Choose a reason for hiding this comment

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

Single quotes.

@indutny
Copy link
Member

indutny commented Nov 11, 2015

No need to start Increase with a capital letter in commit message. What do you think about running make lint?

@@ -122,7 +122,7 @@ assert.throws(function() {
''
].join('\n');
crypto.createSign('RSA-SHA256').update('test').sign(priv);
}, /RSA_sign:digest too big for rsa key/);
}, /(RSA_sign1|RSA_verify_PKCS1_PSS_mgf1):digest too big for rsa key/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this enough to check /:digest too big for rsa key/ ?

@stefanmb stefanmb force-pushed the fips-cs7-increase-arbitrary-crypto-strength branch from c25a4ae to d04aa4b Compare November 12, 2015 22:33
@stefanmb stefanmb changed the title test: Increase crypto strength for FIPS standard test: increase crypto strength for FIPS standard Nov 13, 2015
@stefanmb stefanmb force-pushed the fips-cs7-increase-arbitrary-crypto-strength branch from d04aa4b to 8fe3e9b Compare November 13, 2015 20:54
@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

LGTM. @shigeki @mhdawson ... any thoughts?

@shigeki
Copy link
Contributor

shigeki commented Nov 14, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

@stefanmb ... this patch, for some reason, is not applying cleanly on master. Can you take a look and rebase/update if necessary. It appears to be having a problem with the changes to common.js

Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.
@stefanmb stefanmb force-pushed the fips-cs7-increase-arbitrary-crypto-strength branch from 8fe3e9b to 390f571 Compare November 14, 2015 17:02
@stefanmb
Copy link
Contributor Author

@jasnell I think it should be fixed now, please confirm. I included the commit for common.js in several PRs, but once it landed in the first one it's no longer needed in the others. Thanks!

@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

Ok. In the future, when you have one commit that may be need by several others, it would likely be best to separate that out into a separate pull request and referenced from the other PRs that depend on it. Doing so helps keep changes isolated and the dependencies visible.

jasnell pushed a commit that referenced this pull request Nov 14, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

Landed in 11ad744

@jasnell jasnell closed this Nov 14, 2015
Fishrock123 pushed a commit that referenced this pull request Nov 17, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123
Copy link
Contributor

CI wasn't run for this and seems to be causing #3881, fwiw.

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

Hmm... ok. At the time CI itself was flaky at best so landing was done optimistically after testing locally on osx and ubuntu.

I've been considering working the new node-stress-single-test into my workflow for every PR that lands a significant test change. Or perhaps a variation that merges node-stress-single-test and node-test-pull-request might be worthwhile. It would certainly help us to identify the flaky tests earlier now that CI is relatively stable again.

@evanlucas
Copy link
Contributor

Ah good point. I forgot about that. I like your idea regarding the node-stress-single-test though.

@stefanmb
Copy link
Contributor Author

@Fishrock123 This follow up PR should alleviate the rpi perf issues #3902.

MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
Use stronger crypto (larger keys, etc.) for arbitrary tests so
they will pass in both FIPS and non-FIPS mode without altering
the original intent of the test cases.

PR-URL: #3758
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants