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 9bc21794a9c11b70f066fe60ad09aefb3a183587..9ac7c3249a35b4c429304ccc92368f06dc7bc6a9 100644
--- a/net/ssl/channel_id_service.cc
+++ b/net/ssl/channel_id_service.cc
@@ -88,45 +88,27 @@ 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());
+  std::string private_key_out;
+  std::string public_key_out;
+  *error = ExportKeypair(key, &public_key_out, &private_key_out);
+  if (*error != OK)
+    return result.Pass();
 
   result.reset(new ChannelIDStore::ChannelID(
-      server_identifier,
-      not_valid_before,
-      not_valid_after,
-      key_out,
-      der_cert));
+      server_identifier, not_valid_before, private_key_out, public_key_out));
   UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GenerateCertTime",
                              base::TimeTicks::Now() - start,
                              base::TimeDelta::FromMilliseconds(1),
@@ -138,32 +120,71 @@ scoped_ptr<ChannelIDStore::ChannelID> GenerateChannelID(
 
 }  // namespace
 
+int CreateECPrivateKeyFromSerializedKey(
+    const std::string& public_key,
+    const std::string& private_key,
+    scoped_ptr<crypto::ECPrivateKey>* key_out) {
+  if (public_key.empty() || private_key.empty())
+    return ERR_UNEXPECTED;
+
+  std::vector<uint8> public_key_vector(public_key.size());
+  memcpy(&public_key_vector[0], public_key.data(), public_key.size());

[Ryan Sleevi, 2015/04/15 22:50:02]

DANGER: What if |public_key| is .empty()? This would be a fatal thing
(&public_key_vector[0] is undefined then)

It does seem like simply "fixing" ECPrivateKey to use base::StringPiece resolves
this, right? Do you want to go do that before this complexity?

[nharper, 2015/04/25 02:59:19]

If public_key or private_key is .empty(), then this function doesn't do anything
to key_out and returns ERR_UNEXPECTED (line 128).

My main concern with the complexity here is the memcpy - the rest is just ugly
conversions.

+  std::vector<uint8> private_key_vector(private_key.size());
+  memcpy(&private_key_vector[0], private_key.data(), private_key.size());
+  crypto::ECPrivateKey* key =
+      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
+          ChannelIDService::kEPKIPassword, private_key_vector,
+          public_key_vector);
+  if (key == nullptr)
+    return ERR_UNEXPECTED;
+  key_out->reset(key);
+  return OK;
+}
+
+int ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,

[Ryan Sleevi, 2015/04/15 22:50:02]

pass as const-ref

But really, you should be sending in a naked key, right?

crypto::ECPrivateKey* key

[nharper, 2015/04/25 02:59:19]

Changed to const-ref. Does the style guide have input as to whether I should use
const scoped_ptr<T>& vs T*? In the only callsite of this function (outside of
unittests), the key is a scoped_ptr<crypto::ECPrivateKey> instead of a raw
pointer (to facilitate deleting the object pointed to appropriately), and I
don't think there's any disagreement about using a scoped_ptr there. In the
unittests, using a scoped_ptr is handy because it takes care of deleting the
object when it goes out of scope at the end of the test case.

My reasoning for using scoped_ptr<crypto::ECPrivateKey>& here instead of
crypto::ECPrivateKey* is because it makes it easier on the callsite (since all
callers are using a scoped_ptr) - this way the caller doesn't have to call
.get() on the scoped_ptr.

I'm not that familiar with working with scoped_ptrs, so if convention suggests
doing things differently (or if I'm missing something), I'd be glad to change
it.

+                  std::string* public_key,
+                  std::string* private_key) {
+  std::vector<uint8> public_key_vector;
+  std::vector<uint8> private_key_vector;
+
+  if (!key)
+    return ERR_UNEXPECTED;
+
+  if (!key->ExportEncryptedPrivateKey(ChannelIDService::kEPKIPassword, 1,
+                                      &private_key_vector) ||
+      !key->ExportPublicKey(&public_key_vector)) {
+    DLOG(ERROR) << "Unable to export key";
+    return ERR_PRIVATE_KEY_EXPORT_FAILED;
+  }
+
+  // TODO(rkn): Perhaps ExportPrivateKey should be changed to output a
+  // std::string* to prevent this copying.
+  *public_key = std::string(public_key_vector.begin(), public_key_vector.end());

[Ryan Sleevi, 2015/04/15 22:50:02]

Does this work across platforms without throwing conversion errors about
signed/unsigned conversions?

[nharper, 2015/04/25 02:59:20]

This code was copied from the lhs of this diff (see line 120), so it should be
fine.

+  *private_key =
+      std::string(private_key_vector.begin(), private_key_vector.end());
+  return OK;
+}

[Ryan Sleevi, 2015/04/15 22:50:02]

