Skip to content
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

TLS: improve compliance with shutdown standard, remove hacks #36111

Closed
wants to merge 2 commits into from

Conversation

vtjnash
Copy link
Contributor

@vtjnash vtjnash commented Nov 13, 2020

RFC 5246 section-7.2.1 requires that the implementation must immediately
stop using the stream, as it is no longer TLS-encrypted. The stream is
permitted to still pump events (and errors) to other users, but those
are now unencrypted, so we should not process them here. But therefore,
we do not want to stop the underlying stream, as there could be another
user of it, but we do remove ourselves as a listener.

The section also states that the application must destroy the stream
immediately (discarding any pending writes, and sending a close_notify
response back), but we leave that to the upper layer of the application
here, as it should be sufficient to permit standards compliant usage
just to be ignoring read events.

EDIT: In August 2018, TLS v1.3 changed that requirement, per https://tools.ietf.org/html/rfc8446#section-6.1

Does not address new feature #35904
Closes: #35946
Co-authored-by: Momtchil Momtchev momtchil@momtchev.com @mmomtchev
CI with libuv/libuv#3036: https://ci.nodejs.org/view/libuv/job/libuv-in-node/171/ (the libuv PR shouldn't be needed to pass CI, but instead should make it even harder to pass tests)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net
  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. labels Nov 13, 2020
@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 13, 2020

I'm somewhat hoping someone else will take this over and address the FIXME there, as this PR fix should address some of the CI flakiness around tls/https connections. That FIXME is related to what's causing the occasional failure in the newly added test (e.g. https://ci.nodejs.org/job/node-test-binary-windows-js-suites/6766/RUN_SUBSET=1,nodes=win10-COMPILED_BY-vs2019/testReport/junit/(root)/test/parallel_test_https_agent_jssocket_close/), though possibly not descriptive of the specific fix desired. What's happening there is a conflict between the ownership semantics of the read and write side of the JS TLSWrap implementation: on the write side, the TLSWrap object is taking full ownership (thus shutdown/destroy on the TLS also cause shutdown/destroy on the underlying socket) but on the read side, it's taking shared ownership (EOF on the read doesn't have a way to tell the underlying socket to destroy the read capabilities). There was recently a hack merged to call readStop as a way to hint to the underlying socket that it should be destroyed now, but that wasn't quite right either, as it left the stream in a purgatory half state: neither truly open (shared) nor closed (owned).

for posterity:

test.parallel/test-https-agent-jssocket-close

node:events:306
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:216:20)
Emitted 'error' event on Socket instance at:
    at emitErrorNT (node:internal/streams/destroy:188:8)
    at emitErrorCloseNT (node:internal/streams/destroy:153:3)
    at processTicksAndRejections (node:internal/process/task_queues:80:21) {
  errno: -4077,
  code: 'ECONNRESET',
  syscall: 'read'
}

@Trott Trott requested review from ronag and jasnell November 14, 2020 03:56
// Pull through final chunk, if anything is buffered.
// the ondata function will handle it properly, and this
// is a no-op if no final chunk remains.
socket.read();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment says, this should be a no-op. The underlying socket is not supposed to deliver a read event after a close event, but TLS would try to do so (while racing to destroy it first). Looking back at the commit history, it was added as a hack to work around this issue, but didn't really fix it, as evidenced by flaky CI.

Copy link
Member

@lpinca lpinca Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is buffered data (for example if there is buffered data and the socket is destroyed) shouldn't it be read? We do something similar in ws to ensure that all received data is actually used and delivered to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no data buffered here, that wouldn't make any sense as the http protocol doesn't allow for more http data to be included after the http message is ended.

Copy link
Member

@lpinca lpinca Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know that the http message ended? Isn't this only a listener for the 'close' event of the socket? That 'close' event can be emitted in the middle of a HTTP request/response.

Copy link
Member

@lpinca lpinca Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does not affect ws luckily. I've only mentioned it because the same pattern used here (to read buffered data after 'close') is also used by ws as per #36111 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have an example for why you added those lines to ws? I see only the commit where you added it:
websockets/ws@6046a28. Note that the documentation for 'error' also indicates that this call to .read should not trigger any callbacks to occur ("no further events other than 'close' should be emitted"), so by adding that comment and code there, it seems like you may be causing ws to violate the stream.Readable interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also seems like you had previously changed it to explicitly have the opposite behavior in websockets/ws@5d8ab0e, but was changed back later during a general code refactoring? From the comment description, it appears to me to say that code was intending to be be part of the 'error' handler (before that handler called destroy), and should be triggering the 'readable' callback, not the 'read' callback.

