dtls-stun piggybacking: make it compatible with DTLS 1.3

DTLS 1.3 encrypts more parts of the handshake so we move from
deep packet inspection to looking at the state of DTLS to
decide whether to intercept the packet.

BUG=webrtc:367395350

Change-Id: Idb1eda0437f24002f48381af5d6a167a4a153381
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374501
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43794}

p2p/BUILD.gn

@@ -570,7 +570,6 @@ rtc_library("p2p_transport_channel") {
     ":connection",
     ":connection_info",
     ":dtls_stun_piggyback_controller",
-    ":dtls_utils",
     ":ice_agent_interface",
     ":ice_controller_factory_interface",
     ":ice_controller_interface",

p2p/base/ice_transport_internal.h

@@ -410,7 +410,7 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal {
     piggybacked_dtls_callback_ = std::move(callback);
   }
   virtual void SetDtlsDataToPiggyback(rtc::ArrayView<const uint8_t>) {}
-  virtual void SetDtlsHandshakeComplete(bool is_dtls_client) {}
+  virtual void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) {}
   virtual bool IsDtlsPiggybackSupportedByPeer() { return false; }
 
  protected:

p2p/base/p2p_transport_channel.cc

@@ -55,7 +55,6 @@
 #include "p2p/base/regathering_controller.h"
 #include "p2p/base/transport_description.h"
 #include "p2p/base/wrapping_active_ice_controller.h"
-#include "p2p/dtls/dtls_utils.h"
 #include "rtc_base/async_packet_socket.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/dscp.h"
@@ -1634,22 +1633,22 @@ int P2PTransportChannel::SendPacket(const char* data,
     error_ = EINVAL;
     return -1;
   }
+
+  // If we try to use DTLS-in-STUN, DTLS packets will be sent as part of STUN
+  // packets and are consumed here.
+  rtc::ArrayView<const uint8_t> data_view =
+      rtc::MakeArrayView(reinterpret_cast<const uint8_t*>(data), len);
+  if (config_.dtls_handshake_in_stun &&
+      dtls_stun_piggyback_controller_.MaybeConsumePacket(data_view)) {
+    return len;
+  }
+
   // If we don't think the connection is working yet, return ENOTCONN
   // instead of sending a packet that will probably be dropped.
   if (!ReadyToSend(selected_connection_)) {
     error_ = ENOTCONN;
     return -1;
   }
-  /*
-   * When trying DTLS-STUN piggyback we need to drop handshake packets
-   * as we start fresh if this fails.
-   */
-  if (config_.dtls_handshake_in_stun && IsDtlsPiggybackSupportedByPeer() &&
-      IsDtlsHandshakePacket(
-          rtc::MakeArrayView(reinterpret_cast<const uint8_t*>(data), len))) {
-    RTC_LOG(LS_INFO) << "Dropping DTLS handshake while attemping DTLS-in-STUN";
-    return len;
-  }
 
   packets_sent_++;
   last_sent_packet_id_ = options.packet_id;

p2p/base/p2p_transport_channel.h

@@ -252,12 +252,12 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal,
   const webrtc::FieldTrialsView* field_trials() const override {
     return field_trials_;
   }
-  void SetDtlsDataToPiggyback(rtc::ArrayView<const uint8_t> data) override {
+
-    dtls_stun_piggyback_controller_.SetDataToPiggyback(data);
+  void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) override {
-  }
+    dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client,
-  void SetDtlsHandshakeComplete(bool is_dtls_client) override {
+                                                             is_dtls13);
-    dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client);
   }
+
   bool IsDtlsPiggybackSupportedByPeer() override {
     RTC_DCHECK_RUN_ON(network_thread_);
     return config_.dtls_handshake_in_stun &&
@@ -265,6 +265,15 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal,
                DtlsStunPiggybackController::State::OFF;
   }
 
