Add Expect CT policy that gets checked on all certs

Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806
Committed: https://crrev.com/0fc8d0784ff725ffcab646769ec1f1765c2d4013
Cr-Commit-Position: refs/heads/master@{#377662}

[estark, 2016-01-12 21:00:41]

estark@chromium.org changed reviewers:
+ rsleevi@chromium.org

[estark, 2016-01-12 21:00:42]

And this is the second CL pulled out from
https://codereview.chromium.org/1579063002/ ("Implement a skeleton version of
Expect CT reports").

This one checks CT policy for all certs, not just EV, and renames the
policy-checking method accordingly and updates some comments.

[Ryan Sleevi, 2016-01-12 21:24:56]

rsleevi@chromium.org changed reviewers:
+ eranm@chromium.org

[Ryan Sleevi, 2016-01-12 21:24:56]

LGTM with reservations.

So, as we implement policies for CNNIC & Symantec, I suspect we're going to need
to change things up here, so that we're distinguishing which is an "EV penalty"
and which is a "certificate penalty". For example, for Symantec, we'll need two
additional whitelists (a whitelist of non-Symantec-but-chaining-to-Symantec
intermediates and, potentially, a whitelist of Symantec certificates - TBD).

So I'm wondering if we shouldn't figure out whether we should keep a dedicated
method for EV policy (which takes an EV whitelist), a dedicated method for
'expect-CT' (or more generally, "DV"), and a dedicated method for things like
Symantec/policy-imposed CT policies.

On a more design-level discussion for expect-CT, I wouldn't think we'd want the
EV whitelist to be 'accepted' for those expect-CT cases, since it'd be violating
the "send SCTs" thing if we were relying on the whitelist.

(Sorry for the change in feedback, this is mostly due to sitting down with Eran
and hashing things out a bit more)

https://codereview.chromium.org/1578993003/diff/20001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/20001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:264: void
CheckCTPolicyCompliance(X509Certificate* cert,
git cl format

[estark, 2016-01-12 23:16:29]

On 2016/01/12 21:24:56, Ryan Sleevi wrote:
> LGTM with reservations.
> 
> So, as we implement policies for CNNIC & Symantec, I suspect we're going to
need
> to change things up here, so that we're distinguishing which is an "EV
penalty"
> and which is a "certificate penalty". For example, for Symantec, we'll need
two
> additional whitelists (a whitelist of non-Symantec-but-chaining-to-Symantec
> intermediates and, potentially, a whitelist of Symantec certificates - TBD).
> 
> So I'm wondering if we shouldn't figure out whether we should keep a dedicated
> method for EV policy (which takes an EV whitelist), a dedicated method for
> 'expect-CT' (or more generally, "DV"), and a dedicated method for things like
> Symantec/policy-imposed CT policies.
> 
> On a more design-level discussion for expect-CT, I wouldn't think we'd want
the
> EV whitelist to be 'accepted' for those expect-CT cases, since it'd be
violating
> the "send SCTs" thing if we were relying on the whitelist.

Hmm, I hadn't thought about that. I suppose it does seem reasonable that we'd
want to send reports for sites on the EV whitelist. Eran, do you agree?

If so, then I agree with Ryan that we should have a couple separate methods on
CTPolicyEnforcer: maybe one that computes whether there are enough (and diverse
enough) SCTs, and one that checks the EV whitelist and is only ever called for
EV certs.

> 
> (Sorry for the change in feedback, this is mostly due to sitting down with
Eran
> and hashing things out a bit more)
> 
>
https://codereview.chromium.org/1578993003/diff/20001/net/cert/ct_policy_enfo...
> File net/cert/ct_policy_enforcer.cc (right):
> 
>
https://codereview.chromium.org/1578993003/diff/20001/net/cert/ct_policy_enfo...
> net/cert/ct_policy_enforcer.cc:264: void
> CheckCTPolicyCompliance(X509Certificate* cert,
> git cl format

[Eran Messeri, 2016-01-14 11:29:05]

> Hmm, I hadn't thought about that. I suppose it does seem reasonable that we'd
> want to send reports for sites on the EV whitelist. Eran, do you agree?
> 
> If so, then I agree with Ryan that we should have a couple separate methods on
> CTPolicyEnforcer: maybe one that computes whether there are enough (and
diverse
> enough) SCTs, and one that checks the EV whitelist and is only ever called for
> EV certs.
> 

Yes, that makes sense - then later, when we have the whitelists for Symantec /
CNNIC we can can add methods that will check in those whitelists.

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

Sorry for the delay! Here's another version that splits out the EV check into
its own method.

This CTPolicyEnforcer interface does impact what metrics we can record, though,
because we lose information about why exactly the certificate doesn't comply
with the SCT policy when we are deciding whether to strip its EV status. So we
now record that a certificate lost its EV status because its SCTs didn't comply
with the policy, but we can't record that its SCTs were not diverse enough (for
example) because that is checked in a separate method. I'm not sure who exactly
uses these UMA metrics and for what, so I'm not sure if this change is
acceptable or not. If we want to keep recording the same metrics, another option
might be to return an enum (complies/not diverse enough SCTs/not enough SCTs)
from DoesConformToCertPolicy() and pass that enum in to DoesConformToEVPolicy(),
so that DoesConformToEVPolicy() knows exactly why the cert doesn't comply with
the SCT policy.

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:251: bool
CheckCertPolicyCompliance(X509Certificate* cert,
I can add UMA metrics and/or netlog events in this function if we think they
would be useful.

[estark, 2016-01-20 16:43:38]

ping :)

[Eran Messeri, 2016-01-21 11:55:02]

I'm fine with dropping the fine-grained information about the reason a
certificate did not comply with the policy. AFAIK only I was using these
numbers, no decision was made based on them.
So LGTM.

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

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:270: result->status = EV_POLICY_STATUS_COMPLIANT;
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.

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:311: return details.build_timely;
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:45:19]

Patchset #5 (id:80001) has been deleted

[estark, 2016-01-21 22:46:15]

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:270: result->status = EV_POLICY_STATUS_COMPLIANT;
On 2016/01/21 13:15:38, Eran Messeri wrote:
> 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.

Done.

https://codereview.chromium.org/1578993003/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:311: return details.build_timely;
On 2016/01/21 13:15:38, Eran Messeri wrote:
> 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)

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?

