From 46c5c3388d24615d8bcd887bb366d4171e99fdee Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 16 Jan 2019 11:12:30 -0800 Subject: [PATCH] src: in-source comments and minor TLS cleanups Renamed some internal C++ methods and properties for consistency, and commented SSL I/O. - Rename waiting_new_session_ after is_waiting_new_session(), instead of using reverse naming (new_session_wait_), and change "waiting" to "awaiting". - Make TLSWrap::ClearIn() return void, the value is never used. - Fix a getTicketKeys() cut-n-paste error. Since it doesn't use the arguments, remove them from the js wrapper. - Remove call of setTicketKeys(getTicketKeys()), its a no-op. PR-URL: https://github.com/nodejs/node/pull/25713 Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Ben Noordhuis --- lib/_tls_wrap.js | 6 ++-- src/node_crypto.cc | 5 +-- src/node_crypto.h | 6 ++-- src/node_crypto_bio.h | 11 ++++--- src/node_crypto_clienthello.h | 3 ++ src/stream_wrap.cc | 2 +- src/tls_wrap.cc | 60 +++++++++++++++++++++++++---------- src/tls_wrap.h | 30 ++++++++++++++---- 8 files changed, 84 insertions(+), 39 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index ef2e5e3a7bbba9..816fcb689607c7 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -1003,8 +1003,6 @@ Server.prototype.setSecureContext = function(options) { if (options.ticketKeys) { this.ticketKeys = options.ticketKeys; this.setTicketKeys(this.ticketKeys); - } else { - this.setTicketKeys(this.getTicketKeys()); } }; @@ -1021,8 +1019,8 @@ Server.prototype._setServerData = function(data) { }; -Server.prototype.getTicketKeys = function getTicketKeys(keys) { - return this._sharedCreds.context.getTicketKeys(keys); +Server.prototype.getTicketKeys = function getTicketKeys() { + return this._sharedCreds.context.getTicketKeys(); }; diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 01593914a1f501..0b6c6c8582887a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1532,7 +1532,7 @@ int SSLWrap::NewSessionCallback(SSL* s, SSL_SESSION* sess) { reinterpret_cast(session_id_data), session_id_length).ToLocalChecked(); Local argv[] = { session_id, session }; - w->new_session_wait_ = true; + w->awaiting_new_session_ = true; w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv); return 0; @@ -2128,6 +2128,7 @@ void SSLWrap::Renegotiate(const FunctionCallbackInfo& args) { ClearErrorOnReturn clear_error_on_return; + // XXX(sam) Return/throw an error, don't discard the SSL error reason. bool yes = SSL_renegotiate(w->ssl_.get()) == 1; args.GetReturnValue().Set(yes); } @@ -2161,7 +2162,7 @@ template void SSLWrap::NewSessionDone(const FunctionCallbackInfo& args) { Base* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder()); - w->new_session_wait_ = false; + w->awaiting_new_session_ = false; w->NewSessionDoneCb(); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 1b950846a7240c..32b61882593900 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -218,7 +218,7 @@ class SSLWrap { kind_(kind), next_sess_(nullptr), session_callbacks_(false), - new_session_wait_(false), + awaiting_new_session_(false), cert_cb_(nullptr), cert_cb_arg_(nullptr), cert_cb_running_(false) { @@ -234,7 +234,7 @@ class SSLWrap { inline void enable_session_callbacks() { session_callbacks_ = true; } inline bool is_server() const { return kind_ == kServer; } inline bool is_client() const { return kind_ == kClient; } - inline bool is_waiting_new_session() const { return new_session_wait_; } + inline bool is_awaiting_new_session() const { return awaiting_new_session_; } inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; } protected: @@ -325,7 +325,7 @@ class SSLWrap { SSLSessionPointer next_sess_; SSLPointer ssl_; bool session_callbacks_; - bool new_session_wait_; + bool awaiting_new_session_; // SSL_set_cert_cb CertCb cert_cb_; diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h index 1c62fbbd359405..b7f1d4f169edfe 100644 --- a/src/node_crypto_bio.h +++ b/src/node_crypto_bio.h @@ -34,8 +34,8 @@ namespace node { namespace crypto { // This class represents buffers for OpenSSL I/O, implemented as a singly-linked -// list of chunks. It can be used both for writing data from Node to OpenSSL -// and back, but only one direction per instance. +// list of chunks. It can be used either for writing data from Node to OpenSSL, +// or for reading data back, but not both. // The structure is only accessed, and owned by, the OpenSSL BIOPointer // (a.k.a. std::unique_ptr). class NodeBIO : public MemoryRetainer { @@ -80,11 +80,12 @@ class NodeBIO : public MemoryRetainer { // Put `len` bytes from `data` into buffer void Write(const char* data, size_t size); - // Return pointer to internal data and amount of - // contiguous data available for future writes + // Return pointer to contiguous block of reserved data and the size available + // for future writes. Call Commit() once the write is complete. char* PeekWritable(size_t* size); - // Commit reserved data + // Specify how much data was written into the block returned by + // PeekWritable(). void Commit(size_t size); diff --git a/src/node_crypto_clienthello.h b/src/node_crypto_clienthello.h index 2ced72c4e8d1e6..48f49771fdf401 100644 --- a/src/node_crypto_clienthello.h +++ b/src/node_crypto_clienthello.h @@ -30,6 +30,9 @@ namespace node { namespace crypto { +// Parse the client hello so we can do async session resumption. OpenSSL's +// session resumption uses synchronous callbacks, see SSL_CTX_sess_set_get_cb +// and get_session_cb. class ClientHelloParser { public: inline ClientHelloParser(); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ae54b019fea034..627bcf5949c545 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -232,7 +232,7 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) { type = uv_pipe_pending_type(reinterpret_cast(stream())); } - // We should not be getting this callback if someone as already called + // We should not be getting this callback if someone has already called // uv_close() on the handle. CHECK_EQ(persistent().IsEmpty(), false); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index d86e31c6afddf7..e467e2d167f628 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -115,6 +115,11 @@ void TLSWrap::InitSSL() { #endif // SSL_MODE_RELEASE_BUFFERS SSL_set_app_data(ssl_.get(), this); + // Using InfoCallback isn't how we are supposed to check handshake progress: + // https://github.com/openssl/openssl/issues/7199#issuecomment-420915993 + // + // Note on when this gets called on various openssl versions: + // https://github.com/openssl/openssl/issues/7199#issuecomment-420670544 SSL_set_info_callback(ssl_.get(), SSLInfoCallback); if (is_server()) { @@ -193,6 +198,9 @@ void TLSWrap::Start(const FunctionCallbackInfo& args) { // Send ClientHello handshake CHECK(wrap->is_client()); + // Seems odd to read when when we want to send, but SSL_read() triggers a + // handshake if a session isn't established, and handshake will cause + // encrypted data to become available for output. wrap->ClearOut(); wrap->EncOut(); } @@ -247,7 +255,7 @@ void TLSWrap::EncOut() { return; // Wait for `newSession` callback to be invoked - if (is_waiting_new_session()) + if (is_awaiting_new_session()) return; // Split-off queue @@ -257,7 +265,7 @@ void TLSWrap::EncOut() { if (ssl_ == nullptr) return; - // No data to write + // No encrypted output ready to write to the underlying stream. if (BIO_pending(enc_out_) == 0) { if (pending_cleartext_input_.empty()) InvokeQueued(0); @@ -479,13 +487,13 @@ void TLSWrap::ClearOut() { } -bool TLSWrap::ClearIn() { +void TLSWrap::ClearIn() { // Ignore cycling data if ClientHello wasn't yet parsed if (!hello_parser_.IsEnded()) - return false; + return; if (ssl_ == nullptr) - return false; + return; std::vector buffers; buffers.swap(pending_cleartext_input_); @@ -505,8 +513,9 @@ bool TLSWrap::ClearIn() { // All written if (i == buffers.size()) { + // We wrote all the buffers, so no writes failed (written < 0 on failure). CHECK_GE(written, 0); - return true; + return; } // Error or partial write @@ -518,6 +527,8 @@ bool TLSWrap::ClearIn() { Local arg = GetSSLError(written, &err, &error_str); if (!arg.IsEmpty()) { write_callback_scheduled_ = true; + // XXX(sam) Should forward an error object with .code/.function/.etc, if + // possible. InvokeQueued(UV_EPROTO, error_str.c_str()); } else { // Push back the not-yet-written pending buffers into their queue. @@ -528,7 +539,7 @@ bool TLSWrap::ClearIn() { buffers.end()); } - return false; + return; } @@ -584,6 +595,7 @@ void TLSWrap::ClearError() { } +// Called by StreamBase::Write() to request async write of clear text into SSL. int TLSWrap::DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count, @@ -597,18 +609,26 @@ int TLSWrap::DoWrite(WriteWrap* w, } bool empty = true; - - // Empty writes should not go through encryption process size_t i; - for (i = 0; i < count; i++) + for (i = 0; i < count; i++) { if (bufs[i].len > 0) { empty = false; break; } + } + + // We want to trigger a Write() on the underlying stream to drive the stream + // system, but don't want to encrypt empty buffers into a TLS frame, so see + // if we can find something to Write(). + // First, call ClearOut(). It does an SSL_read(), which might cause handshake + // or other internal messages to be encrypted. If it does, write them later + // with EncOut(). + // If there is still no encrypted output, call Write(bufs) on the underlying + // stream. Since the bufs are empty, it won't actually write non-TLS data + // onto the socket, we just want the side-effects. After, make sure the + // WriteWrap was accepted by the stream, or that we call Done() on it. if (empty) { ClearOut(); - // However, if there is any data that should be written to the socket, - // the callback should not be invoked immediately if (BIO_pending(enc_out_) == 0) { CHECK_NULL(current_empty_write_); current_empty_write_ = w; @@ -628,7 +648,7 @@ int TLSWrap::DoWrite(WriteWrap* w, CHECK_NULL(current_write_); current_write_ = w; - // Write queued data + // Write encrypted data to underlying stream and call Done(). if (empty) { EncOut(); return 0; @@ -647,17 +667,20 @@ int TLSWrap::DoWrite(WriteWrap* w, if (i != count) { int err; Local arg = GetSSLError(written, &err, &error_); + + // If we stopped writing because of an error, it's fatal, discard the data. if (!arg.IsEmpty()) { current_write_ = nullptr; return UV_EPROTO; } + // Otherwise, save unwritten data so it can be written later by ClearIn(). pending_cleartext_input_.insert(pending_cleartext_input_.end(), &bufs[i], &bufs[count]); } - // Try writing data immediately + // Write any encrypted/handshake output that may be ready. EncOut(); return 0; @@ -689,17 +712,20 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { return; } - // Only client connections can receive data if (ssl_ == nullptr) { EmitRead(UV_EPROTO); return; } - // Commit read data + // Commit the amount of data actually read into the peeked/allocated buffer + // from the underlying stream. crypto::NodeBIO* enc_in = crypto::NodeBIO::FromBIO(enc_in_); enc_in->Commit(nread); - // Parse ClientHello first + // Parse ClientHello first, if we need to. It's only parsed if session event + // listeners are used on the server side. "ended" is the initial state, so + // can mean parsing was never started, or that parsing is finished. Either + // way, ended means we can give the buffered data to SSL. if (!hello_parser_.IsEnded()) { size_t avail = 0; uint8_t* data = reinterpret_cast(enc_in->Peek(&avail)); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 13f2bc1c719e35..cd2701cd6d9fb7 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -72,7 +72,9 @@ class TLSWrap : public AsyncWrap, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; + // Return error_ string or nullptr if it's empty. const char* Error() const override; + // Reset error_ string to empty. Not related to "clear text". void ClearError() override; void NewSessionDoneCb(); @@ -105,11 +107,22 @@ class TLSWrap : public AsyncWrap, static void SSLInfoCallback(const SSL* ssl_, int where, int ret); void InitSSL(); - void EncOut(); - bool ClearIn(); - void ClearOut(); + // SSL has a "clear" text (unencrypted) side (to/from the node API) and + // encrypted ("enc") text side (to/from the underlying socket/stream). + // On each side data flows "in" or "out" of SSL context. + // + // EncIn() doesn't exist. Encrypted data is pushed from underlying stream into + // enc_in_ via the stream listener's OnStreamAlloc()/OnStreamRead() interface. + void EncOut(); // Write encrypted data from enc_out_ to underlying stream. + void ClearIn(); // SSL_write() clear data "in" to SSL. + void ClearOut(); // SSL_read() clear text "out" from SSL. + + // Call Done() on outstanding WriteWrap request. bool InvokeQueued(int status, const char* error_str = nullptr); + // Drive the SSL state machine by attempting to SSL_read() and SSL_write() to + // it. Transparent handshakes mean SSL_read() might trigger I/O on the + // underlying stream even if there is no clear text to read or write. inline void Cycle() { // Prevent recursion if (++cycle_depth_ > 1) @@ -118,6 +131,7 @@ class TLSWrap : public AsyncWrap, for (; cycle_depth_ > 0; cycle_depth_--) { ClearIn(); ClearOut(); + // EncIn() doesn't exist, it happens via stream listener callbacks. EncOut(); } } @@ -139,16 +153,18 @@ class TLSWrap : public AsyncWrap, static void SetVerifyMode(const v8::FunctionCallbackInfo& args); static void EnableSessionCallbacks( const v8::FunctionCallbackInfo& args); - static void EnableCertCb( - const v8::FunctionCallbackInfo& args); + static void EnableTrace(const v8::FunctionCallbackInfo& args); + static void EnableCertCb(const v8::FunctionCallbackInfo& args); static void DestroySSL(const v8::FunctionCallbackInfo& args); static void GetServername(const v8::FunctionCallbackInfo& args); static void SetServername(const v8::FunctionCallbackInfo& args); static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); crypto::SecureContext* sc_; - BIO* enc_in_ = nullptr; - BIO* enc_out_ = nullptr; + // BIO buffers hold encrypted data. + BIO* enc_in_ = nullptr; // StreamListener fills this for SSL_read(). + BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut(). + // Waiting for ClearIn() to pass to SSL_write(). std::vector pending_cleartext_input_; size_t write_size_ = 0; WriteWrap* current_write_ = nullptr;