net: allow EV indication on Windows when the local OCSP/CRL cache is stale.

net: allow EV indication on Windows when the local OCSP/CRL cache is stale.

BUG=116984
TEST=On a Windows machine, run `certutil -urlcache * delete` on the command line. Then start Chrome and ensure that https://www.paypal.com shows a green EV indication in the URL bar.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125468

[agl, 2012-03-07 15:38:16]

https://chromiumcodereview.appspot.com/9619007/diff/1/net/base/x509_certifica...
File net/base/x509_certificate_unittest.cc (left):

https://chromiumcodereview.appspot.com/9619007/diff/1/net/base/x509_certifica...
net/base/x509_certificate_unittest.cc:225: #if TEST_EV
These certificates have expired and so are no longer valid for testing in any
case.

[Ryan Sleevi, 2012-03-07 16:57:01]

LGTM.

I haven't checked yet, but do we need to make similar changes for
x509_certificate_mac.cc ?

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (left):

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:225: #if TEST_EV
On 2012/03/07 15:38:16, agl wrote:
> These certificates have expired and so are no longer valid for testing in any
> case.

Yeah, I noticed the same thing as well, and made the same changes in one of my
pending CLs. Fine to see these go.

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:352: 
ASSERT_EQ(3U, certs.size()) ? (I noticed this yesterday with some tests)

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:364: "comodo.com", flags, NULL,
&verify_result);
In the past we didn't turn these on for the bots because they 'shouldn't' go out
to the network (the existing tests subtly fetch AIA, and that already can mask
issues)

With the changes you've made lately to revocation checking, do we know for sure
this won't hit the network on any platform? If so, great, otherwise I'd be a
little concerned.

http://codereview.chromium.org/9619007/diff/1/net/data/ssl/certificates/comod...
File net/data/ssl/certificates/comodo.chain.pem (right):

http://codereview.chromium.org/9619007/diff/1/net/data/ssl/certificates/comod...
net/data/ssl/certificates/comodo.chain.pem:1: -----BEGIN CERTIFICATE-----
nit: I tend to request that Wan-Teh include the openssl x509 -text output of all
the .pem certs. While it does add size to the overall source download, it makes
it a lot easier when say, a test starts failing on the waterfall / on some
platform to find out, oh, ok, this intermediate was expired.

[agl, 2012-03-07 17:31:23]

I don't believe that x509_certificate_mac.cc has a problem from looking at the
code. None the less, the tests should tell us if that isn't so.

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:352: 
On 2012/03/07 16:57:01, Ryan Sleevi wrote:
> ASSERT_EQ(3U, certs.size()) ? (I noticed this yesterday with some tests)

Done.

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:364: "comodo.com", flags, NULL,
&verify_result);
On 2012/03/07 16:57:01, Ryan Sleevi wrote:
> With the changes you've made lately to revocation checking, do we know for
sure
> this won't hit the network on any platform? If so, great, otherwise I'd be a
> little concerned.

I don't believe that this will cause any network fetches. The chain is complete
and online revocation checking is off by default. If this does end up being
flaky because of an unexpected network fetch, then that's the sort of thing that
I'd like a test to highlight.

http://codereview.chromium.org/9619007/diff/1/net/data/ssl/certificates/comod...
File net/data/ssl/certificates/comodo.chain.pem (right):

http://codereview.chromium.org/9619007/diff/1/net/data/ssl/certificates/comod...
net/data/ssl/certificates/comodo.chain.pem:1: -----BEGIN CERTIFICATE-----
On 2012/03/07 16:57:01, Ryan Sleevi wrote:
> nit: I tend to request that Wan-Teh include the openssl x509 -text output of
all
> the .pem certs. While it does add size to the overall source download, it
makes
> it a lot easier when say, a test starts failing on the waterfall / on some
> platform to find out, oh, ok, this intermediate was expired.

Done.

[commit-bot: I haz the power, 2012-03-07 19:51:55]

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

[commit-bot: I haz the power, 2012-03-07 22:04:13]

Change committed as 125468

[wtc, 2012-03-08 00:55:26]

Patch set 3 LGTM.

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
File net/base/x509_certificate.h (right):

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
net/base/x509_certificate.h:496: int flags,

Nit: it may be better to replace this argument with a
boolean 'rev_checking_enabled' argument.

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
File net/base/x509_certificate_unittest.cc (right):

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
net/base/x509_certificate_unittest.cc:366: EXPECT_EQ(OK, error);

We need to allow error to be ERR_CERT_DATE_INVALID.

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
File net/base/x509_certificate_win.cc (right):

https://chromiumcodereview.appspot.com/9619007/diff/6/net/base/x509_certifica...
net/base/x509_certificate_win.cc:951: // cause the following two error bits to
be set. Since they are expected,

It would be nice to say what those two error bits mean in
this case (the local OCSP/CRL cache is stale).

In this case, we won't show the EV badge unless we get a
fresh "good" status from either the CRL set or online
revocation checking, right?

https://chromiumcodereview.appspot.com/9619007/diff/6/net/data/ssl/certificat...
File net/data/ssl/certificates/comodo.chain.pem (right):

https://chromiumcodereview.appspot.com/9619007/diff/6/net/data/ssl/certificat...
net/data/ssl/certificates/comodo.chain.pem:1: Certificate:

Please document this file in the README file in this directory.

[wtc, 2012-03-08 01:02:19]

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:364: "comodo.com", flags, NULL,
&verify_result);

On 2012/03/07 17:31:24, agl wrote:
>
> I don't believe that this will cause any network fetches. The chain is
complete
> and online revocation checking is off by default. If this does end up being
> flaky because of an unexpected network fetch, then that's the sort of thing
that
> I'd like a test to highlight.

Hmm... if we report CERT_STATUS_IS_EV without getting a
fresh "good" status from any source -- CRL/OCSP (cached
or online) or CRL sets, then this is a step backward.

Does this unit test use the CRL set?

[agl2, 2012-03-12 16:23:31]

(Please note: I haven't ignored this! I'm working on improving the tests
first.)

On Wed Mar 07 20:02:19 GMT-500 2012, <wtc@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
> File net/base/x509_certificate_unittest.cc (right):
>
>
>
http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unitt...
>
net/base/x509_certificate_unittest.cc:364<https://www.google.com/url?sa=D&q=http://codereview.chromium.org/9619007/diff/1/net/base/x509_certificate_unittest.cc%23newcode364%0Anet/base/x509_certificate_unittest.cc:364>:
> "comodo.com <https://www.google.com/url?sa=D&q=http://comodo.com>",
> flags, NULL,
> &verify_result);
>
> On 2012/03/07 17:31:24, agl wrote:
>
> > I don't believe that this will cause any network fetches. The chain is
> complete
> > and online revocation checking is off by default. If this does end up
> being
> > flaky because of an unexpected network fetch, then that's the sort of
> thing that
> > I'd like a test to highlight.
>
> Hmm... if we report CERT_STATUS_IS_EV without getting a
> fresh "good" status from any source -- CRL/OCSP (cached
> or online) or CRL sets, then this is a step backward.
>
> Does this unit test use the CRL set?
>
>
http://codereview.chromium.org/9619007/<https://www.google.com/url?sa=D&q=htt...
>