test: fix test-https-ci-reneg-attack

[Trott]

Refs: #25381 (comment)

First commit is just #25660. Once that and this land, then pummel tests should be working again. Please 👍 here to fast-track.

Checklist

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

[nodejs-github-bot]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2366/pipeline

[Trott]

Custom pummel CI (same one that is now failing in node-daily-master because of the issues this fixes/works around): https://ci.nodejs.org/job/node-test-commit-custom-suites/837/

[Trott]

Unrelated failure in the CI run. Because pummel tests aren't run in regular CI, the lite CI should be sufficient. I'll now try to track down what caused the new pummel failure.

(Aside: As pummel tests fail and then are fixed, it's my hope to move them to sequential or parallel or create less pummel-y versions for those locations so that they stop breaking.)

[shigeki]

I confirmed that openssl/openssl#8081 resolved this issue. I think it is better to backport it when it is accepted.

[Trott]

I confirmed that openssl/openssl#8081 resolved this issue. I think it is better to backport it when it is accepted.

I agree that is the much better fix. Until then, the test is failing in our nightly CI. Hopefully the patch is accepted in OpenSSL upstream very soon.

To fix the nightly CI, can we float the OpenSSL patch in Node.js immediately even though it's not in the OpenSSL code base yet? It should be a very low-risk thing to do, since it (I assume) only affects s_client and not anything used by the node binary.

[sam-github]

I don't have strong feelings about whether this is important enough to violate our "don't land patches that haven't been accepted upstream". @shigeki 's fix is only 4 hours old, is it possible to disable the test for the week to give them a chance to accept.

I went through the git history. Since node has a .renegotiate() API, I was perplexed as to why we are using the client_s to do renegotiation. It looks to me that when @bnoordhuis added the ancestor of this test, we didn't yet have a renegotiate API. I think this test should be rewritten to use an in-process TLS/HTTPS client instead of the openssl client, fixing the bug and generally being an improvement.

[Trott]

Removing fast-track label as there's obviously question about whether this should land as-is.

I am definitely 👍 on the suggestion from @sam-github that the test use .renegotiate() instead of s_client. I'll hopefully PR that in some time when I can sit in front of a computer for more than 2 minutes, but if someone else beats me to it, even better.

[Trott]

Closing in favor of #25700 and #25720. (Those both need reviews, so feel free to head over there to get them landed and get the pummel test suite green again.)