Add CertErrors* parameter to the main Certificate parsing functions.

Add CertErrors* parameter to the main Certificate parsing functions.

This just updates the interface and callers. Actually emitting errors
in the underlying implementation will be a different CL.

BUG=634443
Committed: https://crrev.com/4f645670f68480c6c78b09b67788942d632937d5
Cr-Commit-Position: refs/heads/master@{#418586}

[eroman, 2016-09-09 23:05:12]

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

[commit-bot: I haz the power, 2016-09-09 23:05:38]

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

[eroman, 2016-09-09 23:12:05]

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

[eroman, 2016-09-09 23:12:24]

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

[commit-bot: I haz the power, 2016-09-09 23:12:31]

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, 2016-09-09 23:20:22]

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

[commit-bot: I haz the power, 2016-09-09 23:20:23]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mattm, 2016-09-12 23:29:27]

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:131: signature_policy.get(), &errors)) {
Should it use a separate errors object just in case there is anything left over
from the ParsedCertificate call?

https://codereview.chromium.org/2327973002/diff/20001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:133: &errors))
should this be nullptr, or should there be a TODO to do something with it?

[eroman, 2016-09-12 23:42:59]

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:131: signature_policy.get(), &errors)) {
On 2016/09/12 23:29:27, mattm wrote:
> Should it use a separate errors object just in case there is anything left
over
> from the ParsedCertificate call?

Good question.

I debated this a bit when writing, and my thinking was:
 (a) If there was an error, the first call would return
 (b) If there was a warning from the parsing, and then VerifySignedData failed,
that warning would leak through here.

In case (b) it isn't necessarily harmful to show that warning here, maybe it is
even relevant. (Logging the warnings each time might be overkill though)

WDYT if I add a ".Clear()" method on CertErrors?
Having two different named "errors" seems a bit weird, so I could either use
.Clear() or use a short lived instance for each call.

https://codereview.chromium.org/2327973002/diff/20001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:133: &errors))
On 2016/09/12 23:29:27, mattm wrote:
> should this be nullptr, or should there be a TODO to do something with it?

Done, changed to nullptr.

[mattm, 2016-09-13 22:27:38]

lgtm with nits

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:131: signature_policy.get(), &errors)) {
On 2016/09/12 23:42:59, eroman wrote:
> On 2016/09/12 23:29:27, mattm wrote:
> > Should it use a separate errors object just in case there is anything left
> over
> > from the ParsedCertificate call?
> 
> Good question.
> 
> I debated this a bit when writing, and my thinking was:
>  (a) If there was an error, the first call would return
>  (b) If there was a warning from the parsing, and then VerifySignedData
failed,
> that warning would leak through here.
> 
> In case (b) it isn't necessarily harmful to show that warning here, maybe it
is
> even relevant. (Logging the warnings each time might be overkill though)
> 
> WDYT if I add a ".Clear()" method on CertErrors?
> Having two different named "errors" seems a bit weird, so I could either use
> .Clear() or use a short lived instance for each call.

I worry a bit about a Clear() method since it seems it could get weird if you
used it in the presence of scopers. I guess you could add rules about when it's
allowed to be called, but seems easier just to not have it.

I suppose there is little chance that parsing and verifying would use the same
error ids, so it wouldn't be overly confusing if it did happen. So I guess just
leaving it could be okay too.

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:13: #include "net/cert/internal/cert_errors.h"
unused

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:131: CertErrors errors;
unused

[eroman, 2016-09-13 22:47:02]

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

[eroman, 2016-09-13 22:47:30]

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2327973002/diff/20001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:131: signature_policy.get(), &errors)) {
On 2016/09/13 22:27:38, mattm wrote:
> On 2016/09/12 23:42:59, eroman wrote:
> > On 2016/09/12 23:29:27, mattm wrote:
> > > Should it use a separate errors object just in case there is anything left
> > over
> > > from the ParsedCertificate call?
> > 
> > Good question.
> > 
> > I debated this a bit when writing, and my thinking was:
> >  (a) If there was an error, the first call would return
> >  (b) If there was a warning from the parsing, and then VerifySignedData
> failed,
> > that warning would leak through here.
> > 
> > In case (b) it isn't necessarily harmful to show that warning here, maybe it
> is
> > even relevant. (Logging the warnings each time might be overkill though)
> > 
> > WDYT if I add a ".Clear()" method on CertErrors?
> > Having two different named "errors" seems a bit weird, so I could either use
> > .Clear() or use a short lived instance for each call.
> 
> I worry a bit about a Clear() method since it seems it could get weird if you
> used it in the presence of scopers. I guess you could add rules about when
it's
> allowed to be called, but seems easier just to not have it.
> 
> I suppose there is little chance that parsing and verifying would use the same
> error ids, so it wouldn't be overly confusing if it did happen. So I guess
just
> leaving it could be okay too.
> 

I went ahead and created 2 different error objects: |parse_errors| and
|verify_errors| to remove the ambiguity.

(I decided against enclosing them in smaller scopes since I found it hurt
readability.)

I'll see what the Cast reviewer thinks.

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:13: #include "net/cert/internal/cert_errors.h"
On 2016/09/13 22:27:38, mattm wrote:
> unused

Done.

https://codereview.chromium.org/2327973002/diff/40001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:131: CertErrors errors;
On 2016/09/13 22:27:38, mattm wrote:
> unused

Done.

[commit-bot: I haz the power, 2016-09-13 22:47:38]

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

[eroman, 2016-09-13 23:06:04]

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

[eroman, 2016-09-13 23:06:05]

+sheretov for Cast approval.

The changes is basically just to adopt the new API (which passes in an error
sink to certificate parsing functions)

There are two related changes I introduced with this:
 (a) Dump the parsing errors to console
 (b) Removed the EXTERNAL_REFERENCE optimization (except when using static
arrays)

I can discuss (b) in more detail if you have concerns. It is technically a very
slight performance regression, as it means an extra copy of the certificate data
is made during verification, but is good for code robustness since the
referenced data variant is very hard to reason about in all but the static data
case.

[commit-bot: I haz the power, 2016-09-13 23:22:08]

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

[commit-bot: I haz the power, 2016-09-13 23:22:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[eroman, 2016-09-13 23:26:47]

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

[commit-bot: I haz the power, 2016-09-13 23:27:17]

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, 2016-09-14 00:52:55]

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

[commit-bot: I haz the power, 2016-09-14 00:52:56]

Dry run: This issue passed the CQ dry run.

[sheretov, 2016-09-14 15:50:56]

lgtm

[eroman, 2016-09-14 16:31:17]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-09-14 16:31:17]

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/2327973002/#ps100001
(title: "StringPiece is kind of dangerous...")

[commit-bot: I haz the power, 2016-09-14 16:31:30]

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-09-14 16:37:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-14 16:40:39]

Description was changed from

==========
Add CertErrors* parameter to the main Certificate parsing functions.

This just updates the interface and callers. Actually emitting errors
in the underlying implementation will be a different CL.

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

to

==========
Add CertErrors* parameter to the main Certificate parsing functions.

This just updates the interface and callers. Actually emitting errors
in the underlying implementation will be a different CL.

BUG=634443

Committed: https://crrev.com/4f645670f68480c6c78b09b67788942d632937d5
Cr-Commit-Position: refs/heads/master@{#418586}
==========

[commit-bot: I haz the power, 2016-09-14 16:40:40]

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