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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This commit is contained in:
Sam Roberts 2019-01-16 11:12:30 -08:00 committed by Anna Henningsen
parent dd317fc1c8
commit 46c5c3388d
No known key found for this signature in database
GPG Key ID: 9C63F3A6CD2AD8F9
8 changed files with 84 additions and 39 deletions

View File

@ -1003,8 +1003,6 @@ Server.prototype.setSecureContext = function(options) {
if (options.ticketKeys) { if (options.ticketKeys) {
this.ticketKeys = options.ticketKeys; this.ticketKeys = options.ticketKeys;
this.setTicketKeys(this.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) { Server.prototype.getTicketKeys = function getTicketKeys() {
return this._sharedCreds.context.getTicketKeys(keys); return this._sharedCreds.context.getTicketKeys();
}; };

View File

@ -1532,7 +1532,7 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
reinterpret_cast<const char*>(session_id_data), reinterpret_cast<const char*>(session_id_data),
session_id_length).ToLocalChecked(); session_id_length).ToLocalChecked();
Local<Value> argv[] = { session_id, session }; Local<Value> argv[] = { session_id, session };
w->new_session_wait_ = true; w->awaiting_new_session_ = true;
w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv); w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);
return 0; return 0;
@ -2128,6 +2128,7 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {
ClearErrorOnReturn clear_error_on_return; 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; bool yes = SSL_renegotiate(w->ssl_.get()) == 1;
args.GetReturnValue().Set(yes); args.GetReturnValue().Set(yes);
} }
@ -2161,7 +2162,7 @@ template <class Base>
void SSLWrap<Base>::NewSessionDone(const FunctionCallbackInfo<Value>& args) { void SSLWrap<Base>::NewSessionDone(const FunctionCallbackInfo<Value>& args) {
Base* w; Base* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder()); ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
w->new_session_wait_ = false; w->awaiting_new_session_ = false;
w->NewSessionDoneCb(); w->NewSessionDoneCb();
} }

View File

