Add Expect CT policy that gets checked on all certs

net/cert/ct_policy_enforcer.cc

Index: net/cert/ct_policy_enforcer.cc
diff --git a/net/cert/ct_policy_enforcer.cc b/net/cert/ct_policy_enforcer.cc
index d9c92421bf86e0ba4d7d101e07268769ad1dd137..d724b81151238b96aa576d255395ad4eccdd2d22 100644
--- a/net/cert/ct_policy_enforcer.cc
+++ b/net/cert/ct_policy_enforcer.cc
@@ -134,29 +134,25 @@ bool HasEnoughDiverseSCTs(const ct::SCTList& verified_scts) {
          (verified_scts.size() != num_google_issued_scts);
 }
 
-enum CTComplianceStatus {
-  CT_NOT_COMPLIANT = 0,
-  CT_IN_WHITELIST = 1,
-  CT_ENOUGH_SCTS = 2,
-  CT_NOT_ENOUGH_DIVERSE_SCTS = 3,
-  CT_COMPLIANCE_MAX,
+enum EVPolicyStatus {
+  EV_POLICY_STATUS_NOT_COMPLIANT = 0,
+  EV_POLICY_STATUS_IN_WHITELIST = 1,
+  EV_POLICY_STATUS_COMPLIANT = 2,
+  EV_POLICY_STATUS_MAX,
 };
 
-const char* ComplianceStatusToString(CTComplianceStatus status) {
+const char* EVPolicyStatusToString(EVPolicyStatus status) {
   switch (status) {
-    case CT_NOT_COMPLIANT:
+    case EV_POLICY_STATUS_NOT_COMPLIANT:
       return "NOT_COMPLIANT";
       break;
-    case CT_IN_WHITELIST:
+    case EV_POLICY_STATUS_IN_WHITELIST:
       return "WHITELISTED";
       break;
-    case CT_ENOUGH_SCTS:
-      return "ENOUGH_SCTS";
+    case EV_POLICY_STATUS_COMPLIANT:
+      return "COMPLIANT";
       break;
-    case CT_NOT_ENOUGH_DIVERSE_SCTS:
-      return "NOT_ENOUGH_DIVERSE_SCTS";
-      break;
-    case CT_COMPLIANCE_MAX:
+    case EV_POLICY_STATUS_MAX:
       break;
   }
 
@@ -170,11 +166,11 @@ enum EVWhitelistStatus {
   EV_WHITELIST_MAX,
 };
 
-void LogCTComplianceStatusToUMA(CTComplianceStatus status,
-                                const ct::EVCertsWhitelist* ev_whitelist) {
-  UMA_HISTOGRAM_ENUMERATION("Net.SSL_EVCertificateCTCompliance", status,
-                            CT_COMPLIANCE_MAX);
-  if (status == CT_NOT_COMPLIANT) {
+void LogEVPolicyStatusToUMA(EVPolicyStatus status,
+                            const ct::EVCertsWhitelist* ev_whitelist) {
+  UMA_HISTOGRAM_ENUMERATION("Net.SSL_EVCTPolicyStatus", status,
+                            EV_POLICY_STATUS_MAX);
+  if (status == EV_POLICY_STATUS_NOT_COMPLIANT) {
     EVWhitelistStatus ev_whitelist_status = EV_WHITELIST_NOT_PRESENT;
     if (ev_whitelist != NULL) {
       if (ev_whitelist->IsValid())
@@ -188,41 +184,32 @@ void LogCTComplianceStatusToUMA(CTComplianceStatus status,
   }
 }
 
-struct ComplianceDetails {
-  ComplianceDetails()
-      : ct_presence_required(false),
-        build_timely(false),
-        status(CT_NOT_COMPLIANT) {}
+struct EVComplianceDetails {
+  EVComplianceDetails()
+      : build_timely(false), status(EV_POLICY_STATUS_NOT_COMPLIANT) {}
 
-  // Whether enforcement of the policy was required or not.
-  bool ct_presence_required;
-  // Whether the build is not older than 10 weeks. The value is meaningful only
-  // if |ct_presence_required| is true.
+  // True if the build is not older than 10 weeks.
   bool build_timely;
-  // Compliance status - meaningful only if |ct_presence_required| and
-  // |build_timely| are true.
-  CTComplianceStatus status;
-  // EV whitelist version.
+  // Compliance status. Cannot be EV_POLICY_STATUS_IS_WHITELISTED if
+  // |build_timely| is false.
+  EVPolicyStatus status;
+  // EV whitelist version. Only set if |build_timely| is true.
   base::Version whitelist_version;
 };
 
 scoped_ptr<base::Value> NetLogComplianceCheckResultCallback(
     X509Certificate* cert,
-    ComplianceDetails* details,
+    EVComplianceDetails* details,
     NetLogCaptureMode capture_mode) {
   scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->Set("certificate", NetLogX509CertificateCallback(cert, capture_mode));
-  dict->SetBoolean("policy_enforcement_required",
-                   details->ct_presence_required);
-  if (details->ct_presence_required) {
-    dict->SetBoolean("build_timely", details->build_timely);
-    if (details->build_timely) {
-      dict->SetString("ct_compliance_status",
-                      ComplianceStatusToString(details->status));
-      if (details->whitelist_version.IsValid())
-        dict->SetString("ev_whitelist_version",
-                        details->whitelist_version.GetString());
-    }
+  dict->SetBoolean("policy_enforcement_required", true);
+  dict->SetBoolean("build_timely", details->build_timely);
+  dict->SetString("ct_compliance_status",
+                  EVPolicyStatusToString(details->status));
+  if (details->whitelist_version.IsValid()) {
+    dict->SetString("ev_whitelist_version",
+                    details->whitelist_version.GetString());
   }
   return std::move(dict);
 }
@@ -261,49 +248,55 @@ bool IsCertificateInWhitelist(const X509Certificate& cert,
   return cert_in_ev_whitelist;
 }
 
-void CheckCTEVPolicyCompliance(X509Certificate* cert,
-                               const ct::EVCertsWhitelist* ev_whitelist,
-                               const ct::CTVerifyResult& ct_result,
-                               ComplianceDetails* result) {
-  result->ct_presence_required = true;
+bool CheckCertPolicyCompliance(X509Certificate* cert,

[estark, 2016/01/18 17:04:43]

I can add UMA metrics and/or netlog events in this function if we think they
would be useful.

+                               const ct::CTVerifyResult& ct_result) {
+  if (!HasRequiredNumberOfSCTs(*cert, ct_result))
+    return false;
+
+  if (AllSCTsPastDistinctSCTRequirementEnforcementDate(
+          ct_result.verified_scts) &&
+      !HasEnoughDiverseSCTs(ct_result.verified_scts)) {
+    return false;
+  }
+
+  return true;
+}
 
+void CheckEVPolicyCompliance(X509Certificate* cert,
+                             CertStatus cert_status,
+                             const ct::EVCertsWhitelist* ev_whitelist,
+                             EVComplianceDetails* result) {
+  if ((cert_status & CERT_STATUS_CT_COMPLIANCE_FAILED) == 0)
+    result->status = EV_POLICY_STATUS_COMPLIANT;

[Eran Messeri, 2016/01/21 13:15:38]

Nit: Set the result->status to something else so this function does not depend
on how |result| was initialized, particularly (line 272) if the build is not
timely.

[estark, 2016/01/21 22:46:14]

Done.

   if (!IsBuildTimely())
     return;
   result->build_timely = true;
-
   if (ev_whitelist && ev_whitelist->IsValid())
     result->whitelist_version = ev_whitelist->Version();
 
-  if (IsCertificateInWhitelist(*cert, ev_whitelist)) {
-    result->status = CT_IN_WHITELIST;
-    return;
-  }
-
-  if (!HasRequiredNumberOfSCTs(*cert, ct_result)) {
-    result->status = CT_NOT_COMPLIANT;
-    return;
-  }
-
-  if (AllSCTsPastDistinctSCTRequirementEnforcementDate(
-          ct_result.verified_scts) &&
-      !HasEnoughDiverseSCTs(ct_result.verified_scts)) {
-    result->status = CT_NOT_ENOUGH_DIVERSE_SCTS;
-    return;
+  if (result->status != EV_POLICY_STATUS_COMPLIANT &&
+      IsCertificateInWhitelist(*cert, ev_whitelist)) {
+    result->status = EV_POLICY_STATUS_IN_WHITELIST;
   }
-
-  result->status = CT_ENOUGH_SCTS;
 }
 
 }  // namespace
 
-bool CTPolicyEnforcer::DoesConformToCTEVPolicy(
+bool CTPolicyEnforcer::DoesConformToCertPolicy(
     X509Certificate* cert,
+    const ct::CTVerifyResult& ct_result) {
+  return CheckCertPolicyCompliance(cert, ct_result);
+}
+
+bool CTPolicyEnforcer::DoesConformToEVPolicy(
+    X509Certificate* cert,
+    CertStatus cert_status,
     const ct::EVCertsWhitelist* ev_whitelist,
-    const ct::CTVerifyResult& ct_result,
     const BoundNetLog& net_log) {
-  ComplianceDetails details;
+  DCHECK_NE((cert_status & CERT_STATUS_IS_EV), 0u);
 
-  CheckCTEVPolicyCompliance(cert, ev_whitelist, ct_result, &details);
+  EVComplianceDetails details;
+  CheckEVPolicyCompliance(cert, cert_status, ev_whitelist, &details);
 
   NetLog::ParametersCallback net_log_callback =
       base::Bind(&NetLogComplianceCheckResultCallback, base::Unretained(cert),
@@ -312,18 +305,12 @@ bool CTPolicyEnforcer::DoesConformToCTEVPolicy(
   net_log.AddEvent(NetLog::TYPE_EV_CERT_CT_COMPLIANCE_CHECKED,
                    net_log_callback);
 
-  if (!details.ct_presence_required)
-    return true;
-
-  if (!details.build_timely)
-    return false;
-
-  LogCTComplianceStatusToUMA(details.status, ev_whitelist);
+  LogEVPolicyStatusToUMA(details.status, ev_whitelist);
 
-  if (details.status == CT_IN_WHITELIST || details.status == CT_ENOUGH_SCTS)
-    return true;
+  if (details.status == EV_POLICY_STATUS_IN_WHITELIST)
+    return details.build_timely;

[Eran Messeri, 2016/01/21 13:15:38]

Seems like there's a logical change hiding in here - this method returned false
if build_timely was false, regardless of the status.

Shouldn't it be:
if (!details.build_timely)
  return false;

return (details.status == EV_POLICY_STATUS_COMPLIANT || details.status ==
EV_POLICY_STATUS_IN_WHITELIST)

[estark, 2016/01/21 22:46:14]

Hmm, I think I might have actually done this intentionally, though I forgot to
highlight it and ask about it here -- sorry. Why is it that we should always
strip EV status if the build isn't timely? And a closely related question...
what's the purpose of the timeliness check? Is it to ensure that the whitelist
is up-to-date? Or is it so that Chrome does not continue to enforce an SCT
policy that might be obsolete? (If the latter, I would sort of expect this to
always return true if |build_timely| is false... i.e. enforce no policy when the
build is out-of-date rather than stripping EV off every certificate). Am I
totally confused?

 
-  return false;
+  return (details.status == EV_POLICY_STATUS_COMPLIANT);
 }
 
 }  // namespace net