Revert "Add a flag to actively reset the SRTP parameters"

This reverts commit bae1031. 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 <[email protected]> > Reviewed-by: Taylor Brandstetter <[email protected]> > Cr-Commit-Position: refs/heads/master@{#23570} [email protected],[email protected] 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 <[email protected]> Commit-Queue: Zhi Huang <[email protected]> Cr-Commit-Position: refs/heads/master@{#23571}
375187588 · Jun 12, 2018 · 6c789e0 · 6c789e0
1 parent bae1031
commit 6c789e0
Show file tree

Hide file tree

Showing 7 changed files with 2 additions and 74 deletions.
diff --git 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
@@ -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
@@ -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<std::vector<int>> send_extension_ids_;
   rtc::Optional<std::vector<int>> recv_extension_ids_;
-
-  bool active_reset_srtp_params_ = false;
 };
 
 }  // namespace webrtc

diff --git 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<FakeDtlsTransport>(
-      "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
-  auto rtcp_dtls1 = rtc::MakeUnique<FakeDtlsTransport>(
-      "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
-  auto rtp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
-      "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
-  auto rtcp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
-      "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
@@ -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
@@ -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
@@ -693,7 +693,6 @@ bool PeerConnectionInterface::RTCConfiguration::operator==(
     webrtc::TurnCustomizer* turn_customizer;
     SdpSemantics sdp_semantics;
     rtc::Optional<rtc::AdapterType> 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(