test: fix test-tls-junk-closes-server #55089

mhdawson · 2024-09-23T21:26:02Z

TLS spec seems to indicate there should should be a response sent when TLS handshake fails. See
https://datatracker.ietf.org/doc/html/rfc8446#page-85

When compiled with OpenSSL32 we see the
the following response '15 03 03 00 02 02 16' which decodes as a fatal (0x02) TLS error alert number 22 (0x16). which corresponds to TLS1_AD_RECORD_OVERFLOW which matches the error we see if NODE_DEBUG is turned on once you get through the define aliases.

If there is a response from the server the test used to hang because the end event will not be emitted until after the response is consumed. This PR fixes the test so it consumes the response.

Some earlier OpenSSL versions did not seem to send a response but the error handling seems to have been re-written/improved in OpenSSL32.

Refs: nodejs#53382 TLS spec seems to indicate there should should be a response sent when TLS handshake fails. See https://datatracker.ietf.org/doc/html/rfc8446#page-85 When compiled with OpenSSL32 we see the the following response '15 03 03 00 02 02 16' which decodes as a fatal (0x02) TLS error alert number 22 (0x16). which corresponds to TLS1_AD_RECORD_OVERFLOW which matches the error we see if NODE_DEBUG is turned on once you get through the define aliases. If there is a response from the server the test used to hang because the end event will not be emitted until after the response is consumed. This PR fixes the test so it consumes the response. Some earlier OpenSSL versions did not seem to send a response but the error handling seems to have been re-written/improved in OpenSSL32. Signed-off-by: Michael Dawson <midawson@redhat.com>

codecov · 2024-09-23T23:06:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (76edde5) to head (ff25b99).
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55089      +/-   ##
==========================================
- Coverage   88.26%   88.24%   -0.02%     
==========================================
  Files         651      651              
  Lines      183894   183877      -17     
  Branches    35858    35856       -2     
==========================================
- Hits       162315   162266      -49     
- Misses      14882    14893      +11     
- Partials     6697     6718      +21

see 32 files with indirect coverage changes

nodejs-github-bot · 2024-09-24T00:02:20Z

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

test/parallel/test-tls-junk-closes-server.js

Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

mhdawson · 2024-09-24T12:55:36Z

Test on OpenSSL32 (Had already run locally and seen pass, but just in case good to see pass in CI as well) - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/64/ (All tests passed)

nodejs-github-bot · 2024-09-24T13:09:04Z

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

nodejs-github-bot · 2024-09-24T15:36:04Z

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

nodejs-github-bot · 2024-09-24T16:13:43Z

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

lpinca · 2024-09-24T16:15:58Z

test/parallel/test-tls-junk-closes-server.js

