Negotiate the same SRTP crypto suites for every DTLS association formed.
Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A full set for audio m= sections.
- The full set, minus SRTP_AES128_CM_SHA1_32 for video m= sections.
However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media. If
video is "bundled on" the audio transport (which is typical), it
will actually end up using SRTP_AES128_CM_SHA1_32.
So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.
This fixes an issue (see linked bug) that was occurring when audio/video
were "bundled onto" the data transport. Since the data transport
wasn't negotiating any SRTP crypto suites, none were available to use
for audio/video, so the application would get black video/no audio.
This CL doesn't affect the SDES SRTP crypto suite negotiation;
it only affects the negotiation in the DLTS handshake, through
the use_srtp extension.
BUG=chromium:711243
Review-Url: https://codereview.webrtc.org/2815513012
Cr-Commit-Position: refs/heads/master@{#17810}
diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc
index 99cca88..baacac8 100644
--- a/webrtc/pc/channel_unittest.cc
+++ b/webrtc/pc/channel_unittest.cc
@@ -97,10 +97,9 @@
SECURE = 0x4,
SSRC_MUX = 0x8,
DTLS = 0x10,
- GCM_CIPHER = 0x20,
// Use BaseChannel with PacketTransportInternal rather than
// DtlsTransportInternal.
- RAW_PACKET_TRANSPORT = 0x40,
+ RAW_PACKET_TRANSPORT = 0x20,
};
ChannelTest(bool verify_playout,
@@ -263,9 +262,6 @@
worker_thread, network_thread, signaling_thread, engine, ch,
cricket::CN_AUDIO, (flags & RTCP_MUX_REQUIRED) != 0,
(flags & SECURE) != 0);
- rtc::CryptoOptions crypto_options;
- crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0;
- channel->SetCryptoOptions(crypto_options);
if (!channel->NeedsRtcpTransport()) {
fake_rtcp_dtls_transport = nullptr;
}
@@ -469,25 +465,6 @@
bool CheckNoRtcp2() {
return media_channel2_->CheckNoRtcp();
}
- // Checks that the channel is using GCM iff GCM_CIPHER is set in flags.
- // Returns true if so.
- bool CheckGcmCipher(typename T::Channel* channel, int flags) {
- int suite;
- cricket::FakeDtlsTransport* transport =
- (channel == channel1_.get()) ? fake_rtp_dtls_transport1_.get()
- : fake_rtp_dtls_transport2_.get();
- RTC_DCHECK(transport);
- if (!transport->GetSrtpCryptoSuite(&suite)) {
- return false;
- }
-
- if (flags & GCM_CIPHER) {
- return rtc::IsGcmCryptoSuite(suite);
- } else {
- return (suite != rtc::SRTP_INVALID_CRYPTO_SUITE &&
- !rtc::IsGcmCryptoSuite(suite));
- }
- }
void CreateContent(int flags,
const cricket::AudioCodec& audio_codec,
@@ -1339,13 +1316,10 @@
}
// Test that we properly send SRTP with RTCP in both directions.
- // You can pass in DTLS, RTCP_MUX, GCM_CIPHER and RAW_PACKET_TRANSPORT as
- // flags.
+ // You can pass in DTLS, RTCP_MUX, and RAW_PACKET_TRANSPORT as flags.
void SendSrtpToSrtp(int flags1_in = 0, int flags2_in = 0) {
- RTC_CHECK((flags1_in &
- ~(RTCP_MUX | DTLS | GCM_CIPHER | RAW_PACKET_TRANSPORT)) == 0);
- RTC_CHECK((flags2_in &
- ~(RTCP_MUX | DTLS | GCM_CIPHER | RAW_PACKET_TRANSPORT)) == 0);
+ RTC_CHECK((flags1_in & ~(RTCP_MUX | DTLS | RAW_PACKET_TRANSPORT)) == 0);
+ RTC_CHECK((flags2_in & ~(RTCP_MUX | DTLS | RAW_PACKET_TRANSPORT)) == 0);
int flags1 = SECURE | flags1_in;
int flags2 = SECURE | flags2_in;
@@ -1363,14 +1337,6 @@
EXPECT_TRUE(channel2_->secure());
EXPECT_EQ(dtls1 && dtls2, channel1_->secure_dtls());
EXPECT_EQ(dtls1 && dtls2, channel2_->secure_dtls());
- // We can only query the negotiated cipher suite for DTLS-SRTP transport
- // channels.
- if (dtls1 && dtls2) {
- // A GCM cipher is only used if both channels support GCM ciphers.
- int common_gcm_flags = flags1 & flags2 & GCM_CIPHER;
- EXPECT_TRUE(CheckGcmCipher(channel1_.get(), common_gcm_flags));
- EXPECT_TRUE(CheckGcmCipher(channel2_.get(), common_gcm_flags));
- }
SendRtp1();
SendRtp2();
SendRtcp1();
@@ -2124,9 +2090,6 @@
cricket::VideoChannel* channel = new cricket::VideoChannel(
worker_thread, network_thread, signaling_thread, ch, cricket::CN_VIDEO,
(flags & RTCP_MUX_REQUIRED) != 0, (flags & SECURE) != 0);
- rtc::CryptoOptions crypto_options;
- crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0;
- channel->SetCryptoOptions(crypto_options);
if (!channel->NeedsRtcpTransport()) {
fake_rtcp_dtls_transport = nullptr;
}
@@ -2349,18 +2312,6 @@
Base::SendSrtpToSrtp(DTLS, DTLS);
}
-TEST_F(VoiceChannelSingleThreadTest, SendDtlsSrtpToDtlsSrtpGcmBoth) {
- Base::SendSrtpToSrtp(DTLS | GCM_CIPHER, DTLS | GCM_CIPHER);
-}
-
-TEST_F(VoiceChannelSingleThreadTest, SendDtlsSrtpToDtlsSrtpGcmOne) {
- Base::SendSrtpToSrtp(DTLS | GCM_CIPHER, DTLS);
-}
-
-TEST_F(VoiceChannelSingleThreadTest, SendDtlsSrtpToDtlsSrtpGcmTwo) {
- Base::SendSrtpToSrtp(DTLS, DTLS | GCM_CIPHER);
-}
-
TEST_F(VoiceChannelSingleThreadTest, SendDtlsSrtpToDtlsSrtpRtcpMux) {
Base::SendSrtpToSrtp(DTLS | RTCP_MUX, DTLS | RTCP_MUX);
}
@@ -2682,18 +2633,6 @@
Base::SendSrtpToSrtp(DTLS, DTLS);
}
-TEST_F(VoiceChannelDoubleThreadTest, SendDtlsSrtpToDtlsSrtpGcmBoth) {
- Base::SendSrtpToSrtp(DTLS | GCM_CIPHER, DTLS | GCM_CIPHER);
-}
-
-TEST_F(VoiceChannelDoubleThreadTest, SendDtlsSrtpToDtlsSrtpGcmOne) {
- Base::SendSrtpToSrtp(DTLS | GCM_CIPHER, DTLS);
-}
-
-TEST_F(VoiceChannelDoubleThreadTest, SendDtlsSrtpToDtlsSrtpGcmTwo) {
- Base::SendSrtpToSrtp(DTLS, DTLS | GCM_CIPHER);
-}
-
TEST_F(VoiceChannelDoubleThreadTest, SendDtlsSrtpToDtlsSrtpRtcpMux) {
Base::SendSrtpToSrtp(DTLS | RTCP_MUX, DTLS | RTCP_MUX);
}
@@ -3368,9 +3307,6 @@
cricket::RtpDataChannel* channel = new cricket::RtpDataChannel(
worker_thread, network_thread, signaling_thread, ch, cricket::CN_DATA,
(flags & RTCP_MUX_REQUIRED) != 0, (flags & SECURE) != 0);
- rtc::CryptoOptions crypto_options;
- crypto_options.enable_gcm_crypto_suites = (flags & GCM_CIPHER) != 0;
- channel->SetCryptoOptions(crypto_options);
if (!channel->NeedsRtcpTransport()) {
fake_rtcp_dtls_transport = nullptr;
}
