Sanitize the selected candidate pair in the public API.
The public API to obtain the selected candidate pair is changed to
GetSelectedCandidatePair in the ICE transport, and the returned pair
has address-sanitized candidates.
Bug: chromium:993878
Change-Id: I44f9d2385a84f9e22447108be2e57ef9e62671eb
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/149080
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28869}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 6d4c3de..7c6ae5c 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -720,6 +720,38 @@
}
}
+CandidatePair Connection::ToCandidatePairAndSanitizeIfNecessary() const {
+ auto get_sanitized_copy = [](const Candidate& c) {
+ bool use_hostname_address = c.type() == LOCAL_PORT_TYPE;
+ bool filter_related_address = c.type() == STUN_PORT_TYPE;
+ return c.ToSanitizedCopy(use_hostname_address, filter_related_address);
+ };
+
+ CandidatePair pair;
+ if (port_->Network()->GetMdnsResponder() != nullptr) {
+ // When the mDNS obfuscation of local IPs is enabled, we sanitize local
+ // candidates.
+ pair.local = get_sanitized_copy(local_candidate());
+ } else {
+ pair.local = local_candidate();
+ }
+
+ if (!remote_candidate().address().hostname().empty()) {
+ // If the remote endpoint signaled us a hostname candidate, we assume it is
+ // supposed to be sanitized in the stats.
+ //
+ // A prflx remote candidate should not have a hostname set.
+ RTC_DCHECK(remote_candidate().type() != PRFLX_PORT_TYPE);
+ // A remote hostname candidate should have a resolved IP before we can form
+ // a candidate pair.
+ RTC_DCHECK(!remote_candidate().address().IsUnresolvedIP());
+ pair.remote = get_sanitized_copy(remote_candidate());
+ } else {
+ pair.remote = remote_candidate();
+ }
+ return pair;
+}
+
void Connection::ReceivedPingResponse(
int rtt,
const std::string& request_id,
@@ -1107,33 +1139,9 @@
}
void Connection::CopyCandidatesToStatsAndSanitizeIfNecessary() {
- auto get_sanitized_copy = [](const Candidate& c) {
- bool use_hostname_address = c.type() == LOCAL_PORT_TYPE;
- bool filter_related_address = c.type() == STUN_PORT_TYPE;
- return c.ToSanitizedCopy(use_hostname_address, filter_related_address);
- };
-
- if (port_->Network()->GetMdnsResponder() != nullptr) {
- // When the mDNS obfuscation of local IPs is enabled, we sanitize local
- // candidates.
- stats_.local_candidate = get_sanitized_copy(local_candidate());
- } else {
- stats_.local_candidate = local_candidate();
- }
-
- if (!remote_candidate().address().hostname().empty()) {
- // If the remote endpoint signaled us a hostname candidate, we assume it is
- // supposed to be sanitized in the stats.
- //
- // A prflx remote candidate should not have a hostname set.
- RTC_DCHECK(remote_candidate().type() != PRFLX_PORT_TYPE);
- // A remote hostname candidate should have a resolved IP before we can form
- // a candidate pair.
- RTC_DCHECK(!remote_candidate().address().IsUnresolvedIP());
- stats_.remote_candidate = get_sanitized_copy(remote_candidate());
- } else {
- stats_.remote_candidate = remote_candidate();
- }
+ auto pair = ToCandidatePairAndSanitizeIfNecessary();
+ stats_.local_candidate = pair.local_candidate();
+ stats_.remote_candidate = pair.remote_candidate();
}
bool Connection::rtt_converged() const {
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index 82b2c89..92fc2ed 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -36,6 +36,16 @@
// Forward declaration so that a ConnectionRequest can contain a Connection.
class Connection;
+struct CandidatePair final : public CandidatePairInterface {
+ ~CandidatePair() override = default;
+
+ const Candidate& local_candidate() const override { return local; }
+ const Candidate& remote_candidate() const override { return remote; }
+
+ Candidate local;
+ Candidate remote;
+};
+
// A ConnectionRequest is a simple STUN ping used to determine writability.
class ConnectionRequest : public StunRequest {
public:
@@ -228,6 +238,10 @@
void HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg);
int64_t last_data_received() const { return last_data_received_; }
+ // Returns the equivalent candidate pair and sanitizes the local and the
+ // remote candidates if necessary.
+ CandidatePair ToCandidatePairAndSanitizeIfNecessary() const;
+
// Debugging description of this connection
std::string ToDebugId() const;
std::string ToString() const;
diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h
index 1b95a55..467ee2e 100644
--- a/p2p/base/fake_ice_transport.h
+++ b/p2p/base/fake_ice_transport.h
@@ -207,6 +207,10 @@
absl::optional<int> GetRttEstimate() override { return absl::nullopt; }
const Connection* selected_connection() const override { return nullptr; }
+ absl::optional<const CandidatePair> GetSelectedCandidatePair()
+ const override {
+ return absl::nullopt;
+ }
// Fake PacketTransportInternal implementation.
bool writable() const override { return writable_; }
diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h
index 630848f..7f1d70b 100644
--- a/p2p/base/ice_transport_internal.h
+++ b/p2p/base/ice_transport_internal.h
@@ -263,8 +263,14 @@
// absl::optional if there is none.
virtual absl::optional<int> GetRttEstimate() = 0;
+ // TODO(qingsi): Remove this method once Chrome does not depend on it anymore.
virtual const Connection* selected_connection() const = 0;
+ // Returns the selected candidate pair, or an empty absl::optional if there is
+ // none.
+ virtual absl::optional<const CandidatePair> GetSelectedCandidatePair()
+ const = 0;
+
sigslot::signal1<IceTransportInternal*> SignalGatheringState;
// Handles sending and receiving of candidates.
diff --git a/p2p/base/mock_ice_transport.h b/p2p/base/mock_ice_transport.h
index 6aeb950..a28c796 100644
--- a/p2p/base/mock_ice_transport.h
+++ b/p2p/base/mock_ice_transport.h
@@ -63,6 +63,10 @@
void SetIceConfig(const IceConfig& config) override {}
absl::optional<int> GetRttEstimate() override { return absl::nullopt; }
const Connection* selected_connection() const override { return nullptr; }
+ absl::optional<const CandidatePair> GetSelectedCandidatePair()
+ const override {
+ return absl::nullopt;
+ }
void MaybeStartGathering() override {}
void AddRemoteCandidate(const Candidate& candidate) override {}
void RemoveRemoteCandidate(const Candidate& candidate) override {}
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 6356859..01caaa9 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -380,6 +380,16 @@
}
}
+absl::optional<const CandidatePair>
+P2PTransportChannel::GetSelectedCandidatePair() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ if (selected_connection_ == nullptr) {
+ return absl::nullopt;
+ }
+
+ return selected_connection_->ToCandidatePairAndSanitizeIfNecessary();
+}
+
// A channel is considered ICE completed once there is at most one active
// connection per network and at least one active connection.
IceTransportState P2PTransportChannel::ComputeState() const {
@@ -1965,7 +1975,6 @@
}
// Create event for candidate pair change.
-
if (selected_connection_) {
CandidatePairChangeEvent pair_change;
pair_change.reason = reason;
@@ -1975,7 +1984,6 @@
selected_connection_->last_data_received();
SignalCandidatePairChanged(pair_change);
}
-
SignalNetworkRouteChanged(network_route_);
}
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 0546b36..f9ea59d 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -135,6 +135,7 @@
std::vector<CandidateStats>* candidate_stats_list) override;
absl::optional<int> GetRttEstimate() override;
const Connection* selected_connection() const override;
+ absl::optional<const CandidatePair> GetSelectedCandidatePair() const override;
// TODO(honghaiz): Remove this method once the reference of it in
// Chromoting is removed.
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index a8e83ba..1b93475 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -4991,6 +4991,59 @@
DestroyChannels();
}
+// A similar test as above to check the selected candidate pair is sanitized
+// when it is queried via GetSelectedCandidatePair.
+TEST_F(P2PTransportChannelTest,
+ SelectedCandidatePairSanitizedWhenMdnsObfuscationEnabled) {
+ NiceMock<rtc::MockAsyncResolver> mock_async_resolver;
+ webrtc::MockAsyncResolverFactory mock_async_resolver_factory;
+ EXPECT_CALL(mock_async_resolver_factory, Create())
+ .WillOnce(Return(&mock_async_resolver));
+
+ // ep1 and ep2 will gather host candidates with addresses
+ // kPublicAddrs[0] and kPublicAddrs[1], respectively.
+ ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts);
+ // ICE parameter will be set up when creating the channels.
+ set_remote_ice_parameter_source(FROM_SETICEPARAMETERS);
+ GetEndpoint(0)->network_manager_.set_mdns_responder(
+ absl::make_unique<webrtc::FakeMdnsResponder>(rtc::Thread::Current()));
+ GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory;
+ CreateChannels();
+ // Pause sending candidates from both endpoints until we find out what port
+ // number is assigned to ep1's host candidate.
+ PauseCandidates(0);
+ PauseCandidates(1);
+ ASSERT_EQ_WAIT(1u, GetEndpoint(0)->saved_candidates_.size(), kMediumTimeout);
+ const auto& candidates_data = GetEndpoint(0)->saved_candidates_[0];
+ ASSERT_EQ(1u, candidates_data->candidates.size());
+ const auto& local_candidate_ep1 = candidates_data->candidates[0];
+ ASSERT_TRUE(local_candidate_ep1.type() == LOCAL_PORT_TYPE);
+ // This is the underlying private IP address of the same candidate at ep1,
+ // and let the mock resolver of ep2 receive the correct resolution.
+ rtc::SocketAddress resolved_address_ep1(local_candidate_ep1.address());
+ resolved_address_ep1.SetResolvedIP(kPublicAddrs[0].ipaddr());
+ EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _))
+ .WillOnce(DoAll(SetArgPointee<1>(resolved_address_ep1), Return(true)));
+ ResumeCandidates(0);
+ ResumeCandidates(1);
+
+ ASSERT_TRUE_WAIT(ep1_ch1()->selected_connection() != nullptr &&
+ ep2_ch1()->selected_connection() != nullptr,
+ kMediumTimeout);
+
+ const auto pair_ep1 = ep1_ch1()->GetSelectedCandidatePair();
+ ASSERT_TRUE(pair_ep1.has_value());
+ EXPECT_EQ(LOCAL_PORT_TYPE, pair_ep1->local_candidate().type());
+ EXPECT_TRUE(pair_ep1->local_candidate().address().IsUnresolvedIP());
+
+ const auto pair_ep2 = ep2_ch1()->GetSelectedCandidatePair();
+ ASSERT_TRUE(pair_ep2.has_value());
+ EXPECT_EQ(LOCAL_PORT_TYPE, pair_ep2->remote_candidate().type());
+ EXPECT_TRUE(pair_ep2->remote_candidate().address().IsUnresolvedIP());
+
+ DestroyChannels();
+}
+
class MockMdnsResponder : public webrtc::MdnsResponderInterface {
public:
MOCK_METHOD2(CreateNameForAddress,
