tls: fix bugs of double TLS #48796
Closed
tls: fix bugs of double TLS #48796
Conversation
This file contains 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
Fixs two issues in `TLSWrap`, one of them is reported in nodejs#30896. 1. `TLSWrap` has exactly one `StreamListener`, however, that `StreamListener` can be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successor `StreamListener`. 2. A `TLSWrap` does not allow more than one active write, as checked in the assertion about current_write in `TLSWrap::DoWrite()`. However, when users make use of an existing `tls.TLSSocket` to establish double TLS, by either tls.connect({socket: tlssock}) or tlsServer.emit('connection', tlssock) we have both of the user provided `tls.TLSSocket`, tlssock and a brand new created `TLSWrap` writing to the `TLSWrap` bound to tlssock, which easily violates the constranint because two writers have no idea of each other. The design of the fix is: when a `TLSWrap` is created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired.
|
Review requested:
|
nodejs-github-bot
added
c++
Format code.
github-actions
bot
removed
the
request-ci
mscdex
reviewed
Jul 16, 2023
25 tasks
Remove unnecessary saving of `this` for arrow function.
mscdex
reviewed
Jul 17, 2023
test/parallel/test-socket-writes-before-paased-to-tls-socket.js
Outdated
Show resolved
Hide resolved
Fix typo
26 tasks
|
leftover processes found in the github hooked CI But it seems like these failures have nothing to do with this PR. Could somebody rerun the CI? |
25 tasks
|
cc @nodejs/crypto |
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
This was referenced Jul 22, 2023
This was referenced Jul 24, 2023
This was referenced Jul 26, 2023
ShogunPanda
approved these changes
Jul 27, 2023
19 tasks
jasnell
reviewed
Jul 28, 2023
| @@ -136,7 +136,8 @@ class TLSWrap : public AsyncWrap, | |||
| v8::Local<v8::Object> obj, | |||
| Kind kind, | |||
| StreamBase* stream, | |||
| SecureContext* sc); | |||
| SecureContext* sc, | |||
| bool stream_has_active_write); | |||
There was a problem hiding this comment.
Non-blocking nit: I'm generally not a fan of bool arguments to methods, largely because just seeing the true or false a the callsite tends not to be descriptive enough to let folks know what the impact is. I certainly won't block this change because of it, but I'd prefer a enum arg here instead.
jasnell
approved these changes
Jul 28, 2023
|
Landed in 662fe0be9d8e |
23 tasks
21 tasks
|
@ywave620 many thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixs two issues in
TLSWrap, one of them is reported in #30896 (a 3 years log bug ...).TLSWraphas exactly oneStreamListener, however, thatStreamListenercan be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successorStreamListener.A
TLSWrapdoes not allow more than one active write, as checked in the assertion about current_write inTLSWrap::DoWrite().However, when users make use of an existing
tls.TLSSocketto establish double TLS, byeither
or
we have both of the user provided
tls.TLSSocket, tlssock and a brand new createdTLSWrapwriting to theTLSWrapbound to tlssock, which easily violates the constranint because two writers have no idea of each other.The design of the fix is:
when a
TLSWrapis created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired.