Add information to SSLInfo about CT EV policy compliance

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..731926cc87b735b465c069305e11f89f726806a9 100644
--- a/net/cert/ct_policy_enforcer.h
+++ b/net/cert/ct_policy_enforcer.h
@@ -7,13 +7,13 @@
 #include <stddef.h>
 
 #include "net/base/net_export.h"
+#include "net/cert/signed_certificate_timestamp.h"
 #include "net/log/net_log.h"
 
 namespace net {
 
 namespace ct {
 
-struct CTVerifyResult;
 class EVCertsWhitelist;
 
 }  // namespace ct
@@ -24,19 +24,44 @@ class X509Certificate;
 // policies.
 class NET_EXPORT CTPolicyEnforcer {
  public:
+  // Information about the connection's compliance with the EV
+  // certificate policy.
+  enum EVPolicyCompliance {
+    // The certificate was not EV, so the EV policy doesn't apply.
+    EV_POLICY_DOES_NOT_APPLY = 0,
+    // The connection complied with the EV certificate policy by being
+    // included on the EV whitelist.
+    EV_POLICY_COMPLIES_VIA_WHITELIST,
+    // The connection complied with the EV certificate policy by
+    // including SCTs that satisfy the policy.
+    EV_POLICY_COMPLIES_VIA_SCTS,
+    // The connection did not have enough SCTs to retain its EV
+    // status.
+    EV_POLICY_NOT_ENOUGH_SCTS,
+    // The connection did not have diverse enough SCTs to retain its
+    // EV status.
+    EV_POLICY_NOT_DIVERSE_SCTS,
+    // The connection cannot be considered compliant because the build
+    // isn't timely and therefore log information might be out of date
+    // (for example a log might no longer be considered trustworthy).
+    EV_POLICY_BUILD_NOT_TIMELY,
+  };

[Ryan Sleevi, 2016/02/05 02:09:25]

The problem I see with this approach is it breaks the encapsulation originally
intended (although likely incorrectly executed)

That is, this bakes knowledge of all the reasons that EV compliance may fail
into //net, when the choice of a pure virtual interface was to allow embedders
to qualify their own reasons why compliance may have failed.

For example, let's say Opera required that at least one of the SCTs come from an
Opera log. Would they add a new error code? Would they just map it to
NOT_DIVERSE_SCTS, which itself may be one of several reasons today and whose
definition changes over time (originally it was 'X not of same org', now it's 'N
google, N not-google', where N == 1), etc.

I don't have an easy answer for this - but my gut is that we should decide one
way or the other, so long as other embedders are 'happy', and just move on.

To that extent, it'd be good to hear from Opera their thoughts.

It may be that we still leave this as a virtual interface, to facilitate
testing, or we may make this a concrete class and just have Set*ForTesting
helpers. Either approach has its merits, but at least I want to make sure we've
got a good idea as to why this would remain virtual, and what the long-term
vision would be.

*IF* we decided that virtual was to provide an isolation for possible policy
implementations (as it kinda originally was), then I think we'd need to abstract
out the failure reasons into something more opaque, that could then be
"explained" to DevTools (for example, some EVPolicyStatus class which might have
an AsString() function for a human explainer, as a hypothetical). Basically,
poor-man's reflection...

[haavardm, 2016/02/08 12:49:15]

For now, Opera will simply inherit the Google policy. It does not make sense to
have our own policy unless we set up our on CT servers and enforce CAs to embed
SCT from our log as well. We have no plans of that though. Besides, we don't
really believe each browser requiring SCT from own log scales very well.

I'm actually for leaving this patch as is. Currently we use the
CERT_STATUS_CT_COMPLIANCE_FAILED to determine if CT caused the downgrade, and
this patch does not interfere with that (although this change makes the security
bit somewhat unnecessary). I'm a bit skeptical letting //net return these errors
as strings. For now we re-use your implementation, but we might show the strings
elsewhere than in dev-tools, requiring different wording.  Using Enums let us
continue using your logic, while we can make our own version of any strings
shown to users. Also, instead of explaining the exact reason behind
NOT_DIVERSE_SCTS, perhaps it's better to provide a link explaining the current
policy? 
 

+
   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 an enum indicating if the collection of SCTs for the given
+  // certificate conforms with the CT/EV 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.
-  virtual bool DoesConformToCTEVPolicy(X509Certificate* cert,
-                                       const ct::EVCertsWhitelist* ev_whitelist,
-                                       const ct::CTVerifyResult& ct_result,
-                                       const BoundNetLog& net_log);
+  // |verified_scts| contains any SCTs associated with |cert| that were
+  // verified prior to invoking this method and found to be valid.
+  virtual EVPolicyCompliance DoesConformToCTEVPolicy(
+      X509Certificate* cert,
+      const ct::EVCertsWhitelist* ev_whitelist,
+      const std::vector<scoped_refptr<ct::SignedCertificateTimestamp>>&
+          verified_scts,

[estark, 2016/01/30 17:03:29]

I changed this method to take a list of SCTs instead of a CTVerifyResult,
because I thought it was a bit weird for this method to take a partially-filled
CTVerifyResult as input and output a result that was used to further fill in the
CTVerifyResult. Interested in hearing other opinions though.

[Ryan Sleevi, 2016/02/05 02:09:25]

We should probably simplify this with a using

[estark, 2016/02/08 08:36:26]

Did you mean `using SCTList = std::vector...`? If so, done.

+      const BoundNetLog& net_log);
 };
 
 }  // namespace net