@ -218,7 +218,7 @@ class SSLWrap {
kind_(kind), kind_(kind),
next_sess_(nullptr), next_sess_(nullptr),
session_callbacks_(false), session_callbacks_(false),
new_session_wait_(false), awaiting_new_session_(false),
cert_cb_(nullptr), cert_cb_(nullptr),
cert_cb_arg_(nullptr), cert_cb_arg_(nullptr),
cert_cb_running_(false) { cert_cb_running_(false) {
@ -234,7 +234,7 @@ class SSLWrap {
inline void enable_session_callbacks() { session_callbacks_ = true; } inline void enable_session_callbacks() { session_callbacks_ = true; }
inline bool is_server() const { return kind_ == kServer; } inline bool is_server() const { return kind_ == kServer; }
inline bool is_client() const { return kind_ == kClient; } 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; } inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; }
protected: protected:
@ -325,7 +325,7 @@ class SSLWrap {
SSLSessionPointer next_sess_; SSLSessionPointer next_sess_;
SSLPointer ssl_; SSLPointer ssl_;
bool session_callbacks_; bool session_callbacks_;
bool new_session_wait_; bool awaiting_new_session_;
// SSL_set_cert_cb // SSL_set_cert_cb
CertCb cert_cb_; CertCb cert_cb_;

View File

@ -34,8 +34,8 @@ namespace node {
namespace crypto { namespace crypto {
// This class represents buffers for OpenSSL I/O, implemented as a singly-linked // 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 // list of chunks. It can be used either for writing data from Node to OpenSSL,
// and back, but only one direction per instance. // or for reading data back, but not both.
// The structure is only accessed, and owned by, the OpenSSL BIOPointer // The structure is only accessed, and owned by, the OpenSSL BIOPointer
// (a.k.a. std::unique_ptr<BIO>). // (a.k.a. std::unique_ptr<BIO>).
class NodeBIO : public MemoryRetainer { class NodeBIO : public MemoryRetainer {
@ -80,11 +80,12 @@ class NodeBIO : public MemoryRetainer {
// Put `len` bytes from `data` into buffer // Put `len` bytes from `data` into buffer
void Write(const char* data, size_t size); void Write(const char* data, size_t size);
// Return pointer to internal data and amount of // Return pointer to contiguous block of reserved data and the size available
// contiguous data available for future writes // for future writes. Call Commit() once the write is complete.
char* PeekWritable(size_t* size); 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); void Commit(size_t size);

View File

@ -30,6 +30,9 @@
namespace node { namespace node {
namespace crypto { 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 { class ClientHelloParser {
public: public:
inline ClientHelloParser(); inline ClientHelloParser();

View File

@ -232,7 +232,7 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) {
type = uv_pipe_pending_type(reinterpret_cast<uv_pipe_t*>(stream())); type = uv_pipe_pending_type(reinterpret_cast<uv_pipe_t*>(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. // uv_close() on the handle.
CHECK_EQ(persistent().IsEmpty(), false); CHECK_EQ(persistent().IsEmpty(), false);

View File

@ -115,6 +115,11 @@ void TLSWrap::InitSSL() {
#endif // SSL_MODE_RELEASE_BUFFERS #endif // SSL_MODE_RELEASE_BUFFERS
SSL_set_app_data(ssl_.get(), this); 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); SSL_set_info_callback(ssl_.get(), SSLInfoCallback);
if (is_server()) { if (is_server()) {
@ -193,6 +198,9 @@ void TLSWrap::Start(const FunctionCallbackInfo<Value>& args) {
// Send ClientHello handshake // Send ClientHello handshake
CHECK(wrap->is_client()); 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->ClearOut();
wrap->EncOut(); wrap->EncOut();
} }
@ -247,7 +255,7 @@ void TLSWrap::EncOut() {
return; return;
// Wait for `newSession` callback to be invoked // Wait for `newSession` callback to be invoked
if (is_waiting_new_session()) if (is_awaiting_new_session())
return; return;
// Split-off queue // Split-off queue
@ -257,7 +265,7 @@ void TLSWrap::EncOut() {
if (ssl_ == nullptr) if (ssl_ == nullptr)
return; return;
// No data to write // No encrypted output ready to write to the underlying stream.
if (BIO_pending(enc_out_) == 0) { if (BIO_pending(enc_out_) == 0) {
if (pending_cleartext_input_.empty()) if (pending_cleartext_input_.empty())
InvokeQueued(0); InvokeQueued(0);
@ -479,13 +487,13 @@ void TLSWrap::ClearOut() {
} }
bool TLSWrap::ClearIn() { void TLSWrap::ClearIn() {
// Ignore cycling data if ClientHello wasn't yet parsed // Ignore cycling data if ClientHello wasn't yet parsed
if (!hello_parser_.IsEnded()) if (!hello_parser_.IsEnded())
return false; return;
if (ssl_ == nullptr) if (ssl_ == nullptr)
return false; return;
std::vector<uv_buf_t> buffers; std::vector<uv_buf_t> buffers;
buffers.swap(pending_cleartext_input_); buffers.swap(pending_cleartext_input_);
@ -505,8 +513,9 @@ bool TLSWrap::ClearIn() {
// All written // All written
if (i == buffers.size()) { if (i == buffers.size()) {
// We wrote all the buffers, so no writes failed (written < 0 on failure).
CHECK_GE(written, 0); CHECK_GE(written, 0);
return true; return;
} }
// Error or partial write // Error or partial write
@ -518,6 +527,8 @@ bool TLSWrap::ClearIn() {
Local<Value> arg = GetSSLError(written, &err, &error_str); Local<Value> arg = GetSSLError(written, &err, &error_str);
if (!arg.IsEmpty()) { if (!arg.IsEmpty()) {
write_callback_scheduled_ = true; write_callback_scheduled_ = true;
// XXX(sam) Should forward an error object with .code/.function/.etc, if
// possible.
InvokeQueued(UV_EPROTO, error_str.c_str()); InvokeQueued(UV_EPROTO, error_str.c_str());
} else { } else {
// Push back the not-yet-written pending buffers into their queue. // Push back the not-yet-written pending buffers into their queue.
@ -528,7 +539,7 @@ bool TLSWrap::ClearIn() {
buffers.end()); 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, int TLSWrap::DoWrite(WriteWrap* w,
uv_buf_t* bufs, uv_buf_t* bufs,
size_t count, size_t count,
@ -597,18 +609,26 @@ int TLSWrap::DoWrite(WriteWrap* w,
} }
bool empty = true; bool empty = true;
// Empty writes should not go through encryption process
size_t i; size_t i;
for (i = 0; i < count; i++) for (i = 0; i < count; i++) {
if (bufs[i].len > 0) { if (bufs[i].len > 0) {
empty = false; empty = false;
break; 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) { if (empty) {
ClearOut(); 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) { if (BIO_pending(enc_out_) == 0) {
CHECK_NULL(current_empty_write_); CHECK_NULL(current_empty_write_);
current_empty_write_ = w; current_empty_write_ = w;
@ -628,7 +648,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
CHECK_NULL(current_write_); CHECK_NULL(current_write_);
current_write_ = w; current_write_ = w;
// Write queued data // Write encrypted data to underlying stream and call Done().
if (empty) { if (empty) {
EncOut(); EncOut();
return 0; return 0;
@ -647,17 +667,20 @@ int TLSWrap::DoWrite(WriteWrap* w,
if (i != count) { if (i != count) {
int err; int err;
Local<Value> arg = GetSSLError(written, &err, &error_); Local<Value> arg = GetSSLError(written, &err, &error_);
// If we stopped writing because of an error, it's fatal, discard the data.
if (!arg.IsEmpty()) { if (!arg.IsEmpty()) {
current_write_ = nullptr; current_write_ = nullptr;
return UV_EPROTO; return UV_EPROTO;
} }
// Otherwise, save unwritten data so it can be written later by ClearIn().
pending_cleartext_input_.insert(pending_cleartext_input_.end(), pending_cleartext_input_.insert(pending_cleartext_input_.end(),
&bufs[i], &bufs[i],
&bufs[count]); &bufs[count]);
} }
// Try writing data immediately // Write any encrypted/handshake output that may be ready.
EncOut(); EncOut();
return 0; return 0;
@ -689,17 +712,20 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
return; return;
} }
// Only client connections can receive data
if (ssl_ == nullptr) { if (ssl_ == nullptr) {
EmitRead(UV_EPROTO); EmitRead(UV_EPROTO);
return; 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_); crypto::NodeBIO* enc_in = crypto::NodeBIO::FromBIO(enc_in_);
enc_in->Commit(nread); 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()) { if (!hello_parser_.IsEnded()) {
size_t avail = 0; size_t avail = 0;
uint8_t* data = reinterpret_cast<uint8_t*>(enc_in->Peek(&avail)); uint8_t* data = reinterpret_cast<uint8_t*>(enc_in->Peek(&avail));

View File

@ -72,7 +72,9 @@ class TLSWrap : public AsyncWrap,
uv_buf_t* bufs, uv_buf_t* bufs,
size_t count, size_t count,
uv_stream_t* send_handle) override; uv_stream_t* send_handle) override;
// Return error_ string or nullptr if it's empty.
const char* Error() const override; const char* Error() const override;
// Reset error_ string to empty. Not related to "clear text".
void ClearError() override; void ClearError() override;
void NewSessionDoneCb(); void NewSessionDoneCb();
@ -105,11 +107,22 @@ class TLSWrap : public AsyncWrap,
static void SSLInfoCallback(const SSL* ssl_, int where, int ret); static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
void InitSSL(); void InitSSL();
void EncOut(); // SSL has a "clear" text (unencrypted) side (to/from the node API) and
bool ClearIn(); // encrypted ("enc") text side (to/from the underlying socket/stream).
void ClearOut(); // 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); 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() { inline void Cycle() {
// Prevent recursion // Prevent recursion
if (++cycle_depth_ > 1) if (++cycle_depth_ > 1)
@ -118,6 +131,7 @@ class TLSWrap : public AsyncWrap,
for (; cycle_depth_ > 0; cycle_depth_--) { for (; cycle_depth_ > 0; cycle_depth_--) {
ClearIn(); ClearIn();
ClearOut(); ClearOut();
// EncIn() doesn't exist, it happens via stream listener callbacks.
EncOut(); EncOut();
} }
} }
@ -139,16 +153,18 @@ class TLSWrap : public AsyncWrap,
static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args); static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableSessionCallbacks( static void EnableSessionCallbacks(
const v8::FunctionCallbackInfo<v8::Value>& args); const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableCertCb( static void EnableTrace(const v8::FunctionCallbackInfo<v8::Value>& args);
const v8::FunctionCallbackInfo<v8::Value>& args); static void EnableCertCb(const v8::FunctionCallbackInfo<v8::Value>& args);
static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args); static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args); static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetServername(const v8::FunctionCallbackInfo<v8::Value>& args); static void SetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); static int SelectSNIContextCallback(SSL* s, int* ad, void* arg);
crypto::SecureContext* sc_; crypto::SecureContext* sc_;
BIO* enc_in_ = nullptr; // BIO buffers hold encrypted data.
BIO* enc_out_ = nullptr; 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<uv_buf_t> pending_cleartext_input_; std::vector<uv_buf_t> pending_cleartext_input_;
size_t write_size_ = 0; size_t write_size_ = 0;
WriteWrap* current_write_ = nullptr; WriteWrap* current_write_ = nullptr;