Fix bugs in discarding stun address
If mdns obfuscation is enabled, the stun address "MUST NOT be
considered redundant" when it is equal to local socket address.
Reference:
https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-mdns-ice-candidates-03#section-3.1.2.2
This fixes
Bug: webrtc:13426
Change-Id: I28e981fd1b8818f6f8294bc3fedf23b8d9146819
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/285421
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38774}
diff --git a/AUTHORS b/AUTHORS
index ad0220d..bd7ab24 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -68,6 +68,7 @@
Keiichi Enomoto <enm10k@gmail.com>
Kiran Thind <kiran.thind@gmail.com>
Korniltsev Anatoly <korniltsev.anatoly@gmail.com>
+Kyutae Lee <gorisanson@gmail.com>
Lennart Grahl <lennart.grahl@gmail.com>
Luke Weber <luke.weber@gmail.com>
Maksim Khobat <maksimkhobat@gmail.com>
diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc
index 5d57d1a..fdb7edc 100644
--- a/p2p/base/stun_port.cc
+++ b/p2p/base/stun_port.cc
@@ -550,11 +550,12 @@
}
bind_request_succeeded_servers_.insert(stun_server_addr);
// If socket is shared and `stun_reflected_addr` is equal to local socket
- // address, or if the same address has been added by another STUN server,
- // then discarding the stun address.
+ // address and mDNS obfuscation is not enabled, or if the same address has
+ // been added by another STUN server, then discarding the stun address.
// For STUN, related address is the local socket address.
- if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) &&
- !HasCandidateWithAddress(stun_reflected_addr)) {
+ if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress() ||
+ Network()->GetMdnsResponder() != nullptr) &&
+ !HasStunCandidateWithAddress(stun_reflected_addr)) {
rtc::SocketAddress related_address = socket_->GetLocalAddress();
// If we can't stamp the related address correctly, empty it to avoid leak.
if (!MaybeSetDefaultLocalAddress(&related_address)) {
@@ -637,11 +638,12 @@
stats_.stun_binding_requests_sent++;
}
-bool UDPPort::HasCandidateWithAddress(const rtc::SocketAddress& addr) const {
+bool UDPPort::HasStunCandidateWithAddress(
+ const rtc::SocketAddress& addr) const {
const std::vector<Candidate>& existing_candidates = Candidates();
std::vector<Candidate>::const_iterator it = existing_candidates.begin();
for (; it != existing_candidates.end(); ++it) {
- if (it->address() == addr)
+ if (it->type() == STUN_PORT_TYPE && it->address() == addr)
return true;
}
return false;
diff --git a/p2p/base/stun_port.h b/p2p/base/stun_port.h
index 06b5e1a..1397007 100644
--- a/p2p/base/stun_port.h
+++ b/p2p/base/stun_port.h
@@ -234,7 +234,7 @@
// changed to SignalPortReady.
void MaybeSetPortCompleteOrError();
- bool HasCandidateWithAddress(const rtc::SocketAddress& addr) const;
+ bool HasStunCandidateWithAddress(const rtc::SocketAddress& addr) const;
// If this is a low-cost network, it will keep on sending STUN binding
// requests indefinitely to keep the NAT binding alive. Otherwise, stop
diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc
index 003a58a..59841ad 100644
--- a/p2p/base/stun_port_unittest.cc
+++ b/p2p/base/stun_port_unittest.cc
@@ -89,6 +89,29 @@
DnsResolverExpectations expectations_;
};
+class FakeMdnsResponder : public webrtc::MdnsResponderInterface {
+ public:
+ void CreateNameForAddress(const rtc::IPAddress& addr,
+ NameCreatedCallback callback) override {
+ callback(addr, std::string("unittest-mdns-host-name.local"));
+ }
+
+ void RemoveNameForAddress(const rtc::IPAddress& addr,
+ NameRemovedCallback callback) override {}
+};
+
+class FakeMdnsResponderProvider : public rtc::MdnsResponderProvider {
+ public:
+ FakeMdnsResponderProvider() : mdns_responder_(new FakeMdnsResponder()) {}
+
+ webrtc::MdnsResponderInterface* GetMdnsResponder() const override {
+ return mdns_responder_.get();
+ }
+
+ private:
+ std::unique_ptr<webrtc::MdnsResponderInterface> mdns_responder_;
+};
+
// Base class for tests connecting a StunPort to a fake STUN server
// (cricket::StunServer).
class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
@@ -105,6 +128,7 @@
socket_factory_(ss_.get()),
stun_server_1_(cricket::TestStunServer::Create(ss_.get(), kStunAddr1)),
stun_server_2_(cricket::TestStunServer::Create(ss_.get(), kStunAddr2)),
+ mdns_responder_provider_(new FakeMdnsResponderProvider()),
done_(false),
error_(false),
stun_keepalive_delay_(1),
@@ -200,6 +224,10 @@
/* packet_time_us */ -1);
}
+ void EnableMdnsObfuscation() {
+ network_.set_mdns_responder_provider(mdns_responder_provider_.get());
+ }
+
protected:
static void SetUpTestSuite() {
// Ensure the RNG is inited.
@@ -237,6 +265,7 @@
std::unique_ptr<cricket::TestStunServer> stun_server_1_;
std::unique_ptr<cricket::TestStunServer> stun_server_2_;
std::unique_ptr<rtc::AsyncPacketSocket> socket_;
+ std::unique_ptr<rtc::MdnsResponderProvider> mdns_responder_provider_;
bool done_;
bool error_;
int stun_keepalive_delay_;
@@ -386,6 +415,41 @@
// No crash is success.
}
+// Test that a stun candidate (srflx candidate) is discarded whose address is
+// equal to that of a local candidate if mDNS obfuscation is not enabled.
+TEST_F(StunPortTest, TestStunCandidateDiscardedWithMdnsObfuscationNotEnabled) {
+ CreateSharedUdpPort(kStunAddr1, nullptr);
+ PrepareAddress();
+ EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock);
+ ASSERT_EQ(1U, port()->Candidates().size());
+ EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
+ EXPECT_EQ(port()->Candidates()[0].type(), cricket::LOCAL_PORT_TYPE);
+}
+
+// Test that a stun candidate (srflx candidate) is generated whose address is
+// equal to that of a local candidate if mDNS obfuscation is enabled.
+TEST_F(StunPortTest, TestStunCandidateGeneratedWithMdnsObfuscationEnabled) {
+ EnableMdnsObfuscation();
+ CreateSharedUdpPort(kStunAddr1, nullptr);
+ PrepareAddress();
+ EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock);
+ ASSERT_EQ(2U, port()->Candidates().size());
+
+ // The addresses of the candidates are both equal to kLocalAddr.
+ EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[0].address()));
+ EXPECT_TRUE(kLocalAddr.EqualIPs(port()->Candidates()[1].address()));
+
+ // One of the generated candidates is a local candidate and the other is a
+ // stun candidate.
+ EXPECT_NE(port()->Candidates()[0].type(), port()->Candidates()[1].type());
+ if (port()->Candidates()[0].type() == cricket::LOCAL_PORT_TYPE) {
+ EXPECT_EQ(port()->Candidates()[1].type(), cricket::STUN_PORT_TYPE);
+ } else {
+ EXPECT_EQ(port()->Candidates()[0].type(), cricket::STUN_PORT_TYPE);
+ EXPECT_EQ(port()->Candidates()[1].type(), cricket::LOCAL_PORT_TYPE);
+ }
+}
+
// Test that the same address is added only once if two STUN servers are in
// use.
TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
