Certificate Transparency: Require SCTs for EV certificates.

Certificate Transparency: Require SCTs for EV certificates.

Add a flag to enforce the policy detailed here:
http://dev.chromium.org/Home/chromium-security/certificate-transparency

BUG=397458
Committed: https://crrev.com/6571b2b68658337e301c074763383e0b5c231aea
Cr-Commit-Position: refs/heads/master@{#306612}

[Ryan Sleevi, 2014-08-05 22:19:11]

https://codereview.chromium.org/422063004/diff/20001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:48: const ct::CTVerifyResult& ct_result) = 0;
Comments elsewhere regarding layering, but you can see that, conceptually, the
red flag is here because it requires every CT verifier implement a concrete
definition of what Chrome's policy is. This makes it very easy for two different
CTVerifiers to end up with different definitions of what conforming means, even
though there is only One True Policy.

This is (one of) the reasons to split this out.

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:251: int num_valid_scts =
ct_result.verified_scts.size();
1) Wrong integer type (permeates the rest of this file)

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:258: //about log operators is available.
1) Style: Spaces after //
2) Bugs filed.

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:274: int min_acceptable_logs =
std::max((unsigned long) 2, logs_.size());
1) C-cast
2) Wrong type (.size() == size_t)
3) Bad downcast (size_t -> int)

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:50: const ct::CTVerifyResult& ct_result)
OVERRIDE;
I see nothing that requires this to hang off the CT verifier. There's no special
access to members.

In terms of single-responsibility, I'd rather keep Chrome's specific policies on
validated SCTs + chains independent of the ability to check/validate SCTs.

https://codereview.chromium.org/422063004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3511: }
Another sign of layering concern is that this logic is getting embedded in a
particular platform implementation, rather than somewhere generic/platform
agnostic.

Conceptually, I think:
- We want a single class responsible for verifying certificates/certificate
chains (CertVerifier)
- We want a single class responsible for verifying correctness/validity of SCTs
(CTVerifier)
- We likely want a single class responsible for validating "policy" (where
policy includes things like PKP data, CT policy, etc)

I will have to think about this more, though. Right now, CertVerifier has 'part'
of the policy (validity periods, intranet names), and the SocketNSS has another
part (PKP validation, of which there are several open bugs because of this being
handled here).

There's also a further 'wrench' in things regarding how these policies affect
other aspects of "TLS-using" code - such as extensions/apps and Pepper/NaCl -
that we haven't quite ironed out as to which is the appropriate layer.

[Ryan Sleevi, 2014-08-08 00:25:35]

+wtc

So, today, the Chrome policy specific bits for *certs* are kept in
cert_verify_proc.cc. This is because we want to be able to use caching of
results for multiple connections, and to be able to do multi-threading.

When Wan-Teh and I talked, we liked the idea of making it part of CertVerifier,
but I think having the CT verification as problematic for a couple reasons:
1) CertVerifier::Verify is pure-virtual. The meat lives in CertVerifyProc
2) CertVerifyProc has to be able to be called from any thread, and needs all of
its input parameters (To CertVerifyProc::Verify) to be accessible by any thread

So I don't think we can/should easily plumb the CTVerifier down into
CertVerifyProc. Also, it doesn't seem ideal, because we know we want CT
verification to be isolated to individual connections (that is, just because you
got an SCT for Cert A on Conn 1, if Conn 2 uses Cert A and doesn't supply an
SCT, Conn 2 shouldn't be valid)

Logically, I think Wan-Teh and I are in agreement that we want to have a single
class that provides the ability to (verify a certificate, confirm the
certificate matches CHrome's policies, verify the SCTs, confirm the SCTs match
Chrome's policies, and maybe verify pins). We just don't have that class yet.

But that hopefully gives you the thinking about where this stands.

I'll take another look, and take some more time to noodle on this, especially
when we want things to be consistent across other "SSL-using" code, but where
there may be some differences (like GCM, Extensions, Apps, Pepper, etc)

[Eran Messeri, 2014-10-08 11:21:05]

On 2014/08/08 00:25:35, Ryan Sleevi wrote:
> +wtc
> 
> So, today, the Chrome policy specific bits for *certs* are kept in
> cert_verify_proc.cc. This is because we want to be able to use caching of
> results for multiple connections, and to be able to do multi-threading.
> 
> When Wan-Teh and I talked, we liked the idea of making it part of
CertVerifier,
> but I think having the CT verification as problematic for a couple reasons:
> 1) CertVerifier::Verify is pure-virtual. The meat lives in CertVerifyProc
> 2) CertVerifyProc has to be able to be called from any thread, and needs all
of
> its input parameters (To CertVerifyProc::Verify) to be accessible by any
thread
> 
> So I don't think we can/should easily plumb the CTVerifier down into
> CertVerifyProc. Also, it doesn't seem ideal, because we know we want CT
> verification to be isolated to individual connections (that is, just because
you
> got an SCT for Cert A on Conn 1, if Conn 2 uses Cert A and doesn't supply an
> SCT, Conn 2 shouldn't be valid)
> 
> Logically, I think Wan-Teh and I are in agreement that we want to have a
single
> class that provides the ability to (verify a certificate, confirm the
> certificate matches CHrome's policies, verify the SCTs, confirm the SCTs match
> Chrome's policies, and maybe verify pins). We just don't have that class yet.
> 
> But that hopefully gives you the thinking about where this stands.
> 
> I'll take another look, and take some more time to noodle on this, especially
> when we want things to be consistent across other "SSL-using" code, but where
> there may be some differences (like GCM, Extensions, Apps, Pepper, etc)

Based on this comment and offline discussion, I'll create a separate class to
verify adherence to the CT/EV policy, which will still be called from
SSLClientSocket* (i.e. not threaded further down in the CertVerifier, no moving
around of the CTVerifier).

How does that sound?

[Eran Messeri, 2014-10-20 17:26:29]

eranm@chromium.org changed reviewers:
- wtc@chromium.org

[Eran Messeri, 2014-10-20 17:26:30]

Ryan, I've split the policy checks to a separate class, is that what you have in
mind?

(also removed wtc@ from the reviewers list)

https://codereview.chromium.org/422063004/diff/20001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:48: const ct::CTVerifyResult& ct_result) = 0;
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> Comments elsewhere regarding layering, but you can see that, conceptually, the
> red flag is here because it requires every CT verifier implement a concrete
> definition of what Chrome's policy is. This makes it very easy for two
different
> CTVerifiers to end up with different definitions of what conforming means,
even
> though there is only One True Policy.
> 
> This is (one of) the reasons to split this out.

Moved to a separate class (this interface now only provides the number of logs
it knows about).

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:251: int num_valid_scts =
ct_result.verified_scts.size();
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> 1) Wrong integer type (permeates the rest of this file)

Done.

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:258: //about log operators is available.
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> 1) Style: Spaces after //
> 2) Bugs filed.

