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: remove s_client from test-tls-ci-reneg-attack #25700

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@Trott
Copy link
Member

Trott commented Jan 25, 2019

Rewrite test-tls-ci-reneg-attack to use tls.renegotiate() instead of
external (and potentially unpredictable/quirky/buggy) s_client.

Refs: #25676 (comment)

This test is currently broken (due to a quirk in a recent s_client update that we haven't worked around yet, see Ref above) and this change fixes it. The test is only run once a day in CI (because it's in pummel) so the breakage went unnoticed when the OpenSSL update landed a few days ago. A subsequent PR could probably move this test out of pummel. It seems like it could reasonably run in sequential or maybe even parallel.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott Trott requested review from sam-github , bnoordhuis and shigeki Jan 25, 2019

@Trott Trott force-pushed the Trott:rm-client branch 2 times, most recently from 0612d49 to 585e158 Jan 25, 2019

test: remove s_client from test-tls-ci-reneg-attack
Rewrite test-tls-ci-reneg-attack to use tls.renegotiate() instead of
external (and potentially unpredictable/quirky/buggy) s_client.

Refs: #25676 (comment)

@Trott Trott force-pushed the Trott:rm-client branch from 585e158 to 9197808 Jan 25, 2019

@sam-github
Copy link
Member

sam-github left a comment

LGTM

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 26, 2019

Lite CI (because pummel tests are not run in regular CI): https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2397/

node-daily-master custom suite test that runs pummel modified to run against this PR and only run this test (because this is not the only broken pummel test right now): https://ci.nodejs.org/job/node-test-commit-custom-suites/840/

const options = {
host: server.address().host,
port: server.address().port,
rejectUnauthorized: false

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2019

Member

Maybe add a trailing comma?

Trott added a commit to Trott/io.js that referenced this pull request Jan 27, 2019

test: remove s_client from test-tls-ci-reneg-attack
Rewrite test-tls-ci-reneg-attack to use tls.renegotiate() instead of
external (and potentially unpredictable/quirky/buggy) s_client.

Refs: nodejs#25676 (comment)

PR-URL: nodejs#25700
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 27, 2019

Landed in c421619

@Trott Trott closed this Jan 27, 2019

addaleax added a commit that referenced this pull request Jan 28, 2019

test: remove s_client from test-tls-ci-reneg-attack
Rewrite test-tls-ci-reneg-attack to use tls.renegotiate() instead of
external (and potentially unpredictable/quirky/buggy) s_client.

Refs: #25676 (comment)

PR-URL: #25700
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment