Mark the Certly.io log as disqualified, as of April 15 2016

Mark the Certly.io log as disqualified, as of April 15 2016

BUG=none

Committed: https://crrev.com/4db616c441f693e7eca3c7f5f7452fdfd4b84eb2
Cr-Commit-Position: refs/heads/master@{#392444}

[Ryan Sleevi, 2016-05-03 01:47:59]

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

[Ryan Sleevi, 2016-05-03 01:47:59]

Eric: This builds on the previous CL -
https://codereview.chromium.org/1941973002/

Eran: For your review / consistency checking. This is a replacement for
https://codereview.chromium.org/1888463003/ which had some comments for you that
I was awaiting feedback on.

[Eran Messeri, 2016-05-03 11:24:34]

LGTM (implements the stated policy as far as I can tell).
I'll note there are no new tests in ct_policy_enforcer_unittest :)

https://codereview.chromium.org/1941273002/diff/1/net/cert/ct_policy_enforcer.cc
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941273002/diff/1/net/cert/ct_policy_enforcer...
net/cert/ct_policy_enforcer.cc:221: base::Time unused;
Nit: Why not make the disqualification_date in IsLogDisqualified optional and
pass a nullptr here?

[eroman, 2016-05-04 22:03:13]

Unittest?

Otherwise LGTM

[Ryan Sleevi, 2016-05-04 22:26:45]

https://codereview.chromium.org/1941273002/diff/1/net/cert/ct_policy_enforcer.cc
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941273002/diff/1/net/cert/ct_policy_enforcer...
net/cert/ct_policy_enforcer.cc:221: base::Time unused;
On 2016/05/03 11:24:34, Eran Messeri wrote:
> Nit: Why not make the disqualification_date in IsLogDisqualified optional and
> pass a nullptr here?

That's less performant.

[Ryan Sleevi, 2016-05-05 00:32:36]

Now with more unittests. PTAL

[eroman, 2016-05-05 01:52:15]

lgtm thanks for the tests

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:110: sct->timestamp =
is this a multi-line if that requires curlies?

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:113: sct->timestamp =
base::Time::FromUTCExploded({2016, 4, 0, 1, 0, 0, 0, 0});
Hm, didn't realize we allowed this feature yet.

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:390: // This chain_ is valid for 10
years - over 121 months - so requires 5 SCTs.
Looks like this is the style used throughout file, but "This chain_" is funky. I
would suggest either:

 This chain is

or

  |chain_| is

[Ryan Sleevi, 2016-05-05 03:08:43]

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:110: sct->timestamp =
On 2016/05/05 01:52:15, eroman wrote:
> is this a multi-line if that requires curlies?

Peter Kasting has said no, but I think yes.

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:113: sct->timestamp =
base::Time::FromUTCExploded({2016, 4, 0, 1, 0, 0, 0, 0});
On 2016/05/05 01:52:15, eroman wrote:
> Hm, didn't realize we allowed this feature yet.

This is not an initializer list (which we do allow now), this is just C89 struct
initialization, which has always been allowed. C99 named member initialization
is not yet blessed (although it should work now that we have MSVC2015 rel 2)

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:390: // This chain_ is valid for 10
years - over 121 months - so requires 5 SCTs.
On 2016/05/05 01:52:15, eroman wrote:
> Looks like this is the style used throughout file, but "This chain_" is funky.
I
> would suggest either:
> 
>  This chain is
> 
> or
> 
>   |chain_| is

I agree, but have left it as an exercise for future cleanup.

[eroman, 2016-05-05 06:02:56]

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/1941273002/diff/40001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:113: sct->timestamp =
base::Time::FromUTCExploded({2016, 4, 0, 1, 0, 0, 0, 0});
On 2016/05/05 03:08:43, Ryan Sleevi wrote:
> On 2016/05/05 01:52:15, eroman wrote:
> > Hm, didn't realize we allowed this feature yet.
> 
> This is not an initializer list (which we do allow now), this is just C89
struct
> initialization, which has always been allowed. C99 named member initialization
> is not yet blessed (although it should work now that we have MSVC2015 rel 2)

To be clear this is not simply C89 struct initialization -- you are relying on a
C++11 feature.

But as you said, we do allow initializer lists now so this is ok. I hadn't
realized we had OK-ed that.

[Ryan Sleevi, 2016-05-09 20:45:50]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-05-09 20:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1941273002/#ps90001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-09 20:46:16]

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

[commit-bot: I haz the power, 2016-05-09 22:14:47]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-05-09 22:17:22]

Description was changed from

==========
Mark the Certly.io log as disqualified, as of April 15 2016

BUG=none
==========

to

==========
Mark the Certly.io log as disqualified, as of April 15 2016

BUG=none

Committed: https://crrev.com/4db616c441f693e7eca3c7f5f7452fdfd4b84eb2
Cr-Commit-Position: refs/heads/master@{#392444}
==========

[commit-bot: I haz the power, 2016-05-09 22:17:23]

Patchset 6 (id:??) landed as
https://crrev.com/4db616c441f693e7eca3c7f5f7452fdfd4b84eb2
Cr-Commit-Position: refs/heads/master@{#392444}

[paulmiller, 2016-05-10 00:51:01]

A revert of this CL (patchset #6 id:90001) has been created in
https://codereview.chromium.org/1956273003/ by paulmiller@chromium.org.

The reason for reverting is: This seems to be the most likely cause of this
CertVerifyProcTest failure:
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20(dbg)/....

[Will Harris, 2016-05-10 01:32:22]

this is also causing the win10 bot to fail

https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...