Done and done.

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:274: int min_acceptable_logs =
std::max((unsigned long) 2, logs_.size());
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> 1) C-cast
> 2) Wrong type (.size() == size_t)
> 3) Bad downcast (size_t -> int)

Fixed all (hopefully) by making min_acceptable_logs size_t and using 2ul as the
literal.

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:50: const ct::CTVerifyResult& ct_result)
OVERRIDE;
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> I see nothing that requires this to hang off the CT verifier. There's no
special
> access to members.
> 
> In terms of single-responsibility, I'd rather keep Chrome's specific policies
on
> validated SCTs + chains independent of the ability to check/validate SCTs.

The number of logs is necessary - but I can make the CTVerifier return that
information.

https://codereview.chromium.org/422063004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3511: }
On 2014/08/05 22:19:10, Ryan Sleevi wrote:
> Another sign of layering concern is that this logic is getting embedded in a
> particular platform implementation, rather than somewhere generic/platform
> agnostic.
> 
> Conceptually, I think:
> - We want a single class responsible for verifying certificates/certificate
> chains (CertVerifier)
> - We want a single class responsible for verifying correctness/validity of
SCTs
> (CTVerifier)
> - We likely want a single class responsible for validating "policy" (where
> policy includes things like PKP data, CT policy, etc)
> 
> I will have to think about this more, though. Right now, CertVerifier has
'part'
> of the policy (validity periods, intranet names), and the SocketNSS has
another
> part (PKP validation, of which there are several open bugs because of this
being
> handled here).
> 
> There's also a further 'wrench' in things regarding how these policies affect
> other aspects of "TLS-using" code - such as extensions/apps and Pepper/NaCl -
> that we haven't quite ironed out as to which is the appropriate layer.

I've created a new class, CertPolicyEnforcer, which would be used from
SSLClientSocketNSS and SSLClientSocketOpenSSL. Does that work? Anything else you
want me to move there?

[Eran Messeri, 2014-10-22 15:52:00]

Addressed issues with casting/overflow, code should be ready for review.

[Ryan Sleevi, 2014-10-22 19:48:36]

Definitely some layering issues. Also, the static is a big red flag - this is
something that we should probably be passing in from the IO thread and pushing
down through the layers.

Also, the QUIC code needs updating.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:41: }
no braces on single-line if's in //net

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:43:
base::saturated_cast<uint16_t>(ct_result.verified_scts.size());
Casting is wrong here, isn't it? Shouldn't it be uint8_t?

