From 52aea5d3f3a9c706c7cb4d3b7fd3d6133252bd76 Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Tue, 3 Mar 2020 13:21:30 +0100 Subject: [PATCH] Unbreak ICE renomination This patch fixes a problem in https://webrtc.googlesource.com/src/+/71ff07369837d6575c04ebff7002d07d6e0af25f that when adding standard compliance validation of ufrag/pwd accidentally broken ice renomination by introducing a new "constructor". Bug: chromium:1044521 Change-Id: If1b18b1d728e55db9da385b37162a9cb5e61ac48 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/169549 Commit-Queue: Jonas Oreland Reviewed-by: Magnus Flodman Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#30670} --- p2p/base/fake_ice_transport.h | 28 +++++++++++++----------- p2p/base/transport_description.cc | 36 ++++++++++++++++++------------- p2p/base/transport_description.h | 6 +++++- pc/jsep_transport.cc | 18 +++++++--------- pc/jsep_transport_unittest.cc | 34 +++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 38 deletions(-) diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h index d0fa1ea8cc..edc5730440 100644 --- a/p2p/base/fake_ice_transport.h +++ b/p2p/base/fake_ice_transport.h @@ -112,10 +112,18 @@ class FakeIceTransport : public IceTransportInternal { int component() const override { return component_; } uint64_t IceTiebreaker() const { return tiebreaker_; } IceMode remote_ice_mode() const { return remote_ice_mode_; } - const std::string& ice_ufrag() const { return ice_ufrag_; } - const std::string& ice_pwd() const { return ice_pwd_; } - const std::string& remote_ice_ufrag() const { return remote_ice_ufrag_; } - const std::string& remote_ice_pwd() const { return remote_ice_pwd_; } + const std::string& ice_ufrag() const { return ice_parameters_.ufrag; } + const std::string& ice_pwd() const { return ice_parameters_.pwd; } + const std::string& remote_ice_ufrag() const { + return remote_ice_parameters_.ufrag; + } + const std::string& remote_ice_pwd() const { + return remote_ice_parameters_.pwd; + } + const IceParameters& ice_parameters() const { return ice_parameters_; } + const IceParameters& remote_ice_parameters() const { + return remote_ice_parameters_; + } IceTransportState GetState() const override { if (legacy_transport_state_) { @@ -157,12 +165,10 @@ class FakeIceTransport : public IceTransportInternal { tiebreaker_ = tiebreaker; } void SetIceParameters(const IceParameters& ice_params) override { - ice_ufrag_ = ice_params.ufrag; - ice_pwd_ = ice_params.pwd; + ice_parameters_ = ice_params; } void SetRemoteIceParameters(const IceParameters& params) override { - remote_ice_ufrag_ = params.ufrag; - remote_ice_pwd_ = params.pwd; + remote_ice_parameters_ = params; } void SetRemoteIceMode(IceMode mode) override { remote_ice_mode_ = mode; } @@ -312,10 +318,8 @@ class FakeIceTransport : public IceTransportInternal { IceConfig ice_config_; IceRole role_ = ICEROLE_UNKNOWN; uint64_t tiebreaker_ = 0; - std::string ice_ufrag_; - std::string ice_pwd_; - std::string remote_ice_ufrag_; - std::string remote_ice_pwd_; + IceParameters ice_parameters_; + IceParameters remote_ice_parameters_; IceMode remote_ice_mode_ = ICEMODE_FULL; size_t connection_count_ = 0; absl::optional transport_state_; diff --git a/p2p/base/transport_description.cc b/p2p/base/transport_description.cc index 5491d44fda..729b4ae8c3 100644 --- a/p2p/base/transport_description.cc +++ b/p2p/base/transport_description.cc @@ -38,7 +38,7 @@ bool IsIceChar(char c) { return absl::ascii_isalnum(c) || c == '+' || c == '/'; } -RTCErrorOr ParseIceUfrag(absl::string_view raw_ufrag) { +RTCError ValidateIceUfrag(absl::string_view raw_ufrag) { if (!(ICE_UFRAG_MIN_LENGTH <= raw_ufrag.size() && raw_ufrag.size() <= ICE_UFRAG_MAX_LENGTH)) { rtc::StringBuilder sb; @@ -53,10 +53,10 @@ RTCErrorOr ParseIceUfrag(absl::string_view raw_ufrag) { "ICE ufrag must contain only alphanumeric characters, '+', and '/'."); } - return std::string(raw_ufrag); + return RTCError::OK(); } -RTCErrorOr ParseIcePwd(absl::string_view raw_pwd) { +RTCError ValidateIcePwd(absl::string_view raw_pwd) { if (!(ICE_PWD_MIN_LENGTH <= raw_pwd.size() && raw_pwd.size() <= ICE_PWD_MAX_LENGTH)) { rtc::StringBuilder sb; @@ -71,35 +71,41 @@ RTCErrorOr ParseIcePwd(absl::string_view raw_pwd) { "ICE pwd must contain only alphanumeric characters, '+', and '/'."); } - return std::string(raw_pwd); + return RTCError::OK(); } } // namespace -// static RTCErrorOr IceParameters::Parse(absl::string_view raw_ufrag, absl::string_view raw_pwd) { + IceParameters parameters(std::string(raw_ufrag), std::string(raw_pwd), + /* renomination= */ false); + auto result = parameters.Validate(); + if (!result.ok()) { + return result; + } + return parameters; +} + +RTCError IceParameters::Validate() const { // For legacy protocols. // TODO(zhihuang): Remove this once the legacy protocol is no longer // supported. - if (raw_ufrag.empty() && raw_pwd.empty()) { - return IceParameters(); + if (ufrag.empty() && pwd.empty()) { + return RTCError::OK(); } - auto ufrag_result = ParseIceUfrag(raw_ufrag); + auto ufrag_result = ValidateIceUfrag(ufrag); if (!ufrag_result.ok()) { - return ufrag_result.MoveError(); + return ufrag_result; } - auto pwd_result = ParseIcePwd(raw_pwd); + auto pwd_result = ValidateIcePwd(pwd); if (!pwd_result.ok()) { - return pwd_result.MoveError(); + return pwd_result; } - IceParameters parameters; - parameters.ufrag = ufrag_result.MoveValue(); - parameters.pwd = pwd_result.MoveValue(); - return parameters; + return RTCError::OK(); } bool StringToConnectionRole(const std::string& role_str, ConnectionRole* role) { diff --git a/p2p/base/transport_description.h b/p2p/base/transport_description.h index d7eedf15ef..1a458c9571 100644 --- a/p2p/base/transport_description.h +++ b/p2p/base/transport_description.h @@ -83,6 +83,10 @@ struct IceParameters { bool operator!=(const IceParameters& other) const { return !(*this == other); } + + // Validate IceParameters, returns a SyntaxError if the ufrag or pwd are + // malformed. + webrtc::RTCError Validate() const; }; extern const char CONNECTIONROLE_ACTIVE_STR[]; @@ -142,7 +146,7 @@ struct TransportDescription { } bool secure() const { return identity_fingerprint != nullptr; } - IceParameters GetIceParameters() { + IceParameters GetIceParameters() const { return IceParameters(ice_ufrag, ice_pwd, HasOption(ICE_OPTION_RENOMINATION)); } diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index bc380402b1..5bf74f1e87 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -178,16 +178,15 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( RTC_DCHECK_RUN_ON(network_thread_); - webrtc::RTCErrorOr ice_parameters_result = - IceParameters::Parse(jsep_description.transport_desc.ice_ufrag, - jsep_description.transport_desc.ice_pwd); + IceParameters ice_parameters = + jsep_description.transport_desc.GetIceParameters(); + webrtc::RTCError ice_parameters_result = ice_parameters.Validate(); if (!ice_parameters_result.ok()) { rtc::StringBuilder sb; - sb << "Invalid ICE parameters: " << ice_parameters_result.error().message(); + sb << "Invalid ICE parameters: " << ice_parameters_result.message(); return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, sb.Release()); } - IceParameters ice_parameters = ice_parameters_result.MoveValue(); if (!SetRtcpMux(jsep_description.rtcp_mux_enabled, type, ContentSource::CS_LOCAL)) { @@ -273,17 +272,16 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( RTC_DCHECK_RUN_ON(network_thread_); - webrtc::RTCErrorOr ice_parameters_result = - IceParameters::Parse(jsep_description.transport_desc.ice_ufrag, - jsep_description.transport_desc.ice_pwd); + IceParameters ice_parameters = + jsep_description.transport_desc.GetIceParameters(); + webrtc::RTCError ice_parameters_result = ice_parameters.Validate(); if (!ice_parameters_result.ok()) { remote_description_.reset(); rtc::StringBuilder sb; - sb << "Invalid ICE parameters: " << ice_parameters_result.error().message(); + sb << "Invalid ICE parameters: " << ice_parameters_result.message(); return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, sb.Release()); } - IceParameters ice_parameters = ice_parameters_result.MoveValue(); if (!SetRtcpMux(jsep_description.rtcp_mux_enabled, type, ContentSource::CS_REMOTE)) { diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index c4193e5974..ccaf01b9a4 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc @@ -1256,5 +1256,39 @@ INSTANTIATE_TEST_SUITE_P( std::make_tuple(Scenario::kDtlsBeforeCallerSendOffer, false), std::make_tuple(Scenario::kDtlsBeforeCallerSetAnswer, false), std::make_tuple(Scenario::kDtlsAfterCallerSetAnswer, false))); + +// This test verifies the ICE parameters are properly applied to the transports. +TEST_F(JsepTransport2Test, SetIceParametersWithRenomination) { + jsep_transport_ = + CreateJsepTransport2(/* rtcp_mux_enabled= */ true, SrtpMode::kDtlsSrtp); + + JsepTransportDescription jsep_description; + jsep_description.transport_desc = TransportDescription(kIceUfrag1, kIcePwd1); + jsep_description.transport_desc.AddOption(ICE_OPTION_RENOMINATION); + ASSERT_TRUE( + jsep_transport_ + ->SetLocalJsepTransportDescription(jsep_description, SdpType::kOffer) + .ok()); + auto fake_ice_transport = static_cast( + jsep_transport_->rtp_dtls_transport()->ice_transport()); + EXPECT_EQ(ICEMODE_FULL, fake_ice_transport->remote_ice_mode()); + EXPECT_EQ(kIceUfrag1, fake_ice_transport->ice_ufrag()); + EXPECT_EQ(kIcePwd1, fake_ice_transport->ice_pwd()); + EXPECT_TRUE(fake_ice_transport->ice_parameters().renomination); + + jsep_description.transport_desc = TransportDescription(kIceUfrag2, kIcePwd2); + jsep_description.transport_desc.AddOption(ICE_OPTION_RENOMINATION); + ASSERT_TRUE(jsep_transport_ + ->SetRemoteJsepTransportDescription(jsep_description, + SdpType::kAnswer) + .ok()); + fake_ice_transport = static_cast( + jsep_transport_->rtp_dtls_transport()->ice_transport()); + EXPECT_EQ(ICEMODE_FULL, fake_ice_transport->remote_ice_mode()); + EXPECT_EQ(kIceUfrag2, fake_ice_transport->remote_ice_ufrag()); + EXPECT_EQ(kIcePwd2, fake_ice_transport->remote_ice_pwd()); + EXPECT_TRUE(fake_ice_transport->remote_ice_parameters().renomination); +} + } // namespace } // namespace cricket