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 <hta@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30269}
diff --git a/api/transport/stun.h b/api/transport/stun.h
index 7860da2..41f76a1 100644
--- a/api/transport/stun.h
+++ b/api/transport/stun.h
@@ -678,7 +678,9 @@
// 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 cd5d290..f3692c5 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -143,7 +143,11 @@
// Default field trials.
const cricket::IceFieldTrials kDefaultFieldTrials;
-constexpr int kSupportGoogPingVersionIndex =
+constexpr int kSupportGoogPingVersionRequestIndex =
+ static_cast<int>(cricket::IceGoogMiscInfoBindingRequestAttributeIndex::
+ SUPPORT_GOOG_PING_VERSION);
+
+constexpr int kSupportGoogPingVersionResponseIndex =
static_cast<int>(cricket::IceGoogMiscInfoBindingResponseAttributeIndex::
SUPPORT_GOOG_PING_VERSION);
@@ -224,6 +228,17 @@
request->AddAttribute(std::make_unique<StunUInt32Attribute>(
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 @@
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 @@
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::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 95f5bb5..e55f7ce 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 @@
// give us chance to find a better connection before starting.
absl::optional<int> 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 f203d48..8d6d99e 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -2648,21 +2648,36 @@
namespace {
// Utility function for testing goog ping.
-absl::optional<int> GetSupportedGoogPingVersion(const StunMessage* response) {
- auto goog_misc = response->GetUInt16List(STUN_ATTR_GOOG_MISC_INFO);
+absl::optional<int> 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<int>(cricket::IceGoogMiscInfoBindingResponseAttributeIndex::
- SUPPORT_GOOG_PING_VERSION)) {
- return absl::nullopt;
+ if (msg->type() == STUN_BINDING_REQUEST) {
+ if (goog_misc->Size() <
+ static_cast<int>(cricket::IceGoogMiscInfoBindingRequestAttributeIndex::
+ SUPPORT_GOOG_PING_VERSION)) {
+ return absl::nullopt;
+ }
+
+ return goog_misc->GetType(
+ static_cast<int>(cricket::IceGoogMiscInfoBindingRequestAttributeIndex::
+ SUPPORT_GOOG_PING_VERSION));
}
- return goog_misc->GetType(
- static_cast<int>(cricket::IceGoogMiscInfoBindingResponseAttributeIndex::
- SUPPORT_GOOG_PING_VERSION));
+ if (msg->type() == STUN_BINDING_RESPONSE) {
+ if (goog_misc->Size() <
+ static_cast<int>(cricket::IceGoogMiscInfoBindingResponseAttributeIndex::
+ SUPPORT_GOOG_PING_VERSION)) {
+ return absl::nullopt;
+ }
+
+ return goog_misc->GetType(
+ static_cast<int>(cricket::IceGoogMiscInfoBindingResponseAttributeIndex::
+ SUPPORT_GOOG_PING_VERSION));
+ }
+ return absl::nullopt;
}
} // namespace
@@ -2710,6 +2725,11 @@
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 @@
// 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 @@
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 @@
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<uint16_t>(
+ 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<uint16_t>(
+ 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 <announce, enable> pairs.
