net: fallback to online revocation checks for EV status when CRLSet has expired.

net: fallback to online revocation checks for EV status when CRLSet has expired.

After this change our CRLSet logic is:
  * If we have a fresh CRLSet then we don't do online revocation checks unless the
    user has configured them. (It can be configured either via the settings UI,
    or with the EnableOnlineRevocationChecks policy option.)
  * If we don't have a CRLSet, or if it has expired, and we're trying EV verification,
    then we require a positive online revocation check in order to show the EV badge.
    An invalid revocation check reply will prevent the EV badge, but not hard-fail
    the whole verification.

BUG=none
TEST=net_unittests

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

[wtc, 2012-03-16 00:33:10]

Patch set 2 LGTM.

rsleevi: could you doublecheck this?  Thanks.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate.cc:611: // the online check fails, we wont show the EV
status.

Nit: wont => won't

It may be a good idea to document that we could optimize and
only enable online checks if the cert is likely to be an EV
cert, but since it is uncommon to have an expired CRLSet,
it is fine to not optimize.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate_mac.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate_mac.cc:690: bool CheckRevocationWithCRLSet(CFArrayRef
chain, CRLSet* crl_set) {

Just curious: why does this function return bool here and in
x509_certificate_win.cc, but return CRLSetResult in
x509_certificate_nss.cc?

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate_win.cc (left):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate_win.cc:501: case CRLSet::CRL_SET_EXPIRED:

Removing this case means we will hit the NOTREACHED() assertion
for CRLSet::CRL_SET_EXPIRED.  I guess this is because
we won't use an expired CRLSet?

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1398: context_->Init();

InitContext(context_) and context_->Init() sound the same to
me :-)

[wtc, 2012-03-16 00:34:13]

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate_win.cc (left):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate_win.cc:501: case CRLSet::CRL_SET_EXPIRED:
On 2012/03/16 00:33:10, wtc wrote:
> 
> Removing this case means we will hit the NOTREACHED() assertion
> for CRLSet::CRL_SET_EXPIRED.  I guess this is because
> we won't use an expired CRLSet?

Please ignore this comment.  I forgot to remove it.

[Ryan Sleevi, 2012-03-16 00:50:52]

Just a few nits - more suggestions than hard requests for changes. LGTM.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.cc#...
net/base/crl_set.cc:571: if (not_after_ == 0)
is 0 some magic value for "never expires" ? I notice it was in the original code
at line 548, but perhaps this is non-obvious, especially since not_after_ will
generally be zero-initialized when creating a new CRLSet.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.h
File net/base/crl_set.h (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.h#n...
net/base/crl_set.h:81: // CRLList contains a list of (issuer SPKI hash, revoked
serial numbers)
As a clean-up point, if these (lines 81-94) are meant only for testing, it would
be better to put them in private: and use base/gtest_prod_util.h to
FRIEND_TEST_ALL_PREFIXES the tests (or use friend class).

But this is a minor point.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate.cc:612: if ((flags & VERIFY_EV_CERT) && (!crl_set ||
crl_set->IsExpired()))
Comment nit: In past reviews, I've been dinged for using first person like "we",
since it's very easy to use passive voice here, and it's not really
we-the-people so much as this-is-the-code.

Hardly a big deal, but...

// If EV verification is requested and no CRLSet is present,
// or the supplied CRLSet has expired, enable online
// revocation checks. If the revocation checks fail, do not
// mark the certificate as EV.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1490: static_cast<bool>(cert_status &
CERT_STATUS_IS_EV));
nit: !!(cert_status & CERT_STATUS_IS_EV) ?

The static_cast<bool>() just feels like a rarer pattern in Chromium code vs the
double negation, but I'm hardly particular.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1614: class
RevCheckedDisabledSSLConfigService : public SSLConfigService {
nit: Just create a single SSLConfigService that takes the flags as parameters?

I totally don't feel strongly about that, just weird to see three
SSLConfigService-derived classes to flip two variables total.

[agl, 2012-03-20 20:02:19]

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/crl_set.cc#...
net/base/crl_set.cc:571: if (not_after_ == 0)
On 2012/03/16 00:50:52, Ryan Sleevi wrote:
> is 0 some magic value for "never expires" ? I notice it was in the original
code
> at line 548, but perhaps this is non-obvious, especially since not_after_ will
> generally be zero-initialized when creating a new CRLSet.

Yes, this is for backwards compatibility was CRLSets that were generated prior
to the addition of the NotAfter field.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate.cc:611: // the online check fails, we wont show the EV
status.
On 2012/03/16 00:33:10, wtc wrote:
> Nit: wont => won't

Done

> It may be a good idea to document that we could optimize and
> only enable online checks if the cert is likely to be an EV
> cert, but since it is uncommon to have an expired CRLSet,
> it is fine to not optimize.

Done.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate.cc:612: if ((flags & VERIFY_EV_CERT) && (!crl_set ||
crl_set->IsExpired()))
On 2012/03/16 00:50:52, Ryan Sleevi wrote:
> Comment nit: In past reviews, I've been dinged for using first person like
"we",

Have de-we'ed the comment.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
File net/base/x509_certificate_mac.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/base/x509_certif...
net/base/x509_certificate_mac.cc:690: bool CheckRevocationWithCRLSet(CFArrayRef
chain, CRLSet* crl_set) {
On 2012/03/16 00:33:10, wtc wrote:
> 
> Just curious: why does this function return bool here and in
> x509_certificate_win.cc, but return CRLSetResult in
> x509_certificate_nss.cc?

Probably a mistake that I should clean up. Originally some plans included more
granular processing in the x509_certificate_*.cc files and NSS was the first one
that I wrote. In the end, a bool was sufficient however.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1398: context_->Init();
On 2012/03/16 00:33:10, wtc wrote:
> InitContext(context_) and context_->Init() sound the same to
> me :-)

Changed to SetupContext.

https://chromiumcodereview.appspot.com/9699043/diff/2001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1614: class
RevCheckedDisabledSSLConfigService : public SSLConfigService {
On 2012/03/16 00:50:52, Ryan Sleevi wrote:
> nit: Just create a single SSLConfigService that takes the flags as parameters?

Done. Makes more sense now that there are three of them.