Centralize the logic for checking public key pins

net/quic/crypto/proof_verifier_chromium.cc

Index: net/quic/crypto/proof_verifier_chromium.cc
diff --git a/net/quic/crypto/proof_verifier_chromium.cc b/net/quic/crypto/proof_verifier_chromium.cc
index 5f70199a5058af1ebecaa2a03411d3006171e858..de4b28dd28440fa1074d7135cefb2bd214d25386 100644
--- a/net/quic/crypto/proof_verifier_chromium.cc
+++ b/net/quic/crypto/proof_verifier_chromium.cc
@@ -236,58 +236,24 @@ int ProofVerifierChromium::Job::DoVerifyCert(int result) {
 int ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
   verifier_.reset();
 
-#if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS)
-  // TODO(wtc): The following code was copied from ssl_client_socket_nss.cc.
-  // Convert it to a new function that can be called by both files. These
-  // variables simulate the arguments to the new function.
   const CertVerifyResult& cert_verify_result =

[wtc, 2014/08/05 18:16:24]

Get rid of the |cert_verify_result| local variable.

[Ryan Hamilton, 2014/08/06 21:51:01]

Discussed offline. Keeping.

-      verify_details_->cert_verify_result;
-  bool sni_available = true;
-  const std::string& host = hostname_;
-  TransportSecurityState* transport_security_state = transport_security_state_;
-  std::string* pinning_failure_log = &verify_details_->pinning_failure_log;
-
-  // Take care of any mandates for public key pinning.
-  //
-  // Pinning is only enabled for official builds to make sure that others don't
-  // end up with pins that cannot be easily updated.
-  //
-  // TODO(agl): We might have an issue here where a request for foo.example.com
-  // merges into a SPDY connection to www.example.com, and gets a different
-  // certificate.
-
-  // Perform pin validation if, and only if, all these conditions obtain:
-  //
-  // * a TransportSecurityState object is available;
-  // * the server's certificate chain is valid (or suffers from only a minor
-  //   error);
-  // * the server's certificate chain chains up to a known root (i.e. not a
-  //   user-installed trust anchor); and
-  // * the build is recent (very old builds should fail open so that users
-  //   have some chance to recover).
-  //
+    verify_details_->cert_verify_result;
   const CertStatus cert_status = cert_verify_result.cert_status;
-  if (transport_security_state &&
-      (result == OK ||
-       (IsCertificateError(result) && IsCertStatusMinorError(cert_status))) &&
-      cert_verify_result.is_issued_by_known_root &&
-      TransportSecurityState::IsBuildTimely()) {
-    if (transport_security_state->HasPublicKeyPins(host, sni_available)) {
-      if (!transport_security_state->CheckPublicKeyPins(
-              host,
-              sni_available,
-              cert_verify_result.public_key_hashes,
-              pinning_failure_log)) {
-        LOG(ERROR) << *pinning_failure_log;
-        result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
-        UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", false);
-        TransportSecurityState::ReportUMAOnPinFailure(host);
-      } else {
-        UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
-      }
-    }
+  if (result != OK &&
+      (!IsCertificateError(result) || !IsCertStatusMinorError(cert_status))) {
+    return result;
+  }

[wtc, 2014/08/05 18:16:24]

BUG: delete this early return (lines 242-245).

[Ryan Hamilton, 2014/08/06 21:51:01]

Done.

+
+  if ((result == OK ||
+       (IsCertificateError(result) || IsCertStatusMinorError(cert_status))) &&
+      transport_security_state_ &&
+      !transport_security_state_->VerifyPinning(

[wtc, 2014/08/05 18:16:24]

Nit: please make these four lines look exactly the same as the corresponding
four lines in ssl_client_socket_nss.cc.

[Ryan Hamilton, 2014/08/06 21:51:01]

Done.

+          cert_verify_result,
+          true,
+          hostname_,
+          &verify_details_->pinning_failure_log)) {
+    result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
   }
-#endif
 
   if (result != OK) {
     error_details_ = StringPrintf("Failed to verify certificate chain: %s",