Certificate Transparency: Adding finch and NetLog logging for EV certs

net/cert/cert_policy_enforcer.cc

 // Copyright 2014 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/cert/cert_policy_enforcer.h"
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/build_time.h"
+#include "base/callback_helpers.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+#include "net/base/net_log.h"
 #include "net/cert/ct_ev_whitelist.h"
 #include "net/cert/ct_verify_result.h"
 #include "net/cert/signed_certificate_timestamp.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_certificate_net_log_param.h"
 
 namespace net {
 
 namespace {
 
 bool IsEmbeddedSCT(const scoped_refptr<ct::SignedCertificateTimestamp>& sct) {
   return sct->origin == ct::SignedCertificateTimestamp::SCT_EMBEDDED;
 }
 
 // Returns true if the current build is recent enough to ensure that
@@ skipping 30 matching lines @@
   CT_IN_WHITELIST = 1,
   CT_ENOUGH_SCTS = 2,
   CT_COMPLIANCE_MAX,
 };
 
 void LogCTComplianceStatusToUMA(CTComplianceStatus status) {
   UMA_HISTOGRAM_ENUMERATION("Net.SSL_EVCertificateCTCompliance", status,
                             CT_COMPLIANCE_MAX);
 }
 
+base::Value* NetLogNonCompliantCertCallback(X509Certificate* cert,
+                                            NetLog::LogLevel log_level) {
+  base::DictionaryValue* dict = new base::DictionaryValue();
+  dict->Set("non_compliant_cert",
+            NetLogX509CertificateCallback(cert, log_level));
+  return dict;
+}
+
 }  // namespace
 
 CertPolicyEnforcer::CertPolicyEnforcer(size_t num_ct_logs,
                                        bool require_ct_for_ev)
     : num_ct_logs_(num_ct_logs), require_ct_for_ev_(require_ct_for_ev) {
 }
 
 CertPolicyEnforcer::~CertPolicyEnforcer() {
 }
 
 bool CertPolicyEnforcer::DoesConformToCTEVPolicy(
     X509Certificate* cert,
     const ct::EVCertsWhitelist* ev_whitelist,
-    const ct::CTVerifyResult& ct_result) {
+    const ct::CTVerifyResult& ct_result,
+    const BoundNetLog& net_log) {
   if (!require_ct_for_ev_)
     return true;
 
   if (!IsBuildTimely())
     return false;
 
   if (IsCertificateInWhitelist(cert, ev_whitelist)) {
     LogCTComplianceStatusToUMA(CT_IN_WHITELIST);
     return true;
   }
 
   if (HasRequiredNumberOfSCTs(cert, ct_result)) {
     LogCTComplianceStatusToUMA(CT_ENOUGH_SCTS);
     return true;
   }
 
   LogCTComplianceStatusToUMA(CT_NOT_COMPLIANT);
+
+  NetLog::ParametersCallback net_log_callback =
+      base::Bind(&NetLogNonCompliantCertCallback, base::Unretained(cert));
+
+  net_log.AddEvent(NetLog::TYPE_NON_COMPLIANT_EV_CERT_ENCOUNTERED,
+                   net_log_callback);

[Ryan Sleevi, 2014/12/09 20:50:42]

Ok, I'm going to push back on this with a "Not LGTM" for now. I know, I know, a
pain.

The problem is that you're only logging a failure path, instead of any of the
other (relevant) details that would be neded during debugging (namely, line 97,
100, 104, 109)

Rather than focusing on a single Event that indicates failure, you should look
to log a start/end event with relevant details. See the CertVerifier as an
example, where the begin event includes the input parameters (for example, the
ct_result, potentially whitelist metadata in the future) and the end event logs
the result (which of the paths this function exited)

[Eran Messeri, 2014/12/10 15:38:38]

I understand the main concern is not logging all paths and all relevant details
that are necessary to debug why the EV treatment was not granted.

Per your suggestion I have changed the code to log an event in all circumstances
with all details.

I have tried using BeginEvent/EndEvent but that ended up looking a bit funny, as
in practice the code is much simpler if I do all the checks and NetLog the
result (which contains all the information) in a single place.

   return false;
 }
 
 bool CertPolicyEnforcer::IsCertificateInWhitelist(
     X509Certificate* cert,
     const ct::EVCertsWhitelist* ev_whitelist) {
   bool cert_in_ev_whitelist = false;
   if (ev_whitelist && ev_whitelist->IsValid()) {
     const SHA256HashValue fingerprint(
         X509Certificate::CalculateFingerprint256(cert->os_cert_handle()));
@@ skipping 47 matching lines @@
   } else {
     num_required_embedded_scts = 2;
   }
 
   size_t min_acceptable_logs = std::max(num_ct_logs_, static_cast<size_t>(2u));
   return num_embedded_scts >=
          std::min(num_required_embedded_scts, min_acceptable_logs);
 }
 
 }  // namespace net