Return enum from TransportSecurityState::CheckPublicKeyPins

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 34e53713b93524540c9caf36741c32c5aed42250..9b09d48ac2a03af69905a72bd618640f61b763c2 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -656,7 +656,7 @@ bool TransportSecurityState::ShouldUpgradeToSSL(const std::string& host) {
   return false;
 }
 
-bool TransportSecurityState::CheckPublicKeyPins(
+TransportSecurityState::PKPStatus TransportSecurityState::CheckPublicKeyPins(
     const HostPortPair& host_port_pair,
     bool is_issued_by_known_root,
     const HashValueVector& public_key_hashes,
@@ -666,25 +666,26 @@ bool TransportSecurityState::CheckPublicKeyPins(
     std::string* pinning_failure_log) {
   // Perform pin validation only if the server actually has public key pins.
   if (!HasPublicKeyPins(host_port_pair.host())) {
-    return true;
+    return PKPStatus::OK;
   }
 
-  bool pins_are_valid = CheckPublicKeyPinsImpl(
+  PKPStatus pin_validity = CheckPublicKeyPinsImpl(
       host_port_pair, is_issued_by_known_root, public_key_hashes,
       served_certificate_chain, validated_certificate_chain, report_status,
       pinning_failure_log);
+
   // Don't track statistics when a local trust anchor would override the pinning
   // anyway.
   if (!is_issued_by_known_root)
-    return pins_are_valid;
+    return pin_validity;
 
-  if (!pins_are_valid) {
+  if (pin_validity == PKPStatus::VIOLATED) {
     LOG(ERROR) << *pinning_failure_log;
     ReportUMAOnPinFailure(host_port_pair.host());
   }
-
-  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pins_are_valid);
-  return pins_are_valid;
+  bool pin_success = (pin_validity == PKPStatus::OK);
+  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pin_success);

[estark, 2016/06/21 18:13:42]

nit: you could just inline this

[dadrian, 2016/06/21 18:28:18]

Done.

+  return pin_validity;
 }
 
 bool TransportSecurityState::HasPublicKeyPins(const std::string& host) {
@@ -806,7 +807,8 @@ void TransportSecurityState::EnablePKPHost(const std::string& host,
   DirtyNotify();
 }
 
-bool TransportSecurityState::CheckPinsAndMaybeSendReport(
+TransportSecurityState::PKPStatus
+TransportSecurityState::CheckPinsAndMaybeSendReport(
     const HostPortPair& host_port_pair,
     bool is_issued_by_known_root,
     const TransportSecurityState::PKPState& pkp_state,
@@ -816,26 +818,30 @@ bool TransportSecurityState::CheckPinsAndMaybeSendReport(
     const TransportSecurityState::PublicKeyPinReportStatus report_status,
     std::string* failure_log) {
   if (pkp_state.CheckPublicKeyPins(hashes, failure_log))
-    return true;
+    return PKPStatus::OK;
 
-  if (!report_sender_ || !is_issued_by_known_root ||
+  // Don't report violations for certificates that chain to local roots.
+  if (!is_issued_by_known_root)
+    return PKPStatus::BYPASSED;
+
+  if (!report_sender_ ||
       report_status != TransportSecurityState::ENABLE_PIN_REPORTS ||
       pkp_state.report_uri.is_empty()) {
-    return false;
+    return PKPStatus::VIOLATED;
   }
 
   DCHECK(pkp_state.report_uri.is_valid());
   // Report URIs should not be used if they are the same host as the pin
   // and are HTTPS, to avoid going into a report-sending loop.
   if (!IsReportUriValidForHost(pkp_state.report_uri, host_port_pair.host()))
-    return false;
+    return PKPStatus::VIOLATED;
 
   std::string serialized_report;
   std::string report_cache_key;
   if (!GetHPKPReport(host_port_pair, pkp_state, served_certificate_chain,
                      validated_certificate_chain, &serialized_report,
                      &report_cache_key)) {
-    return false;
+    return PKPStatus::VIOLATED;
   }
 
   // Limit the rate at which duplicate reports are sent to the same
@@ -844,14 +850,14 @@ bool TransportSecurityState::CheckPinsAndMaybeSendReport(
   // also prevents accidental loops (a.com triggers a report to b.com
   // which triggers a report to a.com). See section 2.1.4 of RFC 7469.
   if (sent_reports_cache_.Get(report_cache_key, base::TimeTicks::Now()))
-    return false;
+    return PKPStatus::VIOLATED;
   sent_reports_cache_.Put(
       report_cache_key, true, base::TimeTicks::Now(),
       base::TimeTicks::Now() +
           base::TimeDelta::FromMinutes(kTimeToRememberHPKPReportsMins));
 
   report_sender_->Send(pkp_state.report_uri, serialized_report);
-  return false;
+  return PKPStatus::VIOLATED;
 }
 
 bool TransportSecurityState::GetStaticExpectCTState(
@@ -1118,7 +1124,8 @@ bool TransportSecurityState::IsBuildTimely() {
   return (base::Time::Now() - build_time).InDays() < 70 /* 10 weeks */;
 }
 
-bool TransportSecurityState::CheckPublicKeyPinsImpl(
+TransportSecurityState::PKPStatus
+TransportSecurityState::CheckPublicKeyPinsImpl(
     const HostPortPair& host_port_pair,
     bool is_issued_by_known_root,
     const HashValueVector& hashes,
@@ -1129,11 +1136,15 @@ bool TransportSecurityState::CheckPublicKeyPinsImpl(
   PKPState pkp_state;
   STSState unused;
 
-  if (!GetDynamicPKPState(host_port_pair.host(), &pkp_state) &&
-      !GetStaticDomainState(host_port_pair.host(), &unused, &pkp_state)) {
-    // HasPublicKeyPins should have returned true in order for this method
-    // to have been called, so if we fall through to here, it's an error.
-    return false;
+  bool found_state =
+      GetDynamicPKPState(host_port_pair.host(), &pkp_state) ||
+      GetStaticDomainState(host_port_pair.host(), &unused, &pkp_state);
+
+  // HasPublicKeyPins should have returned true in order for this method to have
+  // been called, so if found_state is false, its an error.
+  DCHECK(found_state);
+  if (!found_state) {

[estark, 2016/06/21 18:13:42]

No need to handle this case. You should just DCHECK(found_state) and then move
on to `return CheckPinsAndMaybeSendReport(...)`. (See
https://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...)

[dadrian, 2016/06/21 18:28:18]

Done.

+    return PKPStatus::VIOLATED;
   }
 
   return CheckPinsAndMaybeSendReport(