Add Expect CT policy that gets checked on all certs

net/cert/ct_policy_enforcer.h

Index: net/cert/ct_policy_enforcer.h
diff --git a/net/cert/ct_policy_enforcer.h b/net/cert/ct_policy_enforcer.h
index 8c29da5ecb3a9891ef2be90a55ae873ab61c91d4..6d1178ee31534395e0cc185bdc1f161b9bb9598b 100644
--- a/net/cert/ct_policy_enforcer.h
+++ b/net/cert/ct_policy_enforcer.h
@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include "net/base/net_export.h"
+#include "net/cert/cert_status_flags.h"
 #include "net/log/net_log.h"
 
 namespace net {
@@ -20,23 +21,37 @@ class EVCertsWhitelist;
 
 class X509Certificate;
 
-// Class for checking that a given certificate conforms to security-related
-// policies.
+// Class for checking that a given certificate conforms to
+// Certificate-Transparency-related policies.
+//
+// There are two methods for checking policies:
+// DoesConformToCertPolicy() which applies to certificates in general,
+// and DoesConformToEVPolicy() which applies to EV certificates.

[Ryan Sleevi, 2016/01/23 02:08:10]

OK, so, a concrete suggestion would be declaring whether you call A-then-B or
A-or-B, and what the contract will be.

// Class for checking that a given certificate conforms to
// Certificate Transparency-related policies.
//
// Each method can be called independently, to determine whether
// or not it complies with a given policy.
//
// For example, to determine if a certificate complies with the
// EV certificate policy, callers need only to call
// DoesConformToEVPolicy() - it is not necessary to first check
// whether or not DoesConformToCertPolicy.
//
// However, consider the case where a given certificate is desired
// to be EV, but, if it does not conform to the EV policy, will
// be downgraded to DV. In this case, it's necessary to check if
// it complies with either policy. This can be done one of two
// ways, reflected in pseudo-code below:
//
// Recommended:
// // Checks EV certificates against the EV policy. If the
// // certificate fails, it will be downgraded to DV, in which
// // case, the DV policy will apply.
// bool is_ev = ...;
// bool ct_valid = false;
// is_ev = is_ev && DoesConformToEVPolicy(...);
// ct_valid = is_ev || DoesConformToCertPolicy(...);
//
// NOT recommended:
// // Checks all certificates against the basic policy, and only
// // if they meet the baseline policy, check EV.
// bool conforms_to_cert_policy = DoesConformToCertPolicy(...);
// if (conforms_to_cert_policy && is_ev) {
//   conforms_to_cert_policy = DoesConformToEVPolicy(...);
// }
// 
// The reason the second form is NOT recommended is that the EV
// and Cert policies may be mutually exclusive; for example, the
// EV policy supports whitelisting certificates, while the cert
// policy does not. For this reason, callers are encouraged to
// check the policy specific to the certificate type being
// validated, and only call other methods if they are changing the
// type of certificate because it failed one or more policies.


I *think* that captures the right idea? If I've botched something, let me know!

[estark, 2016/01/24 16:47:46]

I'm not understanding something... It seems to me that that the usage here
(under "Recommended") will not allow us to send reports when an EV cert violates
the SCT policy but is on the whitelist.

That is, I feel like we need something to signal to higher layers that the
expect CT check failed (and I was trying to make that "something" the presence
of CT_STATUS_COMPLIANCE_FAILED, though now I realize we can't change the meaning
of an existing flag). Then the higher layers can use that to decide to send a
report. (And the reason why I think it has to be higher layers rather than
CTPolicyEnforcer that decides to send the report is here:
https://codereview.chromium.org/1579063002/diff/40001/chrome/browser/net/chro...)

[Ryan Sleevi, 2016/01/25 22:30:42]

Yeah, perhaps "valid" is an overloaded term. See below.
Right, I think we want to avoid redefining the flag, unless we also rename the
flag (although if it's in the CertStatus, maybe we can't easily do that? I think
we're dangerously close to flags exhaustion...)

If we added a separate flag, perhaps it's worth adding another example to the
proposed documentation, to indicate that what I was trying to suggest was
something like:

is_valid_expect_ct_policy = DoesConformToCertPolicy(...);
is_valid_ev_policy = is_ev && DoesConformToEVPolicy(...);
is_ev &= is_valid_ev_policy;
is_valid_ct = is_valid_ev_policy || is_valid_expect_ct_policy;

That is, if you're trying to reduce the CT status to a single "yes / no" then
you have to consider the OR combination, but it's perfectly fine to evaluate
things singularly.

What I wanted to document as NOT recommended was:
is_valid_ev_policy = is_ev && is_valid_expect_ct_policy &&
DoesConformToEVPolicy(...);

because that ends up ANDing them, and the restrictions may be incompatible.

Certainly, don't take my suggestion for documentation as the only way to write
it, but I mostly wanted to try to capture the caveats between what these
different methods measure - and to encourage OR for "general CT", discourage
AND, but allow for singular policy testing.

 class NET_EXPORT CTPolicyEnforcer {
  public:
   CTPolicyEnforcer() {}
   virtual ~CTPolicyEnforcer() {}
 
-  // Returns true if the collection of SCTs for the given certificate
-  // conforms with the CT/EV policy. Conformance details are logged to
-  // |net_log|.
+  // Returns true if the collection of SCTs for the given |certificate|
+  // conforms with the CT certificate policy.
   // |cert| is the certificate for which the SCTs apply.
   // |ct_result| must contain the result of verifying any SCTs associated with
   // |cert| prior to invoking this method.
-  virtual bool DoesConformToCTEVPolicy(X509Certificate* cert,
-                                       const ct::EVCertsWhitelist* ev_whitelist,
-                                       const ct::CTVerifyResult& ct_result,
-                                       const BoundNetLog& net_log);
+  virtual bool DoesConformToCertPolicy(X509Certificate* cert,
+                                       const ct::CTVerifyResult& ct_result);
+
+  // Returns true if the collection of SCTs for the given |certificate|
+  // conforms with the EV certificate policy. Conformance details are
+  // logged to |net_log|.
+  // |cert| is the certificate for which the SCTs apply.
+  // |ct_result| must contain the result of verifying any SCTs associated with
+  // |cert| prior to invoking this method.
+  // |ev_whitelist| is a whitelist of EV certificates for which CT policy need
+  // not apply.
+  virtual bool DoesConformToEVPolicy(X509Certificate* cert,
+                                     const ct::CTVerifyResult& ct_result,
+                                     const ct::EVCertsWhitelist* ev_whitelist,
+                                     const BoundNetLog& net_log);
 };
 
 }  // namespace net