Merge pull request #10005 from Icinga/graceful-tls-disconnect

Add a dedicated method for disconnecting TLS connections

Author: julianbrost

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 <boost/asio/ssl/context.hpp>
 #include <boost/asio/ssl/verify_context.hpp>
 #include <boost/asio/ssl/verify_mode.hpp>
@@ -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();
+}

lib/base/tlsstream.hpp

@@ -111,6 +111,9 @@ class AsioTlsStream : public boost::asio::buffered_stream<UnbufferedAsioTlsStrea
 	{
 	}
 
+	void ForceDisconnect();
+	void GracefulDisconnect(boost::asio::io_context::strand& strand, boost::asio::yield_context& yc);
+
 private:
 	inline
 	AsioTlsStream(UnbufferedAsioTlsStreamParams init)

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]);
 	}

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]);
 		}
 	});

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());
 

lib/remote/jsonrpcconnection.cpp

@@ -268,36 +268,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);
 		});
 	}
 }