Centralize the logic for checking public key pins

net/http/transport_security_state.cc

Index: net/http/transport_security_state.cc
diff --git a/net/http/transport_security_state.cc b/net/http/transport_security_state.cc
index 0b209b356e834dc7018bb0d9a743711b3e02cff5..485ba5542f7685da142dbb44ee964212738a828f 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -666,6 +666,57 @@ bool TransportSecurityState::AddHPKPHeader(const std::string& host,
   return false;
 }
 
+bool TransportSecurityState::VerifyPinning(
+    const CertVerifyResult& cert_verify_result,
+    bool sni_available,
+    const std::string& host,
+    std::string* pinning_failure_log) {
+

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

Delete the blank line.

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

Done.

+#if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS)
+  // 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).
+  //
+  const CertStatus cert_status = cert_verify_result.cert_status;

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

IMPORTANT: |cert_status| is not used. It seems that the policy of verifying
pinning only when the cert either has no errors or has only minor errors, should
also be centralized in this method.

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

I thought about that, but it seemed confusing to me because the error code which
the caller ends up with needs to be different if there were "real" errors vs,
minor errors:  The flow is:

if (result is major_error) 
  return result;

if (!PinsVerify())
  return ERR_CERT_PINNING_SOMETHING

return OK;

To move the first part of this logic into this method would require passing in
result and possibly returning it :/ This seems ugly. What do you think?

+  if (!cert_verify_result.is_issued_by_known_root ||

[Ryan Hamilton, 2014/08/05 17:42:36]

I wonder if I should pass in the individual fields of cert_verify_result?

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

That seems like a good idea. I am also tempted to break this into two methods.

1. A policy function that determines whether we should verify pinning or not.

2. A function that verifies pinning.

But that might be an overkill.

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

OK, changed the signature but kept it as one method.

+      !TransportSecurityState::IsBuildTimely()) {
+    return true;
+  }
+
+  if (!HasPublicKeyPins(host, sni_available))
+    return true;
+
+  if (CheckPublicKeyPins(host,
+                         sni_available,
+                         cert_verify_result.public_key_hashes,
+                         pinning_failure_log)) {
+    UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
+    return true;
+  }
+
+  LOG(ERROR) << *pinning_failure_log;
+  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", false);
+  TransportSecurityState::ReportUMAOnPinFailure(host);
+#else
+  return true;
+#endif
+}
+
 bool TransportSecurityState::AddHSTS(const std::string& host,
                                      const base::Time& expiry,
                                      bool include_subdomains) {