Clean up comments and code for pin validation.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 0ac5a83a0fa3440830e5840fc900d69138263238..ffefef010b412672447b416b859b1a9e18b22cf1 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -3432,15 +3432,26 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
   // 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
+  // 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:
+  //
+  // * the build is recent (very old builds should fail open so that users
+  //   have some chance to recover);
+  // * 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
+  // * a TransportSecurityState object is available.
+  //
   const CertStatus cert_status = server_cert_verify_result_.cert_status;
-  if ((result == OK || (IsCertificateError(result) &&
-                        IsCertStatusMinorError(cert_status))) &&
+  if (transport_security_state_ &&
+      (result == OK ||
+       (IsCertificateError(result) && IsCertStatusMinorError(cert_status))) &&
       server_cert_verify_result_.is_issued_by_known_root &&
-      transport_security_state_) {
+      TransportSecurityState::IsBuildTimely()) {
     bool sni_available =
         ssl_config_.version_max >= SSL_PROTOCOL_VERSION_TLS1 ||
         ssl_config_.version_fallback;
@@ -3450,16 +3461,15 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
     if (transport_security_state_->GetDomainState(host, sni_available,
                                                   &domain_state) &&
         domain_state.HasPublicKeyPins()) {
-      if (!domain_state.CheckPublicKeyPins(
-               server_cert_verify_result_.public_key_hashes)) {
-        // Pins are not enforced if the build is too old.
-        if (TransportSecurityState::IsBuildTimely()) {
-          result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
-          UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", false);
-          TransportSecurityState::ReportUMAOnPinFailure(host);
-        }
-      } else {
+      if (domain_state.CheckPublicKeyPins(
+              server_cert_verify_result_.public_key_hashes)) {

[Ryan Sleevi, 2013/04/04 21:24:02]

nit: The original form matches our common pattern of "Handle errors first" - the
point being so that errors are close to their conditionals.

[palmer, 2013/04/04 21:34:53]

Done.

         UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
+  DLOG(ERROR) << "pin success";
+      } else {
+        result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
+        UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", false);
+        TransportSecurityState::ReportUMAOnPinFailure(host);
+  DLOG(ERROR) << "pin failure";
       }
     }
   }