Move caching out of MultiThreadedCertVerifier

Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

BUG=612655
Committed: https://crrev.com/6df5418b76ec5d9daa9df35e6f616129b72a1a5d
Cr-Commit-Position: refs/heads/master@{#399446}

[Ryan Sleevi, 2016-05-18 02:13:14]

Description was changed from

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

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

to

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

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

[Ryan Sleevi, 2016-05-18 02:15:28]

WIP:
1) The CertTrustAnchorProvider layering is in a weird place, because the
additional_trust_anchors was only supported by the MultiThreadedCertVerifier
(passing it to CertVerifyProc). This is the sort of weirdness that
CertVerify::RequestParams will hopefully alleviate, it means that some
rejiggering of the Verify() interface is needed first

2) Doesn't actually cache yet (so all tests fail, thankfully). Needs to
interpose itself into the verification chain to capture the result and cache it.

[Ryan Sleevi, 2016-05-20 03:24:12]

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

[Ryan Sleevi, 2016-05-20 03:24:13]

Next up: Get rid of fingerprint and get rid of TAP. I just moved it in this CL
to minimize the diffs and keep the related logic together

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.cc:74: new_params, start_time, callback,
verify_result);
I'm not sure how obvious this callback trick is, nor if it's entirely the right
behaviour that we want.

Namely, because I don't interject an explicit CertVerifier::Request overload, if
the caller cancels this request, we won't end up caching the result (where
previously, in MTCV, we'd cache the result, but just not notify any jobs). I
opted for this because I thought it 'simple' and 'obvious', and we had no code
relying on the 'canceled async requests still go to the cache', but I could be
convinced that I should be subclassing a CertVerifier::Request that could handle
'caching even after cancellation'.

[eroman, 2016-05-31 22:25:18]

Is this ready for review?

[Ryan Sleevi, 2016-06-06 16:26:01]

On 2016/05/31 22:25:18, eroman (OOO till June 9) wrote:
> Is this ready for review?

Sorry, yes that's what I meant when I published the message. GMail threading
kept the "DO NOT COMMIT" which was... ufnortunate.

[eroman, 2016-06-10 00:08:06]

Overall LG.

Mostly just creeped out by SetCertTrustAnchorProvider()

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.cc:52: CertificateList
additional_trust_anchors(params.additional_trust_anchors());
This is... eeew.

I realize this CL doesn't introduce the concept of trust anchor provider, nor
this approach for duplicating the RequestParams to inject the extra anchors. But
it gets weirder with this layering since the SetCertTrustAnchorProvider() is now
a concept of just the CachingCertVerifier(). So for instance you couldn't simply
remove CachingCertVerifier from the chain of verifiers, as its
SetCertTrustAnchorProvider() method is relied on (at least on Chrome OS).

Do you have a timeline for removing CertTrustAnchorProvider from Chrome OS?

Assuming this is going to linger on in the mid-term, I suggest hiding this
through yet another layer of CertVerififer. As a strawman how about:

new CertVerifierWithAdditionalTrustAnchors(
    trust_anchor_provider,
    new CachingCertVerifier(new MultiThreadedCertVerifier));

Here CertVerifierWithAdditionalTrustAnchors() would be a very simple verifier
that just retrieves the extra trust anchors and forwards a (possibly modified)
RequestParams on to the next verifier in the chain.

