Add error information to VerifyCertificateChain().

Add error information to VerifyCertificateChain().

* Errors are identified by strings
* Errors may contain parameters
* Chain verification may set multiple errors/warnings

This is an initial stab at the problem, and there are multiple TODOs to
follow-up on.

BUG=634443
Committed: https://crrev.com/27e6c32af368dbf07477b9ad81cf87afb5789597
Cr-Commit-Position: refs/heads/master@{#415120}

[eroman, 2016-08-27 01:25:50]

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

[eroman, 2016-08-27 01:25:50]

I am still working on updating all the test data to include error information.

But other than that I believe this CL is ready for review.

Per our discussion over IM, I expect to iterate more this design (especially
once integrated with path builder).

But I think it is good enough to give utility in the meantime, as it works
reasonably well for the path validation stuff.

[mattm, 2016-08-29 22:15:11]

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/cert_erro...
File net/cert/internal/cert_errors.h (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder.cc:638: //  TODO(crbug.com/634443): Expose
CertErrors on ResultPath.
I guess there would also be a CertErrors on the Result for errors not associated
with a specific path?

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:458: DCHECK(trust_anchor);
DCHECK(errors) and signature_policy also?

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:529: // Set the certificate anchor
as the current context for any subsequent
Is "anchor" here a typo?

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:582:
DEFINE_CERT_ERROR_TYPE(kNotPermitterdByNameConstraints,
s/Permitterd/Permitted/

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/generate-target-signed-by-512bit-rsa.py
(right):

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-target-signed-by-512bit-rsa.py:26:
errors = ['Signature verification failed']
I assume we would update signature verification to have more specific errors
later?

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem
(right):

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem:285:
-----BEGIN ERRORS-----
Include text version as a comment above the pem data? (would be nice for the
other block types too)

[eroman, 2016-08-29 22:55:18]

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/cert_erro...
File net/cert/internal/cert_errors.h (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/08/29 22:15:11, mattm wrote:
> 2016

Done.

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder.cc:638: //  TODO(crbug.com/634443): Expose
CertErrors on ResultPath.
On 2016/08/29 22:15:11, mattm wrote:
> I guess there would also be a CertErrors on the Result for errors not
associated
> with a specific path?

Yes, I expect so.

(This TODO was specifically about this callsite, where |errors| is otherwise
unused).

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:458: DCHECK(trust_anchor);
On 2016/08/29 22:15:11, mattm wrote:
> DCHECK(errors) and signature_policy also? 

Done.

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:529: // Set the certificate anchor
as the current context for any subsequent
On 2016/08/29 22:15:11, mattm wrote:
> Is "anchor" here a typo?

Yes. Fixed.

https://codereview.chromium.org/2282183004/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:582:
DEFINE_CERT_ERROR_TYPE(kNotPermitterdByNameConstraints,
On 2016/08/29 22:15:11, mattm wrote:
> s/Permitterd/Permitted/

Good spot! Fixed.

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/generate-target-signed-by-512bit-rsa.py
(right):

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-target-signed-by-512bit-rsa.py:26:
errors = ['Signature verification failed']
On 2016/08/29 22:15:11, mattm wrote:
> I assume we would update signature verification to have more specific errors
> later?

Yes correct.

Follow-ups will probably involve:

 * Update VerifySignedData() to take a CertErrors* parameter (naming maybe is
not ideal). It can plumb more specific errors down through this

  * Update SignaturePolicy interface to also take CertErrors* so rejection of
SignatureAlgorithms can be explained

  * Update SignatureAlgorithm parsing to be more error rich.

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem
(right):

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem:285:
-----BEGIN ERRORS-----
On 2016/08/29 22:15:11, mattm wrote:
> Include text version as a comment above the pem data? (would be nice for the
> other block types too)

Will do.

[eroman, 2016-08-29 23:14:55]

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem
(right):

https://codereview.chromium.org/2282183004/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/target-has-pathlen-but-not-ca.pem:285:
-----BEGIN ERRORS-----
On 2016/08/29 22:55:18, eroman wrote:
> On 2016/08/29 22:15:11, mattm wrote:
> > Include text version as a comment above the pem data? (would be nice for the
> > other block types too)
> 
> Will do.

Added text label in latest patchset, for TIME, VERIFY_RESULT, and ERRORS.

(Still in the process of updating all the ERRORS).

[eroman, 2016-08-29 23:42:35]

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

[eroman, 2016-08-29 23:46:46]

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

[eroman, 2016-08-29 23:47:21]

OK I think all feedback is addressed now (updated the test data).

[commit-bot: I haz the power, 2016-08-30 00:27:10]

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

[mattm, 2016-08-30 00:32:45]

lgtm

[eroman, 2016-08-30 01:11:17]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2016-08-30 01:11:17]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-08-30 01:59:17]

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, 2016-08-30 04:53:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-30 04:55:56]

Description was changed from

==========
Add error information to VerifyCertificateChain().

* Errors are identified by strings
* Errors may contain parameters
* Chain verification may set multiple errors/warnings

This is an initial stab at the problem, and there are multiple TODOs to
follow-up on.

BUG=634443
==========

to

==========
Add error information to VerifyCertificateChain().

* Errors are identified by strings
* Errors may contain parameters
* Chain verification may set multiple errors/warnings

This is an initial stab at the problem, and there are multiple TODOs to
follow-up on.

BUG=634443

Committed: https://crrev.com/27e6c32af368dbf07477b9ad81cf87afb5789597
Cr-Commit-Position: refs/heads/master@{#415120}
==========

[commit-bot: I haz the power, 2016-08-30 04:55:58]

Patchset 6 (id:??) landed as
https://crrev.com/27e6c32af368dbf07477b9ad81cf87afb5789597
Cr-Commit-Position: refs/heads/master@{#415120}