From 6c789e08d522de2d76b6a3c6fb58b10e79aee403 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Tue, 12 Jun 2018 00:56:02 +0000 Subject: [PATCH] Revert "Add a flag to actively reset the SRTP parameters" This reverts commit bae103126c5bdaf1361bcff4750eb5ebe10020ee. Reason for revert: Merge native code change with Android and Objc wrapper. Original change's description: > Add a flag to actively reset the SRTP parameters > > Add a new flag to RtcConfiguration. By setting that flag to true, the > SRTP parameters will be reset whenever the DTLS transports are reset > after every offer/answer negotiation. > > This should only be used as a workaround for the linked bug, if the > application knows that the other party is affected (for instance, > using a version number). > > Bug: chromium:835958 > Change-Id: Ifb4b99f68dc272507728ab59c07627f0d1b9c605 > Reviewed-on: https://webrtc-review.googlesource.com/81642 > Commit-Queue: Zhi Huang > Reviewed-by: Taylor Brandstetter > Cr-Commit-Position: refs/heads/master@{#23570} TBR=deadbeef@webrtc.org,zhihuang@webrtc.org Change-Id: Ibd7a3b8f45ff8df4af33d758f8fd3e2d5158e8e2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:835958 Reviewed-on: https://webrtc-review.googlesource.com/83080 Reviewed-by: Zhi Huang Commit-Queue: Zhi Huang Cr-Commit-Position: refs/heads/master@{#23571} --- api/peerconnectioninterface.h | 7 ----- pc/dtlssrtptransport.cc | 5 +--- pc/dtlssrtptransport.h | 8 ------ pc/dtlssrtptransport_unittest.cc | 48 -------------------------------- pc/jseptransportcontroller.cc | 2 -- pc/jseptransportcontroller.h | 1 - pc/peerconnection.cc | 5 +--- 7 files changed, 2 insertions(+), 74 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 757811ece4..ab9f458e01 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -577,13 +577,6 @@ class PeerConnectionInterface : public rtc::RefCountInterface { // For all other users, specify kUnifiedPlan. SdpSemantics sdp_semantics = SdpSemantics::kPlanB; - // Actively reset the SRTP parameters whenever the DTLS transports - // underneath are reset for every offer/answer negotiation. - // This is only intended to be a workaround for crbug.com/835958 - // WARNING: This would cause RTP/RTCP packets decryption failure if not used - // correctly. This flag will be deprecated soon. Do not rely on it. - bool active_reset_srtp_params = false; - // // Don't forget to update operator== if adding something. // diff --git a/pc/dtlssrtptransport.cc b/pc/dtlssrtptransport.cc index e31b2f5872..a452d970d5 100644 --- a/pc/dtlssrtptransport.cc +++ b/pc/dtlssrtptransport.cc @@ -39,10 +39,7 @@ void DtlsSrtpTransport::SetDtlsTransports( // When using DTLS-SRTP, we must reset the SrtpTransport every time the // DtlsTransport changes and wait until the DTLS handshake is complete to set // the newly negotiated parameters. - // If |active_reset_srtp_params_| is true, intentionally reset the SRTP - // parameter even though the DtlsTransport may not change. - if (IsSrtpActive() && (rtp_dtls_transport != rtp_dtls_transport_ || - active_reset_srtp_params_)) { + if (IsSrtpActive() && rtp_dtls_transport != rtp_dtls_transport_) { ResetParams(); } diff --git a/pc/dtlssrtptransport.h b/pc/dtlssrtptransport.h index a2d7aadf67..c24034a146 100644 --- a/pc/dtlssrtptransport.h +++ b/pc/dtlssrtptransport.h @@ -53,12 +53,6 @@ class DtlsSrtpTransport : public SrtpTransport { "Set SRTP keys for DTLS-SRTP is not supported."); } - // If |active_reset_srtp_params_| is set to be true, the SRTP parameters will - // be reset whenever the DtlsTransports are reset. - void SetActiveResetSrtpParams(bool active_reset_srtp_params) { - active_reset_srtp_params_ = active_reset_srtp_params; - } - private: bool IsDtlsActive(); bool IsDtlsConnected(); @@ -90,8 +84,6 @@ class DtlsSrtpTransport : public SrtpTransport { // The encrypted header extension IDs. rtc::Optional> send_extension_ids_; rtc::Optional> recv_extension_ids_; - - bool active_reset_srtp_params_ = false; }; } // namespace webrtc diff --git a/pc/dtlssrtptransport_unittest.cc b/pc/dtlssrtptransport_unittest.cc index cfe817f3f8..e1da4628ad 100644 --- a/pc/dtlssrtptransport_unittest.cc +++ b/pc/dtlssrtptransport_unittest.cc @@ -510,51 +510,3 @@ TEST_F(DtlsSrtpTransportTest, SrtpSessionNotResetWhenRtcpTransportRemoved) { // errors when a packet with a duplicate SRTCP index is received. SendRecvRtcpPackets(); } - -// Tests that RTCP packets can be sent and received if both sides actively reset -// the SRTP parameters with the |active_reset_srtp_params_| flag. -TEST_F(DtlsSrtpTransportTest, ActivelyResetSrtpParams) { - auto rtp_dtls1 = rtc::MakeUnique( - "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP); - auto rtcp_dtls1 = rtc::MakeUnique( - "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP); - auto rtp_dtls2 = rtc::MakeUnique( - "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP); - auto rtcp_dtls2 = rtc::MakeUnique( - "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP); - - MakeDtlsSrtpTransports(rtp_dtls1.get(), rtcp_dtls1.get(), rtp_dtls2.get(), - rtcp_dtls2.get(), /*rtcp_mux_enabled=*/true); - CompleteDtlsHandshake(rtp_dtls1.get(), rtp_dtls2.get()); - CompleteDtlsHandshake(rtcp_dtls1.get(), rtcp_dtls2.get()); - - // Send some RTCP packets, causing the SRTCP index to be incremented. - SendRecvRtcpPackets(); - - // Only set the |active_reset_srtp_params_| flag to be true one side. - dtls_srtp_transport1_->SetActiveResetSrtpParams(true); - // Set RTCP transport to null to trigger the SRTP parameters update. - dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr); - dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr); - - // Sending some RTCP packets. - size_t rtcp_len = sizeof(kRtcpReport); - size_t packet_size = rtcp_len + 4 + kRtpAuthTagLen; - rtc::Buffer rtcp_packet_buffer(packet_size); - rtc::CopyOnWriteBuffer rtcp_packet(kRtcpReport, rtcp_len, packet_size); - int prev_received_packets = transport_observer2_.rtcp_count(); - ASSERT_TRUE(dtls_srtp_transport1_->SendRtcpPacket( - &rtcp_packet, rtc::PacketOptions(), cricket::PF_SRTP_BYPASS)); - // The RTCP packet is not exepected to be received because the SRTP parameters - // are only reset on one side and the SRTCP index is out of sync. - EXPECT_EQ(prev_received_packets, transport_observer2_.rtcp_count()); - - // Set the flag to be true on the other side. - dtls_srtp_transport2_->SetActiveResetSrtpParams(true); - // Set RTCP transport to null to trigger the SRTP parameters update. - dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr); - dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr); - - // RTCP packets flow is expected to work just fine. - SendRecvRtcpPackets(); -} diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 7d456f9601..d05339e1a1 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -478,8 +478,6 @@ JsepTransportController::CreateDtlsSrtpTransport( dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport, rtcp_dtls_transport); - dtls_srtp_transport->SetActiveResetSrtpParams( - config_.active_reset_srtp_params); return dtls_srtp_transport; } diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index 6a2e7d2b6d..ebe105b510 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -77,7 +77,6 @@ class JsepTransportController : public sigslot::has_slots<>, // Used to inject the ICE/DTLS transports created externally. cricket::TransportFactoryInterface* external_transport_factory = nullptr; Observer* transport_observer = nullptr; - bool active_reset_srtp_params = false; }; // The ICE related events are signaled on the |signaling_thread|. diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 1f792c3e71..0a984a703f 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -693,7 +693,6 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( webrtc::TurnCustomizer* turn_customizer; SdpSemantics sdp_semantics; rtc::Optional network_preference; - bool active_reset_srtp_params; }; static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this), "Did you add something to RTCConfiguration and forget to " @@ -740,8 +739,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( ice_regather_interval_range == o.ice_regather_interval_range && turn_customizer == o.turn_customizer && sdp_semantics == o.sdp_semantics && - network_preference == o.network_preference && - active_reset_srtp_params == o.active_reset_srtp_params; + network_preference == o.network_preference; } bool PeerConnectionInterface::RTCConfiguration::operator!=( @@ -939,7 +937,6 @@ bool PeerConnection::Initialize( #if defined(ENABLE_EXTERNAL_AUTH) config.enable_external_auth = true; #endif - config.active_reset_srtp_params = configuration.active_reset_srtp_params; transport_controller_.reset(new JsepTransportController( signaling_thread(), network_thread(), port_allocator_.get(), config)); transport_controller_->SignalIceConnectionState.connect(