But why would the underlying socket be doing an active 'read', thus causing it to encounter an 'error', while data was already pending in the buffer? That seems like it would be disregarding of flow control, so wouldn't actually happen in a correctly implemented stream.

Copy link
Member

@lpinca lpinca Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK calling readable.read() is the only way to pull out buffered data from a readable stream after it is destroyed and in my opinion there must be a way to do that. The fact that doing so causes an additional 'data' event is a side effect that is actually very useful.

Did you have an example for why you added those lines to ws?

See #36111 (comment) and #36111 (comment). The user can call ws.terminate() and destroy the socket. When that happens there might be buffered data in the socket. That data might contain many vaid WebSocket frames that we do not want to drop.

websockets/ws@5d8ab0e is still in place, see https://github.com/websockets/ws/blob/8.0.0/lib/websocket.js#L946. Any data after the close frame must be discarded per specification.

Anyway this is not about ws. ws is not affected by this change. This is about discarding data that was previously not discarded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully followed this discussion but #39639 might be of relevance.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test fail?

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 15, 2020

Yes, the new test may fail, as I mentioned above (it's waiting for an 'end' event, but it doesn't "own" the socket shutdown, so it's racing again the 'reset' event generated by TLSWrap when it closed the socket). I'm hoping someone more core to this code (I work more on the Julia and libuv side) can take over and address the further deficiencies.

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 15, 2020

generated by TLSWrap when it closed the socket

I should perhaps clarify that this RST packet is the expected behavior of the HTTP server that's being used as the test driver here, so perhaps it's not entirely clear if this new test is fully correct, or whether it needs to also explicitly handle the 'error' event. It'd could be correct for raw TLS usage for some application, but not HTTP (which would usually choose to destroy the underlying socket, even if TLSWrap wasn't already doing that). Alternatively (or additionally), HTTP and/or TLSWrap could fully destroy the underlying socket when receiving the 'stream_close' message. That would be more fully in compliance with my reading of the standard, but also initially made some other tests unhappy (they tried to also destroy the stream, and were confused when it already was destroyed)

@ronag
Copy link
Member

ronag commented Nov 15, 2020

I think James is currently the one with best understanding of the TLS parts.

@mmomtchev
Copy link
Contributor

For me, this transforms the active close into a simultaneous close - thus eliminating the RST. It covers only the case where the TLS client is also Node - it does not solve the general case.
My opinion is that there is no other viable-long term solution than to implement active close - but as this is not available at the moment, this solution is definitely much better that the one we currently have (readStop)

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 18, 2020

I've been wondering what you meant by 'active shutdown'. It seems to be a combination of the terms 'active close' (which for http is what happens at the server always, and simply means "the party that called 'close' first". The alternative being "never close".) and of 'graceful shutdown' (a TCP feature which isn't used by TLS). Either way, that's not actually relevant here, as that state change is not under the control of the client code.

The readStop solution actually wasn't bad, but was incomplete, so it improved the situation, but didn't solve the underlying issue. To avoid this error entirely, the TLSWrap code needs to completely destroy the read end (for example, equivalent to shutdown(SHUT_RD)), and not just pause it. The standard I quote above mentions this is usually more appropriate as an application layer (http) feature, but since the write side already automatically calls 'close', the same strategy on the read side may be appropriate. I think the recommendation in the RFC is actually for the read side to call 'close' instead of 'readStop', but that required a bit more code refactoring to handle smoothly.

@mmomtchev
Copy link
Contributor

Normally, most TCP-based protocols signal in some way that the connection is to be closed - some form of a bye message, then both ends proceed to simultaneously call close().
In some cases, one end may decide to close the connection without the other expecting it.
In this case the other one may continue writing or maybe there is still some data transiting on the network.
TCP defines a proper sequence in this case:
The so-called active close end calls shutdown(SEND)
His OS sends a FIN packet
The socket goes into a half-closed state, it can still receive data, but it cannot send anymore
The other side of the connection acknowledges the situation, sends any remaining data and then finally sends its own FIN packet
The application on the active side should continue calling read()/recv() until it gets the EOF message
At this moment it should call close() completely destroying the socket as it knows that the other side won't emit anymore
In order to protect from any random data popping on closed connections, the active side will respond with a RST during the next few minutes (it is a sysctl value on Linux, registry on Windows) to any packet that bears those two port numbers - this is the so-called TIME_WAIT state
This is the so-called active TCP close - as opposed to the simultaneous one which is far more common
From what I saw from the code, libuv supports this - it can be achieved by calling uv_shutdown() which Node does not do
There is a state diagram here : https://users.cs.northwestern.edu/~agupta/cs340/project2/TCPIP_State_Transition_Diagram.pdf

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 18, 2020

