Clean up comments and code for pin validation.

net/socket/ssl_client_socket_nss.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 3430 matching lines @@
     LogConnectionTypeMetrics();
 
   completed_handshake_ = true;
 
 #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
+  // 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 =
         ssl_config_.version_max >= SSL_PROTOCOL_VERSION_TLS1 ||
         ssl_config_.version_fallback;
     const std::string& host = host_and_port_.host();
 
     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
 
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
 
 void SSLClientSocketNSS::LogConnectionTypeMetrics() const {
@@ skipping 40 matching lines @@
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 ServerBoundCertService* SSLClientSocketNSS::GetServerBoundCertService() const {
   return server_bound_cert_service_;
 }
 
 }  // namespace net