Skip to content

test: fix invalid modulesLength during DSA keygen#23732

Closed
AdamMajer wants to merge 1 commit intonodejs:masterfrom
AdamMajer:master
Closed

test: fix invalid modulesLength during DSA keygen#23732
AdamMajer wants to merge 1 commit intonodejs:masterfrom
AdamMajer:master

Conversation

@AdamMajer
Copy link
Copy Markdown
Contributor

@AdamMajer AdamMajer commented Oct 18, 2018

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
@AdamMajer
test: fix invalid modulesLength for DSA keygen
478e360 
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
Copy Markdown
Contributor

sam-github commented Oct 18, 2018

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

@AdamMajer
Copy link
Copy Markdown
Contributor Author

AdamMajer commented Oct 18, 2018

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.

Trott
Trott approved these changes Oct 18, 2018
View reviewed changes
Comment thread test/parallel/test-crypto-keygen.js
// Test async DSA key generation.
generateKeyPair('dsa', {
modulusLength: 256,
modulusLength: 512,
Copy link
Copy Markdown
Member

@Trott Trott Oct 18, 2018 •
edited
Loading

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
Copy Markdown
Contributor

@sam-github sam-github Oct 18, 2018

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
Copy Markdown
Member

Trott commented Oct 18, 2018

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

@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
Copy Markdown
Member

Trott commented Oct 18, 2018 •
edited by refack
Loading

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
Copy Markdown
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
@AdamMajer @danbev
test: fix invalid modulesLength for DSA keygen
a88ca52 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Oct 24, 2018
@AdamMajer @targos
test: fix invalid modulesLength for DSA keygen
1521d89 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@AdamMajer @MylesBorins
test: fix invalid modulesLength for DSA keygen
79be83d 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@AdamMajer @rvagg
test: fix invalid modulesLength for DSA keygen
cb229e0 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@AdamMajer @MylesBorins
test: fix invalid modulesLength for DSA keygen
1747e47 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@sam-github sam-github sam-github approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@AdamMajer @sam-github @Trott @danbev @jasnell @tniessen @BridgeAR @trivikr @MylesBorins @nodejs-github-bot