[Eran Messeri, 2016-01-22 11:27:09]

> 
> 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?

+Ryan for a proper explanation on why EV status is stripped if the build isn't
timely. 
IIRC stripping the EV status if the build isn't timely is intentional, as the
assumption was something along the lines of "if you have a very old Chrome
installation we're going to take away 'strong' security indicators because you
may be exposed to other security vulnerabilities".

[Ryan Sleevi, 2016-01-22 23:49:41]

Explained the Timeliness check below.

Let's hold off on landing until we can sort the design issue.

https://codereview.chromium.org/1578993003/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2510: // been marked as failing CT on
the interstitial.
That's... weird.

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (left):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:271: return;
So the explanation for the timeliness check is not related to the whitelist, but
with respect to recognized logs.

The goal being is that if a client is so old that it doesn't have fresh log
information (e.g. in the event we've pulled a log for getting compromised and
misissuing SCTs), that clients don't trust it.

We could still use the whitelist, at this point, but instead we just disable CT
entirely.

I believe this check SHOULD be included in CheckCertPolicyCompliance, NOT
CheckEVPolicyCompliance, at least based on that logic.

If we want to change that policy, or adjust, I think that's a
reasonable-but-separate conversation we should have, but for now, we should keep
the present behaviour for all CT enforcement.

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:272: if (!IsBuildTimely())
See remarks above about why this is the wrong place.

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:49: const BoundNetLog& net_log);
DESIGN: [Note, this comment just comes from reading the header alone]

