Introduce CertVerifier::RequestParams

Introduce CertVerifier::RequestParams

In anticipation of divorcing caching from multi-threading,
move the MultiThreadedCertVerifier's RequestParams up to
CertVerifier, as part of a public, stable interface for
handling verification request. This mirrors the notion of
HostResolver's RequestInfo.

While Verify() does not yet take a RequestParams (which
will come later), this provides a common interface to use
as a key for determining if two requests should be treated
the same (e.g. for purposes of caching or joining existing
requests).

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

[Ryan Sleevi, 2016-05-18 00:45:54]

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

[Ryan Sleevi, 2016-05-18 00:45:55]

First part. Need to introduce cert_verifier_unittest to cover the
MultiThreadedCertVerifier test

[Ryan Sleevi, 2016-05-18 00:58:58]

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

[commit-bot: I haz the power, 2016-05-18 00:59:42]

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

[eroman, 2016-05-18 01:43:18]

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

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:80: // |flags| are required. The rest are optional.
Are you going to move documentation for the various inputs when updating
Verify() to use this?

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:102: std::vector<SHA1HashValue> request_data_;
Can you speak to the design choice of using a vector of hashes rather than a
single hash? Was this to provide a particular sorting stability? Reduce the time
spent hashing? Other?
Using a single cryptographically secure hash would use less memory.

Also why are we OK with using SHA1 here? If we believe SHA1 to be susceptible to
preimage attacks (now or in the near future), using it to key the certificate
cache doesn't sound like a good idea.

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
File net/cert/cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
net/cert/cert_verifier_unittest.cc:136: }
May want to also test separately that the same key is never equal to itself

EXPECT_FALSE(key1 < key1);
EXPECT_FALSE(key2 < key2);

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

https://codereview.chromium.org/1987113002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:132: void SaveResultToCache(const
CertVerifier::RequestParams& key,
nit: is the CertVerifier:: prefix needed here?

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

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

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:80: // |flags| are required. The rest are optional.
On 2016/05/18 01:43:18, eroman wrote:
> Are you going to move documentation for the various inputs when updating
> Verify() to use this?

Yes, when updating Verify() to use this.

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:102: std::vector<SHA1HashValue> request_data_;
On 2016/05/18 01:43:18, eroman wrote:
> Can you speak to the design choice of using a vector of hashes rather than a
> single hash? Was this to provide a particular sorting stability? Reduce the
time
> spent hashing? Other?
> Using a single cryptographically secure hash would use less memory.
> 
> Also why are we OK with using SHA1 here? If we believe SHA1 to be susceptible
to
> preimage attacks (now or in the near future), using it to key the certificate
> cache doesn't sound like a good idea.

While I appreciate all of this feedback, I don't think it's good to do on this
CL. I'm trying to minimize changes to ease the implementation transition, and am
not comfortable with scope creep.

(In my earlier version, I actually did move to switch to SHA-256, but felt that
was too significant a change for this CL; similar, using a single hash -
although that has issues with separating out distinct fields)

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
File net/cert/cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
net/cert/cert_verifier_unittest.cc:136: }
On 2016/05/18 01:43:18, eroman wrote:
> May want to also test separately that the same key is never equal to itself
> 
> EXPECT_FALSE(key1 < key1);
> EXPECT_FALSE(key2 < key2);

Did you mean to say "never equal"? Or did you mean to say "Always equal"?

[eroman, 2016-05-18 02:06:44]

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

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:102: std::vector<SHA1HashValue> request_data_;
On 2016/05/18 02:02:02, Ryan Sleevi wrote:
> On 2016/05/18 01:43:18, eroman wrote:
> > Can you speak to the design choice of using a vector of hashes rather than a
> > single hash? Was this to provide a particular sorting stability? Reduce the
> time
> > spent hashing? Other?
> > Using a single cryptographically secure hash would use less memory.
> > 
> > Also why are we OK with using SHA1 here? If we believe SHA1 to be
susceptible
> to
> > preimage attacks (now or in the near future), using it to key the
certificate
> > cache doesn't sound like a good idea.
> 
> While I appreciate all of this feedback, I don't think it's good to do on this
> CL. I'm trying to minimize changes to ease the implementation transition, and
am
> not comfortable with scope creep.
> 
> (In my earlier version, I actually did move to switch to SHA-256, but felt
that
> was too significant a change for this CL; similar, using a single hash -
> although that has issues with separating out distinct fields)

