From 219d8ce889d174f1cef84a3d8779ee3cbf4ca1aa Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Wed, 15 Jan 2020 15:29:48 +0100 Subject: [PATCH] GOOG_PING: improve handshake This patch improves handshake wrt GOOG_PING support so that - if goog_ping_enable: sender send it's goog-ping version until it gets STUN_BINDING_RESPONSE - receiver only sends it's goog-ping-version if getting a goog-ping-version in the request This means that the overhead of STUN_ATTR_GOOG_MISC_INFO is only - added on STUN_BINDING_REQUEST until a response is received. - added on STUN_BINDING_RESPONSE if remote peer request it. This is wire compatible with older versions so that - new sender will enable GOOG_PING with new/old receiver. - old sender will enable GOOG_PING with old receiver. - old version will not enable GOOG_PING with new receiver (receiver expecting sender to announce first). BUG: webrtc:11100 Change-Id: Ib3434c593988188150f4c7506918139aaf138d0c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/165787 Reviewed-by: Harald Alvestrand Reviewed-by: Sebastian Jansson Commit-Queue: Jonas Oreland Cr-Commit-Position: refs/heads/master@{#30269} --- api/transport/stun.h | 4 +- p2p/base/connection.cc | 58 +++-- .../p2p_transport_channel_ice_field_trials.h | 5 +- p2p/base/port_unittest.cc | 210 +++++++++++++++++- 4 files changed, 250 insertions(+), 27 deletions(-) diff --git a/api/transport/stun.h b/api/transport/stun.h index 7860da2fdc..41f76a1ba7 100644 --- a/api/transport/stun.h +++ b/api/transport/stun.h @@ -678,7 +678,9 @@ enum IceAttributeType { // consistent with those used in ConnectionRequest::Prepare when forming a STUN // message for the ICE connectivity check, and they are used when parsing a // received STUN message. -enum class IceGoogMiscInfoBindingRequestAttributeIndex {}; +enum class IceGoogMiscInfoBindingRequestAttributeIndex { + SUPPORT_GOOG_PING_VERSION = 0, +}; enum class IceGoogMiscInfoBindingResponseAttributeIndex { SUPPORT_GOOG_PING_VERSION = 0, diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index cd5d290772..f3692c5cc0 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -143,7 +143,11 @@ constexpr int64_t kMinExtraPingDelayMs = 100; // Default field trials. const cricket::IceFieldTrials kDefaultFieldTrials; -constexpr int kSupportGoogPingVersionIndex = +constexpr int kSupportGoogPingVersionRequestIndex = + static_cast(cricket::IceGoogMiscInfoBindingRequestAttributeIndex:: + SUPPORT_GOOG_PING_VERSION); + +constexpr int kSupportGoogPingVersionResponseIndex = static_cast(cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: SUPPORT_GOOG_PING_VERSION); @@ -224,6 +228,17 @@ void ConnectionRequest::Prepare(StunMessage* request) { request->AddAttribute(std::make_unique( STUN_ATTR_PRIORITY, prflx_priority)); + if (connection_->field_trials_->enable_goog_ping && + !connection_->remote_support_goog_ping_.has_value()) { + // Check if remote supports GOOG PING by announcing which version we + // support. This is sent on all STUN_BINDING_REQUEST until we get a + // STUN_BINDING_RESPONSE. + auto list = + StunAttribute::CreateUInt16ListAttribute(STUN_ATTR_GOOG_MISC_INFO); + list->AddTypeAtIndex(kSupportGoogPingVersionRequestIndex, kGoogPingVersion); + request->AddAttribute(std::move(list)); + } + if (connection_->ShouldSendGoogPing(request)) { request->SetType(GOOG_PING_REQUEST); request->ClearAttributes(); @@ -647,10 +662,18 @@ void Connection::SendStunBindingResponse(const StunMessage* request) { STUN_ATTR_XOR_MAPPED_ADDRESS, remote_candidate_.address())); if (field_trials_->announce_goog_ping) { - auto list = - StunAttribute::CreateUInt16ListAttribute(STUN_ATTR_GOOG_MISC_INFO); - list->AddTypeAtIndex(kSupportGoogPingVersionIndex, kGoogPingVersion); - response.AddAttribute(std::move(list)); + // Check if request contains a announce-request. + auto goog_misc = request->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO); + if (goog_misc != nullptr && + goog_misc->Size() >= kSupportGoogPingVersionRequestIndex && + // Which version can we handle...currently any >= 1 + goog_misc->GetType(kSupportGoogPingVersionRequestIndex) >= 1) { + auto list = + StunAttribute::CreateUInt16ListAttribute(STUN_ATTR_GOOG_MISC_INFO); + list->AddTypeAtIndex(kSupportGoogPingVersionResponseIndex, + kGoogPingVersion); + response.AddAttribute(std::move(list)); + } } response.AddMessageIntegrity(local_candidate().password()); @@ -1053,12 +1076,18 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request, response->reduced_transaction_id()); if (request->msg()->type() == STUN_BINDING_REQUEST) { - auto goog_misc = response->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO); - if (goog_misc != nullptr && - goog_misc->Size() >= kSupportGoogPingVersionIndex && - goog_misc->GetType(kSupportGoogPingVersionIndex) >= kGoogPingVersion) { - // The remote peer has indicated that it supports GOOG_PING. - remote_support_goog_ping_ = true; + if (!remote_support_goog_ping_.has_value()) { + auto goog_misc = response->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO); + if (goog_misc != nullptr && + goog_misc->Size() >= kSupportGoogPingVersionResponseIndex) { + // The remote peer has indicated that it {does/does not} supports + // GOOG_PING. + remote_support_goog_ping_ = + goog_misc->GetType(kSupportGoogPingVersionResponseIndex) >= + kGoogPingVersion; + } else { + remote_support_goog_ping_ = false; + } } MaybeUpdateLocalCandidate(request, response); @@ -1289,12 +1318,15 @@ bool Connection::TooManyOutstandingPings( } bool Connection::ShouldSendGoogPing(const StunMessage* message) { - if (remote_support_goog_ping_ && cached_stun_binding_ && + if (remote_support_goog_ping_ == true && cached_stun_binding_ && cached_stun_binding_->EqualAttributes(message, [](int type) { // Ignore these attributes. + // NOTE: Consider what to do if adding more content to + // STUN_ATTR_GOOG_MISC_INFO return type != STUN_ATTR_FINGERPRINT && type != STUN_ATTR_MESSAGE_INTEGRITY && - type != STUN_ATTR_RETRANSMIT_COUNT; + type != STUN_ATTR_RETRANSMIT_COUNT && + type != STUN_ATTR_GOOG_MISC_INFO; })) { return true; } diff --git a/p2p/base/p2p_transport_channel_ice_field_trials.h b/p2p/base/p2p_transport_channel_ice_field_trials.h index 95f5bb57f6..e55f7ce918 100644 --- a/p2p/base/p2p_transport_channel_ice_field_trials.h +++ b/p2p/base/p2p_transport_channel_ice_field_trials.h @@ -32,8 +32,9 @@ struct IceFieldTrials { // give us chance to find a better connection before starting. absl::optional initial_select_dampening_ping_received; - // Announce GOOG_PING support in STUN_BINDING_RESPONSE. - bool announce_goog_ping = false; + // Announce GOOG_PING support in STUN_BINDING_RESPONSE if requested + // by peer. + bool announce_goog_ping = true; // Enable sending GOOG_PING if remote announce it. bool enable_goog_ping = false; diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index f203d48cd4..8d6d99e5c1 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -2648,21 +2648,36 @@ TEST_F(PortTest, TestIceLiteConnectivity) { namespace { // Utility function for testing goog ping. -absl::optional GetSupportedGoogPingVersion(const StunMessage* response) { - auto goog_misc = response->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO); +absl::optional GetSupportedGoogPingVersion(const StunMessage* msg) { + auto goog_misc = msg->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO); if (goog_misc == nullptr) { return absl::nullopt; } - if (goog_misc->Size() < - static_cast(cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: - SUPPORT_GOOG_PING_VERSION)) { - return absl::nullopt; + if (msg->type() == STUN_BINDING_REQUEST) { + if (goog_misc->Size() < + static_cast(cricket::IceGoogMiscInfoBindingRequestAttributeIndex:: + SUPPORT_GOOG_PING_VERSION)) { + return absl::nullopt; + } + + return goog_misc->GetType( + static_cast(cricket::IceGoogMiscInfoBindingRequestAttributeIndex:: + SUPPORT_GOOG_PING_VERSION)); } - return goog_misc->GetType( - static_cast(cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: - SUPPORT_GOOG_PING_VERSION)); + if (msg->type() == STUN_BINDING_RESPONSE) { + if (goog_misc->Size() < + static_cast(cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: + SUPPORT_GOOG_PING_VERSION)) { + return absl::nullopt; + } + + return goog_misc->GetType( + static_cast(cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: + SUPPORT_GOOG_PING_VERSION)); + } + return absl::nullopt; } } // namespace @@ -2710,6 +2725,11 @@ TEST_P(GoogPingTest, TestGoogPingAnnounceEnable) { ASSERT_TRUE_WAIT(port1->last_stun_msg() != NULL, kDefaultTimeout); const IceMessage* request1 = port1->last_stun_msg(); + + ASSERT_EQ(trials.enable_goog_ping, + GetSupportedGoogPingVersion(request1) && + GetSupportedGoogPingVersion(request1) >= kGoogPingVersion); + auto* con = port2->CreateConnection(port1->Candidates()[0], cricket::Port::ORIGIN_MESSAGE); con->SetIceFieldTrials(&trials); @@ -2718,8 +2738,8 @@ TEST_P(GoogPingTest, TestGoogPingAnnounceEnable) { // Then check the response matches the settings. const auto* response = port2->last_stun_msg(); - ASSERT_EQ(response->type(), STUN_BINDING_RESPONSE); - ASSERT_EQ(trials.announce_goog_ping, + EXPECT_EQ(response->type(), STUN_BINDING_RESPONSE); + EXPECT_EQ(trials.enable_goog_ping && trials.announce_goog_ping, GetSupportedGoogPingVersion(response) && GetSupportedGoogPingVersion(response) >= kGoogPingVersion); @@ -2741,6 +2761,10 @@ TEST_P(GoogPingTest, TestGoogPingAnnounceEnable) { con->SendGoogPingResponse(request2); } else { ASSERT_EQ(request2->type(), STUN_BINDING_REQUEST); + // If we sent a BINDING with enable, and we got a reply that + // didn't contain announce, the next ping should not contain + // the enable again. + ASSERT_FALSE(GetSupportedGoogPingVersion(request2).has_value()); con->SendStunBindingResponse(request2); } @@ -2757,6 +2781,170 @@ TEST_P(GoogPingTest, TestGoogPingAnnounceEnable) { ch1.Stop(); } +// This test if a someone send a STUN_BINDING with unsupported version +// (kGoogPingVersion == 0) +TEST_F(PortTest, TestGoogPingUnsupportedVersionInStunBinding) { + IceFieldTrials trials; + trials.announce_goog_ping = true; + trials.enable_goog_ping = true; + + auto port1_unique = + CreateTestPort(kLocalAddr1, "lfrag", "lpass", + cricket::ICEROLE_CONTROLLING, kTiebreaker1); + auto* port1 = port1_unique.get(); + auto port2 = CreateTestPort(kLocalAddr2, "rfrag", "rpass", + cricket::ICEROLE_CONTROLLED, kTiebreaker2); + + TestChannel ch1(std::move(port1_unique)); + // Block usage of STUN_ATTR_USE_CANDIDATE so that + // ch1.conn() will sent GOOG_PING_REQUEST directly. + // This only makes test a bit shorter... + ch1.SetIceMode(ICEMODE_LITE); + // Start gathering candidates. + ch1.Start(); + port2->PrepareAddress(); + + ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout); + ASSERT_FALSE(port2->Candidates().empty()); + + ch1.CreateConnection(GetCandidate(port2.get())); + ASSERT_TRUE(ch1.conn() != NULL); + EXPECT_EQ(Connection::STATE_WRITE_INIT, ch1.conn()->write_state()); + ch1.conn()->SetIceFieldTrials(&trials); + + // Send ping. + ch1.Ping(); + + ASSERT_TRUE_WAIT(port1->last_stun_msg() != NULL, kDefaultTimeout); + const IceMessage* request1 = port1->last_stun_msg(); + + ASSERT_TRUE(GetSupportedGoogPingVersion(request1) && + GetSupportedGoogPingVersion(request1) >= kGoogPingVersion); + + // Modify the STUN message request1 to send GetSupportedGoogPingVersion == 0 + auto modified_request1 = request1->Clone(); + ASSERT_TRUE(modified_request1->RemoveAttribute(STUN_ATTR_GOOG_MISC_INFO) != + nullptr); + ASSERT_TRUE(modified_request1->RemoveAttribute(STUN_ATTR_MESSAGE_INTEGRITY) != + nullptr); + { + auto list = + StunAttribute::CreateUInt16ListAttribute(STUN_ATTR_GOOG_MISC_INFO); + list->AddTypeAtIndex( + static_cast( + cricket::IceGoogMiscInfoBindingRequestAttributeIndex:: + SUPPORT_GOOG_PING_VERSION), + /* version */ 0); + modified_request1->AddAttribute(std::move(list)); + modified_request1->AddMessageIntegrity("rpass"); + } + auto* con = port2->CreateConnection(port1->Candidates()[0], + cricket::Port::ORIGIN_MESSAGE); + con->SetIceFieldTrials(&trials); + + con->SendStunBindingResponse(modified_request1.get()); + + // Then check the response matches the settings. + const auto* response = port2->last_stun_msg(); + EXPECT_EQ(response->type(), STUN_BINDING_RESPONSE); + EXPECT_FALSE(GetSupportedGoogPingVersion(response)); + + ch1.Stop(); +} + +// This test if a someone send a STUN_BINDING_RESPONSE with unsupported version +// (kGoogPingVersion == 0) +TEST_F(PortTest, TestGoogPingUnsupportedVersionInStunBindingResponse) { + IceFieldTrials trials; + trials.announce_goog_ping = true; + trials.enable_goog_ping = true; + + auto port1_unique = + CreateTestPort(kLocalAddr1, "lfrag", "lpass", + cricket::ICEROLE_CONTROLLING, kTiebreaker1); + auto* port1 = port1_unique.get(); + auto port2 = CreateTestPort(kLocalAddr2, "rfrag", "rpass", + cricket::ICEROLE_CONTROLLED, kTiebreaker2); + + TestChannel ch1(std::move(port1_unique)); + // Block usage of STUN_ATTR_USE_CANDIDATE so that + // ch1.conn() will sent GOOG_PING_REQUEST directly. + // This only makes test a bit shorter... + ch1.SetIceMode(ICEMODE_LITE); + // Start gathering candidates. + ch1.Start(); + port2->PrepareAddress(); + + ASSERT_EQ_WAIT(1, ch1.complete_count(), kDefaultTimeout); + ASSERT_FALSE(port2->Candidates().empty()); + + ch1.CreateConnection(GetCandidate(port2.get())); + ASSERT_TRUE(ch1.conn() != NULL); + EXPECT_EQ(Connection::STATE_WRITE_INIT, ch1.conn()->write_state()); + ch1.conn()->SetIceFieldTrials(&trials); + + // Send ping. + ch1.Ping(); + + ASSERT_TRUE_WAIT(port1->last_stun_msg() != NULL, kDefaultTimeout); + const IceMessage* request1 = port1->last_stun_msg(); + + ASSERT_TRUE(GetSupportedGoogPingVersion(request1) && + GetSupportedGoogPingVersion(request1) >= kGoogPingVersion); + + auto* con = port2->CreateConnection(port1->Candidates()[0], + cricket::Port::ORIGIN_MESSAGE); + con->SetIceFieldTrials(&trials); + + con->SendStunBindingResponse(request1); + + // Then check the response matches the settings. + const auto* response = port2->last_stun_msg(); + EXPECT_EQ(response->type(), STUN_BINDING_RESPONSE); + EXPECT_TRUE(GetSupportedGoogPingVersion(response)); + + // Modify the STUN message response to contain GetSupportedGoogPingVersion == + // 0 + auto modified_response = response->Clone(); + ASSERT_TRUE(modified_response->RemoveAttribute(STUN_ATTR_GOOG_MISC_INFO) != + nullptr); + ASSERT_TRUE(modified_response->RemoveAttribute(STUN_ATTR_MESSAGE_INTEGRITY) != + nullptr); + ASSERT_TRUE(modified_response->RemoveAttribute(STUN_ATTR_FINGERPRINT) != + nullptr); + { + auto list = + StunAttribute::CreateUInt16ListAttribute(STUN_ATTR_GOOG_MISC_INFO); + list->AddTypeAtIndex( + static_cast( + cricket::IceGoogMiscInfoBindingResponseAttributeIndex:: + SUPPORT_GOOG_PING_VERSION), + /* version */ 0); + modified_response->AddAttribute(std::move(list)); + modified_response->AddMessageIntegrity("rpass"); + modified_response->AddFingerprint(); + } + + rtc::ByteBufferWriter buf; + modified_response->Write(&buf); + + // Feeding the modified respone message back. + ch1.conn()->OnReadPacket(buf.Data(), buf.Length(), /* packet_time_us */ -1); + + port1->Reset(); + port2->Reset(); + + ch1.Ping(); + ASSERT_TRUE_WAIT(port1->last_stun_msg() != NULL, kDefaultTimeout); + + // This should now be a STUN_BINDING...without a kGoogPingVersion + const IceMessage* request2 = port1->last_stun_msg(); + EXPECT_EQ(request2->type(), STUN_BINDING_REQUEST); + EXPECT_FALSE(GetSupportedGoogPingVersion(request2)); + + ch1.Stop(); +} + INSTANTIATE_TEST_SUITE_P(GoogPingTest, GoogPingTest, // test all combinations of pairs.