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

webrtc/p2p/base/dtlstransportchannel_unittest.cc

 /*
  *  Copyright 2011 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 63 matching lines @@
     return certificate_;
   }
   void SetupSrtp() {
     ASSERT(certificate_);
     use_dtls_srtp_ = true;
   }
   void SetupMaxProtocolVersion(rtc::SSLProtocolVersion version) {
     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);
 
     for (int i = 0; i < count; ++i) {
       cricket::DtlsTransportChannelWrapper* channel =
           static_cast<cricket::DtlsTransportChannelWrapper*>(
               transport_->CreateChannel(i));
       ASSERT_TRUE(channel != NULL);
       channel->SetSslMaxProtocolVersion(ssl_max_version_);
@@ skipping 14 matching lines @@
   cricket::Transport* transport() { return transport_.get(); }
 
   cricket::FakeTransportChannel* GetFakeChannel(int component) {
     cricket::TransportChannelImpl* ch = transport_->GetChannel(component);
     cricket::DtlsTransportChannelWrapper* wrapper =
         static_cast<cricket::DtlsTransportChannelWrapper*>(ch);
     return (wrapper) ?
         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,
                  ConnectionRole local_role, ConnectionRole remote_role,
                  int flags) {
     Negotiate(certificate_, certificate_ ? peer->certificate_ : nullptr, action,
               local_role, remote_role, flags);
   }
 
   void MaybeSetSrtpCryptoSuites() {
@@ skipping 90 matching lines @@
         return false;
       }
     }
     return true;
   }
 
   int received_dtls_client_hellos() const {
     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) {
     if (role == rtc::SSL_CLIENT) {
       ASSERT_EQ(0, received_dtls_client_hellos_);
       ASSERT_GT(received_dtls_server_hellos_, 0);
     } else {
       ASSERT_GT(received_dtls_client_hellos_, 0);
       ASSERT_EQ(0, received_dtls_server_hellos_);
     }
@@ skipping 246 matching lines @@
           client2_.certificate(), cricket::CA_ANSWER, client2_role, 0);
       rv = client1_.Connect(&client2_, false);
       client1_.SetRemoteTransportDescription(
           client2_.certificate(), cricket::CA_ANSWER, client2_role, 0);
     }
 
     EXPECT_TRUE(rv);
     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;
 
     // Check that we used the right roles.
     if (use_dtls_) {
       rtc::SSLRole client1_ssl_role =
           (client1_role == cricket::CONNECTIONROLE_ACTIVE ||
            (client2_role == cricket::CONNECTIONROLE_PASSIVE &&
             client1_role == cricket::CONNECTIONROLE_ACTPASS)) ?
               rtc::SSL_CLIENT : rtc::SSL_SERVER;
@@ skipping 70 matching lines @@
                          client2_role, client1_role, flags);
       client1_.Negotiate(&client2_, cricket::CA_ANSWER,
                          client1_role, client2_role, flags);
     }
   }
 
   void TestTransfer(size_t channel, size_t size, size_t count, bool srtp) {
     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:
   rtc::ScopedFakeClock fake_clock_;
   DtlsTestClient client1_;
   DtlsTestClient client2_;
   int channel_ct_;
   bool use_dtls_;
   bool use_dtls_srtp_;
   rtc::SSLProtocolVersion ssl_expected_version_;
@@ skipping 275 matching lines @@
   MAYBE_SKIP_TEST(HaveDtlsSrtp);
   SetChannelCount(2);
   PrepareDtls(true, true, rtc::KT_DEFAULT);
   PrepareDtlsSrtp(true, true);
   Negotiate();
 
   Renegotiate(&client1_, cricket::CONNECTIONROLE_ACTPASS,
               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);
 }
 
 // Test Certificates state after negotiation but before connection.
 TEST_F(DtlsTransportChannelTest, TestCertificatesBeforeConnect) {
   MAYBE_SKIP_TEST(HaveDtls);
   PrepareDtls(true, true, rtc::KT_DEFAULT);
   Negotiate();
@@ skipping 34 matching lines @@
   ASSERT_TRUE(remote_cert1);
   ASSERT_EQ(remote_cert1->ToPEMString(),
             certificate2->ssl_certificate().ToPEMString());
   std::unique_ptr<rtc::SSLCertificate> remote_cert2 =
       client2_.transport()->GetRemoteSSLCertificate();
   ASSERT_TRUE(remote_cert2);
   ASSERT_EQ(remote_cert2->ToPEMString(),
             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 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.

[pthatcher1, 2016/07/27 19:32:35]

Can you point out why there are only 8 permutations (The handshake can't finish
before the ClientHello and the caller is writable)?

[Taylor Brandstetter, 2016/08/13 00:09:53]

Isn't that exactly what the above paragraph points out? What would you add?

+//
+// For each permutation, the test verifies 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);
-
-  // 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());
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO,
+      HANDSHAKE_FINISHES,
+  });

[pthatcher1, 2016/07/27 19:32:35]

Would it make sense to just have these be methods?

Setup();
CallerReceivesFingerprint();
CallerWritable();
CallerReceivesClientHello();
HandshakeFinishes();
Finish();

[Taylor Brandstetter, 2016/08/13 00:09:53]

I changed how this works; I'm using parameterized tests now.

 }
 
-// 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, EventPermutation2) {
   MAYBE_SKIP_TEST(HaveDtls);
-  PrepareDtls(true, true, rtc::KT_DEFAULT);
-  client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING);
-  client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO,
+      HANDSHAKE_FINISHES,
+  });
+}
 
-  // 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);
+TEST_F(DtlsTransportChannelTest, EventPermutation3) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT,
+      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, EventPermutation4) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_WRITABLE, CALLER_RECEIVES_CLIENTHELLO, HANDSHAKE_FINISHES,
+      CALLER_RECEIVES_FINGERPRINT,
+  });
+}
 
-  // 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, EventPermutation5) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_FINGERPRINT, CALLER_RECEIVES_CLIENTHELLO, CALLER_WRITABLE,
+      HANDSHAKE_FINISHES,
+  });
+}
+
+TEST_F(DtlsTransportChannelTest, EventPermutation6) {
+  MAYBE_SKIP_TEST(HaveDtls);
+  TestEventPermutation(std::vector<Event>{
+      CALLER_RECEIVES_CLIENTHELLO, CALLER_RECEIVES_FINGERPRINT, CALLER_WRITABLE,
+      HANDSHAKE_FINISHES,
+  });
+}
+
+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.
 // 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
 // initializing it to 50ms.
 TEST_F(DtlsTransportChannelTest, TestRetransmissionSchedule) {
   MAYBE_SKIP_TEST(HaveDtls);
   // We can only change the retransmission schedule with a recently-added
   // BoringSSL API. Skip the test if not built with BoringSSL.
   MAYBE_SKIP_TEST(IsBoringSsl);
 
   PrepareDtls(true, true, rtc::KT_DEFAULT);
   // Exchange transport descriptions.
   Negotiate(cricket::CONNECTIONROLE_ACTPASS, cricket::CONNECTIONROLE_ACTIVE);
 
   // 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);
   EXPECT_FALSE(client1_.all_raw_channels_writable());
 
   static int timeout_schedule_ms[] = {50,   100,  200,   400,   800,   1600,
                                       3200, 6400, 12800, 25600, 51200, 60000};
 
   int expected_hellos = 1;
   for (size_t i = 0;
@@ skipping 14 matching lines @@
 // Test that a DTLS connection can be made even if the underlying transport
 // is connected before DTLS fingerprints/roles have been negotiated.
 TEST_F(DtlsTransportChannelTest, TestConnectBeforeNegotiate) {
   MAYBE_SKIP_TEST(HaveDtls);
   PrepareDtls(true, true, rtc::KT_DEFAULT);
   ASSERT_TRUE(Connect(cricket::CONNECTIONROLE_ACTPASS,
                       cricket::CONNECTIONROLE_ACTIVE,
                       CONNECT_BEFORE_NEGOTIATE));
   TestTransfer(0, 1000, 100, false);
 }