diff --git a/AUTHORS b/AUTHORS index ad0220d4d9..bd7ab248c2 100644 --- a/AUTHORS +++ b/AUTHORS @@ -68,6 +68,7 @@ Jose Antonio Olivera Ortega Keiichi Enomoto Kiran Thind Korniltsev Anatoly +Kyutae Lee Lennart Grahl Luke Weber Maksim Khobat diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index 5d57d1ac54..fdb7edce57 100644 --- a/p2p/base/stun_port.cc +++ b/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, - // 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 @@ 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& existing_candidates = Candidates(); std::vector::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 06b5e1ae1c..13970070ed 100644 --- a/p2p/base/stun_port.h +++ b/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 diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index 003a58a9b8..59841adeef 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/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 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 stun_server_1_; std::unique_ptr stun_server_2_; std::unique_ptr socket_; + std::unique_ptr 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) {