Ensuring that UDP TURN servers are always used as STUN servers.

webrtc/p2p/client/portallocator_unittest.cc

 /*
  *  Copyright 2009 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 766 matching lines @@
   ASSERT_EQ(2U, ports_.size());
   EXPECT_PRED5(CheckCandidate, candidates_[0],
       cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
   EXPECT_PRED5(CheckCandidate, candidates_[1],
       cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
       rtc::SocketAddress(kNatAddr.ipaddr(), 0));
   EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
   EXPECT_EQ(3U, candidates_.size());
 }
 
-// Test TURN port in shared socket mode with UDP and TCP TURN server adderesses.
+// Test TURN port in shared socket mode with UDP and TCP TURN server addresses.
 TEST_F(PortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) {
   turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
   AddInterface(kClientAddr);
   allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
 
   AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr);
 
   allocator_->set_step_delay(cricket::kMinimumStepDelay);
   allocator_->set_flags(allocator().flags() |
                         cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
@@ skipping 111 matching lines @@
       rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
   EXPECT_EQ(candidates_[2].related_address(), candidates_[1].address());
 
   EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
   EXPECT_EQ(3U, candidates_.size());
   // Local port will be created first and then TURN port.
   EXPECT_EQ(2U, ports_[0]->Candidates().size());
   EXPECT_EQ(1U, ports_[1]->Candidates().size());
 }
 
+// Test that even when both a STUN and TURN server are configured, the TURN
+// server is used as a STUN server and we get a 'stun' candidate.
+TEST_F(PortAllocatorTest, TestSharedSocketWithNatUsingTurnAndStun) {
+  AddInterface(kClientAddr);
+  // Configure with STUN server but destroy it, so we can ensure that it's
+  // the TURN server actually being used as a STUN server.
+  ResetWithNatServer(kStunAddr);
+  stun_server_.reset();
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+
+  // Must set the step delay to 0 to make sure the relay allocation phase is
+  // started before the STUN candidates are obtained, so that the STUN binding
+  // response is processed when both StunPort and TurnPort exist to reproduce
+  // webrtc issue 3537.
+  allocator_->set_step_delay(0);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(3U, candidates_.size(), kDefaultAllocationTimeout);
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
+      rtc::SocketAddress(kNatAddr.ipaddr(), 0));
+  EXPECT_PRED5(CheckCandidate, candidates_[2],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  EXPECT_EQ(candidates_[2].related_address(), candidates_[1].address());
+
+  // Don't bother waiting for STUN timeout, since we already verified

[juberti1, 2015/06/27 04:39:54]

I don't understand this. Don't we want to get the response from the STUN server,
to ensure it's not a different srflx candidate that we might want to use?

[Taylor Brandstetter, 2015/06/27 17:05:49]

In this case the main thing I wanted to test was that if a STUN server isn't
available (see stun_server_.reset()), we still get a srflx candidate from the
TURN server. This wasn't working before.

But you're right that testing getting two different srflx candidates would be
even more valuable. I'll see if I can figure out how to configure two layers of
NAT and put the STUN/TURN servers at different levels.

[juberti1, 2015/06/29 18:40:00]

OK, I see what you are saying. I think the 2-level NAT might be overkill in that
case.

The main thing that I am troubled by is the 3537 comment and the need for a
specific step delay; that seems like it is testing implementation details.

[Taylor Brandstetter, 2015/06/29 20:18:34]

The 3537 comment and step delay aren't really needed for this test so I'll
remove them.

+  // that we got a STUN candidate from the TURN server.
+}
+
+// Test that when only a TCP TURN server is available, we do NOT use it as
+// a UDP STUN server, as this could leak our IP address. Thus we should only
+// expect two ports, a UDPPort and TurnPort.

[juberti1, 2015/06/27 04:39:54]

You'll only have two ports anytime you use shared socket. 

In order to test this properly, I think you need to have a NAT, and verify that
you don't get a srflx candidate.

[Taylor Brandstetter, 2015/06/27 17:05:49]

That's why I had intended to use non-shared socket mode, since it *would* result
in a separate UDPPort and StunPort. I verified that after doing that (Patch set
2), the test fails if I remove the TCP filter in basicportallocator.cc.

Also: I couldn't do this test with a NAT because the NATServer doesn't work with
TCP (TCP connects are dropped).

+TEST_F(PortAllocatorTest, TestNonSharedSocketWithoutNatUsingTurnTcp) {

[juberti1, 2015/06/27 04:39:54]

The socket is shared, so this name is confusing

[Taylor Brandstetter, 2015/06/27 17:05:50]

As mentioned above, fixed in patch set 2.

+  turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
+  AddInterface(kClientAddr);
+  allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
+
+  AddTurnServers(rtc::SocketAddress(), kTurnTcpIntAddr);
+
+  allocator_->set_step_delay(cricket::kMinimumStepDelay);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(2U, candidates_.size(), kDefaultAllocationTimeout);
+  ASSERT_EQ(2U, ports_.size());
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_EQ(2U, candidates_.size());
+  EXPECT_EQ(1U, ports_[0]->Candidates().size());
+  EXPECT_EQ(1U, ports_[1]->Candidates().size());
+}
+
+// Test that even when PORTALLOCATOR_ENABLE_SHARED_SOCKET is NOT enabled, the
+// TURN server is used as the STUN server and we get 'local', 'stun', and
+// 'relay' candidates.
+TEST_F(PortAllocatorTest, TestNonSharedSocketWithNatUsingTurnAsStun) {
+  AddInterface(kClientAddr);
+  // Use an empty SocketAddress to add a NAT without STUN server.
+  ResetWithNatServer(SocketAddress());
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+
+  allocator_->set_step_delay(0);
+  allocator_->set_flags(allocator().flags() |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_DISABLE_TCP);
+
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+
+  ASSERT_EQ_WAIT(3U, candidates_.size(), kDefaultAllocationTimeout);
+  ASSERT_EQ(3U, ports_.size());
+  EXPECT_PRED5(CheckCandidate, candidates_[0],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "local", "udp", kClientAddr);
+  EXPECT_PRED5(CheckCandidate, candidates_[1],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
+      rtc::SocketAddress(kNatAddr.ipaddr(), 0));
+  EXPECT_PRED5(CheckCandidate, candidates_[2],
+      cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+      rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+  // Not using shared socket, so the STUN request's server reflexive address
+  // should be different than the TURN request's server reflexive address.
+  EXPECT_NE(candidates_[2].related_address(), candidates_[1].address());
+
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
+  EXPECT_EQ(3U, candidates_.size());
+  EXPECT_EQ(1U, ports_[0]->Candidates().size());
+  EXPECT_EQ(1U, ports_[1]->Candidates().size());
+  EXPECT_EQ(1U, ports_[2]->Candidates().size());
+}
+
 // This test verifies when PORTALLOCATOR_ENABLE_SHARED_SOCKET flag is enabled
 // and fail to generate STUN candidate, local UDP candidate is generated
 // properly.
 TEST_F(PortAllocatorTest, TestSharedSocketNoUdpAllowed) {
   allocator().set_flags(allocator().flags() |
                         cricket::PORTALLOCATOR_DISABLE_RELAY |
                         cricket::PORTALLOCATOR_DISABLE_TCP |
                         cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
                         cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET);
   fss_->AddRule(false, rtc::FP_UDP, rtc::FD_ANY, kClientAddr);
@@ skipping 116 matching lines @@
   EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
   session_->StartGettingPorts();
   ASSERT_EQ_WAIT(7U, candidates_.size(), kDefaultAllocationTimeout);
   EXPECT_EQ(4U, ports_.size());
 
   auto it = ports_.begin();
   for (; it != ports_.end(); ++it) {
     (reinterpret_cast<cricket::Port*>(*it))->Destroy();
   }
 }