Fixing issue with DTLS channel when using "presume writable" flag.

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 106 matching lines @@
 
   // 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);
   }
 
+  std::unique_ptr<rtc::SSLFingerprint> MakeFingerprintFromCertificate(
+      const rtc::scoped_refptr<rtc::RTCCertificate>& cert) {
+    std::unique_ptr<rtc::SSLFingerprint> fingerprint;
+    if (cert) {

[pthatcher1, 2016/07/13 00:04:19]

Maybe have it be early return?

if (!cert) {
  return fingerprint;
}
...

[Taylor Brandstetter, 2016/07/13 16:17:41]

Done.

+      std::string digest_algorithm;
+      EXPECT_TRUE(cert->ssl_certificate().GetSignatureDigestAlgorithm(
+          &digest_algorithm));
+      EXPECT_FALSE(digest_algorithm.empty());
+      fingerprint.reset(
+          rtc::SSLFingerprint::Create(digest_algorithm, cert->identity()));
+      EXPECT_TRUE(fingerprint.get() != NULL);
+      EXPECT_EQ(rtc::DIGEST_SHA_256, digest_algorithm);
+    }
+    return fingerprint;
+  }
+
+  void SetLocalTransportDescription(
+      const rtc::scoped_refptr<rtc::RTCCertificate>& cert,
+      cricket::ContentAction action,
+      ConnectionRole role,
+      int flags) {
+    auto fingerprint = MakeFingerprintFromCertificate(cert);
+    cricket::TransportDescription local_desc(
+        std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+        role, fingerprint.get());
+
+    if (action == cricket::CA_OFFER) {
+      if (use_dtls_srtp_ && !(flags & NF_REOFFER)) {
+        // SRTP ciphers will be set only in the beginning.
+        for (std::vector<cricket::DtlsTransportChannelWrapper*>::iterator it =
+                 channels_.begin();
+             it != channels_.end(); ++it) {

[pthatcher1, 2016/07/13 00:04:19]

c++11 style for loop?

[Taylor Brandstetter, 2016/07/13 16:17:41]

Done.

+          std::vector<int> ciphers;
+          ciphers.push_back(rtc::SRTP_AES128_CM_SHA1_80);
+          EXPECT_TRUE((*it)->SetSrtpCryptoSuites(ciphers));
+        }
+      }
+
+      EXPECT_TRUE(transport_->SetLocalTransportDescription(local_desc, action,
+                                                           nullptr));
+    } else {
+      // If |expect_success| is false, expect SRTD or SLTD to fail when
+      // content action is CA_ANSWER.
+      bool expect_success = (flags & NF_EXPECT_FAILURE) ? false : true;
+      EXPECT_EQ(expect_success, transport_->SetLocalTransportDescription(
+                                    local_desc, action, nullptr));
+    }
+    set_local_cert_ = (cert != nullptr);

[pthatcher1, 2016/07/13 00:04:20]

Would has_local_cert_ be a better name?

[Taylor Brandstetter, 2016/07/13 16:17:41]

The method is called SetLocalTransportDescription so that seemed redundant.

[Taylor Brandstetter, 2016/07/13 16:20:16]

Oh I misread your comment. I thought you were asking for cert to be renamed to
local_cert.

Still, I think "set_local_cert" is more clear than "has_local_cert". The test
client class can have a cert without setting it.

+  }
+
+  void SetRemoteTransportDescription(
+      const rtc::scoped_refptr<rtc::RTCCertificate>& cert,
+      cricket::ContentAction action,
+      ConnectionRole role,
+      int flags) {
+    auto fingerprint = MakeFingerprintFromCertificate(cert);
+    cricket::TransportDescription remote_desc(
+        std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
+        role, fingerprint.get());
+
+    if (action == cricket::CA_OFFER) {
+      if (use_dtls_srtp_ && !(flags & NF_REOFFER)) {
+        // SRTP ciphers will be set only in the beginning.
+        for (std::vector<cricket::DtlsTransportChannelWrapper*>::iterator it =
+                 channels_.begin();
+             it != channels_.end(); ++it) {
+          std::vector<int> ciphers;
+          ciphers.push_back(rtc::SRTP_AES128_CM_SHA1_80);
+          EXPECT_TRUE((*it)->SetSrtpCryptoSuites(ciphers));
+        }
+      }

[pthatcher1, 2016/07/13 00:04:19]

It seems like a MakeTransportDescription(action, cert, role) would be useful
here, since this code is duplicated between SetRemoteTranportDescription and
SetLocalTransportDescription.

cricket::TransportDescription remote_desc = MakeTransportDescription(action,
cert, role);
bool expect_failure = ((action == CA_ANSWER) && (flags & NF_EXPECT_FAILURE));
EXPECT_EQ(!expect_failure, transport_->SetRemoteDescription(remote_desc, action,
nullptr));

[Taylor Brandstetter, 2016/07/13 16:17:41]

Setting the ciphers you mean? I'll make a separate method for that. But it
doesn't make sense to be in "MakeTransportDescription". Making the transport
description is only two statements, it doesn't need its own method.

Also, "EXPECT_EQ(!expect_failure" is a double negative; I'd prefer to keep it
how it is.

+
+      EXPECT_TRUE(transport_->SetRemoteTransportDescription(remote_desc, action,
+                                                            nullptr));
+    } else {
+      // If |expect_success| is false, expect SRTD or SLTD to fail when
+      // content action is CA_ANSWER.
+      bool expect_success = (flags & NF_EXPECT_FAILURE) ? false : true;
+      EXPECT_EQ(expect_success, transport_->SetRemoteTransportDescription(
+                                    remote_desc, action, nullptr));
+    }
+    set_remote_cert_ = (cert != nullptr);
+  }
+
   // Allow any DTLS configuration to be specified (including invalid ones).
   void Negotiate(const rtc::scoped_refptr<rtc::RTCCertificate>& local_cert,
                  const rtc::scoped_refptr<rtc::RTCCertificate>& remote_cert,
                  cricket::ContentAction action,
                  ConnectionRole local_role,
                  ConnectionRole remote_role,
                  int flags) {
-    std::unique_ptr<rtc::SSLFingerprint> local_fingerprint;
-    std::unique_ptr<rtc::SSLFingerprint> remote_fingerprint;
-    if (local_cert) {
-      std::string digest_algorithm;
-      ASSERT_TRUE(local_cert->ssl_certificate().GetSignatureDigestAlgorithm(
-          &digest_algorithm));
-      ASSERT_FALSE(digest_algorithm.empty());
-      local_fingerprint.reset(rtc::SSLFingerprint::Create(
-          digest_algorithm, local_cert->identity()));
-      ASSERT_TRUE(local_fingerprint.get() != NULL);
-      EXPECT_EQ(rtc::DIGEST_SHA_256, digest_algorithm);
+    if (action == cricket::CA_OFFER) {
+      SetLocalTransportDescription(local_cert, cricket::CA_OFFER, local_role,
+                                   flags);
+      SetRemoteTransportDescription(remote_cert, cricket::CA_ANSWER,
+                                    remote_role, flags);
+    } else {
+      SetRemoteTransportDescription(remote_cert, cricket::CA_OFFER, remote_role,
+                                    flags);
+      // If remote if the offerer and has no DTLS support, answer will be
+      // without any fingerprint.
+      SetLocalTransportDescription(remote_cert ? local_cert : nullptr,
+                                   cricket::CA_ANSWER, local_role, flags);
     }
-    if (remote_cert) {
-      std::string digest_algorithm;
-      ASSERT_TRUE(remote_cert->ssl_certificate().GetSignatureDigestAlgorithm(
-          &digest_algorithm));
-      ASSERT_FALSE(digest_algorithm.empty());
-      remote_fingerprint.reset(rtc::SSLFingerprint::Create(
-          digest_algorithm, remote_cert->identity()));
-      ASSERT_TRUE(remote_fingerprint.get() != NULL);
-      EXPECT_EQ(rtc::DIGEST_SHA_256, digest_algorithm);
-    }
-
-    if (use_dtls_srtp_ && !(flags & NF_REOFFER)) {
-      // SRTP ciphers will be set only in the beginning.
-      for (std::vector<cricket::DtlsTransportChannelWrapper*>::iterator it =
-           channels_.begin(); it != channels_.end(); ++it) {
-        std::vector<int> ciphers;
-        ciphers.push_back(rtc::SRTP_AES128_CM_SHA1_80);
-        ASSERT_TRUE((*it)->SetSrtpCryptoSuites(ciphers));
-      }
-    }
-
-    cricket::TransportDescription local_desc(
-        std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
-        local_role,
-        // If remote if the offerer and has no DTLS support, answer will be
-        // without any fingerprint.
-        (action == cricket::CA_ANSWER && !remote_cert)
-            ? nullptr
-            : local_fingerprint.get());
-
-    cricket::TransportDescription remote_desc(
-        std::vector<std::string>(), kIceUfrag1, kIcePwd1, cricket::ICEMODE_FULL,
-        remote_role, remote_fingerprint.get());
-
-    bool expect_success = (flags & NF_EXPECT_FAILURE) ? false : true;
-    // If |expect_success| is false, expect SRTD or SLTD to fail when
-    // content action is CA_ANSWER.
-    if (action == cricket::CA_OFFER) {
-      ASSERT_TRUE(transport_->SetLocalTransportDescription(
-          local_desc, cricket::CA_OFFER, NULL));
-      ASSERT_EQ(expect_success, transport_->SetRemoteTransportDescription(
-          remote_desc, cricket::CA_ANSWER, NULL));
-    } else {
-      ASSERT_TRUE(transport_->SetRemoteTransportDescription(
-          remote_desc, cricket::CA_OFFER, NULL));
-      ASSERT_EQ(expect_success, transport_->SetLocalTransportDescription(
-          local_desc, cricket::CA_ANSWER, NULL));
-    }
-    negotiated_dtls_ = (local_cert && remote_cert);
   }
 
   bool Connect(DtlsTestClient* peer, bool asymmetric) {
     transport_->SetDestination(peer->transport_.get(), asymmetric);
     return true;
   }
 
   bool all_channels_writable() const {
     if (channels_.empty()) {
       return false;
@@ skipping 15 matching lines @@
         return false;
       }
     }
     return true;
   }
 
   int received_dtls_client_hellos() const {
     return received_dtls_client_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_);
     }
   }
 
   void CheckSrtp(int expected_crypto_suite) {
     for (std::vector<cricket::DtlsTransportChannelWrapper*>::iterator it =
            channels_.begin(); it != channels_.end(); ++it) {
       int crypto_suite;
 
       bool rv = (*it)->GetSrtpCryptoSuite(&crypto_suite);
-      if (negotiated_dtls_ && expected_crypto_suite) {
+      if (negotiated_dtls() && expected_crypto_suite) {
         ASSERT_TRUE(rv);
 
         ASSERT_EQ(crypto_suite, expected_crypto_suite);
       } else {
         ASSERT_FALSE(rv);
       }
     }
   }
 
   void CheckSsl() {
     for (std::vector<cricket::DtlsTransportChannelWrapper*>::iterator it =
            channels_.begin(); it != channels_.end(); ++it) {
       int cipher;
 
       bool rv = (*it)->GetSslCipherSuite(&cipher);
-      if (negotiated_dtls_) {
+      if (negotiated_dtls()) {
         ASSERT_TRUE(rv);
 
         EXPECT_TRUE(
             rtc::SSLStreamAdapter::IsAcceptableCipher(cipher, rtc::KT_DEFAULT));
       } else {
         ASSERT_FALSE(rv);
       }
     }
   }
 
@@ skipping 108 matching lines @@
     ASSERT_EQ(0, flags);
 
     // Look at the handshake packets to see what role we played.
     // Check that non-handshake packets are DTLS data or SRTP bypass.
     if (data[0] == 22 && size > 17) {
       if (data[13] == 1) {
         ++received_dtls_client_hellos_;
       } else if (data[13] == 2) {
         ++received_dtls_server_hellos_;
       }
-    } else if (negotiated_dtls_ && !(data[0] >= 20 && data[0] <= 22)) {
+    } else if (negotiated_dtls() && !(data[0] >= 20 && data[0] <= 22)) {
       ASSERT_TRUE(data[0] == 23 || IsRtpLeadByte(data[0]));
       if (data[0] == 23) {
         ASSERT_TRUE(VerifyEncryptedPacket(data, size));
       } else if (IsRtpLeadByte(data[0])) {
         ASSERT_TRUE(VerifyPacket(data, size, NULL));
       }
     }
   }
 
  private:
   std::string name_;
   rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
   std::unique_ptr<cricket::FakeTransport> transport_;
   std::vector<cricket::DtlsTransportChannelWrapper*> channels_;
   size_t packet_size_ = 0u;
   std::set<int> received_;
   bool use_dtls_srtp_ = false;
   rtc::SSLProtocolVersion ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
-  bool negotiated_dtls_ = false;
+  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_;
 };
 
 // Note that this test always uses a FakeClock, due to the |fake_clock_| member
 // variable.
 class DtlsTransportChannelTest : public testing::Test {
  public:
   DtlsTransportChannelTest()
@@ skipping 29 matching lines @@
 
     if (c1)
       client1_.SetupSrtp();
     if (c2)
       client2_.SetupSrtp();
 
     if (c1 && c2)
       use_dtls_srtp_ = true;
   }
 