Having recently looked at the code, I believe nodejs usually does send the shutdown message first (it's named 'end'), but note that the TLS specification was originally intended to diverge here from your knowledge of TCP in order to avoid a truncation attack. The shutdown message is also not required (it's not present in the diagram you sent). Please see https://tools.ietf.org/html/rfc5246#section-7.2.1 for the TLS specification however.

This is the so-called active TCP close - as opposed to the simultaneous one which is far more common

If you look at the diagram you sent, a simultaneous close is one type of active close. It is a subset, not an alternative.

@mmomtchev
Copy link
Contributor

Simultaneous close is indeed a special case of an active close where the two FIN packets are sent before one of them has been ACKed. In this case, even if the socket is destroyed by one side, there won't be a RST because there won't be anymore data.
As this is the most common case, most of the time you are good with simply calling close(). But if you plan closing without the other end expecting it, you have to do it in order to avoid the RST.
The TLS is another story - in this case it will be the TLS server which will reset your connection. This is not our case, Node which is the TLS server in this test, lacks reset capability - it is something that is on my todo list - and it is exactly by looking for a way to implement it today that I stumbled upon uv_shutdown()

@mmomtchev
Copy link
Contributor

From what I saw, it seems that the libuv API supposes that it is up to the user to do this - call uv_shutdown() then continue reading until an error, then call uv_close()
Except that Node does not do it
@jasnell, does this uv_shutdown() ring any bells? It has been there since Ryan Dahl times, the git blame is from 9 years ago? Maybe it was broken after some reshuffling of StreamBase/HandleWrap? Because there is a shutdown there?
From what I saw today in libuv, it makes me think that originally, it was assumed that it was up to the user (ie Node) to properly handle an active close? By calling uv_shutdown(), then reading until an EOF and then calling uv_close() - and that in fact - should that assumption be correct - the problem is entirely in Node

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 18, 2020

As this is the most common case, most of the time you are good with simply calling close(). But if you plan closing without the other end expecting it, you have to do it in order to avoid the RST.

This is neither necessary nor sufficient to prevent an RST packet. I realize that TCP documentation is not always clear on this point, as 'closing the stream' and 'closing the handle' are related, but not identical activities.

Except that Node does not do it

Your understanding of TLS is not entirely correct here, as TLS does not fully respect the underlying TCP expectations in this. In particular, "then reading until an EOF" is not required by the standard, and thus must be handled correctly by TLSWrap.

The uv_shutdown call should happen from here

node/lib/net.js

Lines 399 to 423 in b3b0c43

// The user has called .end(), and all the bytes have been
// sent out to the other side.
Socket.prototype._final = function(cb) {
// If still connecting - defer handling `_final` until 'connect' will happen
if (this.pending) {
debug('_final: not yet connected');
return this.once('connect', () => this._final(cb));
}
if (!this._handle)
return cb();
debug('_final: not ended, call shutdown()');
const req = new ShutdownWrap();
req.oncomplete = afterShutdown;
req.handle = this._handle;
req.callback = cb;
const err = this._handle.shutdown(req);
if (err === 1 || err === UV_ENOTCONN) // synchronous finish
return cb();
else if (err !== 0)
return cb(errnoException(err, 'shutdown'));
};

which feels accurate, since I spent some of my debugging time with a breakpoint on uv_shutdown to watch the program flow with read->write->shutdown->close, so I'm pretty confident in my observation that it already happens.

@mmomtchev
Copy link
Contributor

Who is throwing that RST in your theory? That RST can come only from the OS kernel - Node does not have that capability (yet). It can't be from the TLS implementation in Node.

Try this on Windows:

'use strict';

const http = require('http');

const data = 'hello';

const server = http.createServer(function (req, res) {
  res.setHeader('content-length', data.length);
  res.end(data);
  server.close();
});

