Remove certificates from Channel ID

net/ssl/channel_id_service.cc

Index: net/ssl/channel_id_service.cc
diff --git a/net/ssl/channel_id_service.cc b/net/ssl/channel_id_service.cc
index 7fcbc539b32187d0e1abcacc140a62eb88672876..0010eb52905c67b8e4eff7f504c6d86ef87b1e9d 100644
--- a/net/ssl/channel_id_service.cc
+++ b/net/ssl/channel_id_service.cc
@@ -35,12 +35,6 @@ namespace net {
 
 namespace {
 
-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;
-
 // Used by the GetDomainBoundCertResult histogram to record the final
 // outcome of each GetChannelID or GetOrCreateChannelID call.
 // Do not re-use values.
@@ -54,7 +48,7 @@ enum GetChannelIDResult {
   ASYNC_CANCELLED = 2,
   // Cert generation failed.
   ASYNC_FAILURE_KEYGEN = 3,
-  ASYNC_FAILURE_CREATE_CERT = 4,
+  // Result code 4 was removed (ASYNC_FAILURE_CREATE_CERT)
   ASYNC_FAILURE_EXPORT_KEY = 5,
   ASYNC_FAILURE_UNKNOWN = 6,
   // GetChannelID or GetOrCreateChannelID was called with
@@ -88,45 +82,21 @@ void RecordGetChannelIDTime(base::TimeDelta request_time) {
 // unjoined thread, due to relying on a non-leaked LazyInstance
 scoped_ptr<ChannelIDStore::ChannelID> GenerateChannelID(
     const std::string& server_identifier,
-    uint32 serial_number,
     int* error) {
   scoped_ptr<ChannelIDStore::ChannelID> result;
 
   base::TimeTicks start = base::TimeTicks::Now();
   base::Time not_valid_before = base::Time::Now();
-  base::Time not_valid_after =
-      not_valid_before + base::TimeDelta::FromDays(kValidityPeriodInDays);
-  std::string der_cert;
-  std::vector<uint8> private_key_info;
-  scoped_ptr<crypto::ECPrivateKey> key;
-  if (!x509_util::CreateKeyAndChannelIDEC(server_identifier,
-                                          serial_number,
-                                          not_valid_before,
-                                          not_valid_after,
-                                          &key,
-                                          &der_cert)) {
-    DLOG(ERROR) << "Unable to create x509 cert for client";
-    *error = ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED;
-    return result.Pass();
-  }
+  scoped_ptr<crypto::ECPrivateKey> key(crypto::ECPrivateKey::Create());
 
-  if (!key->ExportEncryptedPrivateKey(ChannelIDService::kEPKIPassword,
-                                      1, &private_key_info)) {
-    DLOG(ERROR) << "Unable to export private key";
-    *error = ERR_PRIVATE_KEY_EXPORT_FAILED;
+  if (!key) {
+    DLOG(ERROR) << "Unable to create channel ID key pair";
+    *error = ERR_KEY_GENERATION_FAILED;
     return result.Pass();
   }
 
-  // TODO(rkn): Perhaps ExportPrivateKey should be changed to output a
-  // std::string* to prevent this copying.
-  std::string key_out(private_key_info.begin(), private_key_info.end());
-
-  result.reset(new ChannelIDStore::ChannelID(
-      server_identifier,
-      not_valid_before,
-      not_valid_after,
-      key_out,
-      der_cert));
+  result.reset(new ChannelIDStore::ChannelID(server_identifier,
+                                             not_valid_before, key.get()));

[Ryan Sleevi, 2015/05/08 23:43:13]

Should this be renamed to |creation_date|?

[nharper, 2015/05/11 21:26:44]

Renamed to creation_time.

   UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GenerateCertTime",
                              base::TimeTicks::Now() - start,
                              base::TimeDelta::FromMilliseconds(1),
@@ -143,27 +113,19 @@ class ChannelIDServiceRequest {
  public:
   ChannelIDServiceRequest(base::TimeTicks request_start,
                           const CompletionCallback& callback,
-                          std::string* private_key,
-                          std::string* cert)
-      : request_start_(request_start),
-        callback_(callback),
-        private_key_(private_key),
-        cert_(cert) {
-  }
+                          scoped_ptr<crypto::ECPrivateKey>* key)
+      : request_start_(request_start), callback_(callback), key_(key) {}
 
   // Ensures that the result callback will never be made.
   void Cancel() {
     RecordGetChannelIDResult(ASYNC_CANCELLED);
     callback_.Reset();
-    private_key_ = NULL;
-    cert_ = NULL;
+    key_->reset();

[Ryan Sleevi, 2015/05/08 23:43:13]

Why is this explicitly necessary? Shouldn't the dtor handle this? Are there
mutability concerns? What breaks if this isn't set - and why?

[nharper, 2015/05/11 21:26:44]

I don't know why it would be necessary - I only put it in to match the previous
style of setting private_key_ and cert_ to NULL. (Perhaps it was to keep the
object in a consistent state if it was Cancel()ed without being destroyed?)
Removing it seems to be fine.

   }
 
-  // Copies the contents of |private_key| and |cert| to the caller's output
-  // arguments and calls the callback.
-  void Post(int error,
-            const std::string& private_key,
-            const std::string& cert) {
+  // Copies the contents of |key| to the caller's output argument and calls the
+  // callback.
+  void Post(int error, crypto::ECPrivateKey* key) {
     switch (error) {
       case OK: {
         base::TimeDelta request_time = base::TimeTicks::Now() - request_start_;
@@ -179,9 +141,6 @@ class ChannelIDServiceRequest {
       case ERR_KEY_GENERATION_FAILED:
         RecordGetChannelIDResult(ASYNC_FAILURE_KEYGEN);
         break;
-      case ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED:
-        RecordGetChannelIDResult(ASYNC_FAILURE_CREATE_CERT);
-        break;
       case ERR_PRIVATE_KEY_EXPORT_FAILED:
         RecordGetChannelIDResult(ASYNC_FAILURE_EXPORT_KEY);
         break;
@@ -193,8 +152,8 @@ class ChannelIDServiceRequest {
         break;
     }
     if (!callback_.is_null()) {
-      *private_key_ = private_key;
-      *cert_ = cert;
+      if (key)
+        key_->reset(key->Copy());
       callback_.Run(error);
     }
     delete this;
@@ -205,8 +164,7 @@ class ChannelIDServiceRequest {
  private:
   base::TimeTicks request_start_;
   CompletionCallback callback_;
-  std::string* private_key_;
-  std::string* cert_;
+  scoped_ptr<crypto::ECPrivateKey>* key_;
 };
 
 // ChannelIDServiceWorker runs on a worker thread and takes care of the
@@ -223,7 +181,6 @@ class ChannelIDServiceWorker {
       const std::string& server_identifier,
       const WorkerDoneCallback& callback)
       : server_identifier_(server_identifier),
-        serial_number_(base::RandInt(0, std::numeric_limits<int>::max())),
         origin_loop_(base::MessageLoopProxy::current()),
         callback_(callback) {
   }
@@ -244,7 +201,7 @@ class ChannelIDServiceWorker {
     // Runs on a worker thread.
     int error = ERR_FAILED;
     scoped_ptr<ChannelIDStore::ChannelID> cert =

[Ryan Sleevi, 2015/05/08 23:43:13]

This variable name is no longer correct, is it?

[nharper, 2015/05/11 21:26:44]

Done.

-        GenerateChannelID(server_identifier_, serial_number_, &error);
+        GenerateChannelID(server_identifier_, &error);
     DVLOG(1) << "GenerateCert " << server_identifier_ << " returned " << error;

[Ryan Sleevi, 2015/05/08 23:43:13]

That looks wonky :)

Just delete the DVLOG IMO

[nharper, 2015/05/11 21:26:44]

Done.

 #if !defined(USE_OPENSSL)
     // Detach the thread from NSPR.
@@ -262,9 +219,6 @@ class ChannelIDServiceWorker {
   }
 
   const std::string server_identifier_;
-  // Note that serial_number_ must be initialized on a non-worker thread
-  // (see documentation for GenerateCert).
-  uint32 serial_number_;
   scoped_refptr<base::SequencedTaskRunner> origin_loop_;
   WorkerDoneCallback callback_;
 
@@ -291,24 +245,20 @@ class ChannelIDServiceJob {
     requests_.push_back(request);
   }
 
-  void HandleResult(int error,
-                    const std::string& private_key,
-                    const std::string& cert) {
-    PostAll(error, private_key, cert);
+  void HandleResult(int error, crypto::ECPrivateKey* key) {
+    PostAll(error, key);
   }
 
   bool CreateIfMissing() const { return create_if_missing_; }
 
  private:
-  void PostAll(int error,
-               const std::string& private_key,
-               const std::string& cert) {
+  void PostAll(int error, crypto::ECPrivateKey* key) {
     std::vector<ChannelIDServiceRequest*> requests;
     requests_.swap(requests);
 
     for (std::vector<ChannelIDServiceRequest*>::iterator
          i = requests.begin(); i != requests.end(); i++) {
-      (*i)->Post(error, private_key, cert);
+      (*i)->Post(error, key);
       // Post() causes the ChannelIDServiceRequest to delete itself.
     }
   }
@@ -370,14 +320,10 @@ ChannelIDService::ChannelIDService(
     : channel_id_store_(channel_id_store),
       task_runner_(task_runner),
       requests_(0),
-      cert_store_hits_(0),
+      key_store_hits_(0),
       inflight_joins_(0),
       workers_created_(0),
       weak_ptr_factory_(this) {
-  base::Time start = base::Time::Now();
-  base::Time end = start + base::TimeDelta::FromDays(
-      kValidityPeriodInDays + kSystemTimeValidityBufferInDays);
-  is_system_time_valid_ = x509_util::IsSupportedValidityRange(start, end);
 }
 
 ChannelIDService::~ChannelIDService() {
@@ -396,15 +342,14 @@ std::string ChannelIDService::GetDomainForHost(const std::string& host) {
 
 int ChannelIDService::GetOrCreateChannelID(
     const std::string& host,
-    std::string* private_key,
-    std::string* cert,
+    scoped_ptr<crypto::ECPrivateKey>* key,
     const CompletionCallback& callback,
     RequestHandle* out_req) {
   DVLOG(1) << __FUNCTION__ << " " << host;
   DCHECK(CalledOnValidThread());
   base::TimeTicks request_start = base::TimeTicks::Now();
 
-  if (callback.is_null() || !private_key || !cert || host.empty()) {
+  if (callback.is_null() || !key || host.empty()) {
     RecordGetChannelIDResult(INVALID_ARGUMENT);
     return ERR_INVALID_ARGUMENT;
   }
@@ -419,13 +364,13 @@ int ChannelIDService::GetOrCreateChannelID(
 
   // See if a request for the same domain is currently in flight.
   bool create_if_missing = true;
-  if (JoinToInFlightRequest(request_start, domain, private_key, cert,
-                            create_if_missing, callback, out_req)) {
+  if (JoinToInFlightRequest(request_start, domain, key, create_if_missing,
+                            callback, out_req)) {
     return ERR_IO_PENDING;
   }
 
-  int err = LookupChannelID(request_start, domain, private_key, cert,
-                                  create_if_missing, callback, out_req);
+  int err = LookupChannelID(request_start, domain, key, create_if_missing,
+                            callback, out_req);
   if (err == ERR_FILE_NOT_FOUND) {
     // Sync lookup did not find a valid cert.  Start generating a new one.
     workers_created_++;
@@ -444,11 +389,9 @@ int ChannelIDService::GetOrCreateChannelID(
     inflight_[domain] = job;
 
     ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start,
-        base::Bind(&RequestHandle::OnRequestComplete,
-                   base::Unretained(out_req)),
-        private_key,
-        cert);
+        request_start, base::Bind(&RequestHandle::OnRequestComplete,
+                                  base::Unretained(out_req)),
+        key);
     job->AddRequest(request);
     out_req->RequestStarted(this, request, callback);
     return ERR_IO_PENDING;
@@ -457,17 +400,15 @@ int ChannelIDService::GetOrCreateChannelID(
   return err;
 }
 
-int ChannelIDService::GetChannelID(
-    const std::string& host,
-    std::string* private_key,
-    std::string* cert,
-    const CompletionCallback& callback,
-    RequestHandle* out_req) {
+int ChannelIDService::GetChannelID(const std::string& host,
+                                   scoped_ptr<crypto::ECPrivateKey>* key,
+                                   const CompletionCallback& callback,
+                                   RequestHandle* out_req) {
   DVLOG(1) << __FUNCTION__ << " " << host;
   DCHECK(CalledOnValidThread());
   base::TimeTicks request_start = base::TimeTicks::Now();
 
-  if (callback.is_null() || !private_key || !cert || host.empty()) {
+  if (callback.is_null() || !key || host.empty()) {
     RecordGetChannelIDResult(INVALID_ARGUMENT);
     return ERR_INVALID_ARGUMENT;
   }
@@ -482,22 +423,20 @@ int ChannelIDService::GetChannelID(
 
   // See if a request for the same domain currently in flight.
   bool create_if_missing = false;
-  if (JoinToInFlightRequest(request_start, domain, private_key, cert,
-                            create_if_missing, callback, out_req)) {
+  if (JoinToInFlightRequest(request_start, domain, key, create_if_missing,
+                            callback, out_req)) {
     return ERR_IO_PENDING;
   }
 
-  int err = LookupChannelID(request_start, domain, private_key, cert,
-                            create_if_missing, callback, out_req);
+  int err = LookupChannelID(request_start, domain, key, create_if_missing,
+                            callback, out_req);
   return err;
 }
 
 void ChannelIDService::GotChannelID(
     int err,
     const std::string& server_identifier,
-    base::Time expiration_time,
-    const std::string& key,
-    const std::string& cert) {
+    const scoped_ptr<crypto::ECPrivateKey> key) {
   DCHECK(CalledOnValidThread());
 
   std::map<std::string, ChannelIDServiceJob*>::iterator j;
@@ -509,17 +448,17 @@ void ChannelIDService::GotChannelID(
 
   if (err == OK) {
     // Async DB lookup found a valid cert.
-    DVLOG(1) << "Cert store had valid cert for " << server_identifier;
-    cert_store_hits_++;
+    DVLOG(1) << "Cert store had valid key for " << server_identifier;

[Ryan Sleevi, 2015/05/08 23:43:14]

Can nuke this as well

[nharper, 2015/05/11 21:26:44]

Done.

+    key_store_hits_++;
     // ChannelIDServiceRequest::Post will do the histograms and stuff.
-    HandleResult(OK, server_identifier, key, cert);
+    HandleResult(OK, server_identifier, key.get());
     return;
   }
   // Async lookup failed or the certificate was missing. Return the error
   // directly, unless the certificate was missing and a request asked to create
   // one.
   if (err != ERR_FILE_NOT_FOUND || !j->second->CreateIfMissing()) {
-    HandleResult(err, server_identifier, key, cert);
+    HandleResult(err, server_identifier, key.get());
     return;
   }
   // At least one request asked to create a cert => start generating a new one.
@@ -531,10 +470,7 @@ void ChannelIDService::GotChannelID(
   if (!worker->Start(task_runner_)) {
     // TODO(rkn): Log to the NetLog.
     LOG(ERROR) << "ChannelIDServiceWorker couldn't be started.";
-    HandleResult(ERR_INSUFFICIENT_RESOURCES,
-                 server_identifier,
-                 std::string(),
-                 std::string());
+    HandleResult(ERR_INSUFFICIENT_RESOURCES, server_identifier, nullptr);
   }
 }
 
@@ -550,30 +486,22 @@ void ChannelIDService::CancelRequest(ChannelIDServiceRequest* req) {
 void ChannelIDService::GeneratedChannelID(
     const std::string& server_identifier,
     int error,
-    scoped_ptr<ChannelIDStore::ChannelID> cert) {
+    scoped_ptr<ChannelIDStore::ChannelID> channel_id) {
   DCHECK(CalledOnValidThread());
 
   if (error == OK) {
-    // TODO(mattm): we should just Pass() the cert object to
-    // SetChannelID().
-    channel_id_store_->SetChannelID(
-        cert->server_identifier(),
-        cert->creation_time(),
-        cert->expiration_time(),
-        cert->private_key(),
-        cert->cert());
-
-    HandleResult(error, server_identifier, cert->private_key(), cert->cert());
+    scoped_ptr<crypto::ECPrivateKey> key(channel_id->key()->Copy());
+    channel_id_store_->SetChannelID(channel_id.Pass());
+
+    HandleResult(error, server_identifier, key.get());
   } else {
-    HandleResult(error, server_identifier, std::string(), std::string());
+    HandleResult(error, server_identifier, nullptr);
   }
 }
 
-void ChannelIDService::HandleResult(
-    int error,
-    const std::string& server_identifier,
-    const std::string& private_key,
-    const std::string& cert) {
+void ChannelIDService::HandleResult(int error,
+                                    const std::string& server_identifier,
+                                    crypto::ECPrivateKey* key) {

[Ryan Sleevi, 2015/05/08 23:43:13]

Is there any risk of |key| being invalidated? Would it make more sense to force
the callers of HandleResult() to .Pass() the scoped_ptr<crypto::ECPrivateKey>?

My fear would be that |key| refers to some member variable of some job that ends
up deleted. I suppose this is technically a risk for |server_identifier| as well
(because it's a const-ref and thus may be a pointer to a member), but I'm just
curious if forcing an ownership transfer makes for a cleaner experience?

(It may not, just curious your judgement now that you know this code)

[nharper, 2015/05/11 21:26:44]

I'm not concerned about |key| being invalidated. HandleResult only gets called
from ChannelIDService::GotChannelID and ChannelIDService::GeneratedChannelID. In
both of those cases, the underlying crypto::ECPrivateKey object is owned by the
calling method - in GotChannelID it's owned by the scoped_ptr that's passed in
as a param, and in GeneratedChannelID it's owned by a scoped_ptr that's created
in the method.

Ultimately, I decided to refactor ChannelIDService::HandleResult,
ChannelIDServiceJob::HandleResult, ChannelIDServiceJob::PostAll, and
ChannelIDServiceRequest::Post to take scoped_ptrs instead of raw pointers, to
make ownership of objects more clear. The reason why I originally used a raw
pointer instead of scoped_ptr has to do with how ChannelIDServiceJob::PostAll()
and ChannelIDServiceRequest::Post() work. PostAll() gets called by HandleResult,
and then calls Post() on all of the Job's Requests. This makes it necessary to
either use a raw pointer here or to copy the object for all of the calls to
Post. The other complication is that it is perfectly valid to call HandleResult
with nullptr, so when the object is copied, it needs to check for null.

tl;dr: there was a reason for using a raw pointer, but it's cleaner if I use a
scoped_ptr, so I'm changing it to use a scoped_ptr.

   DCHECK(CalledOnValidThread());
 
   std::map<std::string, ChannelIDServiceJob*>::iterator j;
@@ -585,15 +513,14 @@ void ChannelIDService::HandleResult(
   ChannelIDServiceJob* job = j->second;
   inflight_.erase(j);
 
-  job->HandleResult(error, private_key, cert);
+  job->HandleResult(error, key);
   delete job;
 }
 
 bool ChannelIDService::JoinToInFlightRequest(
     const base::TimeTicks& request_start,
     const std::string& domain,
-    std::string* private_key,
-    std::string* cert,
+    scoped_ptr<crypto::ECPrivateKey>* key,
     bool create_if_missing,
     const CompletionCallback& callback,
     RequestHandle* out_req) {
@@ -607,11 +534,9 @@ bool ChannelIDService::JoinToInFlightRequest(
     inflight_joins_++;
 
     ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start,
-        base::Bind(&RequestHandle::OnRequestComplete,
-                   base::Unretained(out_req)),
-        private_key,
-        cert);
+        request_start, base::Bind(&RequestHandle::OnRequestComplete,
+                                  base::Unretained(out_req)),
+        key);
     job->AddRequest(request, create_if_missing);
     out_req->RequestStarted(this, request, callback);
     return true;
@@ -619,29 +544,21 @@ bool ChannelIDService::JoinToInFlightRequest(
   return false;
 }
 
-int ChannelIDService::LookupChannelID(
-    const base::TimeTicks& request_start,
-    const std::string& domain,
-    std::string* private_key,
-    std::string* cert,
-    bool create_if_missing,
-    const CompletionCallback& callback,
-    RequestHandle* out_req) {
-  // Check if a domain bound cert already exists for this domain. Note that
-  // |expiration_time| is ignored, and expired certs are considered valid.
-  base::Time expiration_time;
+int ChannelIDService::LookupChannelID(const base::TimeTicks& request_start,
+                                      const std::string& domain,
+                                      scoped_ptr<crypto::ECPrivateKey>* key,
+                                      bool create_if_missing,
+                                      const CompletionCallback& callback,
+                                      RequestHandle* out_req) {
+  // Check if a channel ID key already exists for this domain.
   int err = channel_id_store_->GetChannelID(
-      domain,
-      &expiration_time  /* ignored */,
-      private_key,
-      cert,
-      base::Bind(&ChannelIDService::GotChannelID,
-                 weak_ptr_factory_.GetWeakPtr()));
+      domain, key, base::Bind(&ChannelIDService::GotChannelID,
+                              weak_ptr_factory_.GetWeakPtr()));
 
   if (err == OK) {
     // Sync lookup found a valid cert.
-    DVLOG(1) << "Cert store had valid cert for " << domain;
-    cert_store_hits_++;
+    DVLOG(1) << "Cert store had valid key for " << domain;
+    key_store_hits_++;
     RecordGetChannelIDResult(SYNC_SUCCESS);
     base::TimeDelta request_time = base::TimeTicks::Now() - request_start;
     UMA_HISTOGRAM_TIMES("DomainBoundCerts.GetCertTimeSync", request_time);
@@ -655,11 +572,9 @@ int ChannelIDService::LookupChannelID(
     inflight_[domain] = job;
 
     ChannelIDServiceRequest* request = new ChannelIDServiceRequest(
-        request_start,
-        base::Bind(&RequestHandle::OnRequestComplete,
-                   base::Unretained(out_req)),
-        private_key,
-        cert);
+        request_start, base::Bind(&RequestHandle::OnRequestComplete,
+                                  base::Unretained(out_req)),
+        key);
     job->AddRequest(request);
     out_req->RequestStarted(this, request, callback);
     return ERR_IO_PENDING;