Update CertVerifier::Verify to use RequestParams instead

Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw

Committed: https://crrev.com/06bd78559d65ac5b38b7bcfd370baf35532e79ea
Cr-Commit-Position: refs/heads/master@{#398707}

[Ryan Sleevi, 2016-05-19 22:57:20]

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

[Ryan Sleevi, 2016-05-19 22:57:21]

Eric: Next stage.

I ended up moving everything to use RequestParams now because of issues with
chaining the ChromeOS code. Separately, I want to nuke the TrustAnchorProvider
code, which this enables.

I debated back and forth on adding a short-circuit overload for RequestParams
that doesn't take the OCSP data or additional trust anchors. I'm curious for
your thoughts here - that is, whether we should have a 3-arg ctor + two methods,
a 3-arg ctor, a 4arg ctor, and 1 method, or 3-arg ctor, 4-arg ctor, and 5-arg
ctor

the 3/4/5 are (cert, hostname, flags), (cert, hostname, flags, ocsp), cert,
hostname, flags, ocsp, additional_trust_anchors)

[eroman, 2016-05-20 00:41:18]

> I'm curious for your thoughts here - that is, whether we should 
> have a 3-arg ctor + two methods, a 3-arg ctor, a 4arg ctor, and
> 1 method, or 3-arg ctor, 4-arg ctor, and 5-arg ctor

The current version with one 5-arg ctor looks fine to me, and is probably what I
would have gone with.

I definitely would avoid any setter methods, since that complicates where/when
to calculate |key_|.

Overall change LG

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

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.cc:36: // For efficiency sake, rather than compare all of
the fields for each
Have you confirmed this is worth doing with benchmarks?

