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

webrtc/base/opensslstreamadapter.h

Index: webrtc/base/opensslstreamadapter.h
diff --git a/webrtc/base/opensslstreamadapter.h b/webrtc/base/opensslstreamadapter.h
index 05e81021696162b626029bf2a9d723475305665c..d124798e370d7323d54fc842b733e4c4f1b29cc0 100644
--- a/webrtc/base/opensslstreamadapter.h
+++ b/webrtc/base/opensslstreamadapter.h
@@ -107,6 +107,8 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   bool SetDtlsSrtpCryptoSuites(const std::vector<int>& crypto_suites) override;
   bool GetDtlsSrtpCryptoSuite(int* crypto_suite) override;
 
+  bool IsTlsConnected() override { return state_ == SSL_CONNECTED; }
+
   // Capabilities interfaces
   static bool HaveDtls();
   static bool HaveDtlsSrtp();
@@ -133,6 +135,12 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
 
   enum { MSG_TIMEOUT = MSG_MAX+1};
 
+  enum SSLTopology {

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

SSL really only has one topology: client-server. The real question we're
addressing is how to validate the other side's certificate.

Fun question: if we're the *server* in a "traditional" / "client-server" pairing
and enable_client_auth() is true, what function will we call to set the other
side's certificate? Which of these "topologies" would we choose?

I think we'll want to drop this enum, for these reasons and those described
below.

[Taylor Brandstetter, 2016/07/22 18:52:20]

Enum dropped, as discussed.

[pthatcher1, 2016/07/22 18:58:27]

Nothing every uses StartSSLWithServer any more, so we could probably simplify
here a lot.

[Taylor Brandstetter, 2016/07/25 23:54:53]

Can we remove StartSSLWithServer in a separate CL, to keep this one focused?

+    TOPOLOGY_CLIENT_SERVER,
+    TOPOLOGY_PEER_TO_PEER,
+    TOPOLOGY_UNKNOWN
+  };
+
   // The following three methods return 0 on success and a negative
   // error code on failure. The error code may be from OpenSSL or -1
   // on some other error cases, so it can't really be interpreted
@@ -165,15 +173,26 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   // SSL library configuration
   SSL_CTX* SetupSSLContext();
   // SSL verification check
-  bool SSLPostConnectionCheck(SSL* ssl, const char* server_name,
-                              const X509* peer_cert,
-                              const std::string& peer_digest);
+  bool SSLPostConnectionCheck();
+  bool VerifyPeerCertificate();
   // SSL certification verification error handler, called back from
   // the openssl library. Returns an int interpreted as a boolean in
   // the C style: zero means verification failure, non-zero means
   // passed.
   static int SSLVerifyCallback(int ok, X509_STORE_CTX* store);
 
+  bool waiting_to_verify_client_cert() const {
+    // If |ssl_server_name_| is non-empty we're in client/server mode and don't
+    // need to verify a peer certificate.
+    return ssl_server_name_.empty() && client_auth_enabled() &&

[pthatcher1, 2016/07/22 18:58:27]

set_client_auth_enabled() is never called by anything other than unit tests, and
thus client_auth_enabled_ is always true, so we could probably just remove it.

If we remove that and StartSSLWithServer, then this whole method would be
!certificate_verified_, so you could just make it certificate_verified(), or
better yet, remote_certificate_verified().

[Taylor Brandstetter, 2016/07/25 23:54:53]

See above.

+           !certificate_verified_;
+  }
+
+  bool have_peer_certificate_digest() const {
+    return peer_certificate_digest_algorithm_.size() &&

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

personal preference: !xyz.empty()

[Taylor Brandstetter, 2016/07/22 18:52:20]

rtc::Buffer didn't have an "empty" method so I added one.

[pthatcher1, 2016/07/22 18:58:27]

+1

+           peer_certificate_digest_value_.size();
+  }
+
   SSLState state_;
   SSLRole role_;
   int ssl_error_code_;  // valid when state_ == SSL_ERROR or SSL_CLOSED
@@ -185,6 +204,7 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   SSL* ssl_;
   SSL_CTX* ssl_ctx_;
 
+  SSLTopology topology_ = TOPOLOGY_UNKNOWN;

[mattdr-at-webrtc.org, 2016/07/21 08:04:23]

It's nice to be explicit, but in a way this is duplicative state.

We're in TOPOLOGY_UNKNOWN iff state_ is SSL_NONE (thanks to StartSSL). If state_
is not SSL_NONE, then we're in TOPOLOGY_CLIENT_SERVER iff server_name_ is
nonnull. Thus, we can always compute this from existing fields.

Fields with redundant state are begging to get out of sync and break invariants.
:)

[Taylor Brandstetter, 2016/07/22 18:52:20]

You're very right. I replaced every "topology" check with a method that returns
a bool. I can have one method that returns an enum instead if you prefer that
flavor.

   // Our key and certificate, mostly useful in peer-to-peer mode.
   std::unique_ptr<OpenSSLIdentity> identity_;
   // in traditional mode, the server name that the server's certificate
@@ -197,6 +217,7 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   // the peer must present.
   Buffer peer_certificate_digest_value_;
   std::string peer_certificate_digest_algorithm_;
+  bool certificate_verified_ = false;

[pthatcher1, 2016/07/22 18:58:27]

Since the method that changes this is VerifyPeerCertificate and the certificate
is called peer_certificate_, shouldn't this be peer_certificate_verified_? 
Also, can you move this to right below peer_certificate_?

[Taylor Brandstetter, 2016/07/25 23:54:53]

Done.

 
   // OpenSSLAdapter::custom_verify_callback_ result

[pthatcher1, 2016/07/22 18:58:27]

Would it make sense to make everything "remote" as in "remote_certificate_"
instead of "peer" as in "peer_certificate_"?  That would be a lot more
consistent with all our other code.

[Taylor Brandstetter, 2016/07/25 23:54:53]

What about the PeerConnection? :)

Anyway you're probably right, but I think the renaming can happen in a later CL.

   bool custom_verification_succeeded_;