Use LOAD_VERIFY_EV_CERT to verify EV-ness in Verify()....

Use LOAD_VERIFY_EV_CERT to verify EV-ness in Verify().

If LOAD_VERIFY_EV_CERT is requested on load_flags
and revokation checking is performed, Verify() peforms
EV certificate verification as well, and sets
CERT_STATUS_IS_EV in verify_result.

Eliminate X509Certificate::IsEV()

BUG=3592
TEST=net_unittests with ALLOW_EXTERNAL_ACCESS=1, \
 visit https://www.thawte.com/ and shows EV info.


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

[wtc, 2009-06-18 22:31:07]

Sorry about the delay in review this CL.  It looks good,
but I'd like to suggest a change of API.  You may want to
read my comments for x509_certificate.h first, and then read
the rest of the comments.

Thanks!

http://codereview.chromium.org/125120/diff/28/30
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/125120/diff/28/30#newcode48
Line 48: // If |verify_ev_cert_enabled| is true, EV certification verification
is
Nit: certification => certificate

http://codereview.chromium.org/125120/diff/28/30#newcode49
Line 49: // performed.  It requries |rev_checking_enabled| is true, too.
Alternatively, we can ignore verify_ev_cert_enabled if
rev_checking_enabled is false.

http://codereview.chromium.org/125120/diff/28/30#newcode58
Line 58: bool rev_checking_enabled, bool verify_ev_cert_enabled,
When we have more than one 'bool' parameter, it would be
better to use one 'int flags' parameter of bit flags:
  enum {
    VERIFY_REV_CHECKING_ENABLED = 1 << 0,
    VERIFY_EV_CERT = 1 << 1,
  };

http://codereview.chromium.org/125120/diff/28/31
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/125120/diff/28/31#newcode27
Line 27: bool verify_ev_cert_enabled;  // True if EV certificate verification
enabled.
Nit: let's name this member "verify_ev_cert".

Please move this member to the end of this struct, right
below 'send_client_cert', because it should be moved to
the (not yet exist) SSLParams structure eventually.

http://codereview.chromium.org/125120/diff/28/38
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/125120/diff/28/38#newcode214
Line 214: bool verify_ev_cert_enabled,
Same as my comment for cert_verifier.h: I think we should use
an "int flags" argument of bit flags rather than two bool parameters.  This will
make the code more readable.

http://codereview.chromium.org/125120/diff/28/38#newcode219
Line 219: bool IsEV(int cert_status) const;
We should get rid of this method, because now we can just
check the CERT_STATUS_IS_EV bit in request->ssl_info().cert_status.

