From 29f040a769bdcd7dd0c4f895ccb93f4c2492dbda Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 31 Jan 2019 14:41:10 -0800 Subject: [PATCH] tls: in-line comments and other cleanups PR-URL: https://github.com/nodejs/node/pull/25861 Reviewed-By: Fedor Indutny Reviewed-By: Anna Henningsen Backport-PR-URL: https://github.com/nodejs/node/pull/25968 --- lib/_tls_wrap.js | 31 +++++++++++++++++++++++------- src/node_crypto.cc | 48 ++++++++++------------------------------------ src/tls_wrap.cc | 6 ++++-- src/tls_wrap.h | 4 ++++ 4 files changed, 42 insertions(+), 47 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index cefd321edaf3cc..098ed76f27b5d0 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -59,6 +59,8 @@ const kSNICallback = Symbol('snicallback'); const noop = () => {}; +// Server side times how long a handshake is taking to protect against slow +// handshakes being used for DoS. function onhandshakestart(now) { debug('onhandshakestart'); @@ -118,6 +120,7 @@ function loadSession(hello) { return owner.destroy(new ERR_SOCKET_CLOSED()); owner._handle.loadSession(session); + // Session is loaded. End the parser to allow handshaking to continue. owner._handle.endParser(); } @@ -125,6 +128,11 @@ function loadSession(hello) { hello.tlsTicket || owner.server && !owner.server.emit('resumeSession', hello.sessionId, onSession)) { + // Sessions without identifiers can't be resumed. + // Sessions with tickets can be resumed directly from the ticket, no server + // session storage is necessary. + // Without a call to a resumeSession listener, a session will never be + // loaded, so end the parser to allow handshaking to continue. owner._handle.endParser(); } } @@ -219,13 +227,17 @@ function onnewsessionclient(sessionId, session) { } function onnewsession(sessionId, session) { + debug('onnewsession'); const owner = this[owner_symbol]; + // XXX(sam) no server to emit the event on, but handshake won't continue + // unless newSessionDone() is called, should it be? if (!owner.server) return; var once = false; const done = () => { + debug('onnewsession done'); if (once) return; once = true; @@ -316,8 +328,12 @@ function TLSSocket(socket, opts) { var wrap; if ((socket instanceof net.Socket && socket._handle) || !socket) { + // 1. connected socket + // 2. no socket, one will be created with net.Socket().connect wrap = socket; } else { + // 3. socket has no handle so it is js not c++ + // 4. unconnected sockets are wrapped // 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. @@ -337,7 +353,7 @@ function TLSSocket(socket, opts) { }); // Proxy for API compatibility - this.ssl = this._handle; + this.ssl = this._handle; // C++ TLSWrap object this.on('error', this._tlsError); @@ -433,8 +449,8 @@ TLSSocket.prototype._wrapHandle = function(wrap) { const res = tls_wrap.wrap(externalStream, context.context, !!options.isServer); - res._parent = handle; - res._parentWrap = wrap; + res._parent = handle; // C++ "wrap" object: TCPWrap, JSStream, ... + res._parentWrap = wrap; // JS object: net.Socket, JSStreamSocket, ... res._secureContext = context; res.reading = handle.reading; this[kRes] = res; @@ -484,8 +500,8 @@ TLSSocket.prototype._init = function(socket, wrap) { this.server = options.server; - // For clients, we will always have either a given ca list or be using - // default one + // Clients (!isServer) always request a cert, servers request a client cert + // only on explicit configuration. const requestCert = !!options.requestCert || !options.isServer; const rejectUnauthorized = !!options.rejectUnauthorized; @@ -506,6 +522,7 @@ TLSSocket.prototype._init = function(socket, wrap) { if (this.server) { if (this.server.listenerCount('resumeSession') > 0 || this.server.listenerCount('newSession') > 0) { + // Also starts the client hello parser as a side effect. ssl.enableSessionCallbacks(); } if (this.server.listenerCount('OCSPRequest') > 0) @@ -728,7 +745,7 @@ TLSSocket.prototype.getCipher = function(err) { // TODO: support anonymous (nocert) and PSK -function onSocketSecure() { +function onServerSocketSecure() { if (this._requestCert) { const verifyError = this._handle.verifyError(); if (verifyError) { @@ -779,7 +796,7 @@ function tlsConnectionListener(rawSocket) { SNICallback: this[kSNICallback] || SNICallback }); - socket.on('secure', onSocketSecure); + socket.on('secure', onServerSocketSecure); socket[kErrorEmitted] = false; socket.on('close', onSocketClose); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 6808c17fdd9d7a..be40b91feb9286 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2643,47 +2643,19 @@ int SSLWrap::SetCACerts(SecureContext* sc) { } int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { - // Quoting SSL_set_verify(3ssl): + // From https://www.openssl.org/docs/man1.1.1/man3/SSL_verify_cb: // - // The VerifyCallback function is used to control the behaviour when - // the SSL_VERIFY_PEER flag is set. It must be supplied by the - // application and receives two arguments: preverify_ok indicates, - // whether the verification of the certificate in question was passed - // (preverify_ok=1) or not (preverify_ok=0). x509_ctx is a pointer to - // the complete context used for the certificate chain verification. - // - // The certificate chain is checked starting with the deepest nesting - // level (the root CA certificate) and worked upward to the peer's - // certificate. At each level signatures and issuer attributes are - // checked. Whenever a verification error is found, the error number is - // stored in x509_ctx and VerifyCallback is called with preverify_ok=0. - // By applying X509_CTX_store_* functions VerifyCallback can locate the - // certificate in question and perform additional steps (see EXAMPLES). - // If no error is found for a certificate, VerifyCallback is called - // with preverify_ok=1 before advancing to the next level. - // - // The return value of VerifyCallback controls the strategy of the - // further verification process. If VerifyCallback returns 0, the - // verification process is immediately stopped with "verification - // failed" state. If SSL_VERIFY_PEER is set, a verification failure - // alert is sent to the peer and the TLS/SSL handshake is terminated. If - // VerifyCallback returns 1, the verification process is continued. If + // If VerifyCallback returns 1, the verification process is continued. If // VerifyCallback always returns 1, the TLS/SSL handshake will not be - // terminated with respect to verification failures and the connection - // will be established. The calling process can however retrieve the - // error code of the last verification error using - // SSL_get_verify_result(3) or by maintaining its own error storage - // managed by VerifyCallback. - // - // If no VerifyCallback is specified, the default callback will be - // used. Its return value is identical to preverify_ok, so that any - // verification failure will lead to a termination of the TLS/SSL - // handshake with an alert message, if SSL_VERIFY_PEER is set. + // terminated with respect to verification failures and the connection will + // be established. The calling process can however retrieve the error code + // of the last verification error using SSL_get_verify_result(3) or by + // maintaining its own error storage managed by VerifyCallback. // - // Since we cannot perform I/O quickly enough in this callback, we ignore - // all preverify_ok errors and let the handshake continue. It is - // imperative that the user use Connection::VerifyError after the - // 'secure' callback has been made. + // Since we cannot perform I/O quickly enough with X509_STORE_CTX_ APIs in + // this callback, we ignore all preverify_ok errors and let the handshake + // continue. It is imperative that the user use Connection::VerifyError after + // the 'secure' callback has been made. return 1; } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 14602eb56960aa..7349c5c68da710 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -735,7 +735,7 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { if (wrap->is_server()) { bool request_cert = args[0]->IsTrue(); if (!request_cert) { - // Note reject_unauthorized ignored. + // If no cert is requested, there will be none to reject as unauthorized. verify_mode = SSL_VERIFY_NONE; } else { bool reject_unauthorized = args[1]->IsTrue(); @@ -744,7 +744,9 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { verify_mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT; } } else { - // Note request_cert and reject_unauthorized are ignored for clients. + // Servers always send a cert if the cipher is not anonymous (anon is + // disabled by default), so use VERIFY_NONE and check the cert after the + // handshake has completed. verify_mode = SSL_VERIFY_NONE; } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index cc91c2edc7f111..d3cbb992bafb29 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -80,6 +80,8 @@ class TLSWrap : public AsyncWrap, // Reset error_ string to empty. Not related to "clear text". void ClearError() override; + + // Called by the done() callback of the 'newSession' event. void NewSessionDoneCb(); // Implement MemoryRetainer: @@ -88,6 +90,8 @@ class TLSWrap : public AsyncWrap, SET_SELF_SIZE(TLSWrap) protected: + // Alternative to StreamListener::stream(), that returns a StreamBase instead + // of a StreamResource. inline StreamBase* underlying_stream() { return static_cast(stream_); }