dtls-in-stun: actually wire up behavior to ice-option which means that we no longer speculative start without an indication that the peer supports this. Peer support is discovered by two triggers: * an offer/answer with the ice-option * a STUN packet with embedded DTLS which is passed to the DTLS transport as piggybacked If the peer does not support SPED this no longer configures the 24h timeout and behaves like the existing non-sped path. If the peer does support SPED then it is also assumed to support the API for adjusting the timeout mid-session. The timeout could be set lower though, e.g. to the signaling RTT. Bug: webrtc:367395350 Change-Id: Ia77106dabf3268c007649ef5bc3d9aac7033d330 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/476202 Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Commit-Queue: Philipp Hancke <philipp.hancke@googlemail.com> Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#48808}
diff --git a/p2p/dtls/dtls_ice_integration_fixture.h b/p2p/dtls/dtls_ice_integration_fixture.h index 3777fb0..f1f2cc7 100644 --- a/p2p/dtls/dtls_ice_integration_fixture.h +++ b/p2p/dtls/dtls_ice_integration_fixture.h
@@ -566,6 +566,11 @@ ep.env, ep.ice_transport, crypto_options, ep.config.max_protocol_version); + // No SDP exchange in this fixture; stand in for JsepTransport's call. + if (ep.config.dtls_in_stun) { + ep.dtls->MaybeStartDtlsInStun(); + } + if (ice_lite_agent) { ep.dtls->SetFakeIceLite(); }
diff --git a/p2p/dtls/dtls_transport.cc b/p2p/dtls/dtls_transport.cc index 471be59..6bd53c4 100644 --- a/p2p/dtls/dtls_transport.cc +++ b/p2p/dtls/dtls_transport.cc
@@ -262,12 +262,13 @@ RTC_DCHECK(ice_transport_); ConnectToIceTransport(); if (SSLStreamAdapter::IsBoringSsl()) { - dtls_in_stun_ = ice_transport_->internal()->config().dtls_handshake_in_stun; + dtls_in_stun_configured_ = + ice_transport_->internal()->config().dtls_handshake_in_stun; } } DtlsTransportInternalImpl::~DtlsTransportInternalImpl() { - if (dtls_in_stun_) { + if (dtls_in_stun_configured_) { CompleteDtlsInStun(/*success=*/false); } ice_transport()->ResetDtlsStunPiggybackCallbacks(); @@ -509,14 +510,14 @@ RTC_DCHECK(dtls_role_); if (SSLStreamAdapter::IsBoringSsl()) { - dtls_in_stun_ = ice_transport()->config().dtls_handshake_in_stun; + dtls_in_stun_configured_ = ice_transport()->config().dtls_handshake_in_stun; } { auto downward = std::make_unique<StreamInterfaceChannel>(ice_transport()); StreamInterfaceChannel* downward_ptr = downward.get(); - if (dtls_in_stun_ && !dtls_in_stun_complete_) { + if (dtls_in_stun_configured_ && !dtls_in_stun_complete_) { downward_ptr->SetDtlsStunPiggybackController( &dtls_stun_piggyback_controller_); @@ -545,7 +546,7 @@ // TODO(jonaso,webrtc:367395350): Add more clever handling of MTU // (such as automatic packetization smoothing). - if (dtls_in_stun_) { + if (dtls_in_stun_configured_) { // - This is only needed when using PQC but we don't know that here. // - 900 is sufficiently small so that dtls pqc handshake packets // can get put into STUN attributes and still fit into two packets. @@ -589,7 +590,8 @@ } RTC_LOG(LS_INFO) << ToString() - << ": DTLS setup complete, dtls_in_stun: " << dtls_in_stun_; + << ": DTLS setup complete, dtls_in_stun_configured: " + << dtls_in_stun_configured_; // If the underlying ice_transport is already writable at this point, we may // be able to start DTLS right away. @@ -693,6 +695,19 @@ } } +void DtlsTransportInternalImpl::MaybeStartDtlsInStun() { + RTC_DCHECK_RUN_ON(&thread_checker_); + if (dtls_in_stun_negotiated_) { + return; + } + dtls_in_stun_negotiated_ = true; + // The remote description may arrive after ICE became writable and DTLS + // already started. + if (dtls_state() == DtlsTransportState::kNew) { + MaybeStartDtls(); + } +} + IceTransportInternal* DtlsTransportInternalImpl::ice_transport() { return ice_transport_->internal(); } @@ -759,7 +774,7 @@ [&](auto stun_message_type) { std::optional<absl::string_view> data; std::optional<std::vector<uint32_t>> ack; - if (dtls_in_stun_) { + if (dtls_in_stun_configured_) { data = dtls_stun_piggyback_controller_.GetDataToPiggyback( stun_message_type); ack = dtls_stun_piggyback_controller_.GetAckToPiggyback( @@ -769,7 +784,7 @@ }, [&](std::optional<std::span<uint8_t>> data, std::optional<std::vector<uint32_t>> acks) { - if (!dtls_in_stun_) { + if (!dtls_in_stun_configured_) { return; } dtls_stun_piggyback_controller_.ReportDataPiggybacked(data, acks); @@ -828,14 +843,14 @@ break; case DtlsTransportState::kConnected: // Note: SignalWritableState fired by set_writable. - if (dtls_in_stun_ && dtls_ && first_ice_writable) { + if (dtls_in_stun_configured_ && dtls_ && first_ice_writable) { UpdateHandshakeTimeout(); FlushPendingDtlsPacket(); } set_writable(ice_transport()->writable()); break; case DtlsTransportState::kConnecting: - if (dtls_in_stun_ && dtls_) { + if (dtls_in_stun_configured_ && dtls_) { UpdateHandshakeTimeout(); FlushPendingDtlsPacket(); } @@ -906,6 +921,11 @@ // the peer has chosen the client role, and proceed with the handshake. // The fingerprint will be verified when it's set. if (!dtls_ && local_certificate_) { + if (piggybacked) { + // A ClientHello arriving before the answer has been applied changes + // the state here, the ice-option has set it in all other cases. + dtls_in_stun_negotiated_ = true; + } SetDtlsRole(SSL_SERVER); SetupDtls(); } @@ -1048,7 +1068,8 @@ void DtlsTransportInternalImpl::MaybeStartDtls() { // When adding the DTLS handshake in STUN we want to call StartSSL even // before the ICE transport is ready. - if (dtls_ && (ice_transport()->writable() || dtls_in_stun_)) { + if (dtls_ && (ice_transport()->writable() || + (dtls_in_stun_configured_ && dtls_in_stun_negotiated_))) { ConfigureHandshakeTimeout(); RTC_LOG(LS_INFO) @@ -1116,7 +1137,7 @@ if (writable && !ice_has_been_writable_) { // Wait with reporting writable until ICE has become writable once, // so as to not confuse other part of stack (such as sctp). - RTC_DCHECK(dtls_in_stun_); + RTC_DCHECK(dtls_in_stun_configured_); RTC_LOG(LS_INFO) << ToString() << ": defer set_writable(true) until ICE has become writable once"; @@ -1204,7 +1225,7 @@ RTC_LOG(LS_INFO) << ToString() << ": configuring DTLS handshake timeout " << initial_timeout_ms << "ms based on ICE RTT " << *rtt_ms; dtls_->SetInitialRetransmissionTimeout(initial_timeout_ms); - } else if (dtls_in_stun_) { + } else if (dtls_in_stun_configured_ && dtls_in_stun_negotiated_) { // Configure a very high timeout to effectively disable the DTLS timeout // and avoid fragmented resends. This is ok since DTLS-in-STUN caches // the handshake pacets and resends them using the pacing of ICE. @@ -1249,16 +1270,17 @@ bool DtlsTransportInternalImpl::IsDtlsPiggybackSupportedByPeer() { RTC_DCHECK_RUN_ON(&thread_checker_); - return dtls_in_stun_ && (dtls_stun_piggyback_controller_.state() != - DtlsStunPiggybackController::State::OFF); + return dtls_in_stun_configured_ && (dtls_stun_piggyback_controller_.state() != + DtlsStunPiggybackController::State::OFF); } bool DtlsTransportInternalImpl::WasDtlsCompletedByPiggybacking() { RTC_DCHECK_RUN_ON(&thread_checker_); - return dtls_in_stun_ && (dtls_stun_piggyback_controller_.state() == - DtlsStunPiggybackController::State::COMPLETE || - dtls_stun_piggyback_controller_.state() == - DtlsStunPiggybackController::State::PENDING); + return dtls_in_stun_configured_ && + (dtls_stun_piggyback_controller_.state() == + DtlsStunPiggybackController::State::COMPLETE || + dtls_stun_piggyback_controller_.state() == + DtlsStunPiggybackController::State::PENDING); } void DtlsTransportInternalImpl::FlushPendingDtlsPacket() { @@ -1270,7 +1292,7 @@ return; } - if (ice_transport()->writable() && dtls_in_stun_) { + if (ice_transport()->writable() && dtls_in_stun_configured_) { auto data_to_send = dtls_stun_piggyback_controller_.GetPending(); if (data_to_send.empty()) { // No data to send, we're done. @@ -1293,7 +1315,7 @@ } int DtlsTransportInternalImpl::GetStunDataCount() const { - if (!dtls_in_stun_) { + if (!dtls_in_stun_configured_) { return 0; } return dtls_stun_piggyback_controller_.GetCountOfReceivedData();
diff --git a/p2p/dtls/dtls_transport.h b/p2p/dtls/dtls_transport.h index 559648d..8cb47da 100644 --- a/p2p/dtls/dtls_transport.h +++ b/p2p/dtls/dtls_transport.h
@@ -159,7 +159,6 @@ SSLProtocolVersion max_version = SSL_PROTOCOL_DTLS_12, SslStreamFactory ssl_stream_factory = nullptr); - ~DtlsTransportInternalImpl() override; DtlsTransportInternalImpl(const DtlsTransportInternalImpl&) = delete; @@ -232,6 +231,8 @@ bool AppendSrtpKeyingMaterial( ZeroOnFreeBuffer<uint8_t>& keying_material) override; + void MaybeStartDtlsInStun() override; + IceTransportInternal* ice_transport() override; // For informational purposes. Tells if the DTLS handshake has finished. @@ -344,10 +345,10 @@ // of the stack. bool ice_has_been_writable_ = false; - // Initialized in constructor based on WebRTC-IceHandshakeDtls, - // (so that we return PIGGYBACK_ACK to client if we get STUN_BINDING_REQUEST - // directly). Maybe disabled in SetupDtls has been called. - bool dtls_in_stun_ = false; + // Whether SPED was configured locally. + bool dtls_in_stun_configured_ = false; + // Whether the remote negotiated or used SPED. + bool dtls_in_stun_negotiated_ = false; // Has DtlsInStun Complete been run? // This variable is used to prevent reinitializing after dtls-restart. bool dtls_in_stun_complete_ = false; @@ -366,5 +367,4 @@ } // namespace webrtc - #endif // P2P_DTLS_DTLS_TRANSPORT_H_
diff --git a/p2p/dtls/dtls_transport_internal.h b/p2p/dtls/dtls_transport_internal.h index 807936a..dbb2a9d 100644 --- a/p2p/dtls/dtls_transport_internal.h +++ b/p2p/dtls/dtls_transport_internal.h
@@ -100,6 +100,9 @@ return false; } + // Enable early DTLS start because DTLS-in-STUN was negotiated. + virtual void MaybeStartDtlsInStun() = 0; + // Set DTLS remote fingerprint and role. Must be after local identity set. virtual RTCError SetRemoteParameters(absl::string_view digest_alg, const uint8_t* digest,
diff --git a/p2p/dtls/dtls_transport_unittest.cc b/p2p/dtls/dtls_transport_unittest.cc index 8be89d5..77b3c76 100644 --- a/p2p/dtls/dtls_transport_unittest.cc +++ b/p2p/dtls/dtls_transport_unittest.cc
@@ -122,7 +122,8 @@ // Set up fake ICE transport and real DTLS transport under test. void SetupTransports(const Environment& env, IceRole role, - bool rtt_estimate = true) { + bool rtt_estimate = true, + bool dtls_in_stun = false) { dtls_transport_ = nullptr; ice_transport_ = nullptr; @@ -145,6 +146,11 @@ fake_ice_transport->SetAsyncDelay(async_delay_ms_); } fake_ice_transport->SetIceRole(role); + if (dtls_in_stun) { + auto ice_config = fake_ice_transport->config(); + ice_config.dtls_handshake_in_stun = true; + fake_ice_transport->SetIceConfig(ice_config); + } // Hook the raw packets so that we can verify they are encrypted. fake_ice_transport->RegisterReceivedPacketCallback( this, [&](PacketTransportInternal* transport, @@ -1112,15 +1118,18 @@ client2_.dtls_transport()->SetDtlsRole( config2.ssl_role.value_or(SSL_SERVER)); + // No SDP exchange in this fixture; stand in for JsepTransport's call. if (config1.dtls_in_stun) { auto config = client1_.fake_ice_transport()->config(); config.dtls_handshake_in_stun = true; client1_.fake_ice_transport()->SetIceConfig(config); + client1_.dtls_transport()->MaybeStartDtlsInStun(); } if (config2.dtls_in_stun) { auto config = client2_.fake_ice_transport()->config(); config.dtls_handshake_in_stun = true; client2_.fake_ice_transport()->SetIceConfig(config); + client2_.dtls_transport()->MaybeStartDtlsInStun(); } SetRemoteFingerprintFromCert(client1_.dtls_transport(), @@ -1617,6 +1626,97 @@ IsRtcOk()); } +TEST_F(DtlsTransportInternalImplTest, + NoEarlyDtlsInStunStartWithoutPeerSupport) { + if (!SSLStreamAdapter::IsBoringSsl()) { + GTEST_SKIP() << "DTLS-in-STUN requires BoringSSL."; + } + PrepareDtls(KT_DEFAULT); + + client1_.SetupTransports(env_, ICEROLE_CONTROLLING); + + auto ice_config = client1_.fake_ice_transport()->config(); + ice_config.dtls_handshake_in_stun = true; + client1_.fake_ice_transport()->SetIceConfig(ice_config); + ASSERT_FALSE(client1_.fake_ice_transport()->writable()); + + client1_.dtls_transport()->SetDtlsRole(SSL_SERVER); + SetRemoteFingerprintFromCert(client1_.dtls_transport(), + client2_.certificate()); + + // Without DTLS-in-STUN, DTLS starts after ICE becomes writable. + EXPECT_EQ(client1_.dtls_transport()->dtls_state(), DtlsTransportState::kNew); + + client1_.fake_ice_transport()->SetWritable(true); + EXPECT_EQ(client1_.dtls_transport()->dtls_state(), + DtlsTransportState::kConnecting); +} + +// MaybeStartDtlsInStun() confirms peer support; DTLS starts before ICE is +// writable. +TEST_F(DtlsTransportInternalImplTest, + EarlyDtlsInStunStartAfterPeerSupportConfirmed) { + if (!SSLStreamAdapter::IsBoringSsl()) { + GTEST_SKIP() << "DTLS-in-STUN requires BoringSSL."; + } + PrepareDtls(KT_DEFAULT); + + client1_.SetupTransports(env_, ICEROLE_CONTROLLING); + + auto ice_config = client1_.fake_ice_transport()->config(); + ice_config.dtls_handshake_in_stun = true; + client1_.fake_ice_transport()->SetIceConfig(ice_config); + ASSERT_FALSE(client1_.fake_ice_transport()->writable()); + + client1_.dtls_transport()->MaybeStartDtlsInStun(); + + client1_.dtls_transport()->SetDtlsRole(SSL_SERVER); + SetRemoteFingerprintFromCert(client1_.dtls_transport(), + client2_.certificate()); + + // With DTLS-in-STUN, DTLS starts immediately. + EXPECT_EQ(client1_.dtls_transport()->dtls_state(), + DtlsTransportState::kConnecting); +} + +// A piggybacked ClientHello arriving before the answer has been applied starts +// DTLS even though the ice-option has not been negotiated. +TEST_F(DtlsTransportInternalImplTest, EarlyDtlsInStunStartOnPiggybackedHello) { + if (!SSLStreamAdapter::IsBoringSsl()) { + GTEST_SKIP() << "DTLS-in-STUN requires BoringSSL."; + } + PrepareDtls(KT_DEFAULT); + + // Deliver packets synchronously. + client1_.set_async_delay(0); + client2_.set_async_delay(0); + + // The ice config must be set before the DTLS transport is created since that + // is when the piggybacking callbacks are registered. + client1_.SetupTransports(env_, ICEROLE_CONTROLLING, /*rtt_estimate=*/true, + /*dtls_in_stun=*/true); + client2_.SetupTransports(env_, ICEROLE_CONTROLLED, /*rtt_estimate=*/true, + /*dtls_in_stun=*/true); + + // client2_ applied the answer and starts the handshake as the DTLS client. + // client1_ has not applied it so it knows neither the remote fingerprint nor + // that SPED was negotiated. + client2_.dtls_transport()->MaybeStartDtlsInStun(); + client2_.dtls_transport()->SetDtlsRole(SSL_CLIENT); + SetRemoteFingerprintFromCert(client2_.dtls_transport(), + client1_.certificate()); + ASSERT_EQ(client2_.dtls_transport()->dtls_state(), + DtlsTransportState::kConnecting); + ASSERT_EQ(client1_.dtls_transport()->dtls_state(), DtlsTransportState::kNew); + + ASSERT_TRUE(client2_.ConnectIceTransport(&client1_)); + ASSERT_TRUE(client2_.SendIcePing()); + ASSERT_FALSE(client1_.fake_ice_transport()->writable()); + + EXPECT_EQ(client1_.dtls_transport()->dtls_state(), + DtlsTransportState::kConnecting); +} + // Test that packets are retransmitted according to the expected schedule. // Each time a timeout occurs, the retransmission timer should be doubled up to // 60 seconds. The timer defaults to 1 second, but for WebRTC we should be
diff --git a/p2p/dtls/fake_dtls_transport.h b/p2p/dtls/fake_dtls_transport.h index f1eb613..9d9fc01 100644 --- a/p2p/dtls/fake_dtls_transport.h +++ b/p2p/dtls/fake_dtls_transport.h
@@ -111,6 +111,8 @@ ice_transport_->DeregisterReceivedPacketCallback(this); } + void MaybeStartDtlsInStun() override {} + // Get inner fake ICE transport. FakeIceTransportInternal* fake_ice_transport() { return ice_transport_; }
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 18899b2..b84da1b 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc
@@ -475,6 +475,21 @@ } } + // Negotiating either option lets DTLS start before ICE is writable without + // waiting for a piggybacked packet, since an answer only echoes an option + // that the peer supports. + bool dtls_in_stun_negotiated = + (local_description_->transport_desc.HasOption(ICE_OPTION_SPED) && + remote_description_->transport_desc.HasOption(ICE_OPTION_SPED)) || + (local_description_->transport_desc.HasOption(ICE_OPTION_GOOG_SPED_V1) && + remote_description_->transport_desc.HasOption(ICE_OPTION_GOOG_SPED_V1)); + if (dtls_in_stun_negotiated) { + rtp_dtls_transport()->MaybeStartDtlsInStun(); + if (rtcp_dtls_transport()) { + rtcp_dtls_transport()->MaybeStartDtlsInStun(); + } + } + RTCError error = SetNegotiatedDtlsParameters( rtp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); if (!error.ok()) {
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index a7b2b12..eb8a1ed 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc
@@ -15,6 +15,7 @@ // do NOT add it here, but instead add it to the file // slow_peer_connection_integrationtest.cc +#include <algorithm> #include <cstddef> #include <cstdint> #include <memory> @@ -5098,6 +5099,50 @@ EXPECT_EQ(caller()->dtls_transport_information().ssl_group_id(), expected); } +TEST_P(PeerConnectionIntegrationTest, + SpedWireTriggerStartsDtlsWithoutRemoteDescription) { + if (!SSLStreamAdapter::IsBoringSsl()) { + GTEST_SKIP() << "DTLS-in-STUN requires BoringSSL."; + } + SetFieldTrials("WebRTC-IceHandshakeDtls/Enabled/"); + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + // Suppress SetRemoteDescription and ICE candidates from callee. + caller()->SetReceivedSdpMunger( + [](std::unique_ptr<SessionDescriptionInterface>& desc) { desc.reset(); }); + callee()->set_signal_ice_candidates(false); + + caller()->CreateDataChannel(); + caller()->CreateAndSetAndSignalOffer(); + + ASSERT_THAT( + WaitUntil( + [&] { + const auto& history = caller()->peer_connection_state_history(); + return std::find(history.begin(), history.end(), + PeerConnectionInterface::PeerConnectionState:: + kConnecting) != history.end(); + }, + IsTrue()), + IsRtcOk()); +} + +TEST_P(PeerConnectionIntegrationTest, NoEarlyDtlsStartWhenSpedNotInAnswer) { + if (!SSLStreamAdapter::IsBoringSsl()) { + GTEST_SKIP() << "DTLS-in-STUN requires BoringSSL."; + } + SetFieldTrials(kCallerName, "WebRTC-IceHandshakeDtls/Enabled/"); + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignalingForSdpOnly(); + caller()->CreateDataChannel(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_THAT(WaitUntil([&] { return SignalingStateStable(); }, IsTrue()), + IsRtcOk()); + EXPECT_THAT(caller()->peer_connection_state_history(), + ::testing::Not(::testing::Contains( + PeerConnectionInterface::PeerConnectionState::kConnecting))); +} + #endif // WEBRTC_HAVE_SCTP TEST_P(PeerConnectionIntegrationTest, PerPeerConnectionHeaderExtensions) {