From a2b26d2afba4a54124ec3768bf99c388d6ad5faf Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 28 Jan 2019 13:07:56 -0800 Subject: [PATCH 1/9] deps: openssl: openssl#8096 Cherry-pick: https://github.com/openssl/openssl/commit/4af5836b55442f31795eff6c8c81ea7a1b8cf94b Original-Pr: https://github.com/openssl/openssl/pull/8096 --- deps/openssl/openssl/CHANGES | 13 +++++ .../doc/man3/SSL_CTX_set_info_callback.pod | 14 ++---- deps/openssl/openssl/include/openssl/ssl.h | 2 + deps/openssl/openssl/ssl/statem/statem.c | 6 ++- deps/openssl/openssl/ssl/statem/statem_lib.c | 11 +++-- deps/openssl/openssl/ssl/statem/statem_srvr.c | 19 ------- deps/openssl/openssl/test/sslapitest.c | 49 ++++++++----------- 7 files changed, 53 insertions(+), 61 deletions(-) diff --git a/deps/openssl/openssl/CHANGES b/deps/openssl/openssl/CHANGES index 4b68f4832909b8..4fabaa4c1bb375 100644 --- a/deps/openssl/openssl/CHANGES +++ b/deps/openssl/openssl/CHANGES @@ -7,6 +7,19 @@ https://github.com/openssl/openssl/commits/ and pick the appropriate release branch. + + Changes between 1.1.1a and 1.1.1b [xx XXX xxxx] + + *) Change the info callback signals for the start and end of a post-handshake + message exchange in TLSv1.3. In 1.1.1/1.1.1a we used SSL_CB_HANDSHAKE_START + and SSL_CB_HANDSHAKE_DONE. Experience has shown that many applications get + confused by this and assume that a TLSv1.2 renegotiation has started. This + can break KeyUpdate handling. Instead we now use + SSL_CB_POST_HANDSHAKE_START and SSL_CB_POST_HANDSHAKE_DONE. This could + break some applications that were expecting the old signals. However + without this KeyUpdate is not usable for many applications. + [Matt Caswell] + Changes between 1.1.1 and 1.1.1a [20 Nov 2018] *) Timing vulnerability in DSA signature generation diff --git a/deps/openssl/openssl/doc/man3/SSL_CTX_set_info_callback.pod b/deps/openssl/openssl/doc/man3/SSL_CTX_set_info_callback.pod index f01ca66fce7c14..2dc82cad0c3e39 100644 --- a/deps/openssl/openssl/doc/man3/SSL_CTX_set_info_callback.pod +++ b/deps/openssl/openssl/doc/man3/SSL_CTX_set_info_callback.pod @@ -92,17 +92,13 @@ Callback has been called due to an alert being sent or received. =item SSL_CB_HANDSHAKE_START -Callback has been called because a new handshake is started. In TLSv1.3 this is -also used for the start of post-handshake message exchanges such as for the -exchange of session tickets, or for key updates. It also occurs when resuming a -handshake following a pause to handle early data. +Callback has been called because a new handshake is started. It also occurs when +resuming a handshake following a pause to handle early data. -=item SSL_CB_HANDSHAKE_DONE 0x20 +=item SSL_CB_HANDSHAKE_DONE -Callback has been called because a handshake is finished. In TLSv1.3 this is -also used at the end of an exchange of post-handshake messages such as for -session tickets or key updates. It also occurs if the handshake is paused to -allow the exchange of early data. +Callback has been called because a handshake is finished. It also occurs if the +handshake is paused to allow the exchange of early data. =back diff --git a/deps/openssl/openssl/include/openssl/ssl.h b/deps/openssl/openssl/include/openssl/ssl.h index d6b1b4e6a67032..ec69993bd377fe 100644 --- a/deps/openssl/openssl/include/openssl/ssl.h +++ b/deps/openssl/openssl/include/openssl/ssl.h @@ -1054,6 +1054,8 @@ typedef enum { # define SSL_CB_CONNECT_EXIT (SSL_ST_CONNECT|SSL_CB_EXIT) # define SSL_CB_HANDSHAKE_START 0x10 # define SSL_CB_HANDSHAKE_DONE 0x20 +# define SSL_CB_POST_HANDSHAKE_START 0x40 +# define SSL_CB_POST_HANDSHAKE_DONE 0x80 /* Is the SSL_connection established? */ # define SSL_in_connect_init(a) (SSL_in_init(a) && !SSL_is_server(a)) diff --git a/deps/openssl/openssl/ssl/statem/statem.c b/deps/openssl/openssl/ssl/statem/statem.c index f76c0e48034b8f..f418676d7dcd25 100644 --- a/deps/openssl/openssl/ssl/statem/statem.c +++ b/deps/openssl/openssl/ssl/statem/statem.c @@ -342,8 +342,10 @@ static int state_machine(SSL *s, int server) } s->server = server; - if (cb != NULL) - cb(s, SSL_CB_HANDSHAKE_START, 1); + if (cb != NULL) { + if (SSL_IS_FIRST_HANDSHAKE(s) || !SSL_IS_TLS13(s)) + cb(s, SSL_CB_HANDSHAKE_START, 1); + } /* * Fatal errors in this block don't send an alert because we have diff --git a/deps/openssl/openssl/ssl/statem/statem_lib.c b/deps/openssl/openssl/ssl/statem/statem_lib.c index 4324896f500ac1..693aac6c07721b 100644 --- a/deps/openssl/openssl/ssl/statem/statem_lib.c +++ b/deps/openssl/openssl/ssl/statem/statem_lib.c @@ -1028,6 +1028,7 @@ unsigned long ssl3_output_cert_chain(SSL *s, WPACKET *pkt, CERT_PKEY *cpk) WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) { void (*cb) (const SSL *ssl, int type, int val) = NULL; + int cleanuphand = s->statem.cleanuphand; if (clearbufs) { if (!SSL_IS_DTLS(s)) { @@ -1054,7 +1055,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) * Only set if there was a Finished message and this isn't after a TLSv1.3 * post handshake exchange */ - if (s->statem.cleanuphand) { + if (cleanuphand) { /* skipped if we just sent a HelloRequest */ s->renegotiate = 0; s->new_session = 0; @@ -1132,8 +1133,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) /* The callback may expect us to not be in init at handshake done */ ossl_statem_set_in_init(s, 0); - if (cb != NULL) - cb(s, SSL_CB_HANDSHAKE_DONE, 1); + if (cb != NULL) { + if (cleanuphand + || !SSL_IS_TLS13(s) + || SSL_IS_FIRST_HANDSHAKE(s)) + cb(s, SSL_CB_HANDSHAKE_DONE, 1); + } if (!stop) { /* If we've got more work to do we go back into init */ diff --git a/deps/openssl/openssl/ssl/statem/statem_srvr.c b/deps/openssl/openssl/ssl/statem/statem_srvr.c index e7c11c4bea4dee..acd95cad5c89b9 100644 --- a/deps/openssl/openssl/ssl/statem/statem_srvr.c +++ b/deps/openssl/openssl/ssl/statem/statem_srvr.c @@ -4028,7 +4028,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) uint64_t nonce; static const unsigned char nonce_label[] = "resumption"; const EVP_MD *md = ssl_handshake_md(s); - void (*cb) (const SSL *ssl, int type, int val) = NULL; int hashleni = EVP_MD_size(md); /* Ensure cast to size_t is safe */ @@ -4040,24 +4039,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) } hashlen = (size_t)hashleni; - if (s->info_callback != NULL) - cb = s->info_callback; - else if (s->ctx->info_callback != NULL) - cb = s->ctx->info_callback; - - if (cb != NULL) { - /* - * We don't start and stop the handshake in between each ticket when - * sending more than one - but it should appear that way to the info - * callback. - */ - if (s->sent_tickets != 0) { - ossl_statem_set_in_init(s, 0); - cb(s, SSL_CB_HANDSHAKE_DONE, 1); - ossl_statem_set_in_init(s, 1); - } - cb(s, SSL_CB_HANDSHAKE_START, 1); - } /* * If we already sent one NewSessionTicket, or we resumed then * s->session may already be in a cache and so we must not modify it. diff --git a/deps/openssl/openssl/test/sslapitest.c b/deps/openssl/openssl/test/sslapitest.c index 108d57e4781cd9..73c98c99a95437 100644 --- a/deps/openssl/openssl/test/sslapitest.c +++ b/deps/openssl/openssl/test/sslapitest.c @@ -4710,18 +4710,14 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWSC"}, {SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, - {SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, - {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, - {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"}, + {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL}, + {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, + {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"}, + {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"}, + {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, + {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, + {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 client followed by resumption */ {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, @@ -4729,20 +4725,16 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRSC"}, {SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, + {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, + {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 server, early_data */ {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, @@ -4751,8 +4743,7 @@ static struct info_cb_states_st { {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, + {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 client, early_data */ @@ -4763,9 +4754,8 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { {0, NULL}, } @@ -4804,8 +4794,11 @@ static void sslapi_info_callback(const SSL *s, int where, int ret) return; } - /* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init */ - if ((where & SSL_CB_HANDSHAKE_DONE) && SSL_in_init((SSL *)s) != 0) { + /* + * Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init + */ + if ((where & SSL_CB_HANDSHAKE_DONE) + && SSL_in_init((SSL *)s) != 0) { info_cb_failed = 1; return; } From 5af0f9924502859d90a7f2ca7110dcf280ce8ad6 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 13 Feb 2019 14:54:07 -0800 Subject: [PATCH 2/9] tls: WIP enableTrace, will PR after 1.3 support --- .../asm/include/openssl/opensslconf.h | 1 - lib/_tls_wrap.js | 7 ++++++- src/tls_wrap.cc | 19 +++++++++++++++++++ src/tls_wrap.h | 1 + 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/deps/openssl/config/archs/linux-x86_64/asm/include/openssl/opensslconf.h b/deps/openssl/config/archs/linux-x86_64/asm/include/openssl/opensslconf.h index 0e8c6f508bd81f..975285b7524780 100644 --- a/deps/openssl/config/archs/linux-x86_64/asm/include/openssl/opensslconf.h +++ b/deps/openssl/config/archs/linux-x86_64/asm/include/openssl/opensslconf.h @@ -79,7 +79,6 @@ extern "C" { # define OPENSSL_NO_SCTP #endif #ifndef OPENSSL_NO_SSL_TRACE -# define OPENSSL_NO_SSL_TRACE #endif #ifndef OPENSSL_NO_SSL3 # define OPENSSL_NO_SSL3 diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index eaf937640ed088..a2c6678d2eb66e 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -742,7 +742,7 @@ function makeSocketMethodProxy(name) { [ 'getFinished', 'getPeerFinished', 'getSession', 'isSessionReused', - 'getEphemeralKeyInfo', 'getProtocol', 'getTLSTicket' + 'getEphemeralKeyInfo', 'getProtocol', 'getTLSTicket', 'enableTrace', ].forEach((method) => { TLSSocket.prototype[method] = makeSocketMethodProxy(method); }); @@ -806,6 +806,8 @@ function tlsConnectionListener(rawSocket) { ALPNProtocols: this.ALPNProtocols, SNICallback: this[kSNICallback] || SNICallback }); + if (this._enableTrace) + socket.enableTrace(); socket.on('secure', onServerSocketSecure); @@ -926,6 +928,9 @@ function Server(options, listener) { if (listener) { this.on('secureConnection', listener); } + + if (options.enableTrace) + this._enableTrace = true; } util.inherits(Server, net.Server); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index df6bb9c56fd2ee..17ceba3ca1f7b8 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -812,6 +812,24 @@ void TLSWrap::EnableSessionCallbacks( wrap); } +// XXX(sam) worth adding as a feature? +void TLSWrap::EnableTrace( + const FunctionCallbackInfo& args) { + TLSWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + +#ifndef OPENSSL_NO_SSL_TRACE + if (wrap->ssl_) { + BIO* b = BIO_new_fp(stderr, BIO_NOCLOSE | BIO_FP_TEXT); + SSL_set_msg_callback(wrap->ssl_.get(), SSL_trace); + SSL_set_msg_callback_arg(wrap->ssl_.get(), b); + + args.GetReturnValue().Set(true); + } else { + args.GetReturnValue().Set(false); + } +#endif +} void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { TLSWrap* wrap; @@ -975,6 +993,7 @@ void TLSWrap::Initialize(Local target, env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode); env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks); + env->SetProtoMethod(t, "enableTrace", EnableTrace); env->SetProtoMethod(t, "destroySSL", DestroySSL); env->SetProtoMethod(t, "enableCertCb", EnableCertCb); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index be694526abf203..b75c06a0a3af74 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -159,6 +159,7 @@ class TLSWrap : public AsyncWrap, static void SetVerifyMode(const v8::FunctionCallbackInfo& args); static void EnableSessionCallbacks( 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); From 3113c9a1c9c72325fb164fa855b8dc2832a6e171 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 13 Feb 2019 14:57:19 -0800 Subject: [PATCH 3/9] tls: fprintf tracing XXX won't PR - but wish c++ had a debug() equiv --- src/node_crypto.cc | 26 +++++++++ src/tls_wrap.cc | 128 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9bd557f9ac3f69..2c50d7b96acac7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -943,6 +943,10 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) { } +#if 1 +#define fprintf(...) +#endif + void SecureContext::SetCiphers(const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); @@ -1280,6 +1284,13 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { memcpy(wrap->ticket_key_hmac_, Buffer::Data(args[0]) + 16, 16); memcpy(wrap->ticket_key_aes_, Buffer::Data(args[0]) + 32, 16); + fprintf(stderr, "SetTicketKeys %02x%02x%02x%02x\n", + wrap->ticket_key_name_[0], + wrap->ticket_key_name_[1], + wrap->ticket_key_name_[2], + wrap->ticket_key_name_[3] + ); + args.GetReturnValue().Set(true); #endif // !def(OPENSSL_NO_TLSEXT) && def(SSL_CTX_get_tlsext_ticket_keys) } @@ -1399,6 +1410,9 @@ int SecureContext::TicketCompatibilityCallback(SSL* ssl, if (enc) { memcpy(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_)); + fprintf(stderr, "TicketKeyCallback() issue name %02x%02x%02x%02x\n", + name[0], name[1], name[2], name[3]); + if (RAND_bytes(iv, 16) <= 0 || EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, sc->ticket_key_aes_, iv) <= 0 || @@ -1409,6 +1423,11 @@ int SecureContext::TicketCompatibilityCallback(SSL* ssl, return 1; } + fprintf(stderr, "TicketKeyCallback() reuse name %02x%02x%02x%02x with %02x%02x%02x%02x, match? %d\n", + name[0], name[1], name[2], name[3], + sc->ticket_key_name_[0], sc->ticket_key_name_[1], sc->ticket_key_name_[2], sc->ticket_key_name_[3], + 0 == memcmp(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_))); + if (memcmp(name, sc->ticket_key_name_, sizeof(sc->ticket_key_name_)) != 0) { // The ticket key name does not match. Discard the ticket. return 0; @@ -1518,6 +1537,9 @@ int SSLWrap::NewSessionCallback(SSL* s, SSL_SESSION* sess) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); +//fprintf(stderr, "%s SSLWrap::NewSessionCallback() enabled? %d\n", +// w->is_server() ? "server" : "client", w->session_callbacks_); + if (!w->session_callbacks_) return 0; @@ -6175,6 +6197,10 @@ void TimingSafeEqual(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(CRYPTO_memcmp(buf1, buf2, buf_length) == 0); } +#ifdef fprintf +#undef fprintf +#endif + void InitCryptoOnce() { SSL_load_error_strings(); OPENSSL_no_config(); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 17ceba3ca1f7b8..864bc70f551ff4 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -29,6 +29,10 @@ #include "stream_base-inl.h" #include "util-inl.h" +#if 1 +#define fprintf(...) +#endif + namespace node { using crypto::SecureContext; @@ -80,6 +84,10 @@ TLSWrap::~TLSWrap() { bool TLSWrap::InvokeQueued(int status, const char* error_str) { + fprintf(stderr, "%s TLSWrap::InvokeQueued(%d, %s) scheduled? %d current? %p\n", + is_server() ? "server" : "client", + status, error_str, write_callback_scheduled_, current_write_); + if (!write_callback_scheduled_) return false; @@ -89,6 +97,7 @@ bool TLSWrap::InvokeQueued(int status, const char* error_str) { w->Done(status, error_str); } + fprintf(stderr, "...TLSWrap::InvokeQueued()\n"); return true; } @@ -99,6 +108,10 @@ void TLSWrap::NewSessionDoneCb() { void TLSWrap::InitSSL() { + fprintf(stderr, "%s TLSWrap::InitSSL() set handshake state: %s\n", + is_server() ? "server" : "client", + is_server() ? "accept" : "connect"); + // Initialize SSL – OpenSSL takes ownership of these. enc_in_ = crypto::NodeBIO::New(env()).release(); enc_out_ = crypto::NodeBIO::New(env()).release(); @@ -179,6 +192,11 @@ void TLSWrap::Receive(const FunctionCallbackInfo& args) { char* data = Buffer::Data(args[0]); size_t len = Buffer::Length(args[0]); + fprintf(stderr, "%s TLSWrap::Receive(buf.len %zd)\n", + wrap->is_server() ? "server" : "client", + len + ); + // Copy given buffer entirely or partiall if handle becomes closed while (len > 0 && wrap->IsAlive() && !wrap->IsClosing()) { uv_buf_t buf = wrap->OnStreamAlloc(len); @@ -201,6 +219,8 @@ void TLSWrap::Start(const FunctionCallbackInfo& args) { wrap->started_ = true; + fprintf(stderr, "client TLSWrap::Start()\n"); + // Send ClientHello handshake CHECK(wrap->is_client()); // Seems odd to read when when we want to send, but SSL_read() triggers a @@ -210,7 +230,11 @@ void TLSWrap::Start(const FunctionCallbackInfo& args) { wrap->EncOut(); } - +const char* itox(int i) { + static char b[10]; + sprintf(b, "%#x", i); + return b; +} void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { if (!(where & (SSL_CB_HANDSHAKE_START | SSL_CB_HANDSHAKE_DONE))) return; @@ -218,6 +242,22 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { // SSL_renegotiate_pending() should take `const SSL*`, but it does not. SSL* ssl = const_cast(ssl_); TLSWrap* c = static_cast(SSL_get_app_data(ssl_)); + + fprintf(stderr, "%s TLSWrap::SSLInfoCallback(where %s, alert %s) established? %d\n" + " state %#x %s: %s %s\n" + , + c->is_server() ? "server" : "client", + where & SSL_CB_HANDSHAKE_START ? "SSL_CB_HANDSHAKE_START" : + where & SSL_CB_HANDSHAKE_DONE ? "SSL_CB_HANDSHAKE_DONE" : + itox(where), + SSL_alert_type_string(ret), + c->established_, + SSL_get_state(ssl_), + SSL_state_string(ssl_), + SSL_state_string_long(ssl_), + SSL_get_version(c->ssl_.get()) + ); + Environment* env = c->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -246,10 +286,19 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { c->MakeCallback(callback.As(), 0, nullptr); } } + fprintf(stderr, "...TLSWrap::SSLInfoCallback()\n"); } void TLSWrap::EncOut() { + fprintf(stderr, + "%s TLSWrap::EncOut() established? %d pending=%d write_size=%d waiting? %d\n", + is_server() ? "server" : "client", + established_, BIO_pending(enc_out_), + (int)write_size_, is_awaiting_new_session() + ); + + // Ignore cycling data if ClientHello wasn't yet parsed if (!hello_parser_.IsEnded()) return; @@ -290,6 +339,8 @@ void TLSWrap::EncOut() { buf[i] = uv_buf_init(data[i], size[i]); StreamWriteResult res = underlying_stream()->Write(bufs, count); + fprintf(stderr, " write %zd bufs res .err %d .async? %d\n", + count, res.err, res.async); if (res.err != 0) { InvokeQueued(res.err); return; @@ -307,6 +358,10 @@ void TLSWrap::EncOut() { void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { + fprintf(stderr, + "%s TLSWrap::OnStreamAfterWrite(%p %d) current? %d\n", + is_server() ? "server" : "client", req_wrap, status, !!current_empty_write_ + ); if (current_empty_write_ != nullptr) { WriteWrap* finishing = current_empty_write_; current_empty_write_ = nullptr; @@ -337,9 +392,32 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { // Try writing more data write_size_ = 0; EncOut(); + fprintf(stderr, "...TLSWrap::OnStreamAfterWrite()\n"); } +void DebugGetSSLError(int status, int err, const std::string& msg, const Local& arg, int line) { +#ifndef fprintf + const char* estr; + switch(err) { + case SSL_ERROR_NONE: estr = "SSL_ERROR_NONE:"; break; + case SSL_ERROR_WANT_READ: estr = "SSL_ERROR_WANT_READ"; break; + case SSL_ERROR_WANT_WRITE: estr = "SSL_ERROR_WANT_WRITE"; break; + case SSL_ERROR_WANT_X509_LOOKUP: estr = "SSL_ERROR_WANT_X509_LOOKUP"; break; + case SSL_ERROR_ZERO_RETURN: estr = "SSL_ERROR_ZERO_RETURN"; break; + case SSL_ERROR_SSL: estr = "SSL_ERROR_SSL"; break; + case SSL_ERROR_SYSCALL: estr = "SSL_ERROR_SYSCALL"; break; + default: UNREACHABLE(); break; + } + fprintf(stderr, " GetSSLError() => %s: err? %s err msg '%s' (line %d)\n", + estr, + arg.IsEmpty() ? "no" : "yes", + msg.c_str(), + line + ); +#endif +} + Local TLSWrap::GetSSLError(int status, int* err, std::string* msg) { EscapableHandleScope scope(env()->isolate()); @@ -422,6 +500,12 @@ Local TLSWrap::GetSSLError(int status, int* err, std::string* msg) { void TLSWrap::ClearOut() { + fprintf(stderr, + "%s TLSWrap::ClearOut() established? %d parse hello? %d eof? %d ssl? %d\n", + is_server() ? "server" : "client", + established_, !hello_parser_.IsEnded(), eof_, ssl_ != nullptr + ); + // Ignore cycling data if ClientHello wasn't yet parsed if (!hello_parser_.IsEnded()) return; @@ -439,6 +523,7 @@ void TLSWrap::ClearOut() { int read; for (;;) { read = SSL_read(ssl_.get(), out, sizeof(out)); + fprintf(stderr, " SSL_read() => %d\n", read); if (read <= 0) break; @@ -466,6 +551,7 @@ void TLSWrap::ClearOut() { int flags = SSL_get_shutdown(ssl_.get()); if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { + fprintf(stderr, " SSL_get_shutdown() => SSL_RECEIVED_SHUTDOWN\n"); eof_ = true; EmitRead(UV_EOF); } @@ -476,7 +562,9 @@ void TLSWrap::ClearOut() { if (read <= 0) { HandleScope handle_scope(env()->isolate()); int err; - Local arg = GetSSLError(read, &err, nullptr); + std::string msg; + Local arg = GetSSLError(read, &err, &msg); + DebugGetSSLError(read, err, msg, arg, __LINE__); // Ignore ZERO_RETURN after EOF, it is basically not a error if (err == SSL_ERROR_ZERO_RETURN && eof_) @@ -495,6 +583,13 @@ void TLSWrap::ClearOut() { void TLSWrap::ClearIn() { + fprintf(stderr, + "%s TLSWrap::ClearIn() established? %d parse hello? %d ssl? %d pending=%d\n", + is_server() ? "server" : "client", + established_, !hello_parser_.IsEnded(), ssl_ != nullptr, + (int)pending_cleartext_input_.size() + ); + // Ignore cycling data if ClientHello wasn't yet parsed if (!hello_parser_.IsEnded()) return; @@ -532,6 +627,7 @@ void TLSWrap::ClearIn() { int err; std::string error_str; Local arg = GetSSLError(written, &err, &error_str); + DebugGetSSLError(written, err, error_str, arg, __LINE__); if (!arg.IsEmpty()) { write_callback_scheduled_ = true; // XXX(sam) Should forward an error object with .code/.function/.etc, if @@ -624,6 +720,14 @@ int TLSWrap::DoWrite(WriteWrap* w, } } + fprintf(stderr, "%s TLSWrap::DoWrite() established? %d count %zd empty? %d\n", + is_server() ? "server" : "client", + established_, + count, + empty + ); + + // 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(). @@ -666,6 +770,7 @@ int TLSWrap::DoWrite(WriteWrap* w, int written = 0; for (i = 0; i < count; i++) { written = SSL_write(ssl_.get(), bufs[i].base, bufs[i].len); + fprintf(stderr, " SSL_write([%zd].len %zd) => %d\n", i, bufs[i].len, written); CHECK(written == -1 || written == static_cast(bufs[i].len)); if (written == -1) break; @@ -674,6 +779,7 @@ int TLSWrap::DoWrite(WriteWrap* w, if (i != count) { int err; Local arg = GetSSLError(written, &err, &error_); + DebugGetSSLError(written, err, error_, arg, __LINE__); // If we stopped writing because of an error, it's fatal, discard the data. if (!arg.IsEmpty()) { @@ -690,6 +796,7 @@ int TLSWrap::DoWrite(WriteWrap* w, // Write any encrypted/handshake output that may be ready. EncOut(); + fprintf(stderr, "...TLSWrap::DoWrite()\n"); return 0; } @@ -704,6 +811,15 @@ uv_buf_t TLSWrap::OnStreamAlloc(size_t suggested_size) { void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { + fprintf(stderr, "%s TLSWrap::OnStreamRead(nread %zd) established? %d ssl? %d parsing? %d eof? %d\n", + is_server() ? "server" : "client", + nread, + established_, + !!ssl_, + !hello_parser_.IsEnded(), + eof_ + ); + if (nread < 0) { // Error should be emitted only after all data was read ClearOut(); @@ -751,6 +867,12 @@ ShutdownWrap* TLSWrap::CreateShutdownWrap(Local req_wrap_object) { int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) { + fprintf(stderr, "%s TLSWrap::DoShutdown() established? %d ssl? %d\n", + is_server() ? "server" : "client", + established_, + !!ssl_ + ); + crypto::MarkPopErrorOnReturn mark_pop_error_on_return; if (ssl_ && SSL_shutdown(ssl_.get()) == 0) @@ -800,6 +922,8 @@ void TLSWrap::EnableSessionCallbacks( TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK_NOT_NULL(wrap->ssl_); + fprintf(stderr, "%s TLSWrap::EnableSessionCallbacks()\n", + wrap->is_server() ? "server" : "client"); wrap->enable_session_callbacks(); // Clients don't use the HelloParser. From b2ed3a075bdd24de93e17a8834657d8025487585 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 28 Nov 2018 17:58:08 -0800 Subject: [PATCH 4/9] tls: tls1.3 WIP --- lib/_tls_common.js | 43 +++++- lib/_tls_wrap.js | 87 +++++++++-- lib/internal/stream_base_commons.js | 4 + lib/tls.js | 2 +- lib/util.js | 6 +- src/node_constants.cc | 1 + src/node_constants.h | 8 +- src/node_crypto.cc | 145 +++++++++++++----- src/node_crypto.h | 5 + src/node_crypto_clienthello.cc | 2 + src/node_crypto_clienthello.h | 6 + src/tls_wrap.cc | 24 ++- test/async-hooks/test-graph.tls-write-12.js | 11 ++ test/async-hooks/test-graph.tls-write.js | 11 +- test/async-hooks/test-tlswrap.js | 8 +- .../test-https-client-renegotiation-limit.js | 3 + test/parallel/test-https-client-resume.js | 3 +- test/parallel/test-tls-alert-handling.js | 6 +- .../test-tls-async-cb-after-socket-end.js | 5 +- test/parallel/test-tls-cli-min-version-1.0.js | 2 +- test/parallel/test-tls-cli-min-version-1.1.js | 2 +- test/parallel/test-tls-client-auth.js | 35 ++++- .../test-tls-client-getephemeralkeyinfo.js | 3 + test/parallel/test-tls-client-reject-12.js | 13 ++ test/parallel/test-tls-client-reject.js | 15 +- .../test-tls-client-renegotiation-13.js | 38 +++++ .../test-tls-client-renegotiation-limit.js | 3 + test/parallel/test-tls-client-resume-12.js | 13 ++ test/parallel/test-tls-client-resume.js | 45 +++++- test/parallel/test-tls-destroy-stream.js | 13 +- .../test-tls-disable-renegotiation.js | 13 ++ test/parallel/test-tls-min-max-version.js | 22 ++- test/parallel/test-tls-ocsp-callback.js | 10 +- test/parallel/test-tls-server-verify.js | 2 + test/parallel/test-tls-set-ciphers.js | 133 ++++++++++------ test/parallel/test-tls-ticket-12.js | 12 ++ test/parallel/test-tls-ticket-cluster.js | 20 ++- test/parallel/test-tls-ticket.js | 55 ++++++- 38 files changed, 676 insertions(+), 153 deletions(-) create mode 100644 test/async-hooks/test-graph.tls-write-12.js create mode 100644 test/parallel/test-tls-client-reject-12.js create mode 100644 test/parallel/test-tls-client-renegotiation-13.js create mode 100644 test/parallel/test-tls-client-resume-12.js create mode 100644 test/parallel/test-tls-ticket-12.js diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 7ddb0d4757eb80..1c3187e21b2dfb 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -35,6 +35,7 @@ const { TLS1_VERSION, TLS1_1_VERSION, TLS1_2_VERSION, + TLS1_3_VERSION, } = internalBinding('constants').crypto; // Lazily loaded from internal/crypto/util. @@ -45,6 +46,7 @@ function toV(which, v, def) { if (v === 'TLSv1') return TLS1_VERSION; if (v === 'TLSv1.1') return TLS1_1_VERSION; if (v === 'TLSv1.2') return TLS1_2_VERSION; + if (v === 'TLSv1.3') return TLS1_3_VERSION; throw new ERR_TLS_INVALID_PROTOCOL_VERSION(v, which); } @@ -148,10 +150,43 @@ exports.createSecureContext = function createSecureContext(options) { } } - if (options.ciphers) - c.context.setCiphers(options.ciphers); - else - c.context.setCiphers(tls.DEFAULT_CIPHERS); + if (options.ciphers && typeof options.ciphers !== 'string') { + // Compatible with THROW_ERR_MISSING_ARGS() in SecureContext::SetCiphers(). + const err = new TypeError('Ciphers must be a string'); + err.code = 'ERR_INVALID_ARG_TYPE'; + throw err; + + // XXX modern code would do this, change now, or in semver-major update? + throw new ERR_INVALID_ARG_TYPE( + 'options.ciphers', + 'string', + options.ciphers + ); + } + + // Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below, + // cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3 + // cipher suites all have a standard name format beginning with TLS_, so split + // the ciphers and pass them to the appropriate API. + let ciphers = (options.ciphers || tls.DEFAULT_CIPHERS).split(':'); + let cipherList = ciphers.filter(_ => !_.match(/^TLS_/)).join(':'); + let cipherSuites = ciphers.filter(_ => _.match(/^TLS_/)).join(':'); + + // Set cipher suites first, so that setCiphers() will error if there are + // neither TLSv1.2 nor TLSv1.3 cipher suites enabled. + // XXX test to confirm this works as intended, particularly that it detects + // bad TLS1.3 ciphers and also when a bad TLS1.2 cipher list causes there to + // be no cipher suites at all. + c.context.setCipherSuites(cipherSuites); + c.context.setCiphers(cipherList); + + if (cipherSuites === '' && c.context.getMaxProto() > TLS1_2_VERSION + && c.context.getMinProto() < TLS1_3_VERSION) + c.context.setMaxProto(TLS1_2_VERSION); + + if (cipherList === '' && c.context.getMinProto() < TLS1_3_VERSION + && c.context.getMaxProto() > TLS1_2_VERSION) + c.context.setMinProto(TLS1_3_VERSION); if (options.ecdhCurve === undefined) c.context.setECDHCurve(tls.DEFAULT_ECDH_CURVE); diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index a2c6678d2eb66e..d751e44bb12093 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -65,7 +65,7 @@ let ipServernameWarned = false; // Server side times how long a handshake is taking to protect against slow // handshakes being used for DoS. function onhandshakestart(now) { - debug('onhandshakestart'); + debug('server onhandshakestart'); const { lastHandshakeTime } = this; assert(now >= lastHandshakeTime, @@ -83,6 +83,9 @@ function onhandshakestart(now) { this.handshakes++; const owner = this[owner_symbol]; + + assert(owner._tlsOptions.isServer); + if (this.handshakes > tls.CLIENT_RENEG_LIMIT) { owner._emitTLSError(new ERR_TLS_SESSION_ATTACK()); return; @@ -93,9 +96,10 @@ function onhandshakestart(now) { } function onhandshakedone() { - debug('onhandshakedone'); + debug('server onhandshakedone'); const owner = this[owner_symbol]; + assert(owner._tlsOptions.isServer); // `newSession` callback wasn't called yet if (owner._newSessionPending) { @@ -108,10 +112,15 @@ function onhandshakedone() { function loadSession(hello) { + debug('server onclienthello', + 'sessionid.len', hello.sessionId.length, + 'ticket?', hello.tlsTicket + ); const owner = this[owner_symbol]; var once = false; function onSession(err, session) { + debug('server resumeSession callback(err %j, sess? %s)', err, !!session); if (once) return owner.destroy(new ERR_MULTIPLE_CALLBACK()); once = true; @@ -140,7 +149,7 @@ function loadSession(hello) { } } - +// XXX crappy name, since it might load SNI, or might request OCSP :-( function loadSNI(info) { const owner = this[owner_symbol]; const servername = info.servername; @@ -167,7 +176,7 @@ function loadSNI(info) { }); } - +// XXX rename socket to tlsSock function requestOCSP(socket, info) { if (!info.OCSPRequest || !socket.server) return requestOCSPDone(socket); @@ -193,6 +202,8 @@ function requestOCSP(socket, info) { let once = false; const onOCSP = (err, response) => { + debug('server OCSPRequest done', 'handle?', !!socket._handle, 'once?', once, + 'response?', !!response, 'err?', err); if (once) return socket.destroy(new ERR_MULTIPLE_CALLBACK()); once = true; @@ -208,6 +219,7 @@ function requestOCSP(socket, info) { requestOCSPDone(socket); }; + debug('server oncertcb emit OCSPRequest'); socket.server.emit('OCSPRequest', ctx.getCertificate(), ctx.getIssuer(), @@ -215,16 +227,17 @@ function requestOCSP(socket, info) { } function requestOCSPDone(socket) { + debug('server certcb done'); try { socket._handle.certCbDone(); } catch (e) { + debug('server certcb done errored', e); socket.destroy(e); } } - function onnewsessionclient(sessionId, session) { - debug('client onnewsessionclient', sessionId, session); + debug('client emit session'); const owner = this[owner_symbol]; owner.emit('session', session); } @@ -263,11 +276,15 @@ function onnewsession(sessionId, session) { function onocspresponse(resp) { + debug('client onocspresponse'); this[owner_symbol].emit('OCSPResponse', resp); } function onerror(err) { const owner = this[owner_symbol]; + debug('%s onerror %s had? %j', + owner._tlsOptions.isServer ? 'server' : 'client', err, + owner._hadError); if (owner._hadError) return; @@ -285,7 +302,7 @@ function onerror(err) { // Ignore server's authorization errors owner.destroy(); } else { - // Throw error + // Emit error owner._emitTLSError(err); } } @@ -293,6 +310,11 @@ function onerror(err) { // Used by both client and server TLSSockets to start data flowing from _handle, // read(0) causes a StreamBase::ReadStart, via Socket._read. function initRead(tlsSocket, socket) { + debug('%s initRead', + tlsSocket._tlsOptions.isServer ? 'server' : 'client', + 'handle?', !!tlsSocket._handle, + 'buffered?', !!socket && socket.readableLength + ); // If we were destroyed already don't bother reading if (!tlsSocket._handle) return; @@ -497,12 +519,17 @@ TLSSocket.prototype._destroySSL = function _destroySSL() { this.ssl = null; }; +// Constructor guts, arbitrarily factored out. TLSSocket.prototype._init = function(socket, wrap) { var options = this._tlsOptions; var ssl = this._handle; - this.server = options.server; + debug('%s _init', + options.isServer ? 'server' : 'client', + 'handle?', !!ssl + ); + // Clients (!isServer) always request a cert, servers request a client cert // only on explicit configuration. const requestCert = !!options.requestCert || !options.isServer; @@ -533,7 +560,10 @@ TLSSocket.prototype._init = function(socket, wrap) { } } else { ssl.onhandshakestart = noop; - ssl.onhandshakedone = this._finishInit.bind(this); + ssl.onhandshakedone = () => { + debug('client onhandshakedone'); + this._finishInit(); + }; ssl.onocspresponse = onocspresponse; if (options.session) @@ -604,6 +634,11 @@ TLSSocket.prototype.renegotiate = function(options, callback) { if (callback !== undefined && typeof callback !== 'function') throw new ERR_INVALID_CALLBACK(); + debug('%s renegotiate()', + this._tlsOptions.isServer ? 'server' : 'client', + 'destroyed?', this.destroyed + ); + if (this.destroyed) return; @@ -671,9 +706,25 @@ TLSSocket.prototype._releaseControl = function() { }; TLSSocket.prototype._finishInit = function() { - debug('secure established'); + // Guard against getting onhandshakedone() after .destroy(). + // * 1.2: If destroy() during onocspresponse(), then write of next handshake + // record fails, the handshake done info callbacks does not occur, and the + // socket closes. + // * 1.3: The OCSP response comes in the same record that finishes handshake, + // so even after .destroy(), the handshake done info callback occurs + // immediately after onocspresponse(). Ignore it. + if (!this._handle) + return; + this.alpnProtocol = this._handle.getALPNNegotiatedProtocol(); this.servername = this._handle.getServername(); + + debug('%s _finishInit', + this._tlsOptions.isServer ? 'server' : 'client', + 'handle?', !!this._handle, + 'alpn', this.alpnProtocol, + 'servername', this.servername); + this._secureEstablished = true; if (this._tlsOptions.handshakeTimeout > 0) this.setTimeout(0, this._handleTimeout); @@ -681,6 +732,12 @@ TLSSocket.prototype._finishInit = function() { }; TLSSocket.prototype._start = function() { + debug('%s _start', + this._tlsOptions.isServer ? 'server' : 'client', + 'handle?', !!this._handle, + 'connecting?', this.connecting, + 'requestOCSP?', !!this._tlsOptions.requestOCSP, + ); if (this.connecting) { this.once('connect', this._start); return; @@ -690,7 +747,6 @@ TLSSocket.prototype._start = function() { if (!this._handle) return; - debug('start'); if (this._tlsOptions.requestOCSP) this._handle.requestOCSP(); this._handle.start(); @@ -769,13 +825,16 @@ function onServerSocketSecure() { } } - if (!this.destroyed && this._releaseControl()) + if (!this.destroyed && this._releaseControl()) { + debug('server emit secureConnection'); this._tlsOptions.server.emit('secureConnection', this); + } } function onSocketTLSError(err) { if (!this._controlReleased && !this[kErrorEmitted]) { this[kErrorEmitted] = true; + debug('server emit tlsClientError:', err); this._tlsOptions.server.emit('tlsClientError', err, this); } } @@ -796,6 +855,7 @@ function onSocketClose(err) { } function tlsConnectionListener(rawSocket) { + debug('net.Server.on(connection): new TLSSocket') const socket = new TLSSocket(rawSocket, { secureContext: this._sharedCreds, isServer: true, @@ -1189,6 +1249,7 @@ function onConnectSecure() { const ekeyinfo = this.getEphemeralKeyInfo(); if (ekeyinfo.type === 'DH' && ekeyinfo.size < options.minDHSize) { const err = new ERR_TLS_DH_PARAM_SIZE(ekeyinfo.size); + debug('client emit:', err); this.emit('error', err); this.destroy(); return; @@ -1215,10 +1276,12 @@ function onConnectSecure() { this.destroy(verifyError); return; } else { + debug('client emit secureConnect'); this.emit('secureConnect'); } } else { this.authorized = true; + debug('client emit secureConnect'); this.emit('secureConnect'); } diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index 6de03c09d83d1b..e4cad88aceabef 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -29,6 +29,8 @@ const kUpdateTimer = Symbol('kUpdateTimer'); const kAfterAsyncWrite = Symbol('kAfterAsyncWrite'); const kSession = Symbol('session'); +const debug = require('util').debuglog('stream'); + function handleWriteReq(req, data, encoding) { const { handle } = req; @@ -65,6 +67,8 @@ function handleWriteReq(req, data, encoding) { } function onWriteComplete(status) { + debug('onWriteComplete', status, this.error); + const stream = this.handle[owner_symbol]; if (stream.destroyed) { diff --git a/lib/tls.js b/lib/tls.js index 645c3e9269a624..46248eb8086e91 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -54,7 +54,7 @@ exports.DEFAULT_CIPHERS = exports.DEFAULT_ECDH_CURVE = 'auto'; -exports.DEFAULT_MAX_VERSION = 'TLSv1.2'; +exports.DEFAULT_MAX_VERSION = 'TLSv1.3'; if (getOptionValue('--tls-v1.0')) exports.DEFAULT_MIN_VERSION = 'TLSv1'; diff --git a/lib/util.js b/lib/util.js index 43e2c01301a3eb..448d49bb72515e 100644 --- a/lib/util.js +++ b/lib/util.js @@ -206,7 +206,11 @@ function debuglog(set) { emitWarningIfNeeded(set); debugs[set] = function debug() { const msg = exports.format.apply(exports, arguments); - console.error('%s %d: %s', set, pid, msg); + //console.error('%s %d: %s', set, pid, msg); + // XXX debug() uses console.log, which uses streams... making debugging + // stream internals horrid. hack: redirect to rawDebug + const log = exports.format('%s %d: %s', set, pid, msg); + process._rawDebug(log); }; } else { debugs[set] = function debug() {}; diff --git a/src/node_constants.cc b/src/node_constants.cc index debcf0e10045a1..a68661d0dba73b 100644 --- a/src/node_constants.cc +++ b/src/node_constants.cc @@ -1242,6 +1242,7 @@ void DefineCryptoConstants(Local target) { NODE_DEFINE_CONSTANT(target, TLS1_VERSION); NODE_DEFINE_CONSTANT(target, TLS1_1_VERSION); NODE_DEFINE_CONSTANT(target, TLS1_2_VERSION); + NODE_DEFINE_CONSTANT(target, TLS1_3_VERSION); #endif NODE_DEFINE_CONSTANT(target, INT_MAX); } diff --git a/src/node_constants.h b/src/node_constants.h index 6f73fb4d7d9bfc..af5aa002eb5795 100644 --- a/src/node_constants.h +++ b/src/node_constants.h @@ -41,7 +41,13 @@ #define RSA_PSS_SALTLEN_AUTO -2 #endif -#define DEFAULT_CIPHER_LIST_CORE "ECDHE-RSA-AES128-GCM-SHA256:" \ +// TLSv1.3 suites start with TLS_, and are the OpenSSL defaults, see: +// https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_ciphersuites.html +#define DEFAULT_CIPHER_LIST_CORE \ + "TLS_AES_256_GCM_SHA384:" \ + "TLS_CHACHA20_POLY1305_SHA256:" \ + "TLS_AES_128_GCM_SHA256:" \ + "ECDHE-RSA-AES128-GCM-SHA256:" \ "ECDHE-ECDSA-AES128-GCM-SHA256:" \ "ECDHE-RSA-AES256-GCM-SHA384:" \ "ECDHE-ECDSA-AES256-GCM-SHA384:" \ diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2c50d7b96acac7..8d8845f57bafd3 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -347,9 +347,14 @@ void SecureContext::Initialize(Environment* env, Local target) { env->SetProtoMethod(t, "addCACert", AddCACert); env->SetProtoMethod(t, "addCRL", AddCRL); env->SetProtoMethod(t, "addRootCerts", AddRootCerts); + env->SetProtoMethod(t, "setCipherSuites", SetCipherSuites); env->SetProtoMethod(t, "setCiphers", SetCiphers); env->SetProtoMethod(t, "setECDHCurve", SetECDHCurve); env->SetProtoMethod(t, "setDHParam", SetDHParam); + env->SetProtoMethod(t, "setMaxProto", SetMaxProto); + env->SetProtoMethod(t, "setMinProto", SetMinProto); + env->SetProtoMethod(t, "getMaxProto", GetMaxProto); + env->SetProtoMethod(t, "getMinProto", GetMinProto); env->SetProtoMethod(t, "setOptions", SetOptions); env->SetProtoMethod(t, "setSessionIdContext", SetSessionIdContext); env->SetProtoMethod(t, "setSessionTimeout", SetSessionTimeout); @@ -401,6 +406,8 @@ void SecureContext::New(const FunctionCallbackInfo& args) { } +const int MAX_SUPPORTED_VERSION = TLS1_2_VERSION; + void SecureContext::Init(const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); @@ -414,13 +421,16 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { int max_version = args[2].As()->Value(); const SSL_METHOD* method = TLS_method(); + if (max_version == 0) + max_version = MAX_SUPPORTED_VERSION; + if (args[0]->IsString()) { const node::Utf8Value sslmethod(env->isolate(), args[0]); // Note that SSLv2 and SSLv3 are disallowed but SSLv23_method and friends // are still accepted. They are OpenSSL's way of saying that all known - // protocols are supported unless explicitly disabled (which we do below - // for SSLv2 and SSLv3.) + // protocols below TLS 1.3 are supported unless explicitly disabled (which + // we do below for SSLv2 and SSLv3.) if (strcmp(*sslmethod, "SSLv2_method") == 0) { THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, "SSLv2 methods disabled"); return; @@ -440,21 +450,23 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, "SSLv3 methods disabled"); return; } else if (strcmp(*sslmethod, "SSLv23_method") == 0) { - // noop + max_version = TLS1_2_VERSION; } else if (strcmp(*sslmethod, "SSLv23_server_method") == 0) { + max_version = TLS1_2_VERSION; method = TLS_server_method(); } else if (strcmp(*sslmethod, "SSLv23_client_method") == 0) { + max_version = TLS1_2_VERSION; method = TLS_client_method(); } else if (strcmp(*sslmethod, "TLS_method") == 0) { min_version = 0; - max_version = 0; + max_version = MAX_SUPPORTED_VERSION; } else if (strcmp(*sslmethod, "TLS_server_method") == 0) { min_version = 0; - max_version = 0; + max_version = MAX_SUPPORTED_VERSION; method = TLS_server_method(); } else if (strcmp(*sslmethod, "TLS_client_method") == 0) { min_version = 0; - max_version = 0; + max_version = MAX_SUPPORTED_VERSION; method = TLS_client_method(); } else if (strcmp(*sslmethod, "TLSv1_method") == 0) { min_version = TLS1_VERSION; @@ -518,12 +530,6 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { SSL_SESS_CACHE_NO_AUTO_CLEAR); SSL_CTX_set_min_proto_version(sc->ctx_.get(), min_version); - - if (max_version == 0) { - // Selecting some secureProtocol methods allows the TLS version to be "any - // supported", but we don't support TLSv1.3, even if OpenSSL does. - max_version = TLS1_2_VERSION; - } SSL_CTX_set_max_proto_version(sc->ctx_.get(), max_version); // OpenSSL 1.1.0 changed the ticket key size, but the OpenSSL 1.0.x size was @@ -947,40 +953,44 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) { #define fprintf(...) #endif -void SecureContext::SetCiphers(const FunctionCallbackInfo& args) { +void SecureContext::SetCipherSuites(const FunctionCallbackInfo& args) { + // If TLSv1.3 is not supported, silently ignore its cipher suites. +#ifdef TLS1_3_VERSION SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); Environment* env = sc->env(); ClearErrorOnReturn clear_error_on_return; - if (args.Length() != 1) { - return THROW_ERR_MISSING_ARGS(env, "Ciphers argument is mandatory"); + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsString()); + + const node::Utf8Value ciphers(args.GetIsolate(), args[0]); + if (!SSL_CTX_set_ciphersuites(sc->ctx_.get(), *ciphers)) { + unsigned long err = ERR_get_error(); // NOLINT(runtime/int) + if (!err) { + // XXX confirm failure with no error is possible + return env->ThrowError("Failed to set ciphers"); + } + return ThrowCryptoError(env, err); } +#endif +} - THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Ciphers"); - // Note: set_ciphersuites() is for TLSv1.3 and was introduced in openssl - // 1.1.1, set_cipher_list() is for TLSv1.2 and earlier. - // - // In openssl 1.1.0, set_cipher_list() would error if it resulted in no - // TLSv1.2 (and earlier) cipher suites, and there is no TLSv1.3 support. - // - // In openssl 1.1.1, set_cipher_list() will not error if it results in no - // TLSv1.2 cipher suites if there are any TLSv1.3 cipher suites, which there - // are by default. There will be an error later, during the handshake, but - // that results in an async error event, rather than a sync error thrown, - // which is a semver-major change for the tls API. - // - // Since we don't currently support TLSv1.3, work around this by removing the - // TLSv1.3 cipher suites, so we get backwards compatible synchronous errors. +void SecureContext::SetCiphers(const FunctionCallbackInfo& args) { + SecureContext* sc; + ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + Environment* env = sc->env(); + ClearErrorOnReturn clear_error_on_return; + + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsString()); + const node::Utf8Value ciphers(args.GetIsolate(), args[0]); - if ( -#ifdef TLS1_3_VERSION - !SSL_CTX_set_ciphersuites(sc->ctx_.get(), "") || -#endif - !SSL_CTX_set_cipher_list(sc->ctx_.get(), *ciphers)) { + if (!SSL_CTX_set_cipher_list(sc->ctx_.get(), *ciphers)) { unsigned long err = ERR_get_error(); // NOLINT(runtime/int) if (!err) { + // XXX confirm failure with no error is possible return env->ThrowError("Failed to set ciphers"); } return ThrowCryptoError(env, err); @@ -1051,6 +1061,54 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo& args) { } +void SecureContext::SetMinProto(const FunctionCallbackInfo& args) { + SecureContext* sc; + ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsInt32()); + + int version = args[0].As()->Value(); + + CHECK(SSL_CTX_set_min_proto_version(sc->ctx_.get(), version)); +} + + +void SecureContext::SetMaxProto(const FunctionCallbackInfo& args) { + SecureContext* sc; + ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + + CHECK_EQ(args.Length(), 1); + CHECK(args[0]->IsInt32()); + + int version = args[0].As()->Value(); + + CHECK(SSL_CTX_set_max_proto_version(sc->ctx_.get(), version)); +} + + +void SecureContext::GetMinProto(const FunctionCallbackInfo& args) { + SecureContext* sc; + ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + + CHECK_EQ(args.Length(), 0); + + long version = SSL_CTX_get_min_proto_version(sc->ctx_.get()); + args.GetReturnValue().Set(static_cast(version)); +} + + +void SecureContext::GetMaxProto(const FunctionCallbackInfo& args) { + SecureContext* sc; + ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); + + CHECK_EQ(args.Length(), 0); + + long version = SSL_CTX_get_max_proto_version(sc->ctx_.get()); + args.GetReturnValue().Set(static_cast(version)); +} + + void SecureContext::SetOptions(const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); @@ -1269,6 +1327,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); Environment* env = wrap->env(); + // XXX(sam) Move type and len check to js, and CHECK() in C++. if (args.Length() < 1) { return THROW_ERR_MISSING_ARGS(env, "Ticket keys argument is mandatory"); } @@ -2143,6 +2202,7 @@ void SSLWrap::LoadSession(const FunctionCallbackInfo& args) { Base* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder()); + // XXX(sam) check arg length and types in js, and CHECK in c++ if (args.Length() >= 1 && Buffer::HasInstance(args[0])) { ssize_t slen = Buffer::Length(args[0]); char* sbuf = Buffer::Data(args[0]); @@ -2273,6 +2333,7 @@ void SSLWrap::GetEphemeralKeyInfo( case EVP_PKEY_EC: // TODO(shigeki) Change this to EVP_PKEY_X25519 and add EVP_PKEY_X448 // after upgrading to 1.1.1. + // XXX figure out what shigeki means, and do it case NID_X25519: { const char* curve_name; @@ -2294,9 +2355,12 @@ void SSLWrap::GetEphemeralKeyInfo( EVP_PKEY_bits(key))).FromJust(); } break; + default: + UNREACHABLE(); // XXX @shigeki - true? Only EC and DH for ephemeral? } EVP_PKEY_free(key); } + // XXX(sam) semver-major: else return ThrowCryptoError(env, ERR_get_error()) return args.GetReturnValue().Set(info); } @@ -2512,7 +2576,10 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { w->MakeCallback(env->onocspresponse_string(), 1, &arg); - // Somehow, client is expecting different return value here + // No async acceptance is possible, so always return 1 to accept the + // response. The listener for 'OCSPResponse' event has no control over + // return value, but it can .destroy() the connection if the response is not + // acceptable. return 1; } else { // Outgoing response @@ -2544,6 +2611,8 @@ void SSLWrap::WaitForCertCb(CertCb cb, void* arg) { } +// XXX TLSv1.3, ensure this is called at the right time, when servername and +// OCSP extensions have been received template int SSLWrap::SSLCertCallback(SSL* s, void* arg) { Base* w = static_cast(SSL_get_app_data(s)); @@ -2555,6 +2624,8 @@ int SSLWrap::SSLCertCallback(SSL* s, void* arg) { return 1; if (w->cert_cb_running_) + // Not an error. Suspend handshake with SSL_ERROR_WANT_X509_LOOKUP, and + // handshake will continue after certcb is done. return -1; Environment* env = w->env(); @@ -2630,6 +2701,8 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { if (rv) rv = w->SetCACerts(sc); if (!rv) { + // XXX(sam) not clear why sometimes we throw error, and sometimes we call + // onerror(). Both cause .destroy(), but onerror does a bit more. unsigned long err = ERR_get_error(); // NOLINT(runtime/int) if (!err) return env->ThrowError("CertCbDone"); diff --git a/src/node_crypto.h b/src/node_crypto.h index 7c346a6c1435d1..9d237985d6e1d8 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -146,6 +146,7 @@ class SecureContext : public BaseObject { static void AddCACert(const v8::FunctionCallbackInfo& args); static void AddCRL(const v8::FunctionCallbackInfo& args); static void AddRootCerts(const v8::FunctionCallbackInfo& args); + static void SetCipherSuites(const v8::FunctionCallbackInfo& args); static void SetCiphers(const v8::FunctionCallbackInfo& args); static void SetECDHCurve(const v8::FunctionCallbackInfo& args); static void SetDHParam(const v8::FunctionCallbackInfo& args); @@ -154,6 +155,10 @@ class SecureContext : public BaseObject { const v8::FunctionCallbackInfo& args); static void SetSessionTimeout( const v8::FunctionCallbackInfo& args); + static void SetMinProto(const v8::FunctionCallbackInfo& args); + static void SetMaxProto(const v8::FunctionCallbackInfo& args); + static void GetMinProto(const v8::FunctionCallbackInfo& args); + static void GetMaxProto(const v8::FunctionCallbackInfo& args); static void Close(const v8::FunctionCallbackInfo& args); static void LoadPKCS12(const v8::FunctionCallbackInfo& args); #ifndef OPENSSL_NO_ENGINE diff --git a/src/node_crypto_clienthello.cc b/src/node_crypto_clienthello.cc index b0375755774318..bf7fae3704ebc5 100644 --- a/src/node_crypto_clienthello.cc +++ b/src/node_crypto_clienthello.cc @@ -85,6 +85,8 @@ void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) { // (3,2) TLS v1.1 // (3,3) TLS v1.2 // + // Note that TLS v1.3 uses a TLS v1.2 handshake so requires no specific + // support here. if (data[body_offset_ + 4] != 0x03 || data[body_offset_ + 5] < 0x01 || data[body_offset_ + 5] > 0x03) { diff --git a/src/node_crypto_clienthello.h b/src/node_crypto_clienthello.h index 48f49771fdf401..5bcc1c89ba7850 100644 --- a/src/node_crypto_clienthello.h +++ b/src/node_crypto_clienthello.h @@ -33,6 +33,12 @@ 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. +// +// TLS1.3 handshakes masquerade as TLS1.2 session resumption, and to do this, +// they always include a session_id in the ClientHello, making up a bogus value +// if necessary. The parser can't know if its a bogus id, and will cause a +// 'newSession' event to be emitted. This should do no harm, the id won't be +// found, and the handshake will continue. class ClientHelloParser { public: inline ClientHelloParser(); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 864bc70f551ff4..dbdf1ccff89779 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -125,6 +125,12 @@ void TLSWrap::InitSSL() { SSL_set_mode(ssl_.get(), SSL_MODE_RELEASE_BUFFERS); #endif // SSL_MODE_RELEASE_BUFFERS + // XXX Revert 1.1.1 change that made this the default? Its not clear if it + // matters. Notes suggest that this is a work-around for common misuses of + // OpenSSL read API related to select()... not clear if it effects our code. + // - https://wiki.openssl.org/index.php/TLS1.3#Non-application_data_records + //SSL_clear_mode(ssl_.get(), SSL_MODE_AUTO_RETRY); + 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 @@ -263,7 +269,15 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { Context::Scope context_scope(env->context()); Local object = c->object(); - if (where & SSL_CB_HANDSHAKE_START) { + // Session ticket read/write triggers a info callback start/done pair... + // check the session state to distinguish tickets from handshakes. + // XXX https://github.com/openssl/openssl/pull/8096 makes that unnecessary + //int state = SSL_get_state(ssl_); + if (where & SSL_CB_HANDSHAKE_START + //&& state != TLS_ST_SW_SESSION_TICKET + ) { + // Start is tracked to limit number and frequency of renegotiation attempts, + // since excessive renegotiation may be an attack. Local callback; if (object->Get(env->context(), env->onhandshakestart_string()) @@ -276,7 +290,13 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { // SSL_CB_HANDSHAKE_START and SSL_CB_HANDSHAKE_DONE are called // sending HelloRequest in OpenSSL-1.1.1. // We need to check whether this is in a renegotiation state or not. - if (where & SSL_CB_HANDSHAKE_DONE && !SSL_renegotiate_pending(ssl)) { + // XXX do we need the pending check with patched openssl? Yes, at least for + // - parallel/test-tls-server-verify + if (where & SSL_CB_HANDSHAKE_DONE + && !SSL_renegotiate_pending(ssl) + // && state == TLS_ST_OK + ) { + CHECK(!SSL_renegotiate_pending(ssl)); Local callback; c->established_ = true; diff --git a/test/async-hooks/test-graph.tls-write-12.js b/test/async-hooks/test-graph.tls-write-12.js new file mode 100644 index 00000000000000..748234402c0e92 --- /dev/null +++ b/test/async-hooks/test-graph.tls-write-12.js @@ -0,0 +1,11 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const tls = require('tls'); + +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +require('./test-graph.tls-write.js'); diff --git a/test/async-hooks/test-graph.tls-write.js b/test/async-hooks/test-graph.tls-write.js index 2ea03283e4c861..580264316dea90 100644 --- a/test/async-hooks/test-graph.tls-write.js +++ b/test/async-hooks/test-graph.tls-write.js @@ -39,8 +39,10 @@ function onlistening() { function onsecureConnection() {} function onsecureConnect() { - // Destroying client socket - this.destroy(); + // end() client socket, which causes slightly different hook events than + // destroy(), but with TLS1.3 destroy() rips the connection down before the + // server completes the handshake. + this.end(); // Closing server server.close(common.mustCall(onserverClosed)); @@ -68,7 +70,8 @@ function onexit() { { type: 'WRITEWRAP', id: 'write:2', triggerAsyncId: null }, { type: 'WRITEWRAP', id: 'write:3', triggerAsyncId: null }, { type: 'WRITEWRAP', id: 'write:4', triggerAsyncId: null }, - { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:1' }, - { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:2' } ] + { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' }, + { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' }, + ] ); } diff --git a/test/async-hooks/test-tlswrap.js b/test/async-hooks/test-tlswrap.js index 354cd7ad0cd91d..fb8662f56aad3c 100644 --- a/test/async-hooks/test-tlswrap.js +++ b/test/async-hooks/test-tlswrap.js @@ -15,6 +15,10 @@ const { checkInvocations } = require('./hook-checks'); const hooks = initHooks(); hooks.enable(); +// XXX(sam) assumes server handshake completes before client, true for 1.2, not +// for 1.3. Might need a rewrite for TLS1.3. +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + // // Creating server and listening on port // @@ -52,7 +56,7 @@ function onsecureConnection() { // const as = hooks.activitiesOfTypes('TLSWRAP'); assert.strictEqual(as.length, 2); - client = as[1]; + client = as[1]; // XXX(sam) this happens after onsecureConnect, with TLS1.3 assert.strictEqual(client.type, 'TLSWRAP'); assert.strictEqual(typeof client.uid, 'number'); assert.strictEqual(typeof client.triggerAsyncId, 'number'); @@ -78,7 +82,7 @@ function onsecureConnect() { // // Destroying client socket // - this.destroy(); + this.destroy(); // This destroys client before server handshakes, with TLS1.3 checkInvocations(svr, { init: 1, before: 2, after: 1 }, 'server: when destroying client'); checkInvocations(client, { init: 1, before: 2, after: 2 }, diff --git a/test/parallel/test-https-client-renegotiation-limit.js b/test/parallel/test-https-client-renegotiation-limit.js index 1fccc7bb5d1629..3527c48f4b9580 100644 --- a/test/parallel/test-https-client-renegotiation-limit.js +++ b/test/parallel/test-https-client-renegotiation-limit.js @@ -32,6 +32,9 @@ const tls = require('tls'); const https = require('https'); const fixtures = require('../common/fixtures'); +// Renegotiation as a protocol feature was dropped after TLS1.2. +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + // renegotiation limits to test const LIMITS = [0, 1, 2, 3, 5, 10, 16]; diff --git a/test/parallel/test-https-client-resume.js b/test/parallel/test-https-client-resume.js index cf1bbdf2626823..8904e24180b329 100644 --- a/test/parallel/test-https-client-resume.js +++ b/test/parallel/test-https-client-resume.js @@ -55,7 +55,8 @@ server.listen(0, common.mustCall(function() { '\r\n'); })); - client1.on('session', common.mustCall((session) => { + // TLS1.2 servers issue 1 ticket, TLS1.3 issues more, but only use the first. + client1.once('session', common.mustCall((session) => { console.log('session'); const opts = { diff --git a/test/parallel/test-tls-alert-handling.js b/test/parallel/test-tls-alert-handling.js index 63b845122fc1f0..69be14daf9d31a 100644 --- a/test/parallel/test-tls-alert-handling.js +++ b/test/parallel/test-tls-alert-handling.js @@ -40,11 +40,13 @@ const errorHandler = common.mustCall((err) => { server.close(); }); const server = tls.createServer(opts, common.mustCall(function(s) { + console.log('server on secureConnection'); s.pipe(s); s.on('error', errorHandler); }, 2)); server.listen(0, common.mustCall(function() { + console.log('server on listen'); sendClient(); })); @@ -57,6 +59,7 @@ function sendClient() { rejectUnauthorized: false }); client.on('data', common.mustCall(function() { + console.log('client on data, iter %d', iter+1); if (iter++ === 2) sendBADTLSRecord(); if (iter < max_iter) { client.write('a'); @@ -64,9 +67,10 @@ function sendClient() { } client.end(); }, max_iter)); - client.write('a'); + client.write('a', common.mustCall()); client.on('error', common.mustNotCall()); client.on('close', common.mustCall(function() { + console.log('client on close'); clientClosed = true; if (canCloseServer()) server.close(); diff --git a/test/parallel/test-tls-async-cb-after-socket-end.js b/test/parallel/test-tls-async-cb-after-socket-end.js index 5c812c8f0432ea..49ca0cebc9b524 100644 --- a/test/parallel/test-tls-async-cb-after-socket-end.js +++ b/test/parallel/test-tls-async-cb-after-socket-end.js @@ -14,7 +14,6 @@ const tls = require('tls'); // new and resume session events will never be emitted on the server. const options = { - maxVersion: 'TLSv1.2', secureOptions: SSL_OP_NO_TICKET, key: fixtures.readSync('test_key.pem'), cert: fixtures.readSync('test_cert.pem') @@ -38,6 +37,10 @@ server.on('resumeSession', common.mustCall((id, cb) => { server.listen(0, common.mustCall(() => { const clientOpts = { + // Don't send a TLS1.3/1.2 ClientHello, they contain a fake session_id, + // which triggers a 'resumeSession' event for client1. TLS1.2 ClientHello + // won't have a session_id until client2, which will have a valid session. + maxVersion: 'TLSv1.2', port: server.address().port, rejectUnauthorized: false, session: false diff --git a/test/parallel/test-tls-cli-min-version-1.0.js b/test/parallel/test-tls-cli-min-version-1.0.js index f2f39ce60f6624..c1a64b751b63b0 100644 --- a/test/parallel/test-tls-cli-min-version-1.0.js +++ b/test/parallel/test-tls-cli-min-version-1.0.js @@ -8,7 +8,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const tls = require('tls'); -assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.2'); +assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.3'); assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1'); // Check the min-max version protocol versions against these CLI settings. diff --git a/test/parallel/test-tls-cli-min-version-1.1.js b/test/parallel/test-tls-cli-min-version-1.1.js index 404ee98ff326d9..b6d2fc2e2b6d0a 100644 --- a/test/parallel/test-tls-cli-min-version-1.1.js +++ b/test/parallel/test-tls-cli-min-version-1.1.js @@ -8,7 +8,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const tls = require('tls'); -assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.2'); +assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.3'); assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1.1'); // Check the min-max version protocol versions against these CLI settings. diff --git a/test/parallel/test-tls-client-auth.js b/test/parallel/test-tls-client-auth.js index 1f8c7e6096ff11..a07cfad4b629df 100644 --- a/test/parallel/test-tls-client-auth.js +++ b/test/parallel/test-tls-client-auth.js @@ -1,10 +1,10 @@ 'use strict'; -require('../common'); +const common = require('../common'); const fixtures = require('../common/fixtures'); const { - assert, connect, keys + assert, connect, keys, tls } = require(fixtures.path('tls-connect')); // Use ec10 and agent10, they are the only identities with intermediate CAs. @@ -63,9 +63,10 @@ connect({ return cleanup(); }); -// Request cert from client that doesn't have one. +// Request cert from TLS1.2 client that doesn't have one. connect({ client: { + maxVersion: 'TLSv1.2', ca: server.ca, checkServerIdentity, }, @@ -76,10 +77,36 @@ connect({ requestCert: true, }, }, function(err, pair, cleanup) { - assert.strictEqual(err.code, 'ECONNRESET'); + assert.strictEqual(pair.server.err.code, 'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE'); + assert.strictEqual(pair.client.err.code, 'ECONNRESET'); return cleanup(); }); +// Request cert from TLS1.3 client that doesn't have one. +if (tls.DEFAULT_MAX_VERSION == 'TLSv1.3') connect({ + client: { + ca: server.ca, + checkServerIdentity, + }, + server: { + key: server.key, + cert: server.cert, + ca: client.ca, + requestCert: true, + }, +}, function(err, pair, cleanup) { + assert.strictEqual(pair.server.err.code, 'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE'); + + // TLS1.3 client completes handshake before server, and its only after the + // server handshakes, requests certs, gets back a zero-length list of certs, + // and sends a fatal Alert to the client that the client discovers there has + // been a fatal error. + pair.client.conn.once('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED'); + cleanup(); + })); +}); + // Typical configuration error, incomplete cert chains sent, we have to know the // peer's subordinate CAs in order to verify the peer. connect({ diff --git a/test/parallel/test-tls-client-getephemeralkeyinfo.js b/test/parallel/test-tls-client-getephemeralkeyinfo.js index 8a9cc65a1cac25..b1b41f450510d7 100644 --- a/test/parallel/test-tls-client-getephemeralkeyinfo.js +++ b/test/parallel/test-tls-client-getephemeralkeyinfo.js @@ -10,6 +10,9 @@ const tls = require('tls'); const key = fixtures.readKey('agent2-key.pem'); const cert = fixtures.readKey('agent2-cert.pem'); +// XXX test works with TLS1.3, rework test to add +// 'ECDH' with 'TLS_AES_128_GCM_SHA256', + function loadDHParam(n) { return fixtures.readKey(`dh${n}.pem`); } diff --git a/test/parallel/test-tls-client-reject-12.js b/test/parallel/test-tls-client-reject-12.js new file mode 100644 index 00000000000000..f77d463f44dc71 --- /dev/null +++ b/test/parallel/test-tls-client-reject-12.js @@ -0,0 +1,13 @@ +'use strict'; + +// test-tls-client-reject specifically for TLS1.2. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const tls = require('tls'); + +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +require('./test-tls-client-reject.js'); diff --git a/test/parallel/test-tls-client-reject.js b/test/parallel/test-tls-client-reject.js index 9eff6cb9cead01..733fcd2ec73478 100644 --- a/test/parallel/test-tls-client-reject.js +++ b/test/parallel/test-tls-client-reject.js @@ -34,7 +34,10 @@ const options = { }; const server = tls.createServer(options, function(socket) { + console.log('server on secureConnection'); socket.pipe(socket); + // Pipe already ends... but leaving this here tests .end() after .end(). + // XXX(sam) is that good, or is it bad? socket.on('end', () => socket.end()); }).listen(0, common.mustCall(function() { unauthorized(); @@ -47,13 +50,21 @@ function unauthorized() { servername: 'localhost', rejectUnauthorized: false }, common.mustCall(function() { - console.log('... unauthorized'); + let _data; + console.log('... unauthorized', socket.getProtocol()); assert(!socket.authorized); socket.on('data', common.mustCall((data) => { assert.strictEqual(data.toString(), 'ok'); + _data = data; + })); + socket.on('end', common.mustCall(() => { + assert(_data, 'data failed to echo!'); })); socket.on('end', () => rejectUnauthorized()); })); + socket.once('session', common.mustCall(() => { + console.log('client on session'); + })); socket.on('error', common.mustNotCall()); socket.end('ok'); } @@ -65,7 +76,7 @@ function rejectUnauthorized() { }, common.mustNotCall()); socket.on('data', common.mustNotCall()); socket.on('error', common.mustCall(function(err) { - console.log('... rejected:', err); + console.log('... rejected:', err.code, err.message); authorized(); })); socket.end('ng'); diff --git a/test/parallel/test-tls-client-renegotiation-13.js b/test/parallel/test-tls-client-renegotiation-13.js new file mode 100644 index 00000000000000..de1233157248ae --- /dev/null +++ b/test/parallel/test-tls-client-renegotiation-13.js @@ -0,0 +1,38 @@ +'use strict'; + +const common = require('../common'); +const fixtures = require('../common/fixtures'); + +// Confirm that for TLSv1.3, renegotiate() is disallowed. + +const { + assert, connect, keys, tls +} = require(fixtures.path('tls-connect')); + +const client = keys.ec10; +const server = keys.agent10; + +connect({ + client: { + ca: server.ca, + checkServerIdentity: common.mustCall(), + }, + server: { + key: server.key, + cert: server.cert, + }, +}, function(err, pair, cleanup) { + assert.ifError(err); + + const client = pair.client.conn; + + assert.strictEqual(client.getProtocol(), 'TLSv1.3'); + + const ok = client.renegotiate({}, common.mustCall((err) => { + assert(err.code, 'ERR_TLS_RENEGOTIATE'); + assert(err.message, 'Attempt to renegotiate TLS session failed'); + cleanup(); + })); + + assert.strictEqual(ok, false); +}); diff --git a/test/parallel/test-tls-client-renegotiation-limit.js b/test/parallel/test-tls-client-renegotiation-limit.js index daae92eeb46d67..010fd8596aa0c8 100644 --- a/test/parallel/test-tls-client-renegotiation-limit.js +++ b/test/parallel/test-tls-client-renegotiation-limit.js @@ -31,6 +31,9 @@ const assert = require('assert'); const tls = require('tls'); const fixtures = require('../common/fixtures'); +// Renegotiation as a protocol feature was dropped after TLS1.2. +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + // renegotiation limits to test const LIMITS = [0, 1, 2, 3, 5, 10, 16]; diff --git a/test/parallel/test-tls-client-resume-12.js b/test/parallel/test-tls-client-resume-12.js new file mode 100644 index 00000000000000..7767d3dd2a5cc9 --- /dev/null +++ b/test/parallel/test-tls-client-resume-12.js @@ -0,0 +1,13 @@ +'use strict'; + +// test-tls-client-resume specifically for TLS1.2. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const tls = require('tls'); + +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +require('./test-tls-client-resume.js'); diff --git a/test/parallel/test-tls-client-resume.js b/test/parallel/test-tls-client-resume.js index 9f868fdcdc0a49..39356ce8e2b569 100644 --- a/test/parallel/test-tls-client-resume.js +++ b/test/parallel/test-tls-client-resume.js @@ -44,32 +44,61 @@ const server = tls.Server(options, common.mustCall((socket) => { // start listening server.listen(0, common.mustCall(function() { - - let sessionx = null; - let session1 = null; + let sessionx = null; // From right after connect, invalid for TLS1.3 + let session1 = null; // Delivered by the session event, always valid. + let sessions = 0; + let tls13; const client1 = tls.connect({ port: this.address().port, rejectUnauthorized: false }, common.mustCall(() => { - console.log('connect1'); + console.log('connect1', client1.getProtocol()); + tls13 = client1.getProtocol() === 'TLSv1.3'; assert.strictEqual(client1.isSessionReused(), false); sessionx = client1.getSession(); + assert(sessionx); + + if (session1) + reconnect(); })); + client1.on('data', (d) => { + console.log('data1', d.toString()); + }); + client1.once('session', common.mustCall((session) => { console.log('session1'); session1 = session; + assert(session1); + if (sessionx) + reconnect(); })); - client1.on('close', common.mustCall(() => { + client1.on('session', () => { + console.log('client1 session#', ++sessions); + }); + + client1.on('close', () => { + console.log('client1 close'); + assert.strictEqual(sessions, tls13 ? 2 : 1); + }); + + function reconnect() { assert(sessionx); assert(session1); - assert.strictEqual(sessionx.compare(session1), 0); + if(tls13) + // For TLS1.3, the session immediately after handshake is a dummy, + // unresumable session. The one delivered later in session event is + // resumable. + assert.notStrictEqual(sessionx.compare(session1), 0); + else + // For TLS1.2, they are identical. + assert.strictEqual(sessionx.compare(session1), 0); const opts = { port: server.address().port, rejectUnauthorized: false, - session: session1 + session: session1, }; const client2 = tls.connect(opts, common.mustCall(() => { @@ -83,7 +112,7 @@ server.listen(0, common.mustCall(function() { })); client2.resume(); - })); + } client1.resume(); })); diff --git a/test/parallel/test-tls-destroy-stream.js b/test/parallel/test-tls-destroy-stream.js index eb7a2ca338bd40..30c790ac9daa62 100644 --- a/test/parallel/test-tls-destroy-stream.js +++ b/test/parallel/test-tls-destroy-stream.js @@ -9,6 +9,8 @@ const net = require('net'); const assert = require('assert'); const tls = require('tls'); +tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; + // This test ensures that an instance of StreamWrap should emit "end" and // "close" when the socket on the other side call `destroy()` instead of // `end()`. @@ -21,7 +23,8 @@ const tlsServer = tls.createServer( ca: [fixtures.readSync('test_ca.pem')], }, (socket) => { - socket.on('error', common.mustNotCall()); + console.log('tls server connection'); + //socket.on('error', common.mustNotCall(console.log)); socket.on('close', common.mustCall()); socket.write(CONTENT); socket.destroy(); @@ -29,9 +32,11 @@ const tlsServer = tls.createServer( ); const server = net.createServer((conn) => { + console.log('net server connection'); conn.on('error', common.mustNotCall()); // Assume that we want to use data to determine what to do with connections. conn.once('data', common.mustCall((chunk) => { + console.log('net conn data'); const { clientSide, serverSide } = makeDuplexPair(); serverSide.on('close', common.mustCall(() => { conn.destroy(); @@ -47,6 +52,7 @@ const server = net.createServer((conn) => { })); process.nextTick(() => { + console.log('net conn unshift data'); conn.unshift(chunk); }); @@ -61,9 +67,6 @@ server.listen(0, () => { assert.strictEqual(data.toString('utf8'), CONTENT); })); conn.on('error', common.mustNotCall()); - conn.on( - 'close', - common.mustCall(() => server.close()), - ); + conn.on('close', common.mustCall(() => server.close())); }); }); diff --git a/test/parallel/test-tls-disable-renegotiation.js b/test/parallel/test-tls-disable-renegotiation.js index 13e08112e596d6..7b9a476b3f4a80 100644 --- a/test/parallel/test-tls-disable-renegotiation.js +++ b/test/parallel/test-tls-disable-renegotiation.js @@ -10,6 +10,9 @@ if (!common.hasCrypto) const tls = require('tls'); +// Renegotiation as a protocol feature was dropped after TLS1.2. +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + const options = { key: fixtures.readKey('agent1-key.pem'), cert: fixtures.readKey('agent1-cert.pem'), @@ -17,6 +20,7 @@ const options = { const server = tls.Server(options, common.mustCall((socket) => { socket.on('error', common.mustCall((err) => { + console.log('server on error: %s', err.code); common.expectsError({ type: Error, code: 'ERR_TLS_RENEGOTIATION_DISABLED', @@ -29,10 +33,13 @@ const server = tls.Server(options, common.mustCall((socket) => { // Demonstrates that renegotiation works successfully up until // disableRenegotiation is called. socket.on('data', common.mustCall((chunk) => { + console.log('server on data <%s>', chunk); + console.log('server disabling renegotiation'); socket.write(chunk); socket.disableRenegotiation(); })); socket.on('secure', common.mustCall(() => { + console.log('server on secure', socket._handle.handshakes); assert(socket._handle.handshakes < 2, `Too many handshakes [${socket._handle.handshakes}]`); })); @@ -86,5 +93,11 @@ server.listen(0, common.mustCall(() => { })); })); assert.strictEqual(ok, true); + client.on('secureConnect', common.mustCall(() => { + console.log('client on secureConnect'); + })); + client.on('secure', common.mustCall(() => { + console.log('client on secure'); + })); })); })); diff --git a/test/parallel/test-tls-min-max-version.js b/test/parallel/test-tls-min-max-version.js index 9a8b73c40e496e..98c2882f48be77 100644 --- a/test/parallel/test-tls-min-max-version.js +++ b/test/parallel/test-tls-min-max-version.js @@ -7,6 +7,12 @@ const fixtures = require('../common/fixtures'); const { assert, connect, keys, tls } = require(fixtures.path('tls-connect')); +// XXX test all combinations of +// min 1/1.1/1.2/1.3 +// max 1.2/1.3 +// XXX add tests specific to TLSv1.3.. there are none ATM +tls.DEFAULT_MIN_VERSION = 'TLSv1.2'; +tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; const DEFAULT_MIN_VERSION = tls.DEFAULT_MIN_VERSION; const DEFAULT_MAX_VERSION = tls.DEFAULT_MAX_VERSION; @@ -64,8 +70,8 @@ function test(cmin, cmax, cprot, smin, smax, sprot, proto, cerr, serr) { const U = undefined; -// Default protocol is TLSv1.2. -test(U, U, U, U, U, U, 'TLSv1.2'); +// Default protocol is the max version. +test(U, U, U, U, U, U, DEFAULT_MAX_VERSION); // Insecure or invalid protocols cannot be enabled. test(U, U, U, U, U, 'SSLv2_method', @@ -147,9 +153,13 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') { test(U, U, U, U, U, 'TLSv1_1_method', U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); test(U, U, U, U, U, 'TLSv1_method', - U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); + U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); } else { - assert(false, 'unreachable'); + // TLS1.3 client hellos are are not understood by TLS1.1 or below. + test(U, U, U, U, U, 'TLSv1_1_method', + U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + test(U, U, U, U, U, 'TLSv1_method', + U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); } } @@ -164,7 +174,9 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.1') { test(U, U, U, U, U, 'TLSv1_method', U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); } else { - assert(false, 'unreachable'); + // TLS1.3 client hellos are are not understood by TLS1.1 or below. + test(U, U, U, U, U, 'TLSv1_method', + U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); } } diff --git a/test/parallel/test-tls-ocsp-callback.js b/test/parallel/test-tls-ocsp-callback.js index 2670fefd3dcf72..2848425ef9188c 100644 --- a/test/parallel/test-tls-ocsp-callback.js +++ b/test/parallel/test-tls-ocsp-callback.js @@ -60,7 +60,9 @@ function test(testOptions, cb) { } const server = tls.createServer(options, function(cleartext) { + console.log('server on secureConnection'); cleartext.on('error', function(er) { + console.log('server on err', err.code); // We're ok with getting ECONNRESET in this test, but it's // timing-dependent, and thus unreliable. Any other errors // are just failures, though. @@ -71,17 +73,20 @@ function test(testOptions, cb) { cleartext.end(); }); server.on('OCSPRequest', function(cert, issuer, callback) { + console.log('server on OCSPRequest'); ++ocspCount; assert.ok(Buffer.isBuffer(cert)); assert.ok(Buffer.isBuffer(issuer)); // Just to check that async really works there setTimeout(function() { + console.log('server OCSPRequest done: %j', testOptions.response); callback(null, testOptions.response ? Buffer.from(testOptions.response) : null); }, 100); }); server.listen(0, function() { + console.log('client connect, with OCSP?', testOptions.ocsp !== false); const client = tls.connect({ port: this.address().port, requestOCSP: testOptions.ocsp !== false, @@ -89,14 +94,17 @@ function test(testOptions, cb) { SSL_OP_NO_TICKET : 0, rejectUnauthorized: false }, function() { + console.log('client on secureConnect'); clientSecure++; }); client.on('OCSPResponse', function(resp) { + console.log('client on OCSPResponse: <%s>', resp, resp ? 'destroy!' : ''); ocspResponse = resp; if (resp) client.destroy(); }); - client.on('close', function() { + client.on('close', function(err) { + console.log('client on close, err?', err); server.close(cb); }); }); diff --git a/test/parallel/test-tls-server-verify.js b/test/parallel/test-tls-server-verify.js index 2fd815d627fdf7..347dfd985ab97d 100644 --- a/test/parallel/test-tls-server-verify.js +++ b/test/parallel/test-tls-server-verify.js @@ -272,6 +272,8 @@ function runTest(port, testIndex) { if (tcase.renegotiate) { serverOptions.secureOptions = SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION; + // Renegotiation as a protocol feature was dropped after TLS1.2. + serverOptions.maxVersion = 'TLSv1.2'; } let renegotiated = false; diff --git a/test/parallel/test-tls-set-ciphers.js b/test/parallel/test-tls-set-ciphers.js index ef2c2543517e5f..a31886e4efd90e 100644 --- a/test/parallel/test-tls-set-ciphers.js +++ b/test/parallel/test-tls-set-ciphers.js @@ -1,62 +1,93 @@ -// Copyright Joyent, Inc. and other Node contributors. -// -// Permission is hereby granted, free of charge, to any person obtaining a -// copy of this software and associated documentation files (the -// "Software"), to deal in the Software without restriction, including -// without limitation the rights to use, copy, modify, merge, publish, -// distribute, sublicense, and/or sell copies of the Software, and to permit -// persons to whom the Software is furnished to do so, subject to the -// following conditions: -// -// The above copyright notice and this permission notice shall be included -// in all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS -// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF -// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN -// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, -// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR -// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE -// USE OR OTHER DEALINGS IN THE SOFTWARE. - 'use strict'; const common = require('../common'); +const fixtures = require('../common/fixtures'); -if (!common.opensslCli) - common.skip('node compiled without OpenSSL CLI.'); +// Test cipher: option for TLS. -if (!common.hasCrypto) - common.skip('missing crypto'); +const { + assert, connect, keys +} = require(fixtures.path('tls-connect')); +const crypto = require('crypto'); -const assert = require('assert'); -const exec = require('child_process').exec; -const tls = require('tls'); -const fixtures = require('../common/fixtures'); -const options = { - key: fixtures.readKey('agent2-key.pem'), - cert: fixtures.readKey('agent2-cert.pem'), - ciphers: 'AES256-SHA' -}; +function test(cciphers, sciphers, cipher, cerr, serr) { + assert(cipher || cerr || serr, 'test missing any expectations'); + const where = (new Error()).stack.split('\n')[2].replace(/[^(]*/, ''); + connect({ + client: { + checkServerIdentity: (servername, cert) => { }, + ca: `${keys.agent1.cert}\n${keys.agent6.ca}`, + ciphers: cciphers, + }, + server: { + cert: keys.agent6.cert, + key: keys.agent6.key, + ciphers: sciphers, + }, + }, common.mustCall((err, pair, cleanup) => { + function u(_) { return _ === undefined ? 'U' : _; } + console.log('test:', u(cciphers), u(sciphers), + 'expect', u(cipher), u(cerr), u(serr)); + console.log(' ', where); + if (!cipher) { + console.log('client', pair.client.err ? pair.client.err.code : undefined); + console.log('server', pair.server.err ? pair.server.err.code : undefined); + if (cerr) { + assert(pair.client.err); + assert.strictEqual(pair.client.err.code, cerr); + } + if (serr) { + assert(pair.server.err); + assert.strictEqual(pair.server.err.code, serr); + } + return cleanup(); + } -const reply = 'I AM THE WALRUS'; // something recognizable -let response = ''; + const reply = 'So long and thanks for all the fish.'; -process.on('exit', function() { - assert.ok(response.includes(reply)); -}); + assert.ifError(err); + assert.ifError(pair.server.err); + assert.ifError(pair.client.err); + assert(pair.server.conn); + assert(pair.client.conn); + assert.strictEqual(pair.client.conn.getCipher().name, cipher); + assert.strictEqual(pair.server.conn.getCipher().name, cipher); -const server = tls.createServer(options, common.mustCall(function(conn) { - conn.end(reply); -})); + pair.server.conn.write(reply); -server.listen(0, '127.0.0.1', function() { - const cmd = `"${common.opensslCli}" s_client -cipher ${ - options.ciphers} -connect 127.0.0.1:${this.address().port}`; + pair.client.conn.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString(), reply); + return cleanup(); + })); + })); +} - exec(cmd, function(err, stdout, stderr) { - assert.ifError(err); - response = stdout; - server.close(); - }); -}); +const U = undefined; + +// Have shared ciphers. +test(U, 'AES256-SHA', 'AES256-SHA'); +test('AES256-SHA', U, 'AES256-SHA'); + +test(U, 'TLS_AES_256_GCM_SHA384', 'TLS_AES_256_GCM_SHA384'); +test('TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384'); + +// Do not have shared ciphers. +test('TLS_AES_256_GCM_SHA384', 'TLS_CHACHA20_POLY1305_SHA256', + U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + +test('AES128-SHA', 'AES256-SHA', U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); +test('AES128-SHA:TLS_AES_256_GCM_SHA384', + 'TLS_CHACHA20_POLY1305_SHA256:AES256-SHA', + U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + +// Cipher order ignored, TLS1.3 chosen before TLS1.2. +test('AES256-SHA:TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384'); +test(U, 'AES256-SHA:TLS_AES_256_GCM_SHA384', 'TLS_AES_256_GCM_SHA384'); + +// TLS_AES_128_CCM_8_SHA256 & TLS_AES_128_CCM_SHA256 are not enabled by +// default, but work. +test('TLS_AES_128_CCM_8_SHA256', U, + U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + +test('TLS_AES_128_CCM_8_SHA256', 'TLS_AES_128_CCM_8_SHA256', + 'TLS_AES_128_CCM_8_SHA256'); diff --git a/test/parallel/test-tls-ticket-12.js b/test/parallel/test-tls-ticket-12.js new file mode 100644 index 00000000000000..600c571a03bec4 --- /dev/null +++ b/test/parallel/test-tls-ticket-12.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +// Run test-tls-ticket.js with TLS1.2 + +const tls = require('tls'); + +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +require('./test-tls-ticket.js'); diff --git a/test/parallel/test-tls-ticket-cluster.js b/test/parallel/test-tls-ticket-cluster.js index 98fe533b6969d6..93aebd20ad20bb 100644 --- a/test/parallel/test-tls-ticket-cluster.js +++ b/test/parallel/test-tls-ticket-cluster.js @@ -40,13 +40,19 @@ if (cluster.isMaster) { let workerPort = null; function shoot() { - console.error('[master] connecting', workerPort); + console.error('[master] connecting', workerPort, 'have session?', !!lastSession); const c = tls.connect(workerPort, { session: lastSession, rejectUnauthorized: false }, () => { + console.log('[master] on secureConnect'); c.end(); - + }).on('close', () => { + console.log('[master] on close'); + // Wait for close to shoot off another connection. We don't want to shoot + // until a new session is allocated, if one will be. The new session is + // not guaranteed on secureConnect (it depends on TLS1.2 vs TLS1.3), but + // it is guaranteed to happen before the connection is closed. if (++reqCount === expectedReqCount) { Object.keys(cluster.workers).forEach(function(id) { cluster.workers[id].send('die'); @@ -55,8 +61,12 @@ if (cluster.isMaster) { shoot(); } }).once('session', (session) => { + console.log('client on session'); + assert(!lastSession); lastSession = session; }); + + c.resume(); // XXX see close_notify comment in server } function fork() { @@ -93,12 +103,16 @@ const cert = fixtures.readSync('agent.crt'); const options = { key, cert }; const server = tls.createServer(options, (c) => { + console.error('[worker] connection reused?', c.isSessionReused()); if (c.isSessionReused()) { process.send({ msg: 'reused' }); } else { process.send({ msg: 'not-reused' }); } - c.end(); + // XXX used to just .end(), but that means client gets close_notify + // before NewSessionTicket. Send data until that problem is solved. + // c.end(); + c.end('x'); }); server.listen(0, () => { diff --git a/test/parallel/test-tls-ticket.js b/test/parallel/test-tls-ticket.js index d11535dd3a5255..b5eb5ad32f6150 100644 --- a/test/parallel/test-tls-ticket.js +++ b/test/parallel/test-tls-ticket.js @@ -34,6 +34,8 @@ const keys = crypto.randomBytes(48); const serverLog = []; const ticketLog = []; +let s; + let serverCount = 0; function createServer() { const id = serverCount++; @@ -47,16 +49,44 @@ function createServer() { ticketKeys: keys }, function(c) { serverLog.push(id); - c.end(); + // XXX triggers close_notify before NewSessionTicket bug + // c.end(); + c.end('x'); counter++; + console.log('server %d: reused %j keys %s id %d:%d proto %s', + c.remotePort, c.isSessionReused(), + server.getTicketKeys().compare(keys) === 0 ? 'shared' : 'random', + id, counter, + c.getProtocol(), + ); + // Rotate ticket keys + // + // Take especial care to account for TLS1.2 and TLS1.3 differences around + // when ticket keys are encrypted. In TLS1.2, they are encrypted before the + // handshake complete callback, but in TLS1.3, they are encrypted after. + // There is no callback or way for us to know when they were sent, so hook + // the client's reception of the keys, and use it as proof that the current + // keys were used, and its safe to rotate them. + // + // Rotation can occur right away if the session was reused, the keys were + // already decrypted or we wouldn't have a reused session. + function setTicketKeys(keys) { + if (c.isSessionReused()) + server.setTicketKeys(keys); + else + s.once('session', () => { + server.setTicketKeys(keys); + }); + } if (counter === 1) { previousKey = server.getTicketKeys(); - server.setTicketKeys(crypto.randomBytes(48)); + assert.strictEqual(previousKey.compare(keys), 0); + setTicketKeys(crypto.randomBytes(48)); } else if (counter === 2) { - server.setTicketKeys(previousKey); + setTicketKeys(previousKey); } else if (counter === 3) { // Use keys from counter=2 } else { @@ -95,20 +125,33 @@ function start(callback) { let left = servers.length; function connect() { - const s = tls.connect(shared.address().port, { + let localPort + s = tls.connect(shared.address().port, { session: sess, rejectUnauthorized: false }, function() { - sess = sess || s.getSession(); - ticketLog.push(s.getTLSTicket().toString('hex')); + console.log('client %d: reused %j', s.localPort, s.isSessionReused()); + localPort = s.localPort; + if (s.isSessionReused()) + ticketLog.push(s.getTLSTicket().toString('hex')); + }); + s.on('data', () => { + console.log('client %d: on data, do .end()', s.localPort); + s.end(); }); s.on('close', function() { + console.log('client %d: on close', localPort); if (--left === 0) callback(); else connect(); }); + s.on('session', (session) => { + console.log('ticket %d: saved? %j', s.localPort, !sess); + sess = sess || session; + }); s.once('session', (session) => onNewSession(s, session)); + s.once('session', () => ticketLog.push(s.getTLSTicket().toString('hex'))); } connect(); From 6ab4f7b257a3a2df5636c7f34ef95ad624e08678 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 14 Feb 2019 11:25:39 -0800 Subject: [PATCH 5/9] tls: support setting min/max for 1.3 Allow enabling/disabling 1.3, as well as disabling 1.2. --- lib/tls.js | 10 ++++ src/node_options.cc | 23 +++++++++- src/node_options.h | 3 ++ test/parallel/test-tls-cli-max-version-1.2.js | 15 ++++++ test/parallel/test-tls-cli-max-version-1.3.js | 15 ++++++ test/parallel/test-tls-cli-min-version-1.3.js | 15 ++++++ test/parallel/test-tls-min-max-version.js | 46 +++++++++++++++---- 7 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-tls-cli-max-version-1.2.js create mode 100644 test/parallel/test-tls-cli-max-version-1.3.js create mode 100644 test/parallel/test-tls-cli-min-version-1.3.js diff --git a/lib/tls.js b/lib/tls.js index 46248eb8086e91..5cdf57b1465d0a 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -60,9 +60,19 @@ if (getOptionValue('--tls-v1.0')) exports.DEFAULT_MIN_VERSION = 'TLSv1'; else if (getOptionValue('--tls-v1.1')) exports.DEFAULT_MIN_VERSION = 'TLSv1.1'; +else if (getOptionValue('--tls-min-v1.3')) + exports.DEFAULT_MIN_VERSION = 'TLSv1.3'; else exports.DEFAULT_MIN_VERSION = 'TLSv1.2'; +if (getOptionValue('--tls-max-v1.3')) + exports.DEFAULT_MAX_VERSION = 'TLSv1.3'; +else if(getOptionValue('--tls-max-v1.2')) + exports.DEFAULT_MAX_VERSION = 'TLSv1.2'; +else + exports.DEFAULT_MAX_VERSION = 'TLSv1.3'; // Will depend on node version. + + exports.getCiphers = internalUtil.cachedResult( () => internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true) ); diff --git a/src/node_options.cc b/src/node_options.cc index e68487a2bf9a99..62efee1960f520 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -288,13 +288,32 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { #if HAVE_OPENSSL AddOption("--tls-v1.0", - "enable TLSv1.0 and greater by default", + "set default TLS minimum to TLSv1.0 (default: TLSv1.2)", &EnvironmentOptions::tls_v1_0, kAllowedInEnvironment); AddOption("--tls-v1.1", - "enable TLSv1.1 and greater by default", + "set default TLS minimum to TLSv1.1 (default: TLSv1.2)", &EnvironmentOptions::tls_v1_1, kAllowedInEnvironment); + AddOption("--tls-min-v1.3", + "set default TLS minimum to TLSv1.3 (default: TLSv1.2)", + &EnvironmentOptions::tls_min_v1_3, + kAllowedInEnvironment); + AddOption("--tls-max-v1.2", + "set default TLS maximum to TLSv1.2 (default: TLSv1.3)", + &EnvironmentOptions::tls_max_v1_2, + kAllowedInEnvironment); + // Current plan is: + // - 10.x and below: TLS1.3 is opt-in with --tls-max-v1.3 + // - 11.x: XXX would like it to be opt-out, but might need to be opt-out if + // if its too disruptive + // - 12.x: TLS1.3 is opt-out with --tls-max-v1.2 + // Support both options wherever 1.3 is possible, so they are uniformly + // available. + AddOption("--tls-max-v1.3", + "set default TLS maximum to TLSv1.3 (default: TLSv1.3)", + &EnvironmentOptions::tls_max_v1_3, + kAllowedInEnvironment); #endif #if HAVE_INSPECTOR diff --git a/src/node_options.h b/src/node_options.h index e68e1cdfd734ca..ce335e10ad037c 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -117,6 +117,9 @@ class EnvironmentOptions : public Options { #if HAVE_OPENSSL bool tls_v1_0 = false; bool tls_v1_1 = false; + bool tls_min_v1_3 = false; + bool tls_max_v1_2 = false; + bool tls_max_v1_3 = false; #endif std::vector preload_modules; diff --git a/test/parallel/test-tls-cli-max-version-1.2.js b/test/parallel/test-tls-cli-max-version-1.2.js new file mode 100644 index 00000000000000..9bbc9ff0ecadcc --- /dev/null +++ b/test/parallel/test-tls-cli-max-version-1.2.js @@ -0,0 +1,15 @@ +// Flags: --tls-max-v1.2 +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +// Check that node `--tls-max-v1.2` is supported. + +const assert = require('assert'); +const tls = require('tls'); + +assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.2'); +assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1.2'); + +// Check the min-max version protocol versions against these CLI settings. +require('./test-tls-min-max-version.js'); diff --git a/test/parallel/test-tls-cli-max-version-1.3.js b/test/parallel/test-tls-cli-max-version-1.3.js new file mode 100644 index 00000000000000..c04354fe4ac9a1 --- /dev/null +++ b/test/parallel/test-tls-cli-max-version-1.3.js @@ -0,0 +1,15 @@ +// Flags: --tls-max-v1.3 +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +// Check that node `--tls-max-v1.3` is supported. + +const assert = require('assert'); +const tls = require('tls'); + +assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.3'); +assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1.2'); + +// Check the min-max version protocol versions against these CLI settings. +require('./test-tls-min-max-version.js'); diff --git a/test/parallel/test-tls-cli-min-version-1.3.js b/test/parallel/test-tls-cli-min-version-1.3.js new file mode 100644 index 00000000000000..1bccc2f6cd331a --- /dev/null +++ b/test/parallel/test-tls-cli-min-version-1.3.js @@ -0,0 +1,15 @@ +// Flags: --tls-min-v1.3 +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) common.skip('missing crypto'); + +// Check that node `--tls-min-v1.3` is supported. + +const assert = require('assert'); +const tls = require('tls'); + +assert.strictEqual(tls.DEFAULT_MAX_VERSION, 'TLSv1.3'); +assert.strictEqual(tls.DEFAULT_MIN_VERSION, 'TLSv1.3'); + +// Check the min-max version protocol versions against these CLI settings. +require('./test-tls-min-max-version.js'); diff --git a/test/parallel/test-tls-min-max-version.js b/test/parallel/test-tls-min-max-version.js index 98c2882f48be77..6aec935d630e0a 100644 --- a/test/parallel/test-tls-min-max-version.js +++ b/test/parallel/test-tls-min-max-version.js @@ -7,18 +7,13 @@ const fixtures = require('../common/fixtures'); const { assert, connect, keys, tls } = require(fixtures.path('tls-connect')); -// XXX test all combinations of -// min 1/1.1/1.2/1.3 -// max 1.2/1.3 -// XXX add tests specific to TLSv1.3.. there are none ATM -tls.DEFAULT_MIN_VERSION = 'TLSv1.2'; -tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; const DEFAULT_MIN_VERSION = tls.DEFAULT_MIN_VERSION; const DEFAULT_MAX_VERSION = tls.DEFAULT_MAX_VERSION; function test(cmin, cmax, cprot, smin, smax, sprot, proto, cerr, serr) { assert(proto || cerr || serr, 'test missing any expectations'); + const where = (new Error()).stack.split('\n')[2].replace(/[^(]*/, ''); connect({ client: { checkServerIdentity: (servername, cert) => { }, @@ -38,6 +33,7 @@ function test(cmin, cmax, cprot, smin, smax, sprot, proto, cerr, serr) { function u(_) { return _ === undefined ? 'U' : _; } console.log('test:', u(cmin), u(cmax), u(cprot), u(smin), u(smax), u(sprot), 'expect', u(proto), u(cerr), u(serr)); + console.log(' ', where); if (!proto) { console.log('client', pair.client.err ? pair.client.err.code : undefined); console.log('server', pair.server.err ? pair.server.err.code : undefined); @@ -107,7 +103,23 @@ test(U, U, 'TLS_method', U, U, 'TLSv1_method', 'TLSv1'); // SSLv23 also means "any supported protocol" greater than the default // minimum (which is configurable via command line). -test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', 'TLSv1.2'); +if (DEFAULT_MIN_VERSION === 'TLSv1.3') { + test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', + U, 'ECONNRESET', 'ERR_SSL_INTERNAL_ERROR'); +} else { + test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', 'TLSv1.2'); +} + +if (DEFAULT_MIN_VERSION === 'TLSv1.3') { + test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', + U, 'ECONNRESET', 'ERR_SSL_INTERNAL_ERROR'); + test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', + U, 'ECONNRESET', 'ERR_SSL_INTERNAL_ERROR'); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', + U, 'ERR_SSL_NO_PROTOCOLS_AVAILABLE', 'ERR_SSL_UNEXPECTED_MESSAGE'); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', + U, 'ERR_SSL_NO_PROTOCOLS_AVAILABLE', 'ERR_SSL_UNEXPECTED_MESSAGE'); +} if (DEFAULT_MIN_VERSION === 'TLSv1.2') { test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', @@ -153,7 +165,7 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') { test(U, U, U, U, U, 'TLSv1_1_method', U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); test(U, U, U, U, U, 'TLSv1_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); } else { // TLS1.3 client hellos are are not understood by TLS1.1 or below. test(U, U, U, U, U, 'TLSv1_1_method', @@ -192,14 +204,32 @@ if (DEFAULT_MIN_VERSION === 'TLSv1') { test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_method', 'TLSv1'); test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_1_method', 'TLSv1.1'); test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_2_method', 'TLSv1.2'); +test('TLSv1', 'TLSv1.2', U, U, U, 'TLS_method', 'TLSv1.2'); test(U, U, 'TLSv1_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1'); test(U, U, 'TLSv1_1_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); test(U, U, 'TLSv1_2_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1.2'); +test('TLSv1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1.1'); test('TLSv1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); test('TLSv1', 'TLSv1.2', U, 'TLSv1', 'TLSv1.1', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1', 'TLSv1.1', U, 'TLSv1.1'); test('TLSv1', 'TLSv1', U, 'TLSv1', 'TLSv1.1', U, 'TLSv1'); test('TLSv1', 'TLSv1.2', U, 'TLSv1', 'TLSv1', U, 'TLSv1'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1', 'TLSv1', U, 'TLSv1'); test('TLSv1.1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); test('TLSv1', 'TLSv1.2', U, 'TLSv1.1', 'TLSv1.1', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1.2', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1.2'); + +// v-any client can connect to v-specific server +test('TLSv1', 'TLSv1.3', U, 'TLSv1.3', 'TLSv1.3', U, 'TLSv1.3'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1.2', 'TLSv1.3', U, 'TLSv1.3'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1.2', 'TLSv1.2', U, 'TLSv1.2'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1.1', 'TLSv1.1', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1.3', U, 'TLSv1', 'TLSv1', U, 'TLSv1'); + +// v-specific client can connect to v-any server +test('TLSv1.3', 'TLSv1.3', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1.3'); +test('TLSv1.2', 'TLSv1.2', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1.2'); +test('TLSv1.1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1', U, 'TLSv1', 'TLSv1.3', U, 'TLSv1'); From 1407d42aeb96c581e91cef269c389ca6706392cf Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 11 Feb 2019 21:41:13 -0800 Subject: [PATCH 6/9] XXX when should WriteWrap->Done() be called? What appears to be happening is that `SSL_write()` is writing the 5 bytes of `hello`, but it isn't being flushed out as encrypted text into the BIO. This might be because its in the middle of SSL code where the info callback is called, but its about to send the NewTickets, so the data is just buffered briefly before encrypting it out in an appdata record. Just a guess. Since there is nothing to flush, the code wanders down to EncOut(), where it sees there is nothing to write, so it calls InvokeQueued, which does a sync w->Done(), in violation of the DoWrite() contract. XXX There is an open question of exactly when to call it. Sync is illegal, SetImmediate() works, but it seems to me it shouldn't be called until the data that went into SSL_write() comes out of EncOut() and into the next stream, and the next stream callbacks, that Done() should happen. Tried to make the latter change, but it makes no difference to the unit tests, so I've no idea if its right or not. --- src/stream_base.h | 2 ++ src/tls_wrap.cc | 24 +++++++++++++++++-- src/tls_wrap.h | 1 + .../test-tls-net-socket-keepalive-12.js | 13 ++++++++++ .../parallel/test-tls-net-socket-keepalive.js | 6 ++++- 5 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-tls-net-socket-keepalive-12.js diff --git a/src/stream_base.h b/src/stream_base.h index cde3343d624fa4..1b57e14cd56638 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -214,6 +214,8 @@ class StreamResource { // Return 0 for success and a libuv error code for failures. virtual int DoTryWrite(uv_buf_t** bufs, size_t* count); // Perform a write of data, and either call req_wrap->Done() when finished + // call when? can we call sync? "call Done() and return" suggests + // first Done, then return, but probably not right // and return 0, or return a libuv error code for synchronous failures. virtual int DoWrite(WriteWrap* w, uv_buf_t* bufs, diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index dbdf1ccff89779..0f329bcb49d5d5 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -92,6 +92,7 @@ bool TLSWrap::InvokeQueued(int status, const char* error_str) { return false; if (current_write_ != nullptr) { + CHECK(!in_dowrite_); WriteWrap* w = current_write_; current_write_ = nullptr; w->Done(status, error_str); @@ -340,8 +341,23 @@ void TLSWrap::EncOut() { // No encrypted output ready to write to the underlying stream. if (BIO_pending(enc_out_) == 0) { - if (pending_cleartext_input_.empty()) - InvokeQueued(0); + if (pending_cleartext_input_.empty()) { + if (!in_dowrite_) { + InvokeQueued(0); + } else { + // XXX if we are in_dowrite_, then SSL_write wrote some appdata. + // If we are here, nothing was flushed to enc_out_. + // calling Done() in the next tick "works", but since the write is + // not flushed, it seems its too soon. Just returning and letting + // the next EncOut() call Done() seems the right thing, but in the + // absence of any docs or comments on how the streams are supposed + // to work, I'm not sure what to do. The tests don't care either way. + // return; // comment in, or out, no difference to the unit tests. + env()->SetImmediate([](Environment* env, void* data) { + static_cast(data)->InvokeQueued(0); + }, this, object()); + } + } return; } @@ -719,6 +735,7 @@ void TLSWrap::ClearError() { // Called by StreamBase::Write() to request async write of clear text into SSL. +// XXX Should there be a TLSWrap::DoTryWrite()? int TLSWrap::DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count, @@ -814,7 +831,10 @@ int TLSWrap::DoWrite(WriteWrap* w, } // Write any encrypted/handshake output that may be ready. + // XXX WIP ... guard against sync `w (aka current_write_)->Done()` + in_dowrite_ = true; EncOut(); + in_dowrite_ = false; fprintf(stderr, "...TLSWrap::DoWrite()\n"); return 0; diff --git a/src/tls_wrap.h b/src/tls_wrap.h index b75c06a0a3af74..6fd2300f82b141 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -174,6 +174,7 @@ class TLSWrap : public AsyncWrap, std::vector pending_cleartext_input_; size_t write_size_ = 0; WriteWrap* current_write_ = nullptr; + bool in_dowrite_ = false; WriteWrap* current_empty_write_ = nullptr; bool write_callback_scheduled_ = false; bool started_ = false; diff --git a/test/parallel/test-tls-net-socket-keepalive-12.js b/test/parallel/test-tls-net-socket-keepalive-12.js new file mode 100644 index 00000000000000..d2fb230796e553 --- /dev/null +++ b/test/parallel/test-tls-net-socket-keepalive-12.js @@ -0,0 +1,13 @@ +'use strict'; + +// test-tls-net-socket-keepalive specifically for TLS1.2. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const tls = require('tls'); + +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +require('./test-tls-net-socket-keepalive.js'); diff --git a/test/parallel/test-tls-net-socket-keepalive.js b/test/parallel/test-tls-net-socket-keepalive.js index c184e902b42685..4acb4e80224ee9 100644 --- a/test/parallel/test-tls-net-socket-keepalive.js +++ b/test/parallel/test-tls-net-socket-keepalive.js @@ -20,8 +20,11 @@ const options = { }; const server = tls.createServer(options, common.mustCall((conn) => { - conn.write('hello'); + conn.write('hello', common.mustCall()); conn.on('data', common.mustCall()); + conn.on('end', common.mustCall()); + conn.on('data', common.mustCall()); + conn.on('close', common.mustCall()); conn.end(); })).listen(0, common.mustCall(() => { const netSocket = new net.Socket({ @@ -42,6 +45,7 @@ const server = tls.createServer(options, common.mustCall((conn) => { address, }); + socket.on('secureConnect', common.mustCall()); socket.on('end', common.mustCall()); socket.on('data', common.mustCall()); socket.on('close', common.mustCall(() => { From 928e0eb4e89db94dc9d7cd260ebe41086ce8fd8a Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 14 Feb 2019 14:54:03 -0800 Subject: [PATCH 7/9] XXX remove workaround for close_notify before NewSessionTicket Code passes with the work-around, but it's a bug. Not sure how to fix. Calling SSL_read after after a close_notify is a bit dicy, but might have to do that, but make sure that only 'session' events occur (ie., discard data and anything else). Seems subtly wrong. Have asked openssl list how this is supposed to work. This only happens with servers that .end() sync with no data in the secureConnection event, and maybe those servers don't deserve ticket support? Not sure. --- test/parallel/test-tls-client-resume.js | 7 ++++++- test/parallel/test-tls-ticket.js | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-tls-client-resume.js b/test/parallel/test-tls-client-resume.js index 39356ce8e2b569..fe3025c88c7c5c 100644 --- a/test/parallel/test-tls-client-resume.js +++ b/test/parallel/test-tls-client-resume.js @@ -33,13 +33,17 @@ const tls = require('tls'); const fixtures = require('../common/fixtures'); const options = { + enableTrace: true, key: fixtures.readKey('agent2-key.pem'), cert: fixtures.readKey('agent2-cert.pem') }; // create server const server = tls.Server(options, common.mustCall((socket) => { - socket.end('Goodbye'); + // XXX a data-less end is needed to trigger the close_notify, followed + // by the NewSessionTicket records (which don't get read). + // socket.end('Goodbye'); + socket.end(); }, 2)); // start listening @@ -61,6 +65,7 @@ server.listen(0, common.mustCall(function() { if (session1) reconnect(); })); + client1.enableTrace(); client1.on('data', (d) => { console.log('data1', d.toString()); diff --git a/test/parallel/test-tls-ticket.js b/test/parallel/test-tls-ticket.js index b5eb5ad32f6150..6b58ee71639d2b 100644 --- a/test/parallel/test-tls-ticket.js +++ b/test/parallel/test-tls-ticket.js @@ -50,8 +50,8 @@ function createServer() { }, function(c) { serverLog.push(id); // XXX triggers close_notify before NewSessionTicket bug - // c.end(); - c.end('x'); + c.end(); + // c.end('x'); counter++; @@ -137,7 +137,7 @@ function start(callback) { }); s.on('data', () => { console.log('client %d: on data, do .end()', s.localPort); - s.end(); + //s.end(); }); s.on('close', function() { console.log('client %d: on close', localPort); From f978966799c9e817ded7f62bd57e944a90fa0c8c Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 13 Feb 2019 13:05:27 -0800 Subject: [PATCH 8/9] XXX should `.write(); .destroy()` flush data? Sometimes it does, sometimes it doesn't, I'm not sure if this expected streams behaviour, or not. Compare this test with test/parallel/test-tls-invoke-queued.js. --- test/parallel/test-tls-invoke-queued-multi.js | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 test/parallel/test-tls-invoke-queued-multi.js diff --git a/test/parallel/test-tls-invoke-queued-multi.js b/test/parallel/test-tls-invoke-queued-multi.js new file mode 100644 index 00000000000000..beaef7dddc3a25 --- /dev/null +++ b/test/parallel/test-tls-invoke-queued-multi.js @@ -0,0 +1,67 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +const common = require('../common'); + +// XXX should `.write(); .destroy()` flush data? +// +// Sometimes it does, sometimes it doesn't, I'm not sure if this expected +// streams behaviour, or not. +// +// Compare this test with test/parallel/test-tls-invoke-queued.js. + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); + +const fixtures = require('../common/fixtures'); + +let received = ''; + +const server = tls.createServer({ + // XXX with 1.2, 'hello ' is received by client, with 1.3, none of the + // writes are received. + maxVersion: 'TLSv1.3', + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}, common.mustCall(function(c) { + c.write('hello ', null, common.mustCall()); + c.write('world!', null, common.mustCall()); + c.write(' gosh', null, common.mustCall()); + c.destroy(); + + server.close(); +})).listen(0, common.mustCall(function() { + const c = tls.connect(this.address().port, { + rejectUnauthorized: false + }, common.mustCall(function() { + c.on('data', function(chunk) { + process._rawDebug('client got chunk <%s>', chunk); + received += chunk; + }); + c.on('end', common.mustCall(function() { + assert.strictEqual(received, 'hello world! gosh'); + })); + })); +})); From 0db2eed56375644e7c23d958222a13f4caaa9d9d Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 13 Feb 2019 15:06:35 -0800 Subject: [PATCH 9/9] XXX write before destroy can fail after-the-fact This is a bug, it isn't supposed to happen, but how to prevent it? When TLS finishes the DoWrite the underlying stream is closed, so the stream_resource()->Write() in EncOut() fails with write-after-destroy. See test/parallel/test-tls-destroy-stream.js, and its inline comments. --- lib/net.js | 1 + src/js_stream.cc | 1 + src/node_crypto.cc | 3 +++ test/parallel/test-tls-destroy-stream.js | 33 ++++++++++++++++++------ 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/lib/net.js b/lib/net.js index 2726e73272c73d..7c1865223c3876 100644 --- a/lib/net.js +++ b/lib/net.js @@ -581,6 +581,7 @@ Socket.prototype._destroy = function(exception, cb) { this[kBytesRead] = this._handle.bytesRead; this[kBytesWritten] = this._handle.bytesWritten; + // process._rawDebug('handle.close', this._handle.close+''); this._handle.close(() => { debug('emit close'); this.emit('close', isException); diff --git a/src/js_stream.cc b/src/js_stream.cc index 32dea04db5aeec..49f32fa1192b78 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -153,6 +153,7 @@ void JSStream::New(const FunctionCallbackInfo& args) { template void JSStream::Finish(const FunctionCallbackInfo& args) { + // finishShutdown? CHECK(args[0]->IsObject()); Wrap* w = static_cast(StreamReq::FromObject(args[0].As())); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8d8845f57bafd3..cebd1551a24734 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2732,6 +2732,9 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { template void SSLWrap::DestroySSL() { + fprintf(stderr, "%s SSLWrap::DestroySSL() ssl? %d\n", + is_server() ? "server" : "client", !!ssl_); + if (!ssl_) return; diff --git a/test/parallel/test-tls-destroy-stream.js b/test/parallel/test-tls-destroy-stream.js index 30c790ac9daa62..b7c4f7d04b53c6 100644 --- a/test/parallel/test-tls-destroy-stream.js +++ b/test/parallel/test-tls-destroy-stream.js @@ -9,7 +9,7 @@ const net = require('net'); const assert = require('assert'); const tls = require('tls'); -tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; +tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; // XXX Switch to 1.2 to see old behaviour // This test ensures that an instance of StreamWrap should emit "end" and // "close" when the socket on the other side call `destroy()` instead of @@ -18,25 +18,42 @@ tls.DEFAULT_MAX_VERSION = 'TLSv1.3'; const CONTENT = 'Hello World'; const tlsServer = tls.createServer( { + maxVersion: 'TLSv1.3', // passes with 1.2 key: fixtures.readSync('test_key.pem'), cert: fixtures.readSync('test_cert.pem'), ca: [fixtures.readSync('test_ca.pem')], }, (socket) => { - console.log('tls server connection'); - //socket.on('error', common.mustNotCall(console.log)); - socket.on('close', common.mustCall()); - socket.write(CONTENT); + process._rawDebug('server on secureConnection', socket.getProtocol()); + socket.on('error', (err) => { + process._rawDebug('server on error:', err); + // process.reallyExit(7); + //XXX assert.ifError(err); + }); + socket.on('close', common.mustCall((hadError) => { + process._rawDebug('server on close: hadError?', hadError); + // XXX? assert.ok(hadError); + })); + // XXX destroy() tears down socket before CONTENT gets flushed. probably + // because of key update messages... confirm this with tracing. Is it + // a bug in the test? + process._rawDebug('server socket.write()'); + socket.write(CONTENT, (err) => { + process._rawDebug('server write cb, err:', err); + // process.reallyExit(9); + }); + process._rawDebug('server socket.destroy()'); socket.destroy(); + //process.reallyExit(3); }, ); const server = net.createServer((conn) => { - console.log('net server connection'); + process._rawDebug('net server connection'); conn.on('error', common.mustNotCall()); // Assume that we want to use data to determine what to do with connections. conn.once('data', common.mustCall((chunk) => { - console.log('net conn data'); + process._rawDebug('net conn data'); const { clientSide, serverSide } = makeDuplexPair(); serverSide.on('close', common.mustCall(() => { conn.destroy(); @@ -52,7 +69,7 @@ const server = net.createServer((conn) => { })); process.nextTick(() => { - console.log('net conn unshift data'); + process._rawDebug('net conn unshift data'); conn.unshift(chunk); });