Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 6d9af41

Browse filesBrowse files
sam-githubaddaleax
sam-github
authored and
addaleax
committed
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: #25713 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
1 parent 09a1085 commit 6d9af41
Copy full SHA for 6d9af41

File tree

Expand file treeCollapse file tree

8 files changed

+84
-39
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+84
-39
lines changed
Open diff view settings
Collapse file

‎lib/_tls_wrap.js‎

Copy file name to clipboardExpand all lines: lib/_tls_wrap.js
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -998,8 +998,6 @@ Server.prototype.setSecureContext = function(options) {
998998
if (options.ticketKeys) {
999999
this.ticketKeys = options.ticketKeys;
10001000
this.setTicketKeys(this.ticketKeys);
1001-
} else {
1002-
this.setTicketKeys(this.getTicketKeys());
10031001
}
10041002
};
10051003

@@ -1016,8 +1014,8 @@ Server.prototype._setServerData = function(data) {
10161014
};
10171015

10181016

1019-
Server.prototype.getTicketKeys = function getTicketKeys(keys) {
1020-
return this._sharedCreds.context.getTicketKeys(keys);
1017+
Server.prototype.getTicketKeys = function getTicketKeys() {
1018+
return this._sharedCreds.context.getTicketKeys();
10211019
};
10221020

10231021

Collapse file

‎src/node_crypto.cc‎

Copy file name to clipboardExpand all lines: src/node_crypto.cc
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1505,7 +1505,7 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
15051505
reinterpret_cast<const char*>(session_id_data),
15061506
session_id_length).ToLocalChecked();
15071507
Local<Value> argv[] = { session_id, session };
1508-
w->new_session_wait_ = true;
1508+
w->awaiting_new_session_ = true;
15091509
w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);
15101510

15111511
return 0;
@@ -2101,6 +2101,7 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {
21012101

21022102
ClearErrorOnReturn clear_error_on_return;
21032103

2104+
// XXX(sam) Return/throw an error, don't discard the SSL error reason.
21042105
bool yes = SSL_renegotiate(w->ssl_.get()) == 1;
21052106
args.GetReturnValue().Set(yes);
21062107
}
@@ -2134,7 +2135,7 @@ template <class Base>
21342135
void SSLWrap<Base>::NewSessionDone(const FunctionCallbackInfo<Value>& args) {
21352136
Base* w;
21362137
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
2137-
w->new_session_wait_ = false;
2138+
w->awaiting_new_session_ = false;
21382139
w->NewSessionDoneCb();
21392140
}
21402141

Collapse file

‎src/node_crypto.h‎

