From a54a0801121e05f797e514731cc5c9bad2f5e597 Mon Sep 17 00:00:00 2001 From: honghaiz Date: Wed, 16 Dec 2015 18:37:23 -0800 Subject: [PATCH] Add ufrag to the ICE candidate signaling. On the receiving side, if a candidate arrives with an old ufrag, it will be dropped. If it contains a new frag that has never seen before, it will hold the ufrag and create connections, although those connections are not pingable until the ICE credentials are received. This could avoid a bunch of ICE generation issues. BUG=webrtc:5138,webrt:5292 Review URL: https://codereview.webrtc.org/1498993002 Cr-Commit-Position: refs/heads/master@{#11060} --- talk/app/webrtc/webrtcsdp.cc | 30 +++-- talk/app/webrtc/webrtcsdp_unittest.cc | 16 ++- webrtc/p2p/base/p2ptransportchannel.cc | 111 ++++++++++-------- webrtc/p2p/base/p2ptransportchannel.h | 30 ++++- .../p2p/base/p2ptransportchannel_unittest.cc | 49 +++++++- 5 files changed, 170 insertions(+), 66 deletions(-) diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index bc5dc1d1da..60cc53966c 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -140,8 +140,8 @@ static const char kAttributeCandidate[] = "candidate"; static const char kAttributeCandidateTyp[] = "typ"; static const char kAttributeCandidateRaddr[] = "raddr"; static const char kAttributeCandidateRport[] = "rport"; -static const char kAttributeCandidateUsername[] = "username"; -static const char kAttributeCandidatePassword[] = "password"; +static const char kAttributeCandidateUfrag[] = "ufrag"; +static const char kAttributeCandidatePwd[] = "pwd"; static const char kAttributeCandidateGeneration[] = "generation"; static const char kAttributeFingerprint[] = "fingerprint"; static const char kAttributeSetup[] = "setup"; @@ -262,6 +262,7 @@ static void BuildRtpMap(const MediaContentDescription* media_desc, const MediaType media_type, std::string* message); static void BuildCandidate(const std::vector& candidates, + bool include_ufrag, std::string* message); static void BuildIceOptions(const std::vector& transport_options, std::string* message); @@ -878,7 +879,7 @@ std::string SdpSerializeCandidate( std::string message; std::vector candidates; candidates.push_back(candidate.candidate()); - BuildCandidate(candidates, &message); + BuildCandidate(candidates, true, &message); // From WebRTC draft section 4.8.1.1 candidate-attribute will be // just candidate: not a=candidate:CRLF ASSERT(message.find("a=") == 0); @@ -1072,10 +1073,9 @@ bool ParseCandidate(const std::string& message, Candidate* candidate, } // Extension - // Empty string as the candidate username and password. - // Will be updated later with the ice-ufrag and ice-pwd. - // TODO: Remove the username/password extension, which is currently - // kept for backwards compatibility. + // Though non-standard, we support the ICE ufrag and pwd being signaled on + // the candidate to avoid issues with confusing which generation a candidate + // belongs to when trickling multiple generations at the same time. std::string username; std::string password; uint32_t generation = 0; @@ -1086,9 +1086,9 @@ bool ParseCandidate(const std::string& message, Candidate* candidate, if (!GetValueFromString(first_line, fields[++i], &generation, error)) { return false; } - } else if (fields[i] == kAttributeCandidateUsername) { + } else if (fields[i] == kAttributeCandidateUfrag) { username = fields[++i]; - } else if (fields[i] == kAttributeCandidatePassword) { + } else if (fields[i] == kAttributeCandidatePwd) { password = fields[++i]; } else { // Skip the unknown extension. @@ -1285,8 +1285,9 @@ void BuildMediaDescription(const ContentInfo* content_info, } } - // Build the a=candidate lines. - BuildCandidate(candidates, message); + // Build the a=candidate lines. We don't include ufrag and pwd in the + // candidates in the SDP to avoid redundancy. + BuildCandidate(candidates, false, message); // Use the transport_info to build the media level ice-ufrag and ice-pwd. if (transport_info) { @@ -1717,6 +1718,7 @@ void BuildRtpMap(const MediaContentDescription* media_desc, } void BuildCandidate(const std::vector& candidates, + bool include_ufrag, std::string* message) { std::ostringstream os; @@ -1766,6 +1768,9 @@ void BuildCandidate(const std::vector& candidates, // Extensions os << kAttributeCandidateGeneration << " " << it->generation(); + if (include_ufrag && !it->username().empty()) { + os << " " << kAttributeCandidateUfrag << " " << it->username(); + } AddLine(os.str(), message); } @@ -2677,7 +2682,8 @@ bool ParseContent(const std::string& message, // Update the candidates with the media level "ice-pwd" and "ice-ufrag". for (Candidates::iterator it = candidates_orig.begin(); it != candidates_orig.end(); ++it) { - ASSERT((*it).username().empty()); + ASSERT((*it).username().empty() || + (*it).username() == transport->ice_ufrag); (*it).set_username(transport->ice_ufrag); ASSERT((*it).password().empty()); (*it).set_password(transport->ice_pwd); diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index fb55e31c9c..97b6a26a26 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc @@ -396,9 +396,9 @@ static const char kRawIPV6Candidate[] = "abcd::abcd::abcd::abcd::abcd::abcd::abcd::abcd 1234 typ host generation 2"; // One candidate reference string. -static const char kSdpOneCandidateOldFormat[] = +static const char kSdpOneCandidateWithUfragPwd[] = "a=candidate:a0+B/1 1 udp 2130706432 192.168.1.5 1234 typ host network_name" - " eth0 username user_rtp password password_rtp generation 2\r\n"; + " eth0 ufrag user_rtp pwd password_rtp generation 2\r\n"; // Session id and version static const char kSessionId[] = "18446744069414584320"; @@ -1727,6 +1727,13 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithExtmap) { TEST_F(WebRtcSdpTest, SerializeCandidates) { std::string message = webrtc::SdpSerializeCandidate(*jcandidate_); EXPECT_EQ(std::string(kRawCandidate), message); + + Candidate candidate_with_ufrag(candidates_.front()); + candidate_with_ufrag.set_username("ABC"); + jcandidate_.reset(new JsepIceCandidate(std::string("audio_content_name"), 0, + candidate_with_ufrag)); + message = webrtc::SdpSerializeCandidate(*jcandidate_); + EXPECT_EQ(std::string(kRawCandidate) + " ufrag ABC", message); } // TODO(mallinath) : Enable this test once WebRTCSdp capable of parsing @@ -2323,9 +2330,10 @@ TEST_F(WebRtcSdpTest, DeserializeCandidateWithDifferentTransport) { EXPECT_TRUE(jcandidate.candidate().IsEquivalent(jcandidate_->candidate())); } -TEST_F(WebRtcSdpTest, DeserializeCandidateOldFormat) { +TEST_F(WebRtcSdpTest, DeserializeCandidateWithUfragPwd) { JsepIceCandidate jcandidate(kDummyMid, kDummyIndex); - EXPECT_TRUE(SdpDeserializeCandidate(kSdpOneCandidateOldFormat,&jcandidate)); + EXPECT_TRUE( + SdpDeserializeCandidate(kSdpOneCandidateWithUfragPwd, &jcandidate)); EXPECT_EQ(kDummyMid, jcandidate.sdp_mid()); EXPECT_EQ(kDummyIndex, jcandidate.sdp_mline_index()); Candidate ref_candidate = jcandidate_->candidate(); diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index c95b04f0d2..5101bf3411 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -218,7 +218,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, remote_ice_mode_(ICEMODE_FULL), ice_role_(ICEROLE_UNKNOWN), tiebreaker_(0), - remote_candidate_generation_(0), gathering_state_(kIceGatheringNew), check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5), receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50), @@ -347,26 +346,19 @@ void P2PTransportChannel::SetIceCredentials(const std::string& ice_ufrag, void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag, const std::string& ice_pwd) { ASSERT(worker_thread_ == rtc::Thread::Current()); - bool ice_restart = false; - if (!remote_ice_ufrag_.empty() && !remote_ice_pwd_.empty()) { - ice_restart = (remote_ice_ufrag_ != ice_ufrag) || - (remote_ice_pwd_!= ice_pwd); + IceParameters* current_ice = remote_ice(); + IceParameters new_ice(ice_ufrag, ice_pwd); + if (!current_ice || *current_ice != new_ice) { + // Keep the ICE credentials so that newer connections + // are prioritized over the older ones. + remote_ice_parameters_.push_back(new_ice); } - remote_ice_ufrag_ = ice_ufrag; - remote_ice_pwd_ = ice_pwd; - // We need to update the credentials for any peer reflexive candidates. std::vector::iterator it = connections_.begin(); for (; it != connections_.end(); ++it) { (*it)->MaybeSetRemoteIceCredentials(ice_ufrag, ice_pwd); } - - if (ice_restart) { - // We need to keep track of the remote ice restart so newer - // connections are prioritized over the older. - ++remote_candidate_generation_; - } } void P2PTransportChannel::SetRemoteIceMode(IceMode mode) { @@ -536,8 +528,11 @@ void P2PTransportChannel::OnUnknownAddress( // The STUN binding request may arrive after setRemoteDescription and before // adding remote candidate, so we need to set the password to the shared // password if the user name matches. - if (remote_password.empty() && remote_username == remote_ice_ufrag_) { - remote_password = remote_ice_pwd_; + if (remote_password.empty()) { + IceParameters* current_ice = remote_ice(); + if (current_ice && remote_username == current_ice->ufrag) { + remote_password = current_ice->pwd; + } } Candidate remote_candidate; @@ -655,19 +650,39 @@ void P2PTransportChannel::OnNominated(Connection* conn) { void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) { ASSERT(worker_thread_ == rtc::Thread::Current()); - uint32_t generation = candidate.generation(); - // Network may not guarantee the order of the candidate delivery. If a - // remote candidate with an older generation arrives, drop it. - if (generation != 0 && generation < remote_candidate_generation_) { - LOG(LS_WARNING) << "Dropping a remote candidate because its generation " - << generation - << " is lower than the current remote generation " - << remote_candidate_generation_; + uint32_t generation = GetRemoteCandidateGeneration(candidate); + // If a remote candidate with a previous generation arrives, drop it. + if (generation < remote_ice_generation()) { + LOG(LS_WARNING) << "Dropping a remote candidate because its ufrag " + << candidate.username() + << " indicates it was for a previous generation."; return; } + Candidate new_remote_candidate(candidate); + new_remote_candidate.set_generation(generation); + // ICE candidates don't need to have username and password set, but + // the code below this (specifically, ConnectionRequest::Prepare in + // port.cc) uses the remote candidates's username. So, we set it + // here. + if (remote_ice()) { + if (candidate.username().empty()) { + new_remote_candidate.set_username(remote_ice()->ufrag); + } + if (new_remote_candidate.username() == remote_ice()->ufrag) { + if (candidate.password().empty()) { + new_remote_candidate.set_password(remote_ice()->pwd); + } + } else { + // The candidate belongs to the next generation. Its pwd will be set + // when the new remote ICE credentials arrive. + LOG(LS_WARNING) << "A remote candidate arrives with an unknown ufrag: " + << candidate.username(); + } + } + // Create connections to this remote candidate. - CreateConnections(candidate, NULL); + CreateConnections(new_remote_candidate, NULL); // Resort the connections list, which may have new elements. SortConnections(); @@ -680,20 +695,6 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate, PortInterface* origin_port) { ASSERT(worker_thread_ == rtc::Thread::Current()); - Candidate new_remote_candidate(remote_candidate); - new_remote_candidate.set_generation( - GetRemoteCandidateGeneration(remote_candidate)); - // ICE candidates don't need to have username and password set, but - // the code below this (specifically, ConnectionRequest::Prepare in - // port.cc) uses the remote candidates's username. So, we set it - // here. - if (remote_candidate.username().empty()) { - new_remote_candidate.set_username(remote_ice_ufrag_); - } - if (remote_candidate.password().empty()) { - new_remote_candidate.set_password(remote_ice_pwd_); - } - // If we've already seen the new remote candidate (in the current candidate // generation), then we shouldn't try creating connections for it. // We either already have a connection for it, or we previously created one @@ -702,7 +703,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate, // immediately be re-pruned, churning the network for no purpose. // This only applies to candidates received over signaling (i.e. origin_port // is NULL). - if (!origin_port && IsDuplicateRemoteCandidate(new_remote_candidate)) { + if (!origin_port && IsDuplicateRemoteCandidate(remote_candidate)) { // return true to indicate success, without creating any new connections. return true; } @@ -715,7 +716,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate, bool created = false; std::vector::reverse_iterator it; for (it = ports_.rbegin(); it != ports_.rend(); ++it) { - if (CreateConnection(*it, new_remote_candidate, origin_port)) { + if (CreateConnection(*it, remote_candidate, origin_port)) { if (*it == origin_port) created = true; } @@ -723,12 +724,12 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate, if ((origin_port != NULL) && std::find(ports_.begin(), ports_.end(), origin_port) == ports_.end()) { - if (CreateConnection(origin_port, new_remote_candidate, origin_port)) + if (CreateConnection(origin_port, remote_candidate, origin_port)) created = true; } // Remember this remote candidate so that we can add it to future ports. - RememberRemoteCandidate(new_remote_candidate, origin_port); + RememberRemoteCandidate(remote_candidate, origin_port); return created; } @@ -789,9 +790,27 @@ uint32_t P2PTransportChannel::GetRemoteCandidateGeneration( const Candidate& candidate) { // We need to keep track of the remote ice restart so newer // connections are prioritized over the older. - ASSERT(candidate.generation() == 0 || - candidate.generation() == remote_candidate_generation_); - return remote_candidate_generation_; + const auto& params = remote_ice_parameters_; + if (!candidate.username().empty()) { + // If remote side sets the ufrag, we use that to determine the candidate + // generation. + // Search backward as it is more likely to find it near the end. + auto it = std::find_if(params.rbegin(), params.rend(), + [candidate](const IceParameters& param) { + return param.ufrag == candidate.username(); + }); + if (it == params.rend()) { + // If not found, assume it is the next (future) generation. + return static_cast(remote_ice_parameters_.size()); + } + return params.rend() - it - 1; + } + // If candidate generation is set, use that. + if (candidate.generation() > 0) { + return candidate.generation(); + } + // Otherwise, assume the generation from remote ice parameters. + return remote_ice_generation(); } // Check if remote candidate is already cached. diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 2b02d36a61..92c0534e7c 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -36,6 +36,18 @@ namespace cricket { extern const uint32_t WEAK_PING_DELAY; +struct IceParameters { + std::string ufrag; + std::string pwd; + IceParameters(const std::string& ice_ufrag, const std::string& ice_pwd) + : ufrag(ice_ufrag), pwd(ice_pwd) {} + + bool operator==(const IceParameters& other) { + return ufrag == other.ufrag && pwd == other.pwd; + } + bool operator!=(const IceParameters& other) { return !(*this == other); } +}; + // Adds the port on which the candidate originated. class RemoteCandidate : public Candidate { public: @@ -229,6 +241,20 @@ class P2PTransportChannel : public TransportChannelImpl, Connection* best_nominated_connection() const; bool IsBackupConnection(Connection* conn) const; + // Returns the latest remote ICE parameters or nullptr if there are no remote + // ICE parameters yet. + IceParameters* remote_ice() { + return remote_ice_parameters_.empty() ? nullptr + : &remote_ice_parameters_.back(); + } + // Returns the index of the latest remote ICE parameters, or 0 if no remote + // ICE parameters have been received. + uint32_t remote_ice_generation() { + return remote_ice_parameters_.empty() + ? 0 + : static_cast(remote_ice_parameters_.size() - 1); + } + P2PTransport* transport_; PortAllocator* allocator_; rtc::Thread* worker_thread_; @@ -248,12 +274,10 @@ class P2PTransportChannel : public TransportChannelImpl, OptionMap options_; std::string ice_ufrag_; std::string ice_pwd_; - std::string remote_ice_ufrag_; - std::string remote_ice_pwd_; + std::vector remote_ice_parameters_; IceMode remote_ice_mode_; IceRole ice_role_; uint64_t tiebreaker_; - uint32_t remote_candidate_generation_; IceGatheringState gathering_state_; int check_receiving_delay_; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 8b3d0d9000..8f06f1c147 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1767,12 +1767,14 @@ class P2PTransportChannelPingTest : public testing::Test, cricket::Candidate CreateCandidate(const std::string& ip, int port, - int priority) { + int priority, + const std::string& ufrag = "") { cricket::Candidate c; c.set_address(rtc::SocketAddress(ip, port)); c.set_component(1); c.set_protocol(cricket::UDP_PROTOCOL_NAME); c.set_priority(priority); + c.set_username(ufrag); return c; } @@ -1856,6 +1858,51 @@ TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { EXPECT_EQ(conn2, ch.FindNextPingableConnection()); } +// Test adding remote candidates with different ufrags. If a remote candidate +// is added with an old ufrag, it will be discarded. If it is added with a +// ufrag that was not seen before, it will be used to create connections +// although the ICE pwd in the remote candidate will be set when the ICE +// credentials arrive. If a remote candidate is added with the current ICE +// ufrag, its pwd and generation will be set properly. +TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithVariousUfrags) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("add candidate", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.Connect(); + ch.MaybeStartGathering(); + // Add a candidate with a future ufrag. + ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 1, kIceUfrag[2])); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + const cricket::Candidate& candidate = conn1->remote_candidate(); + EXPECT_EQ(kIceUfrag[2], candidate.username()); + EXPECT_TRUE(candidate.password().empty()); + EXPECT_TRUE(ch.FindNextPingableConnection() == nullptr); + + // Set the remote credentials with the "future" ufrag. + // This should set the ICE pwd in the remote candidate of |conn1|, making + // it pingable. + ch.SetRemoteIceCredentials(kIceUfrag[2], kIcePwd[2]); + EXPECT_EQ(kIceUfrag[2], candidate.username()); + EXPECT_EQ(kIcePwd[2], candidate.password()); + EXPECT_EQ(conn1, ch.FindNextPingableConnection()); + + // Add a candidate with an old ufrag. No connection will be created. + ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 2, kIceUfrag[1])); + rtc::Thread::Current()->ProcessMessages(500); + EXPECT_TRUE(GetConnectionTo(&ch, "2.2.2.2", 2) == nullptr); + + // Add a candidate with the current ufrag, its pwd and generation will be + // assigned, even if the generation is not set. + ch.AddRemoteCandidate(CreateCandidate("3.3.3.3", 3, 0, kIceUfrag[2])); + cricket::Connection* conn3 = nullptr; + ASSERT_TRUE_WAIT((conn3 = GetConnectionTo(&ch, "3.3.3.3", 3)) != nullptr, + 3000); + const cricket::Candidate& new_candidate = conn3->remote_candidate(); + EXPECT_EQ(kIcePwd[2], new_candidate.password()); + EXPECT_EQ(1U, new_candidate.generation()); +} + TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("connection resurrection", 1, nullptr, &pa);