Do not perform online revocation checking when the user has explicitly disabled it, except for when…

Do not perform online revocation checking when the user has explicitly disabled it, except for when verifying EV certificates where a CRLSet is not present or fresh.

This changes how EVRootMetaData exposes the EV information when NSS is used, in order to efficiently detect when a leaf certificate may be an EV certificate.

BUG=142815
TEST=Test modem enrollment on CrOS as described in chrome-os-partner:9087

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

[Ryan Sleevi, 2012-08-15 22:48:11]

agl: PTAL at the changes to CertVerifyProc and confirm they're in line with your
intent for CRLSets.

wtc: Please review the platform-specific changes

rkc: FYI and for testing, assuming it compiles the first time on Linux :)

[agl, 2012-08-15 22:57:52]

have to run to a meeting, but CRLSet behaviour LGTM.

http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss.cc
File net/base/cert_verify_proc_nss.cc (right):

http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss....
net/base/cert_verify_proc_nss.cc:604: bool IsEVCandidate(EVRootCAMetadata*
metadata,
Is the assumption that there'll only be one EV OID correct?

What if I issue my leaves with a pair of EV OIDs because I'm not sure which root
the client will end up with?

(Later:) looks like Windows code might already be making this assumption so I
guess it's ok.

[Ryan Sleevi, 2012-08-15 23:04:58]

http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss.cc
File net/base/cert_verify_proc_nss.cc (right):

http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss....
net/base/cert_verify_proc_nss.cc:604: bool IsEVCandidate(EVRootCAMetadata*
metadata,
On 2012/08/15 22:57:53, agl wrote:
> Is the assumption that there'll only be one EV OID correct?
> 
> What if I issue my leaves with a pair of EV OIDs because I'm not sure which
root
> the client will end up with?
> 
> (Later:) looks like Windows code might already be making this assumption so I
> guess it's ok.


That's what I'm not sure about. That's certainly the implemented behaviour on OS
X, and historically Chrome, so I'm guessing CAs are locked into this.

However, CAs don't cross-pollinate OIDs (at least, I don't think - hence the
cabfpub mail), so they should always be issuing the leaf with one EV or the
other, and just associate both roots (old and new) with either set of OIDs.

[rkc, 2012-08-15 23:10:26]

Tested it on ChromeOS, fixes the issue with the captive portal.

On 2012/08/15 23:04:58, Ryan Sleevi wrote:
>
http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss.cc
> File net/base/cert_verify_proc_nss.cc (right):
> 
>
http://codereview.chromium.org/10857020/diff/1/net/base/cert_verify_proc_nss....
> net/base/cert_verify_proc_nss.cc:604: bool IsEVCandidate(EVRootCAMetadata*
> metadata,
> On 2012/08/15 22:57:53, agl wrote:
> > Is the assumption that there'll only be one EV OID correct?
> > 
> > What if I issue my leaves with a pair of EV OIDs because I'm not sure which
> root
> > the client will end up with?
> > 
> > (Later:) looks like Windows code might already be making this assumption so
I
> > guess it's ok.
> 
> 
> That's what I'm not sure about. That's certainly the implemented behaviour on
OS
> X, and historically Chrome, so I'm guessing CAs are locked into this.
> 
> However, CAs don't cross-pollinate OIDs (at least, I don't think - hence the
> cabfpub mail), so they should always be issuing the leaf with one EV or the
> other, and just associate both roots (old and new) with either set of OIDs.

[Ryan Sleevi, 2012-08-16 01:57:45]

Ok, now (really) ready for review. I forgot the annoyances of the OS X
implementation, so I've tried to explicitly document in comments what behaviour
is actually happening here. I'll be thrilled once we can move that code to
libpkix.

[wtc, 2012-08-16 23:17:39]

Patch set 5 LGTM.

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata.cc:324: return policy_oid != SEC_OID_UNKNOWN &&

Nit: If policy_oid is SEC_OID_UNKNOWN, it cannot possibly in
policy_oids_, right?  We should be able to omit the
policy_oid != SEC_OID_UNKNOWN test.

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata.cc:363: policy_oids_.erase(oid);

Is it possible to detect that 'oid' is not in policy_oids_
and return false in that case?  The original code does not.
Not sure if that's important.

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadata.h
File net/base/ev_root_ca_metadata.h (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata.h:40: #if defined(USE_NSS) || defined(OS_WIN)

The CL's description should mention the cleanup you made to
EVRootCAMetadata.

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata_unittest.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata_unittest.cc:44: SECOidTag RegisterOID(PLArenaPool*
arena, const char* const oid_string) {

If you declare 'oid_string' as
    const char* const oid_string
it seems that you should also declare 'arena' as
    PLArenaPool* const arena

http://codereview.chromium.org/10857020/diff/7003/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/x509_certificate.h...
net/base/x509_certificate.h:95: VERIFY_REV_CHECKING_ENABLED_EV_ONLY = 1 << 3,

Nit: the name VERIFY_REV_CHECKING_ENABLED_EV_ONLY seems to imply
that it is mutually exclusive with VERIFY_REV_CHECKING_ENABLED.
But in fact it is not -- we can set both.  Would be nice to
clarify that setting both is equivalent to VERIFY_REV_CHECKING_ENABLED.

http://codereview.chromium.org/10857020/diff/7003/net/base/x509_util_mac.h
File net/base/x509_util_mac.h (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/x509_util_mac.h#ne...
net/base/x509_util_mac.h:48: // for an online OCSP response) will be permitted.
However, if the system

Nit: the system => the OS

I guess "the system" is referring to the OS.

[Ryan Sleevi, 2012-08-16 23:26:31]

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata.cc:324: return policy_oid != SEC_OID_UNKNOWN &&
On 2012/08/16 23:17:39, wtc wrote:
> 
> Nit: If policy_oid is SEC_OID_UNKNOWN, it cannot possibly in
> policy_oids_, right?  We should be able to omit the
> policy_oid != SEC_OID_UNKNOWN test.

Fair enough

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata.cc:363: policy_oids_.erase(oid);
On 2012/08/16 23:17:39, wtc wrote:
> 
> Is it possible to detect that 'oid' is not in policy_oids_
> and return false in that case?  The original code does not.
> Not sure if that's important.

If the fingerprint is found, then it must be in policy_oids_ (line 352)

This is only used by unit tests, and the only place it'd get tricky is if two
different fingerprints used the same OID set. I don't think we support that
though (which may or may not be a bug for the main code, but is fine for unit
tests)

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata_unittest.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata_unittest.cc:44: SECOidTag RegisterOID(PLArenaPool*
arena, const char* const oid_string) {
On 2012/08/16 23:17:39, wtc wrote:
> 
> If you declare 'oid_string' as
>     const char* const oid_string
> it seems that you should also declare 'arena' as
>     PLArenaPool* const arena

The "const char* const" was just to allow the compiler to keep
kVerisignPolicy/kThawtePolicy/kFakePolicy in the .data section.

If it was "const char*", GCC doesn't behave as intelligently.

[wtc, 2012-08-16 23:28:03]

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
File net/base/cert_verify_proc_mac.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
net/base/cert_verify_proc_mac.cc:160: (flags &
X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY),

We pass
  (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY)
as this argument, but this argument is named enable_ev_checking
as opposed to enable_revocation_checking_ev_only. This is a little
confusing.

I think the enable_ev_checking argument does mean "enable EV checking". Perhaps
a comment here would be useful.
(We only set VERIFY_REV_CHECKING_ENABLED_EV_ONLY if VERIFY_EV_CERT is true.)

[wtc, 2012-08-16 23:40:45]

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata_unittest.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata_unittest.cc:44: SECOidTag RegisterOID(PLArenaPool*
arena, const char* const oid_string) {

On 2012/08/16 23:26:31, Ryan Sleevi wrote:
>
> The "const char* const" was just to allow the compiler to keep
> kVerisignPolicy/kThawtePolicy/kFakePolicy in the .data section.
> 
> If it was "const char*", GCC doesn't behave as intelligently.

'oid_string' is a function argument.  Why does the type
    const char* oid_string
prevent kVerisignPolicy/kThawtePolicy/kFakePolicy from
being put in the .data section?  When we call
    RegisterOID(pool.get(), kVerisignPolicy)
the value of &kVerisignPolicy[0] is passed as the second
argument.  It should not matter whether kVerisignPolicy is
in the .data section or not.

[Ryan Sleevi, 2012-08-16 23:55:30]

wtc: Enhanced the comments. Please let me know if this addresses your nits

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
File net/base/cert_verify_proc_mac.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
net/base/cert_verify_proc_mac.cc:160: (flags &
X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY),
On 2012/08/16 23:28:03, wtc wrote:
> 
> We pass
>   (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY)
> as this argument, but this argument is named enable_ev_checking
> as opposed to enable_revocation_checking_ev_only. This is a little
> confusing.
> 
> I think the enable_ev_checking argument does mean "enable EV checking".
Perhaps
> a comment here would be useful.
> (We only set VERIFY_REV_CHECKING_ENABLED_EV_ONLY if VERIFY_EV_CERT is true.)

Just to make sure - you're asking for a comment here in addition to the comment
at the CreateRevocationPolicies?

I would rather refine the comment there if you think this is confusing.

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
File net/base/ev_root_ca_metadata_unittest.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/ev_root_ca_metadat...
net/base/ev_root_ca_metadata_unittest.cc:44: SECOidTag RegisterOID(PLArenaPool*
arena, const char* const oid_string) {
On 2012/08/16 23:40:45, wtc wrote:
> 
> On 2012/08/16 23:26:31, Ryan Sleevi wrote:
> >
> > The "const char* const" was just to allow the compiler to keep
> > kVerisignPolicy/kThawtePolicy/kFakePolicy in the .data section.
> > 
> > If it was "const char*", GCC doesn't behave as intelligently.
> 
> 'oid_string' is a function argument.  Why does the type
>     const char* oid_string
> prevent kVerisignPolicy/kThawtePolicy/kFakePolicy from
> being put in the .data section?  When we call
>     RegisterOID(pool.get(), kVerisignPolicy)
> the value of &kVerisignPolicy[0] is passed as the second
> argument.  It should not matter whether kVerisignPolicy is
> in the .data section or not.

Ah, right, I'm conditioned from thinking of it when declaring variables.
Changed, as either way it doesn't really matter for the unit test.

[wtc, 2012-08-17 00:34:44]

Patch set 6 LGTM.  Thanks.

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
File net/base/cert_verify_proc_mac.cc (right):

http://codereview.chromium.org/10857020/diff/7003/net/base/cert_verify_proc_m...
net/base/cert_verify_proc_mac.cc:160: (flags &
X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY),

On 2012/08/16 23:55:30, Ryan Sleevi wrote:
>
> Just to make sure - you're asking for a comment here in addition to the
comment
> at the CreateRevocationPolicies?

Yes, I was asking for an extra comment here, at the call
site, to explain the seemingly mismatch between
  (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED_EV_ONLY)
and
  enable_ev_checking

But it is fine to not add more comments.

http://codereview.chromium.org/10857020/diff/4007/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/10857020/diff/4007/net/base/x509_certificate.h...
net/base/x509_certificate.h:116: VERIFY_REV_CHECKING_ENABLED_EV_ONLY = 1 << 3,

Nit: the fact that this flag is tacked on by CertVerifyProc::Verify()
internally probably should be described.  Perhaps
this flag should be considered as an internal flag?

[Ryan Sleevi, 2012-08-17 00:37:18]

http://codereview.chromium.org/10857020/diff/4007/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/10857020/diff/4007/net/base/x509_certificate.h...
net/base/x509_certificate.h:116: VERIFY_REV_CHECKING_ENABLED_EV_ONLY = 1 << 3,
On 2012/08/17 00:34:44, wtc wrote:
> 
> Nit: the fact that this flag is tacked on by CertVerifyProc::Verify()
> internally probably should be described.  Perhaps
> this flag should be considered as an internal flag?

The intent is to make it not an internal-only flag; http://crbug.com/142974

[commit-bot: I haz the power, 2012-08-17 00:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10857020/4007

[commit-bot: I haz the power, 2012-08-17 00:38:10]

Presubmit check for 10857020-4007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/cocoa

Presubmit checks took 1.7s to calculate.

[Ryan Sleevi, 2012-08-17 00:53:00]

thakis: TBR the chrome/browser/ui/cocoa change, since we want to get this in a
release RSN.

[Nico, 2012-08-17 00:54:37]

Cocoa change lgtm

This would've been a good place to use enums instead of bools btw.

[commit-bot: I haz the power, 2012-08-17 01:14:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10857020/4007

[commit-bot: I haz the power, 2012-08-17 04:35:10]

Change committed as 152043