@@ -39,6 +39,22 @@ const server = tls.createServer(options, common.mustNotCall());
 server.listen(0, common.mustCall(function() {
  const c = net.createConnection(this.address().port);

+  c.on('data', function() {


Just a nit that can be ignored: replace with c.resume().

nodejs-github-bot · 2024-09-25T21:28:06Z

Landed in 0f7bdcc

Refs: nodejs#53382 TLS spec seems to indicate there should should be a response sent when TLS handshake fails. See https://datatracker.ietf.org/doc/html/rfc8446#page-85 When compiled with OpenSSL32 we see the the following response '15 03 03 00 02 02 16' which decodes as a fatal (0x02) TLS error alert number 22 (0x16). which corresponds to TLS1_AD_RECORD_OVERFLOW which matches the error we see if NODE_DEBUG is turned on once you get through the define aliases. If there is a response from the server the test used to hang because the end event will not be emitted until after the response is consumed. This PR fixes the test so it consumes the response. Some earlier OpenSSL versions did not seem to send a response but the error handling seems to have been re-written/improved in OpenSSL32. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: nodejs#55089 Refs: nodejs#52482 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jithil P Ponnan <jithil@outlook.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: #53382 TLS spec seems to indicate there should should be a response sent when TLS handshake fails. See https://datatracker.ietf.org/doc/html/rfc8446#page-85 When compiled with OpenSSL32 we see the the following response '15 03 03 00 02 02 16' which decodes as a fatal (0x02) TLS error alert number 22 (0x16). which corresponds to TLS1_AD_RECORD_OVERFLOW which matches the error we see if NODE_DEBUG is turned on once you get through the define aliases. If there is a response from the server the test used to hang because the end event will not be emitted until after the response is consumed. This PR fixes the test so it consumes the response. Some earlier OpenSSL versions did not seem to send a response but the error handling seems to have been re-written/improved in OpenSSL32. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: #55089 Refs: #52482 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jithil P Ponnan <jithil@outlook.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: nodejs#53382 TLS spec seems to indicate there should should be a response sent when TLS handshake fails. See https://datatracker.ietf.org/doc/html/rfc8446#page-85 When compiled with OpenSSL32 we see the the following response '15 03 03 00 02 02 16' which decodes as a fatal (0x02) TLS error alert number 22 (0x16). which corresponds to TLS1_AD_RECORD_OVERFLOW which matches the error we see if NODE_DEBUG is turned on once you get through the define aliases. If there is a response from the server the test used to hang because the end event will not be emitted until after the response is consumed. This PR fixes the test so it consumes the response. Some earlier OpenSSL versions did not seem to send a response but the error handling seems to have been re-written/improved in OpenSSL32. Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: nodejs#55089 Refs: nodejs#52482 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jithil P Ponnan <jithil@outlook.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 23, 2024

mhdawson force-pushed the openssl32-fixes-11 branch from 6835d01 to bd68ff8 Compare September 23, 2024 21:28

mhdawson force-pushed the openssl32-fixes-11 branch from bd68ff8 to 9472340 Compare September 23, 2024 21:30

richardlau approved these changes Sep 23, 2024

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024

This comment was marked as outdated.

Sign in to view

aduh95 approved these changes Sep 23, 2024

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024

VoltrexKeyva reviewed Sep 24, 2024

View reviewed changes

test/parallel/test-tls-junk-closes-server.js Outdated Show resolved Hide resolved

test/parallel/test-tls-junk-closes-server.js Outdated Show resolved Hide resolved

mhdawson and others added 2 commits September 24, 2024 08:53

Update test/parallel/test-tls-junk-closes-server.js

0f549f4

Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

Update test/parallel/test-tls-junk-closes-server.js

ff25b99

Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

aduh95 mentioned this pull request Sep 24, 2024

[v18.x backport] test: update TLS tests for OpenSSL 3.2 #55101

Merged

MrJithil approved these changes Sep 24, 2024

View reviewed changes

richardlau added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 24, 2024

lpinca approved these changes Sep 24, 2024

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2024

nodejs-github-bot merged commit 0f7bdcc into nodejs:main Sep 25, 2024
58 checks passed

richardlau mentioned this pull request Sep 26, 2024

Test failures with OpenSSL 3.2.2 #53382

Open

26 tasks

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

aduh95 mentioned this pull request Nov 7, 2024

2024-11-12, Version 18.20.5 'Hydrogen' (LTS) #55768

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test-tls-junk-closes-server #55089

test: fix test-tls-junk-closes-server #55089

mhdawson commented Sep 23, 2024 •

edited by richardlau

Loading

This comment was marked as outdated.

codecov bot commented Sep 23, 2024 •

edited

Loading

nodejs-github-bot commented Sep 24, 2024

mhdawson commented Sep 24, 2024 •

edited

Loading

nodejs-github-bot commented Sep 24, 2024

nodejs-github-bot commented Sep 24, 2024

nodejs-github-bot commented Sep 24, 2024

lpinca Sep 24, 2024

nodejs-github-bot commented Sep 25, 2024

test: fix test-tls-junk-closes-server #55089

test: fix test-tls-junk-closes-server #55089

Conversation

mhdawson commented Sep 23, 2024 • edited by richardlau Loading

This comment was marked as outdated.

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Sep 24, 2024

mhdawson commented Sep 24, 2024 • edited Loading

nodejs-github-bot commented Sep 24, 2024

nodejs-github-bot commented Sep 24, 2024

nodejs-github-bot commented Sep 24, 2024

lpinca Sep 24, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 25, 2024

mhdawson commented Sep 23, 2024 •

edited by richardlau

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

mhdawson commented Sep 24, 2024 •

edited

Loading