Improvements to the net/cert/internal error handling.

Improvements to the net/cert/internal error handling.

 * Don't short-circuit on the first error in VerifyCertificateChain().
   This way multiple errors can be accumulated and reported.

 * Remove the CertErrorScoper class, which was an overly complicated
   abstraction only being used to bucket errors per certificate. Instead
   do this directly by creating separate error stores.

BUG=634443
Review-Url: https://codereview.chromium.org/2759023002
Cr-Commit-Position: refs/heads/master@{#459278}
Committed: https://chromium.googlesource.com/chromium/src/+/23885b6eeb422241f91094c808fdcc9ce5f80eb3

[eroman, 2017-03-19 00:57:02]

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

[commit-bot: I haz the power, 2017-03-19 00:57:12]

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

[eroman, 2017-03-19 00:57:18]

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

[commit-bot: I haz the power, 2017-03-19 01:57:52]

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

[commit-bot: I haz the power, 2017-03-19 01:57:53]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-03-23 00:32:06]

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:114: if (node.severity ==
CertError::SEVERITY_HIGH)
Maybe use a switch so this is forced to be updated if new severities are added?
Or use (severity != SEVERITY_WARNING) so that it fails safe?

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:176: if (i < certs.size() && certs[i]) {
I guess this means the trust anchor errors won't show a name, as the passed in
cert list doesn't include the trust anchor? (I noticed you have a TODO about
improving the trust anchor error stuff in verify_certificate_chain, so if this
is part of that todo that's okay.)

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:178: if
(ParseNameValue(certs[i]->normalized_subject(), &subject) &&
use the original (un-normalized) subject from tbs() for display

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:31: // certfificate in the verified chain.
certificate

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:149: std::string ToDebugString(const
ParsedCertificateList& certs) const;
comment? (like that it will be an empty string if there are no errors/warnings)

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:190: if (cert.normalized_issuer()
!= working_normalized_issuer_name) {
{} no longer needed here

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:272: --(*max_path_length_ptr);
underflow if max_path_length is already 0?

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:514: // Output errors for the
currently certificate into an error bucket that is
missing word or typo in "currently certificate"

[eroman, 2017-03-23 01:29:02]

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

[commit-bot: I haz the power, 2017-03-23 01:29:49]

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

[eroman, 2017-03-23 01:32:12]

Thanks for the review!

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:114: if (node.severity ==
CertError::SEVERITY_HIGH)
On 2017/03/23 00:32:05, mattm wrote:
> Maybe use a switch so this is forced to be updated if new severities are
added?
> Or use (severity != SEVERITY_WARNING) so that it fails safe?

I ended up solving this by changing the signature (takes a severity level as
input now), which I think resolves some naming ambiguity I had before on what
"HasErrors()" means.

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:176: if (i < certs.size() && certs[i]) {
On 2017/03/23 00:32:05, mattm wrote:
> I guess this means the trust anchor errors won't show a name, as the passed in
> cert list doesn't include the trust anchor? (I noticed you have a TODO about
> improving the trust anchor error stuff in verify_certificate_chain, so if this
> is part of that todo that's okay.)

You are correct, that case is not handled.
I have added another TODO to clarify that here.

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.cc:178: if
(ParseNameValue(certs[i]->normalized_subject(), &subject) &&
On 2017/03/23 00:32:05, mattm wrote:
> use the original (un-normalized) subject from tbs() for display

Done.

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:31: // certfificate in the verified chain.
On 2017/03/23 00:32:05, mattm wrote:
> certificate

Done.

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/cert_erro...
net/cert/internal/cert_errors.h:149: std::string ToDebugString(const
ParsedCertificateList& certs) const;
On 2017/03/23 00:32:05, mattm wrote:
> comment? (like that it will be an empty string if there are no
errors/warnings)

Done.

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

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:190: if (cert.normalized_issuer()
!= working_normalized_issuer_name) {
On 2017/03/23 00:32:05, mattm wrote:
> {} no longer needed here

Done.

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:272: --(*max_path_length_ptr);
On 2017/03/23 00:32:05, mattm wrote:
> underflow if max_path_length is already 0?

Good spot, thanks!

(I was afraid of issues like that from removing short circuiting, but didn't
catch this one. Interestingly there is a test for this, however the underflow
doesn't cause it to fail, as it wraps around to something big).

https://codereview.chromium.org/2759023002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:514: // Output errors for the
currently certificate into an error bucket that is
On 2017/03/23 00:32:05, mattm wrote:
> missing word or typo in "currently certificate"

Done.

[commit-bot: I haz the power, 2017-03-23 02:01:10]

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

[commit-bot: I haz the power, 2017-03-23 02:01:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-03-23 20:13:10]

lgtm (looks like you need to update the test expectations for the non-normalized
names though)

https://codereview.chromium.org/2759023002/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.h (right):

https://codereview.chromium.org/2759023002/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_errors.h:148: // Shortcut for
ContainsHighSeverityErrors().
Should that be "Shortcut for
ContainsAnyErrorWithSeverity(CertError::SEVERITY_HIGH)"?

[eroman, 2017-03-23 21:54:21]

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

[eroman, 2017-03-23 21:54:35]

https://codereview.chromium.org/2759023002/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_errors.h (right):

https://codereview.chromium.org/2759023002/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_errors.h:148: // Shortcut for
ContainsHighSeverityErrors().
On 2017/03/23 20:13:10, mattm wrote:
> Should that be "Shortcut for
> ContainsAnyErrorWithSeverity(CertError::SEVERITY_HIGH)"?

Done.

[commit-bot: I haz the power, 2017-03-23 21:54:58]

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-03-23 23:01:02]

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

[commit-bot: I haz the power, 2017-03-23 23:01:03]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-03-23 23:22:29]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-03-23 23:22:29]

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/2759023002/#ps60001
(title: "fix comment")

[commit-bot: I haz the power, 2017-03-23 23:23:07]

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-03-23 23:30:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490311349224920,
"parent_rev": "6e421c60e822ffaba52747b847bd4a72df4f82d8", "commit_rev":
"23885b6eeb422241f91094c808fdcc9ce5f80eb3"}

[commit-bot: I haz the power, 2017-03-23 23:31:09]

Description was changed from

==========
Improvements to the net/cert/internal error handling.

 * Don't short-circuit on the first error in VerifyCertificateChain().
   This way multiple errors can be accumulated and reported.

 * Remove the CertErrorScoper class, which was an overly complicated
   abstraction only being used to bucket errors per certificate. Instead
   do this directly by creating separate error stores.

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

to

==========
Improvements to the net/cert/internal error handling.

 * Don't short-circuit on the first error in VerifyCertificateChain().
   This way multiple errors can be accumulated and reported.

 * Remove the CertErrorScoper class, which was an overly complicated
   abstraction only being used to bucket errors per certificate. Instead
   do this directly by creating separate error stores.

BUG=634443

Review-Url: https://codereview.chromium.org/2759023002
Cr-Commit-Position: refs/heads/master@{#459278}
Committed:
https://chromium.googlesource.com/chromium/src/+/23885b6eeb422241f91094c808fd...
==========

[commit-bot: I haz the power, 2017-03-23 23:31:10]

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