Improve testing of SRTP external auth code paths.

webrtc/pc/srtpfilter_unittest.cc

 /*
  *  Copyright 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 89 matching lines @@
                      const std::vector<CryptoParams>& params2) {
     EXPECT_TRUE(f1_.SetOffer(params1, CS_LOCAL));
     EXPECT_TRUE(f2_.SetOffer(params1, CS_REMOTE));
     EXPECT_FALSE(f1_.IsActive());
     EXPECT_FALSE(f2_.IsActive());
     EXPECT_TRUE(f2_.SetAnswer(params2, CS_LOCAL));
     EXPECT_TRUE(f1_.SetAnswer(params2, CS_REMOTE));
     EXPECT_TRUE(f1_.IsActive());
     EXPECT_TRUE(f2_.IsActive());
   }
+  void TestRtpAuthParams(cricket::SrtpFilter* filter, const std::string cs) {

[Taylor Brandstetter, 2017/03/03 02:20:35]

nit: Think you meant to use const ref.

[joachim, 2017/03/03 20:42:57]

Done.

+    int overhead;
+    EXPECT_TRUE(filter->GetSrtpOverhead(&overhead));
+    switch (rtc::SrtpCryptoSuiteFromName(cs)) {
+      case rtc::SRTP_AES128_CM_SHA1_32:
+        EXPECT_EQ(32/8, overhead);  // 32-bit tag.
+        break;
+      case rtc::SRTP_AES128_CM_SHA1_80:
+        EXPECT_EQ(80/8, overhead);  // 80-bit tag.
+        break;
+      default:
+        RTC_NOTREACHED();
+        break;
+    }
+
+    uint8_t* auth_key = nullptr;
+    int key_len = 0;
+    int tag_len = 0;
+    EXPECT_TRUE(filter->GetRtpAuthParams(&auth_key, &key_len, &tag_len));
+    EXPECT_NE(nullptr, auth_key);
+    EXPECT_EQ(160/8, key_len);  // Length of SHA-1 is 160 bits.
+    EXPECT_EQ(overhead, tag_len);
+  }
   void TestProtectUnprotect(const std::string& cs1, const std::string& cs2) {
     rtc::Buffer rtp_buffer(sizeof(kPcmuFrame) + rtp_auth_tag_len(cs1));
     char* rtp_packet = rtp_buffer.data<char>();
     char original_rtp_packet[sizeof(kPcmuFrame)];
     rtc::Buffer rtcp_buffer(sizeof(kRtcpReport) + 4 + rtcp_auth_tag_len(cs2));
     char* rtcp_packet = rtcp_buffer.data<char>();
     int rtp_len = sizeof(kPcmuFrame), rtcp_len = sizeof(kRtcpReport), out_len;
     memcpy(rtp_packet, kPcmuFrame, rtp_len);
     // In order to be able to run this test function multiple times we can not
     // use the same sequence number twice. Increase the sequence number by one.
     rtc::SetBE16(reinterpret_cast<uint8_t*>(rtp_packet) + 2,
                  ++sequence_number_);
     memcpy(original_rtp_packet, rtp_packet, rtp_len);
     memcpy(rtcp_packet, kRtcpReport, rtcp_len);
 
     EXPECT_TRUE(f1_.ProtectRtp(rtp_packet, rtp_len,
                                static_cast<int>(rtp_buffer.size()),
                                &out_len));
     EXPECT_EQ(out_len, rtp_len + rtp_auth_tag_len(cs1));
     EXPECT_NE(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
-    EXPECT_TRUE(f2_.UnprotectRtp(rtp_packet, out_len, &out_len));
-    EXPECT_EQ(rtp_len, out_len);
-    EXPECT_EQ(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
+    if (!f1_.IsExternalAuthActive()) {
+      EXPECT_TRUE(f2_.UnprotectRtp(rtp_packet, out_len, &out_len));
+      EXPECT_EQ(rtp_len, out_len);
+      EXPECT_EQ(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
+    } else {
+      TestRtpAuthParams(&f1_, cs1);
+    }
 
     EXPECT_TRUE(f2_.ProtectRtp(rtp_packet, rtp_len,
                                static_cast<int>(rtp_buffer.size()),
                                &out_len));
     EXPECT_EQ(out_len, rtp_len + rtp_auth_tag_len(cs2));
     EXPECT_NE(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
-    EXPECT_TRUE(f1_.UnprotectRtp(rtp_packet, out_len, &out_len));
-    EXPECT_EQ(rtp_len, out_len);
-    EXPECT_EQ(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
+    if (!f2_.IsExternalAuthActive()) {
+      EXPECT_TRUE(f1_.UnprotectRtp(rtp_packet, out_len, &out_len));
+      EXPECT_EQ(rtp_len, out_len);
+      EXPECT_EQ(0, memcmp(rtp_packet, original_rtp_packet, rtp_len));
+    } else {
+      TestRtpAuthParams(&f2_, cs2);

[Taylor Brandstetter, 2017/03/03 02:20:35]

Can you leave a comment explaining why this method is called when external auth
is enabled?

[joachim, 2017/03/03 20:42:57]

Done.

+    }
 
     EXPECT_TRUE(f1_.ProtectRtcp(rtcp_packet, rtcp_len,
                                 static_cast<int>(rtcp_buffer.size()),
                                 &out_len));
     EXPECT_EQ(out_len, rtcp_len + 4 + rtcp_auth_tag_len(cs1));  // NOLINT
     EXPECT_NE(0, memcmp(rtcp_packet, kRtcpReport, rtcp_len));
     EXPECT_TRUE(f2_.UnprotectRtcp(rtcp_packet, out_len, &out_len));
     EXPECT_EQ(rtcp_len, out_len);
     EXPECT_EQ(0, memcmp(rtcp_packet, kRtcpReport, rtcp_len));
 
     EXPECT_TRUE(f2_.ProtectRtcp(rtcp_packet, rtcp_len,
                                 static_cast<int>(rtcp_buffer.size()),
                                 &out_len));
     EXPECT_EQ(out_len, rtcp_len + 4 + rtcp_auth_tag_len(cs2));  // NOLINT
     EXPECT_NE(0, memcmp(rtcp_packet, kRtcpReport, rtcp_len));
     EXPECT_TRUE(f1_.UnprotectRtcp(rtcp_packet, out_len, &out_len));
     EXPECT_EQ(rtcp_len, out_len);
     EXPECT_EQ(0, memcmp(rtcp_packet, kRtcpReport, rtcp_len));
   }
+  void TestProtectSetParamsDirect(bool allow_external_auth, int cs,
+      const uint8_t* key1, int key1_len, const uint8_t* key2, int key2_len,
+      const std::string& cs_name) {
+    EXPECT_EQ(key1_len, key2_len);
+    EXPECT_EQ(cs_name, rtc::SrtpCryptoSuiteToName(cs));
+    f1_.AllowExternalAuthForTest(allow_external_auth);
+    f2_.AllowExternalAuthForTest(allow_external_auth);
+    EXPECT_TRUE(f1_.SetRtpParams(cs, key1, key1_len, cs, key2, key2_len));
+    EXPECT_TRUE(f2_.SetRtpParams(cs, key2, key2_len, cs, key1, key1_len));
+    EXPECT_TRUE(f1_.SetRtcpParams(cs, key1, key1_len, cs, key2, key2_len));
+    EXPECT_TRUE(f2_.SetRtcpParams(cs, key2, key2_len, cs, key1, key1_len));
+    EXPECT_TRUE(f1_.IsActive());
+    EXPECT_TRUE(f2_.IsActive());
+    if (rtc::IsGcmCryptoSuite(cs)) {
+      EXPECT_FALSE(f1_.IsExternalAuthActive());
+      EXPECT_FALSE(f2_.IsExternalAuthActive());
+    } else if (allow_external_auth) {
+      EXPECT_TRUE(f1_.IsExternalAuthActive());
+      EXPECT_TRUE(f2_.IsExternalAuthActive());
+    }
+    TestProtectUnprotect(cs_name, cs_name);
+  }
   cricket::SrtpFilter f1_;
   cricket::SrtpFilter f2_;
   int sequence_number_;
 };
 
 // Test that we can set up the session and keys properly.
 TEST_F(SrtpFilterTest, TestGoodSetupOneCipherSuite) {
   EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
   EXPECT_FALSE(f1_.IsActive());
   EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams2), CS_REMOTE));
@@ skipping 371 matching lines @@
   // Complete the negotiation.
   EXPECT_TRUE(f2_.SetAnswer(answer, CS_LOCAL));
   EXPECT_TRUE(f1_.SetAnswer(answer, CS_REMOTE));
 
   EXPECT_FALSE(f1_.IsActive());
   EXPECT_FALSE(f2_.IsActive());
 }
 
 // Test directly setting the params with AES_CM_128_HMAC_SHA1_80.
 TEST_F(SrtpFilterTest, TestProtect_SetParamsDirect_AES_CM_128_HMAC_SHA1_80) {
-  EXPECT_TRUE(f1_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey1,
-                               kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_80,
-                               kTestKey2, kTestKeyLen));
-  EXPECT_TRUE(f2_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey2,
-                               kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_80,
-                               kTestKey1, kTestKeyLen));
-  EXPECT_TRUE(f1_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey1,
-                                kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_80,
-                                kTestKey2, kTestKeyLen));
-  EXPECT_TRUE(f2_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey2,
-                                kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_80,
-                                kTestKey1, kTestKeyLen));
-  EXPECT_TRUE(f1_.IsActive());
-  EXPECT_TRUE(f2_.IsActive());
-#if defined(ENABLE_EXTERNAL_AUTH)
-  EXPECT_TRUE(f1_.IsExternalAuthActive());
-  EXPECT_TRUE(f2_.IsExternalAuthActive());
-#endif
-  TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_80, CS_AES_CM_128_HMAC_SHA1_80);
+  TestProtectSetParamsDirect(false, rtc::SRTP_AES128_CM_SHA1_80,

[Taylor Brandstetter, 2017/03/03 02:20:35]

nit: We often do this:

bool external_auth = false;
TestSetParamsDirect(external_auth, ...

To make it more clear what the boolean parameter means.

Also, you don't have to do it, but this would be a good place to use
parameterized tests: 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

[joachim, 2017/03/03 20:42:57]

Done.

+      kTestKey1, kTestKeyLen, kTestKey2, kTestKeyLen,
+      CS_AES_CM_128_HMAC_SHA1_80);
+}
+
+TEST_F(SrtpFilterTest,
+    TestProtect_SetParamsDirect_AES_CM_128_HMAC_SHA1_80_ExternalAuth) {
+  TestProtectSetParamsDirect(true, rtc::SRTP_AES128_CM_SHA1_80,
+      kTestKey1, kTestKeyLen, kTestKey2, kTestKeyLen,
+      CS_AES_CM_128_HMAC_SHA1_80);
 }
 
 // Test directly setting the params with AES_CM_128_HMAC_SHA1_32.
 TEST_F(SrtpFilterTest, TestProtect_SetParamsDirect_AES_CM_128_HMAC_SHA1_32) {
-  EXPECT_TRUE(f1_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey1,
-                               kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
-                               kTestKey2, kTestKeyLen));
-  EXPECT_TRUE(f2_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey2,
-                               kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
-                               kTestKey1, kTestKeyLen));
-  EXPECT_TRUE(f1_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey1,
-                                kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
-                                kTestKey2, kTestKeyLen));
-  EXPECT_TRUE(f2_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey2,
-                                kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
-                                kTestKey1, kTestKeyLen));
-  EXPECT_TRUE(f1_.IsActive());
-  EXPECT_TRUE(f2_.IsActive());
-#if defined(ENABLE_EXTERNAL_AUTH)
-  EXPECT_TRUE(f1_.IsExternalAuthActive());
-  EXPECT_TRUE(f2_.IsExternalAuthActive());
-#endif
-  TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_32, CS_AES_CM_128_HMAC_SHA1_32);
+  TestProtectSetParamsDirect(false, rtc::SRTP_AES128_CM_SHA1_32,
+      kTestKey1, kTestKeyLen, kTestKey2, kTestKeyLen,
+      CS_AES_CM_128_HMAC_SHA1_32);
+}
+
+TEST_F(SrtpFilterTest,
+    TestProtect_SetParamsDirect_AES_CM_128_HMAC_SHA1_32_ExternalAuth) {
+  TestProtectSetParamsDirect(true, rtc::SRTP_AES128_CM_SHA1_32,
+      kTestKey1, kTestKeyLen, kTestKey2, kTestKeyLen,
+      CS_AES_CM_128_HMAC_SHA1_32);
 }
 
 // Test directly setting the params with SRTP_AEAD_AES_128_GCM.
 TEST_F(SrtpFilterTest, TestProtect_SetParamsDirect_SRTP_AEAD_AES_128_GCM) {
-  EXPECT_TRUE(f1_.SetRtpParams(rtc::SRTP_AEAD_AES_128_GCM, kTestKeyGcm128_1,
-                               kTestKeyGcm128Len, rtc::SRTP_AEAD_AES_128_GCM,
-                               kTestKeyGcm128_2, kTestKeyGcm128Len));
-  EXPECT_TRUE(f2_.SetRtpParams(rtc::SRTP_AEAD_AES_128_GCM, kTestKeyGcm128_2,
-                               kTestKeyGcm128Len, rtc::SRTP_AEAD_AES_128_GCM,
-                               kTestKeyGcm128_1, kTestKeyGcm128Len));
-  EXPECT_TRUE(f1_.SetRtcpParams(rtc::SRTP_AEAD_AES_128_GCM, kTestKeyGcm128_1,
-                                kTestKeyGcm128Len, rtc::SRTP_AEAD_AES_128_GCM,
-                                kTestKeyGcm128_2, kTestKeyGcm128Len));
-  EXPECT_TRUE(f2_.SetRtcpParams(rtc::SRTP_AEAD_AES_128_GCM, kTestKeyGcm128_2,
-                                kTestKeyGcm128Len, rtc::SRTP_AEAD_AES_128_GCM,
-                                kTestKeyGcm128_1, kTestKeyGcm128Len));
-  EXPECT_TRUE(f1_.IsActive());
-  EXPECT_TRUE(f2_.IsActive());
-#if defined(ENABLE_EXTERNAL_AUTH)
-  EXPECT_FALSE(f1_.IsExternalAuthActive());
-  EXPECT_FALSE(f2_.IsExternalAuthActive());
-#endif
-  TestProtectUnprotect(CS_AEAD_AES_128_GCM, CS_AEAD_AES_128_GCM);
+  TestProtectSetParamsDirect(false, rtc::SRTP_AEAD_AES_128_GCM,
+      kTestKeyGcm128_1, kTestKeyGcm128Len, kTestKeyGcm128_2, kTestKeyGcm128Len,
+      CS_AEAD_AES_128_GCM);
+}
+
+TEST_F(SrtpFilterTest,
+    TestProtect_SetParamsDirect_SRTP_AEAD_AES_128_GCM_ExternalAuth) {
+  TestProtectSetParamsDirect(true, rtc::SRTP_AEAD_AES_128_GCM,
+      kTestKeyGcm128_1, kTestKeyGcm128Len, kTestKeyGcm128_2, kTestKeyGcm128Len,
+      CS_AEAD_AES_128_GCM);
 }
 
 // Test directly setting the params with SRTP_AEAD_AES_256_GCM.
 TEST_F(SrtpFilterTest, TestProtect_SetParamsDirect_SRTP_AEAD_AES_256_GCM) {
-  EXPECT_TRUE(f1_.SetRtpParams(rtc::SRTP_AEAD_AES_256_GCM, kTestKeyGcm256_1,
-                               kTestKeyGcm256Len, rtc::SRTP_AEAD_AES_256_GCM,
-                               kTestKeyGcm256_2, kTestKeyGcm256Len));
-  EXPECT_TRUE(f2_.SetRtpParams(rtc::SRTP_AEAD_AES_256_GCM, kTestKeyGcm256_2,
-                               kTestKeyGcm256Len, rtc::SRTP_AEAD_AES_256_GCM,
-                               kTestKeyGcm256_1, kTestKeyGcm256Len));
-  EXPECT_TRUE(f1_.SetRtcpParams(rtc::SRTP_AEAD_AES_256_GCM, kTestKeyGcm256_1,
-                                kTestKeyGcm256Len, rtc::SRTP_AEAD_AES_256_GCM,
-                                kTestKeyGcm256_2, kTestKeyGcm256Len));
-  EXPECT_TRUE(f2_.SetRtcpParams(rtc::SRTP_AEAD_AES_256_GCM, kTestKeyGcm256_2,
-                                kTestKeyGcm256Len, rtc::SRTP_AEAD_AES_256_GCM,
-                                kTestKeyGcm256_1, kTestKeyGcm256Len));
-  EXPECT_TRUE(f1_.IsActive());
-  EXPECT_TRUE(f2_.IsActive());
-#if defined(ENABLE_EXTERNAL_AUTH)
-  EXPECT_FALSE(f1_.IsExternalAuthActive());
-  EXPECT_FALSE(f2_.IsExternalAuthActive());
-#endif
-  TestProtectUnprotect(CS_AEAD_AES_256_GCM, CS_AEAD_AES_256_GCM);
+  TestProtectSetParamsDirect(false, rtc::SRTP_AEAD_AES_256_GCM,
+      kTestKeyGcm256_1, kTestKeyGcm256Len, kTestKeyGcm256_2, kTestKeyGcm256Len,
+      CS_AEAD_AES_256_GCM);
+}
+
+TEST_F(SrtpFilterTest,
+    TestProtect_SetParamsDirect_SRTP_AEAD_AES_256_GCM_ExternalAuth) {
+  TestProtectSetParamsDirect(true, rtc::SRTP_AEAD_AES_256_GCM,
+      kTestKeyGcm256_1, kTestKeyGcm256Len, kTestKeyGcm256_2, kTestKeyGcm256Len,
+      CS_AEAD_AES_256_GCM);
 }
 
 // Test directly setting the params with bogus keys.
 TEST_F(SrtpFilterTest, TestSetParamsKeyTooShort) {
   EXPECT_FALSE(f1_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey1,
                                 kTestKeyLen - 1, rtc::SRTP_AES128_CM_SHA1_80,
                                 kTestKey1, kTestKeyLen - 1));
   EXPECT_FALSE(f1_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_80, kTestKey1,
                                  kTestKeyLen - 1, rtc::SRTP_AES128_CM_SHA1_80,
                                  kTestKey1, kTestKeyLen - 1));
 }
 
-#if defined(ENABLE_EXTERNAL_AUTH)
 TEST_F(SrtpFilterTest, TestGetSendAuthParams) {

[Taylor Brandstetter, 2017/03/03 02:20:35]

Is this test still necessary, now that the above tests will test
GetRtpAuthParams?

[joachim, 2017/03/03 20:42:57]

True, the same is already tested above.

+  f1_.AllowExternalAuthForTest(true);
   EXPECT_TRUE(f1_.SetRtpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey1,
                                kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
                                kTestKey2, kTestKeyLen));
   EXPECT_TRUE(f1_.SetRtcpParams(rtc::SRTP_AES128_CM_SHA1_32, kTestKey1,
                                 kTestKeyLen, rtc::SRTP_AES128_CM_SHA1_32,
                                 kTestKey2, kTestKeyLen));
   // Non-GCM ciphers support external auth.
   EXPECT_TRUE(f1_.IsExternalAuthActive());
   uint8_t* auth_key = NULL;
   int auth_key_len = 0, auth_tag_len = 0;
   EXPECT_TRUE(f1_.GetRtpAuthParams(&auth_key, &auth_key_len, &auth_tag_len));
   EXPECT_TRUE(auth_key != NULL);
   EXPECT_EQ(20, auth_key_len);
   EXPECT_EQ(4, auth_tag_len);
 }
-#endif
 
 class SrtpSessionTest : public testing::Test {
  protected:
   virtual void SetUp() {
     rtp_len_ = sizeof(kPcmuFrame);
     rtcp_len_ = sizeof(kRtcpReport);
     memcpy(rtp_packet_, kPcmuFrame, rtp_len_);
     memcpy(rtcp_packet_, kRtcpReport, rtcp_len_);
   }
   void TestProtectRtp(const std::string& cs) {
@@ skipping 333 matching lines @@
   srtp_stat_.AddUnprotectRtcpResult(srtp_err_status_fail);
   EXPECT_EQ(-1, mode_);
   EXPECT_EQ(cricket::SrtpFilter::ERROR_NONE, error_);
   // Now the error will be triggered again.
   Reset();
   rtc::Thread::Current()->SleepMs(210);
   srtp_stat_.AddUnprotectRtcpResult(srtp_err_status_fail);
   EXPECT_EQ(cricket::SrtpFilter::UNPROTECT, mode_);
   EXPECT_EQ(cricket::SrtpFilter::ERROR_FAIL, error_);
 }