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: replace s_client in test-https-ci-reneg-attack #25720

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Trott
Copy link
Member

Trott commented Jan 26, 2019

Replace s_client in test-https-ci-reneg-attack with built-in
client calling tls.renegotiate(). This also fixes the currently-broken
test. (It is broken due to a change in behavior in a
recently-updated-in-core version of s_client.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
test: replace s_client in test-https-ci-reneg-attack
Replace `s_client` in test-https-ci-reneg-attack with built-in
client calling `tls.renegotiate()`. This also fixes the currently-broken
test. (It is broken due to a change in behavior in a
recently-updated-in-core version of `s_client`.)

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

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 26, 2019

Pummel tests are not run in regular CI so this combination should be sufficient:

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2398/

Repurposed custom-suites job from node-daily-master that runs the pummel tests, but set to only run this one (because there is one other broken pummel test fixed in another PR that hasn't landed yet): https://ci.nodejs.org/job/node-test-commit-custom-suites/841/

});
const options = {
rejectUnauthorized: false,
agent

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2019

Member

Maybe add a trailing comma?

@Trott Trott added the author ready label Jan 26, 2019

@lpinca

lpinca approved these changes Jan 27, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 28, 2019

Landed in 43c2a13`

@Trott Trott closed this Jan 28, 2019

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

test: replace s_client in test-https-ci-reneg-attack
Replace `s_client` in test-https-ci-reneg-attack with built-in
client calling `tls.renegotiate()`. This also fixes the currently-broken
test. (It is broken due to a change in behavior in a
recently-updated-in-core version of `s_client`.)

PR-URL: nodejs#25720
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack

This comment has been minimized.

Copy link
Member

refack commented Jan 28, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 28, 2019

BTW, a run with the full suite fails

Interesting. it's a different test that fails (test-tls rather than test-https) but it's still TLS renegotiation...

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 28, 2019

@refack Is it possible something funky is up with the git checkout on that run? Here's output from that run:

08:25:56     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
08:25:56     
08:25:56       assert(/TLS session renegotiation attack/.test(err))
08:25:56     
08:25:56         at TLSSocket.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites/default/test/pummel/test-tls-ci-reneg-attack.js:59:7)

But the assert() in question is not on line 59 of test-tls-ci-reneg-attack.js. It's on line 56.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 28, 2019

Yeah, in fact, I'm going to call "funky checkout" on this one. It seems that somehow c421619 is missing. That change fixed that test for this sort of thing. And before that change, the assert() was online 59, as reported in CI. But now it's on line 56. So it sure seems like it's running without that change.

@refack

This comment has been minimized.

Copy link
Member

refack commented Jan 28, 2019

something funky is up with the git checkout on that run?

I used the head of this PR - b2ad179:

11:20:08  > git fetch --no-tags --progress git@github.com:nodejs/node.git +refs/heads/*:refs/remotes/origin/* +refs/pull/25720/head:refs/remotes/origin/_jenkins_local_branch # timeout=20
11:20:15 Checking out Revision b2ad1797bde3da028813b073e68dc5ae2d3c9541 (refs/remotes/origin/_jenkins_local_branch)
11:20:15  > git config core.sparsecheckout # timeout=10
11:20:15  > git checkout -f b2ad1797bde3da028813b073e68dc5ae2d3c9541 # timeout=10

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

test: replace s_client in test-https-ci-reneg-attack
Replace `s_client` in test-https-ci-reneg-attack with built-in
client calling `tls.renegotiate()`. This also fixes the currently-broken
test. (It is broken due to a change in behavior in a
recently-updated-in-core version of `s_client`.)

PR-URL: #25720
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 28, 2019

I used the head of this PR - b2ad179:

Ah! That'll do it. This branch didn't have the fix for the other test in it. But it had landed on master.

@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