Don't expire certs used as channel IDs.

net/ssl/server_bound_cert_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ssl/server_bound_cert_service.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 28 matching lines @@
 const int kValidityPeriodInDays = 365;
 // When we check the system time, we add this many days to the end of the check
 // so the result will still hold even after chrome has been running for a
 // while.
 const int kSystemTimeValidityBufferInDays = 90;
 
 bool IsSupportedCertType(uint8 type) {
   switch(type) {
     case CLIENT_CERT_ECDSA_SIGN:
       return true;
-    // If we add any more supported types, CertIsValid will need to be updated
-    // to check that the returned type matches one of the requested types.
     default:
       return false;
   }
 }
 
-bool CertIsValid(const std::string& domain,
-                 SSLClientCertType type,
-                 base::Time expiration_time) {
-  if (expiration_time < base::Time::Now()) {
-    DVLOG(1) << "Cert store had expired cert for " << domain;
-    return false;
-  } else if (!IsSupportedCertType(type)) {
-    DVLOG(1) << "Cert store had cert of wrong type " << type << " for "
-             << domain;
-    return false;
-  }
-  return true;
-}
-
 // Used by the GetDomainBoundCertResult histogram to record the final
 // outcome of each GetDomainBoundCert call.  Do not re-use values.
 enum GetCertResult {
   // Synchronously found and returned an existing domain bound cert.
   SYNC_SUCCESS = 0,
   // Retrieved or generated and returned a domain bound cert asynchronously.
   ASYNC_SUCCESS = 1,
   // Retrieval/generation request was cancelled before the cert generation
   // completed.
   ASYNC_CANCELLED = 2,
@@ skipping 328 matching lines @@
     ServerBoundCertStore* server_bound_cert_store,
     const scoped_refptr<base::TaskRunner>& task_runner)
     : server_bound_cert_store_(server_bound_cert_store),
       task_runner_(task_runner),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
       requests_(0),
       cert_store_hits_(0),
       inflight_joins_(0) {
   base::Time start = base::Time::Now();
   base::Time end = start + base::TimeDelta::FromDays(
-          kValidityPeriodInDays + kSystemTimeValidityBufferInDays);
+      kValidityPeriodInDays + kSystemTimeValidityBufferInDays);
   is_system_time_valid_ = x509_util::IsSupportedValidityRange(start, end);
 }
 
 ServerBoundCertService::~ServerBoundCertService() {
   STLDeleteValues(&inflight_);
 }
 
 //static
 std::string ServerBoundCertService::GetDomainForHost(const std::string& host) {
   std::string domain =
@@ skipping 71 matching lines @@
         request_start,
         base::Bind(&RequestHandle::OnRequestComplete,
                    base::Unretained(out_req)),
         type, private_key, cert);
     job->AddRequest(request);
     out_req->RequestStarted(this, request, callback);
     return ERR_IO_PENDING;
   }
 
   // Check if a domain bound cert of an acceptable type already exists for this
-  // domain, and that it has not expired.
+  // domain. Note that |expiration_time| is ignored, and expired certs are
+  // considered valid.
   base::Time expiration_time;
   if (server_bound_cert_store_->GetServerBoundCert(
       domain,
       type,
-      &expiration_time,
+      &expiration_time /* ignored */,
       private_key,
       cert,
       base::Bind(&ServerBoundCertService::GotServerBoundCert,
                  weak_ptr_factory_.GetWeakPtr()))) {
-    if (*type != CLIENT_CERT_INVALID_TYPE) {
-      // Sync lookup found a cert.
-      if (CertIsValid(domain, *type, expiration_time)) {
-        DVLOG(1) << "Cert store had valid cert for " << domain
-                 << " of type " << *type;
-        cert_store_hits_++;
-        RecordGetDomainBoundCertResult(SYNC_SUCCESS);
-        base::TimeDelta request_time = base::TimeTicks::Now() - request_start;
-        UMA_HISTOGRAM_TIMES("DomainBoundCerts.GetCertTimeSync", request_time);
-        RecordGetCertTime(request_time);
-        return OK;
-      }
+    if (type && IsSupportedCertType(*type)) {

[mattm, 2013/04/01 23:03:12]

Can remove the "type && " check, since it is already assumed valid when we
passed it to GetServerBoundCert.  (If you really wanted to, you could add it to
the invalid argument check at the start of the function)

[thaidn_google, 2013/04/01 23:56:14]

Done.

+      // Sync lookup found a valid cert.
+      DVLOG(1) << "Cert store had valid cert for " << domain
+               << " of type " << *type;
+      cert_store_hits_++;
+      RecordGetDomainBoundCertResult(SYNC_SUCCESS);
+      base::TimeDelta request_time = base::TimeTicks::Now() - request_start;
+      UMA_HISTOGRAM_TIMES("DomainBoundCerts.GetCertTimeSync", request_time);
+      RecordGetCertTime(request_time);
+      return OK;
     }
 
-    // Sync lookup did not find a cert, or it found an expired one.  Start
-    // generating a new one.
+    // Sync lookup did not find a valid cert.  Start generating a new one.
     ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
         domain,
         preferred_type,
         base::Bind(&ServerBoundCertService::GeneratedServerBoundCert,
                    weak_ptr_factory_.GetWeakPtr()));
     if (!worker->Start(task_runner_)) {
       delete worker;
       // TODO(rkn): Log to the NetLog.
       LOG(ERROR) << "ServerBoundCertServiceWorker couldn't be started.";
       RecordGetDomainBoundCertResult(WORKER_FAILURE);
@@ skipping 24 matching lines @@
   DCHECK(CalledOnValidThread());
 
   std::map<std::string, ServerBoundCertServiceJob*>::iterator j;
   j = inflight_.find(server_identifier);
   if (j == inflight_.end()) {
     NOTREACHED();
     return;
   }
   ServerBoundCertServiceJob* job = j->second;
 
-  if (type != CLIENT_CERT_INVALID_TYPE) {
-    // Async DB lookup found a cert.
-    if (CertIsValid(server_identifier, type, expiration_time)) {
-      DVLOG(1) << "Cert store had valid cert for " << server_identifier
-               << " of type " << type;
-      cert_store_hits_++;
-      // ServerBoundCertServiceRequest::Post will do the histograms and stuff.
-      HandleResult(OK, server_identifier, type, key, cert);
-      return;
-    }
+  if (IsSupportedCertType(type)) {
+    // Async DB lookup found a valid cert.
+    DVLOG(1) << "Cert store had valid cert for " << server_identifier
+             << " of type " << type;
+    cert_store_hits_++;
+    // ServerBoundCertServiceRequest::Post will do the histograms and stuff.
+    HandleResult(OK, server_identifier, type, key, cert);
+    return;
   }
 
-  // Async lookup did not find a cert, or it found an expired one.  Start
-  // generating a new one.
+  // Async lookup did not find a valid cert. Start generating a new one.
   ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
       server_identifier,
       job->type(),
       base::Bind(&ServerBoundCertService::GeneratedServerBoundCert,
                  weak_ptr_factory_.GetWeakPtr()));
   if (!worker->Start(task_runner_)) {
     delete worker;
     // TODO(rkn): Log to the NetLog.
     LOG(ERROR) << "ServerBoundCertServiceWorker couldn't be started.";
     HandleResult(ERR_INSUFFICIENT_RESOURCES, server_identifier,
@@ skipping 50 matching lines @@
 
   job->HandleResult(error, type, private_key, cert);
   delete job;
 }
 
 int ServerBoundCertService::cert_count() {
   return server_bound_cert_store_->GetCertCount();
 }
 
 }  // namespace net