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: fix invalid modulesLength during DSA keygen #23732

Closed
wants to merge 1 commit into from

Conversation

AdamMajer
Copy link
Contributor

In #23430, key sizes were reduced to speed up the test. Unfortunately this resulted in invalid DSA key generation parameters.

In standard build of OpenSSL, key generation uses dsa_builtin_paramgen and that will reset modulusLength to 512. But in dsa_builtin_paramgen2, used in FIPS, this does not happen, leading to an infinite loop and test failure.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 18, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

Refs: nodejs#23430
@sam-github
Copy link
Contributor

Thanks @AdamMajer. Other than this, did the tests pass on FIPS?

@AdamMajer
Copy link
Contributor Author

So far I've only run it with OpenSSL that has fips option enabled, but not on system running in FIPS mode. Having said that, all crypto related tests pass.

@@ -182,7 +182,7 @@ function convertDERToPEM(label, der) {
{
// Test async DSA key generation.
generateKeyPair('dsa', {
modulusLength: 256,
modulusLength: 512,
Copy link
Member

@Trott Trott Oct 18, 2018

Choose a reason for hiding this comment

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

Looks good to me.

(UPDATE: Previous suggestion removed because I'm not actually sure it's correct and I don't have the ability to test it. It adds complexity anyway, so whatevs.)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, openssl just internally increases 256 to 512 (when not FIPs), so always using 512 is no slower and more clear.

@Trott
Copy link
Member

Trott commented Oct 18, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2018
@Trott
Copy link
Member

Trott commented Oct 18, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17979/ ✔️
(refack: CI is green even though it did not propagate to GitHub)

@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Oct 19, 2018
@danbev
Copy link
Contributor

danbev commented Oct 24, 2018

Landed in a88ca52.

@danbev danbev closed this Oct 24, 2018
danbev pushed a commit that referenced this pull request Oct 24, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Oct 24, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
During key generation, the default dsa_builtin_paramgen will reset
modulusLength to 512. But in dsa_builtin_paramgen2 this does not
happen, leading to lockup in FIPS mode.

PR-URL: #23732
Refs: #23430
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

10 participants