So from an API direction, I'm a little confused why the ct::CTVerifyResult isn't
supplied/modified in this call, or why the CertStatus is necessary (just from
reading this header)

I'm thinking it may be useful to perhaps add class-level documentation of
example usage, which may help sanity-check the design.

https://codereview.chromium.org/1578993003/diff/100001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:3141: net_log_)) {
The interface between these two methods feels wrong. PolicyEnforcer (previously)
was ignorant of the CertStatus behaviour and machinations - PolicyEnforcer's
responsibilities were solely about returning results about the CT status.

The new API introduces a dependency on the CertStatus that we likely don't want
to do.

One possible solution is to move this to an else-if on 3136-3137, another would
be to make the EVPolicy separately re-evaluate the certificate using the
ct_verify_result_ (which may be more robust anyways), although that would mean
double-validation in the case of EV.

[estark, 2016-01-23 01:38:41]

Thanks Ryan.

https://codereview.chromium.org/1578993003/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2510: // been marked as failing CT on
the interstitial.
On 2016/01/22 23:49:41, Ryan Sleevi wrote:
> That's... weird.

Yeah :/ Not sure how else to deal with it though... I guess maybe this test
shouldn't necessarily be requiring that interstitial_ssl_status.cert_status ==
after_interstitial_ssl_status.cert_status, but rather just checking that they
both contain the relevant cert error.

Also apparently I didn't understand subject verb agreement when I wrote
CheckSSLStatusesEquals(). *twitch*

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (left):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:271: return;
On 2016/01/22 23:49:41, Ryan Sleevi wrote:
> So the explanation for the timeliness check is not related to the whitelist,
but
> with respect to recognized logs.
> 
> The goal being is that if a client is so old that it doesn't have fresh log
> information (e.g. in the event we've pulled a log for getting compromised and
> misissuing SCTs), that clients don't trust it.
> 
> We could still use the whitelist, at this point, but instead we just disable
CT
> entirely.
> 
> I believe this check SHOULD be included in CheckCertPolicyCompliance, NOT
> CheckEVPolicyCompliance, at least based on that logic.
> 
> If we want to change that policy, or adjust, I think that's a
> reasonable-but-separate conversation we should have, but for now, we should
keep
> the present behaviour for all CT enforcement.

Ah, I see. Though, to keep the logic exactly the same as it is now (untimely
build => not CT compliant, no whitelist check), I think we actually need the
build-timelinesss check in both CheckCertPolicyCompliance() and in
CheckEVPolicyCompliance(). Let me know if that looks correct to you. (If we only
have the build-timely check in CheckCertPolicyCompliance(), then by the time we
get to CheckEVPolicyCompliance(), the cert looks non-compliant and we go to the
whitelist same as if the server hadn't provided enough SCTs.)

I do have some questions/thoughts about how this build-timeliness logic should
work with expect CT, but we can discuss on a future CL.

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:272: if (!IsBuildTimely())
On 2016/01/22 23:49:41, Ryan Sleevi wrote:
> See remarks above about why this is the wrong place.

Acknowledged.

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/100001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:49: const BoundNetLog& net_log);
On 2016/01/22 23:49:41, Ryan Sleevi wrote:
> DESIGN: [Note, this comment just comes from reading the header alone]
> 
> So from an API direction, I'm a little confused why the ct::CTVerifyResult
isn't
> supplied/modified in this call, or why the CertStatus is necessary (just from
> reading this header)
> 
> I'm thinking it may be useful to perhaps add class-level documentation of
> example usage, which may help sanity-check the design.

I changed the interface a bit as you suggested below, and added a bit more
documentation at the top of the class, though hopefully the new API makes it
clearer?

I can still add example usage if you think it'd be helpful... I'm just not sure
if it adds much with the modified API.

