test: fix test-tls-junk-closes-server#55089
Merged
nodejs-github-bot merged 3 commits intonodejs:mainfrom Sep 25, 2024
Merged
Conversation
nodejs-github-bot
added
needs-ci
mhdawson
force-pushed
the
openssl32-fixes-11
branch
from
6835d01 to
bd68ff8
Compare
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>
mhdawson
force-pushed
the
openssl32-fixes-11
branch
from
bd68ff8 to
9472340
Compare
richardlau
approved these changes
Sep 23, 2024
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
aduh95
approved these changes
Sep 23, 2024
aduh95
added
the
author ready
Collaborator
aduh95
added
the
commit-queue
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Member
Author
|
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) |
github-actions
bot
removed
the
request-ci
Collaborator
MrJithil
approved these changes
Sep 24, 2024
Collaborator
richardlau
added
commit-queue-squash
Collaborator
lpinca
approved these changes
Sep 24, 2024
| server.listen(0, common.mustCall(function() { | ||
| const c = net.createConnection(this.address().port); | ||
|
|
||
| c.on('data', function() { |
Member
There was a problem hiding this comment.
Just a nit that can be ignored: replace with c.resume().
aduh95
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
Collaborator
|
Landed in 0f7bdcc |
26 tasks
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs: #53382
Refs: #52482
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.