Revert "p2p: separate ICE tie breaker and foundation seed" This reverts commit d99499abbae94793a02944a1f28f7015816447f5. Reason for revert: Breaks downstream projects and I can also repro locally when running the rtc_unittest test target (it does however pass in isolation indicating test cleanup/setup needs to be fixed) Original change's description: > p2p: separate ICE tie breaker and foundation seed > > BUG=webrtc:14626 > > Change-Id: I189a708192c9cef0b50c3fcbe798b30376d3b547 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/338982 > Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> > Commit-Queue: Philipp Hancke <phancke@microsoft.com> > Reviewed-by: Harald Alvestrand <hta@webrtc.org> > Cr-Commit-Position: refs/heads/main@{#41806} Bug: webrtc:14626 Change-Id: If45f8a33395c562c9388b3d3748e8566efa87ecb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/341081 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Jeremy Leconte <jleconte@google.com> Commit-Queue: Christoffer Dewerin <jansson@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Owners-Override: Christoffer Dewerin <jansson@webrtc.org> Cr-Commit-Position: refs/heads/main@{#41812}
diff --git a/api/candidate.cc b/api/candidate.cc index 85083e5..930b874 100644 --- a/api/candidate.cc +++ b/api/candidate.cc
@@ -263,7 +263,7 @@ } void Candidate::ComputeFoundation(const rtc::SocketAddress& base_address, - uint64_t seed) { + uint64_t tie_breaker) { // https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3 // The foundation is an identifier, scoped within a session. Two candidates // MUST have the same foundation ID when all of the following are true: @@ -282,9 +282,14 @@ rtc::StringBuilder sb; sb << type_name() << base_address.ipaddr().ToString() << protocol_ << relay_protocol_; - // The random seed is passed to prevent a reverse CRC32 lookup of the address - // based on the foundation. - sb << rtc::ToString(seed); + + // https://www.rfc-editor.org/rfc/rfc5245#section-5.2 + // [...] it is possible for both agents to mistakenly believe they are + // controlled or controlling. To resolve this, each agent MUST select a random + // number, called the tie-breaker, uniformly distributed between 0 and (2**64) + // - 1 (that is, a 64-bit positive integer). This number is used in + // connectivity checks to detect and repair this case [...] + sb << rtc::ToString(tie_breaker); foundation_ = rtc::ToString(rtc::ComputeCrc32(sb.Release())); }
diff --git a/api/candidate.h b/api/candidate.h index f8f73d2..9535962 100644 --- a/api/candidate.h +++ b/api/candidate.h
@@ -254,9 +254,10 @@ // then the foundation will be different. Two candidate pairs with // the same foundation pairs are likely to have similar network // characteristics. Foundations are used in the frozen algorithm. - // A session wide (peerconnection) seed is applied to the foundation, + // A session wide (peerconnection) tie-breaker is applied to the foundation, // adds additional randomness and must be the same for all candidates. - void ComputeFoundation(const rtc::SocketAddress& base_address, uint64_t seed); + void ComputeFoundation(const rtc::SocketAddress& base_address, + uint64_t tie_breaker); // https://www.rfc-editor.org/rfc/rfc5245#section-7.2.1.3 // Call to populate the foundation field for a new peer reflexive remote
diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h index 4ce3058..a51a7ca 100644 --- a/p2p/base/fake_port_allocator.h +++ b/p2p/base/fake_port_allocator.h
@@ -128,7 +128,6 @@ field_trials_)); RTC_DCHECK(port_); port_->SetIceTiebreaker(allocator_->ice_tiebreaker()); - port_->SetFoundationSeed(allocator_->foundation_seed()); port_->SubscribePortDestroyed( [this](PortInterface* port) { OnPortDestroyed(port); }); AddPort(port_.get());
diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 81e3374..917167c 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc
@@ -199,14 +199,6 @@ return tiebreaker_; } -void Port::SetFoundationSeed(uint64_t foundation_seed) { - foundation_seed_ = foundation_seed; -} - -uint64_t Port::FoundationSeed() const { - return foundation_seed_; -} - bool Port::SharedSocket() const { return shared_socket_; } @@ -263,8 +255,8 @@ type, generation_, "", network_->id(), network_cost_); // Set the relay protocol before computing the foundation field. c.set_relay_protocol(relay_protocol); - // TODO(bugs.webrtc.org/14605): ensure FoundationSeed() is set. - c.ComputeFoundation(base_address, foundation_seed_); + // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set. + c.ComputeFoundation(base_address, tiebreaker_); c.set_priority( c.GetPriority(type_preference, network_->preference(), relay_preference,
diff --git a/p2p/base/port.h b/p2p/base/port.h index 1758aa9..4021c25 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h
@@ -211,9 +211,6 @@ void SetIceTiebreaker(uint64_t tiebreaker) override; uint64_t IceTiebreaker() const override; - void SetFoundationSeed(uint64_t foundation_seed) override; - uint64_t FoundationSeed() const override; - bool SharedSocket() const override; void ResetSharedSocket() { shared_socket_ = false; } @@ -497,7 +494,6 @@ bool enable_port_packets_; IceRole ice_role_; uint64_t tiebreaker_; - uint64_t foundation_seed_; bool shared_socket_; // Information to use when going through a proxy. std::string user_agent_;
diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index baf7cb5..52fc8c1 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc
@@ -100,8 +100,7 @@ step_delay_(kDefaultStepDelay), allow_tcp_listen_(true), candidate_filter_(CF_ALL), - tiebreaker_(rtc::CreateRandomId64()), - foundation_seed_(rtc::CreateRandomId64()) { + tiebreaker_(rtc::CreateRandomId64()) { // The allocator will be attached to a thread in Initialize. thread_checker_.Detach(); }
diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index 1aee4c1..63ecfc6 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h
@@ -452,7 +452,6 @@ Candidate SanitizeCandidate(const Candidate& c) const; uint64_t ice_tiebreaker() const { return tiebreaker_; } - uint64_t foundation_seed() const { return foundation_seed_; } uint32_t flags() const { CheckRunOnValidThreadIfInitialized(); @@ -665,14 +664,8 @@ std::vector<std::unique_ptr<PortAllocatorSession>>::const_iterator FindPooledSession(const IceParameters* ice_credentials = nullptr) const; - // ICE Tie-breaker as described in - // https://www.rfc-editor.org/rfc/rfc5245#section-5.2 + // ICE tie breaker. uint64_t tiebreaker_; - // A random value that is included in the foundation of all ICE candidates - // generated by ports derived from this allocator. See - // https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3 - // Never sent over the network. - uint64_t foundation_seed_; }; } // namespace cricket
diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h index e9f61ab..8a1d18d 100644 --- a/p2p/base/port_interface.h +++ b/p2p/base/port_interface.h
@@ -62,11 +62,6 @@ virtual void SetIceTiebreaker(uint64_t tiebreaker) = 0; virtual uint64_t IceTiebreaker() const = 0; - // Methods to set/get the PortAllocator random value which is used to seed the - // foundation. - virtual void SetFoundationSeed(uint64_t foundation_seed) = 0; - virtual uint64_t FoundationSeed() const = 0; - virtual bool SharedSocket() const = 0; virtual bool SupportsProtocol(absl::string_view protocol) const = 0;
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index eeb940a..ab3ff86 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc
@@ -109,7 +109,6 @@ constexpr int kTiebreaker1 = 11111; constexpr int kTiebreaker2 = 22222; constexpr int kTiebreakerDefault = 44444; -constexpr int kFoundationSeed = 42; const char* data = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890"; @@ -556,7 +555,6 @@ username_, password_, true, absl::nullopt, &field_trials_); port->SetIceTiebreaker(kTiebreakerDefault); - port->SetFoundationSeed(kFoundationSeed); return port; } @@ -569,7 +567,6 @@ MakeNetworkMultipleAddrs(global_addr, link_local_addr), 0, 0, username_, password_, true, absl::nullopt, &field_trials_); port->SetIceTiebreaker(kTiebreakerDefault); - port->SetFoundationSeed(kFoundationSeed); return port; } std::unique_ptr<TCPPort> CreateTcpPort(const SocketAddress& addr) { @@ -580,7 +577,6 @@ auto port = TCPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0, username_, password_, true, &field_trials_); port->SetIceTiebreaker(kTiebreakerDefault); - port->SetFoundationSeed(kFoundationSeed); return port; } std::unique_ptr<StunPort> CreateStunPort(const SocketAddress& addr, @@ -591,7 +587,6 @@ username_, password_, stun_servers, absl::nullopt, &field_trials_); port->SetIceTiebreaker(kTiebreakerDefault); - port->SetFoundationSeed(kFoundationSeed); return port; } std::unique_ptr<Port> CreateRelayPort(const SocketAddress& addr,
diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 2a37a5b..cc38a66 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc
@@ -1484,7 +1484,6 @@ if (port) { port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); - port->SetFoundationSeed(session_->allocator()->foundation_seed()); // If shared socket is enabled, STUN candidate will be allocated by the // UDPPort. if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) { @@ -1521,7 +1520,6 @@ session_->allocator()->field_trials()); if (port) { port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); - port->SetFoundationSeed(session_->allocator()->foundation_seed()); session_->AddAllocatedPort(port.release(), this); // Since TCPPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1552,7 +1550,6 @@ session_->allocator()->field_trials()); if (port) { port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); - port->SetFoundationSeed(session_->allocator()->foundation_seed()); session_->AddAllocatedPort(port.release(), this); // Since StunPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1656,7 +1653,6 @@ } RTC_DCHECK(port != NULL); port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker()); - port->SetFoundationSeed(session_->allocator()->foundation_seed()); session_->AddAllocatedPort(port.release(), this); } }