From 1e00dbccc23f4dd35159c3c95913a49b6360d931 Mon Sep 17 00:00:00 2001 From: Min Wang Date: Wed, 26 Jun 2019 13:08:29 -0500 Subject: [PATCH] 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 Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#28421} --- AUTHORS | 1 + p2p/base/stun_server.cc | 6 ++-- p2p/base/stun_server.h | 2 +- p2p/base/stun_server_unittest.cc | 52 ++++++++++++++++++++++++++++---- p2p/base/test_stun_server.cc | 2 +- 5 files changed, 52 insertions(+), 11 deletions(-) diff --git a/AUTHORS b/AUTHORS index bdd24f3eaf..fbf93539fe 100644 --- a/AUTHORS +++ b/AUTHORS @@ -78,6 +78,7 @@ Tomas Popela Jan Grulich Eike Rathke Michel Promonet +Min Wang &yet LLC <*@andyet.com> Agora IO <*@agora.io> diff --git a/p2p/base/stun_server.cc b/p2p/base/stun_server.cc index 38ccc3fab3..382b787951 100644 --- a/p2p/base/stun_server.cc +++ b/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 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); diff --git a/p2p/base/stun_server.h b/p2p/base/stun_server.h index 7ddc5c10e3..01d74e28ed 100644 --- a/p2p/base/stun_server.h +++ b/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; diff --git a/p2p/base/stun_server_unittest.cc b/p2p/base/stun_server_unittest.cc index efc1cd2b30..7b11d6f50c 100644 --- a/p2p/base/stun_server_unittest.cc +++ b/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,11 +90,50 @@ 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 (" - << mapped_addr->ipaddr().ToString() << ") != local IP (" - << client_addr.ipaddr().ToString() << ")"; - } + + delete msg; +} + +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; } diff --git a/p2p/base/test_stun_server.cc b/p2p/base/test_stun_server.cc index 3c98cd8d64..9330a00075 100644 --- a/p2p/base/test_stun_server.cc +++ b/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); } }