https://codereview.chromium.org/1578993003/diff/100001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1578993003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:3141: net_log_)) {
On 2016/01/22 23:49:41, Ryan Sleevi wrote:
> The interface between these two methods feels wrong. PolicyEnforcer
(previously)
> was ignorant of the CertStatus behaviour and machinations - PolicyEnforcer's
> responsibilities were solely about returning results about the CT status.
> 
> The new API introduces a dependency on the CertStatus that we likely don't
want
> to do.
> 
> One possible solution is to move this to an else-if on 3136-3137, another
would
> be to make the EVPolicy separately re-evaluate the certificate using the
> ct_verify_result_ (which may be more robust anyways), although that would mean
> double-validation in the case of EV.

If we're okay with double-validating for EV, that sounds good to me (and maybe
even potentially useful in the future, if we ever, say, wanted to apply a more
strict SCT check to EV certs?).

[Ryan Sleevi, 2016-01-23 02:08:11]

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:286: if (!IsBuildTimely())
Perhaps it's worth documenting why we check build timeliness (here and 297)

Also, why wouldn't we want to NetLog these checks as well? Will that come
seperately? Not at all? Seems very useful for Expect-CT debugging, and
consistent with being introduced in this CL.

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:29: // and DoesConformToEVPolicy() which applies
to EV certificates.
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!

https://codereview.chromium.org/1578993003/diff/120001/net/quic/crypto/proof_...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/1578993003/diff/120001/net/quic/crypto/proof_...
net/quic/crypto/proof_verifier_chromium.cc:292:
CERT_STATUS_CT_COMPLIANCE_FAILED;
BUG: This changes the meaning of this status, in that it no longer respects the
whitelist (like the code previously did) for EV certificates.

I think the proposed class-level comments to the policy enforcer may explain
what I *think* is the right flow to check for this. Let me know what you think!

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

(no code changes, just discussion inline)

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:29: // and DoesConformToEVPolicy() which applies
to EV certificates.
On 2016/01/23 02:08:10, Ryan Sleevi wrote:
> 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!

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]

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:29: // and DoesConformToEVPolicy() which applies
to EV certificates.
On 2016/01/24 16:47:46, estark wrote:
> 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.

Yeah, perhaps "valid" is an overloaded term. See below.

> 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). 

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.

[estark, 2016-01-26 06:46:46]

On 2016/01/25 22:30:42, Ryan Sleevi wrote:
>
https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
> File net/cert/ct_policy_enforcer.h (right):
> 
>
https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
> net/cert/ct_policy_enforcer.h:29: // and DoesConformToEVPolicy() which applies
> to EV certificates.
> On 2016/01/24 16:47:46, estark wrote:
> > 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.
> 
> Yeah, perhaps "valid" is an overloaded term. See below.
> 
> > 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). 
> 
> 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.

Ah, ok, I think I'm now understanding what you're getting at with the
documentation and that makes sense to me, but now I'm starting to get all
confused about what this CL should be now that I'm trying to not change the
meaning of CT_COMPLIANCE_FAILED. When setting up a connection, I want to do two
things:
a. Evaluate DoesConformToEVPolicy() for EV certs only. If it returns false,
strip EV status and set CT_COMPLIANCE_FAILED (preserving the exact same meaning
for this flag).
b. Evaluate DoesConformToCertPolicy() for all certs and record that in some way
for higher layers to read to decide to send a report.

First, to make sure we're on the same page, do those two things sound generally
right to you?

Second, can we rename CT_COMPLIANCE_FAILED without consuming additional
CertStatus bits? i.e. could I rename it to EV_CT_COMPLIANCE_FAILED or something
like that? Or does renaming consume additional bits for some reason? The reason
I ask is because it's not really clear to me what exactly "CT compliance failed"
means as we introduce additional policies, and it seems like maybe the flag
should correspond to one particular CT policy (EV) rather than CT compliance in
general.