But the size_t remarks will address this.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:61: // Will not be able to calculate the
certificate's validity period.
Shouldn't this be the first check? to avoid iterating the number of SCTs if the
information is unusuable?

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:83: }  // namespace net
newline between 82 & 83

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:19: class NET_EXPORT CertPolicyEnforcer {
Document

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:21: explicit CertPolicyEnforcer(uint32_t
num_ct_logs);
Document

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:34: static void SetEnforceCTEVPolicy(bool
enforce_policy);
Why is this a static? Why is this just not an artifact of the enforcer?

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:37: uint8_t num_ct_logs_;
STYLE: Why these data type conversions? See
http://www.chromium.org/developers/coding-style#TOC-Types (use size_t )

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:24:
CertPolicyEnforcer::SetEnforceCTEVPolicy(true);
This test has global effect that isn't reset.

(As an artifact that our tests run a single test per executable run, you're
being saved, but that's generally a test smell)

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:58: // We know that the chain_ is
valid for 10 years - over 121 months - so
Don't include pronouns in comments -
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:86: 
Seems like there's a lot of stuff in the enforcer that remains untested, such as
the 4/3/2 SCT split, invalid dates, etc. Please consider adding a test for every
branch in the enforcer.

https://codereview.chromium.org/422063004/diff/80001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:45: virtual uint32_t GetNumKnownLogs() = 0;
size_t

That said, this is a layering violation that the CTVerifier is exposing details
about the number of known logs, which this interface hides the details of. I'd
rather not expose this, and get the information passed through another way.

https://codereview.chromium.org/422063004/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:2811: policy_enforcer_(new
CertPolicyEnforcer(
This should be passed in, not created.

[Eran Messeri, 2014-10-24 12:12:36]

As discussed offline, threaded the CertPolicyEnforcer from the IOThread all the
way to the SSLClientSocket*.

Please note:
* The Quic verifier does not support CT at all at the moment so I haven't
touched it in this CL. I hope to do this in a follow-up one.
* I am feeling somewhat bad for adding a 17th constructor parameter. I've
searched, but not found, for another parameter to incorporate into the new
CertPolicyEnforcer. Suggestions welcome.

Ryan, PTAL before I add a reviewer for the chrome/browser/profiles/ changes.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:41: }
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> no braces on single-line if's in //net

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:43:
base::saturated_cast<uint16_t>(ct_result.verified_scts.size());
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Casting is wrong here, isn't it? Shouldn't it be uint8_t?
> 
> But the size_t remarks will address this.

Switched to size_t throughout, as suggested.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:61: // Will not be able to calculate the
certificate's validity period.
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Shouldn't this be the first check? to avoid iterating the number of SCTs if
the
> information is unusuable?

I believe not: The policy (outlined in the CT/EV plan) only cares about the
validity period if there aren't enough non-embedded SCTs. This code tries to
accurately implement the policy so as long as there are enough non-embedded SCTs
I don't care if it has a valid start/expiry dates.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.cc:83: }  // namespace net
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> newline between 82 & 83

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:19: class NET_EXPORT CertPolicyEnforcer {
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:21: explicit CertPolicyEnforcer(uint32_t
num_ct_logs);
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:34: static void SetEnforceCTEVPolicy(bool
enforce_policy);
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Why is this a static? Why is this just not an artifact of the enforcer?

Removed in favour of a c'tor parameter.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer.h:37: uint8_t num_ct_logs_;
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> STYLE: Why these data type conversions? See
> http://www.chromium.org/developers/coding-style#TOC-Types (use size_t )

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:24:
CertPolicyEnforcer::SetEnforceCTEVPolicy(true);
On 2014/10/22 19:48:36, Ryan Sleevi wrote:
> This test has global effect that isn't reset.
> 
> (As an artifact that our tests run a single test per executable run, you're
> being saved, but that's generally a test smell)

Good point - now that it's not a global state, this line is gone.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:58: // We know that the chain_ is
valid for 10 years - over 121 months - so
On 2014/10/22 19:48:36, Ryan Sleevi wrote:
> Don't include pronouns in comments -
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

Done.

https://codereview.chromium.org/422063004/diff/80001/net/cert/cert_policy_enf...
net/cert/cert_policy_enforcer_unittest.cc:86: 
On 2014/10/22 19:48:35, Ryan Sleevi wrote:
> Seems like there's a lot of stuff in the enforcer that remains untested, such
as
> the 4/3/2 SCT split, invalid dates, etc. Please consider adding a test for
every
> branch in the enforcer.

Good point, added tests for all branches.

https://codereview.chromium.org/422063004/diff/80001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/422063004/diff/80001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:45: virtual uint32_t GetNumKnownLogs() = 0;
On 2014/10/22 19:48:36, Ryan Sleevi wrote:
> size_t
> 
> That said, this is a layering violation that the CTVerifier is exposing
details
> about the number of known logs, which this interface hides the details of. I'd
> rather not expose this, and get the information passed through another way.

Removed this method entirely.

https://codereview.chromium.org/422063004/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:2811: policy_enforcer_(new
CertPolicyEnforcer(
On 2014/10/22 19:48:36, Ryan Sleevi wrote:
> This should be passed in, not created.

Done. It's passed *all* the way from the IOThread.

[Eran Messeri, 2014-11-02 20:43:34]

Added tests as requested. Ryan, PTAL.

[Eran Messeri, 2014-11-04 22:56:16]

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

[Eran Messeri, 2014-11-04 22:57:41]

Adding mmenke@ for the chrome/browser/profiles/* changes.
Matt, let me know if I should provide additional context. Willchan@ reviewed
CT-related changes recently, but I understand he's on extended leave.

[mmenke, 2014-11-05 19:32:36]

https://codereview.chromium.org/422063004/diff/220001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/422063004/diff/220001/net/url_request/url_req...
net/url_request/url_request_context.h:180: }
Does this need to be part of the context?

You're not setting it in a number of the contexts created for a profile, and I'm
wondering if we really need it at this layer, or if we should fix that.

[Ryan Sleevi, 2014-11-06 00:16:43]

https://codereview.chromium.org/422063004/diff/220001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:648: // by for EV certificates. Remove this flag for
M41.
While I know the delays have been due to my review load, this won't make it in
for M40.

So this is for M41, which should remove this flag.

However, you will want to Finch gate it ( http://go/finch ). I can walk you
through that tomorrow morning if you want.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:50: 
Note that this code is missing the build date freshness check

see
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport...

This is also where you can place the Finch flag (which is part of //base as
well)

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:62: if (num_non_embedded_scts >= 2) {
net code omits braces for single-line conditionals (lines 62, 66), but uses them
for multi-line (lines 70-73 are fine) or, traditionally, for complex cases
(lines 80-87 are fine)

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:76: uint32_t expiry_in_months_approx =
expiry_period.InDays() / 30.14;
Does it make more sense to adopt math similar to that on
https://codereview.chromium.org/20628006/diff/289001/net/cert/cert_verify_pro...

(See lines 627-644)

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:90: size_t min_acceptable_logs =
std::max(static_cast<size_t>(2), num_ct_logs_);
just do 2U and it should be sufficient, right?

Or place num_ct_logs_ as the first arg?

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:16: }  // namespace ct
newline before 14 & after 15

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:29: CertPolicyEnforcer(size_t num_ct_logs, bool
require_ct_for_ev);
You can drop the bool now, explicit CertPolicyEnforcer

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:40: // TODO(eranm): Add netlog
Move the TODO to line 37, add a bug #

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3551: (policy_enforcer_)) {
Let's make sure we 'fail closed', which also addresses Matt's concern re:
contexts

if (!policy_enforcer) {
  server_cert_verify_result_.cert_status &= ~CERT_STATUS_IS_EV;
} else {
  ....
}

This guarantees we'll never grant EV for anything that doesn't go through the CT
check, and everything that does can just have it disabled if necessary (Finch or
bool, tomato tomato)

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1200: }
ditto here

[Ryan Sleevi, 2014-11-11 02:50:18]

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:57: ct_result.verified_scts.end(),
IsEmbeddedSCT);
So, from the ct-policy discussion, I should have paid closer attention to the
policy.

This allows certificates that were logged BEFORE the log is included (per the
policy) to be accepted as if they were valid embedded certs. It's a BUG because
the code doesn't match the stated policy.

I think our internal database of verified_scts needs to include the approval
date for verification of it being embedded (in the CTVerifier or possibly
CTLogVerifier - not sure where this policy flag should land)

[Eran Messeri, 2014-11-19 09:59:35]

On 2014/11/11 02:50:18, Ryan Sleevi (OOO until 11-18) wrote:
>
https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
> File net/cert/cert_policy_enforcer.cc (right):
> 
>
https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
> net/cert/cert_policy_enforcer.cc:57: ct_result.verified_scts.end(),
> IsEmbeddedSCT);
> So, from the ct-policy discussion, I should have paid closer attention to the
> policy.
> 
> This allows certificates that were logged BEFORE the log is included (per the
> policy) to be accepted as if they were valid embedded certs. It's a BUG
because
> the code doesn't match the stated policy.
> 
> I think our internal database of verified_scts needs to include the approval
> date for verification of it being embedded (in the CTVerifier or possibly
> CTLogVerifier - not sure where this policy flag should land)

My understanding of the policy is the wording there is to make it clear SCTs
from invalid logs are discounted. I agree with your opinion that once a log has
been disqualified for misbehaviour, it's game over for it - SCTs issued by that
log are no longer honoured. As long as the log has not been shown to misbehave,
though, I don't see why we should care about when it issued an SCT.
I also would like to avoid over-complicating the policy checks here by adding
the inclusion date of the log (it would otherwise re-introduce the
chicken-and-egg problem where no actor has an incentive to log certificates in a
log that is not yet recognized by Chrome, so we would end up monitoring a log
that is not really in active use and can't really assess it's reliability).

[Eran Messeri, 2014-11-19 10:00:23]

Quick update: I am in the process of addressing Ryan's comments and removing the
CertPolicyEnforcer from the URLRequestContext per mmenke@'s suggestion. Will
update the issue shortly.

https://codereview.chromium.org/422063004/diff/220001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/422063004/diff/220001/net/url_request/url_req...
net/url_request/url_request_context.h:180: }
On 2014/11/05 19:32:36, mmenke wrote:
> Does this need to be part of the context?
> 
> You're not setting it in a number of the contexts created for a profile, and
I'm
> wondering if we really need it at this layer, or if we should fix that.

Matt says:
"..I'm not suggesting it not be added to the HttpNetworkSession. Just that it
not be added as part of the URLRequestContext"
Further suggesting that the CertPolicyenforcer is set for the
HttpNetworkSession::Params in IOThread::InitializeNetworkSessionParams.

Regarding thread safety:
"In terms of lifetimes, it's safe. IOThread outlives ProfileIOData for all
profiles
They basically all depend on IOThread::Globals, so already not safe to destroy
them after the IOThread
(And the destruction of the IOThread object is the last thing the IO Thread
does, IIRC)"

[Eran Messeri, 2014-11-20 11:49:56]

In this patchset:
* I've addressed all comments.
* Reverted the changes to the URLRequestContext (they are not necessary).
* Not added any Finch-related code (will do in a follow-up patch).

PTAL.

https://codereview.chromium.org/422063004/diff/220001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:648: // by for EV certificates. Remove this flag for
M41.
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> While I know the delays have been due to my review load, this won't make it in
> for M40.
> 
> So this is for M41, which should remove this flag.
> 
> However, you will want to Finch gate it ( http://go/finch ). I can walk you
> through that tomorrow morning if you want.

Per our offline discussion, this patch will be immediately followed up by a
patch introducing Finch support, but it would be useful to have this patch land
with the policy enforcement behind a flag so we can tell CAs they can already
test compliance.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:50: 
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> Note that this code is missing the build date freshness check
> 
> see
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport...
> 
> This is also where you can place the Finch flag (which is part of //base as
> well)

Added (by shamelessly copying it from TransportSecurityState class. Can you
suggest somewhere else for it to live?)

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:57: ct_result.verified_scts.end(),
IsEmbeddedSCT);
On 2014/11/11 02:50:18, Ryan Sleevi (OOO until 11-18) wrote:
> So, from the ct-policy discussion, I should have paid closer attention to the
> policy.
> 
> This allows certificates that were logged BEFORE the log is included (per the
> policy) to be accepted as if they were valid embedded certs. It's a BUG
because
> the code doesn't match the stated policy.
> 
> I think our internal database of verified_scts needs to include the approval
> date for verification of it being embedded (in the CTVerifier or possibly
> CTLogVerifier - not sure where this policy flag should land)

See my previous reply - IIUC that's to avoid distrusted logs rather than logs
which we've yet to fully accept because we're  monitoring them (so there's no
evidence of misbehaviour yet).

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:62: if (num_non_embedded_scts >= 2) {
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> net code omits braces for single-line conditionals (lines 62, 66), but uses
them
> for multi-line (lines 70-73 are fine) or, traditionally, for complex cases
> (lines 80-87 are fine)

Done per your guidelines.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:76: uint32_t expiry_in_months_approx =
expiry_period.InDays() / 30.14;
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> Does it make more sense to adopt math similar to that on
>
https://codereview.chromium.org/20628006/diff/289001/net/cert/cert_verify_pro...
> 
> (See lines 627-644)

Yes, used similar logic here.
I considered adding the functionality to TImeDelta but unlike other time units
there, it would be an approximation rather than exact value, so I'll follow up
with an owner to see if that makes sense.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:90: size_t min_acceptable_logs =
std::max(static_cast<size_t>(2), num_ct_logs_);
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> just do 2U and it should be sufficient, right?
> 
> Or place num_ct_logs_ as the first arg?

Tried both, the compiler was equally unhappy - I have to explicitly cast to
size_t.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:16: }  // namespace ct
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> newline before 14 & after 15

Done.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:29: CertPolicyEnforcer(size_t num_ct_logs, bool
require_ct_for_ev);
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> You can drop the bool now, explicit CertPolicyEnforcer

Will do when we use Finch.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:29: CertPolicyEnforcer(size_t num_ct_logs, bool
require_ct_for_ev);
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> You can drop the bool now, explicit CertPolicyEnforcer

Will do, when I introduce Finch.

https://codereview.chromium.org/422063004/diff/220001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:40: // TODO(eranm): Add netlog
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> Move the TODO to line 37, add a bug #

Removed; turns out I don't need the NetLog here.

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3551: (policy_enforcer_)) {
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> Let's make sure we 'fail closed', which also addresses Matt's concern re:
> contexts
> 
> if (!policy_enforcer) {
>   server_cert_verify_result_.cert_status &= ~CERT_STATUS_IS_EV;
> } else {
>   ....
> }
> 
> This guarantees we'll never grant EV for anything that doesn't go through the
CT
> check, and everything that does can just have it disabled if necessary (Finch
or
> bool, tomato tomato)

Done.

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/422063004/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1200: }
On 2014/11/06 00:16:43, Ryan Sleevi (OOO until 11-18) wrote:
> ditto here

Done.

[mmenke, 2014-11-20 15:38:22]

How this is hooked up LGTM.  You still need rsleevi to sign off on the details,
of course.

[Ryan Sleevi, 2014-11-28 15:27:45]

Updated the description to remove the bit about the flag.

1) Because I suck at reviewing, we're already at Chrome 41
2) We may leave this flag in place for a small amount of time for "Enterprise"
reasons.

Mostly nits here and FUTURE WORK. I think the biggest change is to some of the
tests for coverage.

I PROMISE I'll review any follow-up changes in a timely manner, since this
almost LG :)

https://codereview.chromium.org/422063004/diff/260001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:647: // by for EV certificates. Remove this flag for
M41.
This comment is out of date.

// TODO(eranm): Control with Finch

(See https://goto.google.com/finch-and-flags for the way to do this )

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:26: // Taken from
TransportSecurityState::IsBuildTimely
Don't describe where this is from, describe why it exists and is relevant.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:43: end.UTCExplode(&exploded_expiry);
There are base::Time values that may not be represented as UTC time values (due
to limitations of time_t)

You want to check whether either of these times is == Max()

[Note, for certs, it's messier; the X509Certificate won't parse on Windows and
the OSCertHandle will == NULL. On Linux, because of our use of NSS, the PRTime
can represent a greater range than time_t, so it can be < Max() but still >
TimeT::Max(); on OS X/Android/iOS it will == Max()}

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:73: std::string(reinterpret_cast<const
char*>(fingerprint.data), 8);
Note: If we make ContainsCertificateHash to take a base::StringPiece instead of
an std::string, we could truncate just using a base::StringPiece here and avoid
a copy.

Then again, this will trigger std::string's small string optimization and copy 8
bytes onto the stack, so perhaps its no big deal. Just calling it out.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:80: << (cert_in_ev_whitelist ? "true" :
"false");
I'm not keen on this VLOG. If the intent is to aid debugging in the field, it
should go into the NetLog instead (or likely, a BoundNetLog which can be bound
to the certificate verification event). Alternatively, removing it.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:104: return true;
I find this conditional hard to reason about (as with those on 106/107), so you
should consider explaining why / what portion of this policy it applies to and
how this logically works.

// If at least two valid SCTs were delivered by means other than embedding (i.e.
in a TLS extension or OCSP),
// then the certificate conforms to Sections 2/3 of the CT/EV policy.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:107: return true;
This doesn't seem spelled out in our policy at all. I think this should be
removed?

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:131: std::min(num_required_embedded_scts,
min_acceptable_logs);
BUG/FUTURE: This doesn't consider when we need to distrust a log. Can you file a
bug and add a TODO here to indicate that the number of embedded SCTs that are
signature valid are X, but the number of embedded SCTs required to be valid
(recognized by Chrome) is 1.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:36: // certificates (as indicated in the c'tor).
I'd dropped everything from this comment beginning with "and CT presence ..."
and ends with "c'tor)."

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:38: // to determine the certificate's lifetime).
Drop the ( ... ) comment

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:39: // |ct_result| is the CTVerifyResult filled
in by the Verify call.
This comment needs more fleshing out

// |ct_result| must contain the result of verifying any SCTs associated with
|cert| prior to invoking this method.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:46: bool enforce_ct_requirement_;
NIT: name this consistently with your constructor argument.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:38: };
Seems like this could/should be replaced with GMock, since that's what this
really is.

class MockEVCertsWhitelist : public ct::EVCertsWhitelist {
 public:
  MockEVCertsWhitelist() {
    ON_CALL(*this, IsValid()).WillByDefault(Return(true));
    ON_CALL(*this, ContainsCertificateHash(_)).WillByDefault(Return(false));
  }
  MOCK_CONST_METHOD0(IsValid(), bool());
  MOCK_CONST_METHOD1(ContainsCertificateHash(), bool(const std::string&));
};

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:47: der_test_cert.length());
s/.length()/.size()/

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:49: whitelist_ = new
DummyEVCertsWhitelist(false);
Nuke this in favour of consistency among the tests (each initializing whitelist
themselves?)

The asymmetry of 177 makes it weird.

This also lets you override how IsValid() works and to ALSO make sure that an EV
whitelist that isn't valid is ignored.

Tests for:
1) No whitelist (== nullptr) - 123 - 161 indirectly test this, but not really
2) Whitelist !IsValid() - No such test exists for this right now 
3) Whitelist IsValid(), !ContainsCertificateHash() - implicitly being tested by
70 - 175
4) Whitelist IsValid(), ContainsCertificateHash() - being tested by 177-186

Note that for each of the EXPECT_FALSE(Conforms(...)), I'd like to see an
EXPECT_TRUE(Conforms(...)) with a different MockEVCertsWhitelist (or you can
just use the existing one and replace the ON_CALL).

This is to make sure our whitelist is good for _all_ cases (namely, that we
always check the whitelist first, and the whitelist overrides any other
requirements we may have)

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3559: <<
ct_verify_result_.unknown_logs_scts.size();
This is all NetLogged right now - we should be nuking this VLOG.

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3573: << " does not conform to CT policy,
removing EV status.";
NetLog

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1234: <<
ct_verify_result_.unknown_logs_scts.size();
Already netlogged as well.

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1248: << " does not conform to CT
policy, removing EV status.";
NetLog

[Eran Messeri, 2014-11-29 23:07:07]

Ryan, per your request, adding Finch and removing the command-line flag.

[Eran Messeri, 2014-12-01 13:59:03]

Addressed all review comments, PTAL.
Notes:
* The follow-up items are documented in
https://code.google.com/p/chromium/issues/detail?id=437766
* The flag remains.

Thanks!

https://codereview.chromium.org/422063004/diff/260001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:647: // by for EV certificates. Remove this flag for
M41.
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> This comment is out of date.
> 
> // TODO(eranm): Control with Finch
> 
> (See https://goto.google.com/finch-and-flags for the way to do this )

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:26: // Taken from
TransportSecurityState::IsBuildTimely
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Don't describe where this is from, describe why it exists and is relevant.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:43: end.UTCExplode(&exploded_expiry);
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> There are base::Time values that may not be represented as UTC time values
(due
> to limitations of time_t)
> 
> You want to check whether either of these times is == Max()
> 
> [Note, for certs, it's messier; the X509Certificate won't parse on Windows and
> the OSCertHandle will == NULL. On Linux, because of our use of NSS, the PRTime
> can represent a greater range than time_t, so it can be < Max() but still >
> TimeT::Max(); on OS X/Android/iOS it will == Max()}

As discussed offline, added checks for is_max below; Can't easily  add a test
because it depends on whether it's running on a 32-bit or a 64-bit machine
(it'll only fail on a 32-bit machine)

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:73: std::string(reinterpret_cast<const
char*>(fingerprint.data), 8);
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Note: If we make ContainsCertificateHash to take a base::StringPiece instead
of
> an std::string, we could truncate just using a base::StringPiece here and
avoid
> a copy.
> 
> Then again, this will trigger std::string's small string optimization and copy
8
> bytes onto the stack, so perhaps its no big deal. Just calling it out.

Acknowledged, since it's changing the interface (which is implemented in
//chrome/browser/net) it'll be neater to address in a follow-up optimization CL.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:80: << (cert_in_ev_whitelist ? "true" :
"false");
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> I'm not keen on this VLOG. If the intent is to aid debugging in the field, it
> should go into the NetLog instead (or likely, a BoundNetLog which can be bound
> to the certificate verification event). Alternatively, removing it.

Removed, for now. I'll can NetLog logging in a follow-up change (although I'm
not sure it's necessary because it should be able to tell from the histograms
what happened with each cert).

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:104: return true;
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> I find this conditional hard to reason about (as with those on 106/107), so
you
> should consider explaining why / what portion of this policy it applies to and
> how this logically works.
> 
> // If at least two valid SCTs were delivered by means other than embedding
(i.e.
> in a TLS extension or OCSP),
> // then the certificate conforms to Sections 2/3 of the CT/EV policy.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:107: return true;
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> This doesn't seem spelled out in our policy at all. I think this should be
> removed?

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.h (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:36: // certificates (as indicated in the c'tor).
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> I'd dropped everything from this comment beginning with "and CT presence ..."
> and ends with "c'tor)."

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:38: // to determine the certificate's lifetime).
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Drop the ( ... ) comment

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:39: // |ct_result| is the CTVerifyResult filled
in by the Verify call.
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> This comment needs more fleshing out
> 
> // |ct_result| must contain the result of verifying any SCTs associated with
> |cert| prior to invoking this method.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.h:46: bool enforce_ct_requirement_;
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> NIT: name this consistently with your constructor argument.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:38: };
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Seems like this could/should be replaced with GMock, since that's what this
> really is.
> 
> class MockEVCertsWhitelist : public ct::EVCertsWhitelist {
>  public:
>   MockEVCertsWhitelist() {
>     ON_CALL(*this, IsValid()).WillByDefault(Return(true));
>     ON_CALL(*this, ContainsCertificateHash(_)).WillByDefault(Return(false));
>   }
>   MOCK_CONST_METHOD0(IsValid(), bool());
>   MOCK_CONST_METHOD1(ContainsCertificateHash(), bool(const std::string&));
> };

Done with a slight variation: I couldn't use ON_CALL on the constructed object
because it's a ref-counted class, so I passed in the return parameters  to the
c'tor.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:47: der_test_cert.length());
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> s/.length()/.size()/

Done.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:49: whitelist_ = new
DummyEVCertsWhitelist(false);
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Nuke this in favour of consistency among the tests (each initializing
whitelist
> themselves?)
> 
> The asymmetry of 177 makes it weird.
> 
> This also lets you override how IsValid() works and to ALSO make sure that an
EV
> whitelist that isn't valid is ignored.
> 
> Tests for:
> 1) No whitelist (== nullptr) - 123 - 161 indirectly test this, but not really
> 2) Whitelist !IsValid() - No such test exists for this right now 
> 3) Whitelist IsValid(), !ContainsCertificateHash() - implicitly being tested
by
> 70 - 175
> 4) Whitelist IsValid(), ContainsCertificateHash() - being tested by 177-186
> 
> Note that for each of the EXPECT_FALSE(Conforms(...)), I'd like to see an
> EXPECT_TRUE(Conforms(...)) with a different MockEVCertsWhitelist (or you can
> just use the existing one and replace the ON_CALL).
> 
> This is to make sure our whitelist is good for _all_ cases (namely, that we
> always check the whitelist first, and the whitelist overrides any other
> requirements we may have)

Done, per your suggestion, particularly:
(1) used nullptr as whitelist in all tests that expect success.
(2) tested nullptr whitelist in a test of its own (IgnoresNullEVWhitelist)
(3) Explicit test for !IsValid whitelist (IgnoresInvalidEVWhitelist)
(4) Explicit test for IsValid, ContainsCertificateHash (
ConformsToPolicyByEVWhitelistPresence)
(5) In all tests that expect failure, added a second EXPECT_TRUE with a
whitelist where IsValid and ContainsCertificateHash return true.
(6) In one of the tests also added the use of a valid whitelist where
ContainsCertificateHash returns false.

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3559: <<
ct_verify_result_.unknown_logs_scts.size();
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> This is all NetLogged right now - we should be nuking this VLOG.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:3573: << " does not conform to CT policy,
removing EV status.";
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> NetLog

Done (added TODO with bug#).

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1234: <<
ct_verify_result_.unknown_logs_scts.size();
On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> Already netlogged as well.

Done.

https://codereview.chromium.org/422063004/diff/260001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1248: << " does not conform to CT
policy, removing EV status.";
On 2014/11/28 15:27:45, Ryan Sleevi wrote:
> NetLog

Done (added TODO with bug#).

[Ryan Sleevi, 2014-12-01 15:27:55]

LGTM mod a few nits.

You'll need to add a histograms.xml reviewer if you keep the UMA. Suggesting
isherman.

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:43: end.UTCExplode(&exploded_expiry);
On 2014/12/01 13:59:02, Eran wrote:
> On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> > There are base::Time values that may not be represented as UTC time values
> (due
> > to limitations of time_t)
> > 
> > You want to check whether either of these times is == Max()
> > 
> > [Note, for certs, it's messier; the X509Certificate won't parse on Windows
and
> > the OSCertHandle will == NULL. On Linux, because of our use of NSS, the
PRTime
> > can represent a greater range than time_t, so it can be < Max() but still >
> > TimeT::Max(); on OS X/Android/iOS it will == Max()}
> 
> As discussed offline, added checks for is_max below; Can't easily  add a test
> because it depends on whether it's running on a 32-bit or a 64-bit machine
> (it'll only fail on a 32-bit machine)

Yeah, we'll come back to this as a follow-on (we can look at
std::numeric_limits<time_t>::max() on the POSIX-ish systems, for example, and
use that as a gauge for EXPECT_TRUE or EXPECT_FALSE, etc)

https://codereview.chromium.org/422063004/diff/260001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:80: << (cert_in_ev_whitelist ? "true" :
"false");
On 2014/12/01 13:59:02, Eran wrote:
> On 2014/11/28 15:27:44, Ryan Sleevi wrote:
> > I'm not keen on this VLOG. If the intent is to aid debugging in the field,
it
> > should go into the NetLog instead (or likely, a BoundNetLog which can be
bound
> > to the certificate verification event). Alternatively, removing it.
> 
> Removed, for now. I'll can NetLog logging in a follow-up change (although I'm
> not sure it's necessary because it should be able to tell from the histograms
> what happened with each cert).

The histograms won't tell us (or the net-internals user) which cert it was that
failed or why. We'll know that some certs aren't in, but if the user doesn't see
EV, they won't know if it's for this reason or some other reason.

So I'm still keen on NetLogs for this.

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:646: // TODO(eranm): Control with Finch.
Bug # :)

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/net/pack...
File chrome/browser/net/packed_ct_ev_whitelist.cc (right):

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/net/pack...
chrome/browser/net/packed_ct_ev_whitelist.cc:117: is_whitelist_valid_ = true;
This doesn't seem used at all in this .cc; unnecessary? Or did you also mean to
update line 137?

Seems like this part of the change should be nuked.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:58: "ExperimentGroup";
Unnecessary part of your in-process change?

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:72: }
missed this before, add a newline between the two }

and then on line 72
}  // namespace

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:86: if (!(RequireCTForEV() ||
require_ct_for_ev_))
I generally dislike extra nested

if (!RequireCTForEV() && !require_ct_for_ev_)
  return true;

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:29:
.WillOnce(testing::Return(contains_hash_return));
Right, I wanted to entirely avoid EXPECT_CALL, it's more of a GMock
anti-pattern.

Put differently: I know it's a pain, but perhaps the original solution was
better; if GMock didn't work, I liked your original code :)

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:97: chain_.get(),
non_including_whitelist.get(), result));
did clang-format (aka "git cl format") do this? If not, be sure to run it on the
CL prior to submitting.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:101: new MockEVCertsWhitelist(true,
true);
Note dominant style in this file is to use ctor initialization rather than copy
initialization

scoped_refptr<ct:...> whitelist(
    new MockEVCertsWhitelist(...));

See line 107, 118, etc

[Ryan Sleevi, 2014-12-01 15:28:12]

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

[Ryan Sleevi, 2014-12-01 15:28:13]

actually adding isherman for histograms now.

[Eran Messeri, 2014-12-01 17:29:55]

Addressed all comments.

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:646: // TODO(eranm): Control with Finch.
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> Bug # :)

Done.

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/net/pack...
File chrome/browser/net/packed_ct_ev_whitelist.cc (right):

https://codereview.chromium.org/422063004/diff/300001/chrome/browser/net/pack...
chrome/browser/net/packed_ct_ev_whitelist.cc:117: is_whitelist_valid_ = true;
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> This doesn't seem used at all in this .cc; unnecessary? Or did you also mean
to
> update line 137?
> 
> Seems like this part of the change should be nuked.

My bad, this is a result of a bad merge. Removed.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer.cc (right):

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:58: "ExperimentGroup";
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> Unnecessary part of your in-process change?

Done, removed.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:72: }
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> missed this before, add a newline between the two }
> 
> and then on line 72
> }  // namespace

Done.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer.cc:86: if (!(RequireCTForEV() ||
require_ct_for_ev_))
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> I generally dislike extra nested
> 
> if (!RequireCTForEV() && !require_ct_for_ev_)
>   return true;

Done.

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
File net/cert/cert_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:97: chain_.get(),
non_including_whitelist.get(), result));
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> did clang-format (aka "git cl format") do this? If not, be sure to run it on
the
> CL prior to submitting.

I can only assume it did (I ran git-cl format).

https://codereview.chromium.org/422063004/diff/300001/net/cert/cert_policy_en...
net/cert/cert_policy_enforcer_unittest.cc:101: new MockEVCertsWhitelist(true,
true);
On 2014/12/01 15:27:55, Ryan Sleevi wrote:
> Note dominant style in this file is to use ctor initialization rather than
copy
> initialization
> 
> scoped_refptr<ct:...> whitelist(
>     new MockEVCertsWhitelist(...));
> 
> See line 107, 118, etc

Done.

[Ilya Sherman, 2014-12-01 23:14:08]

histograms lgtm

https://codereview.chromium.org/422063004/diff/320001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/422063004/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42807: +  <int value="2"
label="CT_ENOUGH_SCTS"/>
Optional suggestion: You might want to use more human-friendly labels, like "Not
compliant", "Whitelisted", etc.

[Eran Messeri, 2014-12-03 14:45:37]

https://codereview.chromium.org/422063004/diff/320001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/422063004/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42807: +  <int value="2"
label="CT_ENOUGH_SCTS"/>
On 2014/12/01 23:14:08, Ilya Sherman wrote:
> Optional suggestion: You might want to use more human-friendly labels, like
"Not
> compliant", "Whitelisted", etc.

Done.

[Eran Messeri, 2014-12-03 14:45:53]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-12-03 14:46:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/422063004/340001

[commit-bot: I haz the power, 2014-12-03 15:53:33]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-12-03 15:54:15]

Patchset 18 (id:??) landed as
https://crrev.com/6571b2b68658337e301c074763383e0b5c231aea
Cr-Commit-Position: refs/heads/master@{#306612}