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

webrtc/base/opensslstreamadapter.cc

Index: webrtc/base/opensslstreamadapter.cc
diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc
index e04eb04d67c8d05eee38938f406484ccbe5d5247..3a152a557e39971da5bcbc8d0966ed885f0ed40f 100644
--- a/webrtc/base/opensslstreamadapter.cc
+++ b/webrtc/base/opensslstreamadapter.cc
@@ -313,7 +313,7 @@ bool OpenSSLStreamAdapter::SetPeerCertificateDigest(const std::string
                                                     const unsigned char*
                                                     digest_val,
                                                     size_t digest_len) {
-  ASSERT(!peer_certificate_);
+  ASSERT(!certificate_verified_);
   ASSERT(peer_certificate_digest_algorithm_.size() == 0);
   ASSERT(ssl_server_name_.empty());
   size_t expected_len;
@@ -322,12 +322,25 @@ bool OpenSSLStreamAdapter::SetPeerCertificateDigest(const std::string
     LOG(LS_WARNING) << "Unknown digest algorithm: " << digest_alg;
     return false;
   }
-  if (expected_len != digest_len)
+  if (expected_len != digest_len) {
     return false;
+  }
 
   peer_certificate_digest_value_.SetData(digest_val, digest_len);
   peer_certificate_digest_algorithm_ = digest_alg;
 
+  if (peer_certificate_) {
+    if (VerifyPeerCertificate()) {
+      if (state_ == SSL_CONNECTED) {
+        // Post event to unwind stack. Caller may not be expecting this event
+        // to occur synchronously.
+        PostEvent(SE_OPEN | SE_READ | SE_WRITE, 0);
+      }
+    } else {
+      Error("SetPeerCertificateDigest", -1, false);

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

instead of -1, X509_V_ERR_CERT_UNTRUSTED might be reasonable.

Should the "signal" argument here be true?

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

No, in this case the caller can check the return value so no signal is
necessary. The signal seems to be meant for asynchronous events.

+      return false;
+    }
+  }
   return true;
 }
 
@@ -508,6 +521,9 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data, size_t data_len,
     return SR_BLOCK;
 
   case SSL_CONNECTED:
+    if (client_auth_enabled() && !certificate_verified_) {

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

We repeat this expression more than a few times. Maybe give it a name?

if (waiting_for_client_cert()) ...

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

Good idea, done.

+      return SR_BLOCK;
+    }
     break;
 
   case SSL_ERROR:
@@ -567,6 +583,9 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, size_t data_len,
       return SR_BLOCK;
 
     case SSL_CONNECTED:
+      if (client_auth_enabled() && !certificate_verified_) {
+        return SR_BLOCK;
+      }
       break;
 
     case SSL_CLOSED:
