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

net/base/multi_threaded_cert_verifier.cc

Index: net/base/multi_threaded_cert_verifier.cc
diff --git a/net/base/multi_threaded_cert_verifier.cc b/net/base/multi_threaded_cert_verifier.cc
index 18122dd6627a09748f86707a623eb9456efff4f4..22170293a1bb90d5ddfa6fca94a3d2f00383cff4 100644
--- a/net/base/multi_threaded_cert_verifier.cc
+++ b/net/base/multi_threaded_cert_verifier.cc
@@ -83,6 +83,18 @@ MultiThreadedCertVerifier::CachedResult::CachedResult() : error(ERR_FAILED) {}
 
 MultiThreadedCertVerifier::CachedResult::~CachedResult() {}
 
+MultiThreadedCertVerifier::ValidityRange::ValidityRange(const base::Time& now)
+    : verification_time(now),
+      expiration_time(now) {
+}
+
+MultiThreadedCertVerifier::ValidityRange::ValidityRange(
+    const base::Time& now,
+    const base::Time& expiration)
+    : verification_time(now),
+      expiration_time(expiration) {
+}
+
 // Represents the output and result callback of a request.
 class CertVerifierRequest {
  public:
@@ -328,7 +340,8 @@ class CertVerifierJob {
 };
 
 MultiThreadedCertVerifier::MultiThreadedCertVerifier()
-    : cache_(kMaxCacheEntries),
+    : cache_(kMaxCacheEntries,
+             &MultiThreadedCertVerifier::CompareValidityRange),
       requests_(0),
       cache_hits_(0),
       inflight_joins_(0),
@@ -362,7 +375,7 @@ int MultiThreadedCertVerifier::Verify(X509Certificate* cert,
   const RequestParams key(cert->fingerprint(), cert->ca_fingerprint(),
                           hostname, flags);
   const CertVerifierCache::value_type* cached_entry =
-      cache_.Get(key, base::TimeTicks::Now());
+      cache_.Get(key, ValidityRange(base::Time::Now()));
   if (cached_entry) {
     ++cache_hits_;
     *out_req = NULL;
@@ -411,6 +424,35 @@ void MultiThreadedCertVerifier::CancelRequest(RequestHandle req) {
   request->Cancel();
 }
 
+// static
+bool MultiThreadedCertVerifier::CompareValidityRange(
+    const ValidityRange& now,
+    const ValidityRange& expiration) {
+  // |now| contains only a single time (verification_time), while |expiration|
+  // contains the validity range - both when the certificate was verified and
+  // when the verification should expire.
+  //
+  // If the user receives a "not yet valid" message, and adjusts their clock
+  // foward to the correct time, this will (typically) cause
+  // now.verification_time to advance past expiration.expiration_time, thus
+  // treating the cached result as an expired entry and re-verifying.
+  // If the user receives a "expired" message, and adjusts their clock backwards
+  // to the correct time, this will cause now.verification_time to be less than
+  // expiration_verification_time, thus treating the cached result as an expired

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

Unclear to me: Should now.verification_time < expiration.verification_time be
treated as something different than an expiration issue? 

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

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.

+  // entry and re-verifying.
+  // If the user receives either of those messages, and does not adjust their
+  // clock, then the result will be (typically) be cached until the expiration
+  // TTL.
+  //
+  // This algorithm is only problematic if the user consistently keeps adjusting
+  // their clock backwards in increments smaller than the expiration TTL, in
+  // which case, cached elements continue to be added. However, because the
+  // cache has a fixed upper bound, if no entries are expired, a 'random' entry
+  // will be, thus keeping the memory constraints bounded over time.
+  return now.verification_time >= expiration.verification_time &&

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

DCHECK that now.verification_time == now.expiration_time?

+         now.verification_time < expiration.expiration_time;
+}
+
 // HandleResult is called by CertVerifierWorker on the origin message loop.
 // It deletes CertVerifierJob.
 void MultiThreadedCertVerifier::HandleResult(
@@ -427,8 +469,9 @@ void MultiThreadedCertVerifier::HandleResult(
   CachedResult cached_result;
   cached_result.error = error;
   cached_result.result = verify_result;
-  cache_.Put(key, cached_result, base::TimeTicks::Now(),
-             base::TimeDelta::FromSeconds(kTTLSecs));
+  base::Time now = base::Time::Now();
+  cache_.Put(key, cached_result, ValidityRange(now),
+             ValidityRange(now, now + base::TimeDelta::FromSeconds(kTTLSecs)));

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

If the result is successful, shouldn't we take min(kTTLSecs,
cert->valid_expiry()) ?

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

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.

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

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.

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

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

 
   std::map<RequestParams, CertVerifierJob*>::iterator j;
   j = inflight_.find(key);