From 56d58112830d69a259464973f641b8403de552b0 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Mon, 12 Feb 2024 16:28:37 +0100 Subject: [PATCH 1/4] AsioTlsStream: add GracefulDisconnect() and ForceDisconnect() Calling `AsioTlsStream::async_shutdown()` performs a TLS shutdown which exchanges messages (that's why it takes a `yield_context`) and thus has the potential to block the coroutine. Therefore, it should be protected with a timeout. As `async_shutdown()` doesn't simply take a timeout, this has to be implemented using a timer. So far, these timers are scattered throughout the codebase with some places missing them entirely. This commit adds helper functions to properly shutdown a TLS connection with a single function call. --- lib/base/tlsstream.cpp | 64 ++++++++++++++++++++++++++++++++++++++++++ lib/base/tlsstream.hpp | 3 ++ 2 files changed, 67 insertions(+) diff --git a/lib/base/tlsstream.cpp b/lib/base/tlsstream.cpp index a71451a53..ed8005837 100644 --- a/lib/base/tlsstream.cpp +++ b/lib/base/tlsstream.cpp @@ -7,6 +7,8 @@ #include "base/logger.hpp" #include "base/configuration.hpp" #include "base/convert.hpp" +#include "base/defer.hpp" +#include "base/io-engine.hpp" #include #include #include @@ -103,3 +105,65 @@ void UnbufferedAsioTlsStream::BeforeHandshake(handshake_type type) } #endif /* SSL_CTRL_SET_TLSEXT_HOSTNAME */ } + +/** + * Forcefully close the connection, typically (details are up to the operating system) using a TCP RST. + */ +void AsioTlsStream::ForceDisconnect() +{ + if (!lowest_layer().is_open()) { + // Already disconnected, nothing to do. + return; + } + + boost::system::error_code ec; + + // Close the socket. In case the connection wasn't shut down cleanly by GracefulDisconnect(), the operating system + // will typically terminate the connection with a TCP RST. Otherwise, this just releases the file descriptor. + lowest_layer().close(ec); +} + +/** + * Try to cleanly shut down the connection. This involves sending a TLS close_notify shutdown alert and terminating the + * underlying TCP connection. Sending these additional messages can block, hence the method takes a yield context and + * internally implements a timeout of 10 seconds for the operation after which the connection is forcefully terminated + * using ForceDisconnect(). + * + * @param strand Asio strand used for other operations on this connection. + * @param yc Yield context for Asio coroutines + */ +void AsioTlsStream::GracefulDisconnect(boost::asio::io_context::strand& strand, boost::asio::yield_context& yc) +{ + if (!lowest_layer().is_open()) { + // Already disconnected, nothing to do. + return; + } + + { + Timeout::Ptr shutdownTimeout(new Timeout(strand.context(), strand, boost::posix_time::seconds(10), + [this](boost::asio::yield_context yc) { + // Forcefully terminate the connection if async_shutdown() blocked more than 10 seconds. + ForceDisconnect(); + } + )); + Defer cancelTimeout ([&shutdownTimeout]() { + shutdownTimeout->Cancel(); + }); + + // Close the TLS connection, effectively uses SSL_shutdown() to send a close_notify shutdown alert to the peer. + boost::system::error_code ec; + next_layer().async_shutdown(yc[ec]); + } + + if (!lowest_layer().is_open()) { + // Connection got closed in the meantime, most likely by the timeout, so nothing more to do. + return; + } + + // Shut down the TCP connection. + boost::system::error_code ec; + lowest_layer().shutdown(lowest_layer_type::shutdown_both, ec); + + // Clean up the connection (closes the file descriptor). + ForceDisconnect(); +} diff --git a/lib/base/tlsstream.hpp b/lib/base/tlsstream.hpp index 9a6340baf..9eed8d3b1 100644 --- a/lib/base/tlsstream.hpp +++ b/lib/base/tlsstream.hpp @@ -111,6 +111,9 @@ public: { } + void ForceDisconnect(); + void GracefulDisconnect(boost::asio::io_context::strand& strand, boost::asio::yield_context& yc); + private: inline AsioTlsStream(UnbufferedAsioTlsStreamParams init) From 007e3fbe7ef08696706df023a2fe44f110fb2ea2 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Mon, 12 Feb 2024 17:02:35 +0100 Subject: [PATCH 2/4] JsonRpcConnection: use AsioTlsStream::GracefulDisconnect() This new helper functions allows deduplicating the timeout handling for `async_shutdown()`. --- lib/remote/jsonrpcconnection.cpp | 28 +--------------------------- 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/lib/remote/jsonrpcconnection.cpp b/lib/remote/jsonrpcconnection.cpp index cd684af6e..913177a91 100644 --- a/lib/remote/jsonrpcconnection.cpp +++ b/lib/remote/jsonrpcconnection.cpp @@ -225,36 +225,10 @@ void JsonRpcConnection::Disconnect() m_WriterDone.Wait(yc); - /* - * Do not swallow exceptions in a coroutine. - * https://github.com/Icinga/icinga2/issues/7351 - * We must not catch `detail::forced_unwind exception` as - * this is used for unwinding the stack. - * - * Just use the error_code dummy here. - */ - boost::system::error_code ec; - m_CheckLivenessTimer.cancel(); m_HeartbeatTimer.cancel(); - m_Stream->lowest_layer().cancel(ec); - - Timeout::Ptr shutdownTimeout (new Timeout( - m_IoStrand.context(), - m_IoStrand, - boost::posix_time::seconds(10), - [this, keepAlive](asio::yield_context yc) { - boost::system::error_code ec; - m_Stream->lowest_layer().cancel(ec); - } - )); - - m_Stream->next_layer().async_shutdown(yc[ec]); - - shutdownTimeout->Cancel(); - - m_Stream->lowest_layer().shutdown(m_Stream->lowest_layer().shutdown_both, ec); + m_Stream->GracefulDisconnect(m_IoStrand, yc); }); } } From e6d103d0ddd7b22fe3f1f5415d583e851f0327c9 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Mon, 12 Feb 2024 17:03:54 +0100 Subject: [PATCH 3/4] HttpServerConnection: use AsioTlsStream::GracefulDisconnect() This new helper function has proper timeout handling which was missing here. --- lib/remote/httpserverconnection.cpp | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/lib/remote/httpserverconnection.cpp b/lib/remote/httpserverconnection.cpp index bb5831f4a..bb5620c86 100644 --- a/lib/remote/httpserverconnection.cpp +++ b/lib/remote/httpserverconnection.cpp @@ -88,23 +88,9 @@ void HttpServerConnection::Disconnect(boost::asio::yield_context yc) Log(LogInformation, "HttpServerConnection") << "HTTP client disconnected (from " << m_PeerAddress << ")"; - /* - * Do not swallow exceptions in a coroutine. - * https://github.com/Icinga/icinga2/issues/7351 - * We must not catch `detail::forced_unwind exception` as - * this is used for unwinding the stack. - * - * Just use the error_code dummy here. - */ - boost::system::error_code ec; - m_CheckLivenessTimer.cancel(); - m_Stream->lowest_layer().cancel(ec); - - m_Stream->next_layer().async_shutdown(yc[ec]); - - m_Stream->lowest_layer().shutdown(m_Stream->lowest_layer().shutdown_both, ec); + m_Stream->GracefulDisconnect(m_IoStrand, yc); auto listener (ApiListener::GetInstance()); From a506d562ae1eddbb40f5d0e1cc4beca40574e670 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 13 Nov 2024 17:18:28 +0100 Subject: [PATCH 4/4] Add comment for remaining uses of async_shutdown() why it's safe The reason for introducing AsioTlsStream::GracefulDisconnect() was to handle the TLS shutdown properly with a timeout since it involves a timeout. However, the implementation of this timeout involves spwaning coroutines which are redundant in some cases. This commit adds comments to the remaining calls of async_shutdown() stating why calling it is safe in these places. --- lib/methods/ifwapichecktask.cpp | 2 ++ lib/remote/apilistener.cpp | 3 +++ 2 files changed, 5 insertions(+) diff --git a/lib/methods/ifwapichecktask.cpp b/lib/methods/ifwapichecktask.cpp index 16e2f7e63..9a62444b6 100644 --- a/lib/methods/ifwapichecktask.cpp +++ b/lib/methods/ifwapichecktask.cpp @@ -102,6 +102,8 @@ static void DoIfwNetIo( } { + // Using async_shutdown() instead of AsioTlsStream::GracefulDisconnect() as this whole function + // is already guarded by a timeout based on the check timeout. boost::system::error_code ec; sslConn.async_shutdown(yc[ec]); } diff --git a/lib/remote/apilistener.cpp b/lib/remote/apilistener.cpp index 201b22865..9c2a489da 100644 --- a/lib/remote/apilistener.cpp +++ b/lib/remote/apilistener.cpp @@ -719,6 +719,9 @@ void ApiListener::NewClientHandlerInternal( // Ignore the error, but do not throw an exception being swallowed at all cost. // https://github.com/Icinga/icinga2/issues/7351 boost::system::error_code ec; + + // Using async_shutdown() instead of AsioTlsStream::GracefulDisconnect() as this whole function + // is already guarded by a timeout based on the connect timeout. sslConn.async_shutdown(yc[ec]); } });