Third, for how to record the result of DoesConformToCertPolicy() so that higher
layers can read it... the two possibilities I can think of are a new CertStatus
flag, or adding a field to SSLInfo. The latter could maybe even be a bitfield to
signal the results of the different policy checks -- whether EV status was
stripped, whether expect CT failed, maybe later whether the Symantec policy
check failed -- because it would be nice for UI to be able to say "This
certificate's EV status was stripped because of CT", though maybe that's more
information than you want to expose out of net? What do you think of those
approaches, or do you have any others in mind?

[Eran Messeri, 2016-01-26 11:02:24]

> check failed -- because it would be nice for UI to be able to say "This
> certificate's EV status was stripped because of CT", though maybe that's more
> information than you want to expose out of net? What do you think of those
> approaches, or do you have any others in mind?

I support exporting information on whether CT checks were applied and if so
which succeeded - it is going to make domain owners' lives much easier if we can
export this information through the UI.
As for how: Instead of continuing the abuse of CertStatus:
- Extend CTVerifyResult to have a status word indicating whether SCTs were
checked at all, whether the EV policy holds, whether it holds by being
whitelisted, Symantec's policy, etc.
- Copy that status word to a new field in the SSLInfo in
SSLInfo::UpdateSignedCertificateTimestamps
That way there would be no need to duplicate the code between SSLClientSocket*,
QUIC proof verifier).

Suggestion: the new status word can have bitfields set to indicate the
following: 
- Whether CT checks were performed at all (if the CTVerifier instance is null,
for example, they will not be).
- Whether the diverse SCT requirement was met
- Whether the certificate is in the EV whitelist (only relevant if the diverse
SCT requirement was not met).
- Whether the Symantec policy was applied.

[Ryan Sleevi, 2016-01-26 22:26:14]

On 2016/01/26 06:46:46, estark wrote:
> First, to make sure we're on the same page, do those two things sound
generally
> right to you?

Absolutely :)

> Second, can we rename CT_COMPLIANCE_FAILED without consuming additional
> CertStatus bits? i.e. could I rename it to EV_CT_COMPLIANCE_FAILED or
something
> like that? Or does renaming consume additional bits for some reason? The
reason
> I ask is because it's not really clear to me what exactly "CT compliance
failed"
> means as we introduce additional policies, and it seems like maybe the flag
> should correspond to one particular CT policy (EV) rather than CT compliance
in
> general.

Yes, that's fine as well. What's persisted is the bits.

One caveat to be aware of is that NetLog stores the stringified names as a
pretty-lookup-table. That is, the NetLog entries themselves will be "1" or "2",
but then the NetLog file will contain a mapping of { "1": "SOME_STRING", "2":
"ANOTHER_STRING" }. So if we rename SOME_STRING to NEW_STRING, then 'old'
NetLogs will say SOME_STRING, and 'new' NetLogs will say NEW_STRING. So we try
not to 'reuse' SOME_STRING, even though it's perfectly fine, because of that
ambiguity. But it's also not a deal breaker, since 'old' NetLogs will disappear
in 18 weeks (the time it takes for TOT to roll to Stable)

> Third, for how to record the result of DoesConformToCertPolicy() so that
higher
> layers can read it... the two possibilities I can think of are a new
CertStatus
> flag, or adding a field to SSLInfo. The latter could maybe even be a bitfield
to
> signal the results of the different policy checks -- whether EV status was
> stripped, whether expect CT failed, maybe later whether the Symantec policy
> check failed -- because it would be nice for UI to be able to say "This
> certificate's EV status was stripped because of CT", though maybe that's more
> information than you want to expose out of net? What do you think of those
> approaches, or do you have any others in mind?

My concern for any information we expose outside of //net is that we have to
decide whether it's part of the transient or persisted data set. I have no
objections to adding new transient-only information, just that it's clear we
document as such.

