Implement X509Certificate::IsEV for NSS....

Implement X509Certificate::IsEV for NSS.

Factor out PKIXVerifyCert function from Verify and share it with IsEV.

BUG=10911
TEST=EV info shown on omnibar for https://www.thawte.com/, but not on https://bugs.webkit.org/


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

[ukai, 2009-06-02 09:58:50]

This is very early stage and still needs to work, but I'd be happy if you review
I'm on the way.
I tried this code and got IsEV()==false for https://bugs.webkit.org/ and
IsEV()==true for https://www.thawte.com/

I have several questions/comments:
- X509CertificateTest in net_unittests with ALLOW_EXTERNAL_ACCESS=1 won't pass
for www.thawte.com, because it is out of date (SEC_ERROR_CERT_NOT_VALID).

- IsEV is called in browser. ResourceDispatcherHost::CompleteResponseStarted()
in chrome/browser/renderer_host/resource_dispatcher_host.cc.
  Is it ok that IsEV() will connect external resource while checking EV-ness of
a cert?

- We also need OCSP handlers that use url_request or so, instead of default
handlers?

- TabContents::GetSSLEVText in chrome/browser/tab_contents/tab_contents.{h,cc}
is defined only for OS_WIN and never used?
  CERT_STATUS_IS_EV is checked only in ToolbarModel::GetInfoText()?

- we need to implement LocationBarViewGtk to show EV-ness info like Windows. 
Are we planning to use LocationBarView for Linux as well?

[wtc, 2009-06-02 22:21:30]

On 2009/06/02 09:58:50, ukai wrote:
>
> I have several questions/comments:
> - X509CertificateTest in net_unittests with ALLOW_EXTERNAL_ACCESS=1 won't pass
> for www.thawte.com, because it is out of date (SEC_ERROR_CERT_NOT_VALID).

I guess this is because the thawte_cert in x509_certificate_unittest.cc has
expired?
Let's replace it with the current www.thawte.com cert.
 
> - IsEV is called in browser. ResourceDispatcherHost::CompleteResponseStarted()
> in chrome/browser/renderer_host/resource_dispatcher_host.cc.
>   Is it ok that IsEV() will connect external resource while checking EV-ness
of
> a cert?

Yes, this is a problem because the IO thread should not call
functions that may block indefinitely.  The ideal solution is
to combine the EV cert verification with the normal cert
verification in X509Certificate::Verify, which will be running
on a worker thread soon, and change X509Certificate::IsEV to
simply return the cached result of X509Certificate::Verify.

> - We also need OCSP handlers that use url_request or so, instead of default
> handlers?

Yes, I'm glad you asked this question.  We need to set up the OCSP
handlers properly before we formally turn on EV certificate
verification for Linux.

Since there is a lot of work to do, we should do the work in
stages and check in CLs that are work in progress.

> - TabContents::GetSSLEVText in chrome/browser/tab_contents/tab_contents.{h,cc}
> is defined only for OS_WIN and never used?

Hmm... you're right.  It is dead code now.  Could you
create a CL to remove it and ask jcampan to review?

>   CERT_STATUS_IS_EV is checked only in ToolbarModel::GetInfoText()?

No.  It's also checked in SecurityTabView::SecurityTabView().

> - we need to implement LocationBarViewGtk to show EV-ness info like Windows. 
> Are we planning to use LocationBarView for Linux as well?

I know very little about our UI.  Please ask jcampan this question
because he wrote the Windows EV UI.

[wtc, 2009-06-03 00:52:15]

On 2009/06/02 22:21:30, wtc wrote:
>
> > - TabContents::GetSSLEVText in
chrome/browser/tab_contents/tab_contents.{h,cc}
> > is defined only for OS_WIN and never used?
> 
> Hmm... you're right.  It is dead code now.  Could you
> create a CL to remove it and ask jcampan to review?

I created this CL to remove the dead code:
http://codereview.chromium.org/118145
So you don't need to create it.

[ukai, 2009-06-03 04:13:32]

On 2009/06/02 22:21:30, wtc wrote:
> On 2009/06/02 09:58:50, ukai wrote:
> >
> > I have several questions/comments:
> > - X509CertificateTest in net_unittests with ALLOW_EXTERNAL_ACCESS=1 won't
pass
> > for www.thawte.com, because it is out of date (SEC_ERROR_CERT_NOT_VALID).
> 
> I guess this is because the thawte_cert in x509_certificate_unittest.cc has
> expired?
> Let's replace it with the current www.thawte.com cert.

Yes.  I'll send a change to update it.

>  
> > - IsEV is called in browser.
ResourceDispatcherHost::CompleteResponseStarted()
> > in chrome/browser/renderer_host/resource_dispatcher_host.cc.
> >   Is it ok that IsEV() will connect external resource while checking EV-ness
> of
> > a cert?
> 
> Yes, this is a problem because the IO thread should not call
> functions that may block indefinitely.  The ideal solution is
> to combine the EV cert verification with the normal cert
> verification in X509Certificate::Verify, which will be running
> on a worker thread soon, and change X509Certificate::IsEV to
> simply return the cached result of X509Certificate::Verify.

