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 c762e9350adc2539c7d3263728ced8c5a1af2e98..50e9e496017dfd4d2fa822f8ad0d58c3567e432c 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -3448,13 +3448,24 @@ 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 (TransportSecurityState::IsBuildTimely() &&

[Ryan Sleevi, 2013/04/02 23:28:21]

I'm not so sure about moving this clause out, as it will prevent dynamic pins
from working.

[Ryan Sleevi, 2013/04/03 19:33:10]

The only thing I'd do is move the IsBuildTimely() check to the last of the
conditional, since everything above it is just an integer comparison, and the
function naming "suggests" it may be an expensive (eg: non-trivial) call.

This is just pedantry though.

[palmer, 2013/04/04 20:56:33]

You know I like pedantry. Done.

+      (result == OK ||
+       (IsCertificateError(result) && IsCertStatusMinorError(cert_status))) &&
       server_cert_verify_result_.is_issued_by_known_root &&
       transport_security_state_) {
     bool sni_available =
@@ -3465,18 +3476,13 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
     TransportSecurityState::DomainState domain_state;
     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 {
-        UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", true);
-      }
+        !domain_state.CheckPublicKeyPins(
+            server_cert_verify_result_.public_key_hashes)) {
+      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);

[Ryan Sleevi, 2013/04/03 19:33:10]

This is going to start artificially increasing this #.

In the previous code, we only UMA' success d on HasPublicKeyPins . You now UMA
it unconditionally (CheckPublicKeyPins returns true even if there are no pins)

[palmer, 2013/04/04 20:56:33]

Oops. Fixed.

     }
   }
 #endif