From 237ce05b89fe3dd3a928b1879573195806d24a35 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. --- 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 8ea7155223..ebb57a3390 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -1771,6 +1771,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 68aabecfaf..4b416fae06 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -569,8 +569,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 547c518b02..867267ea11 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -1119,6 +1119,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 88524e26ecae286bf62d4a4b5535c70ccff1573d 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. --- 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 867267ea11..69b867964b 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -353,6 +353,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) { @@ -493,6 +510,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) { @@ -504,6 +522,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 ac4fb34f18978d95e100e4218880dce834950a42 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. --- 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 69b867964b..adde8e069a 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -951,13 +951,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 fc74b15e677d8810aea3b5abcc1b1f7eb9e7541a 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. --- 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 adde8e069a..7ba09d8f45 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -671,6 +671,9 @@ isc_nm_listentls(isc_nm_t *mgr, uint32_t workers, 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)); @@ -829,6 +832,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); } } @@ -926,6 +933,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->result = ISC_R_UNSET; From 8585b92f9873c7522614a6264ef5e7f5d0921848 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. --- 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 e2b0f5fe66..42f47cb423 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -1059,38 +1059,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 0f9b6a7bc18856ffd087ec23bb97f1ea3a5964b6 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). --- 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 1bd5d68819..d4f002f84c 100644 --- a/tests/isc/netmgr_test.c +++ b/tests/isc/netmgr_test.c @@ -384,9 +384,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); } @@ -1217,7 +1220,7 @@ stream_noresponse(void **state __attribute__((unused))) { stream_connect(connect_connect_cb, NULL, T_CONNECT); 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); @@ -1231,7 +1234,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); @@ -1729,7 +1732,7 @@ ISC_RUN_TEST_IMPL(tcpdns_noresponse) { connect_connect_cb, NULL, T_CONNECT); 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); @@ -1743,7 +1746,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); @@ -2282,7 +2285,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); @@ -2296,7 +2299,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); @@ -2747,7 +2750,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); @@ -2762,7 +2764,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 ffcb54211e148ecc7e145f6ea79a0ab2a58a2287 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. --- 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 7ba09d8f45..169b391002 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -323,7 +323,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; @@ -341,13 +341,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); @@ -396,7 +401,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; @@ -419,7 +424,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); @@ -447,7 +466,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); @@ -466,9 +485,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; @@ -526,8 +545,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; @@ -1080,8 +1104,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);