From 321ec3ba99349271d631f35b79301fe4538cb36f Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 10 Feb 2022 13:42:18 +0000 Subject: [PATCH] Add tests for SSL role and datachannel ID assignment. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#35973} --- pc/data_channel_integrationtest.cc | 86 ++++++++++++++++++++++++++++++ pc/dtls_transport.cc | 4 +- pc/dtls_transport_unittest.cc | 1 + pc/peer_connection.cc | 11 +++- 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/pc/data_channel_integrationtest.cc b/pc/data_channel_integrationtest.cc index c70078b0df..005f2e7ae7 100644 --- a/pc/data_channel_integrationtest.cc +++ b/pc/data_channel_integrationtest.cc @@ -86,6 +86,13 @@ class DataChannelIntegrationTestUnifiedPlan : 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 @@ TEST_P(DataChannelIntegrationTest, ClosingConnectionStopsPacketFlow) { 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 5cee54c98e..e8d6ae9b6a 100644 --- a/pc/dtls_transport.cc +++ b/pc/dtls_transport.cc @@ -114,10 +114,10 @@ void DtlsTransport::UpdateInformation() { 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 477133ee0b..1400ff94c2 100644 --- a/pc/dtls_transport_unittest.cc +++ b/pc/dtls_transport_unittest.cc @@ -130,6 +130,7 @@ TEST_F(DtlsTransportTest, RoleAppearsOnConnect) { 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 b80725f21f..5c7177be68 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2168,11 +2168,18 @@ bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) { 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; }