That sounds reasonable.
According to comment where IsEV call, one concern is that it might be expensive
to check EV-ness for all? 
In ResourceDispatcherHost::CompleteResponseStarted(), IsEV is called only for
ResourceType::MAIN_FRAME and we don't know resource type in network layer.
How should we address it?  pass resource_type to network layer?

> 
> > - We also need OCSP handlers that use url_request or so, instead of default
> > handlers?
> 
> Yes, I'm glad you asked this question.  We need to set up the OCSP
> handlers properly before we formally turn on EV certificate
> verification for Linux.
> 
> Since there is a lot of work to do, we should do the work in
> stages and check in CLs that are work in progress.

Ok.

> >   CERT_STATUS_IS_EV is checked only in ToolbarModel::GetInfoText()?
> 
> No.  It's also checked in SecurityTabView::SecurityTabView().

Oops. Sorry, i missed that.

> > - we need to implement LocationBarViewGtk to show EV-ness info like Windows.

> > Are we planning to use LocationBarView for Linux as well?
> 
> I know very little about our UI.  Please ask jcampan this question
> because he wrote the Windows EV UI.

Ok. I'll ask him.

[ukai, 2009-06-03 04:13:43]

On 2009/06/03 00:52:15, wtc wrote:
> On 2009/06/02 22:21:30, wtc wrote:
> >
> > > - TabContents::GetSSLEVText in
> chrome/browser/tab_contents/tab_contents.{h,cc}
> > > is defined only for OS_WIN and never used?
> > 
> > Hmm... you're right.  It is dead code now.  Could you
> > create a CL to remove it and ask jcampan to review?
> 
> I created this CL to remove the dead code:
> http://codereview.chromium.org/118145
> So you don't need to create it.

Thanks!

[wtc, 2009-06-04 01:49:00]

On 2009/06/03 04:13:32, ukai wrote:
>
> According to comment where IsEV call, one concern is that it might be
expensive
> to check EV-ness for all? 

Yes, but I wrote that comment when we were still using WinHTTP.
Since WinHTTP verifies certs for us, and it does a normal cert
verification, EV cert verification is an additional verification.

In the current code, we should be able to eliminate the normal
cert verification and just do EV cert verification.  This reduces
the number of verifications to one.  Perhaps EV cert verification
is still more expensive than normal cert verification, but it
shouldn't be too much.

Also, you may have noticed that we verify the same cert again
and again.  We can also cache cert verification results, which
will further reduce the total number of verifications.

> In ResourceDispatcherHost::CompleteResponseStarted(), IsEV is called only for
> ResourceType::MAIN_FRAME and we don't know resource type in network layer.
> How should we address it?  pass resource_type to network layer?

We should be able to improve cert verification performance a lot
without passing resource type to the network layer.  But I think
at some point we may need to pass resource type...  One concern
is that we design the network layer to be general, but resource
type is a notion specific to a web browser.  We recently had to
assign different priorities to requests for different resource
types, and our solution was to pass a priority level rather than
resource type to the network layer.  So we could do a similar
thing and add a load flags (load_flags.h) for request that
requires EV cert verification or perhaps to indicate it's a
main frame.  (We already have a LOAD_IS_DOWNLOAD flag.)

[ukai, 2009-06-04 09:01:12]

On 2009/06/04 01:49:00, wtc wrote:
> On 2009/06/03 04:13:32, ukai wrote:
> >
> > According to comment where IsEV call, one concern is that it might be
> expensive
> > to check EV-ness for all? 
> 
> Yes, but I wrote that comment when we were still using WinHTTP.
> Since WinHTTP verifies certs for us, and it does a normal cert
> verification, EV cert verification is an additional verification.
> 
> In the current code, we should be able to eliminate the normal
> cert verification and just do EV cert verification.  This reduces
> the number of verifications to one.  Perhaps EV cert verification
> is still more expensive than normal cert verification, but it
> shouldn't be too much.
> 
> Also, you may have noticed that we verify the same cert again
> and again.  We can also cache cert verification results, which
> will further reduce the total number of verifications.

Ok. Then, we'll refactor X509Certificate as
 Verify() verifies the cert with checking EV-ness and cahces the result.
 IsEV() returns the cached result.

May I do this before this change or after this change?

> 
> > In ResourceDispatcherHost::CompleteResponseStarted(), IsEV is called only
for
> > ResourceType::MAIN_FRAME and we don't know resource type in network layer.
> > How should we address it?  pass resource_type to network layer?
> 
> We should be able to improve cert verification performance a lot
> without passing resource type to the network layer.  But I think
> at some point we may need to pass resource type...  One concern
> is that we design the network layer to be general, but resource
> type is a notion specific to a web browser.  We recently had to
> assign different priorities to requests for different resource
> types, and our solution was to pass a priority level rather than
> resource type to the network layer.  So we could do a similar
> thing and add a load flags (load_flags.h) for request that
> requires EV cert verification or perhaps to indicate it's a
> main frame.  (We already have a LOAD_IS_DOWNLOAD flag.)

Sounds reasonable. May I add LOAD_CHECK_EV_CERT or so?

[wtc, 2009-06-04 17:17:09]

On 2009/06/04 09:01:12, ukai wrote:
>
> Ok. Then, we'll refactor X509Certificate as
>  Verify() verifies the cert with checking EV-ness and cahces the result.
>  IsEV() returns the cached result.

Correct.