I'm not sure I fully grok Eran's approach, so I would suggest you run your
understanding of it through that lens, since I suspect you know what I'm going
on about, and know about the UI inconsistencies that can arise for transient vs
intransient information.

[estark, 2016-01-30 17:01:56]

Ok, thanks for all the discussion so far, Ryan and Eran. I've uploaded
https://codereview.chromium.org/1652603002/, which shows a possible approach for
exporting information about CT policy compliance out of net (similar to what
eranm@ was suggesting, I think). I'd suggest looking at that one first. That CL
is about providing a place to expose CT policy info and using it to expose EV
info (which I expect Lucas will want to use soon in devtools). I put some notes
in that CL about how I think we can cope with the fact that this information is
transient.

Then I rebased this CL on top of 1652603002, so that this CL introduces the
expect-CT policy and records the result using the mechanism introduced in
1652603002.

One note: while working on this I noticed that one of the CTPolicyEnforcer tests
has (I think) a bug which isn't totally trivial to fix, so I temporarily
disabled the test and filed a bug for it. I plan to try to fix the test in a
separate CL, and expect that the fix might land before this CL in which case
I'll un-disable the test before landing this one -- just wanted to have it
disabled here so I can run trybots, etc.

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/120001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:286: if (!IsBuildTimely())
On 2016/01/23 02:08:10, Ryan Sleevi wrote:
> Perhaps it's worth documenting why we check build timeliness (here and 297)

Done.

> 
> Also, why wouldn't we want to NetLog these checks as well? Will that come
> seperately? Not at all? Seems very useful for Expect-CT debugging, and
> consistent with being introduced in this CL.

Done.

[estark, 2016-02-18 23:19:57]

Ryan, Eran -- I've rebased this patch now that
https://codereview.chromium.org/1652603002/ (adding CT policy results to
SSLInfo) has landed. Could you take another look, please?

[estark, 2016-02-18 23:22:23]

Description was changed from

==========
Check CT policy on all certs, not just EV

In preparation for Expect-CT, this CL checks for CT policy compliance on
all certs, not just EV certs. It renames the method to
DoesConformToCTPolicy() accordingly (from DoesConformToCTEVPolicy()).

This is based on https://codereview.chromium.org/1579233002/

BUG=568806
==========

to

==========
Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806
==========

[estark, 2016-02-22 21:50:49]

friendly ping!

[Ryan Sleevi, 2016-02-22 23:31:22]

My only uncertainty is with respect to calling it cert compliance, since we have
not yet documented what the requirements will be for DV (although they should
generally remain the same). That is, expect_ct vs DV policy is still...
unclear... to me.

This LGTM, since all I have are nits, although at some point it looks like we
need to refactor the boiler place that we have littered in three places (for
dealing with CT compliance).

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:332:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS;
This value never is used / assignment makes no sense.

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:333: if (build_timely)
Usually we check the 'error' case first, for early return ease. While I realize
we can't short-circuit here, it makes sense when taken in context of the
comment:

if (!build_timely)
  compliance = ... BUILD_NOT_TIMELY;
else
  compliance = CheckCertPolicyCompliance(...)

ditto 359-364 (which you use inconsistent braces for)

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:88: verified_scts,
Why isn't this "const SCTList& verified_scts" ?

[estark, 2016-02-23 06:31:32]

estark@chromium.org changed reviewers:
+ jwd@chromium.org

[estark, 2016-02-23 06:31:33]

jwd: could you please review histograms.xml?

Thanks Ryan. Agree about refactoring the very repetitive call sites at some
point. I'll discuss the expect-CT vs DV policy issue with CT team tomorrow at
our weekly sync.

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:332:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS;
On 2016/02/22 23:31:22, Ryan Sleevi wrote:
> This value never is used / assignment makes no sense.