OK, we can discuss that separately.

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
File net/cert/cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
net/cert/cert_verifier_unittest.cc:136: }
On 2016/05/18 02:02:02, Ryan Sleevi wrote:
> On 2016/05/18 01:43:18, eroman wrote:
> > May want to also test separately that the same key is never equal to itself
> > 
> > EXPECT_FALSE(key1 < key1);
> > EXPECT_FALSE(key2 < key2);
> 
> Did you mean to say "never equal"? Or did you mean to say "Always equal"?

See the code snippet, that is what I meant.
(A key is never less than itself)

[Ryan Sleevi, 2016-05-18 02:07:40]

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
File net/cert/cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
net/cert/cert_verifier_unittest.cc:136: }
On 2016/05/18 02:06:43, eroman wrote:
> On 2016/05/18 02:02:02, Ryan Sleevi wrote:
> > On 2016/05/18 01:43:18, eroman wrote:
> > > May want to also test separately that the same key is never equal to
itself
> > > 
> > > EXPECT_FALSE(key1 < key1);
> > > EXPECT_FALSE(key2 < key2);
> > 
> > Did you mean to say "never equal"? Or did you mean to say "Always equal"?
> 
> See the code snippet, that is what I meant.
> (A key is never less than itself)

Right, I understood the snippet, but you said "never equal" and in the two cases
you gave, the keys are equal. I suppose you meant "never less" / "always equal",
correct?

[eroman, 2016-05-18 02:29:36]

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
File net/cert/cert_verifier_unittest.cc (right):

https://codereview.chromium.org/1987113002/diff/20001/net/cert/cert_verifier_...
net/cert/cert_verifier_unittest.cc:136: }
On 2016/05/18 02:07:40, Ryan Sleevi wrote:
> On 2016/05/18 02:06:43, eroman wrote:
> > On 2016/05/18 02:02:02, Ryan Sleevi wrote:
> > > On 2016/05/18 01:43:18, eroman wrote:
> > > > May want to also test separately that the same key is never equal to
> itself
> > > > 
> > > > EXPECT_FALSE(key1 < key1);
> > > > EXPECT_FALSE(key2 < key2);
> > > 
> > > Did you mean to say "never equal"? Or did you mean to say "Always equal"?
> > 
> > See the code snippet, that is what I meant.
> > (A key is never less than itself)
> 
> Right, I understood the snippet, but you said "never equal" and in the two
cases
> you gave, the keys are equal. I suppose you meant "never less" / "always
equal",
> correct?

Correct. I should have said "never less than"

[eroman, 2016-05-18 02:33:40]

lgtm

[commit-bot: I haz the power, 2016-05-18 03:45:40]

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

[commit-bot: I haz the power, 2016-05-18 03:45:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ryan Sleevi, 2016-05-19 18:35:18]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-05-19 18:35:18]

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/1987113002/#ps40001
(title: "Add identity check")

[commit-bot: I haz the power, 2016-05-19 18:36:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987113002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987113002/40001

[commit-bot: I haz the power, 2016-05-19 19:45:49]

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

[commit-bot: I haz the power, 2016-05-19 19:45:50]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ryan Sleevi, 2016-05-19 19:58:01]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-05-19 19:59:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987113002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987113002/40001

[commit-bot: I haz the power, 2016-05-19 21:03:45]

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

[commit-bot: I haz the power, 2016-05-19 21:03:46]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

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

The CQ bit was checked by rsleevi@chromium.org

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

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/1987113002/#ps60001
(title: "Add NET_EXPORT")

[commit-bot: I haz the power, 2016-05-20 02:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987113002/60001

[commit-bot: I haz the power, 2016-05-20 02:49:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 02:50:36]

Description was changed from

==========
Introduce CertVerifier::RequestParams

In anticipation of divorcing caching from multi-threading,
move the MultiThreadedCertVerifier's RequestParams up to
CertVerifier, as part of a public, stable interface for
handling verification request. This mirrors the notion of
HostResolver's RequestInfo.

While Verify() does not yet take a RequestParams (which
will come later), this provides a common interface to use
as a key for determining if two requests should be treated
the same (e.g. for purposes of caching or joining existing
requests).

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

to

==========
Introduce CertVerifier::RequestParams

In anticipation of divorcing caching from multi-threading,
move the MultiThreadedCertVerifier's RequestParams up to
CertVerifier, as part of a public, stable interface for
handling verification request. This mirrors the notion of
HostResolver's RequestInfo.

While Verify() does not yet take a RequestParams (which
will come later), this provides a common interface to use
as a key for determining if two requests should be treated
the same (e.g. for purposes of caching or joining existing
requests).

BUG=612655

Committed: https://crrev.com/e51d72bacc26ac549ee7de34b6a96916e89cc5a9
Cr-Commit-Position: refs/heads/master@{#394967}
==========

[commit-bot: I haz the power, 2016-05-20 02:50:37]

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