Don't expire certs used as channel IDs.

BUG=224999

Review URL: https://chromiumcodereview.appspot.com/13130004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@191775 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 37 insertions, 53 deletions

diff --git a/net/ssl/server_bound_cert_service.cc b/net/ssl/server_bound_cert_service.cc
index bf77719..bed97cf 100644
--- a/net/ssl/server_bound_cert_service.cc
+++ b/net/ssl/server_bound_cert_service.cc
@@ -46,27 +46,11 @@ 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 {
@@ -415,7 +399,7 @@ ServerBoundCertService::ServerBoundCertService(
       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);
 }
 
@@ -507,32 +491,30 @@ int ServerBoundCertService::GetDomainBoundCert(
   }
 
   // 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 (IsSupportedCertType(*type)) {
+      // 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,
@@ -577,20 +559,17 @@ void ServerBoundCertService::GotServerBoundCert(
   }
   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(),
diff --git a/net/ssl/server_bound_cert_service.h b/net/ssl/server_bound_cert_service.h
index 734199e..6663fad 100644
--- a/net/ssl/server_bound_cert_service.h
+++ b/net/ssl/server_bound_cert_service.h
@@ -29,7 +29,14 @@ class ServerBoundCertServiceJob;
 class ServerBoundCertServiceRequest;
 class ServerBoundCertServiceWorker;
 
-// A class for creating and fetching server bound certs.
+// A class for creating and fetching server bound certs. These certs are used
+// to identify users' machines; their public keys are used as channel IDs in
+// http://tools.ietf.org/html/draft-balfanz-tls-channelid-00.
+// As a result although certs are set to be invalid after one year, we don't
+// actually expire them. Once generated, certs are valid as long as the users
+// want. Users can delete existing certs, and new certs will be generated
+// automatically.
+
 // Inherits from NonThreadSafe in order to use the function
 // |CalledOnValidThread|.
 class NET_EXPORT ServerBoundCertService
diff --git a/net/ssl/server_bound_cert_service_unittest.cc b/net/ssl/server_bound_cert_service_unittest.cc
index 928efc8..a718fa9 100644
--- a/net/ssl/server_bound_cert_service_unittest.cc
+++ b/net/ssl/server_bound_cert_service_unittest.cc
@@ -528,7 +528,7 @@ TEST_F(ServerBoundCertServiceTest, Expiration) {
   TestCompletionCallback callback;
   ServerBoundCertService::RequestHandle request_handle;
 
-  // Cert still valid - synchronous completion.
+  // Cert is valid - synchronous completion.
   SSLClientCertType type1;
   std::string private_key_info1, der_cert1;
   error = service_->GetDomainBoundCert(
@@ -541,20 +541,18 @@ TEST_F(ServerBoundCertServiceTest, Expiration) {
   EXPECT_STREQ("a", private_key_info1.c_str());
   EXPECT_STREQ("b", der_cert1.c_str());
 
-  // Cert expired - New cert will be generated, asynchronous completion.
+  // Expired cert is valid as well - synchronous completion.
   SSLClientCertType type2;
   std::string private_key_info2, der_cert2;
   error = service_->GetDomainBoundCert(
       "https://expired", types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
-  EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle.is_active());
-  error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
+  EXPECT_FALSE(request_handle.is_active());
   EXPECT_EQ(2, service_->cert_count());
   EXPECT_EQ(CLIENT_CERT_ECDSA_SIGN, type2);
-  EXPECT_LT(1U, private_key_info2.size());
-  EXPECT_LT(1U, der_cert2.size());
+  EXPECT_STREQ("c", private_key_info2.c_str());
+  EXPECT_STREQ("d", der_cert2.c_str());
 }
 
 #endif  // !defined(USE_OPENSSL)