> May I do this before this change or after this change?

You can certainly do that after this change.  It is best to
make changes in smaller changelists so that we can make
steady progress and the changelists are easier to review.

> Sounds reasonable. May I add LOAD_CHECK_EV_CERT or so?

Yes, adding a load flag (let's name it LOAD_VERIFY_EV_CERT)
is a good way to pass the info to the network stack.  Please ask
Darin to review it when he gets back to work next week if we
should just pass the resource type to the network stack
instead.

[ukai, 2009-06-08 07:31:41]

On 2009/06/04 17:17:09, wtc wrote:
> On 2009/06/04 09:01:12, ukai wrote:
> >
> > Ok. Then, we'll refactor X509Certificate as
> >  Verify() verifies the cert with checking EV-ness and cahces the result.
> >  IsEV() returns the cached result.
> 
> Correct.
> 
> > May I do this before this change or after this change?
> 
> You can certainly do that after this change.  It is best to
> make changes in smaller changelists so that we can make
> steady progress and the changelists are easier to review.

Ok, so what should I do in this change?
Should I work on OCSP handlers before it?

> > Sounds reasonable. May I add LOAD_CHECK_EV_CERT or so?
> 
> Yes, adding a load flag (let's name it LOAD_VERIFY_EV_CERT)
> is a good way to pass the info to the network stack.  Please ask
> Darin to review it when he gets back to work next week if we
> should just pass the resource type to the network stack
> instead.

I see.  I send the CL for review.

[wtc, 2009-06-09 00:10:13]

On 2009/06/08 07:31:41, ukai wrote:
>
> Ok, so what should I do in this change?
> Should I work on OCSP handlers before it?

Thank you for asking.  I actually thought about this issue over
the weekend and wanted to talk to you about it.  Unfortunately
I forgot what my conclusions were :-)

I just thought about this again.  I suggest that you work on
OCSP handlers first because OCSP handlers seem more challenging.
It's usually good to tackle the more challenging items first, so
that you're not surprised by them at the last minute.

[ukai, 2009-06-10 09:34:36]

On 2009/06/09 00:10:13, wtc wrote:
> On 2009/06/08 07:31:41, ukai wrote:
> >
> > Ok, so what should I do in this change?
> > Should I work on OCSP handlers before it?
> 
> Thank you for asking.  I actually thought about this issue over
> the weekend and wanted to talk to you about it.  Unfortunately
> I forgot what my conclusions were :-)
> 
> I just thought about this again.  I suggest that you work on
> OCSP handlers first because OCSP handlers seem more challenging.
> It's usually good to tackle the more challenging items first, so
> that you're not surprised by them at the last minute.

Ok, I'll work on OSCP handlers first.

[ukai, 2009-07-31 09:00:11]

On 2009/06/10 09:34:36, ukai wrote:
> On 2009/06/09 00:10:13, wtc wrote:
> > On 2009/06/08 07:31:41, ukai wrote:
> > >
> > > Ok, so what should I do in this change?
> > > Should I work on OCSP handlers before it?
> > 
> > Thank you for asking.  I actually thought about this issue over
> > the weekend and wanted to talk to you about it.  Unfortunately
> > I forgot what my conclusions were :-)
> > 
> > I just thought about this again.  I suggest that you work on
> > OCSP handlers first because OCSP handlers seem more challenging.
> > It's usually good to tackle the more challenging items first, so
> > that you're not surprised by them at the last minute.
> 
> Ok, I'll work on OSCP handlers first.

OCSP handlders done.
Could you take a look at this, please?

[wtc, 2009-08-05 00:44:50]

I made a pass through this CL.  It looks good.  But
I'd like to read it again tomorrow because I'm a little
tired now...

