Consider the verification time as well as the expiration time when caching certificate verification…

Consider the verification time as well as the expiration time when caching certificate verifications.

When caching certificate verification results, rather than simply
considering whether the current tick count (monotonically increasing) is
greater than or equal to the expiration tick count, consider the actual
(system) time of the verification when it was cached in addition to the
configurated expiration time.

This will cause cached certificate verification results to be considered
expired, and thus re-validated, if the user adjusts the system clock. Since
certificate verification results are dependent upon the system clock (eg:
they may not be valid yet or may be expired, based on what the system clock
says), this is the desired behavior.

BUG=132124
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143020

[Ryan Sleevi, 2012-06-15 21:35:29]

Chris,

You helped review the early version, mind taking a stab at the enhanced?

The naming and behaviour of the ExpiringCache is modeled further after the STL's
map template, much like the original caches were. The naming
ExpirationType/ExpirationCompare are thus modeled similarly.

The choice of a constructor with a default argument, though a violation of the
style guide, seems to be consistent both with the spirit of STL's map,
consistent with the fact that the template argument itself is an optional
argument, and consistent with the fact that the 'other' way of writing it would
be to provide two constructors, one which only took a size_t, which would have
identical behaviour as the default argument.

[szym, 2012-06-15 22:19:30]

Drive-by: Other than comments, expiring_cache.* and host_cache.* changes look
good.

http://codereview.chromium.org/10556022/diff/1/net/base/expiring_cache.h
File net/base/expiring_cache.h (right):

http://codereview.chromium.org/10556022/diff/1/net/base/expiring_cache.h#newc...
net/base/expiring_cache.h:24: //  ExpirationType must be CopyConstructible and
Assignable
nit: end with '.'

http://codereview.chromium.org/10556022/diff/1/net/base/expiring_cache.h#newc...
net/base/expiring_cache.h:25: //  ExpirationCompare is a class that takes two
arguments of the
I suggest "function class" or "class of function objects".

http://codereview.chromium.org/10556022/diff/1/net/base/expiring_cache.h#newc...
net/base/expiring_cache.h:29: //    period of |b|. That is, |b| is the
expiration criteria, and |a| is the
Maybe it'd be simpler to say:
If |comp| is an instance of ExpirationCompare, the expression |comp(current,
expiration)| shall return true iff |current| is still valid within |expiration|.

http://codereview.chromium.org/10556022/diff/1/net/base/expiring_cache.h#newc...
net/base/expiring_cache.h:77: // |expiration_comp| to compare the expirations of
elements.
suggest: "check if elements are expired".

[Ryan Sleevi, 2012-06-18 16:05:28]

ping?

[szym, 2012-06-18 16:11:37]

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:474: ValidityRange(now, now +
base::TimeDelta::FromSeconds(kTTLSecs)));
If the result is successful, shouldn't we take min(kTTLSecs,
cert->valid_expiry()) ?

[Ryan Sleevi, 2012-06-18 18:00:46]

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:474: ValidityRange(now, now +
base::TimeDelta::FromSeconds(kTTLSecs)));
On 2012/06/18 16:11:37, szym wrote:
> If the result is successful, shouldn't we take min(kTTLSecs,
> cert->valid_expiry()) ?

Possibly. Definitely an edge case. Another edge case even with that approach
would be that expiration time may be influenced by intermediates - that is,
chain validity is the min of all certs' in the chain valid_expiry, which is
something not captured.

It's a small edge case though (a recently expiring cert), and accurately
invalidating it may not be reasonable (eg: do we also invalidate any existing
SSL sessions? What about when we load things from the cache? etc). I think I'm
fine with a small window of incorrectness followed by eventual consistency, but
I'll need to think about the impact some more.

