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

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 705df2d95f0d6e28a37a1e38f63feaf087969b68..1481c7cc1eb73f7acee0b4133bfff31a2e0a5ade 100644
--- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc
+++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc
@@ -124,74 +124,110 @@ class DtlsTestClient : public sigslot::has_slots<> {
               local_role, remote_role, flags);
   }
 
-  // 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 (remote_cert) {
+  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;
-      ASSERT_TRUE(remote_cert->ssl_certificate().GetSignatureDigestAlgorithm(
+      EXPECT_TRUE(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_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;
+  }
 
-    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));
-      }
-    }
-
+  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,
-        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());
+        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,
-        remote_role, remote_fingerprint.get());
+        role, 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));
+      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 {
-      ASSERT_TRUE(transport_->SetRemoteTransportDescription(
-          remote_desc, cricket::CA_OFFER, NULL));
-      ASSERT_EQ(expect_success, transport_->SetLocalTransportDescription(
-          local_desc, cricket::CA_ANSWER, NULL));
+      // 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) {
+    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);
     }
-    negotiated_dtls_ = (local_cert && remote_cert);
   }
 
   bool Connect(DtlsTestClient* peer, bool asymmetric) {
@@ -227,6 +263,8 @@ class DtlsTestClient : public sigslot::has_slots<> {
     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_);
@@ -243,7 +281,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
       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);
@@ -259,7 +297,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
       int cipher;
 
       bool rv = (*it)->GetSslCipherSuite(&cipher);
-      if (negotiated_dtls_) {
+      if (negotiated_dtls()) {
         ASSERT_TRUE(rv);
 
         EXPECT_TRUE(
@@ -388,7 +426,7 @@ class DtlsTestClient : public sigslot::has_slots<> {
       } 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));
@@ -407,7 +445,8 @@ 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 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_;
@@ -457,10 +496,33 @@ class DtlsTransportChannelTest : public testing::Test {
       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;
@@ -1018,3 +1080,14 @@ TEST_F(DtlsTransportChannelTest, TestRetransmissionSchedule) {
     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);
+}