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..138d8d191bda1acf53343e933bdf3446d4cda3f3 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,
@@ -229,6 +235,10 @@ class DtlsTestClient : public sigslot::has_slots<> {
     return received_dtls_client_hellos_;
   }
 
+  int received_dtls_server_hellos() const {
+    return received_dtls_server_hellos_;
+  }
+
   bool negotiated_dtls() const { return set_local_cert_ && set_remote_cert_; }
 
   void CheckRole(rtc::SSLRole role) {
@@ -495,9 +505,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 +598,74 @@ 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_);
+  }
+
+  enum Event {
+    CALLER_RECEIVES_FINGERPRINT,
+    CALLER_WRITABLE,
+    CALLER_RECEIVES_CLIENTHELLO,
+    HANDSHAKE_FINISHES
+  };
+  void TestEventPermutation(const std::vector<Event>& events) {
+    // 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 (Event e : events) {
+      switch (e) {
+        case CALLER_RECEIVES_FINGERPRINT:
+          client1_.SetRemoteTransportDescription(
+              client2_.certificate(), cricket::CA_ANSWER,
+              cricket::CONNECTIONROLE_ACTIVE, 0);
+          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());
+          EXPECT_TRUE_SIMULATED_WAIT(
+              client1_.GetDtlsChannel(0)->IsDtlsConnected(), kTimeout,
+              fake_clock_);
+          break;
+      }
+    }
+
+    EXPECT_TRUE_SIMULATED_WAIT(
+        client1_.all_channels_writable() && client2_.all_channels_writable(),
+        kTimeout, fake_clock_);
+    EXPECT_EQ(1, client1_.received_dtls_client_hellos());
+    EXPECT_EQ(1, client2_.received_dtls_server_hellos());
+    TestTransfer(0, 1000, 100, false);
   }
 
  protected:
@@ -884,9 +961,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,70 +1018,77 @@ 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) {
+// 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 possible permutations of these events,

[mattdr-at-webrtc.org, 2016/07/20 00:50:27]

where by "all possible" you mean "all causally consistent", since 3 must precede
4.

[Taylor Brandstetter, 2016/07/20 17:36:52]

Actually 2 and 3, since we won't "write" packets until the caller is writable.
Reducing the permutations from 24 to 8. I'll update the comment to reflect this.

+// and verify that a connection is established and fingerprint verified
+// without any DTLS packet needing to be retransmitted.
+TEST_F(DtlsTransportChannelTest, EventPermutation1) {
   MAYBE_SKIP_TEST(HaveDtls);
-  PrepareDtls(true, true, rtc::KT_DEFAULT);
-  // Exchange transport descriptions.
-  Negotiate(cricket::CONNECTIONROLE_ACTPASS, cricket::CONNECTIONROLE_ACTIVE);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // Make client2_ writable, but not client1_.
-  EXPECT_TRUE(client2_.Connect(&client1_, true));
-  EXPECT_TRUE_WAIT(client2_.all_raw_channels_writable(), kTimeout);
+TEST_F(DtlsTransportChannelTest, EventPermutation2) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // 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());
+TEST_F(DtlsTransportChannelTest, EventPermutation3) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // 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_F(DtlsTransportChannelTest, EventPermutation4) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES,
+      CALLER_RECEIVES_FINGERPRINT,
+  });
 }
 
-// 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) {
+TEST_F(DtlsTransportChannelTest, EventPermutation5) {
   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);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // 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());
+TEST_F(DtlsTransportChannelTest, EventPermutation6) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // 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_F(DtlsTransportChannelTest, EventPermutation7) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE, CALLER_RECEIVES_FINGERPRINT,
+      HANDSHAKE_FINISHES,
+  });
+}
+
+TEST_F(DtlsTransportChannelTest, EventPermutation8) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE, HANDSHAKE_FINISHES,
+      CALLER_RECEIVES_FINGERPRINT,
+  });
 }
 
 // Test that packets are retransmitted according to the expected schedule.
@@ -1024,7 +1108,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);