Done.

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:333: if (build_timely)
On 2016/02/22 23:31:22, Ryan Sleevi wrote:
> Usually we check the 'error' case first, for early return ease. While I
realize
> we can't short-circuit here, it makes sense when taken in context of the
> comment:
> 
> if (!build_timely)
>   compliance = ... BUILD_NOT_TIMELY;
> else
>   compliance = CheckCertPolicyCompliance(...)
> 
> ditto 359-364 (which you use inconsistent braces for)

Done.

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.h (right):

https://codereview.chromium.org/1578993003/diff/240001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.h:88: verified_scts,
On 2016/02/22 23:31:22, Ryan Sleevi wrote:
> Why isn't this "const SCTList& verified_scts" ?

Done.

[estark, 2016-02-23 06:31:33]

estark@chromium.org changed reviewers:
+ jwd@chromium.org

[estark, 2016-02-23 06:31:34]

jwd: could you please review histograms.xml?

Thanks Ryan. Agree about refactoring the very repetitive call sites at some
point. I'll discuss the expect-CT vs DV policy issue with CT team tomorrow at
our weekly sync.

[Eran Messeri, 2016-02-24 14:35:05]

Still LGTM, a few small questions.

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:144: return "NOT_DIVERSE_SCTS";
nit: SCTS_NOT_DIVERSE?

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_verify_res...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_verify_res...
net/cert/ct_verify_result.cc:16:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS),
Why did you choose this default value?

[estark, 2016-02-24 22:03:59]

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_policy_enf...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_policy_enf...
net/cert/ct_policy_enforcer.cc:144: return "NOT_DIVERSE_SCTS";
On 2016/02/24 14:35:05, Eran Messeri wrote:
> nit: SCTS_NOT_DIVERSE?

Done.

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_verify_res...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/1578993003/diff/260001/net/cert/ct_verify_res...
net/cert/ct_verify_result.cc:16:
ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS),
On 2016/02/24 14:35:05, Eran Messeri wrote:
> Why did you choose this default value?

No reason in particular, it's just the first one in the enum. I could add an
UNKNOWN value but, I dunno if it makes sense to add an UNKNOWN value just for
use as a default initializer?

[jwd, 2016-02-25 17:44:51]

lgtm

[estark, 2016-02-25 18:04:41]

The CQ bit was checked by estark@chromium.org

[estark, 2016-02-25 18:04:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, eranm@chromium.org
Link to the patchset: https://codereview.chromium.org/1578993003/#ps280001
(title: "eranm nit")

[commit-bot: I haz the power, 2016-02-25 18:07:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578993003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578993003/280001

[commit-bot: I haz the power, 2016-02-25 18:14:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-25 18:14:51]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[estark, 2016-02-25 18:49:02]

The CQ bit was checked by estark@chromium.org

[estark, 2016-02-25 18:49:03]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
rsleevi@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/1578993003/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-25 18:50:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578993003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578993003/300001

[commit-bot: I haz the power, 2016-02-25 20:41:28]

Description was changed from

==========
Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806
==========

to

==========
Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806
==========

[commit-bot: I haz the power, 2016-02-25 20:41:29]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2016-02-25 20:42:49]

Description was changed from

==========
Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806
==========

to

==========
Add Expect CT policy that gets checked on all certs

This CL introduces an Expect CT policy in the form of a
CTPolicyEnforcer::DoesConformToCertPolicy() method. This policy is
checked on all certs, and the results are stored in SSLInfo. In a future CL,
this SSLInfo field will be used to determine whether or not to send a
report for a site that expected valid CT to info to be present on its
connections.

BUG=568806

Committed: https://crrev.com/0fc8d0784ff725ffcab646769ec1f1765c2d4013
Cr-Commit-Position: refs/heads/master@{#377662}
==========

[commit-bot: I haz the power, 2016-02-25 20:42:51]

Patchset 15 (id:??) landed as
https://crrev.com/0fc8d0784ff725ffcab646769ec1f1765c2d4013
Cr-Commit-Position: refs/heads/master@{#377662}