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,win: speedup tls-server-verify #1836

Closed

Conversation

orangemocha
Copy link
Contributor

Addresses #1461 (and nodejs/node-v0.x-archive#25279 once ported there).

With the -no-rand-screen changes, the test is now much much faster (less than 1s on my machine). I am not sure however if speeding up a test is a good justification for adding a floating patch in openssl. I added all the commits in here to get more feedback.

@orangemocha
Copy link
Contributor Author

cc @nodejs/crypto

@Fishrock123 Fishrock123 added windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. openssl Issues and PRs related to the OpenSSL dependency. labels May 29, 2015
@piscisaureus
Copy link
Contributor

<3

@rvagg
Copy link
Member

rvagg commented May 30, 2015

what are the chances of getting these upstreamed to openssl?

@shigeki
Copy link
Contributor

shigeki commented May 30, 2015

@rvagg No chance because this makes a client connection be in a low entropy state. This change only applied to openssl s_client command on Windows and no affects to libopenssl.a so that iojs has a good entropy state.

@mathiask88
Copy link
Contributor

@shigeki Why is it a low entropy? RAND_poll(CryptGenRandom) still gets called and for the CI the screen content is always the same, so not much entropy added in this case.

@@ -446,6 +447,10 @@ static void sc_usage(void)
" -keymatexport label - Export keying material using label\n");
BIO_printf(bio_err,
" -keymatexportlen len - Export len bytes of keying material (default 20)\n");
#ifdef OPENSSL_SYS_WINDOWS
BIO_printf(bio_err,
" -no-rand-screen - Do not use RAND_screen() to initialize random state\n");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: name it -no_rand_screen for consistency with other options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it. Fixed.

@shigeki
Copy link
Contributor

shigeki commented Jun 1, 2015

@mathiask88 I should have written it as "lower than the current". I don't think it is absolutely low for RAND_add with CryptGenRandom but I can't say it is enough high without RAND_screen for general uses. In CI, I agree that we need not it for this server-verify tests.

@shigeki
Copy link
Contributor

shigeki commented Jun 1, 2015

@orangemocha According to Ben's comments, my commits were updated in shigeki@833b236 , shigeki@9f0f7c3 and shigeki@2ff517e .

joaocgreis and others added 6 commits June 1, 2015 18:27
OpenSSL s_client introduces some delay on Windows. With all clients
running sequentially, this delay is big enough to break CI. This fix runs
the clients in parallel (unless the test includes renegotiation),
reducing the total run time.

(cherry picked from commit 12d4ea3c8ae7f8011de9ca9f50bdc08ee8cfb8bf)
Different servers must use different ports. Since we can count only on
common.PORT and common.PORT+1, run only 2 servers in parallel.

(cherry picked from commit efb0075ee0e1f4123974fe5b74ded8727bad4a9e)
When running in parallel, it is not easy to identify what server and
client failed when the test fails. This adds identifiers to all lines
of console output.

(cherry picked from commit 29c651522df2f47c4360660264eb857fb6232b23)
For better performance of the test, the parent kills child processes
so as not to wait them to be ended.

(cherry picked from commit 833b23636045f7afc929196139021630a390391a)
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

(cherry picked from commit 9f0f7c38e6df975dd39735d0e9ef968076369c74)
This improves the performance of openssl s_client on Windows and
gains several seconds to finish test-tls-server-verify.

(cherry picked from commit 2ff517e0e410ea33ba5a3d289a82fc315d120e8e)
@joaocgreis joaocgreis force-pushed the joaocgreis-io-tls-server-verify branch from ec14721 to e809f01 Compare June 1, 2015 17:59
@joaocgreis
Copy link
Member

@bnoordhuis Thanks for your comments, I updated my commits. I also picked the updated commits from @shigeki .

@bnoordhuis
Copy link
Member

@orangemocha
Copy link
Contributor Author

Thanks, @bnoordhuis . I will land this shortly.

joaocgreis added a commit to JaneaSystems/node that referenced this pull request Jun 3, 2015
OpenSSL s_client introduces some delay on Windows. With all clients
running sequentially, this delay is big enough to break CI. This fix runs
the clients in parallel (unless the test includes renegotiation),
reducing the total run time.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Jun 3, 2015
Different servers must use different ports. Since we can count only on
common.PORT and common.PORT+1, run only 2 servers in parallel.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this pull request Dec 7, 2017
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This was referenced Dec 7, 2017
shigeki pushed a commit to shigeki/node that referenced this pull request Mar 27, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Backport-PR-URL: #19638
Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
shigeki pushed a commit to shigeki/node that referenced this pull request Aug 15, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
rvagg pushed a commit that referenced this pull request Nov 23, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Nov 23, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Nov 24, 2018
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
shigeki pushed a commit to shigeki/node that referenced this pull request Feb 26, 2019
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
sam-github pushed a commit to sam-github/node that referenced this pull request Sep 5, 2019
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs pushed a commit that referenced this pull request Sep 19, 2019
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: #1461
Backport-PR-URL: #28230
PR-URL: #1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BethGriggs BethGriggs mentioned this pull request Sep 19, 2019
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs/node#1461
PR-URL: nodejs/node#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs/node#1461
PR-URL: nodejs/node#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants