Add initial CertVerifyProcBuiltin.

Add initial CertVerifyProcBuiltin.

This is an implementation of CertVerifyProc that uses the Chromium PathBuilder to validate certificates.

This doesn't handle everything yet (and one of the tests is disabled).

BUG=649017
Review-Url: https://codereview.chromium.org/2755483008
Cr-Commit-Position: refs/heads/master@{#457613}
Committed: https://chromium.googlesource.com/chromium/src/+/8ccd62d4dadcfbef51508e3b57615140faa8739b

[eroman, 2017-03-15 20:52:05]

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

[commit-bot: I haz the power, 2017-03-15 20:52:52]

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-15 21:45:25]

Description was changed from

==========
TODO
BUG=
==========

to

==========
Add initial CertVerifyProcBuiltin.

This is an implementation of CertVerifyProc that uses the Chromium PathBuilder
to validate certificates.

This doesn't handle everything yet (and one of the tests is disabled).

BUG=649017
==========

[eroman, 2017-03-15 21:45:42]

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

[commit-bot: I haz the power, 2017-03-15 22:13:12]

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

[commit-bot: I haz the power, 2017-03-15 22:13:13]

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

[eroman, 2017-03-16 01:16:28]

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

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

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-16 02:22:02]

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

[commit-bot: I haz the power, 2017-03-16 02:22:03]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-03-16 02:58:24]

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
File net/cert/cert_verify_proc_builtin.cc (right):

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:169: // requires searching for the trust
anchor by name in NSS (path building
I think you could use CERT_FindCertByDERCert instead

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:237: LOG(ERROR)
NOT_IMPLEMENTED() ?

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:278: VLOG(1) <<
partial_path.errors.ToDebugString();
Is this temporary? should it be DVLOG?

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

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:92: auto cert_handles =
cert->GetIntermediateCertificates();
Overuse of auto? (I think it should be "const auto&" at least, but const
X509Certificate::OSCertHandles& is probably better.)

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:95: auto cert =
ParseCertificateFromOSHandle(*it, &errors);
I believe there was also concern over using auto for smart pointer types.. so
maybe just spell out the scoped_refptr<ParsedCertificate> here.

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:95: auto cert =
ParseCertificateFromOSHandle(*it, &errors);
avoid rebinding of |cert| variable name

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:142: for (const auto& x590_cert :
additional_trust_anchors) {
x509_cert ?

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:147:
TrustAnchor::CreateFromCertificateNoConstraints(cert));
std::move(cert) ?

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:399: // TODO(eroman): Is this case
reachable?
Yeah, the path builder currently only returns paths that chained to an anchor
but failed to verify, so if no path reached an anchor there will be zero paths
returned.

It doesn't currently return partial paths that didn't reach an anchor, though
there is a todo about maybe doing that, and the API is intended to allow it.

[eroman, 2017-03-16 17:35:21]

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

[eroman, 2017-03-16 17:35:41]

Thanks for the review!

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
File net/cert/cert_verify_proc_builtin.cc (right):

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:169: // requires searching for the trust
anchor by name in NSS (path building
On 2017/03/16 02:58:23, mattm wrote:
> I think you could use CERT_FindCertByDERCert instead

Good idea, Done.

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:237: LOG(ERROR)
On 2017/03/16 02:58:24, mattm wrote:
> NOT_IMPLEMENTED() ?

Done.

https://codereview.chromium.org/2755483008/diff/1/net/cert/cert_verify_proc_b...
net/cert/cert_verify_proc_builtin.cc:278: VLOG(1) <<
partial_path.errors.ToDebugString();
On 2017/03/16 02:58:24, mattm wrote:
> Is this temporary? should it be DVLOG?

I changed it to LOG(ERROR) so it is in line with how cert_verify_proc_nss.cc
logs errors:

https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_nss.cc?q=cert_...

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

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:92: auto cert_handles =
cert->GetIntermediateCertificates();
On 2017/03/16 02:58:24, mattm wrote:
> Overuse of auto? (I think it should be "const auto&" at least, but const
> X509Certificate::OSCertHandles& is probably better.)

Done.

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:95: auto cert =
ParseCertificateFromOSHandle(*it, &errors);
On 2017/03/16 02:58:24, mattm wrote:
> I believe there was also concern over using auto for smart pointer types.. so
> maybe just spell out the scoped_refptr<ParsedCertificate> here.

Done.

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:95: auto cert =
ParseCertificateFromOSHandle(*it, &errors);
On 2017/03/16 02:58:24, mattm wrote:
> avoid rebinding of |cert| variable name

Done.

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:142: for (const auto& x590_cert :
additional_trust_anchors) {
On 2017/03/16 02:58:24, mattm wrote:
> x509_cert ?

Done.

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:147:
TrustAnchor::CreateFromCertificateNoConstraints(cert));
On 2017/03/16 02:58:24, mattm wrote:
> std::move(cert) ?

Done (however I wonder now if that should take a const ref instead).

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:399: // TODO(eroman): Is this case
reachable?
On 2017/03/16 02:58:24, mattm wrote:
> Yeah, the path builder currently only returns paths that chained to an anchor
> but failed to verify, so if no path reached an anchor there will be zero paths
> returned.
> 
> It doesn't currently return partial paths that didn't reach an anchor, though
> there is a todo about maybe doing that, and the API is intended to allow it.

Removed the TODO.

[commit-bot: I haz the power, 2017-03-16 17:36:07]

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-16 17:36:26]

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

https://codereview.chromium.org/2755483008/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:399: // TODO(eroman): Is this case
reachable?
On 2017/03/16 17:35:41, eroman wrote:
> On 2017/03/16 02:58:24, mattm wrote:
> > Yeah, the path builder currently only returns paths that chained to an
anchor
> > but failed to verify, so if no path reached an anchor there will be zero
paths
> > returned.
> > 
> > It doesn't currently return partial paths that didn't reach an anchor,
though
> > there is a todo about maybe doing that, and the API is intended to allow it.
> 
> Removed the TODO.

Correction -- I didn't remove the TODO, i just reworded it.

[commit-bot: I haz the power, 2017-03-16 17:44:53]

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

[commit-bot: I haz the power, 2017-03-16 17:44:54]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[eroman, 2017-03-16 18:24:08]

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

[commit-bot: I haz the power, 2017-03-16 18:27:05]

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-16 20:52:06]

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