[cbentzel, 2012-06-18 19:08:28]

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h
File net/base/expiring_cache.h (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h#n...
net/base/expiring_cache.h:25: //  ExpirationCompare is a function class that
takes two arguments of the
This is interesting - given the name of the template argument and the use of
less<> as the default implementation, it looks a lot like a comparator. However,
it is not. Thanks for the detailed comment to clarify this.

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h#n...
net/base/expiring_cache.h:76: explicit ExpiringCache(
You provided good justification for this, but it may be worth getting some
feedback from other more style-conscious folks than myself about whether this is
acceptable. Personally, I'd just choose the two constructor route here as it
does not seem overly onerous to either the writer or reader. If this eventually
leads to a combinatorial explosion of constructors due to multiple default
cases, could reevaluate.

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache_uni...
File net/base/expiring_cache_unittest.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache_uni...
net/base/expiring_cache_unittest.cc:146: TEST(ExpiringCacheTest, SetWithCompact)
{
Should you add a test which provides a custom expiration comp class function?

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:441: // expiration_verification_time,
thus treating the cached result as an expired
Unclear to me: Should now.verification_time < expiration.verification_time be
treated as something different than an expiration issue?

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:452: return now.verification_time >=
expiration.verification_time &&
DCHECK that now.verification_time == now.expiration_time?

[wtc, 2012-06-18 22:10:38]

Drive-by review comments on patch set 2:

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h
File net/base/expiring_cache.h (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h#n...
net/base/expiring_cache.h:30: typename ExpirationType = base::TimeTicks,

Nit: Ideally this type parameter should be named ExpirationTimeType.
I think it'd be better to abbreviate it as TimeType.
I find ExpirationType a little confusing.

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.h:119: struct ValidityRange {

Nit: this seems to be more commonly called the validity period.

[Ryan Sleevi, 2012-06-19 00:59:28]

Nits & Comments adjusted, save for szym's point.

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h
File net/base/expiring_cache.h (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h#n...
net/base/expiring_cache.h:30: typename ExpirationType = base::TimeTicks,
On 2012/06/18 22:10:38, wtc wrote:
> 
> Nit: Ideally this type parameter should be named ExpirationTimeType.
> I think it'd be better to abbreviate it as TimeType.
> I find ExpirationType a little confusing.

Now at least, by virtue of taking ExpirationType + comparison function, there is
no requirement that these be Time based.

http://codereview.chromium.org/10556022/diff/3002/net/base/expiring_cache.h#n...
net/base/expiring_cache.h:76: explicit ExpiringCache(
On 2012/06/18 19:08:28, cbentzel wrote:
> You provided good justification for this, but it may be worth getting some
> feedback from other more style-conscious folks than myself about whether this
is
> acceptable. Personally, I'd just choose the two constructor route here as it
> does not seem overly onerous to either the writer or reader. If this
eventually
> leads to a combinatorial explosion of constructors due to multiple default
> cases, could reevaluate.

Checking the internal style list that led to the Google C++ style guide, the
response from csilvers was

"In general, the same arguments against default arguments for functions applies
to templates as well <snip of justification>. That said, if your template is
STL-like, you can choose to use STL coding conventions for the class itself.
This would have several templates with default arguments, which might include
such things as a hash function, an allocator, etc."

However, while searching the discussions, a reasonable point was raised in that
using Functor objects is preferable over Pointer types, in that it's friendlier
to the inlining phase of the compiler optimizations. Storing a function pointer
is less likely to lead to inlining.

As such, I removed the second constructor (which was needed to handle function
pointer passing), and just let the object instantiate a default functor.

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:441: // expiration_verification_time,
thus treating the cached result as an expired
On 2012/06/18 19:08:28, cbentzel wrote:
> Unclear to me: Should now.verification_time < expiration.verification_time be
> treated as something different than an expiration issue? 

It's a cache expiration issue, but not a cert expiration issue. Hooray for
overloaded terminology.

The point is, if we move into the past, we should re-verify. My experience is
that people are mostly moving into the future, after their CMOS battery
dies/fails (or the CrOS case of no battery, AFAICT), but I wanted to cover both
scenarios of user action.

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.h:119: struct ValidityRange {
On 2012/06/18 22:10:38, wtc wrote:
> 
> Nit: this seems to be more commonly called the validity period.

To some degree, I think I wanted to avoid that term, because this is not tied to
the certificate's validity period (as szym pointed out), but instead part of the
cache's validity period. However, period vs range may not be as significant as
'validity' in terms of naming.

CacheValidityPeriod is perhaps a better name, if wordier.

[cbentzel, 2012-06-19 15:12:54]

LGTM, pending resolution of szym's comment.

I did wonder if there should be a default argument for the expiration
function-like template parameter (ExpirationCompare = std::less<ExpirationType>
for example) but not sure if this also falls afoul of the no default arguments
style guide.

[Ryan Sleevi, 2012-06-19 17:57:34]

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:474: ValidityRange(now, now +
base::TimeDelta::FromSeconds(kTTLSecs)));
On 2012/06/18 18:00:46, Ryan Sleevi wrote:
> On 2012/06/18 16:11:37, szym wrote:
> > If the result is successful, shouldn't we take min(kTTLSecs,
> > cert->valid_expiry()) ?

In thinking about it, I think it's probably more complexity then needed, and it
still doesn't fully capture the dates at play:
- The validity period of the particular chain built (of which there may be
multiple candidates for different validity periods)
- The validity period of any OCSP/CRL responses
- Whether or not we may be re-using any existing connections (SPDY or SSL
sessions)
- SSL session resumption

Validity periods are primarily to ensure that subscriber information, as
represented in certificates, is accurate and regularly validated. To a lesser
degree, it's also to ensure best practices like key rotation. Immediately
invalidating the cache and treating the cert as expired thus doesn't really add
to the security properties, so I'm inclined to omit it, lest it be confused for
something that does.

[commit-bot: I haz the power, 2012-06-19 17:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10556022/17001

[szym, 2012-06-19 18:01:13]

lgtm

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (right):

http://codereview.chromium.org/10556022/diff/3002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:474: ValidityRange(now, now +
base::TimeDelta::FromSeconds(kTTLSecs)));
On 2012/06/19 17:57:34, Ryan Sleevi wrote:
> On 2012/06/18 18:00:46, Ryan Sleevi wrote:
> > On 2012/06/18 16:11:37, szym wrote:
> > > If the result is successful, shouldn't we take min(kTTLSecs,
> > > cert->valid_expiry()) ?
> 
> In thinking about it, I think it's probably more complexity then needed, and
it
> still doesn't fully capture the dates at play:
> - The validity period of the particular chain built (of which there may be
> multiple candidates for different validity periods)

This alone convinced me that it's better to leave it out so that it doesn't give
a false sense of security.

[commit-bot: I haz the power, 2012-06-19 18:10:28]

Try job failure for 10556022-17001 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-06-19 18:19:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10556022/24003

[commit-bot: I haz the power, 2012-06-19 19:37:32]

Change committed as 143020