Return enum from TransportSecurityState::CheckPublicKeyPins

net/http/transport_security_state.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.
 
 #include "net/http/transport_security_state.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
@@ skipping 638 matching lines @@
   STSState static_sts_state;
   PKPState unused;
   if (GetStaticDomainState(host, &static_sts_state, &unused) &&
       static_sts_state.ShouldUpgradeToSSL()) {
     return true;
   }
 
   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,
     const X509Certificate* served_certificate_chain,
     const X509Certificate* validated_certificate_chain,
     const PublicKeyPinReportStatus report_status,
     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) {
   PKPState dynamic_state;
   if (GetDynamicPKPState(host, &dynamic_state))
     return dynamic_state.HasPublicKeyPins();
 
   STSState unused;
   PKPState static_pkp_state;
   if (GetStaticDomainState(host, &unused, &static_pkp_state)) {
@@ skipping 101 matching lines @@
 
     enabled_pkp_hosts_[HashHost(canonicalized_host)] = pkp_state;
   } else {
     const std::string hashed_host = HashHost(canonicalized_host);
     enabled_pkp_hosts_.erase(hashed_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,
     const HashValueVector& hashes,
     const X509Certificate* served_certificate_chain,
     const X509Certificate* validated_certificate_chain,
     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
   // report URI. The same report will not be sent within
   // |kTimeToRememberHPKPReportsMins|, which reduces load on servers and
   // 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(
     const std::string& host,
     ExpectCTState* expect_ct_state) const {
   DCHECK(CalledOnValidThread());
 
   if (!IsBuildTimely())
     return false;
 
@@ skipping 246 matching lines @@
       "Net.PublicKeyPinFailureDomain", result.domain_id);
 }
 
 // static
 bool TransportSecurityState::IsBuildTimely() {
   const base::Time build_time = base::GetBuildTime();
   // We consider built-in information to be timely for 10 weeks.
   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,
     const X509Certificate* served_certificate_chain,
     const X509Certificate* validated_certificate_chain,
     const PublicKeyPinReportStatus report_status,
     std::string* failure_log) {
   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;
+    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(
       host_port_pair, is_issued_by_known_root, pkp_state, hashes,
       served_certificate_chain, validated_certificate_chain, report_status,
       failure_log);
 }
 
 bool TransportSecurityState::GetStaticDomainState(const std::string& host,
                                                   STSState* sts_state,
@@ skipping 254 matching lines @@
 TransportSecurityState::PKPStateIterator::PKPStateIterator(
     const TransportSecurityState& state)
     : iterator_(state.enabled_pkp_hosts_.begin()),
       end_(state.enabled_pkp_hosts_.end()) {
 }
 
 TransportSecurityState::PKPStateIterator::~PKPStateIterator() {
 }
 
 }  // namespace