[commit-bot: I haz the power, 2017-03-16 20:52:06]

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

[mattm, 2017-03-16 22:22:56]

lgtm

[eroman, 2017-03-16 23:09:12]

The CQ bit was checked by eroman@chromium.org

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489705752648270,
"parent_rev": "62a3acd978daee61b3ea99b4cd705d4401203ee3", "commit_rev":
"8ccd62d4dadcfbef51508e3b57615140faa8739b"}

[commit-bot: I haz the power, 2017-03-16 23:55:11]

Description was changed from

==========
Add initial CertVerifyProcBuiltin.

This is an implementation of CertVerifyProc that uses the Chromium PathBuilder
to validate certificates.

This doesn't handle everything yet (and one of the tests is disabled).

BUG=649017
==========

to

==========
Add initial CertVerifyProcBuiltin.

This is an implementation of CertVerifyProc that uses the Chromium PathBuilder
to validate certificates.

This doesn't handle everything yet (and one of the tests is disabled).

BUG=649017

Review-Url: https://codereview.chromium.org/2755483008
Cr-Commit-Position: refs/heads/master@{#457613}
Committed:
https://chromium.googlesource.com/chromium/src/+/8ccd62d4dadcfbef51508e3b5761...
==========

[commit-bot: I haz the power, 2017-03-16 23:55:13]

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