Add tests for SSL role and datachannel ID assignment.
Document with a comment the suspected place that could cause a bug.
Also fix an error in previous role observation code.
Bug: webrtc:13668
Change-Id: Id7f6af6905d90f7974b5570145c201c8339aaf72
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251388
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35973}
diff --git a/pc/data_channel_integrationtest.cc b/pc/data_channel_integrationtest.cc
index c70078b..005f2e7 100644
--- a/pc/data_channel_integrationtest.cc
+++ b/pc/data_channel_integrationtest.cc
@@ -86,6 +86,13 @@
: PeerConnectionIntegrationBaseTest(SdpSemantics::kUnifiedPlan) {}
};
+void MakeActiveSctpOffer(cricket::SessionDescription* desc) {
+ auto& transport_infos = desc->transport_infos();
+ for (auto& transport_info : transport_infos) {
+ transport_info.description.connection_role = cricket::CONNECTIONROLE_ACTIVE;
+ }
+}
+
// This test causes a PeerConnection to enter Disconnected state, and
// sends data on a DataChannel while disconnected.
// The data should be surfaced when the connection reestablishes.
@@ -577,6 +584,85 @@
EXPECT_EQ(sent_packets_a, sent_packets_b);
}
+TEST_P(DataChannelIntegrationTest, DtlsRoleIsSetNormally) {
+ ASSERT_TRUE(CreatePeerConnectionWrappers());
+ ConnectFakeSignaling();
+ caller()->CreateDataChannel();
+ ASSERT_FALSE(caller()->pc()->GetSctpTransport());
+ caller()->CreateAndSetAndSignalOffer();
+ ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+ ASSERT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout);
+ ASSERT_TRUE(caller()->pc()->GetSctpTransport());
+ ASSERT_TRUE(
+ caller()->pc()->GetSctpTransport()->Information().dtls_transport());
+ EXPECT_TRUE(caller()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role());
+ EXPECT_EQ(caller()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role(),
+ DtlsTransportTlsRole::kServer);
+ EXPECT_EQ(callee()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role(),
+ DtlsTransportTlsRole::kClient);
+ // ID should be assigned according to the odd/even rule based on role; client
+ // gets even numbers, server gets odd ones.
+ // RFC 8832 section 6.
+ // TODO(hta): Test multiple channels.
+ EXPECT_EQ(caller()->data_channel()->id(), 1);
+}
+
+TEST_P(DataChannelIntegrationTest, DtlsRoleIsSetWhenReversed) {
+ ASSERT_TRUE(CreatePeerConnectionWrappers());
+ ConnectFakeSignaling();
+ caller()->CreateDataChannel();
+ callee()->SetReceivedSdpMunger(MakeActiveSctpOffer);
+ caller()->CreateAndSetAndSignalOffer();
+ ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+ ASSERT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout);
+ EXPECT_TRUE(caller()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role());
+ EXPECT_EQ(caller()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role(),
+ DtlsTransportTlsRole::kClient);
+ EXPECT_EQ(callee()
+ ->pc()
+ ->GetSctpTransport()
+ ->Information()
+ .dtls_transport()
+ ->Information()
+ .role(),
+ DtlsTransportTlsRole::kServer);
+ // ID should be assigned according to the odd/even rule based on role; client
+ // gets even numbers, server gets odd ones.
+ // RFC 8832 section 6.
+ // TODO(hta): Test multiple channels.
+ EXPECT_EQ(caller()->data_channel()->id(), 0);
+}
+
// Test that transport stats are generated by the RTCStatsCollector for a
// connection that only involves data channels. This is a regression test for
// crbug.com/826972.
diff --git a/pc/dtls_transport.cc b/pc/dtls_transport.cc
index 5cee54c..e8d6ae9 100644
--- a/pc/dtls_transport.cc
+++ b/pc/dtls_transport.cc
@@ -114,10 +114,10 @@
if (success) {
switch (internal_role) {
case rtc::SSL_CLIENT:
- role = DtlsTransportTlsRole::kServer;
+ role = DtlsTransportTlsRole::kClient;
break;
case rtc::SSL_SERVER:
- role = DtlsTransportTlsRole::kClient;
+ role = DtlsTransportTlsRole::kServer;
break;
}
}
diff --git a/pc/dtls_transport_unittest.cc b/pc/dtls_transport_unittest.cc
index 477133e..1400ff9 100644
--- a/pc/dtls_transport_unittest.cc
+++ b/pc/dtls_transport_unittest.cc
@@ -130,6 +130,7 @@
kDefaultTimeout);
EXPECT_TRUE(observer_.info_.role());
EXPECT_TRUE(transport()->Information().role());
+ EXPECT_EQ(transport()->Information().role(), DtlsTransportTlsRole::kClient);
}
TEST_F(DtlsTransportTest, CertificateAppearsOnConnect) {
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index b80725f..5c7177b 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2168,11 +2168,18 @@
return transport_controller_->GetDtlsRole(*sctp_mid_n_);
});
if (!dtls_role && sdp_handler_->is_caller().has_value()) {
+ // This works fine if we are the offerer, but can be a mistake if
+ // we are the answerer and the remote offer is PASSIVE. In that
+ // case, we will guess the role wrong.
+ // TODO(bugs.webrtc.org/13668): Check if this actually happens.
+ RTC_LOG(LS_ERROR) << "Possible risk: DTLS role guesser is active";
dtls_role =
*sdp_handler_->is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
}
- *role = *dtls_role;
- return true;
+ if (dtls_role) {
+ *role = *dtls_role;
+ return true;
+ }
}
return false;
}
