Bound the StopTLS V1 downgrade with a timeout

Summary: Bounds the StopTLS V1 downgrade with the connection's handshake-timeout budget, so a peer that stalls it is dropped rather than holding its transport open indefinitely.

Reviewed By: robertroeser

Differential Revision: D116715818

fbshipit-source-id: 479375138f8296a71ad768a9550708b03661be5e

Author: Ankit Kumar

third-party/thrift/src/thrift/lib/cpp2/fast_thrift/connection/security/handler/StopTLSV1Handler.h

@@ -17,6 +17,7 @@
 #pragma once
 
 #include <cstdint>
+#include <optional>
 #include <utility>
 
 #include <fizz/server/AsyncFizzServer.h>
@@ -50,9 +51,9 @@ namespace apache::thrift::fast_thrift::connection::security::handler {
  * StopTLS failures are logged at DBG3 and the connection is dropped.
  *
  * The downgrades parked here are capped: past the cap a connection is refused
- * rather than started. The exchange has no timeout of its own, so a peer that
- * negotiates StopTLS V1 and then stalls the downgrade holds its socket until
- * it disconnects.
+ * rather than started. Each one is also bounded by the handshake-timeout
+ * budget off the incoming message, so a peer that negotiates StopTLS V1 and
+ * then stalls the downgrade is dropped rather than parked indefinitely.
  */
 class StopTLSV1Handler {
  public:
@@ -118,9 +119,13 @@ class StopTLSV1Handler {
     fizz::server::AsyncFizzServer::UniquePtr fizzServer(fizzPtr);
     auto clientAddr = std::move(incoming.clientAddr);
     auto extension = std::move(incoming.extension);
+    auto stopTLSTimeout = incoming.tlsParams
+        ? incoming.tlsParams->handshakeTimeout
+        : std::nullopt;
 
     util::StopTLSHelper::UniquePtr helper(new util::StopTLSHelper(
         std::move(fizzServer),
+        stopTLSTimeout,
         [this](util::StopTLSHelper* h) noexcept { inFlight_.erase(h); },
         [this, clientAddr, extension](
             folly::AsyncTransport::UniquePtr plaintext,

third-party/thrift/src/thrift/lib/cpp2/fast_thrift/connection/security/util/StopTLSHelper.cpp

@@ -24,11 +24,31 @@
 
 namespace apache::thrift::fast_thrift::connection::security::util {
 
+namespace {
+
+// Zero already means "unbounded" to AsyncStopTLS, so a non-positive budget
+// cannot be honoured as a deadline. Degrade to unbounded — the documented
+// behavior for an absent budget — and shout, since the only way to arrive
+// here is misuse.
+std::chrono::milliseconds resolveTimeout(
+    std::optional<std::chrono::milliseconds> timeout) {
+  if (timeout && timeout->count() <= 0) {
+    XLOG(DFATAL) << "StopTLSHelper: pass std::nullopt for unbounded, not "
+                 << timeout->count() << "ms; treating as unbounded.";
+    return std::chrono::milliseconds{0};
+  }
+  return timeout.value_or(std::chrono::milliseconds{0});
+}
+
+} // namespace
+
 StopTLSHelper::StopTLSHelper(
     fizz::server::AsyncFizzServer::UniquePtr fizzServer,
+    std::optional<std::chrono::milliseconds> timeout,
     OnTerminal onTerminal,
     Callback callback)
     : fizzServer_(std::move(fizzServer)),
+      timeout_(resolveTimeout(timeout)),
       onTerminal_(std::move(onTerminal)),
       callback_(std::move(callback)) {}
 
@@ -49,12 +69,12 @@ void StopTLSHelper::start() {
 
   XLOG(DBG3) << "Beginning StopTLS V1 negotiation";
   stopTlsFrame_.reset(new apache::thrift::AsyncStopTLS(*this));
-  // 0ms = no internal timeout. Matches legacy FizzPeeker.cpp convention;
-  // shutdown lifetime is managed by the owner via cancel().
+  // AsyncStopTLS arms its own timer when this is non-zero, and routes expiry
+  // through stopTLSError like any other failure. Zero leaves the exchange
+  // unbounded, which is only safe for an owner that imposes a deadline of its
+  // own; nothing here does, so the budget is passed through instead.
   stopTlsFrame_->start(
-      fizzServer_.get(),
-      apache::thrift::AsyncStopTLS::Role::Server,
-      std::chrono::milliseconds{0});
+      fizzServer_.get(), apache::thrift::AsyncStopTLS::Role::Server, timeout_);
 }
 
 void StopTLSHelper::stopTLSSuccess(std::unique_ptr<folly::IOBuf> postTLSData) {

third-party/thrift/src/thrift/lib/cpp2/fast_thrift/connection/security/util/StopTLSHelper.h

@@ -16,7 +16,9 @@
 
 #pragma once
 
+#include <chrono>
 #include <memory>
+#include <optional>
 
 #include <fizz/server/AsyncFizzServer.h>
 #include <folly/ExceptionWrapper.h>
@@ -36,7 +38,11 @@ namespace apache::thrift::fast_thrift::connection::security::util {
  *   - Constructed on the EventBase thread that owns the fizz transport. Held
  *     by the owner (typically StopTLSV1Handler) via a unique_ptr with a
  *     folly::DelayedDestruction::Destructor deleter.
- *   - start() kicks off the StopTLS exchange.
+ *   - start() kicks off the StopTLS exchange, bounded by the timeout passed
+ *     at construction. Nothing else bounds it: the exchange completes only
+ *     when the peer answers our close_notify with its own, so without a
+ *     deadline a peer that stops responding holds the transport open until
+ *     it disconnects.
  *   - On success/error, invokes the user Callback exactly once and then
  *     synchronously invokes onTerminal(this) so the owner can drop its
  *     unique_ptr. The deleter triggers destroy(); DelayedDestruction defers
@@ -60,8 +66,13 @@ class StopTLSHelper : private apache::thrift::AsyncStopTLS::Callback,
   // Receives `this`; owner uses it to drop its owning UniquePtr.
   using OnTerminal = folly::Function<void(StopTLSHelper*) noexcept>;
 
+  // timeout bounds the whole exchange; std::nullopt is unbounded. A
+  // non-positive value is misuse and is logged and treated as unbounded —
+  // zero reads as "no timeout" to AsyncStopTLS, the opposite of what a zero
+  // deadline means elsewhere in this pipeline.
   StopTLSHelper(
       fizz::server::AsyncFizzServer::UniquePtr fizzServer,
+      std::optional<std::chrono::milliseconds> timeout,
       OnTerminal onTerminal,
       Callback callback);
 
@@ -97,6 +108,8 @@ class StopTLSHelper : private apache::thrift::AsyncStopTLS::Callback,
       folly::exception_wrapper ex) noexcept;
 
   fizz::server::AsyncFizzServer::UniquePtr fizzServer_;
+  // Resolved for AsyncStopTLS, whose contract is zero-means-unbounded.
+  std::chrono::milliseconds timeout_;
   apache::thrift::AsyncStopTLS::UniquePtr stopTlsFrame_;
   OnTerminal onTerminal_;
   Callback callback_;

third-party/thrift/src/thrift/lib/cpp2/fast_thrift/connection/security/util/test/StopTLSHelperTest.cpp

@@ -18,14 +18,18 @@
 
 #include <sys/socket.h>
 #include <array>
+#include <chrono>
 #include <memory>
+#include <optional>
+#include <string>
 
 #include <gtest/gtest.h>
 
 #include <fizz/server/AsyncFizzServer.h>
 #include <folly/io/async/AsyncSocket.h>
 #include <folly/io/async/DelayedDestruction.h>
 #include <folly/io/async/ScopedEventBaseThread.h>
+#include <folly/synchronization/Baton.h>
 
 #include <thrift/lib/cpp2/fast_thrift/security/FizzServerCertConfig.h>
 #include <thrift/lib/cpp2/fast_thrift/security/FizzServerContextBuilder.h>
@@ -97,6 +101,7 @@ TEST_F(StopTLSHelperTest, CancelBeforeStart) {
     auto fizzServer = makeFizzServer(sp.server);
     current_.reset(new StopTLSHelper(
         std::move(fizzServer),
+        /*timeout=*/std::nullopt,
         [this](StopTLSHelper*) noexcept {
           ++terminalCount_;
           current_.reset();
@@ -121,4 +126,56 @@ TEST_F(StopTLSHelperTest, CancelBeforeStart) {
   ::close(sp.client.toFd());
 }
 
+// A peer that never answers our close_notify would otherwise hold the
+// transport open until it disconnects — nothing else bounds the exchange.
+TEST_F(StopTLSHelperTest, TimesOutWhenPeerNeverAnswers) {
+  constexpr auto kTimeout = std::chrono::milliseconds{50};
+  auto sp = makeSocketPair();
+  folly::Baton<> done;
+  folly::exception_wrapper timeoutEx;
+  // Both stamped on the EventBase thread, ordered before done.post().
+  std::chrono::steady_clock::time_point startedAt;
+  std::chrono::steady_clock::duration elapsed{};
+
+  ASSERT_NE(evb_, nullptr);
+  evb_->runInEventBaseThreadAndWait([&] {
+    auto fizzServer = makeFizzServer(sp.server);
+    current_.reset(new StopTLSHelper(
+        std::move(fizzServer),
+        kTimeout,
+        // Posted from the terminal callback, not the user callback: the
+        // terminal fires last, so waking the main thread any earlier races it
+        // against the writes below.
+        [this, &done, &startedAt, &elapsed](StopTLSHelper*) noexcept {
+          ++terminalCount_;
+          elapsed = std::chrono::steady_clock::now() - startedAt;
+          current_.reset();
+          done.post();
+        },
+        [&](folly::AsyncTransport::UniquePtr,
+            folly::exception_wrapper ex) noexcept {
+          timeoutEx = std::move(ex);
+        }));
+    startedAt = std::chrono::steady_clock::now();
+    current_->start();
+  });
+
+  ASSERT_TRUE(done.try_wait_for(std::chrono::seconds{5}));
+  EXPECT_EQ(terminalCount_, 1u);
+  // Assert on the timer's own error: any other failure would satisfy a bare
+  // "did it fail" check without proving the deadline fired.
+  EXPECT_NE(
+      timeoutEx.what().toStdString().find("timeout expired"), std::string::npos)
+      << "expected AsyncStopTLS timeout, got: " << timeoutEx.what();
+  // Independent of the message: a terminal that arrived before the budget
+  // elapsed cannot have been driven by the deadline. Only a lower bound is
+  // checked — the timer is armed after startedAt, and scheduling slack only
+  // ever pushes the terminal later.
+  const auto elapsedMs =
+      std::chrono::duration_cast<std::chrono::milliseconds>(elapsed);
+  EXPECT_GE(elapsedMs.count(), kTimeout.count());
+
+  ::close(sp.client.toFd());
+}
+
 } // namespace apache::thrift::fast_thrift::connection::security::util::test