http://codereview.chromium.org/119026/diff/5001/5002
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/5001/5002#newcode322
Line 322: SECOidTag* GetPolicyOidsTag(net::EVRootCAMetadata* metadata) {
Nit: this function should be named GetPolicyOidTags.

http://codereview.chromium.org/119026/diff/5001/5002#newcode327
Line 327: SECItem sec_item;
Nit: name this variable 'oid' or 'oid_item'.

http://codereview.chromium.org/119026/diff/5001/5002#newcode330
Line 330: SEC_StringToOID(NULL, &sec_item, policy_oids[i],
strlen(policy_oids[i]));
Check the return value of SEC_StringToOID.

You can pass 0 instead of strlen(policy_oids[i]) as the
fourth argument.

http://codereview.chromium.org/119026/diff/5001/5002#newcode353
Line 353: bool use_ocsp,
Now that we have implemented OCSP, do we still need the
use_ocsp parameter?

http://codereview.chromium.org/119026/diff/5001/5002#newcode422
Line 422: X509Certificate::Fingerprint& fingerprint,
Add 'const'?

http://codereview.chromium.org/119026/diff/5001/5002#newcode432
Line 432: SECItem sec_item;
Same comments above apply to the 'sec_item' variable and
the SEC_StringToOID call here.

http://codereview.chromium.org/119026/diff/5001/5002#newcode435
Line 435: SEC_StringToOID(NULL, &sec_item, ev_policy_oid.c_str(),
Here we could use ev_policy_oid.data() to avoid the
conversion to a C string.  This would require passing
ev_policy_oid.length() as the fourth argument.

http://codereview.chromium.org/119026/diff/5001/5002#newcode442
Line 442: const SECOidTag& ev_policy_tag) {
Nit: SECOidTag is an enum type, so we don't need to pass a
const reference here.  Just pass 'SECOidTag ev_policy_tag'.

http://codereview.chromium.org/119026/diff/5001/5002#newcode530
Line 530: // TODO(ukai): use flags to set use_ocsp.
I don't quite understand this comment...

http://codereview.chromium.org/119026/diff/5001/5002#newcode534
Line 534: // EV requires revocation checking.
These three lines (534-536) are now dead code.  They should
be deleted.

http://codereview.chromium.org/119026/diff/5001/5002#newcode570
Line 570: if (VerifyEV()) {
Nit: no need to use braces {} here.

[ukai, 2009-08-05 06:05:30]

Thanks for review.
addressed all of your comments.

On 2009/08/05 00:44:50, wtc wrote:
> I made a pass through this CL.  It looks good.  But
> I'd like to read it again tomorrow because I'm a little
> tired now...
> 
> http://codereview.chromium.org/119026/diff/5001/5002
> File net/base/x509_certificate_nss.cc (right):
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode322
> Line 322: SECOidTag* GetPolicyOidsTag(net::EVRootCAMetadata* metadata) {
> Nit: this function should be named GetPolicyOidTags.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode327
> Line 327: SECItem sec_item;
> Nit: name this variable 'oid' or 'oid_item'.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode330
> Line 330: SEC_StringToOID(NULL, &sec_item, policy_oids[i],
> strlen(policy_oids[i]));
> Check the return value of SEC_StringToOID.
> 
> You can pass 0 instead of strlen(policy_oids[i]) as the
> fourth argument.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode353
> Line 353: bool use_ocsp,
> Now that we have implemented OCSP, do we still need the
> use_ocsp parameter?
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode422
> Line 422: X509Certificate::Fingerprint& fingerprint,
> Add 'const'?
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode432
> Line 432: SECItem sec_item;
> Same comments above apply to the 'sec_item' variable and
> the SEC_StringToOID call here.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode435
> Line 435: SEC_StringToOID(NULL, &sec_item, ev_policy_oid.c_str(),
> Here we could use ev_policy_oid.data() to avoid the
> conversion to a C string.  This would require passing
> ev_policy_oid.length() as the fourth argument.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode442
> Line 442: const SECOidTag& ev_policy_tag) {
> Nit: SECOidTag is an enum type, so we don't need to pass a
> const reference here.  Just pass 'SECOidTag ev_policy_tag'.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode530
> Line 530: // TODO(ukai): use flags to set use_ocsp.
> I don't quite understand this comment...
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode534
> Line 534: // EV requires revocation checking.
> These three lines (534-536) are now dead code.  They should
> be deleted.
> 
> http://codereview.chromium.org/119026/diff/5001/5002#newcode570
> Line 570: if (VerifyEV()) {
> Nit: no need to use braces {} here.

[wtc, 2009-08-06 01:54:38]

Sorry, I forgot to read this CL again today.  I will do
it first thing tomorrow morning.

I reviewed the changes you made yesterday.  I have two
comments below.

http://codereview.chromium.org/119026/diff/6003/5004
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/6003/5004#newcode322
Line 322: SECOidTag* GetPolicyOidTag(net::EVRootCAMetadata* metadata) {
Nit: GetPolicyOidTag => GetPolicyOidTags, with 's' at the end.

http://codereview.chromium.org/119026/diff/6003/5004#newcode335
Line 335: policies[i] = SECOID_FindOIDTag(&oid_item);
If we continue above, policies[i] won't be set for that 'i'.
So we need to use another index, 'j', for the 'policies' array.

Also, it seems that we're reading policies[i] without
initializing it here...

[ukai, 2009-08-06 04:26:34]

On 2009/08/06 01:54:38, wtc wrote:
> Sorry, I forgot to read this CL again today.  I will do
> it first thing tomorrow morning.
> 
> I reviewed the changes you made yesterday.  I have two
> comments below.

Thanks for review.
Fixed function name and make it use std::vector instead of scoped_array.
> 
> http://codereview.chromium.org/119026/diff/6003/5004
> File net/base/x509_certificate_nss.cc (right):
> 
> http://codereview.chromium.org/119026/diff/6003/5004#newcode322
> Line 322: SECOidTag* GetPolicyOidTag(net::EVRootCAMetadata* metadata) {
> Nit: GetPolicyOidTag => GetPolicyOidTags, with 's' at the end.
> 
> http://codereview.chromium.org/119026/diff/6003/5004#newcode335
> Line 335: policies[i] = SECOID_FindOIDTag(&oid_item);
> If we continue above, policies[i] won't be set for that 'i'.
> So we need to use another index, 'j', for the 'policies' array.
> 
> Also, it seems that we're reading policies[i] without
> initializing it here...

[wtc, 2009-08-06 20:53:28]

LGTM.  Very nice work!  This has been a long journey.

Please make as many of my suggested changes below
as you can, but feel free to add TODO comments for the
suggested changes that require more work (such as adding
Linux-only GetPolicyOIDTag/GetPolicyOIDTags methods to the
EVRootCAMetadata class).

Two general comments first.

1. If you copied Mozilla code, please document which
functions from which files you copied, and include
Mozilla's license header.  If you just studied Mozilla
code to learn what functions you need to use and then
wrote the code yourself, let's still acknowledge that
we consulted Mozilla code (but don't include Mozilla's
license header in this case).

2. I recommend studying the mozilla-1.9.1 source tree
(http://mxr.mozilla.org/mozilla1.9.1/).  This is the
source code of Firefox 3.5.x, so it is more trustworthy
than the Mozilla trunk (called "mozilla-central").

Also, I need to find out if we need to call these
OCSP-related NSS functions:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/vfychain...

http://mxr.mozilla.org/mozilla1.9.1/source/security/manager/ssl/src/nsNSSComp...

http://mxr.mozilla.org/security/ident?i=CERT_EnableOCSPChecking
http://mxr.mozilla.org/security/ident?i=CERT_DisableOCSPDefaultResponder
http://mxr.mozilla.org/security/ident?i=CERT_SetOCSPFailureMode

http://codereview.chromium.org/119026/diff/8001/7002
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/8001/7002#newcode332
Line 332: LOG(ERROR) << "Failed to convert to OID:" << policy_oids[i];
Nit: add a space after the colon (:).

http://codereview.chromium.org/119026/diff/8001/7002#newcode335
Line 335: SECOidTag policy = SECOID_FindOIDTag(&oid_item);
We shouldn't call SECOID_FindOIDTag here.  The problem is
that we are calling GetPolicyOidTags every time we do EV
verification.

We should call GetPolicyOidTags only once to initialize
the policy OID tag array.  Here is one possible solution:
make GetPolicyOidTags a Linux-only method of the
EVRootCAMetadata class, with this prototype:

#if defined(OS_LINUX)
  const SECOidTag* GetPolicyOIDTags() const;
#endif

http://codereview.chromium.org/119026/diff/8001/7002#newcode337
Line 337: SECOidData od;
Let's add a comment here:
  // Register the OID.

Alternatively, you can copy Mozilla's register_oid function.

http://codereview.chromium.org/119026/diff/8001/7002#newcode344
Line 344: SECOID_AddEntry(&od);
SECOID_AddEntry returns the OID tag.  We should use that
rather than calling SECOID_FindOIDTag to find it.
  policy = SECOID_AddEntry(&od);

http://codereview.chromium.org/119026/diff/8001/7002#newcode355
Line 355: // If metadata is not NULL, policies are also checked.
It may be better to use a policy_oids parameter instead of
metadata:
  const SECOidTag* policy_oids,
  int num_policy_oids,

http://codereview.chromium.org/119026/diff/8001/7002#newcode360
Line 360: CERTRevocationFlags revocation_flags;
Move this declaration down to line 379 so that it is next
to where we use this variable.

http://codereview.chromium.org/119026/diff/8001/7002#newcode374
Line 374: int number_of_defined_methods;
Delete number_of_defined_methods.  Let's just use
arraysize(method_flags) below.

http://codereview.chromium.org/119026/diff/8001/7002#newcode386
Line 386: revocation_method_independent_flags;
Add a blank line here to separate leafTests from chainTests.

http://codereview.chromium.org/119026/diff/8001/7002#newcode398
Line 398: // We can't use PK11_ListCerts(PK11CertListCA, NULL) for
cert_pi_trustAnchors.
We can delete lines 398-400 of this comment.  It's enough to
just say "No need to set cert_pi_trustAnchors here."

http://codereview.chromium.org/119026/diff/8001/7002#newcode420
Line 420: bool GetEvPolicyOidTag(net::EVRootCAMetadata* metadata,
I propose that we make this a Linux-only method of the
EVRootCAMetadata class.  This way we can implement it more
efficiently.

  #if defined(OS_LINUX)
  bool GetPolidyOIDTag(const X509Certificate::Fingerprint& fingerprint,
                       SECOidTag* policy_oid) const;
  #endif

We can use a std::map from certificate fingerprint to
SECOidTag.

In fact, for Linux, we don't need the GetPolicyOID and
GetPolicyOIDs methods.  Linux only needs the OIDTag version
of those two methods.

http://codereview.chromium.org/119026/diff/8001/7002#newcode430
Line 430: PRUint8 buf[124];
This should be 1024.

http://codereview.chromium.org/119026/diff/8001/7002#newcode446
Line 446: SECItem ext_policy;
Nit: let's name this variable policy_ext for "policy extension".

http://codereview.chromium.org/119026/diff/8001/7002#newcode450
Line 450: LOG(ERROR) << "Cert has no extensions.";
This error message should say "no policies extension" instead
of "no extensions".

http://codereview.chromium.org/119026/diff/8001/7002#newcode456
Line 456: LOG(ERROR) << "Failed to get certificate policy.";
Nit: get => decode

http://codereview.chromium.org/119026/diff/8001/7002#newcode462
Line 462: SECOidTag oid_tag = policy_info->oid;
Mozilla code does
  SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
here.  Are you sure we can just use policy_info->oid and
don't need to call SECOID_FindOIDTag?

http://codereview.chromium.org/119026/diff/8001/7002#newcode535
Line 535: cvout[cvout_index].type = cert_po_trustAnchor;
We don't need the trust anchor for the first PKIXVerifyCert
call.

http://codereview.chromium.org/119026/diff/8001/7002#newcode564
Line 564: verify_result->cert_status |= net::CERT_STATUS_REV_CHECKING_ENABLED;
We should do this before the first PKIXVerifyCert call on
line 545 above because we always do revocation checking right
now.

Once this line is moved up, the origial code is actually OK.

http://codereview.chromium.org/119026/diff/8001/7002#newcode572
Line 572: bool X509Certificate::VerifyEV() const {
Please add a TODO(wtc) comment here:
  We may be able to request cert_po_policyOID and just
  check if any of the returned policies is the EV policy
  of the trust anchor.

Another possible optimization is that we get the trust
anchor from the first PKIXVerifyCert call.  We look up the
EV policy for the trust anchor.  If the trust anchor has no
EV policy, we know the cert isn't EV.  Otherwise, we pass
just that EV policy (as opposed to all the EV policies) to
the second PKIXVerifyCert call.

You can also put the above in the TODO comment.

http://codereview.chromium.org/119026/diff/8001/7002#newcode581
Line 581: cvout[cvout_index].type = cert_po_certList;
Delete this entry because we don't need the cert chain.

http://codereview.chromium.org/119026/diff/8001/7002#newcode591
Line 591: CERTCertificate* issuer_cert =
Let's call this root_ca or trust_anchor.

[ukai, 2009-08-07 03:44:08]

Thanks for review!

I've addressed your comments, and added TODOs for functions that should be part
of EVRootCAMetadata.

On 2009/08/06 20:53:28, wtc wrote:
> LGTM.  Very nice work!  This has been a long journey.
> 
> Please make as many of my suggested changes below
> as you can, but feel free to add TODO comments for the
> suggested changes that require more work (such as adding
> Linux-only GetPolicyOIDTag/GetPolicyOIDTags methods to the
> EVRootCAMetadata class).
> 
> Two general comments first.
> 
> 1. If you copied Mozilla code, please document which
> functions from which files you copied, and include
> Mozilla's license header.  If you just studied Mozilla
> code to learn what functions you need to use and then
> wrote the code yourself, let's still acknowledge that
> we consulted Mozilla code (but don't include Mozilla's
> license header in this case).
> 
> 2. I recommend studying the mozilla-1.9.1 source tree
> (http://mxr.mozilla.org/mozilla1.9.1/).  This is the
> source code of Firefox 3.5.x, so it is more trustworthy
> than the Mozilla trunk (called "mozilla-central").
> 
> Also, I need to find out if we need to call these
> OCSP-related NSS functions:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/vfychain...
> 
>
http://mxr.mozilla.org/mozilla1.9.1/source/security/manager/ssl/src/nsNSSComp...
> 
> http://mxr.mozilla.org/security/ident?i=CERT_EnableOCSPChecking
> http://mxr.mozilla.org/security/ident?i=CERT_DisableOCSPDefaultResponder
> http://mxr.mozilla.org/security/ident?i=CERT_SetOCSPFailureMode
> 
> http://codereview.chromium.org/119026/diff/8001/7002
> File net/base/x509_certificate_nss.cc (right):
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode332
> Line 332: LOG(ERROR) << "Failed to convert to OID:" << policy_oids[i];
> Nit: add a space after the colon (:).
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode335
> Line 335: SECOidTag policy = SECOID_FindOIDTag(&oid_item);
> We shouldn't call SECOID_FindOIDTag here.  The problem is
> that we are calling GetPolicyOidTags every time we do EV
> verification.
> 
> We should call GetPolicyOidTags only once to initialize
> the policy OID tag array.  Here is one possible solution:
> make GetPolicyOidTags a Linux-only method of the
> EVRootCAMetadata class, with this prototype:
> 
> #if defined(OS_LINUX)
>   const SECOidTag* GetPolicyOIDTags() const;
> #endif
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode337
> Line 337: SECOidData od;
> Let's add a comment here:
>   // Register the OID.
> 
> Alternatively, you can copy Mozilla's register_oid function.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode344
> Line 344: SECOID_AddEntry(&od);
> SECOID_AddEntry returns the OID tag.  We should use that
> rather than calling SECOID_FindOIDTag to find it.
>   policy = SECOID_AddEntry(&od);
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode355
> Line 355: // If metadata is not NULL, policies are also checked.
> It may be better to use a policy_oids parameter instead of
> metadata:
>   const SECOidTag* policy_oids,
>   int num_policy_oids,
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode360
> Line 360: CERTRevocationFlags revocation_flags;
> Move this declaration down to line 379 so that it is next
> to where we use this variable.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode374
> Line 374: int number_of_defined_methods;
> Delete number_of_defined_methods.  Let's just use
> arraysize(method_flags) below.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode386
> Line 386: revocation_method_independent_flags;
> Add a blank line here to separate leafTests from chainTests.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode398
> Line 398: // We can't use PK11_ListCerts(PK11CertListCA, NULL) for
> cert_pi_trustAnchors.
> We can delete lines 398-400 of this comment.  It's enough to
> just say "No need to set cert_pi_trustAnchors here."
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode420
> Line 420: bool GetEvPolicyOidTag(net::EVRootCAMetadata* metadata,
> I propose that we make this a Linux-only method of the
> EVRootCAMetadata class.  This way we can implement it more
> efficiently.
> 
>   #if defined(OS_LINUX)
>   bool GetPolidyOIDTag(const X509Certificate::Fingerprint& fingerprint,
>                        SECOidTag* policy_oid) const;
>   #endif
> 
> We can use a std::map from certificate fingerprint to
> SECOidTag.
> 
> In fact, for Linux, we don't need the GetPolicyOID and
> GetPolicyOIDs methods.  Linux only needs the OIDTag version
> of those two methods.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode430
> Line 430: PRUint8 buf[124];
> This should be 1024.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode446
> Line 446: SECItem ext_policy;
> Nit: let's name this variable policy_ext for "policy extension".
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode450
> Line 450: LOG(ERROR) << "Cert has no extensions.";
> This error message should say "no policies extension" instead
> of "no extensions".
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode456
> Line 456: LOG(ERROR) << "Failed to get certificate policy.";
> Nit: get => decode
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode462
> Line 462: SECOidTag oid_tag = policy_info->oid;
> Mozilla code does
>   SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
> here.  Are you sure we can just use policy_info->oid and
> don't need to call SECOID_FindOIDTag?
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode535
> Line 535: cvout[cvout_index].type = cert_po_trustAnchor;
> We don't need the trust anchor for the first PKIXVerifyCert
> call.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode564
> Line 564: verify_result->cert_status |= net::CERT_STATUS_REV_CHECKING_ENABLED;
> We should do this before the first PKIXVerifyCert call on
> line 545 above because we always do revocation checking right
> now.
> 
> Once this line is moved up, the origial code is actually OK.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode572
> Line 572: bool X509Certificate::VerifyEV() const {
> Please add a TODO(wtc) comment here:
>   We may be able to request cert_po_policyOID and just
>   check if any of the returned policies is the EV policy
>   of the trust anchor.
> 
> Another possible optimization is that we get the trust
> anchor from the first PKIXVerifyCert call.  We look up the
> EV policy for the trust anchor.  If the trust anchor has no
> EV policy, we know the cert isn't EV.  Otherwise, we pass
> just that EV policy (as opposed to all the EV policies) to
> the second PKIXVerifyCert call.
> 
> You can also put the above in the TODO comment.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode581
> Line 581: cvout[cvout_index].type = cert_po_certList;
> Delete this entry because we don't need the cert chain.
> 
> http://codereview.chromium.org/119026/diff/8001/7002#newcode591
> Line 591: CERTCertificate* issuer_cert =
> Let's call this root_ca or trust_anchor.

[wtc, 2009-08-07 03:54:28]

LGTM.

[wtc, 2009-08-07 04:01:19]

Just some nits...

http://codereview.chromium.org/119026/diff/7004/7005
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/7004/7005#newcode322
Line 322: // TODO(ukai): make a Linux-only method of the EVRootCAMetadata.
Nit: make => this should be

Add "class" at the end, or remove "the".

http://codereview.chromium.org/119026/diff/7004/7005#newcode356
Line 356: // If metadata is not NULL, policies are also checked.
This sentence needs to be updated because 'metadata'
has been replaced by policy_oids and num_policy_oids.

http://codereview.chromium.org/119026/diff/7004/7005#newcode463
Line 463: SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
Does it work to just use policy_info->oid?  Mozilla code
is not always right or the best.

http://codereview.chromium.org/119026/diff/7004/7005#newcode563
Line 563: if (flags & VERIFY_EV_CERT) {
Nit: use the original form:

  if ((flags & VERIFY_EV_CERT) && VerifyEV())
    verify_result->cert_status |= CERT_STATUS_IS_EV;

http://codereview.chromium.org/119026/diff/7004/7005#newcode570
Line 570: // Studied Mozilla's code (esp.
security/manager/ssl/src/nsNSSCertHelper.cpp)
Isn't nsIdentityChecking.cpp the most useful file for EV
verification?

[ukai, 2009-08-07 04:07:29]

On 2009/08/07 04:01:19, wtc wrote:
> Just some nits...

Oops. Landed before seeing this comments.
I'll fix them in another CL.

> 
> http://codereview.chromium.org/119026/diff/7004/7005
> File net/base/x509_certificate_nss.cc (right):
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode322
> Line 322: // TODO(ukai): make a Linux-only method of the EVRootCAMetadata.
> Nit: make => this should be
> 
> Add "class" at the end, or remove "the".
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode356
> Line 356: // If metadata is not NULL, policies are also checked.
> This sentence needs to be updated because 'metadata'
> has been replaced by policy_oids and num_policy_oids.
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode463
> Line 463: SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
> Does it work to just use policy_info->oid?  Mozilla code
> is not always right or the best.
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode563
> Line 563: if (flags & VERIFY_EV_CERT) {
> Nit: use the original form:
> 
>   if ((flags & VERIFY_EV_CERT) && VerifyEV())
>     verify_result->cert_status |= CERT_STATUS_IS_EV;
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode570
> Line 570: // Studied Mozilla's code (esp.
> security/manager/ssl/src/nsNSSCertHelper.cpp)
> Isn't nsIdentityChecking.cpp the most useful file for EV
> verification?

[wtc, 2009-08-07 04:10:04]

On 2009/08/07 04:07:29, ukai wrote:
>
> Oops. Landed before seeing this comments.
> I'll fix them in another CL.

No problem.  Because of our timezone difference, you can check
in the new CL with a TBR.

[alv, 2009-08-08 00:47:15]

http://codereview.chromium.org/119026/diff/7004/7005
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/7004/7005#newcode365
Line 365: CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE |
if you plan to allow user to use default source then you will need to call
CERT_EnableOCSPChecking and then call CERT_SetOCSPDefaultResponder to set the
source. Otherwise I would just set CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE.

http://codereview.chromium.org/119026/diff/7004/7005#newcode367
Line 367: CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;
I would also list here the defalt CERT_REV_M_IGNORE_MISSING_FRESH_INFO flag
since it would improve understanding of the code.

http://codereview.chromium.org/119026/diff/7004/7005#newcode421
Line 421: bool GetEvPolicyOidTag(net::EVRootCAMetadata* metadata,
Seems like this method and GetPolicyOidTags belong to EVRootCAMetadata object.
Same for policy oid tags: this way you will do string conversion only ones when
it was not yet converted, and use existing tag in all other cases.

http://codereview.chromium.org/119026/diff/7004/7005#newcode454
Line 454: CERTCertificatePolicies* policies =
Does this function leak policies?

http://codereview.chromium.org/119026/diff/7004/7005#newcode463
Line 463: SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
On 2009/08/07 04:01:20, wtc wrote:
> Does it work to just use policy_info->oid?  Mozilla code
> is not always right or the best.
You right. Decoder for cert policy extension has already populated oid tags. See
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/polcyxtn.c#228

http://codereview.chromium.org/119026/diff/7004/7005#newcode578
Line 578: // to the second PKIXVerifyCert call.
The first option does not seem to be a working solution, since you would still
need to run verification for the second time to make sure EE cert policy is
properly propagated through the chain. The second option should be easy to
implement.

[ukai, 2009-08-12 09:54:09]

Thanks for review.
Addressed your comment in http://codereview.chromium.org/164394
Could you please take a look, please?

On 2009/08/08 00:47:15, alv wrote:
> http://codereview.chromium.org/119026/diff/7004/7005
> File net/base/x509_certificate_nss.cc (right):
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode365
> Line 365: CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE |
> if you plan to allow user to use default source then you will need to call
> CERT_EnableOCSPChecking and then call CERT_SetOCSPDefaultResponder to set the
> source. Otherwise I would just set CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE.
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode367
> Line 367: CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;
> I would also list here the defalt CERT_REV_M_IGNORE_MISSING_FRESH_INFO flag
> since it would improve understanding of the code.
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode421
> Line 421: bool GetEvPolicyOidTag(net::EVRootCAMetadata* metadata,
> Seems like this method and GetPolicyOidTags belong to EVRootCAMetadata object.
> Same for policy oid tags: this way you will do string conversion only ones
when
> it was not yet converted, and use existing tag in all other cases.
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode454
> Line 454: CERTCertificatePolicies* policies =
> Does this function leak policies?
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode463
> Line 463: SECOidTag oid_tag = SECOID_FindOIDTag(&policy_info->policyID);
> On 2009/08/07 04:01:20, wtc wrote:
> > Does it work to just use policy_info->oid?  Mozilla code
> > is not always right or the best.
> You right. Decoder for cert policy extension has already populated oid tags.
See
> http://mxr.mozilla.org/security/source/security/nss/lib/certdb/polcyxtn.c#228
> 
> http://codereview.chromium.org/119026/diff/7004/7005#newcode578
> Line 578: // to the second PKIXVerifyCert call.
> The first option does not seem to be a working solution, since you would still
> need to run verification for the second time to make sure EE cert policy is
> properly propagated through the chain. The second option should be easy to
> implement.

[wtc, 2009-08-13 00:22:06]

Alexei,

Thanks a lot for your review comments.  We will make your
suggested changes.  I have a question for you below.

http://codereview.chromium.org/119026/diff/7004/7005
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/119026/diff/7004/7005#newcode578
Line 578: // to the second PKIXVerifyCert call.
On 2009/08/08 00:47:15, alv wrote:
> The first option does not seem to be a working solution, since you would still
> need to run verification for the second time to make sure EE cert policy is
> properly propagated through the chain.

Alexei, you may have misunderstood me.

What I meant is:
1. We specify cert_po_policyOID in cvout.
2. Call CERT_PKIXVerifyCert
3. On return, cvout should contain all the policies that
   were found to be valid for cert chain.
4. If any of the returned policies is the EV policy of the
   trust anchor, the cert is EV.

Did I miss something?

[alv, 2009-08-13 18:46:56]

> Alexei, you may have misunderstood me.
> 
> What I meant is:
> 1. We specify cert_po_policyOID in cvout.
> 2. Call CERT_PKIXVerifyCert
> 3. On return, cvout should contain all the policies that
>    were found to be valid for cert chain.
> 4. If any of the returned policies is the EV policy of the
>    trust anchor, the cert is EV.
> 
> Did I miss something?
Ah, got it now. Yes, this option will work just fine when cert_po_policyOID flag
will be checked by libpkix code. For now
this feature is not implemented.