diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 05807b388a..3cd819f76e 100644 --- a/p2p/BUILD.gn +++ b/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", diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 3a52073b76..d2f32de91b 100644 --- a/p2p/base/ice_transport_internal.h +++ b/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) {} - virtual void SetDtlsHandshakeComplete(bool is_dtls_client) {} + virtual void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) {} virtual bool IsDtlsPiggybackSupportedByPeer() { return false; } protected: diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index c82e3f1a15..5b0d48af2e 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/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 data_view = + rtc::MakeArrayView(reinterpret_cast(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(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; diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index cef9714f71..7367391234 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/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 data) override { - dtls_stun_piggyback_controller_.SetDataToPiggyback(data); - } - void SetDtlsHandshakeComplete(bool is_dtls_client) override { - dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client); + + void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) override { + dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client, + is_dtls13); } + 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, diff --git a/p2p/dtls/dtls_ice_integrationtest.cc b/p2p/dtls/dtls_ice_integrationtest.cc index 664535b723..21145ee884 100644 --- a/p2p/dtls/dtls_ice_integrationtest.cc +++ b/p2p/dtls/dtls_ice_integrationtest.cc @@ -12,7 +12,6 @@ #include #include #include -#include #include "api/candidate.h" #include "api/crypto/crypto_options.h" @@ -62,7 +61,8 @@ namespace cricket { using ::testing::IsTrue; class DtlsIceIntegrationTest - : public ::testing::TestWithParam>, + : public ::testing::TestWithParam< + std::tuple>, 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, - DtlsIceIntegrationTest, - ::testing::Values(std::make_pair(false, false), - std::make_pair(true, false), - std::make_pair(false, true), - std::make_pair(true, true))); +// Test cases are parametrized by +// * client-piggybacking-enabled, +// * server-piggybacking-enabled, +// * maximum DTLS version to use. +INSTANTIATE_TEST_SUITE_P( + 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 diff --git a/p2p/dtls/dtls_stun_piggyback_controller.cc b/p2p/dtls/dtls_stun_piggyback_controller.cc index e3c5f5414d..602f6235a2 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller.cc +++ b/p2p/dtls/dtls_stun_piggyback_controller.cc @@ -33,34 +33,44 @@ DtlsStunPiggybackController::DtlsStunPiggybackController( DtlsStunPiggybackController::~DtlsStunPiggybackController() {} -void DtlsStunPiggybackController::SetDtlsHandshakeComplete( - bool is_dtls_client) { +void DtlsStunPiggybackController::SetDtlsHandshakeComplete(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; - if (is_dtls_client) { + // For DTLS 1.3 this is reversed since the handshake has one round trip less. + if ((is_dtls_client && !is_dtls13) || (!is_dtls_client && is_dtls13)) { pending_packet_.Clear(); } } -void DtlsStunPiggybackController::SetDataToPiggyback( +bool DtlsStunPiggybackController::MaybeConsumePacket( rtc::ArrayView data) { RTC_DCHECK_RUN_ON(&sequence_checker_); - if (state_ == State::OFF) { - return; + bool should_consume = + (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 - // if this gets called with fragmented DTLS flights. - pending_packet_.SetData(data); + return false; +} + +void DtlsStunPiggybackController::ClearCachedPacketForTesting() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + pending_packet_.Clear(); } std::optional @@ -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()); } diff --git a/p2p/dtls/dtls_stun_piggyback_controller.h b/p2p/dtls/dtls_stun_piggyback_controller.h index e505eab646..c25d93f3f1 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller.h +++ b/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. - void SetDataToPiggyback(rtc::ArrayView data); + // Intercepts DTLS packets which should go into the STUN packets during the + // handshake. + bool MaybeConsumePacket(rtc::ArrayView data); + void ClearCachedPacketForTesting(); // Called by Connection, when sending a STUN BINDING { REQUEST / RESPONSE } // to obtain optional DTLS data or ACKs. diff --git a/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc b/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc index 42e6e0f1fa..80b28aaf28 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc +++ b/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc @@ -65,7 +65,11 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test { void SendClientToServer(const std::vector data, StunMessageType type) { - client_.SetDataToPiggyback(data); + if (!data.empty()) { + EXPECT_TRUE(client_.MaybeConsumePacket(data)); + } else { + client_.ClearCachedPacketForTesting(); + } std::unique_ptr attr_data; if (client_.GetDataToPiggyback(type)) { attr_data = std::make_unique( @@ -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 data, StunMessageType type) { - server_.SetDataToPiggyback(data); + if (!data.empty()) { + EXPECT_TRUE(server_.MaybeConsumePacket(data)); + } else { + server_.ClearCachedPacketForTesting(); + } std::unique_ptr attr_data; if (server_.GetDataToPiggyback(type)) { attr_data = std::make_unique( @@ -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); } diff --git a/p2p/dtls/dtls_transport.cc b/p2p/dtls/dtls_transport.cc index 6f73c7b577..41ebe78e58 100644 --- a/p2p/dtls/dtls_transport.cc +++ b/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 - // sure we don't accidentally frob the state if it's closed. + int ssl_version_bytes; + 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) {