net: reject all CloudFlare certificates issued prior to April 2nd.

net: reject all CloudFlare certificates issued prior to April 2nd.

(By request of CloudFlare.)

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268788

[wtc, 2014-05-02 21:21:16]

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

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:344: /* CloudFlare revoked all certificates issued
prior to April 2nd, 2014. Thus

We should also note when these certificates will all expire. That'll allow us to
determine when we can remove this from the blacklist.

[Ryan Sleevi, 2014-05-02 22:11:33]

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

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:348: const std::string
kCloudFlareCNSuffix(".cloudflare.com");
static const char[] kCloudFlareCNSuffix = ".cloudflare.com";

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:351: kCloudFlareCNSuffix.size(),
arraysize(kCloudFlareCNSuffix);

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:358: if (cert->valid_start() <
base::Time::FromUTCExploded(epoch)) {
Could be more efficient using FromInternalValue() here, if we wanted to treat
this as perf-critical.

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:360: }
no braces (consistent with the rest of the file for one-liners)

[agl, 2014-05-02 23:17:49]

PTAL.

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

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:344: /* CloudFlare revoked all certificates issued
prior to April 2nd, 2014. Thus
On 2014/05/02 21:21:16, wtc wrote:
> 
> We should also note when these certificates will all expire. That'll allow us
to
> determine when we can remove this from the blacklist.

Done.

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:348: const std::string
kCloudFlareCNSuffix(".cloudflare.com");
On 2014/05/02 22:11:33, Ryan Sleevi wrote:
> static const char[] kCloudFlareCNSuffix = ".cloudflare.com";

Done.

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:351: kCloudFlareCNSuffix.size(),
On 2014/05/02 22:11:33, Ryan Sleevi wrote:
> arraysize(kCloudFlareCNSuffix);

Done.

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:358: if (cert->valid_start() <
base::Time::FromUTCExploded(epoch)) {
On 2014/05/02 22:11:33, Ryan Sleevi wrote:
> Could be more efficient using FromInternalValue() here, if we wanted to treat
> this as perf-critical.

Done.

https://codereview.chromium.org/267913003/diff/1/net/cert/cert_verify_proc.cc...
net/cert/cert_verify_proc.cc:360: }
On 2014/05/02 22:11:33, Ryan Sleevi wrote:
> no braces (consistent with the rest of the file for one-liners)

Kept braces because now it's a single if with a multi-line conditional, and
omitting braces for that is a little too aggressive I feel. Don't really care
either way, however.

[Ryan Sleevi, 2014-05-03 00:08:59]

LGTM, mod one bug that will likely end up breaking the Win64 bot.

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:7: #include "base/metrics/histogram.h"
IYWU - add base/basictypes.h to the include list.

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:354: static const int64 kCloudFlareEpoch =
13040870400000000ull;
because arraysize is a constexpr, you could also "static const size_t
kCloudFlareCnSuffixLen = arraysize(kCloudFlareCNSuffix) -1", if you wanted (to
reduce the duplication of the three lines)

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:354: static const int64 kCloudFlareEpoch =
13040870400000000ull;
You declare the var as ULL, except it's an int64. 

However, this is still wrong - use INT64_C(value), which will ensure L/LL as
appropriate for LP64/LLP64

[agl, 2014-05-05 18:50:19]

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:7: #include "base/metrics/histogram.h"
On 2014/05/03 00:08:59, Ryan Sleevi wrote:
> IYWU - add base/basictypes.h to the include list.

Done.

https://codereview.chromium.org/267913003/diff/20001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:354: static const int64 kCloudFlareEpoch =
13040870400000000ull;
On 2014/05/03 00:08:59, Ryan Sleevi wrote:
> However, this is still wrong - use INT64_C(value), which will ensure L/LL as
> appropriate for LP64/LLP64

(Oh joy, another build config.)

Thanks for that.

[wtc, 2014-05-05 21:30:12]

Patch set 3 LGTM.

[agl, 2014-05-06 20:29:34]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-05-06 20:30:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/agl@chromium.org/267913003/40001

[commit-bot: I haz the power, 2014-05-07 11:18:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 15:44:06]

Change committed as 268788