This was actually my original goal.  The IsEV method is an
interim solution (because when we were using WinHTTP, we
couldn't ask WinHTTP to do EV cert verification).

This will require changing resource_dispatcher_host.cc, which is outside the net
module and is the only (non-test)
caller of IsEV.

http://codereview.chromium.org/125120/diff/28/34
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/125120/diff/28/34#newcode559
Line 559: if (verify_ev_cert_enabled && VerifyEV())
Please add a TODO comment here to note that it may be possible
to combine regular cert verification and EV cert verification.
Windows CryptoAPI's certificate verification function
(CertGetCertificateChain) is very flexible and can return
independent pieces of info simultaneously.

With NSS, I believe we need to do the regular and EV
cert verifications separately.

http://codereview.chromium.org/125120/diff/28/34#newcode564
Line 564: // Verifies if the certificate is an extended-validation certificate.
I think the original comment "Returns true ..." would still work...

http://codereview.chromium.org/125120/diff/28/34#newcode566
Line 566: // The certificate has already been verified by the HTTP library. 
cert_status
The first two sentences of this paragraph should be deleted.

Then, change "performs additional checks of" to "checks".

[ukai, 2009-06-22 08:45:52]

http://codereview.chromium.org/125120/diff/28/30
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/125120/diff/28/30#newcode48
Line 48: // If |verify_ev_cert_enabled| is true, EV certification verification
is
On 2009/06/18 22:31:07, wtc wrote:
> Nit: certification => certificate

Done.

http://codereview.chromium.org/125120/diff/28/30#newcode49
Line 49: // performed.  It requries |rev_checking_enabled| is true, too.
On 2009/06/18 22:31:07, wtc wrote:
> Alternatively, we can ignore verify_ev_cert_enabled if
> rev_checking_enabled is false.

Done.

http://codereview.chromium.org/125120/diff/28/30#newcode58
Line 58: bool rev_checking_enabled, bool verify_ev_cert_enabled,
On 2009/06/18 22:31:07, wtc wrote:
> When we have more than one 'bool' parameter, it would be
> better to use one 'int flags' parameter of bit flags:
>   enum {
>     VERIFY_REV_CHECKING_ENABLED = 1 << 0,
>     VERIFY_EV_CERT = 1 << 1,
>   };

Done.

http://codereview.chromium.org/125120/diff/28/31
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/125120/diff/28/31#newcode27
Line 27: bool verify_ev_cert_enabled;  // True if EV certificate verification
enabled.
On 2009/06/18 22:31:07, wtc wrote:
> Nit: let's name this member "verify_ev_cert".
> 
> Please move this member to the end of this struct, right
> below 'send_client_cert', because it should be moved to
> the (not yet exist) SSLParams structure eventually.

Done.

http://codereview.chromium.org/125120/diff/28/38
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/125120/diff/28/38#newcode214
Line 214: bool verify_ev_cert_enabled,
On 2009/06/18 22:31:07, wtc wrote:
> Same as my comment for cert_verifier.h: I think we should use
> an "int flags" argument of bit flags rather than two bool parameters.  This
will
> make the code more readable.

Done.

http://codereview.chromium.org/125120/diff/28/38#newcode219
Line 219: bool IsEV(int cert_status) const;
On 2009/06/18 22:31:07, wtc wrote:
> We should get rid of this method, because now we can just
> check the CERT_STATUS_IS_EV bit in request->ssl_info().cert_status.
> 
> This was actually my original goal.  The IsEV method is an
> interim solution (because when we were using WinHTTP, we
> couldn't ask WinHTTP to do EV cert verification).
> 
> This will require changing resource_dispatcher_host.cc, which is outside the
net
> module and is the only (non-test)
> caller of IsEV.

Done.

http://codereview.chromium.org/125120/diff/28/34
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/125120/diff/28/34#newcode559
Line 559: if (verify_ev_cert_enabled && VerifyEV())
On 2009/06/18 22:31:07, wtc wrote:
> Please add a TODO comment here to note that it may be possible
> to combine regular cert verification and EV cert verification.
> Windows CryptoAPI's certificate verification function
> (CertGetCertificateChain) is very flexible and can return
> independent pieces of info simultaneously.
> 
> With NSS, I believe we need to do the regular and EV
> cert verifications separately.

Done.

http://codereview.chromium.org/125120/diff/28/34#newcode564
Line 564: // Verifies if the certificate is an extended-validation certificate.
On 2009/06/18 22:31:07, wtc wrote:
> I think the original comment "Returns true ..." would still work...

Done.

http://codereview.chromium.org/125120/diff/28/34#newcode566
Line 566: // The certificate has already been verified by the HTTP library. 
cert_status
On 2009/06/18 22:31:07, wtc wrote:
> The first two sentences of this paragraph should be deleted.
> 
> Then, change "performs additional checks of" to "checks".

Done.

[wtc, 2009-06-23 01:16:15]

LGTM.  Please fix the nits below before checking this in.
Thanks!

http://codereview.chromium.org/125120/diff/4022/4034
File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/125120/diff/4022/4034#oldcode931
Line 931: // EV certificate verification could be expensive.  We don't want to
spend
We should move the first two sentences of this comment to
ResourceDispatcherHost::BeginRequest above this code:

    if (request_data.resource_type == ResourceType::MAIN_FRAME)
      load_flags |= net::LOAD_VERIFY_EV_CERT;

http://codereview.chromium.org/125120/diff/4022/4034
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/125120/diff/4022/4034#newcode930
Line 930: int cert_status = request->ssl_info().cert_status;
Nit: you can just plug in request->ssl_info().cert_status
below and get rid of the local variable cert_status.

http://codereview.chromium.org/125120/diff/4022/4031
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/125120/diff/4022/4031#newcode98
Line 98: // bit OR'd of verify_flags.
Nit: change
  bit OR'd
to
  bitwise OR

http://codereview.chromium.org/125120/diff/4022/4031#newcode98
Line 98: // bit OR'd of verify_flags.
Change
  verify_flags
to
  X509Certificate::VerifyFlags

http://codereview.chromium.org/125120/diff/4022/4024
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/125120/diff/4022/4024#newcode52
Line 52: // be failed.
Change
  be failed
to
  not be performed
or
  be skipped

http://codereview.chromium.org/125120/diff/4022/4025
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/125120/diff/4022/4025#newcode42
Line 42: bool verify_ev_cert;  // True if EV certificate verification enabled.
Please change this comment to:
  True if we should verify the certificate for EV.

http://codereview.chromium.org/125120/diff/4022/4032
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/125120/diff/4022/4032#newcode215
Line 215: // |flags| is OR'd of VerifyFlags.
Please make the following changes to the comments in
cert_verifier.h as well.

Nit: change
    OR'd
to
    bitwise OR

http://codereview.chromium.org/125120/diff/4022/4032#newcode216
Line 216: // If VERIFY_REV_CHECKING_ENABLED is set on |flags|, certificate
revocation
Nit: change
  on |flags|
to
  in |flags|

(two occurrences)

http://codereview.chromium.org/125120/diff/4022/4030
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/125120/diff/4022/4030#newcode390
Line 390: bool use_ocsp = false;
It seems that we don't need to move this line.

http://codereview.chromium.org/125120/diff/4022/4030#newcode394
Line 394: if (!(flags & VERIFY_REV_CHECKING_ENABLED))
Add a comment:
  // EV requires revocation checking.

http://codereview.chromium.org/125120/diff/4022/4029
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/125120/diff/4022/4029#newcode398
Line 398: int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED|
Nit: add a space before '|', and align the two "X509...":

  int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
              X509Certificate::VERIFY_EV_CERT;

Please make the same change elsewhere in this file.

http://codereview.chromium.org/125120/diff/4022/4028
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/125120/diff/4022/4028#newcode452
Line 452: flags &= ~VERIFY_EV_CERT;
Add a comment
  // EV requiires revocation checking.

http://codereview.chromium.org/125120/diff/4022/4028#newcode566
Line 566: // This function checks of the certificatePolicies extensions of the
Nit: remove the first "of".

[ukai, 2009-06-23 06:34:05]

http://codereview.chromium.org/125120/diff/4022/4034
File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/125120/diff/4022/4034#oldcode931
Line 931: // EV certificate verification could be expensive.  We don't want to
spend
On 2009/06/23 01:16:16, wtc wrote:
> We should move the first two sentences of this comment to
> ResourceDispatcherHost::BeginRequest above this code:
> 
>     if (request_data.resource_type == ResourceType::MAIN_FRAME)
>       load_flags |= net::LOAD_VERIFY_EV_CERT;

Done.

http://codereview.chromium.org/125120/diff/4022/4034
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/125120/diff/4022/4034#newcode930
Line 930: int cert_status = request->ssl_info().cert_status;
On 2009/06/23 01:16:16, wtc wrote:
> Nit: you can just plug in request->ssl_info().cert_status
> below and get rid of the local variable cert_status.

Done.

http://codereview.chromium.org/125120/diff/4022/4031
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/125120/diff/4022/4031#newcode98
Line 98: // bit OR'd of verify_flags.
On 2009/06/23 01:16:16, wtc wrote:
> Nit: change
>   bit OR'd
> to
>   bitwise OR

Done.

http://codereview.chromium.org/125120/diff/4022/4031#newcode98
Line 98: // bit OR'd of verify_flags.
On 2009/06/23 01:16:16, wtc wrote:
> Change
>   verify_flags
> to
>   X509Certificate::VerifyFlags

Done.

http://codereview.chromium.org/125120/diff/4022/4024
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/125120/diff/4022/4024#newcode52
Line 52: // be failed.
On 2009/06/23 01:16:16, wtc wrote:
> Change
>   be failed
> to
>   not be performed
> or
>   be skipped

Done.

http://codereview.chromium.org/125120/diff/4022/4025
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/125120/diff/4022/4025#newcode42
Line 42: bool verify_ev_cert;  // True if EV certificate verification enabled.
On 2009/06/23 01:16:16, wtc wrote:
> Please change this comment to:
>   True if we should verify the certificate for EV.

Done.

http://codereview.chromium.org/125120/diff/4022/4032
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/125120/diff/4022/4032#newcode215
Line 215: // |flags| is OR'd of VerifyFlags.
On 2009/06/23 01:16:16, wtc wrote:
> Please make the following changes to the comments in
> cert_verifier.h as well.
> 
> Nit: change
>     OR'd
> to
>     bitwise OR

Done.

http://codereview.chromium.org/125120/diff/4022/4032#newcode216
Line 216: // If VERIFY_REV_CHECKING_ENABLED is set on |flags|, certificate
revocation
On 2009/06/23 01:16:16, wtc wrote:
> Nit: change
>   on |flags|
> to
>   in |flags|
> 
> (two occurrences)

Done.

http://codereview.chromium.org/125120/diff/4022/4030
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/125120/diff/4022/4030#newcode390
Line 390: bool use_ocsp = false;
On 2009/06/23 01:16:16, wtc wrote:
> It seems that we don't need to move this line.

Done.

http://codereview.chromium.org/125120/diff/4022/4030#newcode394
Line 394: if (!(flags & VERIFY_REV_CHECKING_ENABLED))
On 2009/06/23 01:16:16, wtc wrote:
> Add a comment:
>   // EV requires revocation checking.

Done.

http://codereview.chromium.org/125120/diff/4022/4029
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/125120/diff/4022/4029#newcode398
Line 398: int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED|
On 2009/06/23 01:16:16, wtc wrote:
> Nit: add a space before '|', and align the two "X509...":
> 
>   int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
>               X509Certificate::VERIFY_EV_CERT;
> 
> Please make the same change elsewhere in this file.

Done.

http://codereview.chromium.org/125120/diff/4022/4028
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/125120/diff/4022/4028#newcode452
Line 452: flags &= ~VERIFY_EV_CERT;
On 2009/06/23 01:16:16, wtc wrote:
> Add a comment
>   // EV requiires revocation checking.

Done.

http://codereview.chromium.org/125120/diff/4022/4028#newcode566
Line 566: // This function checks of the certificatePolicies extensions of the
On 2009/06/23 01:16:16, wtc wrote:
> Nit: remove the first "of".

Done.