From dd12dc9d3840d0f643645f610d95ad6833863204 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 19 Dec 2018 13:57:27 -0800 Subject: [PATCH 1/5] tls: remove unused ocsp extension parsing The OCSP info from parsing the TLS ClientHello has not been used since 550c263, remove it. See: https://github.com/nodejs/node/pull/1464 --- src/node_crypto.cc | 3 --- src/node_crypto_clienthello-inl.h | 1 - src/node_crypto_clienthello.cc | 13 ------------- src/node_crypto_clienthello.h | 5 ----- 4 files changed, 22 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c2faad0a5966bc..8d5ac869196318 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1559,9 +1559,6 @@ void SSLWrap::OnClientHello(void* arg, hello_obj->Set(context, env->tls_ticket_string(), Boolean::New(env->isolate(), hello.has_ticket())).FromJust(); - hello_obj->Set(context, - env->ocsp_request_string(), - Boolean::New(env->isolate(), hello.ocsp_request())).FromJust(); Local argv[] = { hello_obj }; w->MakeCallback(env->onclienthello_string(), arraysize(argv), argv); diff --git a/src/node_crypto_clienthello-inl.h b/src/node_crypto_clienthello-inl.h index 9de8f2e5fcf731..1262186a9277d2 100644 --- a/src/node_crypto_clienthello-inl.h +++ b/src/node_crypto_clienthello-inl.h @@ -48,7 +48,6 @@ inline void ClientHelloParser::Reset() { tls_ticket_ = nullptr; servername_size_ = 0; servername_ = nullptr; - ocsp_request_ = 0; } inline void ClientHelloParser::Start(ClientHelloParser::OnHelloCb onhello_cb, diff --git a/src/node_crypto_clienthello.cc b/src/node_crypto_clienthello.cc index cbe1be32737058..b0375755774318 100644 --- a/src/node_crypto_clienthello.cc +++ b/src/node_crypto_clienthello.cc @@ -112,7 +112,6 @@ void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) { hello.session_id_ = session_id_; hello.session_size_ = session_size_; hello.has_ticket_ = tls_ticket_ != nullptr && tls_ticket_size_ != 0; - hello.ocsp_request_ = ocsp_request_; hello.servername_ = servername_; hello.servername_size_ = static_cast(servername_size_); onhello_cb_(cb_arg_, hello); @@ -149,18 +148,6 @@ void ClientHelloParser::ParseExtension(const uint16_t type, } } break; - case kStatusRequest: - // We are ignoring any data, just indicating the presence of extension - if (len < kMinStatusRequestSize) - return; - - // Unknown type, ignore it - if (data[0] != kStatusRequestOCSP) - break; - - // Ignore extensions, they won't work with caching on backend anyway - ocsp_request_ = 1; - break; case kTLSSessionTicket: tls_ticket_size_ = len; tls_ticket_ = data + len; diff --git a/src/node_crypto_clienthello.h b/src/node_crypto_clienthello.h index 687e9589b6d932..2ced72c4e8d1e6 100644 --- a/src/node_crypto_clienthello.h +++ b/src/node_crypto_clienthello.h @@ -41,7 +41,6 @@ class ClientHelloParser { inline bool has_ticket() const { return has_ticket_; } inline uint8_t servername_size() const { return servername_size_; } inline const uint8_t* servername() const { return servername_; } - inline int ocsp_request() const { return ocsp_request_; } private: uint8_t session_size_; @@ -49,7 +48,6 @@ class ClientHelloParser { bool has_ticket_; uint8_t servername_size_; const uint8_t* servername_; - int ocsp_request_; friend class ClientHelloParser; }; @@ -69,7 +67,6 @@ class ClientHelloParser { static const size_t kMaxTLSFrameLen = 16 * 1024 + 5; static const size_t kMaxSSLExFrameLen = 32 * 1024; static const uint8_t kServernameHostname = 0; - static const uint8_t kStatusRequestOCSP = 1; static const size_t kMinStatusRequestSize = 5; enum ParseState { @@ -93,7 +90,6 @@ class ClientHelloParser { enum ExtensionType { kServerName = 0, - kStatusRequest = 5, kTLSSessionTicket = 35 }; @@ -115,7 +111,6 @@ class ClientHelloParser { const uint8_t* session_id_ = nullptr; uint16_t servername_size_ = 0; const uint8_t* servername_ = nullptr; - uint8_t ocsp_request_ = 0; uint16_t tls_ticket_size_ = -1; const uint8_t* tls_ticket_ = nullptr; }; From f309cec87b73a91e4393bba91c18e8f18a0c0268 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 19 Dec 2018 14:30:20 -0800 Subject: [PATCH 2/5] tls: fix initRead socket argument name "wrapped" argument is the caller's "socket", not its "wrap", and its referred to as "socket" in the comments, so call it that. --- lib/_tls_wrap.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index d2eb7befeaea37..5c943f2cd6c096 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -271,15 +271,17 @@ function onerror(err) { } } -function initRead(tls, wrapped) { +// Used by both client and server TLSSockets to start data flowing from _handle, +// read(0) causes a StreamBase::ReadStart, via Socket._read. +function initRead(tls, socket) { // If we were destroyed already don't bother reading if (!tls._handle) return; // Socket already has some buffered data - emulate receiving it - if (wrapped && wrapped.readableLength) { + if (socket && socket.readableLength) { var buf; - while ((buf = wrapped.read()) !== null) + while ((buf = socket.read()) !== null) tls._handle.receive(buf); } From 2e526ce8d971a4069bf43c339738a84f5962305b Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 19 Dec 2018 14:42:58 -0800 Subject: [PATCH 3/5] tls: do not confuse TLSSocket and Socket Don't use "socket" to describe two different objects in the same function. --- lib/_tls_wrap.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 5c943f2cd6c096..1fc339cc2e85af 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -1227,7 +1227,7 @@ exports.connect = function connect(...args) { const context = options.secureContext || tls.createSecureContext(options); - var socket = new TLSSocket(options.socket, { + var tlssock = new TLSSocket(options.socket, { pipe: !!options.path, secureContext: context, isServer: false, @@ -1238,12 +1238,14 @@ exports.connect = function connect(...args) { requestOCSP: options.requestOCSP }); - socket[kConnectOptions] = options; + tlssock[kConnectOptions] = options; if (cb) - socket.once('secureConnect', cb); + tlssock.once('secureConnect', cb); if (!options.socket) { + // If user provided the socket, its their responsibility to manage its + // connectivity. If we created one internally, we connect it. const connectOpt = { path: options.path, port: options.port, @@ -1252,13 +1254,13 @@ exports.connect = function connect(...args) { localAddress: options.localAddress, lookup: options.lookup }; - socket.connect(connectOpt, socket._start); + tlssock.connect(connectOpt, tlssock._start); } - socket._releaseControl(); + tlssock._releaseControl(); if (options.session) - socket.setSession(options.session); + tlssock.setSession(options.session); if (options.servername) { if (!ipServernameWarned && net.isIP(options.servername)) { @@ -1270,14 +1272,14 @@ exports.connect = function connect(...args) { ); ipServernameWarned = true; } - socket.setServername(options.servername); + tlssock.setServername(options.servername); } if (options.socket) - socket._start(); + tlssock._start(); - socket.on('secure', onConnectSecure); - socket.once('end', onConnectEnd); + tlssock.on('secure', onConnectSecure); + tlssock.once('end', onConnectEnd); - return socket; + return tlssock; }; From c25afe9c9561d3ace22ac79bbe720bf27c566793 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 19 Dec 2018 14:45:51 -0800 Subject: [PATCH 4/5] tls: do not confuse session and session ID session ID was named session in C++ and key in JS, Name them after what they are, as the 'newSession' event docs do. --- lib/_tls_wrap.js | 4 ++-- src/node_crypto.cc | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 1fc339cc2e85af..b07812f1a66fea 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -214,7 +214,7 @@ function requestOCSPDone(socket) { } -function onnewsession(key, session) { +function onnewsession(sessionId, session) { const owner = this[owner_symbol]; if (!owner.server) @@ -238,7 +238,7 @@ function onnewsession(key, session) { }; owner._newSessionPending = true; - if (!owner.server.emit('newSession', key, session, done)) + if (!owner.server.emit('newSession', sessionId, session, done)) done(); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8d5ac869196318..201b1815e1ae80 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1510,20 +1510,20 @@ int SSLWrap::NewSessionCallback(SSL* s, SSL_SESSION* sess) { return 0; // Serialize session - Local buff = Buffer::New(env, size).ToLocalChecked(); - unsigned char* serialized = reinterpret_cast( - Buffer::Data(buff)); - memset(serialized, 0, size); - i2d_SSL_SESSION(sess, &serialized); + Local session = Buffer::New(env, size).ToLocalChecked(); + unsigned char* session_data = reinterpret_cast( + Buffer::Data(session)); + memset(session_data, 0, size); + i2d_SSL_SESSION(sess, &session_data); unsigned int session_id_length; - const unsigned char* session_id = SSL_SESSION_get_id(sess, - &session_id_length); - Local session = Buffer::Copy( + const unsigned char* session_id_data = SSL_SESSION_get_id(sess, + &session_id_length); + Local session_id = Buffer::Copy( env, - reinterpret_cast(session_id), + reinterpret_cast(session_id_data), session_id_length).ToLocalChecked(); - Local argv[] = { session, buff }; + Local argv[] = { session_id, session }; w->new_session_wait_ = true; w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv); From fe15429efe28f3c2b0129de8a35643c7fbe38728 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 19 Dec 2018 14:14:57 -0800 Subject: [PATCH 5/5] src: use consistent names for JSStream Its confusing to call a js class with a handle a "Wrap", usually it's the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its derived from Socket, and makes a JS stream look like a Socket, so call it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated some time. --- lib/_stream_wrap.js | 2 +- lib/_tls_wrap.js | 8 +++++--- lib/internal/http2/core.js | 4 ++-- .../{wrap_js_stream.js => js_stream_socket.js} | 15 ++++++++------- node.gyp | 2 +- test/parallel/test-stream-wrap-drain.js | 4 ++-- test/parallel/test-stream-wrap-encoding.js | 3 ++- test/parallel/test-stream-wrap.js | 2 +- test/parallel/test-wrap-js-stream-destroy.js | 3 ++- test/parallel/test-wrap-js-stream-duplex.js | 3 ++- test/parallel/test-wrap-js-stream-exceptions.js | 2 +- test/parallel/test-wrap-js-stream-read-stop.js | 2 +- 12 files changed, 28 insertions(+), 22 deletions(-) rename lib/internal/{wrap_js_stream.js => js_stream_socket.js} (94%) diff --git a/lib/_stream_wrap.js b/lib/_stream_wrap.js index 10a0cf57e7789e..5b5f476948b23a 100644 --- a/lib/_stream_wrap.js +++ b/lib/_stream_wrap.js @@ -1,3 +1,3 @@ 'use strict'; -module.exports = require('internal/wrap_js_stream'); +module.exports = require('internal/js_stream_socket'); diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index b07812f1a66fea..a72091b22e97fb 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -29,7 +29,7 @@ const net = require('net'); const tls = require('tls'); const util = require('util'); const common = require('_tls_common'); -const { StreamWrap } = require('_stream_wrap'); +const JSStreamSocket = require('internal/js_stream_socket'); const { Buffer } = require('buffer'); const debug = util.debuglog('tls'); const { TCP, constants: TCPConstants } = internalBinding('tcp_wrap'); @@ -310,12 +310,14 @@ function TLSSocket(socket, opts) { this.authorizationError = null; this[kRes] = null; - // Wrap plain JS Stream into StreamWrap var wrap; if ((socket instanceof net.Socket && socket._handle) || !socket) { wrap = socket; } else { - wrap = new StreamWrap(socket); + // TLS expects to interact from C++ with a net.Socket that has a C++ stream + // handle, but a JS stream doesn't have one. Wrap it up to make it look like + // a socket. + wrap = new JSStreamSocket(socket); wrap.once('close', () => this.destroy()); } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f33ec48ff23087..2bca31e24a5473 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -22,7 +22,7 @@ const util = require('util'); const { kIncomingMessage } = require('_http_common'); const { kServerResponse } = require('_http_server'); -const { StreamWrap } = require('_stream_wrap'); +const JSStreamSocket = require('internal/js_stream_socket'); const { defaultTriggerAsyncIdScope, @@ -935,7 +935,7 @@ class Http2Session extends EventEmitter { super(); if (!socket._handle || !socket._handle._externalStream) { - socket = new StreamWrap(socket); + socket = new JSStreamSocket(socket); } // No validation is performed on the input parameters because this diff --git a/lib/internal/wrap_js_stream.js b/lib/internal/js_stream_socket.js similarity index 94% rename from lib/internal/wrap_js_stream.js rename to lib/internal/js_stream_socket.js index cf8f45aa4505ff..8343b6c2645842 100644 --- a/lib/internal/wrap_js_stream.js +++ b/lib/internal/js_stream_socket.js @@ -5,7 +5,7 @@ const util = require('util'); const { Socket } = require('net'); const { JSStream } = internalBinding('js_stream'); const uv = internalBinding('uv'); -const debug = util.debuglog('stream_wrap'); +const debug = util.debuglog('stream_socket'); const { owner_symbol } = require('internal/async_hooks').symbols; const { ERR_STREAM_WRAP } = require('internal/errors').codes; @@ -29,9 +29,9 @@ function onwrite(req, bufs) { return this[owner_symbol].doWrite(req, bufs); } * can skip going through the JS layer and let TLS access the raw C++ handle * of a net.Socket. The flipside of this is that, to maintain composability, * we need a way to create "fake" net.Socket instances that call back into a - * "real" JavaScript stream. JSStreamWrap is exactly this. + * "real" JavaScript stream. JSStreamSocket is exactly this. */ -class JSStreamWrap extends Socket { +class JSStreamSocket extends Socket { constructor(stream) { const handle = new JSStream(); handle.close = (cb) => { @@ -39,7 +39,7 @@ class JSStreamWrap extends Socket { this.doClose(cb); }; // Inside of the following functions, `this` refers to the handle - // and `this[owner_symbol]` refers to this JSStreamWrap instance. + // and `this[owner_symbol]` refers to this JSStreamSocket instance. handle.isClosing = isClosing; handle.onreadstart = onreadstart; handle.onreadstop = onreadstop; @@ -88,9 +88,10 @@ class JSStreamWrap extends Socket { this.read(0); } - // Legacy + // Allow legacy requires in the test suite to keep working: + // const { StreamWrap } = require('internal/js_stream_socket') static get StreamWrap() { - return JSStreamWrap; + return JSStreamSocket; } isClosing() { @@ -223,4 +224,4 @@ class JSStreamWrap extends Socket { } } -module.exports = JSStreamWrap; +module.exports = JSStreamSocket; diff --git a/node.gyp b/node.gyp index 8c42ef446e70b6..e36f9f26aa2f06 100644 --- a/node.gyp +++ b/node.gyp @@ -126,6 +126,7 @@ 'lib/internal/fs/watchers.js', 'lib/internal/http.js', 'lib/internal/inspector_async_hook.js', + 'lib/internal/js_stream_socket.js', 'lib/internal/linkedlist.js', 'lib/internal/modules/cjs/helpers.js', 'lib/internal/modules/cjs/loader.js', @@ -188,7 +189,6 @@ 'lib/internal/streams/state.js', 'lib/internal/streams/pipeline.js', 'lib/internal/streams/end-of-stream.js', - 'lib/internal/wrap_js_stream.js', 'deps/v8/tools/splaytree.js', 'deps/v8/tools/codemap.js', 'deps/v8/tools/consarray.js', diff --git a/test/parallel/test-stream-wrap-drain.js b/test/parallel/test-stream-wrap-drain.js index 068e2d7fd45979..77243399515730 100644 --- a/test/parallel/test-stream-wrap-drain.js +++ b/test/parallel/test-stream-wrap-drain.js @@ -2,12 +2,12 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const { StreamWrap } = require('_stream_wrap'); +const { StreamWrap } = require('internal/js_stream_socket'); const { Duplex } = require('stream'); const { internalBinding } = require('internal/test/binding'); const { ShutdownWrap } = internalBinding('stream_wrap'); -// This test makes sure that when an instance of JSStreamWrap is waiting for +// This test makes sure that when a wrapped stream is waiting for // a "drain" event to `doShutdown`, the instance will work correctly when a // "drain" event emitted. { diff --git a/test/parallel/test-stream-wrap-encoding.js b/test/parallel/test-stream-wrap-encoding.js index ce6f95fa27d68f..72804d77d6bde6 100644 --- a/test/parallel/test-stream-wrap-encoding.js +++ b/test/parallel/test-stream-wrap-encoding.js @@ -1,7 +1,8 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); -const StreamWrap = require('_stream_wrap'); +const StreamWrap = require('internal/js_stream_socket'); const Duplex = require('stream').Duplex; { diff --git a/test/parallel/test-stream-wrap.js b/test/parallel/test-stream-wrap.js index 9a279790d8aceb..670c05fe3f0307 100644 --- a/test/parallel/test-stream-wrap.js +++ b/test/parallel/test-stream-wrap.js @@ -4,7 +4,7 @@ const common = require('../common'); const assert = require('assert'); const { internalBinding } = require('internal/test/binding'); -const StreamWrap = require('_stream_wrap'); +const StreamWrap = require('internal/js_stream_socket'); const { Duplex } = require('stream'); const { ShutdownWrap } = internalBinding('stream_wrap'); diff --git a/test/parallel/test-wrap-js-stream-destroy.js b/test/parallel/test-wrap-js-stream-destroy.js index 16d3e75e2ca7e3..5c1ed1e7e65d10 100644 --- a/test/parallel/test-wrap-js-stream-destroy.js +++ b/test/parallel/test-wrap-js-stream-destroy.js @@ -1,7 +1,8 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); -const StreamWrap = require('_stream_wrap'); +const StreamWrap = require('internal/js_stream_socket'); const net = require('net'); // This test ensures that when we directly call `socket.destroy()` without diff --git a/test/parallel/test-wrap-js-stream-duplex.js b/test/parallel/test-wrap-js-stream-duplex.js index 6bd860e6ba1f56..6a817c1054cf55 100644 --- a/test/parallel/test-wrap-js-stream-duplex.js +++ b/test/parallel/test-wrap-js-stream-duplex.js @@ -1,7 +1,8 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); -const StreamWrap = require('_stream_wrap'); +const StreamWrap = require('internal/js_stream_socket'); const { PassThrough } = require('stream'); const { Socket } = require('net'); diff --git a/test/parallel/test-wrap-js-stream-exceptions.js b/test/parallel/test-wrap-js-stream-exceptions.js index 57ecd70189d106..cde7c178446a11 100644 --- a/test/parallel/test-wrap-js-stream-exceptions.js +++ b/test/parallel/test-wrap-js-stream-exceptions.js @@ -2,7 +2,7 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const JSStreamWrap = require('internal/wrap_js_stream'); +const JSStreamWrap = require('internal/js_stream_socket'); const { Duplex } = require('stream'); process.once('uncaughtException', common.mustCall((err) => { diff --git a/test/parallel/test-wrap-js-stream-read-stop.js b/test/parallel/test-wrap-js-stream-read-stop.js index f51b3ecf524e30..6d86dc2c21a519 100644 --- a/test/parallel/test-wrap-js-stream-read-stop.js +++ b/test/parallel/test-wrap-js-stream-read-stop.js @@ -3,7 +3,7 @@ require('../common'); const assert = require('assert'); -const WrapStream = require('internal/wrap_js_stream'); +const WrapStream = require('internal/js_stream_socket'); const Stream = require('stream'); class FakeStream extends Stream {