-  bool Connect(ConnectionRole client1_role, ConnectionRole client2_role) {
-    Negotiate(client1_role, client2_role);
+  // Negotiate local/remote fingerprint before or after the underlying
+  // tranpsort is connected?
+  enum NegotiateOrdering { NEGOTIATE_BEFORE_CONNECT, CONNECT_BEFORE_NEGOTIATE };
+  bool Connect(ConnectionRole client1_role,
+               ConnectionRole client2_role,
+               NegotiateOrdering ordering = NEGOTIATE_BEFORE_CONNECT) {
+    bool rv;
+    if (ordering == NEGOTIATE_BEFORE_CONNECT) {
+      Negotiate(client1_role, client2_role);
+      rv = client1_.Connect(&client2_, false);
+    } else {
+      client1_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLING);
+      client2_.SetupChannels(channel_ct_, cricket::ICEROLE_CONTROLLED);
+      // This is equivalent to an offer being processed on both sides, but an
+      // answer not yet being received on the initiating side. So the
+      // connection will be made before negotiation has finished on both sides.
+      client1_.SetLocalTransportDescription(client1_.certificate(),
+                                            cricket::CA_OFFER, client1_role, 0);
+      client2_.SetRemoteTransportDescription(
+          client1_.certificate(), cricket::CA_OFFER, client1_role, 0);
+      client2_.SetLocalTransportDescription(
+          client2_.certificate(), cricket::CA_ANSWER, client2_role, 0);
+      rv = client1_.Connect(&client2_, false);
+      client1_.SetRemoteTransportDescription(
+          client2_.certificate(), cricket::CA_ANSWER, client2_role, 0);
+    }
 
-    bool rv = client1_.Connect(&client2_, false);
     EXPECT_TRUE(rv);
     if (!rv)
       return false;
 
     EXPECT_TRUE_WAIT(
         client1_.all_channels_writable() && client2_.all_channels_writable(),
         kTimeout);
     if (!client1_.all_channels_writable() || !client2_.all_channels_writable())
       return false;
 
@@ skipping 537 matching lines @@
     // millisecond before the expected time and verify that no unexpected
     // retransmissions were sent. Then advance it the final millisecond and
     // verify that the expected retransmission was sent.
     fake_clock_.AdvanceTime(
         rtc::TimeDelta::FromMilliseconds(timeout_schedule_ms[i] - 1));
     EXPECT_EQ(expected_hellos, client1_.received_dtls_client_hellos());
     fake_clock_.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1));
     EXPECT_EQ(++expected_hellos, client1_.received_dtls_client_hellos());
   }
 }
+
+// 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);
+}