+  bool IsDtlsPiggybackHandshaking() {
+    RTC_DCHECK_RUN_ON(network_thread_);
+    return config_.dtls_handshake_in_stun &&
+           (dtls_stun_piggyback_controller_.state() ==
+                DtlsStunPiggybackController::State::TENTATIVE ||
+            dtls_stun_piggyback_controller_.state() ==
+                DtlsStunPiggybackController::State::CONFIRMED);
+  }
+
  private:
   P2PTransportChannel(
       absl::string_view transport_name,

p2p/dtls/dtls_ice_integrationtest.cc

@@ -12,7 +12,6 @@
 #include <memory>
 #include <optional>
 #include <tuple>
-#include <utility>
 
 #include "api/candidate.h"
 #include "api/crypto/crypto_options.h"
@@ -62,7 +61,8 @@ namespace cricket {
 using ::testing::IsTrue;
 
 class DtlsIceIntegrationTest
-    : public ::testing::TestWithParam<std::tuple<bool, bool>>,
+    : public ::testing::TestWithParam<
+          std::tuple<bool, bool, rtc::SSLProtocolVersion>>,
       public sigslot::has_slots<> {
  public:
   void CandidateC2S(IceTransportInternal*, const Candidate& c) {
@@ -95,11 +95,11 @@ class DtlsIceIntegrationTest
         client_dtls_(client_ice_.get(),
                      webrtc::CryptoOptions(),
                      /*event_log=*/nullptr,
-                     rtc::SSL_PROTOCOL_DTLS_12),
+                     std::get<2>(GetParam())),
         server_dtls_(server_ice_.get(),
                      webrtc::CryptoOptions(),
                      /*event_log=*/nullptr,
-                     rtc::SSL_PROTOCOL_DTLS_12),
+                     std::get<2>(GetParam())),
         client_ice_parameters_("c_ufrag",
                                "c_icepwd_something_something",
                                false),
@@ -190,11 +190,19 @@ TEST_P(DtlsIceIntegrationTest, SmokeTest) {
             client_dtls_stun_piggyback_ && server_dtls_stun_piggyback_);
 }
 
-INSTANTIATE_TEST_SUITE_P(DtlsStunPiggybackingIntegrationTest,
+// Test cases are parametrized by
-                         DtlsIceIntegrationTest,
+// * client-piggybacking-enabled,
-                         ::testing::Values(std::make_pair(false, false),
+// * server-piggybacking-enabled,
-                                           std::make_pair(true, false),
+// * maximum DTLS version to use.
-                                           std::make_pair(false, true),
+INSTANTIATE_TEST_SUITE_P(
-                                           std::make_pair(true, true)));
+    DtlsStunPiggybackingIntegrationTest,
+    DtlsIceIntegrationTest,
+    ::testing::Values(
+        std::make_tuple(false, false, rtc::SSL_PROTOCOL_DTLS_12),
+        std::make_tuple(true, false, rtc::SSL_PROTOCOL_DTLS_12),
+        std::make_tuple(false, true, rtc::SSL_PROTOCOL_DTLS_12),
+        std::make_tuple(true, true, rtc::SSL_PROTOCOL_DTLS_12),
+        // Skip negative cases that are behaving similar for DTLS 1.3
+        std::make_tuple(true, true, rtc::SSL_PROTOCOL_DTLS_13)));
 
 }  // namespace cricket

p2p/dtls/dtls_stun_piggyback_controller.cc

