From 5c37a73fe026852aa3f4a25a92dfbcb5a3f36ddd Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 13 Nov 2020 15:40:04 -0500 Subject: [PATCH] TLS: improve compliance with shutdown standard 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. Fixes: https://github.com/nodejs/node/issues/35904 Closes: https://github.com/nodejs/node/pull/35946 Co-authored-by: Momtchil Momtchev --- lib/_http_client.js | 5 -- lib/internal/stream_base_commons.js | 17 +----- src/crypto/crypto_tls.cc | 17 ++++-- .../test-https-agent-jssocket-close.js | 57 +++++++++++++++++++ 4 files changed, 73 insertions(+), 23 deletions(-) create mode 100644 test/parallel/test-https-agent-jssocket-close.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 885016bf485..f2c0dcc98c7 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -409,11 +409,6 @@ function socketCloseListener() { const req = socket._httpMessage; debug('HTTP socket close'); - // 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(); - // NOTE: It's important to get parser here, because it could be freed by // the `socketOnData`. const parser = socket.parser; diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index 0dfee0c9c7b..1fbc67ef51e 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -207,6 +207,9 @@ function onStreamRead(arrayBuffer) { return; } + // note: handle should never deliver any more read events after this point. + // (equivalently, it should have called readStop on itself alread). + if (nread !== UV_EOF) { // CallJSOnreadMethod expects the return value to be a buffer. // Ref: https://github.com/nodejs/node/pull/34375 @@ -222,20 +225,6 @@ function onStreamRead(arrayBuffer) { if (stream[kMaybeDestroy]) stream.on('end', stream[kMaybeDestroy]); - // TODO(ronag): Without this `readStop`, `onStreamRead` - // will be called once more (i.e. after Readable.ended) - // on Windows causing a ECONNRESET, failing the - // test-https-truncate test. - if (handle.readStop) { - const err = handle.readStop(); - if (err) { - // CallJSOnreadMethod expects the return value to be a buffer. - // Ref: https://github.com/nodejs/node/pull/34375 - stream.destroy(errnoException(err, 'read')); - return; - } - } - // Push a null to signal the end of data. // Do it before `maybeDestroy` for correct order of events: // `end` -> `close` diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index f4850e425f4..25c535d84c5 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -808,6 +808,11 @@ void TLSWrap::ClearOut() { int flags = SSL_get_shutdown(ssl_.get()); if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { eof_ = true; + // FIXME: rfc5246#section-7.2.1 says we should call Destroy + // here, but Shutdown and Destroy in this class also affect the + // underlying stream. That behavior is permitted by the + // standard of the application protocol, but not of nodejs at + // the protocol layer in this file. EmitRead(UV_EOF); } @@ -921,7 +926,7 @@ bool TLSWrap::IsClosing() { int TLSWrap::ReadStart() { Debug(this, "ReadStart()"); - if (underlying_stream() != nullptr) + if (underlying_stream() != nullptr && !eof_) return underlying_stream()->ReadStart(); return 0; } @@ -1080,14 +1085,18 @@ uv_buf_t TLSWrap::OnStreamAlloc(size_t suggested_size) { void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { Debug(this, "Read %zd bytes from underlying stream", nread); + + // Ignore everything after close_notify (rfc5246#section-7.2.1) + // TODO: unregister our interest in read events + if (eof_) + return; + if (nread < 0) { // Error should be emitted only after all data was read ClearOut(); - // Ignore EOF if received close_notify if (nread == UV_EOF) { - if (eof_) - return; + // underlying stream already should have also called ReadStop on itself eof_ = true; } diff --git a/test/parallel/test-https-agent-jssocket-close.js b/test/parallel/test-https-agent-jssocket-close.js new file mode 100644 index 00000000000..9e5a6ba488b --- /dev/null +++ b/test/parallel/test-https-agent-jssocket-close.js @@ -0,0 +1,57 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); +const https = require('https'); +const key = fixtures.readKey('agent1-key.pem'); +const cert = fixtures.readKey('agent1-cert.pem'); +const net = require('net'); +const { Duplex } = require('stream'); + +class CustomAgent extends https.Agent { + createConnection(options, cb) { + const realSocket = net.createConnection(options); + const stream = new Duplex({ + emitClose: false, + read(n) { + (function retry() { + const data = realSocket.read(); + if (data === null) + return realSocket.once('readable', retry); + stream.push(data); + })(); + }, + write(chunk, enc, callback) { + realSocket.write(chunk, enc, callback); + }, + }); + realSocket.on('end', () => { + stream.push(null); + }); + + stream.on('end', common.mustCall()); + return tls.connect({ ...options, socket: stream }); + } +} + +const httpsServer = https.createServer({ + key, + cert, +}, (req, res) => { + httpsServer.close(); + res.end('hello world!'); +}); +httpsServer.listen(0, 'localhost', () => { + const agent = new CustomAgent(); + https.get({ + host: 'localhost', + port: httpsServer.address().port, + agent, + headers: { Connection: 'close' }, + rejectUnauthorized: false + }, (res) => { + res.resume(); + }); +});