Copy file name to clipboardExpand all lines: src/node_crypto.h
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ class SSLWrap {
218218
kind_(kind),
219219
next_sess_(nullptr),
220220
session_callbacks_(false),
221-
new_session_wait_(false),
221+
awaiting_new_session_(false),
222222
cert_cb_(nullptr),
223223
cert_cb_arg_(nullptr),
224224
cert_cb_running_(false) {
@@ -234,7 +234,7 @@ class SSLWrap {
234234
inline void enable_session_callbacks() { session_callbacks_ = true; }
235235
inline bool is_server() const { return kind_ == kServer; }
236236
inline bool is_client() const { return kind_ == kClient; }
237-
inline bool is_waiting_new_session() const { return new_session_wait_; }
237+
inline bool is_awaiting_new_session() const { return awaiting_new_session_; }
238238
inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; }
239239

240240
protected:
@@ -325,7 +325,7 @@ class SSLWrap {
325325
SSLSessionPointer next_sess_;
326326
SSLPointer ssl_;
327327
bool session_callbacks_;
328-
bool new_session_wait_;
328+
bool awaiting_new_session_;
329329

330330
// SSL_set_cert_cb
331331
CertCb cert_cb_;
Collapse file

‎src/node_crypto_bio.h‎

Copy file name to clipboardExpand all lines: src/node_crypto_bio.h
+6-5Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ namespace node {
3434
namespace crypto {
3535

3636
// This class represents buffers for OpenSSL I/O, implemented as a singly-linked
37-
// list of chunks. It can be used both for writing data from Node to OpenSSL
38-
// and back, but only one direction per instance.
37+
// list of chunks. It can be used either for writing data from Node to OpenSSL,
38+
// or for reading data back, but not both.
3939
// The structure is only accessed, and owned by, the OpenSSL BIOPointer
4040
// (a.k.a. std::unique_ptr<BIO>).
4141
class NodeBIO : public MemoryRetainer {
@@ -80,11 +80,12 @@ class NodeBIO : public MemoryRetainer {
8080
// Put `len` bytes from `data` into buffer
8181
void Write(const char* data, size_t size);
8282

83-
// Return pointer to internal data and amount of
84-
// contiguous data available for future writes
83+
// Return pointer to contiguous block of reserved data and the size available
84+
// for future writes. Call Commit() once the write is complete.
8585
char* PeekWritable(size_t* size);
8686

87-
// Commit reserved data
87+
// Specify how much data was written into the block returned by
88+
// PeekWritable().
8889
void Commit(size_t size);
8990

9091

Collapse file

‎src/node_crypto_clienthello.h‎

Copy file name to clipboardExpand all lines: src/node_crypto_clienthello.h
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
namespace node {
3131
namespace crypto {
3232

33+
// Parse the client hello so we can do async session resumption. OpenSSL's
34+
// session resumption uses synchronous callbacks, see SSL_CTX_sess_set_get_cb
35+
// and get_session_cb.
3336
class ClientHelloParser {
3437
public:
3538
inline ClientHelloParser();
Collapse file

‎src/stream_wrap.cc‎

Copy file name to clipboardExpand all lines: src/stream_wrap.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) {
232232
type = uv_pipe_pending_type(reinterpret_cast<uv_pipe_t*>(stream()));
233233
}
234234

235-
// We should not be getting this callback if someone as already called
235+
// We should not be getting this callback if someone has already called
236236
// uv_close() on the handle.
237237
CHECK_EQ(persistent().IsEmpty(), false);
238238

Collapse file

‎src/tls_wrap.cc‎

Copy file name to clipboardExpand all lines: src/tls_wrap.cc
+43-17Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,11 @@ void TLSWrap::InitSSL() {
114114
#endif // SSL_MODE_RELEASE_BUFFERS
115115

116116
SSL_set_app_data(ssl_.get(), this);
117+
// Using InfoCallback isn't how we are supposed to check handshake progress:
118+
// https://github.com/openssl/openssl/issues/7199#issuecomment-420915993
119+
//
120+
// Note on when this gets called on various openssl versions:
121+
// https://github.com/openssl/openssl/issues/7199#issuecomment-420670544
117122
SSL_set_info_callback(ssl_.get(), SSLInfoCallback);
118123

119124
if (is_server()) {
@@ -192,6 +197,9 @@ void TLSWrap::Start(const FunctionCallbackInfo<Value>& args) {
192197

193198
// Send ClientHello handshake
194199
CHECK(wrap->is_client());
200+
// Seems odd to read when when we want to send, but SSL_read() triggers a
201+
// handshake if a session isn't established, and handshake will cause
202+
// encrypted data to become available for output.
195203
wrap->ClearOut();
196204
wrap->EncOut();
197205
}
@@ -246,7 +254,7 @@ void TLSWrap::EncOut() {
246254
return;
247255

248256
// Wait for `newSession` callback to be invoked
249-
if (is_waiting_new_session())
257+
if (is_awaiting_new_session())
250258
return;
251259

252260
// Split-off queue
@@ -256,7 +264,7 @@ void TLSWrap::EncOut() {
256264
if (ssl_ == nullptr)
257265
return;
258266

259-
// No data to write
267+
// No encrypted output ready to write to the underlying stream.
260268
if (BIO_pending(enc_out_) == 0) {
261269
if (pending_cleartext_input_.empty())
262270
InvokeQueued(0);
@@ -443,13 +451,13 @@ void TLSWrap::ClearOut() {
443451
}
444452

445453

446-
bool TLSWrap::ClearIn() {
454+
void TLSWrap::ClearIn() {
447455
// Ignore cycling data if ClientHello wasn't yet parsed
448456
if (!hello_parser_.IsEnded())
449-
return false;
457+
return;
450458

451459
if (ssl_ == nullptr)
452-
return false;
460+
return;
453461

454462
std::vector<uv_buf_t> buffers;
455463
buffers.swap(pending_cleartext_input_);
@@ -469,8 +477,9 @@ bool TLSWrap::ClearIn() {
469477

470478
// All written
471479
if (i == buffers.size()) {
480+
// We wrote all the buffers, so no writes failed (written < 0 on failure).
472481
CHECK_GE(written, 0);
473-
return true;
482+
return;
474483
}
475484

476485
// Error or partial write
@@ -482,6 +491,8 @@ bool TLSWrap::ClearIn() {
482491
Local<Value> arg = GetSSLError(written, &err, &error_str);
483492
if (!arg.IsEmpty()) {
484493
write_callback_scheduled_ = true;
494+
// XXX(sam) Should forward an error object with .code/.function/.etc, if
495+
// possible.
485496
InvokeQueued(UV_EPROTO, error_str.c_str());
486497
} else {
487498
// Push back the not-yet-written pending buffers into their queue.
@@ -492,7 +503,7 @@ bool TLSWrap::ClearIn() {
492503
buffers.end());
493504
}
494505

495-
return false;
506+
return;
496507
}
497508

498509

@@ -548,6 +559,7 @@ void TLSWrap::ClearError() {
548559
}
549560

550561

562+
// Called by StreamBase::Write() to request async write of clear text into SSL.
551563
int TLSWrap::DoWrite(WriteWrap* w,
552564
uv_buf_t* bufs,
553565
size_t count,
@@ -561,18 +573,26 @@ int TLSWrap::DoWrite(WriteWrap* w,
561573
}
562574

563575
bool empty = true;
564-
565-
// Empty writes should not go through encryption process
566576
size_t i;
567-
for (i = 0; i < count; i++)
577+
for (i = 0; i < count; i++) {
568578
if (bufs[i].len > 0) {
569579
empty = false;
570580
break;
571581
}
582+
}
583+
584+
// We want to trigger a Write() on the underlying stream to drive the stream
585+
// system, but don't want to encrypt empty buffers into a TLS frame, so see
586+
// if we can find something to Write().
587+
// First, call ClearOut(). It does an SSL_read(), which might cause handshake
588+
// or other internal messages to be encrypted. If it does, write them later
589+
// with EncOut().
590+
// If there is still no encrypted output, call Write(bufs) on the underlying
591+
// stream. Since the bufs are empty, it won't actually write non-TLS data
592+
// onto the socket, we just want the side-effects. After, make sure the
593+
// WriteWrap was accepted by the stream, or that we call Done() on it.
572594
if (empty) {
573595
ClearOut();
574-
// However, if there is any data that should be written to the socket,
575-
// the callback should not be invoked immediately
576596
if (BIO_pending(enc_out_) == 0) {
577597
CHECK_NULL(current_empty_write_);
578598
current_empty_write_ = w;
@@ -592,7 +612,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
592612
CHECK_NULL(current_write_);
593613
current_write_ = w;
594614

595-
// Write queued data
615+
// Write encrypted data to underlying stream and call Done().
596616
if (empty) {
597617
EncOut();
598618
return 0;
@@ -611,17 +631,20 @@ int TLSWrap::DoWrite(WriteWrap* w,
611631
if (i != count) {
612632
int err;
613633
Local<Value> arg = GetSSLError(written, &err, &error_);
634+
635+
// If we stopped writing because of an error, it's fatal, discard the data.
614636
if (!arg.IsEmpty()) {
615637
current_write_ = nullptr;
616638
return UV_EPROTO;
617639
}
618640

641+
// Otherwise, save unwritten data so it can be written later by ClearIn().
619642
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
620643
&bufs[i],
621644
&bufs[count]);
622645
}
623646

624-
// Try writing data immediately
647+
// Write any encrypted/handshake output that may be ready.
625648
EncOut();
626649

627650
return 0;
@@ -653,17 +676,20 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
653676
return;
654677
}
655678

656-
// Only client connections can receive data
657679
if (ssl_ == nullptr) {
658680
EmitRead(UV_EPROTO);
659681
return;
660682
}
661683

662-
// Commit read data
684+
// Commit the amount of data actually read into the peeked/allocated buffer
685+
// from the underlying stream.
663686
crypto::NodeBIO* enc_in = crypto::NodeBIO::FromBIO(enc_in_);
664687
enc_in->Commit(nread);
665688

666-
// Parse ClientHello first
689+
// Parse ClientHello first, if we need to. It's only parsed if session event
690+
// listeners are used on the server side. "ended" is the initial state, so
691+
// can mean parsing was never started, or that parsing is finished. Either
692+
// way, ended means we can give the buffered data to SSL.
667693
if (!hello_parser_.IsEnded()) {
668694
size_t avail = 0;
669695
uint8_t* data = reinterpret_cast<uint8_t*>(enc_in->Peek(&avail));
Collapse file

‎src/tls_wrap.h‎

Copy file name to clipboardExpand all lines: src/tls_wrap.h
+23-7Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,9 @@ class TLSWrap : public AsyncWrap,
7272
uv_buf_t* bufs,
7373
size_t count,
7474
uv_stream_t* send_handle) override;
75+
// Return error_ string or nullptr if it's empty.
7576
const char* Error() const override;
77+
// Reset error_ string to empty. Not related to "clear text".
7678
void ClearError() override;
7779

7880
void NewSessionDoneCb();
@@ -105,11 +107,22 @@ class TLSWrap : public AsyncWrap,
105107

106108
static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
107109
void InitSSL();
108-
void EncOut();
109-
bool ClearIn();
110-
void ClearOut();
110+
// SSL has a "clear" text (unencrypted) side (to/from the node API) and
111+
// encrypted ("enc") text side (to/from the underlying socket/stream).
112+
// On each side data flows "in" or "out" of SSL context.
113+
//
114+
// EncIn() doesn't exist. Encrypted data is pushed from underlying stream into
115+
// enc_in_ via the stream listener's OnStreamAlloc()/OnStreamRead() interface.
116+
void EncOut(); // Write encrypted data from enc_out_ to underlying stream.
117+
void ClearIn(); // SSL_write() clear data "in" to SSL.
118+
void ClearOut(); // SSL_read() clear text "out" from SSL.
119+
120+
// Call Done() on outstanding WriteWrap request.
111121
bool InvokeQueued(int status, const char* error_str = nullptr);
112122

123+
// Drive the SSL state machine by attempting to SSL_read() and SSL_write() to
124+
// it. Transparent handshakes mean SSL_read() might trigger I/O on the
125+
// underlying stream even if there is no clear text to read or write.
113126
inline void Cycle() {
114127
// Prevent recursion
115128
if (++cycle_depth_ > 1)
@@ -118,6 +131,7 @@ class TLSWrap : public AsyncWrap,
118131
for (; cycle_depth_ > 0; cycle_depth_--) {
119132
ClearIn();
120133
ClearOut();
134+
// EncIn() doesn't exist, it happens via stream listener callbacks.
121135
EncOut();
122136
}
123137
}
@@ -139,16 +153,18 @@ class TLSWrap : public AsyncWrap,
139153
static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args);
140154
static void EnableSessionCallbacks(
141155
const v8::FunctionCallbackInfo<v8::Value>& args);
142-
static void EnableCertCb(
143-
const v8::FunctionCallbackInfo<v8::Value>& args);
156+
static void EnableTrace(const v8::FunctionCallbackInfo<v8::Value>& args);
157+
static void EnableCertCb(const v8::FunctionCallbackInfo<v8::Value>& args);
144158
static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args);
145159
static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
146160
static void SetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
147161
static int SelectSNIContextCallback(SSL* s, int* ad, void* arg);
148162

149163
crypto::SecureContext* sc_;
150-
BIO* enc_in_ = nullptr;
151-
BIO* enc_out_ = nullptr;
164+
// BIO buffers hold encrypted data.
165+
BIO* enc_in_ = nullptr; // StreamListener fills this for SSL_read().
166+
BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut().
167+
// Waiting for ClearIn() to pass to SSL_write().
152168
std::vector<uv_buf_t> pending_cleartext_input_;
153169
size_t write_size_ = 0;
154170
WriteWrap* current_write_ = nullptr;

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.