Given that all the data is being kept alive now, would read more naturally to
compare fields directly.

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.cc:45: for (const auto& cert_handle :
certificate_->GetIntermediateCertificates()) {
Do you expect to separately change the fingerprint and ca_fingerprint() in
X509Certificate to use SHA256?

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:80: // Verification parameters to verify |certificate|
against the supplied
nit: "Verification parameter to verify" --> Parameters for verifying
|certificate|

(or other re-wording to not say "verify" twice).

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:81: // |hostname| as an SSL server.
Should this comment document expectation around the format of |hostname| ? (is
it expected to be canonicalized, does it not matter).

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:116: std::string hostname_;
I suggest marking these all as  const.

https://codereview.chromium.org/1994353002/diff/20001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1994353002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:440:
trust_anchor_provider_->GetAdditionalTrustAnchors().begin(),
My expectation is for GetAdditionalTrustAnchors() to return a CertificateList.
But it does in fact return a |const CertificateList&| so this could be right.

But certainly it looks very fishy when reading it.

Do you have a suggestion for improving this? Or just kill off TrustAnchor
Provider.

[Ryan Sleevi, 2016-05-20 02:39:43]

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

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.cc:36: // For efficiency sake, rather than compare all of
the fields for each
On 2016/05/20 00:41:18, eroman wrote:
> Have you confirmed this is worth doing with benchmarks?

It should be obviously beneficial; computing the DER involves additional copies,
and comparing the certs requires memcmp'ing the entire certificate - meaning
that, say, comparing two certificate chains of 3certs each would be a 12K vs 12K
comparison (with no cache locality) vs the 32-byte comparison here.

> 
> Given that all the data is being kept alive now, would read more naturally to
> compare fields directly.

We don't have lexicographical comparison for certificates, intentionally.

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.cc:45: for (const auto& cert_handle :
certificate_->GetIntermediateCertificates()) {
On 2016/05/20 00:41:18, eroman wrote:
> Do you expect to separately change the fingerprint and ca_fingerprint() in
> X509Certificate to use SHA256?

No, I intend to remove those.

https://codereview.chromium.org/1994353002/diff/20001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1994353002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:440:
trust_anchor_provider_->GetAdditionalTrustAnchors().begin(),
On 2016/05/20 00:41:18, eroman wrote:
> My expectation is for GetAdditionalTrustAnchors() to return a CertificateList.

Why? It's the same as GetIntermediateCertificates() on X509Certificate.

> But it does in fact return a |const CertificateList&| so this could be right.
> 
> But certainly it looks very fishy when reading it.
> 
> Do you have a suggestion for improving this? Or just kill off TrustAnchor
> Provider.

Killing off TAP is the next cleanup, once this lands. Well, ok, second to next -
killing off fingerprint/ca_fingerprint is next, then killing off TAP as it's
unnecessary once PolicyCertVerifier can just do the injection (that is, moving
this entirely out of MTCV, or CachingCertVerifier where I next move it up to -
that is, I'm gently shuffling the TAP into the higher layer with each CL)

[Ryan Sleevi, 2016-05-20 06:18:02]

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

[commit-bot: I haz the power, 2016-05-20 06:18:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994353002/80001

[Ryan Sleevi, 2016-05-20 06:27:36]

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

https://codereview.chromium.org/1994353002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.cc:36: // For efficiency sake, rather than compare all of
the fields for each
On 2016/05/20 02:39:43, Ryan Sleevi wrote:
 > > Given that all the data is being kept alive now, would read more naturally
to
> > compare fields directly.
> 
> We don't have lexicographical comparison for certificates, intentionally.

Turns out we do (X509Certificate::LessThan), and beyond being in tests, it's
being used in the memoizing cert store.

I'd like to get rid of it, but in the fingerprint code, I tightened it up.
However, it's still expensive (full comparison, not hash comparison), and it
seems like comparing on the hashes of the parameters here will still be a
net-win for perf.

But I'm really, really torn on this still. To gain the simplicity here, that is:

if (flags != other.flags)
  return flags < other.flags;
if (hostname != other.hostname)
  return hostname < other.hostname;
if (ocsp_response != other.ocsp_response)
  return ocsp_response < other.ocsp_response;
if (certificate != other.certificate)
  return X509Certificate::LessThan()(certificate, other.certificate);
return std::lexicographical_compare(additional_trust_anchors_.begin(),
additional_trust_anchors_.end(), other.additional_trust_anchors_.begin(),
other.additional_trust_anchors_.end(), X509Certificate::LessThan());

We'd more than likely need to reintroduce the fingerprint_ and ca_fingerprint_
members to X509Certificate, and rewrite X509Certificate::LessThan to be

if (cert.fingerprint_ != other.cert.fingerprint_)
  return cert.fingerprint_ < other.cert.fingerprint_;
if (cert.ca_fingerprint_ != other.cert.ca_fingerprint_)
  return cert.ca_fingerprint_ < other.cert.ca_fingerprint_;
if (cert_der != other.cert_der)
  return cert_der < other.cert_der;
return ca_der < other.ca_der;

And that seems.... bleh.

But I can be swayed, now that you can see the other CL for context.

[commit-bot: I haz the power, 2016-05-20 06:36:17]

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

[commit-bot: I haz the power, 2016-05-20 06:36:18]

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

[Ryan Sleevi, 2016-05-20 08:00:29]

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

[commit-bot: I haz the power, 2016-05-20 08:00:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994353002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994353002/100001

[commit-bot: I haz the power, 2016-05-20 09:07:47]

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

[commit-bot: I haz the power, 2016-05-20 09:07:48]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-05-25 07:12:42]

Ping? :)

[Kevin M, 2016-05-25 17:17:10]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-05-25 17:17:11]

blimp lgtm

[eroman, 2016-05-26 00:03:44]

lgtm

https://codereview.chromium.org/1994353002/diff/100001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1994353002/diff/100001/net/cert/cert_verifier...
net/cert/cert_verifier.h:89: // brackets, such as "[::1]".
Good comment, thanks! (Would be nice to express this more generally in other
parts of //net too).

[Ryan Sleevi, 2016-05-27 04:51:22]

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

[commit-bot: I haz the power, 2016-05-27 04:51:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994353002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994353002/100001

[commit-bot: I haz the power, 2016-05-27 06:21:27]

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

[commit-bot: I haz the power, 2016-05-27 06:21:28]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-06-08 20:54:51]

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

[commit-bot: I haz the power, 2016-06-08 20:55:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994353002/120001

[commit-bot: I haz the power, 2016-06-08 22:17:44]

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

[commit-bot: I haz the power, 2016-06-08 22:17:45]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-06-08 22:28:05]

rsleevi@chromium.org changed reviewers:
+ brettw@chromium.org

[Ryan Sleevi, 2016-06-08 22:28:06]

Brett: TBRing this to you; it's just an interface change in //net that
propagates to other consumers, and since you're top-level OWNER, seems
reasonable.

[Ryan Sleevi, 2016-06-08 22:28:25]

Description was changed from

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
==========

to

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw
==========

[Ryan Sleevi, 2016-06-08 22:28:41]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-08 22:28:41]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1994353002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-06-08 22:29:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994353002/120001

[brettw, 2016-06-08 22:30:13]

rs lgtm

[commit-bot: I haz the power, 2016-06-08 22:35:00]

Description was changed from

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw
==========

to

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw
==========

[commit-bot: I haz the power, 2016-06-08 22:35:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-08 22:36:05]

Description was changed from

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw
==========

to

==========
Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw

Committed: https://crrev.com/06bd78559d65ac5b38b7bcfd370baf35532e79ea
Cr-Commit-Position: refs/heads/master@{#398707}
==========

[commit-bot: I haz the power, 2016-06-08 22:36:07]

Patchset 7 (id:??) landed as
https://crrev.com/06bd78559d65ac5b38b7bcfd370baf35532e79ea
Cr-Commit-Position: refs/heads/master@{#398707}