Return enum from TransportSecurityState::CheckPublicKeyPins

net/quic/crypto/proof_verifier_chromium.cc

 // Copyright 2013 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.
 
 #include "net/quic/crypto/proof_verifier_chromium.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 318 matching lines @@
     }
 
     verify_details_->ct_verify_result.cert_policy_compliance =
         policy_enforcer_->DoesConformToCertPolicy(
             cert_verify_result.verified_cert.get(),
             verify_details_->ct_verify_result.verified_scts, net_log_);
   }
 
   if (transport_security_state_ &&
       (result == OK ||
-       (IsCertificateError(result) && IsCertStatusMinorError(cert_status))) &&
-      !transport_security_state_->CheckPublicKeyPins(
-          HostPortPair(hostname_, port_),
-          cert_verify_result.is_issued_by_known_root,
-          cert_verify_result.public_key_hashes, cert_.get(),
-          cert_verify_result.verified_cert.get(),
-          TransportSecurityState::ENABLE_PIN_REPORTS,
-          &verify_details_->pinning_failure_log)) {
-    if (cert_verify_result.is_issued_by_known_root)
-      result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
-    else
-      verify_details_->pkp_bypassed = true;
+       (IsCertificateError(result) && IsCertStatusMinorError(cert_status)))) {
+    TransportSecurityState::PKPStatus pin_validity =
+        transport_security_state_->CheckPublicKeyPins(
+            HostPortPair(hostname_, port_),
+            cert_verify_result.is_issued_by_known_root,
+            cert_verify_result.public_key_hashes, cert_.get(),
+            cert_verify_result.verified_cert.get(),
+            TransportSecurityState::ENABLE_PIN_REPORTS,
+            &verify_details_->pinning_failure_log);
+    switch (pin_validity) {
+      case TransportSecurityState::PKPStatus::VIOLATED:
+        result = ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
+        break;
+      case TransportSecurityState::PKPStatus::BYPASSED:
+        verify_details_->pkp_bypassed = true;
+        break;

[Ryan Sleevi, 2016/06/15 02:04:03]

Should this be an explicit/intentional

// Fall through

rather than break;

So that it's clear we're treating bypassed as equal to OK

[dadrian, 2016/06/15 02:47:55]

I'm wary of using any non-empty fall through, just in a case a future enum value
is added. Instead, we could add a comment indicating we're treating bypass as
equal to OK.

+      case TransportSecurityState::PKPStatus::OK:
+        // Do nothing.
+        break;
+    }
   }
 
   if (result != OK) {
     std::string error_string = ErrorToString(result);
     error_details_ = StringPrintf("Failed to verify certificate chain: %s",
                                   error_string.c_str());
     DLOG(WARNING) << error_details_;
   }
 
   // Exit DoLoop and return the result to the caller to VerifyProof.
@@ skipping 116 matching lines @@
   }
   return status;
 }
 
 void ProofVerifierChromium::OnJobComplete(Job* job) {
   active_jobs_.erase(job);
   delete job;
 }
 
 }  // namespace net