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}

AUTHORS

@@ -68,6 +68,7 @@ Jose Antonio Olivera Ortega <josea.olivera@gmail.com>
 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>

p2p/base/stun_port.cc

@@ -550,11 +550,12 @@ void UDPPort::OnStunBindingRequestSucceeded(
   }
   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,
+  // address and mDNS obfuscation is not enabled, or if the same address has
-  // then discarding the stun address.
+  // 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()) &&
+  if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress() ||
-      !HasCandidateWithAddress(stun_reflected_addr)) {
+       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 @@ void UDPPort::OnSendPacket(const void* data, size_t size, StunRequest* req) {
   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;

p2p/base/stun_port.h

@@ -234,7 +234,7 @@ class UDPPort : public Port {
   // 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

p2p/base/stun_port_unittest.cc

@@ -89,6 +89,29 @@ class MockDnsResolverPacketSocketFactory
   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 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
         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 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
                                      /* 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 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> {
   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 @@ TEST_F(StunPortTestWithRealClock,
   // 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) {