@@ -669,6 +688,9 @@ StreamState OpenSSLStreamAdapter::GetState() const {
     case SSL_CONNECTING:
       return SS_OPENING;
     case SSL_CONNECTED:
+      if (client_auth_enabled() && !certificate_verified_) {
+        return SS_OPENING;
+      }
       return SS_OPEN;
     default:
       return SS_CLOSED;
@@ -749,10 +771,6 @@ int OpenSSLStreamAdapter::StartSSL() {
 
 int OpenSSLStreamAdapter::BeginSSL() {
   ASSERT(state_ == SSL_CONNECTING);
-  // The underlying stream has open. If we are in peer-to-peer mode
-  // then a peer certificate must have been specified by now.
-  ASSERT(!ssl_server_name_.empty() ||
-         !peer_certificate_digest_algorithm_.empty());
   LOG(LS_INFO) << "BeginSSL: "
                << (!ssl_server_name_.empty() ? ssl_server_name_ :
                                                "with peer");
@@ -825,14 +843,21 @@ int OpenSSLStreamAdapter::ContinueSSL() {
     case SSL_ERROR_NONE:
       LOG(LS_VERBOSE) << " -- success";
 
-      if (!SSLPostConnectionCheck(ssl_, ssl_server_name_.c_str(), NULL,
-                                  peer_certificate_digest_algorithm_)) {
+      if (!SSLPostConnectionCheck(
+              ssl_, ssl_server_name_.c_str(),
+              peer_certificate_ ? peer_certificate_->x509() : nullptr)) {
         LOG(LS_ERROR) << "TLS post connection check failed";
         return -1;
       }
 
       state_ = SSL_CONNECTED;
-      StreamAdapterInterface::OnEvent(stream(), SE_OPEN|SE_READ|SE_WRITE, 0);
+      if (!client_auth_enabled() || certificate_verified_) {
+        // If the certificate is not verified (because we're in peer-to-peer

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

Make it a bit more obvious here that you're talking about what *isn't* happening
here.

e.g.

We have everything we need to start the connection, so signal SE_OPEN. If we
need a client certificate fingerprint and don't have it yet, we'll instead
signal SE_OPEN in SetPeerCertificateDigest.

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

Done.

+        // mode and the digest is not yet known), SE_OPEN will be signaled in
+        // SetPeerCertificateDigest.
+        StreamAdapterInterface::OnEvent(stream(), SE_OPEN | SE_READ | SE_WRITE,
+                                        0);
+      }
       break;
 
     case SSL_ERROR_WANT_READ: {
@@ -1042,43 +1067,23 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() {
   return ctx;
 }
 
-int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
-  // Get our SSL structure from the store
-  SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
-                                        store,
-                                        SSL_get_ex_data_X509_STORE_CTX_idx()));
-  OpenSSLStreamAdapter* stream =
-    reinterpret_cast<OpenSSLStreamAdapter*>(SSL_get_app_data(ssl));
-
-  if (stream->peer_certificate_digest_algorithm_.empty()) {
-    return 0;
-  }
-  X509* cert = X509_STORE_CTX_get_current_cert(store);
-  int depth = X509_STORE_CTX_get_error_depth(store);
-
-  // For now We ignore the parent certificates and verify the leaf against
-  // the digest.
-  //
-  // TODO(jiayl): Verify the chain is a proper chain and report the chain to
-  // |stream->peer_certificate_|.
-  if (depth > 0) {
-    LOG(LS_INFO) << "Ignored chained certificate at depth " << depth;
-    return 1;
+bool OpenSSLStreamAdapter::VerifyPeerCertificate() {
+  if (peer_certificate_digest_algorithm_.empty() || !peer_certificate_) {
+    LOG(LS_WARNING) << "Missing digest or peer certificate.";
+    return false;
   }
 
   unsigned char digest[EVP_MAX_MD_SIZE];
   size_t digest_length;
   if (!OpenSSLCertificate::ComputeDigest(
-           cert,
-           stream->peer_certificate_digest_algorithm_,
-           digest, sizeof(digest),
-           &digest_length)) {
+          peer_certificate_->x509(), peer_certificate_digest_algorithm_, digest,
+          sizeof(digest), &digest_length)) {
     LOG(LS_WARNING) << "Failed to compute peer cert digest.";
-    return 0;
+    return false;
   }
 
   Buffer computed_digest(digest, digest_length);
-  if (computed_digest != stream->peer_certificate_digest_value_) {
+  if (computed_digest != peer_certificate_digest_value_) {
     LOG(LS_WARNING) << "Rejected peer certificate due to mismatched digest.";
     return 0;
   }
@@ -1086,32 +1091,60 @@ int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
   // value in checking the validity of a self-signed cert issued by untrusted
   // sources.
   LOG(LS_INFO) << "Accepted peer certificate.";
+  certificate_verified_ = true;
+  return true;
+}
+
+int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
+  // Get our SSL structure from the store
+  SSL* ssl = reinterpret_cast<SSL*>(
+      X509_STORE_CTX_get_ex_data(store, SSL_get_ex_data_X509_STORE_CTX_idx()));
+  X509* cert = X509_STORE_CTX_get_current_cert(store);
+  int depth = X509_STORE_CTX_get_error_depth(store);
+
+  // For now we ignore the parent certificates and verify the leaf against
+  // the digest.
+  //
+  // TODO(jiayl): Verify the chain is a proper chain and report the chain to
+  // |stream->peer_certificate_|.
+  if (depth > 0) {
+    LOG(LS_INFO) << "Ignored chained certificate at depth " << depth;
+    return 1;
+  }
+
+  OpenSSLStreamAdapter* stream =
+      reinterpret_cast<OpenSSLStreamAdapter*>(SSL_get_app_data(ssl));
 
   // Record the peer's certificate.
   stream->peer_certificate_.reset(new OpenSSLCertificate(cert));
-  return 1;
+
+  // If the peer certificate digest isn't known yet, we'll wait to verify
+  // until it's known, and for now just return a success status.
+  if (stream->peer_certificate_digest_algorithm_.empty()) {

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

do we really want to use the digest *algorithm* variable everywhere as a
sentinel to whether we have the *digest*?

The former would be something like "SHA256", while the latter will be a binary
or hex string.

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

Both should always be set at the same time. But this could change in the future,
so I added a "have_peer_certificate_digest" function.

+    LOG(LS_INFO) << "Waiting to verify certificate until digest is known.";
+    return 1;
+  }
+
+  return stream->VerifyPeerCertificate() ? 1 : 0;

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

bool coerces to 1 or 0, so this should be redundant.

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

I always forget this. Done.

 }
 
 // This code is taken from the "Network Security with OpenSSL"
 // sample in chapter 5
 bool OpenSSLStreamAdapter::SSLPostConnectionCheck(SSL* ssl,
                                                   const char* server_name,
-                                                  const X509* peer_cert,
-                                                  const std::string
-                                                  &peer_digest) {
+                                                  const X509* peer_cert) {

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

This function doesn't really need the peer certificate; it just needs to know if
we *have* a peer certificate. Maybe consider a bool?

Then again, it seems like the class assumes an invariant where either we get a
real server name or we're using peer-to-peer auth and have a peer cert. Is that
the case?

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

In fact I'm not sure why this method takes any parameters since they're all
member variables. Maybe historical reasons. I went ahead and cleaned it up.

As for the invariant: it definitely seems to be the case, though it's only
strictly enforced by ASSERTs. If you call "StartSSLWithServer" and then
"StartSSLWithPeer" and asserts are disabled you'll be left in a weird state. I
went ahead and made the enforcement (and "which topology am I using" check) a
bit more explicit.

   ASSERT(server_name != NULL);
   bool ok;
   if (server_name[0] != '\0') {  // traditional mode
     ok = OpenSSLAdapter::VerifyServerName(ssl, server_name, ignore_bad_cert());
 
     if (ok) {
-      ok = (SSL_get_verify_result(ssl) == X509_V_OK ||
-            custom_verification_succeeded_);
+      ok = certificate_verified_ = (SSL_get_verify_result(ssl) == X509_V_OK ||
+                                    custom_verification_succeeded_);

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

Let's avoid chained assignment.

In fact, I'd prefer if rely on the *caller* of SSLPostConnectionCheck to set
certificate_verified_. Setting this member variable is sort of a nonobvious side
effect of calling this.

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

This is no longer necessary, since I have "waiting_to_verify_client_cert()" now
which returns false in client/server mode.

     }
   } else {  // peer-to-peer mode
-    ASSERT((peer_cert != NULL) || (!peer_digest.empty()));
     // no server name validation
-    ok = true;
+    ok = peer_cert != nullptr || !client_auth_enabled();
   }
 
   if (!ok && ignore_bad_cert()) {