Should match order in .h

[nharper, 2015/04/25 02:59:19]

Done.

+
 // Represents the output and result callback of a request.
 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();
   }
 
   // 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) {
+            const std::string& public_key) {
     switch (error) {
       case OK: {
         base::TimeDelta request_time = base::TimeTicks::Now() - request_start_;
@@ -193,8 +214,10 @@ class ChannelIDServiceRequest {
         break;
     }
     if (!callback_.is_null()) {
-      *private_key_ = private_key;
-      *cert_ = cert;
+      int err =
+          CreateECPrivateKeyFromSerializedKey(public_key, private_key, key_);
+      if (error == OK && err != OK)
+        error = err;
       callback_.Run(error);
     }
     delete this;
@@ -205,8 +228,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 +245,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 +265,7 @@ class ChannelIDServiceWorker {
     // Runs on a worker thread.
     int error = ERR_FAILED;
     scoped_ptr<ChannelIDStore::ChannelID> cert =
-        GenerateChannelID(server_identifier_, serial_number_, &error);
+        GenerateChannelID(server_identifier_, &error);
     DVLOG(1) << "GenerateCert " << server_identifier_ << " returned " << error;
 #if defined(USE_NSS)
     // Detach the thread from NSPR.
@@ -262,9 +283,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_;
 
@@ -293,8 +311,8 @@ class ChannelIDServiceJob {
 
   void HandleResult(int error,
                     const std::string& private_key,
-                    const std::string& cert) {
-    PostAll(error, private_key, cert);
+                    const std::string& public_key) {
+    PostAll(error, private_key, public_key);
   }
 
   bool CreateIfMissing() const { return create_if_missing_; }
@@ -302,13 +320,13 @@ class ChannelIDServiceJob {
  private:
   void PostAll(int error,
                const std::string& private_key,
-               const std::string& cert) {
+               const std::string& public_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, private_key, public_key);
       // Post() causes the ChannelIDServiceRequest to delete itself.
     }
   }
@@ -370,7 +388,7 @@ 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) {
@@ -396,15 +414,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;
   }
@@ -417,15 +434,18 @@ int ChannelIDService::GetOrCreateChannelID(
 
   requests_++;
 
+  std::string public_key;
+  std::string private_key;

[Ryan Sleevi, 2015/04/15 22:50:02]

These two are unused, right?

[nharper, 2015/04/25 02:59:19]

Correct. Removed.

+
   // 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 +464,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 +475,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 +498,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) {
+void ChannelIDService::GotChannelID(int err,
+                                    const std::string& server_identifier,
+                                    const std::string& private_key,
+                                    const std::string& public_key) {
   DCHECK(CalledOnValidThread());
 
   std::map<std::string, ChannelIDServiceJob*>::iterator j;
@@ -509,17 +523,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;
+    key_store_hits_++;
     // ChannelIDServiceRequest::Post will do the histograms and stuff.
-    HandleResult(OK, server_identifier, key, cert);
+    HandleResult(OK, server_identifier, private_key, public_key);
     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, private_key, public_key);
     return;
   }
   // At least one request asked to create a cert => start generating a new one.
@@ -556,24 +570,21 @@ void ChannelIDService::GeneratedChannelID(
   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());
+    channel_id_store_->SetChannelID(cert->server_identifier(),
+                                    cert->creation_time(), cert->private_key(),
+                                    cert->public_key());
+
+    HandleResult(error, server_identifier, cert->private_key(),
+                 cert->public_key());
   } else {
     HandleResult(error, server_identifier, std::string(), std::string());
   }
 }
 
-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,
+                                    const std::string& private_key,
+                                    const std::string& public_key) {
   DCHECK(CalledOnValidThread());
 
   std::map<std::string, ChannelIDServiceJob*>::iterator j;
@@ -585,15 +596,14 @@ void ChannelIDService::HandleResult(
   ChannelIDServiceJob* job = j->second;
   inflight_.erase(j);
 
-  job->HandleResult(error, private_key, cert);
+  job->HandleResult(error, private_key, public_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 +617,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 +627,29 @@ 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) {
+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 domain bound cert already exists for this domain. Note that
   // |expiration_time| is ignored, and expired certs are considered valid.
-  base::Time expiration_time;
+  std::string public_key;
+  std::string private_key;
   int err = channel_id_store_->GetChannelID(
-      domain,
-      &expiration_time  /* ignored */,
-      private_key,
-      cert,
+      domain, &private_key, &public_key,
       base::Bind(&ChannelIDService::GotChannelID,
                  weak_ptr_factory_.GetWeakPtr()));
 
   if (err == OK) {
+    err = CreateECPrivateKeyFromSerializedKey(public_key, private_key, key);
+    if (err != OK) {
+      return err;
+    }
     // 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 +663,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;