★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/28390Nouvelle recherche | Portail wanayoo
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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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 <rtrott@gmail.com>
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

Trott added 3 commits Jun 22, 2019
@Trott
test: move non-pummel crypto DH tests to parallel
6c417a5 
Some parts of pummel/test-crypto-dh.js will be just fine in
parallel/test-crypto-dh.js. Move them there.
@Trott
test: split pummel crypto dh test into two separate tests
6a4c29b 
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.
@Trott
test: make test-dh-regr more efficient where possible
caceabd 
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 label Jun 23, 2019
@Trott Trott mentioned this pull request Jun 23, 2019
Closed
4 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@Trott Trott added the fast-track label Jun 23, 2019
@Trott

This comment has been minimized.

Sign in to view
@Trott Trott added wip and removed fast-track labels Jun 23, 2019
@Trott

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@Trott

This comment has been minimized.

Sign in to view
@Trott Trott removed the wip label Jun 23, 2019
@Trott
Copy link
Member Author

@Trott Trott commented Jun 23, 2019

Pummel CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/7609/

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jun 23, 2019

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

@Trott Trott added the fast-track label Jun 23, 2019
@Trott
Copy link
Member Author

@Trott 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
Contributor

@nodejs-github-bot nodejs-github-bot commented Jun 23, 2019 •
edited by Trott

CI: https://ci.nodejs.org/job/node-test-pull-request/24068/ ✔️

bnoordhuis
bnoordhuis approved these changes Jun 24, 2019
View changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

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 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 Trott commented Jun 24, 2019

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

@danbev
Copy link
Member

@danbev 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 issue Jun 25, 2019
@Trott @danbev
test: move non-pummel crypto DH tests to parallel
0f82704 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Trott added a commit to Trott/io.js that referenced this issue Jun 25, 2019
@Trott @danbev
test: split pummel crypto dh test into two separate tests
2e8e070 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Trott added a commit to Trott/io.js that referenced this issue Jun 25, 2019
@Trott @danbev
test: make test-dh-regr more efficient where possible
c328482 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this issue Jul 2, 2019
@Trott @targos
test: move non-pummel crypto DH tests to parallel
e161744 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this issue Jul 2, 2019
@Trott @targos
test: split pummel crypto dh test into two separate tests
9f508e3 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this issue Jul 2, 2019
@Trott @targos
test: make test-dh-regr more efficient where possible
69f17f1 
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@targos targos mentioned this pull request Jul 2, 2019
Merged
@beevelop beevelop mentioned this pull request Jul 3, 2019
Closed
@beevelop beevelop mentioned this pull request Jul 3, 2019
Closed
@beevelop beevelop mentioned this pull request Jul 3, 2019
Closed
@Trott Trott deleted the fix-pummel-dh branch Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis

@danbev danbev

Assignees
No one assigned
Labels
fast-track test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Trott @nodejs-github-bot @danbev @bnoordhuis