Pickle (serialize and deserialize) MultiThreadedCertVerifier's |cache_|

Pickle (serialize and deserialize) MultiThreadedCertVerifier's |cache_|
and it's ojects.

TODO: wire this up to Android's bundle when chrome is killed under
memory pressure.

BUG=590875
R=rsleevi@chromium.org

[ramant (doing other things), 2016-02-29 22:13:22]

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

[commit-bot: I haz the power, 2016-02-29 22:13:55]

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

[ramant (doing other things), 2016-02-29 22:14:26]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2016-02-29 22:15:33]

Description was changed from

==========
Pickle (serialize and deserialize) MultiThreadedCertVerifier's |cache_|
and it's ojects.

TODO: wire this up to Android's bundle when chrome is killed under
memory pressure.

BUG=590875
==========

to

==========
Pickle (serialize and deserialize) MultiThreadedCertVerifier's |cache_|
and it's ojects.

TODO: wire this up to Android's bundle when chrome is killed under
memory pressure.

BUG=590875
R=rsleevi@chromium.org
==========

[ramant (doing other things), 2016-02-29 22:15:33]

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

[ramant (doing other things), 2016-02-29 22:23:33]

Hi Ryan,
  This is a first cut at pickling MultiThreadedCertVerifier's |cache_|. This
code is not hooked up. Will work with Paul Jensen in the next CL.

Couple of questions:
1) Passed the object to be filled as argument to CreateFromPickle method. Should
we change the signature to return the object and caller takes ownership?

2) Would like to make kTTLSecs accessible from unit tests.

PTAL.

thanks
raman

[Ryan Sleevi, 2016-02-29 23:28:41]

Not LGTM.

We should have talked more about the design; I didn't realize you were still
planning to look into this.

I tried to summarize the core objections, rather than provide a nit-picky
design.

My "High Level Perspective" on this is that coupling serialization to API
representation is often a violation of the single responsibility - the nature of
the serialization scheme you choose can differ according to different
requirements and you're now coupling the internal representation to a
semi-stable representation (thus preventing the API from changing or adapting to
new needs).

I will pretty much "Not LGTM" any change that does Persist(base::Pickle*) /
CreateFromPickle(...), because of the problems it introduces. It's only truly
suitable for long-term stable formats, and inevitably those are few and far
between. It also makes it impossible to reason about the lifetime of the
serialized objects - you have no idea if the Pickle will be used for IPC (... It
shouldn't be) or stored to disk (meaning versioning) or something else.

Further, your bug, and the CL description, don't really do good at explaining
_why_ - they explain the _what_ (but not really), but don't really capture many
of the in-person discussions we've had, which are exactly the kind of thing you
want to capture for future source archaelogists.

So let's talk about concrete suggestions:
- Update the bug to explain why the motivation for this change, either as a
design doc (which allows commenting, yay) or as the bug comment (but expect some
discussion)
  - What we're seeing (suitable for public sharing)
  - What we've measured to support this hypothesis
  - What the risks are; for example, does closing Chrome no longer guarantee
we'll flush the cache? What about clock changes in between Chrome launches
(which is a real problem even when Chrome is presently running)
  - What are our success metrics. What are our failure metrics?

- Once we're good on documenting the plan, let's take another stab at the code
change, by decoupling the serialization aspect from the implementation
  - For example, by going through the exercise of documenting on the bug/in a
design doc, we may find the answer to the question of "Should serialization be
handled by the ExpiringCache type?" - which would depend on if other consumers
of the ExpiringCache (such as DNS) are believed to share the same performance
concerns
  - We could evaluate whether it makes sense to expose a public interface for
iterating through the storage (in a read-only fashion) or initializing from a
similar iterator (using fully formed CertVerifierResults).
    - This provides a strong API contract, enforcible by the compiler and
runtime, about what we expect our pre-conditions and post-conditions to be.
  - Whether we should be using protobufs for this, or whether it truly does make
sense to use a Pickle

I realize we've been going round and round on this approach for many months, but
a big part of the challenge has been documenting the discussion and actually
what we're seeing / what we're expecting - that is, the data to support why we
believe this is good.

Yes, Chrome gets evicted more often on mobile. Yes, we see greater verification
latency on mobile. No, I'm not aware of any documentation we've done for metrics
to see if there's any relationship that justifies this approach - or of the
concerns, either security or privacy.

I totally want to figure out how we can make things better, but I want to make
sure we have clear objectives and clear metrics, as well as clear criteria on
whether to keep the code or remove it. I think that's been a problem with a lot
of //net experiments over time, which is why I'm trying to push back here early
on, so that we can either wind everything up or that everything we land we're
good with supporting for years to come.

https://codereview.chromium.org/1745283002/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/1745283002/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:35: CertVerifyResult* result);
I'm strongly opposed to adding pickling methods

1) Security: This is touched upon in
https://www.chromium.org/Home/chromium-security/education/security-tips-for-i....
, but in general, serialization logic like this is subtle, complex, and a rife
opportunity for security bugs.

2) Design: This violates the "Single Responsibility" into adding complex
serialization logic here

3) API DESIGN: This is meant to be an internal type, not a file-stable type.
Adding serialization to this type means complex versioning schemes must
inevitably be dealt with - and we should not encourage that. This is why we have
things like Protobufs, to handle when fields are present or not, rather than
creating ad-hoc serialization schemes.

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

https://codereview.chromium.org/1745283002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:113: RequestParams* param);
Same remarks

[ramant (doing other things), 2016-04-21 18:13:53]

Closing this CL. Working on the following CL which uses protobuf
and which will implement API calls suggested by rsleevi.

https://codereview.chromium.org/1892033002/