Add an ice-option for goog-sped-v1 The SPED draft is currently not compatible with the libwebrtc c++ library. This patch adds a ice-option to SDP so that we can manage potential changes going forward. Bug: webrtc:367395350 Change-Id: I0b513be5d3b9d772a462792fd6251af43ce6cd13 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/468160 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Jonas Oreland <jonaso@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47582}
diff --git a/p2p/base/transport_description.h b/p2p/base/transport_description.h index df04f63..d5592ef 100644 --- a/p2p/base/transport_description.h +++ b/p2p/base/transport_description.h
@@ -96,6 +96,7 @@ constexpr auto* ICE_OPTION_TRICKLE = "trickle"; constexpr auto* ICE_OPTION_RENOMINATION = "renomination"; +constexpr auto* ICE_OPTION_GOOG_SPED_V1 = "goog-sped-v1"; std::optional<ConnectionRole> StringToConnectionRole( absl::string_view role_str);
diff --git a/p2p/base/transport_description_factory.cc b/p2p/base/transport_description_factory.cc index 7b8663e..3bf59ea 100644 --- a/p2p/base/transport_description_factory.cc +++ b/p2p/base/transport_description_factory.cc
@@ -48,6 +48,12 @@ desc->AddOption(ICE_OPTION_RENOMINATION); } + if (field_trials_.IsEnabled("WebRTC-IceHandshakeDtls") && + (!current_description || + current_description->HasOption(ICE_OPTION_GOOG_SPED_V1))) { + desc->AddOption(ICE_OPTION_GOOG_SPED_V1); + } + // If we are not trying to establish a secure transport, don't add a // fingerprint. if (insecure_ && !certificate_) { @@ -90,6 +96,13 @@ if (options.enable_ice_renomination) { desc->AddOption(ICE_OPTION_RENOMINATION); } + if (field_trials_.IsEnabled("WebRTC-IceHandshakeDtls") && + offer->HasOption(ICE_OPTION_GOOG_SPED_V1) && + (current_description == nullptr || + current_description->HasOption(ICE_OPTION_GOOG_SPED_V1))) { + desc->AddOption(ICE_OPTION_GOOG_SPED_V1); + } + // Special affordance for testing: Answer without DTLS params // if we are insecure without a certificate, or if we are // insecure with a non-DTLS offer.
diff --git a/p2p/base/transport_description_factory_unittest.cc b/p2p/base/transport_description_factory_unittest.cc index 37b0c53..a46d737 100644 --- a/p2p/base/transport_description_factory_unittest.cc +++ b/p2p/base/transport_description_factory_unittest.cc
@@ -342,6 +342,118 @@ EXPECT_TRUE(answer->HasOption("trickle")); } +TEST_F(TransportDescriptionFactoryTest, AddsGoogSpedV1OptionWhenEnabled) { + FieldTrials field_trials = + CreateTestFieldTrials("WebRTC-IceHandshakeDtls/Enabled/"); + TransportDescriptionFactory f1(field_trials); + TransportDescriptionFactory f2(field_trials); + f1.set_certificate(cert1_); + f2.set_certificate(cert2_); + + webrtc::TransportOptions options; + std::unique_ptr<TransportDescription> offer = + f1.CreateOffer(options, nullptr, &ice_credentials_); + ASSERT_THAT(offer, NotNull()); + EXPECT_TRUE(offer->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + std::unique_ptr<TransportDescription> answer = + f2.CreateAnswer(offer.get(), options, true, nullptr, &ice_credentials_); + ASSERT_THAT(answer, NotNull()); + EXPECT_TRUE(answer->HasOption(ICE_OPTION_GOOG_SPED_V1)); +} + +TEST_F(TransportDescriptionFactoryTest, + DoesNotAddGoogSpedV1OptionWhenDisabled) { + webrtc::TransportOptions options; + std::unique_ptr<TransportDescription> offer = + f1_.CreateOffer(options, nullptr, &ice_credentials_); + ASSERT_THAT(offer, NotNull()); + EXPECT_FALSE(offer->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + std::unique_ptr<TransportDescription> answer = + f2_.CreateAnswer(offer.get(), options, true, nullptr, &ice_credentials_); + ASSERT_THAT(answer, NotNull()); + EXPECT_FALSE(answer->HasOption(ICE_OPTION_GOOG_SPED_V1)); +} + +TEST_F(TransportDescriptionFactoryTest, + DoesNotAddGoogSpedV1OptionToAnswerIfMissingInOffer) { + FieldTrials field_trials = + CreateTestFieldTrials("WebRTC-IceHandshakeDtls/Enabled/"); + TransportDescriptionFactory f2(field_trials); // Enabled for answer. + f2.set_certificate(cert2_); + + FieldTrials disabled_trials = CreateTestFieldTrials(); // Disabled for offer. + TransportDescriptionFactory f1_disabled(disabled_trials); + f1_disabled.set_certificate(cert1_); + + webrtc::TransportOptions options; + std::unique_ptr<TransportDescription> offer = + f1_disabled.CreateOffer(options, nullptr, &ice_credentials_); + ASSERT_THAT(offer, NotNull()); + EXPECT_FALSE(offer->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + std::unique_ptr<TransportDescription> answer = + f2.CreateAnswer(offer.get(), options, true, nullptr, &ice_credentials_); + ASSERT_THAT(answer, NotNull()); + EXPECT_FALSE(answer->HasOption(ICE_OPTION_GOOG_SPED_V1)); +} + +TEST_F(TransportDescriptionFactoryTest, + DoesNotAddGoogSpedV1OptionToOfferDuringRenegotiationIfMissingInCurrent) { + FieldTrials field_trials = + CreateTestFieldTrials("WebRTC-IceHandshakeDtls/Enabled/"); + TransportDescriptionFactory f1(field_trials); + f1.set_certificate(cert1_); + + webrtc::TransportOptions options; + + FieldTrials disabled_trials = CreateTestFieldTrials(); + TransportDescriptionFactory f1_disabled(disabled_trials); + f1_disabled.set_certificate(cert1_); + std::unique_ptr<TransportDescription> current_desc = + f1_disabled.CreateOffer(options, nullptr, &ice_credentials_); + ASSERT_THAT(current_desc, NotNull()); + ASSERT_FALSE(current_desc->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + std::unique_ptr<TransportDescription> new_offer = + f1.CreateOffer(options, current_desc.get(), &ice_credentials_); + ASSERT_THAT(new_offer, NotNull()); + EXPECT_FALSE(new_offer->HasOption(ICE_OPTION_GOOG_SPED_V1)); +} + +TEST_F( + TransportDescriptionFactoryTest, + DoesNotAddGoogSpedV1OptionToAnswerDuringRenegotiationIfMissingInCurrent) { + FieldTrials field_trials = + CreateTestFieldTrials("WebRTC-IceHandshakeDtls/Enabled/"); + TransportDescriptionFactory f1(field_trials); + TransportDescriptionFactory f2(field_trials); + f1.set_certificate(cert1_); + f2.set_certificate(cert2_); + + webrtc::TransportOptions options; + + std::unique_ptr<TransportDescription> offer = + f1.CreateOffer(options, nullptr, &ice_credentials_); + ASSERT_THAT(offer, NotNull()); + ASSERT_TRUE(offer->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + FieldTrials disabled_trials = CreateTestFieldTrials(); + TransportDescriptionFactory f2_disabled(disabled_trials); + f2_disabled.set_certificate(cert2_); + std::unique_ptr<TransportDescription> current_answer = + f2_disabled.CreateAnswer(offer.get(), options, true, nullptr, + &ice_credentials_); + ASSERT_THAT(current_answer, NotNull()); + ASSERT_FALSE(current_answer->HasOption(ICE_OPTION_GOOG_SPED_V1)); + + std::unique_ptr<TransportDescription> new_answer = f2.CreateAnswer( + offer.get(), options, true, current_answer.get(), &ice_credentials_); + ASSERT_THAT(new_answer, NotNull()); + EXPECT_FALSE(new_answer->HasOption(ICE_OPTION_GOOG_SPED_V1)); +} + // Test CreateOffer with IceCredentialsIterator. TEST_F(TransportDescriptionFactoryTest, CreateOfferIceCredentialsIterator) { std::vector<webrtc::IceParameters> credentials = {
diff --git a/p2p/dtls/dtls_ice_integration_fixture.h b/p2p/dtls/dtls_ice_integration_fixture.h index de16c0a..edc64bf 100644 --- a/p2p/dtls/dtls_ice_integration_fixture.h +++ b/p2p/dtls/dtls_ice_integration_fixture.h
@@ -537,6 +537,14 @@ ep.client ? "client_transport" : "server_transport", /* component= */ 0, std::move(init)); ep.ice_transport = make_ref_counted<FakeIceTransport>(std::move(channel)); + + // Enable(or disable) the dtls_in_stun parameter before + // DTLS is negotiated. + IceConfig config; + config.continual_gathering_policy = GATHER_CONTINUALLY; + config.dtls_handshake_in_stun = ep.config.dtls_in_stun; + ep.ice()->SetIceConfig(config); + // Is peer using ice-lite. if (ep.config.ice_lite && ep.config.ice_role == ICEROLE_CONTROLLING) { ep.ice()->SetRemoteIceMode(ICEMODE_LITE); @@ -555,13 +563,6 @@ ep.dtls->SetFakeIceLite(); } - // Enable(or disable) the dtls_in_stun parameter before - // DTLS is negotiated. - IceConfig config; - config.continual_gathering_policy = GATHER_CONTINUALLY; - config.dtls_handshake_in_stun = ep.config.dtls_in_stun; - ep.ice()->SetIceConfig(config); - // Setup ICE. ep.ice()->SetIceParameters(ep.client ? client_ice_parameters_ : server_ice_parameters_);
diff --git a/p2p/dtls/dtls_transport.cc b/p2p/dtls/dtls_transport.cc index 36e9469..456dead 100644 --- a/p2p/dtls/dtls_transport.cc +++ b/p2p/dtls/dtls_transport.cc
@@ -271,7 +271,7 @@ [this](bool success) { CompleteDtlsInStun(success); }) { RTC_DCHECK(ice_transport_); ConnectToIceTransport(); - dtls_in_stun_ = env_.field_trials().IsEnabled("WebRTC-IceHandshakeDtls"); + dtls_in_stun_ = ice_transport_->internal()->config().dtls_handshake_in_stun; } DtlsTransportInternalImpl::DtlsTransportInternalImpl(
diff --git a/pc/data_channel_integrationtest.cc b/pc/data_channel_integrationtest.cc index be2f2e1..7f1e19c7 100644 --- a/pc/data_channel_integrationtest.cc +++ b/pc/data_channel_integrationtest.cc
@@ -1857,6 +1857,72 @@ ValuesIn(kTrialsVariants), ValuesIn(kTrialsVariants))); +struct SpedV1TestConfig { + bool caller_enabled; + bool callee_enabled; +}; + +class SdpNegotiationGoogSpedV1Test + : public DataChannelIntegrationTestUnifiedPlan, + public ::testing::WithParamInterface<SpedV1TestConfig> {}; + +TEST_P(SdpNegotiationGoogSpedV1Test, VerifySdp) { + const auto& param = GetParam(); + SetFieldTrials( + "Caller", param.caller_enabled ? "WebRTC-IceHandshakeDtls/Enabled/" : ""); + SetFieldTrials( + "Callee", param.callee_enabled ? "WebRTC-IceHandshakeDtls/Enabled/" : ""); + + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->CreateDataChannel(); + + std::unique_ptr<SessionDescriptionInterface> offer; + caller()->SetGeneratedSdpMunger( + [&](std::unique_ptr<SessionDescriptionInterface>& sdp) { + offer = sdp->Clone(); + }); + + std::unique_ptr<SessionDescriptionInterface> answer; + callee()->SetGeneratedSdpMunger( + [&](std::unique_ptr<SessionDescriptionInterface>& sdp) { + answer = sdp->Clone(); + }); + + caller()->CreateAndSetAndSignalOffer(); + + ASSERT_THAT(offer, NotNull()); + std::string offer_sdp; + offer->ToString(&offer_sdp); + + if (param.caller_enabled) { + EXPECT_THAT(offer_sdp, testing::HasSubstr(ICE_OPTION_GOOG_SPED_V1)); + } else { + EXPECT_THAT(offer_sdp, + testing::Not(testing::HasSubstr(ICE_OPTION_GOOG_SPED_V1))); + } + + ASSERT_THAT(WaitUntil([&] { return answer.get() != nullptr; }, IsTrue()), + IsRtcOk()); + + std::string answer_sdp; + answer->ToString(&answer_sdp); + + if (param.callee_enabled && param.caller_enabled) { + EXPECT_THAT(answer_sdp, testing::HasSubstr(ICE_OPTION_GOOG_SPED_V1)); + } else { + EXPECT_THAT(answer_sdp, + testing::Not(testing::HasSubstr(ICE_OPTION_GOOG_SPED_V1))); + } +} + +INSTANTIATE_TEST_SUITE_P(SdpNegotiationGoogSpedV1Test, + SdpNegotiationGoogSpedV1Test, + testing::Values(SpedV1TestConfig{false, false}, + SpedV1TestConfig{false, true}, + SpedV1TestConfig{true, false}, + SpedV1TestConfig{true, true})); + TEST_P(DataChannelIntegrationTestUnifiedPlanFieldTrials, DtlsRestartOneCalleAtATime) { if (auto msg = CheckSupported()) {
diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 1ac63951..4681431 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc
@@ -439,6 +439,7 @@ // between future SetRemote/SetLocal invocations and new transport // creation, we have the negotiation state saved until a new // negotiation happens. + RTC_DCHECK(rtp_dtls_transport()); RTCError error = SetNegotiatedDtlsParameters( rtp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); @@ -450,6 +451,20 @@ error = SetNegotiatedDtlsParameters( rtcp_dtls_transport(), negotiated_dtls_role, remote_fingerprint.get()); } + + bool dtls_in_stun = + local_description_->transport_desc.HasOption(ICE_OPTION_GOOG_SPED_V1); + + IceConfig config = rtp_dtls_transport()->ice_transport()->config(); + config.dtls_handshake_in_stun = dtls_in_stun; + rtp_dtls_transport()->ice_transport()->SetIceConfig(config); + + if (rtcp_dtls_transport()) { + IceConfig rtcp_config = rtcp_dtls_transport()->ice_transport()->config(); + rtcp_config.dtls_handshake_in_stun = dtls_in_stun; + rtcp_dtls_transport()->ice_transport()->SetIceConfig(rtcp_config); + } + return error; }
diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index b72267d..a154b97 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc
@@ -342,6 +342,76 @@ } } +TEST_P(JsepTransport2WithRtcpMux, SetGoogSpedV1OptionUpdatesIceConfig) { + bool rtcp_mux_enabled = GetParam(); + jsep_transport_ = CreateJsepTransport2(rtcp_mux_enabled); + + scoped_refptr<RTCCertificate> local_cert = + RTCCertificate::Create(SSLIdentity::Create("local", KT_DEFAULT)); + scoped_refptr<RTCCertificate> remote_cert = + RTCCertificate::Create(SSLIdentity::Create("remote", KT_DEFAULT)); + jsep_transport_->SetLocalCertificate(local_cert); + + JsepTransportDescription local_description = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag1, kIcePwd1, + local_cert, CONNECTIONROLE_ACTPASS); + local_description.transport_desc.AddOption(ICE_OPTION_GOOG_SPED_V1); + + ASSERT_TRUE( + jsep_transport_ + ->SetLocalJsepTransportDescription(local_description, SdpType::kOffer) + .ok()); + + JsepTransportDescription remote_description = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag2, kIcePwd2, + remote_cert, CONNECTIONROLE_ACTIVE); + + ASSERT_TRUE(jsep_transport_ + ->SetRemoteJsepTransportDescription(remote_description, + SdpType::kAnswer) + .ok()); + + auto fake_ice_transport = static_cast<FakeIceTransportInternal*>( + jsep_transport_->rtp_dtls_transport()->ice_transport()); + + EXPECT_TRUE(fake_ice_transport->config().dtls_handshake_in_stun); +} + +TEST_P(JsepTransport2WithRtcpMux, + MissingGoogSpedV1OptionDoesNotUpdateIceConfig) { + bool rtcp_mux_enabled = GetParam(); + jsep_transport_ = CreateJsepTransport2(rtcp_mux_enabled); + + scoped_refptr<RTCCertificate> local_cert = + RTCCertificate::Create(SSLIdentity::Create("local", KT_DEFAULT)); + scoped_refptr<RTCCertificate> remote_cert = + RTCCertificate::Create(SSLIdentity::Create("remote", KT_DEFAULT)); + jsep_transport_->SetLocalCertificate(local_cert); + + JsepTransportDescription local_description = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag1, kIcePwd1, + local_cert, CONNECTIONROLE_ACTPASS); + + ASSERT_TRUE( + jsep_transport_ + ->SetLocalJsepTransportDescription(local_description, SdpType::kOffer) + .ok()); + + JsepTransportDescription remote_description = + MakeJsepTransportDescription(rtcp_mux_enabled, kIceUfrag2, kIcePwd2, + remote_cert, CONNECTIONROLE_ACTIVE); + + ASSERT_TRUE(jsep_transport_ + ->SetRemoteJsepTransportDescription(remote_description, + SdpType::kAnswer) + .ok()); + + auto fake_ice_transport = static_cast<FakeIceTransportInternal*>( + jsep_transport_->rtp_dtls_transport()->ice_transport()); + + EXPECT_FALSE(fake_ice_transport->config().dtls_handshake_in_stun); +} + // Tests SetNeedsIceRestartFlag and need_ice_restart, ensuring needs_ice_restart // only starts returning "false" once an ICE restart has been initiated. TEST_P(JsepTransport2WithRtcpMux, NeedsIceRestart) {