Relanding: Allow the DTLS fingerprint verification to occur after the handshake.

webrtc/p2p/base/dtlstransportchannel_unittest.cc

Index: webrtc/p2p/base/dtlstransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
index 6eb0f0e3f1aa857461a5e3feecf1451956cc0ef6..0a6e254e8fa726a739d7c60998a9a625e9c6459f 100644
--- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc
+++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
@@ -81,10 +81,11 @@ class DtlsTestClient : public sigslot::has_slots<> {
     ASSERT(!transport_);
     ssl_max_version_ = version;
   }
-  void SetupChannels(int count, cricket::IceRole role) {
+  void SetupChannels(int count, cricket::IceRole role, int async_delay_ms = 0) {
     transport_.reset(new cricket::DtlsTransport<cricket::FakeTransport>(
         "dtls content name", nullptr, certificate_));
     transport_->SetAsync(true);
+    transport_->SetAsyncDelay(async_delay_ms);
     transport_->SetIceRole(role);
     transport_->SetIceTiebreaker(
         (role == cricket::ICEROLE_CONTROLLING) ? 1 : 2);
@@ -119,6 +120,11 @@ class DtlsTestClient : public sigslot::has_slots<> {
         static_cast<cricket::FakeTransportChannel*>(wrapper->channel()) : NULL;
   }
 
+  cricket::DtlsTransportChannelWrapper* GetDtlsChannel(int component) {
+    cricket::TransportChannelImpl* ch = transport_->GetChannel(component);
+    return static_cast<cricket::DtlsTransportChannelWrapper*>(ch);
+  }
+
   // Offer DTLS if we have an identity; pass in a remote fingerprint only if
   // both sides support DTLS.
   void Negotiate(DtlsTestClient* peer, cricket::ContentAction action,
@@ -152,7 +158,6 @@ class DtlsTestClient : public sigslot::has_slots<> {
     EXPECT_EQ(expect_success,
               transport_->SetLocalTransportDescription(
                   MakeTransportDescription(cert, role), action, nullptr));
-    set_local_cert_ = (cert != nullptr);
   }
 
   void SetRemoteTransportDescription(
@@ -167,7 +172,6 @@ class DtlsTestClient : public sigslot::has_slots<> {
     EXPECT_EQ(expect_success,
               transport_->SetRemoteTransportDescription(
                   MakeTransportDescription(cert, role), action, nullptr));
-    set_remote_cert_ = (cert != nullptr);
   }
 
   // Allow any DTLS configuration to be specified (including invalid ones).
@@ -229,7 +233,16 @@ class DtlsTestClient : public sigslot::has_slots<> {
     return received_dtls_client_hellos_;
   }
 
-  bool negotiated_dtls() const { return set_local_cert_ && set_remote_cert_; }
+  int received_dtls_server_hellos() const {
+    return received_dtls_server_hellos_;
+  }
+
+  bool negotiated_dtls() const {
+    return transport_->local_description() &&
+           transport_->local_description()->identity_fingerprint &&
+           transport_->remote_description() &&
+           transport_->remote_description()->identity_fingerprint;
+  }
 
   void CheckRole(rtc::SSLRole role) {
     if (role == rtc::SSL_CLIENT) {
@@ -411,18 +424,19 @@ class DtlsTestClient : public sigslot::has_slots<> {
   std::set<int> received_;
   bool use_dtls_srtp_ = false;
   rtc::SSLProtocolVersion ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
-  bool set_local_cert_ = false;
-  bool set_remote_cert_ = false;
   int received_dtls_client_hellos_ = 0;
   int received_dtls_server_hellos_ = 0;
   rtc::SentPacket sent_packet_;
 };
 
+// Base class for DtlsTransportChannelTest and DtlsEventOrderingTest, which
+// inherit from different variants of testing::Test.
+//
 // Note that this test always uses a FakeClock, due to the |fake_clock_| member
 // variable.
-class DtlsTransportChannelTest : public testing::Test {
+class DtlsTransportChannelTestBase {
  public:
-  DtlsTransportChannelTest()
+  DtlsTransportChannelTestBase()
       : client1_("P1"),
         client2_("P2"),
         channel_ct_(1),
@@ -495,9 +509,9 @@ class DtlsTransportChannelTest : public testing::Test {
     if (!rv)
       return false;
 
-    EXPECT_TRUE_WAIT(
+    EXPECT_TRUE_SIMULATED_WAIT(
         client1_.all_channels_writable() && client2_.all_channels_writable(),
-        kTimeout);
+        kTimeout, fake_clock_);
     if (!client1_.all_channels_writable() || !client2_.all_channels_writable())
       return false;
 
@@ -588,7 +602,8 @@ class DtlsTransportChannelTest : public testing::Test {
     LOG(LS_INFO) << "Expect packets, size=" << size;
     client2_.ExpectPackets(channel, size);
     client1_.SendPackets(channel, size, count, srtp);
-    EXPECT_EQ_WAIT(count, client2_.NumPacketsReceived(), kTimeout);
+    EXPECT_EQ_SIMULATED_WAIT(count, client2_.NumPacketsReceived(), kTimeout,
+                             fake_clock_);
   }
 
  protected:
@@ -601,6 +616,9 @@ class DtlsTransportChannelTest : public testing::Test {
   rtc::SSLProtocolVersion ssl_expected_version_;
 };
 
+class DtlsTransportChannelTest : public DtlsTransportChannelTestBase,
+                                 public ::testing::Test {};
+
 // Test that transport negotiation of ICE, no DTLS works properly.
 TEST_F(DtlsTransportChannelTest, TestChannelSetupIce) {
   Negotiate();
@@ -884,9 +902,9 @@ TEST_F(DtlsTransportChannelTest, TestRenegotiateBeforeConnect) {
               cricket::CONNECTIONROLE_ACTIVE, NF_REOFFER);
   bool rv = client1_.Connect(&client2_, false);
   EXPECT_TRUE(rv);
-  EXPECT_TRUE_WAIT(
+  EXPECT_TRUE_SIMULATED_WAIT(
       client1_.all_channels_writable() && client2_.all_channels_writable(),
-      kTimeout);
+      kTimeout, fake_clock_);
 
   TestTransfer(0, 1000, 100, true);
   TestTransfer(1, 1000, 100, true);
@@ -941,72 +959,6 @@ TEST_F(DtlsTransportChannelTest, TestCertificatesAfterConnect) {
             certificate1->ssl_certificate().ToPEMString());
 }
 
-// Test that DTLS completes promptly if a ClientHello is received before the
-// transport channel is writable (allowing a ServerHello to be sent).
-TEST_F(DtlsTransportChannelTest, TestReceiveClientHelloBeforeWritable) {
-  MAYBE_SKIP_TEST(HaveDtls);
-  PrepareDtls(true, true, rtc::KT_DEFAULT);
-  // Exchange transport descriptions.
-  Negotiate(cricket::CONNECTIONROLE_ACTPASS, cricket::CONNECTIONROLE_ACTIVE);
-
-  // Make client2_ writable, but not client1_.
-  EXPECT_TRUE(client2_.Connect(&client1_, true));
-  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
-
-  // Expect a DTLS ClientHello to be sent even while client1_ isn't writable.
-  EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
-  EXPECT_FALSE(client1_.all_raw_channels_writable());
-
-  // Now make client1_ writable and expect the handshake to complete
-  // without client2_ needing to retransmit the ClientHello.
-  EXPECT_TRUE(client1_.Connect(&client2_, true));
-  EXPECT_TRUE_WAIT(
-      client1_.all_channels_writable() && client2_.all_channels_writable(),
-      kTimeout);
-  EXPECT_EQ(1, client1_.received_dtls_client_hellos());
-}
-
-// Test that DTLS completes promptly if a ClientHello is received before the
-// transport channel has a remote fingerprint (allowing a ServerHello to be
-// sent).
-TEST_F(DtlsTransportChannelTest,
-       TestReceiveClientHelloBeforeRemoteFingerprint) {
-  MAYBE_SKIP_TEST(HaveDtls);
-  PrepareDtls(true, true, rtc::KT_DEFAULT);
-  client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING);
-  client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED);
-
-  // Make client2_ writable and give it local/remote certs, but don't yet give
-  // client1_ a remote fingerprint.
-  client1_.transport()->SetLocalTransportDescription(
-      MakeTransportDescription(client1_.certificate(),
-                               cricket::CONNECTIONROLE_ACTPASS),
-      cricket::CA_OFFER, nullptr);
-  client2_.Negotiate(&client1_, cricket::CA_ANSWER,
-                     cricket::CONNECTIONROLE_ACTIVE,
-                     cricket::CONNECTIONROLE_ACTPASS, 0);
-  EXPECT_TRUE(client2_.Connect(&client1_, true));
-  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
-
-  // Expect a DTLS ClientHello to be sent even while client1_ doesn't have a
-  // remote fingerprint.
-  EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
-  EXPECT_FALSE(client1_.all_raw_channels_writable());
-
-  // Now make give client1_ its remote fingerprint and make it writable, and
-  // expect the handshake to complete without client2_ needing to retransmit
-  // the ClientHello.
-  client1_.transport()->SetRemoteTransportDescription(
-      MakeTransportDescription(client2_.certificate(),
-                               cricket::CONNECTIONROLE_ACTIVE),
-      cricket::CA_ANSWER, nullptr);
-  EXPECT_TRUE(client1_.Connect(&client2_, true));
-  EXPECT_TRUE_WAIT(
-      client1_.all_channels_writable() && client2_.all_channels_writable(),
-      kTimeout);
-  EXPECT_EQ(1, client1_.received_dtls_client_hellos());
-}
-
 // Test that packets are retransmitted according to the expected schedule.
 // Each time a timeout occurs, the retransmission timer should be doubled up to
 // 60 seconds. The timer defaults to 1 second, but for WebRTC we should be
@@ -1024,7 +976,8 @@ TEST_F(DtlsTransportChannelTest, TestRetransmissionSchedule) {
   // Make client2_ writable, but not client1_.
   // This means client1_ will send DTLS client hellos but get no response.
   EXPECT_TRUE(client2_.Connect(&client1_, true));
-  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
+  EXPECT_TRUE_SIMULATED_WAIT(client2_.all_raw_channels_writable(), kTimeout,
+                             fake_clock_);
 
   // Wait for the first client hello to be sent.
   EXPECT_EQ_WAIT(1, client1_.received_dtls_client_hellos(), kTimeout);
@@ -1059,3 +1012,164 @@ TEST_F(DtlsTransportChannelTest, TestConnectBeforeNegotiate) {
                       CONNECT_BEFORE_NEGOTIATE));
   TestTransfer(0, 1000, 100, false);
 }
+
+// The following events can occur in many different orders:
+// 1. Caller receives remote fingerprint.
+// 2. Caller is writable.
+// 3. Caller receives ClientHello.
+// 4. DTLS handshake finishes.
+//
+// The tests below cover all causally consistent permutations of these events;
+// the caller must be writable and receive a ClientHello before the handshake
+// finishes, but otherwise any ordering is possible.
+//
+// For each permutation, the test verifies that a connection is established and
+// fingerprint verified without any DTLS packet needing to be retransmitted.
+//
+// Each permutation is also tested with valid and invalid fingerprints,
+// ensuring that the handshake fails with an invalid fingerprint.
+enum DtlsTransportEvent {
+  CALLER_RECEIVES_FINGERPRINT,
+  CALLER_WRITABLE,
+  CALLER_RECEIVES_CLIENTHELLO,
+  HANDSHAKE_FINISHES
+};
+
+class DtlsEventOrderingTest
+    : public DtlsTransportChannelTestBase,
+      public ::testing::TestWithParam<
+          ::testing::tuple<std::vector<DtlsTransportEvent>, bool>> {
+ protected:
+  // If |valid_fingerprint| is false, the caller will receive a fingerprint
+  // that doesn't match the callee's certificate, so the handshake should fail.
+  void TestEventOrdering(const std::vector<DtlsTransportEvent>& events,
+                         bool valid_fingerprint) {
+    // Pre-setup: Set local certificate on both caller and callee, and
+    // remote fingerprint on callee, but neither is writable and the caller
+    // doesn't have the callee's fingerprint.
+    PrepareDtls(true, true, rtc::KT_DEFAULT);
+    // Simulate packets being sent and arriving asynchronously.
+    // Otherwise the entire DTLS handshake would occur in one clock tick, and
+    // we couldn't inject method calls in the middle of it.
+    int simulated_delay_ms = 10;
+    client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING,
+                           simulated_delay_ms);
+    client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED,
+                           simulated_delay_ms);
+    client1_.SetLocalTransportDescription(client1_.certificate(),
+                                          cricket::CA_OFFER,
+                                          cricket::CONNECTIONROLE_ACTPASS, 0);
+    client2_.Negotiate(&client1_, cricket::CA_ANSWER,
+                       cricket::CONNECTIONROLE_ACTIVE,
+                       cricket::CONNECTIONROLE_ACTPASS, 0);
+
+    for (DtlsTransportEvent e : events) {
+      switch (e) {
+        case CALLER_RECEIVES_FINGERPRINT:
+          if (valid_fingerprint) {
+            client1_.SetRemoteTransportDescription(
+                client2_.certificate(), cricket::CA_ANSWER,
+                cricket::CONNECTIONROLE_ACTIVE, 0);
+          } else {
+            // Create a fingerprint with a correct algorithm but an invalid
+            // digest.
+            cricket::TransportDescription remote_desc =
+                MakeTransportDescription(client2_.certificate(),
+                                         cricket::CONNECTIONROLE_ACTIVE);
+            ++(remote_desc.identity_fingerprint->digest[0]);
+            // Even if certificate verification fails inside this method,
+            // it should return true as long as the fingerprint was formatted
+            // correctly.
+            EXPECT_TRUE(client1_.transport()->SetRemoteTransportDescription(
+                remote_desc, cricket::CA_ANSWER, nullptr));
+          }
+          break;
+        case CALLER_WRITABLE:
+          EXPECT_TRUE(client1_.Connect(&client2_, true));
+          EXPECT_TRUE_SIMULATED_WAIT(client1_.all_raw_channels_writable(),
+                                     kTimeout, fake_clock_);
+          break;
+        case CALLER_RECEIVES_CLIENTHELLO:
+          // Sanity check that a ClientHello hasn't already been received.
+          EXPECT_EQ(0, client1_.received_dtls_client_hellos());
+          // Making client2_ writable will cause it to send the ClientHello.
+          EXPECT_TRUE(client2_.Connect(&client1_, true));
+          EXPECT_TRUE_SIMULATED_WAIT(client2_.all_raw_channels_writable(),
+                                     kTimeout, fake_clock_);
+          EXPECT_EQ_SIMULATED_WAIT(1, client1_.received_dtls_client_hellos(),
+                                   kTimeout, fake_clock_);
+          break;
+        case HANDSHAKE_FINISHES:
+          // Sanity check that the handshake hasn't already finished.
+          EXPECT_FALSE(client1_.GetDtlsChannel(0)->IsDtlsConnected() ||
+                       client1_.GetDtlsChannel(0)->dtls_state() ==
+                           cricket::DTLS_TRANSPORT_FAILED);
+          EXPECT_TRUE_SIMULATED_WAIT(
+              client1_.GetDtlsChannel(0)->IsDtlsConnected() ||
+                  client1_.GetDtlsChannel(0)->dtls_state() ==
+                      cricket::DTLS_TRANSPORT_FAILED,
+              kTimeout, fake_clock_);
+          break;
+      }
+    }
+
+    cricket::DtlsTransportState expected_final_state =
+        valid_fingerprint ? cricket::DTLS_TRANSPORT_CONNECTED
+                          : cricket::DTLS_TRANSPORT_FAILED;
+    EXPECT_EQ_SIMULATED_WAIT(expected_final_state,
+                             client1_.GetDtlsChannel(0)->dtls_state(), kTimeout,
+                             fake_clock_);
+    EXPECT_EQ_SIMULATED_WAIT(expected_final_state,
+                             client2_.GetDtlsChannel(0)->dtls_state(), kTimeout,
+                             fake_clock_);
+
+    // Channel should be writable iff there was a valid fingerprint.
+    EXPECT_EQ(valid_fingerprint, client1_.GetDtlsChannel(0)->writable());
+    EXPECT_EQ(valid_fingerprint, client2_.GetDtlsChannel(0)->writable());
+
+    // Check that no hello needed to be retransmitted.
+    EXPECT_EQ(1, client1_.received_dtls_client_hellos());
+    EXPECT_EQ(1, client2_.received_dtls_server_hellos());
+
+    if (valid_fingerprint) {
+      TestTransfer(0, 1000, 100, false);
+    }
+  }
+};
+
+TEST_P(DtlsEventOrderingTest, TestEventOrdering) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventOrdering(::testing::get<0>(GetParam()),
+                    ::testing::get<1>(GetParam()));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    TestEventOrdering,
+    DtlsEventOrderingTest,
+    ::testing::Combine(
+        ::testing::Values(
+            std::vector<DtlsTransportEvent>{
+                CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE,
+                CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{
+                CALLER_WRITABLE, CALLER_RECEIVES_FINGERPRINT,
+                CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{
+                CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
+                CALLER_RECEIVES_FINGERPRINT, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{
+                CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
+                HANDSHAKE_FINISHES, CALLER_RECEIVES_FINGERPRINT},
+            std::vector<DtlsTransportEvent>{
+                CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO,
+                CALLER_WRITABLE, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{
+                CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT,
+                CALLER_WRITABLE, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{
+                CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE,
+                CALLER_RECEIVES_FINGERPRINT, HANDSHAKE_FINISHES},
+            std::vector<DtlsTransportEvent>{CALLER_RECEIVES_CLIENTHELLO,
+                                            CALLER_WRITABLE, HANDSHAKE_FINISHES,
+                                            CALLER_RECEIVES_FINGERPRINT}),
+        ::testing::Bool()));