@@ -33,34 +33,44 @@ DtlsStunPiggybackController::DtlsStunPiggybackController(
 
 DtlsStunPiggybackController::~DtlsStunPiggybackController() {}
 
-void DtlsStunPiggybackController::SetDtlsHandshakeComplete(
+void DtlsStunPiggybackController::SetDtlsHandshakeComplete(bool is_dtls_client,
-    bool is_dtls_client) {
+                                                           bool is_dtls13) {
   RTC_DCHECK_RUN_ON(&sequence_checker_);
   // Peer does not support this so fallback to a normal DTLS handshake
   // happened.
   if (state_ == State::OFF) {
     return;
   }
-  // As DTLS server we need to keep the last flight around until
+  state_ = State::PENDING;
+  // As DTLS 1.2 server we need to keep the last flight around until
   // we receive the post-handshake acknowledgment.
-  // As DTLS client we have nothing more to send at this point
+  // As DTLS 1.2 client we have nothing more to send at this point
   // but will continue to send ACK attributes until receiving
   // the last flight from the server.
-  state_ = State::PENDING;
+  // For DTLS 1.3 this is reversed since the handshake has one round trip less.
-  if (is_dtls_client) {
+  if ((is_dtls_client && !is_dtls13) || (!is_dtls_client && is_dtls13)) {
     pending_packet_.Clear();
   }
 }
 
-void DtlsStunPiggybackController::SetDataToPiggyback(
+bool DtlsStunPiggybackController::MaybeConsumePacket(
     rtc::ArrayView<const uint8_t> data) {
   RTC_DCHECK_RUN_ON(&sequence_checker_);
-  if (state_ == State::OFF) {
+  bool should_consume =
-    return;
+      (state_ == State::TENTATIVE || state_ == State::CONFIRMED) &&
+      IsDtlsPacket(data);
+  if (should_consume) {
+    // Note: this overwrites the existing packets which is an issue
+    // if this gets called with fragmented DTLS flights.
+    pending_packet_.SetData(data);
+    return true;
   }
-  // Note: this overwrites the existing packets which is an issue
+  return false;
-  // if this gets called with fragmented DTLS flights.
+}
-  pending_packet_.SetData(data);
+
+void DtlsStunPiggybackController::ClearCachedPacketForTesting() {
+  RTC_DCHECK_RUN_ON(&sequence_checker_);
+  pending_packet_.Clear();
 }
 
 std::optional<absl::string_view>
@@ -124,7 +134,7 @@ void DtlsStunPiggybackController::ReportDataPiggybacked(
     state_ = State::CONFIRMED;
   }
 
-  if (ack != nullptr) {
+  if (ack != nullptr && !ack->string_view().empty()) {
     RTC_LOG(LS_VERBOSE) << "DTLS-STUN piggybacking ACK: "
                         << rtc::hex_encode(ack->string_view());
   }

p2p/dtls/dtls_stun_piggyback_controller.h

@@ -60,10 +60,12 @@ class DtlsStunPiggybackController {
   }
 
   // Called by DtlsTransport when handshake is complete.
-  void SetDtlsHandshakeComplete(bool is_dtls_client);
+  void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13);
 
-  // Called by DtlsTransport transport when there is data to piggyback.
+  // Intercepts DTLS packets which should go into the STUN packets during the
-  void SetDataToPiggyback(rtc::ArrayView<const uint8_t> data);
+  // handshake.
+  bool MaybeConsumePacket(rtc::ArrayView<const uint8_t> data);
+  void ClearCachedPacketForTesting();
 
   // Called by Connection, when sending a STUN BINDING { REQUEST / RESPONSE }
   // to obtain optional DTLS data or ACKs.

p2p/dtls/dtls_stun_piggyback_controller_unittest.cc

@@ -65,7 +65,11 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test {
 
   void SendClientToServer(const std::vector<uint8_t> data,
                           StunMessageType type) {
-    client_.SetDataToPiggyback(data);
+    if (!data.empty()) {
+      EXPECT_TRUE(client_.MaybeConsumePacket(data));
+    } else {
+      client_.ClearCachedPacketForTesting();
+    }
     std::unique_ptr<StunByteStringAttribute> attr_data;
     if (client_.GetDataToPiggyback(type)) {
       attr_data = std::make_unique<StunByteStringAttribute>(
@@ -77,14 +81,14 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test {
           STUN_ATTR_META_DTLS_IN_STUN_ACK, *client_.GetAckToPiggyback(type));
     }
     server_.ReportDataPiggybacked(attr_data.get(), attr_ack.get());
-    if (data == dtls_flight3) {
-      // When receiving flight 3, server handshake is complete.
-      server_.SetDtlsHandshakeComplete(/*is_client=*/false);
-    }
   }
   void SendServerToClient(const std::vector<uint8_t> data,
                           StunMessageType type) {
-    server_.SetDataToPiggyback(data);
+    if (!data.empty()) {
+      EXPECT_TRUE(server_.MaybeConsumePacket(data));
+    } else {
+      server_.ClearCachedPacketForTesting();
+    }
     std::unique_ptr<StunByteStringAttribute> attr_data;
     if (server_.GetDataToPiggyback(type)) {
       attr_data = std::make_unique<StunByteStringAttribute>(
@@ -97,8 +101,11 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test {
     }
     client_.ReportDataPiggybacked(attr_data.get(), attr_ack.get());
     if (data == dtls_flight4) {
+      // After sending flight 4, the server handshake is complete.
+      server_.SetDtlsHandshakeComplete(/*is_client=*/false,
+                                       /*is_dtls13=*/false);
       // When receiving flight 4, client handshake is complete.
-      client_.SetDtlsHandshakeComplete(/*is_client=*/true);
+      client_.SetDtlsHandshakeComplete(/*is_client=*/true, /*is_dtls13=*/false);
     }
   }
 
@@ -143,7 +150,7 @@ TEST_F(DtlsStunPiggybackControllerTest, FirstClientPacketLost) {
   // Flight 2+3
   SendServerToClient(dtls_flight2, STUN_BINDING_REQUEST);
   SendClientToServer(dtls_flight3, STUN_BINDING_RESPONSE);
-  EXPECT_EQ(server_.state(), State::PENDING);
+  EXPECT_EQ(server_.state(), State::CONFIRMED);
   EXPECT_EQ(client_.state(), State::CONFIRMED);
 
   // Flight 4
@@ -235,9 +242,9 @@ TEST_F(DtlsStunPiggybackControllerTest,
   DisableSupport(server_);
 
   // Set DTLS complete after normal handshake.
-  client_.SetDtlsHandshakeComplete(true);
+  client_.SetDtlsHandshakeComplete(/*is_client=*/true, /*is_dtls13=*/false);
   EXPECT_EQ(client_.state(), State::OFF);
-  server_.SetDtlsHandshakeComplete(true);
+  server_.SetDtlsHandshakeComplete(/*is_client=*/false, /*is_dtls13=*/false);
   EXPECT_EQ(server_.state(), State::OFF);
 }
 

p2p/dtls/dtls_transport.cc

@@ -99,12 +99,6 @@ rtc::StreamResult StreamInterfaceChannel::Write(
     int& /* error */) {
   RTC_DCHECK_RUN_ON(&callback_sequence_);
 
-  if (IsDtlsHandshakePacket(data) &&
-      ice_transport_->IsDtlsPiggybackSupportedByPeer()) {
-    ice_transport_->SetDtlsDataToPiggyback(data);
-    // The ICE transport is responsible for dropping these packets.
-  }
-
   // Always succeeds, since this is an unreliable transport anyway.
   // TODO(zhihuang): Should this block if ice_transport_'s temporarily
   // unwritable?
@@ -545,12 +539,12 @@ void DtlsTransport::ConnectToIceTransport() {
       [this](rtc::PacketTransportInternal* transport,
              const rtc::ReceivedPacket& packet) {
         RTC_DCHECK(dtls_active_);
-        RTC_DCHECK(IsDtlsHandshakePacket(packet.payload()));
+        RTC_DCHECK(IsDtlsPacket(packet.payload()));
         if (!dtls_active_) {
           // Not doing DTLS.
           return;
         }
-        if (!IsDtlsHandshakePacket(packet.payload())) {
+        if (!IsDtlsPacket(packet.payload())) {
           return;
         }
         OnReadPacket(transport, packet);
@@ -748,15 +742,22 @@ void DtlsTransport::OnReadyToSend(
 
 void DtlsTransport::OnDtlsEvent(int sig, int err) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
+  RTC_DCHECK(dtls_);
+
   if (sig & rtc::SE_OPEN) {
     // This is the first time.
     RTC_LOG(LS_INFO) << ToString() << ": DTLS handshake complete.";
+    // The check for OPEN shouldn't be necessary but let's make
+    // sure we don't accidentally frob the state if it's closed.
     if (dtls_->GetState() == rtc::SS_OPEN) {
-      // The check for OPEN shouldn't be necessary but let's make
+      int ssl_version_bytes;
-      // sure we don't accidentally frob the state if it's closed.
+      bool ret = dtls_->GetSslVersionBytes(&ssl_version_bytes);
+      RTC_DCHECK(ret);
+      ice_transport_->SetDtlsHandshakeComplete(
+          dtls_role_ == rtc::SSL_CLIENT,
+          ssl_version_bytes == rtc::kDtls13VersionBytes);
       set_dtls_state(webrtc::DtlsTransportState::kConnected);
       set_writable(true);
-      ice_transport_->SetDtlsHandshakeComplete(dtls_role_ == rtc::SSL_CLIENT);
     }
   }
   if (sig & rtc::SE_READ) {