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..4d575892c443962fbc7b1105dd758b79d15e98c3 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,25 @@ 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;
-
-  if (!pins_are_valid) {
-    LOG(ERROR) << *pinning_failure_log;
-    ReportUMAOnPinFailure(host_port_pair.host());
+  if (is_issued_by_known_root) {
+    if (pin_validity == PKPStatus::VIOLATED) {
+      LOG(ERROR) << *pinning_failure_log;
+      ReportUMAOnPinFailure(host_port_pair.host());
+    }
+    bool pin_success = (pin_validity == PKPStatus::OK);
+    UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pin_success);
   }
-
-  UMA_HISTOGRAM_BOOLEAN("Net.PublicKeyPinSuccess", pins_are_valid);
-  return pins_are_valid;
+  return pin_validity;

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

I'm not sure why you changed it, but we prefer to do short-circuits early.

if (!is_issued_by_known_root)
  return pin_validity;
if (pin_validity == ... ) {
  LOG(ERROR) << *pinning_failure_log;  // rsleevi note: No idea why we're doing
this and I don't think it's right but I'll let Emily chime in
  ReportUMAOnPinFailure(...);  // No idea why we're doing this either since I
doubt anyone's checking, but again defer to Emily but I suspect it's been broken
since the new generator landed
}
UMA_HISTOGRAM_BOOLEAN(..., pin_validity == PKPStatus::OK);

[estark, 2016/06/15 02:35:00]

Both |pinning_failure_log| and the UMA histograms long predate me. AFAIK nobody
looks at them and we should probably remove them.

What do you mean by "the new generator" though?

[Ryan Sleevi, 2016/06/15 02:44:36]

In order for the histograms to be useful, the UMA ID must never change, and gaps
must never be added. I'm not sure with Lucas' script to generate the preload
dataset if we're still preserving the immutability of that - I'd be surprised if
we were, because that requires a state going back to the first time we were
doing the preloads.

Otherwise, the UMA is only valid for the TransportSecurityState version of the
dataset, which isn't that useful.

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

|pinning_failure_log| is added to the certificate error reports, but that's it.

[estark, 2016/06/15 02:48:04]

Ah, I see.

[dadrian, 2016/06/15 18:58:28]

Added the bail-out-early, but am currently leaving the UMA code for another CL.

 }
 
 bool TransportSecurityState::HasPublicKeyPins(const std::string& host) {
@@ -806,7 +806,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 +817,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 +849,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 +1123,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,
@@ -1133,7 +1139,7 @@ bool TransportSecurityState::CheckPublicKeyPinsImpl(
       !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;
+    return PKPStatus::VIOLATED;

[estark, 2016/06/15 02:48:04]

This whole little block should really be a DCHECK, I think.

bool found_state = GetDynamicPKPState(...) || GetStaticDomainState(...);
DCHECK(found_state);

Maybe that change doesn't belong in this CL though.

[dadrian, 2016/06/15 18:58:28]

Acknowledged.

   }
 
   return CheckPinsAndMaybeSendReport(