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

Fix various crypto Diffie-Hellmann issues in pummel tests #28390

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 23, 2019

commit caceabd828d28f627ec7fd711bcb876d76cd20df (HEAD -> fix-pummel-dh, origin/fix-pummel-dh)
Author: Rich Trott <[email protected]>
Date:   Sat Jun 22 15:57:14 2019 -0600

    test: make test-dh-regr more efficient where possible
    
    test-dh-regr is timing out in CI because crypto.createDiffieHellman() is
    considerably slower after an OpenSSL upgrade. This PR modifies the
    change from 11ad744a92374ad71730cbfb7abea71fda0abb74 which made the test
    FIPS-compatible. When not in FIPS mode, the test will use a shorter key
    which will enable it to run much faster.

commit 6a4c29bdbb695eff52578061cec23772664b7c62
Author: Rich Trott <[email protected]>
Date:   Sat Jun 22 15:38:22 2019 -0600

    test: split pummel crypto dh test into two separate tests
    
    Split test-crypto-dh into two tests so that it does not time out in CI.
    With a recent OpenSSL upgrade, getDiffieHellman() is much slower than
    before.

commit 6c417a5fa22b02e0dcf76c14b213f953d7c26c84
Author: Rich Trott <[email protected]>
Date:   Sat Jun 22 14:06:11 2019 -0600

    test: move non-pummel crypto DH tests to parallel
    
    Some parts of pummel/test-crypto-dh.js will be just fine in
    parallel/test-crypto-dh.js. Move them there.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Some parts of pummel/test-crypto-dh.js will be just fine in
parallel/test-crypto-dh.js. Move them there.
Split test-crypto-dh into two tests so that it does not time out in CI.
With a recent OpenSSL upgrade, getDiffieHellman() is much slower than
before.
test-dh-regr is timing out in CI because crypto.createDiffieHellman() is
considerably slower after an OpenSSL upgrade. This PR modifies the
change from 11ad744 which made the test
FIPS-compatible. When not in FIPS mode, the test will use a shorter key
which will enable it to run much faster.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 23, 2019
@Trott Trott mentioned this pull request Jun 23, 2019
4 tasks
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 23, 2019
@Trott

This comment has been minimized.

@Trott Trott added wip Issues and PRs that are still a work in progress. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Jun 23, 2019
@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Jun 23, 2019
@Trott
Copy link
Member Author

Trott commented Jun 23, 2019

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 23, 2019
@Trott
Copy link
Member Author

Trott commented Jun 23, 2019

Since this fixes tests that are timing out in node-daily-master, I'd like to fast-track this. Please 👍 here to fast-track if you are a Collaborator and agree that this should be fast-tracked.

@nodejs/testing @nodejs/crypto

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 23, 2019

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM (also to fast-track) but I have a question. When you say "crypto.createDiffieHellman() is considerably slower after an OpenSSL upgrade" do you mean the openssl 1.1.1c upgrade?

That openssl release changed the default DH key size from 1024 to 2048 bits but that shouldn't affect our tests as one always passes an explicit key size to crypto.createDiffieHellman().

(For reference, it's commit openssl/openssl@408cb4c.)

@Trott
Copy link
Member Author

Trott commented Jun 24, 2019

LGTM (also to fast-track) but I have a question. When you say "crypto.createDiffieHellman() is considerably slower after an OpenSSL upgrade" do you mean the openssl 1.1.1c upgrade?

That openssl release changed the default DH key size from 1024 to 2048 bits but that shouldn't affect our tests as one always passes an explicit key size to crypto.createDiffieHellman().

(For reference, it's commit openssl/openssl@408cb4c.)

@bnoordhuis Yes, it was the update to 1.1.1c.

@Trott
Copy link
Member Author

Trott commented Jun 24, 2019

@bnoordhuis I'm easily able to reproduce the difference. I've opened #28404.

@danbev
Copy link
Contributor

danbev commented Jun 25, 2019

Landed in 0f82704, 2e8e070, and c328482.

@danbev danbev closed this Jun 25, 2019
Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2019
Some parts of pummel/test-crypto-dh.js will be just fine in
parallel/test-crypto-dh.js. Move them there.

PR-URL: nodejs#28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2019
Split test-crypto-dh into two tests so that it does not time out in CI.
With a recent OpenSSL upgrade, getDiffieHellman() is much slower than
before.

PR-URL: nodejs#28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2019
test-dh-regr is timing out in CI because crypto.createDiffieHellman() is
considerably slower after an OpenSSL upgrade. This PR modifies the
change from 11ad744 which made the test
FIPS-compatible. When not in FIPS mode, the test will use a shorter key
which will enable it to run much faster.

PR-URL: nodejs#28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
Some parts of pummel/test-crypto-dh.js will be just fine in
parallel/test-crypto-dh.js. Move them there.

PR-URL: #28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
Split test-crypto-dh into two tests so that it does not time out in CI.
With a recent OpenSSL upgrade, getDiffieHellman() is much slower than
before.

PR-URL: #28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
test-dh-regr is timing out in CI because crypto.createDiffieHellman() is
considerably slower after an OpenSSL upgrade. This PR modifies the
change from 11ad744 which made the test
FIPS-compatible. When not in FIPS mode, the test will use a shorter key
which will enable it to run much faster.

PR-URL: #28390
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@targos targos mentioned this pull request Jul 2, 2019
@Trott Trott deleted the fix-pummel-dh branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants