Add UMA histogram for Expect-CT header processing

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>
 #include <vector>
@@ skipping 1276 matching lines @@
       ssl_info.cert.get(), ENABLE_PIN_REPORTS, &unused_failure_log);
   return true;
 }
 
 void TransportSecurityState::ProcessExpectCTHeader(
     const std::string& value,
     const HostPortPair& host_port_pair,
     const SSLInfo& ssl_info) {
   DCHECK(CalledOnValidThread());
 
+  // Records the result of processing an Expect-CT header. This enum is
+  // histogrammed, so do not reorder or remove values.

[eroman, 2016/08/25 23:39:31]

Given this, can you assign values to *each* of the enums not just the first?

[estark, 2016/08/27 15:20:05]

Done.

+  enum ExpectCTHeaderResult {
+    // An Expect-CT header was received, but it had the wrong value.
+    EXPECT_CT_HEADER_BAD_VALUE = 0,
+    // The Expect-CT header was ignored because the build was old.
+    EXPECT_CT_HEADER_BUILD_NOT_TIMELY,
+    // The Expect-CT header was ignored because the certificate did not chain to
+    // a public root.
+    EXPECT_CT_HEADER_PRIVATE_ROOT,
+    // The Expect-CT header was ignored because CT compliance details were
+    // unavailable.
+    EXPECT_CT_HEADER_COMPLIANCE_DETAILS_UNAVAILABLE,
+    // The request satisified the Expect-CT compliance policy, so no action was
+    // taken.
+    EXPECT_CT_HEADER_COMPLIED,
+    // The Expect-CT header was ignored because there was no corresponding
+    // preload list entry.
+    EXPECT_CT_HEADER_NOT_PRELOADED,
+    // The Expect-CT header was processed successfully and passed on to the
+    // delegate to send a report.
+    EXPECT_CT_HEADER_PROCESSED,
+    EXPECT_CT_HEADER_MAX

[eroman, 2016/08/25 23:39:31]

I think it is more idiomatic to use a _LAST value and set it to the exact value
of last element (to prevent needing to handle this sentinel value inside of
switch statements).

Given this is only used locally in the function though, I guess it doesn't
really matter.

[estark, 2016/08/27 15:20:05]

Done.

+  };
+
+  ExpectCTHeaderResult result = EXPECT_CT_HEADER_PROCESSED;
+
   if (!expect_ct_reporter_)
     return;
 
-  if (value != "preload")
-    return;
-
-  if (!IsBuildTimely())
-    return;
-
-  if (!ssl_info.is_issued_by_known_root ||
-      !ssl_info.ct_compliance_details_available ||
-      ssl_info.ct_cert_policy_compliance ==
-          ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS) {
-    return;
+  ExpectCTState state;
+  if (value != "preload") {
+    result = EXPECT_CT_HEADER_BAD_VALUE;
+  } else if (!IsBuildTimely()) {
+    result = EXPECT_CT_HEADER_BUILD_NOT_TIMELY;
+  } else if (!ssl_info.is_issued_by_known_root) {
+    result = EXPECT_CT_HEADER_PRIVATE_ROOT;
+  } else if (!ssl_info.ct_compliance_details_available) {
+    result = EXPECT_CT_HEADER_COMPLIANCE_DETAILS_UNAVAILABLE;
+  } else if (ssl_info.ct_cert_policy_compliance ==
+             ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS) {
+    result = EXPECT_CT_HEADER_COMPLIED;
+  } else if (!GetStaticExpectCTState(host_port_pair.host(), &state)) {
+    result = EXPECT_CT_HEADER_NOT_PRELOADED;
   }
 
-  ExpectCTState state;
-  if (!GetStaticExpectCTState(host_port_pair.host(), &state))
+  UMA_HISTOGRAM_ENUMERATION("Net.ExpectCTHeaderResult", result,
+                            EXPECT_CT_HEADER_MAX);
+  if (result != EXPECT_CT_HEADER_PROCESSED)
     return;
 
   expect_ct_reporter_->OnExpectCTFailed(host_port_pair, state.report_uri,
                                         ssl_info);
 }
 
 // static
 void TransportSecurityState::ReportUMAOnPinFailure(const std::string& host) {
   PreloadResult result;
   if (!DecodeHSTSPreload(host, &result) ||
@@ skipping 299 matching lines @@
 TransportSecurityState::PKPStateIterator::PKPStateIterator(
     const TransportSecurityState& state)
     : iterator_(state.enabled_pkp_hosts_.begin()),
       end_(state.enabled_pkp_hosts_.end()) {
 }
 
 TransportSecurityState::PKPStateIterator::~PKPStateIterator() {
 }
 
 }  // namespace