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 <zhihuang@webrtc.org>
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> 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 <zhihuang@webrtc.org>
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23571}diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h
index 757811e..ab9f458 100644
--- a/api/peerconnectioninterface.h
+++ b/api/peerconnectioninterface.h
@@ -577,13 +577,6 @@
// 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 e31b2f5..a452d97 100644
--- a/pc/dtlssrtptransport.cc
+++ b/pc/dtlssrtptransport.cc
@@ -39,10 +39,7 @@
// 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 a2d7aad..c24034a 100644
--- a/pc/dtlssrtptransport.h
+++ b/pc/dtlssrtptransport.h
@@ -53,12 +53,6 @@
"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 @@
// 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
index cfe817f..e1da462 100644
--- a/pc/dtlssrtptransport_unittest.cc
+++ b/pc/dtlssrtptransport_unittest.cc
@@ -510,51 +510,3 @@
// 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
index 7d456f9..d05339e 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -478,8 +478,6 @@
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 6a2e7d2..ebe105b 100644
--- a/pc/jseptransportcontroller.h
+++ b/pc/jseptransportcontroller.h
@@ -77,7 +77,6 @@
// 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 1f792c3..0a984a7 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -693,7 +693,6 @@
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 @@
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 @@
#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(