In the non-Chrome OS case we wouldn't need to instantiate a
CertVerifierWithAdditionalTrustAnchors.

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.h:31: void SetCertTrustAnchorProvider(
This is weird (being part of CachingCertVerifier layer). Described in more
detail in the other comment.

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier_unittest.cc:140: TEST_F(CachingCertVerifierTest,
CertTrustAnchorProvider) {
Are there any tests for asynchronous completion of the nested cert verifier?

https://codereview.chromium.org/1991653002/diff/100001/net/cert/multi_threade...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/multi_threade...
net/cert/multi_threaded_cert_verifier.cc:45: // On a cache hit,
MultiThreadedCertVerifier::Verify() returns synchronously
no longer accurate comment (the synchronous cache completion case is part of
CachingCertVerifier now).

[Ryan Sleevi, 2016-06-10 00:41:10]

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.cc:52: CertificateList
additional_trust_anchors(params.additional_trust_anchors());
On 2016/06/10 00:08:03, eroman wrote:
> Do you have a timeline for removing CertTrustAnchorProvider from Chrome OS?

Right after the Symantec stuff finishes or, if that ends up getting stalled on
reviews, during.

I want to kill it with fire, and have a plan to, but didn't want to take on
another cleanup CL (and load you or David up with more), especially when I've
got some higher priority stuff to burn.

Sorry, I thought I made it clearer on one of the other CLs that I'm
super-grossed out by this as well, and it needs to live up in the ChromeOS code
and the world will be happier.

> Here CertVerifierWithAdditionalTrustAnchors() would be a very simple verifier
> that just retrieves the extra trust anchors and forwards a (possibly modified)
> RequestParams on to the next verifier in the chain.

That's actually what I have in a local branch before I stopped for travel.

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier_unittest.cc:140: TEST_F(CachingCertVerifierTest,
CertTrustAnchorProvider) {
On 2016/06/10 00:08:04, eroman wrote:
> Are there any tests for asynchronous completion of the nested cert verifier?

Nope, mostly because I didn't opt to handle it special at all (per comment #4).
If there was behaviour to test, I wouldn't be opposed to adding one, but it
didn't seem necessary yet.

[eroman, 2016-06-10 01:26:38]

lgtm

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.cc:74: new_params, start_time, callback,
verify_result);
On 2016/05/20 03:24:13, Ryan Sleevi wrote:
> I'm not sure how obvious this callback trick is, nor if it's entirely the
right
> behaviour that we want.
> 
> Namely, because I don't interject an explicit CertVerifier::Request overload,
if
> the caller cancels this request, we won't end up caching the result (where
> previously, in MTCV, we'd cache the result, but just not notify any jobs). I
> opted for this because I thought it 'simple' and 'obvious', and we had no code
> relying on the 'canceled async requests still go to the cache', but I could be
> convinced that I should be subclassing a CertVerifier::Request that could
handle
> 'caching even after cancellation'.

This approach seems fine.

If we really cared about caching even on cancellation I would actually suggest a
different approach. Namely composing caching directly into
MultiThreadedCertVerifier -- similar to how HostResolverImpl embeds a HostCache
member. Trying to subclass CertVerifier::Request to handle this case seems worse
IMO.

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.cc:52: CertificateList
additional_trust_anchors(params.additional_trust_anchors());
On 2016/06/10 00:41:10, Ryan Sleevi wrote:
> On 2016/06/10 00:08:03, eroman wrote:
> > Do you have a timeline for removing CertTrustAnchorProvider from Chrome OS?
> 
> Right after the Symantec stuff finishes or, if that ends up getting stalled on
> reviews, during.
> 
> I want to kill it with fire, and have a plan to, but didn't want to take on
> another cleanup CL (and load you or David up with more), especially when I've
> got some higher priority stuff to burn.
> 
> Sorry, I thought I made it clearer on one of the other CLs that I'm
> super-grossed out by this as well, and it needs to live up in the ChromeOS
code
> and the world will be happier.
> 
> > Here CertVerifierWithAdditionalTrustAnchors() would be a very simple
verifier
> > that just retrieves the extra trust anchors and forwards a (possibly
modified)
> > RequestParams on to the next verifier in the chain.
> 
> That's actually what I have in a local branch before I stopped for travel.

OK, sounds good as is then.

I knew you weren't fond of it, but wanted to confirm this was just a temporary
hack and not something that would get stuck in the CachingCertVerifier.

[eroman, 2016-06-10 01:31:52]

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.h:20: class NET_EXPORT CachingCertVerifier :
public CertVerifier,
Also, please add a comment for this class (even though the behavior is entirely
unsurprising given the name).

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.h:107: uint64_t cache_hits() const { return
cache_hits_; }
side comment: if this is just for unit-testing, is size_t sufficient?

[Ryan Sleevi, 2016-06-10 07:51:21]

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1991653002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.cc:74: new_params, start_time, callback,
verify_result);
On 2016/06/10 01:26:38, eroman wrote:
> If we really cared about caching even on cancellation I would actually suggest
a
> different approach. Namely composing caching directly into
> MultiThreadedCertVerifier -- similar to how HostResolverImpl embeds a
HostCache
> member. Trying to subclass CertVerifier::Request to handle this case seems
worse
> IMO.

Well, it's a good thing we don't care, because I think I disagree on that
layering approach :)

A significant motivation for this is to extract the caching from having any
relationship to threading. That would be a step back :)

I'm not sure why you say it's worse, but I suppose that's mostly moot for now.

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.h:107: uint64_t cache_hits() const { return
cache_hits_; }
On 2016/06/10 01:31:52, eroman wrote:
> side comment: if this is just for unit-testing, is size_t sufficient?

Fair point, will do.

[Ryan Sleevi, 2016-06-10 23:41:42]

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

[Ryan Sleevi, 2016-06-10 23:41:51]

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1991653002/diff/100001/net/cert/caching_cert_...
net/cert/caching_cert_verifier.h:107: uint64_t cache_hits() const { return
cache_hits_; }
On 2016/06/10 07:51:20, Ryan Sleevi wrote:
> On 2016/06/10 01:31:52, eroman wrote:
> > side comment: if this is just for unit-testing, is size_t sufficient?
> 
> Fair point, will do.

Actually, after reading
https://www.chromium.org/developers/coding-style#TOC-Types again, and looking at
google internal practice, I've left it at uint64_t

[commit-bot: I haz the power, 2016-06-10 23:42:09]

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

[Ryan Sleevi, 2016-06-11 00:47:42]

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

[Ryan Sleevi, 2016-06-11 00:47:43]

thestig: The two bits in //chrome (which are mechanical, so feel free to RS)
droger: Same for the iOS bits

[Ryan Sleevi, 2016-06-11 00:48:00]

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

[Ryan Sleevi, 2016-06-11 00:48:01]

Actually adding droger for the mechanical bits

[Lei Zhang, 2016-06-11 01:33:45]

lgtm

[droger, 2016-06-13 08:19:02]

ios LGTM

[Ryan Sleevi, 2016-06-13 12:54:58]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-13 12:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1991653002/#ps240001
(title: "CrOS fixes")

[commit-bot: I haz the power, 2016-06-13 12:55:06]

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

[commit-bot: I haz the power, 2016-06-13 14:34:36]

Description was changed from

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

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

to

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

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

[commit-bot: I haz the power, 2016-06-13 14:34:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-13 14:34:46]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 14:36:12]

Description was changed from

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

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

to

==========
Move caching out of MultiThreadedCertVerifier

Split out the logic for caching cert verification from the
MultiThreadedCertVerifier, and into the CachingCertVerifier,
which can add caching/memoization to any CertVerifier.

BUG=612655

Committed: https://crrev.com/6df5418b76ec5d9daa9df35e6f616129b72a1a5d
Cr-Commit-Position: refs/heads/master@{#399446}
==========

[commit-bot: I haz the power, 2016-06-13 14:36:14]

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