Add tests for keyUsage to the built-in cert verifier.

Add tests for keyUsage to the built-in cert verifier.

This verifies that key usage on end-entities is NOT tested.

Review-Url: https://codereview.chromium.org/2813043002
Cr-Commit-Position: refs/heads/master@{#465117}
Committed: https://chromium.googlesource.com/chromium/src/+/3db4686466461aa65dadcef4f4b96db53e425b9d

[eroman, 2017-04-11 23:03:42]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 23:04:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-04-11 23:07:01]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 23:07:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-04-11 23:08:36]

eroman@chromium.org changed reviewers:
+ mattm@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:22:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:22:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mattm, 2017-04-12 01:58:05]

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1219: verify_proc_type() ==
CERT_VERIFY_PROC_BUILTIN) {
From your other email, NSS was the only one that checks the key usage right? Why
is it that the other platforms don't need to be special cased here too?

[eroman, 2017-04-14 21:07:48]

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1219: verify_proc_type() ==
CERT_VERIFY_PROC_BUILTIN) {
On 2017/04/12 01:58:05, mattm wrote:
> From your other email, NSS was the only one that checks the key usage right?
Why
> is it that the other platforms don't need to be special cased here too?

This test is not well described. There are two core problems with the
certificate being verified:
  (1) It has an EKU for clientAuth (and not serverAuth)
  (2) It is not signed by a provided trust anchor

So really the core problem is that the EKU is wrong (clientAuth and not
serverAuth). NSS can balk about the key usage (whose expectations it sets based
on the required key purpose).

The test would have been more interesting if it set the KeyUsage as
digitalSignature AND it set EKU to serverAuth, since now it would solely be
testing the key usage bit. However if that were the test, then everything but
NSS would be failing consistently with AUTHORITY_INVALID, as they don't actually
care about the key usage.

The specific errors that the implementations return are variations of these two
underlying causes.
   INVALID -- for the incorrect EKU (or in NSS's case, also key usage)
   AUTHORITY_INVALID -- for not chaining to a trust anchor

I will go ahead and update the test description, and try to make the
expectations less confusing.

[eroman, 2017-04-14 21:42:26]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[eroman, 2017-04-14 21:42:48]

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2813043002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:1219: verify_proc_type() ==
CERT_VERIFY_PROC_BUILTIN) {
On 2017/04/14 21:07:47, eroman wrote:
> On 2017/04/12 01:58:05, mattm wrote:
> > From your other email, NSS was the only one that checks the key usage right?
> Why
> > is it that the other platforms don't need to be special cased here too?
> 
> This test is not well described. There are two core problems with the
> certificate being verified:
>   (1) It has an EKU for clientAuth (and not serverAuth)
>   (2) It is not signed by a provided trust anchor
> 
> So really the core problem is that the EKU is wrong (clientAuth and not
> serverAuth). NSS can balk about the key usage (whose expectations it sets
based
> on the required key purpose).
> 
> The test would have been more interesting if it set the KeyUsage as
> digitalSignature AND it set EKU to serverAuth, since now it would solely be
> testing the key usage bit. However if that were the test, then everything but
> NSS would be failing consistently with AUTHORITY_INVALID, as they don't
actually
> care about the key usage.
> 
> The specific errors that the implementations return are variations of these
two
> underlying causes.
>    INVALID -- for the incorrect EKU (or in NSS's case, also key usage)
>    AUTHORITY_INVALID -- for not chaining to a trust anchor
> 
> I will go ahead and update the test description, and try to make the
> expectations less confusing.

Done.

[commit-bot: I haz the power, 2017-04-14 21:42:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-04-14 22:40:00]

lgtm

[commit-bot: I haz the power, 2017-04-14 23:08:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 23:08:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2017-04-18 00:22:01]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 00:22:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 02:11:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 02:11:11]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-04-18 02:45:32]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-04-18 02:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2813043002/#ps60001
(title: "rebase and add test files for ios")

[commit-bot: I haz the power, 2017-04-18 02:45:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 02:50:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492483532109270,
"parent_rev": "930abad73febbf6ec12c9638347912d3ec65b2b2", "commit_rev":
"3db4686466461aa65dadcef4f4b96db53e425b9d"}

[commit-bot: I haz the power, 2017-04-18 02:50:36]

Description was changed from

==========
Add tests for keyUsage to the built-in cert verifier.

This verifies that key usage on end-entities is NOT tested.
==========

to

==========
Add tests for keyUsage to the built-in cert verifier.

This verifies that key usage on end-entities is NOT tested.

Review-Url: https://codereview.chromium.org/2813043002
Cr-Commit-Position: refs/heads/master@{#465117}
Committed:
https://chromium.googlesource.com/chromium/src/+/3db4686466461aa65dadcef4f4b9...
==========

[commit-bot: I haz the power, 2017-04-18 02:50:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3db4686466461aa65dadcef4f4b9...