server.listen(0, function () {
  const opts = { port: this.address().port, rejectUnauthorized: false };
  http.get(opts).on('response', function (res) {
    res.on('data', function (chunk) {
      this.pause();
      setTimeout(() => { this.resume(); }, 1);
    });
  });
});

You will get at least 50% RST probability. You can even do it on Linux if you run it in a while /bin/true loop for long enough.
The trigger is res.end() - doesn't matter if the socket is TLS, HTTP or something else.
I will continue debugging with that new understanding - that in fact Node is supposed to be calling uv_shutdown() - which as you correctly pointed is indeed happening - but I am still convinced that the reason for the RST is that the uv_close() comes immediately after the uv_shutdown() without reading between the two. Except that now it is clear that the problem is in Node, because it is up to Node to do a proper shutdown

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 18, 2020

I think you're confusing two unrelated bugs. This PR is about fixing TLSWrap. Your example code there is for http, and does not use this TLS code, and thus cannot be related. There is a bug in libuv that could possibly could cause that code to emit an RST on Windows, though I haven't been able to trigger it with that example. I do not expect that code to emit RST ever, and from running that for several minutes on both Windows and Linux, I did not see an error generated, even when heavily loaded. I have a patch up for the RST bug in libuv. Unfortunately, that patch also greatly exacerbates an existing problem in node, that has long been a source of CI problems, and for which this patch is intended to drive conversation towards a solution.

James, I'd be happy to take a call to explain and discuss fixes, if that would be helpful!

@mmomtchev
Copy link
Contributor

mmomtchev commented Nov 19, 2020

I am unable to reproduce the plain HTTP RST on Windows, maybe it was because of a modification I had in my executable or something else

What is this RST bug in libuv then?

Normally, there are only two ways to produce a RST packet:

  • Completely closing a socket that still has incoming data - which will trigger a RST when that data is processed by the kernel
  • Setting the lingering to a very aggressive value and then closing the socket - uv_tcp_close_reset() uses this method, it sets the lingering to 0 and the closes the socket provoking an artificial RST - this is meant to be used by Node to reset connections when an error is thrown in the user code, but this still hasn't been implemented

It is exactly because resetting hasn't been implemented in Node, that makes me think that only the first case is possible - and this is exactly what I see in the network dump - FIN then ACK then data which triggers RST which could have only one explanation - that the socket is already in TIME_WAIT state because close() has been called

So the reason is in the TLS code - I agree - but ignoring that last data doesn't mean not reading it from the socket? You don't have any choice on this - either you call shutdown(), then read() until an EOF then close() - either the kernel sends a RST - TCP is implemented in the kernel and you cannot diverge from it

Besides, from reading that RFC, I am coming to the conclusion that TLS accepts only one form of closing - simultaneous closing by exchanging close_notify messages? Can you confirm this? Because if this is true, than this is the reason - this would mean that active closing was never meant to be supported by the TLS layer and in this case, what is not working, is that the two TLS endpoints do not properly exchange close_notify messages in order to avoid the active closing?

@mmomtchev
Copy link
Contributor

mmomtchev commented Nov 19, 2020

Ok, in TLS 1.0 proper exchange of close_notify was mandatory - meaning that there was only simultaneous closing achieved by a proper bye sequence - as most other protocols (in fact HTTP 1.0 is one of the very few protocols to allow one-side active closing)
As lots TLS implementations didn't respect that, then they made it optional in TLS 1.1. In TLS 1.1 it is acceptable to close the connection before receiving the other end's close_notify
Then there is the truncation attack
But for us, this means reading all incoming data from the socket. There is simply no other way - ignore it, but read it. Otherwise the kernel sends a RST

@mmomtchev
Copy link
Contributor

So now that I agree that this is indeed a TLS-specific problem, there is one point we still do not agree:

  • Your PR makes it so that the client (the passive closing end) must stop reading after a close_notify
    • I agree that the data should be ignored (because of the TLS truncation attack) - this should stay
    • But the socket should still be emptied - because now you simply stop reading before receiving the RST packet - thus masking the RST when the client is Node too - this is to be added
  • But for me, the most important change will be on the server side - continuing to read all incoming data after sending the close_notify - that data should be ignored too - because of the TLS truncation attack - but the socket is to be fully drained - otherwise the RST will still be sent

@jasnell
Copy link
Member

jasnell commented Nov 19, 2020

I've been buried in a few other items and haven't really had the chance to go through this in detail... but I do plan to. Unfortunately it likely won't be until early next week most likely.

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 19, 2020

