From 3e69cc35b84a630972d6a2d8d9bdb17af942e28a Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 23 Jun 2022 20:18:58 +0300 Subject: [PATCH 1/7] TLS: Implement isc_nmhandle_setwritetimeout() This commit adds a proper implementation of isc_nmhandle_setwritetimeout() for TLS connections. Now it passes the value to the underlying TCP handle. (cherry picked from commit 237ce05b89fe3dd3a928b1879573195806d24a35) --- lib/isc/netmgr/netmgr-int.h | 4 ++++ lib/isc/netmgr/netmgr.c | 18 +++++++++++++++++- lib/isc/netmgr/tlsstream.c | 17 +++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 624530a35c..a7338d475e 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -1787,6 +1787,10 @@ void isc__nm_async_tls_set_tlsctx(isc_nmsocket_t *listener, isc_tlsctx_t *tlsctx, const int tid); +void +isc__nmhandle_tls_setwritetimeout(isc_nmhandle_t *handle, + uint64_t write_timeout); + void isc__nm_http_stoplistening(isc_nmsocket_t *sock); diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index edb2a9dc66..e1ab339d71 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -572,8 +572,24 @@ void isc_nmhandle_setwritetimeout(isc_nmhandle_t *handle, uint64_t write_timeout) { REQUIRE(VALID_NMHANDLE(handle)); REQUIRE(VALID_NMSOCK(handle->sock)); + REQUIRE(handle->sock->tid == isc_nm_tid()); - handle->sock->write_timeout = write_timeout; + switch (handle->sock->type) { + case isc_nm_tcpsocket: + case isc_nm_udpsocket: + case isc_nm_tcpdnssocket: + case isc_nm_tlsdnssocket: + handle->sock->write_timeout = write_timeout; + break; +#ifdef HAVE_LIBNGHTTP2 + case isc_nm_tlssocket: + isc__nmhandle_tls_setwritetimeout(handle, write_timeout); + break; +#endif /* HAVE_LIBNGHTTP2 */ + default: + UNREACHABLE(); + break; + } } void diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index ff6aff8829..89adfa367c 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -1125,6 +1125,23 @@ isc__nmhandle_tls_keepalive(isc_nmhandle_t *handle, bool value) { } } +void +isc__nmhandle_tls_setwritetimeout(isc_nmhandle_t *handle, + uint64_t write_timeout) { + isc_nmsocket_t *sock = NULL; + + REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(VALID_NMSOCK(handle->sock)); + REQUIRE(handle->sock->type == isc_nm_tlssocket); + + sock = handle->sock; + if (sock->outerhandle != NULL) { + INSIST(VALID_NMHANDLE(sock->outerhandle)); + + isc_nmhandle_setwritetimeout(sock->outerhandle, write_timeout); + } +} + const char * isc__nm_tls_verify_tls_peer_result_string(const isc_nmhandle_t *handle) { isc_nmsocket_t *sock = NULL; From c44633feb2b4b106dfebfe4f750c1c0040bab9e9 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 24 Jun 2022 15:20:13 +0300 Subject: [PATCH 2/7] TLS: try to close sockets whenever there are no pending operations This commit ensures that the underlying TCP socket of a TLS connection gets closed earlier whenever there are no pending operations on it. In the loop-manager branch, in some circumstances the connection could have remained opened for far too long for no reason. This commit ensures that will not happen. (cherry picked from commit 88524e26ecae286bf62d4a4b5535c70ccff1573d) --- lib/isc/netmgr/tlsstream.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 89adfa367c..d404ee2965 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -354,6 +354,23 @@ tls_try_handshake(isc_nmsocket_t *sock) { return (rv); } +static bool +tls_try_to_close_unused_socket(isc_nmsocket_t *sock) { + if (sock->tlsstream.state > TLS_HANDSHAKE && + sock->statichandle == NULL && sock->tlsstream.nsending == 0) + { + /* + * It seems that no action on the socket has been + * scheduled on some point after the handshake, let's + * close the connection. + */ + isc__nmsocket_prep_destroy(sock); + return (true); + } + + return (false); +} + static void tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, isc__nm_uvreq_t *send_data, bool finish) { @@ -494,6 +511,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, switch (tls_status) { case SSL_ERROR_NONE: case SSL_ERROR_ZERO_RETURN: + (void)tls_try_to_close_unused_socket(sock); return; case SSL_ERROR_WANT_WRITE: if (sock->tlsstream.nsending == 0) { @@ -505,6 +523,10 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, } return; case SSL_ERROR_WANT_READ: + if (tls_try_to_close_unused_socket(sock)) { + return; + } + if (sock->tlsstream.reading) { INSIST(VALID_NMHANDLE(sock->outerhandle)); isc_nm_resumeread(sock->outerhandle); From 0754def85dd22eec95202065cf67a76f4feb4dd4 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Fri, 24 Jun 2022 15:49:15 +0300 Subject: [PATCH 3/7] TLS: sometimes TCP conn. handle might be NULL on when connecting In some cases - in particular, in case of errors, NULL might be passed to a connection callback instead of a handle that could have led to an abort. This commit ensures that such a situation will not occur. The issue was found when working on the loopmgr branch. (cherry picked from commit ac4fb34f18978d95e100e4218880dce834950a42) --- lib/isc/netmgr/tlsstream.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index d404ee2965..8753dcae9d 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -957,13 +957,14 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmhandle_t *tlshandle = NULL; REQUIRE(VALID_NMSOCK(tlssock)); - REQUIRE(VALID_NMHANDLE(handle)); tlssock->tid = isc_nm_tid(); if (result != ISC_R_SUCCESS) { goto error; } + INSIST(VALID_NMHANDLE(handle)); + tlssock->iface = handle->sock->iface; tlssock->peer = handle->sock->peer; if (isc__nm_closing(tlssock)) { From ba7fabde019ed8a8a2b9708e2817a8a143726b2a Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 27 Jun 2022 14:27:49 +0300 Subject: [PATCH 4/7] TLS: bail out earlier when NM is stopping In some operations - most prominently when establishing connection - it might be beneficial to bail out earlier when the network manager is stopping. The issue is backported from loopmgr branch, where such a change is not only beneficial, but required. (cherry picked from commit fc74b15e677d8810aea3b5abcc1b1f7eb9e7541a) --- lib/isc/netmgr/tlsstream.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 8753dcae9d..2f3314d4f7 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -674,6 +674,9 @@ isc_nm_listentls(isc_nm_t *mgr, isc_sockaddr_t *iface, isc_nmsocket_t *tsock = NULL; REQUIRE(VALID_NM(mgr)); + if (atomic_load(&mgr->closing)) { + return (ISC_R_SHUTTINGDOWN); + } tlssock = isc_mem_get(mgr->mctx, sizeof(*tlssock)); @@ -834,6 +837,10 @@ isc__nm_tls_resumeread(isc_nmhandle_t *handle) { if (!atomic_compare_exchange_strong(&handle->sock->readpaused, &(bool){ false }, false)) { + if (inactive(handle->sock)) { + return; + } + async_tls_do_bio(handle->sock); } } @@ -931,6 +938,11 @@ isc_nm_tlsconnect(isc_nm_t *mgr, isc_sockaddr_t *local, isc_sockaddr_t *peer, REQUIRE(VALID_NM(mgr)); + if (atomic_load(&mgr->closing)) { + cb(NULL, ISC_R_SHUTTINGDOWN, cbarg); + return; + } + nsock = isc_mem_get(mgr->mctx, sizeof(*nsock)); isc__nmsocket_init(nsock, mgr, isc_nm_tlssocket, local); nsock->extrahandlesize = extrahandlesize; From c85949fbe1ee073b766ad9ac966f28351fc42d28 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 28 Jun 2022 21:05:23 +0300 Subject: [PATCH 5/7] TLSDNS: try pass incoming data to OpenSSL if there are any Otherwise the code path will lead to a call to SSL_get_error() returning SSL_ERROR_SSL, which in turn might lead to closing connection to early in an unexpected way, as it is clearly not what is intended. The issue was found when working on loppmgr branch and appears to be timing related as well. Might be responsible for some unexpected transmission failures e.g. on zone transfers. (cherry picked from commit 8585b92f9873c7522614a6264ef5e7f5d0921848) --- lib/isc/netmgr/tlsdns.c | 60 +++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index e099a62bed..a6831763f8 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1061,38 +1061,46 @@ tls_cycle_input(isc_nmsocket_t *sock) { pending = (int)ISC_NETMGR_TCP_RECVBUF_SIZE; } - if ((sock->buf_len + pending) > sock->buf_size) { - isc__nm_alloc_dnsbuf(sock, - sock->buf_len + pending); - } - - len = 0; - rv = SSL_read_ex(sock->tls.tls, - sock->buf + sock->buf_len, - sock->buf_size - sock->buf_len, &len); - if (rv != 1) { - /* - * Process what's in the buffer so far - */ - result = isc__nm_process_sock_buffer(sock); - if (result != ISC_R_SUCCESS) { - goto failure; + if (pending != 0) { + if ((sock->buf_len + pending) > sock->buf_size) + { + isc__nm_alloc_dnsbuf( + sock, sock->buf_len + pending); } - /* - * FIXME: Should we call - * isc__nm_failed_read_cb()? - */ - break; + + len = 0; + rv = SSL_read_ex(sock->tls.tls, + sock->buf + sock->buf_len, + sock->buf_size - sock->buf_len, + &len); + if (rv != 1) { + /* + * Process what's in the buffer so far + */ + result = isc__nm_process_sock_buffer( + sock); + if (result != ISC_R_SUCCESS) { + goto failure; + } + /* + * FIXME: Should we call + * isc__nm_failed_read_cb()? + */ + break; + } + + INSIST((size_t)pending == len); + + sock->buf_len += len; } - - INSIST((size_t)pending == len); - - sock->buf_len += len; - result = isc__nm_process_sock_buffer(sock); if (result != ISC_R_SUCCESS) { goto failure; } + + if (pending == 0) { + break; + } } } else if (!SSL_is_init_finished(sock->tls.tls)) { if (SSL_is_server(sock->tls.tls)) { From d6041e5d45329efcd060e2252eb679cc0cf5e101 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Sat, 2 Jul 2022 02:20:39 +0300 Subject: [PATCH 6/7] *_noresponse, tlsdns_listen_noalpn: csends == 1 is not guaranteed This commit removes an assertion from the unit test which cannot be guaranteed. According to the test, exactly one client send must succeed. However, it cannot really be guaranteed, as do not start to read data in the accept callback on the server nor attach to the accepted handle. Thus, we can expect the connection to be closed soon after we have returned from the callback. Interestingly enough, the test would pass just fine on TCP because: a) there are fewer layers involved and thus there is less processing; b) it is possible for the data to be sent and end up in an internal OS socket buffer without being touched by an application's code on the server. In such a case the client's write callback still would be called successfully; There is a chance for the test to succeed over TLS as well (as it happily did before), but as the code has been changed to close unused connections as soon as possible, the chance is far slimmer now. What can be guaranteed is: * cconnects == 1 (number client connections equals 1); * saccepts == 1 (number of accepted connections equals 1). (cherry picked from commit 0f9b6a7bc18856ffd087ec23bb97f1ea3a5964b6) --- tests/isc/netmgr_test.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/isc/netmgr_test.c b/tests/isc/netmgr_test.c index f407a3647b..94e4bf792a 100644 --- a/tests/isc/netmgr_test.c +++ b/tests/isc/netmgr_test.c @@ -386,9 +386,12 @@ noop_recv_cb(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, static unsigned int noop_accept_cb(isc_nmhandle_t *handle, unsigned int result, void *cbarg) { UNUSED(handle); - UNUSED(result); UNUSED(cbarg); + if (result == ISC_R_SUCCESS) { + (void)atomic_fetch_add(&saccepts, 1); + } + return (0); } @@ -1209,7 +1212,7 @@ stream_noresponse(void **state __attribute__((unused))) { stream_connect(connect_connect_cb, NULL, T_CONNECT, 0); WAIT_FOR_EQ(cconnects, 1); - WAIT_FOR_EQ(csends, 1); + WAIT_FOR_EQ(saccepts, 1); isc_nm_stoplistening(listen_sock); isc_nmsocket_close(&listen_sock); @@ -1223,7 +1226,7 @@ stream_noresponse(void **state __attribute__((unused))) { X(ssends); atomic_assert_int_eq(cconnects, 1); - atomic_assert_int_eq(csends, 1); + atomic_assert_int_eq(saccepts, 1); atomic_assert_int_eq(creads, 0); atomic_assert_int_eq(sreads, 0); atomic_assert_int_eq(ssends, 0); @@ -1728,7 +1731,7 @@ ISC_RUN_TEST_IMPL(tcpdns_noresponse) { connect_connect_cb, NULL, T_CONNECT, 0); WAIT_FOR_EQ(cconnects, 1); - WAIT_FOR_EQ(csends, 1); + WAIT_FOR_EQ(saccepts, 1); isc_nm_stoplistening(listen_sock); isc_nmsocket_close(&listen_sock); @@ -1742,7 +1745,7 @@ ISC_RUN_TEST_IMPL(tcpdns_noresponse) { X(ssends); atomic_assert_int_eq(cconnects, 1); - atomic_assert_int_eq(csends, 1); + atomic_assert_int_eq(saccepts, 1); atomic_assert_int_eq(creads, 0); atomic_assert_int_eq(sreads, 0); atomic_assert_int_eq(ssends, 0); @@ -2279,7 +2282,7 @@ ISC_RUN_TEST_IMPL(tlsdns_noresponse) { tcp_connect_tlsctx, tcp_tlsctx_client_sess_cache); WAIT_FOR_EQ(cconnects, 1); - WAIT_FOR_EQ(csends, 1); + WAIT_FOR_EQ(saccepts, 1); isc_nm_stoplistening(listen_sock); isc_nmsocket_close(&listen_sock); @@ -2293,7 +2296,7 @@ ISC_RUN_TEST_IMPL(tlsdns_noresponse) { X(ssends); atomic_assert_int_eq(cconnects, 1); - atomic_assert_int_eq(csends, 1); + atomic_assert_int_eq(saccepts, 1); atomic_assert_int_eq(creads, 0); atomic_assert_int_eq(sreads, 0); atomic_assert_int_eq(ssends, 0); @@ -2743,7 +2746,6 @@ ISC_RUN_TEST_IMPL(tlsdns_listen_noalpn) { WAIT_FOR_EQ(saccepts, 1); WAIT_FOR_EQ(cconnects, 1); - WAIT_FOR_EQ(csends, 1); isc_nm_stoplistening(listen_sock); isc_nmsocket_close(&listen_sock); @@ -2758,7 +2760,6 @@ ISC_RUN_TEST_IMPL(tlsdns_listen_noalpn) { atomic_assert_int_eq(saccepts, 1); atomic_assert_int_eq(cconnects, 1); - atomic_assert_int_eq(csends, 1); atomic_assert_int_eq(creads, 0); atomic_assert_int_eq(sreads, 0); atomic_assert_int_eq(ssends, 0); From 814baa3e6baa7b673836b08a07534de47c2404ac Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Sat, 2 Jul 2022 03:31:35 +0300 Subject: [PATCH 7/7] TLS: do not ignore accept callback result Before this change the TLS code would ignore the accept callback result, and would not try to gracefully close the connection. This had not been noticed, as it is not really required for DoH. Now the code tries to shut down the TLS connection gracefully when accepting it is not successful. (cherry picked from commit ffcb54211e148ecc7e145f6ea79a0ab2a58a2287) --- lib/isc/netmgr/tlsstream.c | 48 ++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 2f3314d4f7..a7cebd4669 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -324,7 +324,7 @@ tls_process_outgoing(isc_nmsocket_t *sock, bool finish, } static int -tls_try_handshake(isc_nmsocket_t *sock) { +tls_try_handshake(isc_nmsocket_t *sock, isc_result_t *presult) { int rv = 0; isc_nmhandle_t *tlshandle = NULL; @@ -342,13 +342,18 @@ tls_try_handshake(isc_nmsocket_t *sock) { isc__nmsocket_log_tls_session_reuse(sock, sock->tlsstream.tls); tlshandle = isc__nmhandle_get(sock, &sock->peer, &sock->iface); if (sock->tlsstream.server) { - sock->listener->accept_cb(tlshandle, result, - sock->listener->accept_cbarg); + result = sock->listener->accept_cb( + tlshandle, result, + sock->listener->accept_cbarg); } else { tls_call_connect_cb(sock, tlshandle, result); } isc_nmhandle_detach(&tlshandle); sock->tlsstream.state = TLS_IO; + + if (presult != NULL) { + *presult = result; + } } return (rv); @@ -397,7 +402,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, SSL_set_connect_state(sock->tlsstream.tls); } sock->tlsstream.state = TLS_HANDSHAKE; - rv = tls_try_handshake(sock); + rv = tls_try_handshake(sock, NULL); INSIST(SSL_is_init_finished(sock->tlsstream.tls) == 0); } else if (sock->tlsstream.state == TLS_CLOSED) { return; @@ -420,7 +425,21 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, * handshake is done. */ if (sock->tlsstream.state == TLS_HANDSHAKE) { - rv = tls_try_handshake(sock); + isc_result_t hs_result = ISC_R_UNSET; + rv = tls_try_handshake(sock, &hs_result); + if (sock->tlsstream.state == TLS_IO && + hs_result != ISC_R_SUCCESS) { + /* + * The accept callback has been called + * unsuccessfully. Let's try to shut + * down the TLS connection gracefully. + */ + INSIST(SSL_is_init_finished( + sock->tlsstream.tls) == + 1); + INSIST(!atomic_load(&sock->client)); + finish = true; + } } } else if (send_data != NULL) { INSIST(received_data == NULL); @@ -448,7 +467,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, /* Decrypt and pass data from network to client */ if (sock->tlsstream.state >= TLS_IO && sock->recv_cb != NULL && !atomic_load(&sock->readpaused) && - sock->statichandle != NULL) + sock->statichandle != NULL && !finish) { uint8_t recv_buf[TLS_BUF_SIZE]; INSIST(sock->tlsstream.state > TLS_HANDSHAKE); @@ -467,9 +486,9 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, * nullified (it happens in netmgr.c). If it is * the case, then it means that we are not * interested in keeping the connection alive - * anymore. Let's shutdown the SSL session, send - * what we have in the SSL buffers, and close - * the connection. + * anymore. Let's shut down the SSL session, + * send what we have in the SSL buffers, + * and close the connection. */ if (sock->statichandle == NULL) { finish = true; @@ -527,8 +546,13 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, return; } + if (sock->outerhandle == NULL) { + return; + } + + INSIST(VALID_NMHANDLE(sock->outerhandle)); + if (sock->tlsstream.reading) { - INSIST(VALID_NMHANDLE(sock->outerhandle)); isc_nm_resumeread(sock->outerhandle); } else if (sock->tlsstream.state == TLS_HANDSHAKE) { sock->tlsstream.reading = true; @@ -1086,8 +1110,8 @@ isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) { } else if (sock->type == isc_nm_tlssocket) { if (sock->tlsstream.tls != NULL) { /* - * Let's shutdown the TLS session properly so that the - * session will remain resumable, if required. + * Let's shut down the TLS session properly so that + * the session will remain resumable, if required. */ tls_try_shutdown(sock->tlsstream.tls, true); tls_keep_client_tls_session(sock);