Stun server should return XOR-MAPPED-ADDRESS/MAPPED-ADDRESS correctly

* Return xor mapped address for RFC5389 compatible client
* fix a typo in function name
* update stunserver unitest case
* update author

Bug: webrtc:10764
Change-Id: I466799744a343508233c18b7c477d2212680392a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/143841
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28421}

AUTHORS

@@ -78,6 +78,7 @@ Tomas Popela <tomas.popela@gmail.com>
 Jan Grulich <grulja@gmail.com>
 Eike Rathke <erathke@redhat.com>
 Michel Promonet <michel.promonet.1@gmail.com>
+Min Wang <mingewang@gmail.com>
 
 &yet LLC <*@andyet.com>
 Agora IO <*@agora.io>

p2p/base/stun_server.cc

@@ -54,7 +54,7 @@ void StunServer::OnPacket(rtc::AsyncPacketSocket* socket,
 void StunServer::OnBindingRequest(StunMessage* msg,
                                   const rtc::SocketAddress& remote_addr) {
   StunMessage response;
-  GetStunBindReqponse(msg, remote_addr, &response);
+  GetStunBindResponse(msg, remote_addr, &response);
   SendResponse(response, remote_addr);
 }
 
@@ -83,7 +83,7 @@ void StunServer::SendResponse(const StunMessage& msg,
     RTC_LOG_ERR(LS_ERROR) << "sendto";
 }
 
-void StunServer::GetStunBindReqponse(StunMessage* request,
+void StunServer::GetStunBindResponse(StunMessage* request,
                                      const rtc::SocketAddress& remote_addr,
                                      StunMessage* response) const {
   response->SetType(STUN_BINDING_RESPONSE);
@@ -91,7 +91,7 @@ void StunServer::GetStunBindReqponse(StunMessage* request,
 
   // Tell the user the address that we received their request from.
   std::unique_ptr<StunAddressAttribute> mapped_addr;
-  if (!request->IsLegacy()) {
+  if (request->IsLegacy()) {
     mapped_addr = StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS);
   } else {
     mapped_addr = StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);

p2p/base/stun_server.h

@@ -57,7 +57,7 @@ class StunServer : public sigslot::has_slots<> {
   void SendResponse(const StunMessage& msg, const rtc::SocketAddress& addr);
 
   // A helper method to compose a STUN binding response.
-  void GetStunBindReqponse(StunMessage* request,
+  void GetStunBindResponse(StunMessage* request,
                            const rtc::SocketAddress& remote_addr,
                            StunMessage* response) const;
 

p2p/base/stun_server_unittest.cc

@@ -74,7 +74,8 @@ class StunServerTest : public ::testing::Test {
 
 TEST_F(StunServerTest, TestGood) {
   StunMessage req;
-  std::string transaction_id = "0123456789ab";
+  // kStunLegacyTransactionIdLength = 16 for legacy RFC 3489 request
+  std::string transaction_id = "0123456789abcdef";
   req.SetType(STUN_BINDING_REQUEST);
   req.SetTransactionID(transaction_id);
   Send(req);
@@ -89,12 +90,51 @@ TEST_F(StunServerTest, TestGood) {
   EXPECT_TRUE(mapped_addr != NULL);
   EXPECT_EQ(1, mapped_addr->family());
   EXPECT_EQ(client_addr.port(), mapped_addr->port());
-  if (mapped_addr->ipaddr() != client_addr.ipaddr()) {
+
-    RTC_LOG(LS_WARNING) << "Warning: mapped IP ("
+  delete msg;
-                        << mapped_addr->ipaddr().ToString() << ") != local IP ("
-                        << client_addr.ipaddr().ToString() << ")";
 }
 
+TEST_F(StunServerTest, TestGoodXorMappedAddr) {
+  StunMessage req;
+  // kStunTransactionIdLength = 12 for RFC 5389 request
+  // StunMessage::Write will automatically insert magic cookie (0x2112A442)
+  std::string transaction_id = "0123456789ab";
+  req.SetType(STUN_BINDING_REQUEST);
+  req.SetTransactionID(transaction_id);
+  Send(req);
+
+  StunMessage* msg = Receive();
+  ASSERT_TRUE(msg != NULL);
+  EXPECT_EQ(STUN_BINDING_RESPONSE, msg->type());
+  EXPECT_EQ(req.transaction_id(), msg->transaction_id());
+
+  const StunAddressAttribute* mapped_addr =
+      msg->GetAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);
+  EXPECT_TRUE(mapped_addr != NULL);
+  EXPECT_EQ(1, mapped_addr->family());
+  EXPECT_EQ(client_addr.port(), mapped_addr->port());
+
+  delete msg;
+}
+
+// Send legacy RFC 3489 request, should not get xor mapped addr
+TEST_F(StunServerTest, TestNoXorMappedAddr) {
+  StunMessage req;
+  // kStunLegacyTransactionIdLength = 16 for legacy RFC 3489 request
+  std::string transaction_id = "0123456789abcdef";
+  req.SetType(STUN_BINDING_REQUEST);
+  req.SetTransactionID(transaction_id);
+  Send(req);
+
+  StunMessage* msg = Receive();
+  ASSERT_TRUE(msg != NULL);
+  EXPECT_EQ(STUN_BINDING_RESPONSE, msg->type());
+  EXPECT_EQ(req.transaction_id(), msg->transaction_id());
+
+  const StunAddressAttribute* mapped_addr =
+      msg->GetAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);
+  EXPECT_TRUE(mapped_addr == NULL);
+
   delete msg;
 }
 

p2p/base/test_stun_server.cc

@@ -30,7 +30,7 @@ void TestStunServer::OnBindingRequest(StunMessage* msg,
     StunServer::OnBindingRequest(msg, remote_addr);
   } else {
     StunMessage response;
-    GetStunBindReqponse(msg, fake_stun_addr_, &response);
+    GetStunBindResponse(msg, fake_stun_addr_, &response);
     SendResponse(response, remote_addr);
   }
 }