Thanks, that's great! It's holding up ~3 libuv PRs that exacerbate flaky tests in node, so your time is appreciated, though I know it's a demanding change to review.

@mmomtchev I agree that's a reasonable alternative design, but rewriting the http 1.x server spec and all implementations is a bit outside the scope of this PR. As to "they made it optional in TLS 1.1", that's instructions for the initiator (the http server), not the http client, so that isn't relevant to this PR. I initially made that same mistake in reading it, so it's understandable.

@mmomtchev
Copy link
Contributor

I am positive that the server is the problem - the ECONNRESET error is reported by the client - but the real reason this error is reported is that the server sends a RST packet. And the reason the server sends this packet is that it is not draining the incoming side of the connection after sending a TLS close_notify

Does your solution simply solve the ECONNRESET error on the client or does it eliminate the RST packet sent from the server?

I traced the JS, the Node C++ code and the libuv C code
A res.end() from JS triggers net.Socket.close()
net.Socket.close() goes directly to uv_close()
And the shutdown comes mere microseconds later triggered by a Dispatch from stream_wrap.cc:328
My analysis is that the uv_close() lies on a direct code path and the uv_shutdown() lies on a micro-task scheduled code-path

Trace uv_shutdown() and uv_close() on the server side - you will see what I mean - uv_shutdown() comes a few microseconds later

@mmomtchev
Copy link
Contributor

mmomtchev commented Nov 19, 2020

Yes, in fact I think that uv_close() is not to be called directly at all - it is to be called in the uv_shutdown() callback

@vtjnash
Copy link
Contributor Author

vtjnash commented Nov 19, 2020

Yes, that would likely be a valid alternative design, but since shutdown is not in the http spec, redesigning it to add your requirements is not in scope here. The node's http server implementation appears to meet the spec given (which permits this close call ordering), while the client does not (which is seen to abort the process or hang), so the fix is required in the node client code. The RST packet on the wire does not concern me, it's only the delivery to the application that is wrong.

@mmomtchev
Copy link
Contributor

mmomtchev commented Nov 19, 2020

Yes, I am positive, @addaleax , this bears your name
When doing res.end() from JS this ends in net.Socket.close() and then in TCPWrap::Close() and finally in HandleWrap::Close()
Here you call directly uv_close() - this is a direct, unscheduled code-path
While the shutdown lies on an indirect code path scheduled by a Dispatch micro-task in LibuvStreamWrap::CreateShutdownWrap() - here you call uv_shutdown()
The result is that the shutdown happens one or two microseconds after the close when in fact a proper close should wait for the shutdown to finish
My opinion is the uv_close() should be called only in the uv_shutdown() callback

This will require some major reshuffling of the current code?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Dec 8, 2021

Landed in f3fbeaf.

lpinca pushed a commit that referenced this pull request Dec 8, 2021
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lpinca lpinca closed this Dec 8, 2021
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@vtjnash vtjnash deleted the jn/tls-rfc5246-section-7.2.1 branch January 11, 2022 16:34
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in nodejs#35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: nodejs#36111
Fixes: nodejs#35946
Refs: libuv/libuv#3036
Refs: nodejs#35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
RFC 5246 section-7.2.1 requires that the implementation must immediately
stop reading from the stream, as it is no longer TLS-encrypted. The
underlying stream is permitted to still pump events (and errors) to
other users, but those are now unencrypted, so we should not process
them here. But therefore, we do not want to stop the underlying stream,
as there could be another user of it, but we do need to remove ourselves
as a listener.

Per TLS v1.2, we should have also destroy the TLS state entirely here
(including the writing side), but this was revised in TLS v1.3 to permit
the stream to continue to flush output.

There appears to be some inconsistencies in the way nodejs handles
ownership of the underlying stream, with `TLS.close()` on the write side
also calling shutdown on the underlying stream (thus assuming other
users of the underlying stream are not permitted), while receiving EOF
on the read side leaves the underlying channel open. These
inconsistencies are left for a later person to resolve, if the extra
functionality is needed (as described in #35904). The current goal here
is to the fix the occasional CI exceptions depending on the timing of
these kernel messages through the TCP stack.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
This reverts commit 46f36e3.

PR-URL: libuv#3006
Refs: libuv#2967
Refs: libuv#2409
Refs: libuv#2943
Refs: libuv#2968
Refs: nodejs/node#36111
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.