icinga2 icon indicating copy to clipboard operation
icinga2 copied to clipboard

Add a dedicated method for disconnecting TLS connections

Open julianbrost opened this issue 1 year ago • 0 comments

Properly closing a TLS connection involves sending some shutdown messages so that both ends know that the connection wasn't truncated maliciously. Exchanging those messages can stall for a long time if the underlying TCP connection is broken. The HTTP connection handling was missing any kind of timeout for the TLS shutdown so that dead connections could hang around for a long time.

This PR introduces two new methods on AsioTlsStream, namely ForceDisconnect() which just wraps the call for shutting down the TCP connection and GracefulShutdown() which performs the TLS shutdown with a timeout just like it was done in JsonRpcConnection::Disconnect() before.

As the lambda passed to Timeout has to keep the connection object alive, AsioTlsStream is changed to inherit from SharedObject, adding reference counting to it directly. Previously, it was already created as Shared<AsioTlsStream> everywhere. Thus, a good part of the first commit is changing that type across multiple files.

Open questions:

  • Do we want to try to call ForceDisconnect() directly in case a connection is shut down due to a timeout like "no messages received" on JSON-RPC connections?
  • I'm not yet sure about the last two commits (d6953cc027d986d6c2ddf0a05ee95099c2eb36b5, e90acc57482125ff70a65c5922d5deff3bd32805): they basically replace two calls of async_shutdown() with calls to the new GracefulShutdown(). In both instances, those calls are already guarded by higher-level timeouts (apilistener.cpp, apilistener.cpp, ifwapichecktask.cpp), the idea for replacing them would be to have just a single call to async_shutdown() in our code base that's properly guarded.

fixes #9986

julianbrost avatar Feb 16 '24 16:02 julianbrost