Enable SRTP GCM ciphers by default

Bug: webrtc:15178
Change-Id: I0216ce8f194fffc820723d82b9c04a76573c2f4f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305381
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Victor Boivie <[email protected]>
Cr-Commit-Position: refs/heads/main@{#40828}

Author: saghul

api/crypto/crypto_options.cc

@@ -23,13 +23,6 @@ CryptoOptions::CryptoOptions(const CryptoOptions& other) {
 
 CryptoOptions::~CryptoOptions() {}
 
-// static
-CryptoOptions CryptoOptions::NoGcm() {
-  CryptoOptions options;
-  options.srtp.enable_gcm_crypto_suites = false;
-  return options;
-}
-
 std::vector<int> CryptoOptions::GetSupportedDtlsSrtpCryptoSuites() const {
   std::vector<int> crypto_suites;
   // Note: kSrtpAes128CmSha1_80 is what is required to be supported (by

api/crypto/crypto_options.h

@@ -25,11 +25,6 @@ struct RTC_EXPORT CryptoOptions {
   CryptoOptions(const CryptoOptions& other);
   ~CryptoOptions();
 
-  // Helper method to return an instance of the CryptoOptions with GCM crypto
-  // suites disabled. This method should be used instead of depending on current
-  // default values set by the constructor.
-  static CryptoOptions NoGcm();
-
   // Returns a list of the supported DTLS-SRTP Crypto suites based on this set
   // of crypto options.
   std::vector<int> GetSupportedDtlsSrtpCryptoSuites() const;
@@ -41,7 +36,7 @@ struct RTC_EXPORT CryptoOptions {
   struct Srtp {
     // Enable GCM crypto suites from RFC 7714 for SRTP. GCM will only be used
     // if both sides enable it.
-    bool enable_gcm_crypto_suites = false;
+    bool enable_gcm_crypto_suites = true;
 
     // If set to true, the (potentially insecure) crypto cipher
     // kSrtpAes128CmSha1_32 will be included in the list of supported ciphers

api/peer_connection_interface.h

@@ -1493,7 +1493,7 @@ class RTC_EXPORT PeerConnectionFactoryInterface
     rtc::SSLProtocolVersion ssl_max_version = rtc::SSL_PROTOCOL_DTLS_12;
 
     // Sets crypto related options, e.g. enabled cipher suites.
-    CryptoOptions crypto_options = CryptoOptions::NoGcm();
+    CryptoOptions crypto_options = {};
   };
 
   // Set the options to be used for subsequently created PeerConnections.

pc/media_session_unittest.cc

@@ -284,6 +284,7 @@ static const char* kMediaProtocolsDtls[] = {
 // default changes.
 static const char* kDefaultSrtpCryptoSuite = kCsAesCm128HmacSha1_80;
 static const char* kDefaultSrtpCryptoSuiteGcm = kCsAeadAes256Gcm;
+static const uint8_t kDefaultCryptoSuiteSize = 3U;
 
 // These constants are used to make the code using "AddMediaDescriptionOptions"
 // more readable.
@@ -622,9 +623,8 @@ class MediaSessionDescriptionFactoryTest : public ::testing::Test {
     ASSERT_TRUE(video_media_desc);
     EXPECT_TRUE(CompareCryptoParams(audio_media_desc->cryptos(),
                                     video_media_desc->cryptos()));
-    EXPECT_EQ(1u, audio_media_desc->cryptos().size());
-    EXPECT_EQ(kDefaultSrtpCryptoSuite,
-              audio_media_desc->cryptos()[0].crypto_suite);
+    ASSERT_CRYPTO(audio_media_desc, offer ? kDefaultCryptoSuiteSize : 1U,
+                  kDefaultSrtpCryptoSuite);
 
     // Verify the selected crypto is one from the reference audio
     // media content.
@@ -819,7 +819,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) {
   EXPECT_EQ(0U, acd->first_ssrc());             // no sender is attached.
   EXPECT_EQ(kAutoBandwidth, acd->bandwidth());  // default bandwidth (auto)
   EXPECT_TRUE(acd->rtcp_mux());                 // rtcp-mux defaults on
-  ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
   EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol());
 }
 
@@ -844,14 +844,14 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateVideoOffer) {
   EXPECT_EQ(0U, acd->first_ssrc());             // no sender is attached
   EXPECT_EQ(kAutoBandwidth, acd->bandwidth());  // default bandwidth (auto)
   EXPECT_TRUE(acd->rtcp_mux());                 // rtcp-mux defaults on
-  ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
   EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol());
   EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type());
   EXPECT_EQ(f1_.video_sendrecv_codecs(), vcd->codecs());
   EXPECT_EQ(0U, vcd->first_ssrc());             // no sender is attached
   EXPECT_EQ(kAutoBandwidth, vcd->bandwidth());  // default bandwidth (auto)
   EXPECT_TRUE(vcd->rtcp_mux());                 // rtcp-mux defaults on
-  ASSERT_CRYPTO(vcd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
   EXPECT_EQ(cricket::kMediaProtocolSavpf, vcd->protocol());
 }
 
@@ -1298,7 +1298,6 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioAnswerGcm) {
   f1_.set_secure(SEC_ENABLED);
   f2_.set_secure(SEC_ENABLED);
   MediaSessionOptions opts = CreatePlanBMediaSessionOptions();
-  opts.crypto_options.srtp.enable_gcm_crypto_suites = true;
   std::unique_ptr<SessionDescription> offer =
       f1_.CreateOfferOrError(opts, nullptr).MoveValue();
   ASSERT_TRUE(offer.get());
@@ -2475,11 +2474,11 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateMultiStreamVideoOffer) {
 
   EXPECT_EQ(kAutoBandwidth, acd->bandwidth());  // default bandwidth (auto)
   EXPECT_TRUE(acd->rtcp_mux());                 // rtcp-mux defaults on
-  ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
 
   EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type());
   EXPECT_EQ(f1_.video_sendrecv_codecs(), vcd->codecs());
-  ASSERT_CRYPTO(vcd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
 
   const StreamParamsVec& video_streams = vcd->streams();
   ASSERT_EQ(1U, video_streams.size());
@@ -2512,9 +2511,9 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateMultiStreamVideoOffer) {
   EXPECT_EQ(acd->codecs(), updated_acd->codecs());
   EXPECT_EQ(vcd->type(), updated_vcd->type());
   EXPECT_EQ(vcd->codecs(), updated_vcd->codecs());
-  ASSERT_CRYPTO(updated_acd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(updated_acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
   EXPECT_TRUE(CompareCryptoParams(acd->cryptos(), updated_acd->cryptos()));
-  ASSERT_CRYPTO(updated_vcd, 1U, kDefaultSrtpCryptoSuite);
+  ASSERT_CRYPTO(updated_vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite);
   EXPECT_TRUE(CompareCryptoParams(vcd->cryptos(), updated_vcd->cryptos()));
 
   const StreamParamsVec& updated_audio_streams = updated_acd->streams();
@@ -3881,8 +3880,8 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCryptoDtls) {
   ASSERT_TRUE(audio_media_desc);
   video_media_desc = offer->GetContentDescriptionByName("video");
   ASSERT_TRUE(video_media_desc);
-  EXPECT_EQ(1u, audio_media_desc->cryptos().size());
-  EXPECT_EQ(1u, video_media_desc->cryptos().size());
+  EXPECT_EQ(kDefaultCryptoSuiteSize, audio_media_desc->cryptos().size());
+  EXPECT_EQ(kDefaultCryptoSuiteSize, video_media_desc->cryptos().size());
 
   audio_trans_desc = offer->GetTransportDescriptionByName("audio");
   ASSERT_TRUE(audio_trans_desc);