Make CertificateProviderService vend ClientCertIdentities directly.

chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc

 // Copyright 2015 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 "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
 
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_piece.h"
 #include "base/task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 #include "net/base/net_errors.h"
-#include "net/ssl/client_key_store.h"
 
 namespace chromeos {
 
 namespace {
 
 void PostSignResultToTaskRunner(
     const scoped_refptr<base::TaskRunner>& target_task_runner,
     const net::SSLPrivateKey::SignCallback& callback,
     net::Error error,
     const std::vector<uint8_t>& signature) {
   target_task_runner->PostTask(FROM_HERE,
                                base::Bind(callback, error, signature));
 }
 
-void PostCertificatesToTaskRunner(
+void PostIdentitiesToTaskRunner(
     const scoped_refptr<base::TaskRunner>& target_task_runner,
-    const base::Callback<void(const net::CertificateList&)>& callback,
-    const net::CertificateList& certs) {
-  target_task_runner->PostTask(FROM_HERE, base::Bind(callback, certs));
+    const base::Callback<void(net::ClientCertIdentityList)>& callback,
+    net::ClientCertIdentityList certs) {
+  target_task_runner->PostTask(FROM_HERE,
+                               base::BindOnce(callback, std::move(certs)));

[davidben, 2017/06/16 00:28:25]

[The sheer amount of code in Chromium that's devoted to thread-hopping is kind
of mind-blowing.]

 }
 
 }  // namespace
 
-class CertificateProviderService::CertKeyProviderImpl
-    : public net::ClientKeyStore::CertKeyProvider {
- public:
-  // |certificate_map| must outlive this provider. |service| must be
-  // dereferenceable on |service_task_runner|.
-  // This provider may be accessed from any thread. Methods and destructor must
-  // never be called concurrently.
-  CertKeyProviderImpl(
-      const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
-      const base::WeakPtr<CertificateProviderService>& service,
-      certificate_provider::ThreadSafeCertificateMap* certificate_map);
-  ~CertKeyProviderImpl() override;
-
-  bool GetCertificateKey(
-      const net::X509Certificate& cert,
-      scoped_refptr<net::SSLPrivateKey>* private_key) override;
-
- private:
-  const scoped_refptr<base::SequencedTaskRunner> service_task_runner_;
-  // Must be dereferenced on |service_task_runner_| only.
-  base::WeakPtr<CertificateProviderService> service_;
-  certificate_provider::ThreadSafeCertificateMap* const certificate_map_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertKeyProviderImpl);
-};
-
 class CertificateProviderService::CertificateProviderImpl
     : public CertificateProvider {
  public:
   // Any calls back to |service| will be posted to |service_task_runner|.
   // |service| must be dereferenceable on |service_task_runner|.
   // This provider is not thread safe, but can be used on any thread.
   CertificateProviderImpl(
       const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
       const base::WeakPtr<CertificateProviderService>& service);
   ~CertificateProviderImpl() override;
 
-  void GetCertificates(const base::Callback<void(const net::CertificateList&)>&
+  void GetCertificates(const base::Callback<void(net::ClientCertIdentityList)>&
                            callback) override;
 
   std::unique_ptr<CertificateProvider> Copy() override;
 
  private:
   static void GetCertificatesOnServiceThread(
       const base::WeakPtr<CertificateProviderService>& service,
-      const base::Callback<void(const net::CertificateList&)>& callback);
+      const base::Callback<void(net::ClientCertIdentityList)>& callback);
 
   const scoped_refptr<base::SequencedTaskRunner> service_task_runner_;
   // Must be dereferenced on |service_task_runner_| only.
   const base::WeakPtr<CertificateProviderService> service_;
 
   DISALLOW_COPY_AND_ASSIGN(CertificateProviderImpl);
 };
 
 // Implements an SSLPrivateKey backed by the signing function exposed by an
 // extension through the certificateProvider API.
@@ skipping 33 matching lines @@
   const CertificateInfo cert_info_;
   scoped_refptr<base::SequencedTaskRunner> service_task_runner_;
   // Must be dereferenced on |service_task_runner_| only.
   const base::WeakPtr<CertificateProviderService> service_;
   base::ThreadChecker thread_checker_;
   base::WeakPtrFactory<SSLPrivateKey> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLPrivateKey);
 };
 
-CertificateProviderService::CertKeyProviderImpl::CertKeyProviderImpl(
-    const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
-    const base::WeakPtr<CertificateProviderService>& service,
-    certificate_provider::ThreadSafeCertificateMap* certificate_map)
-    : service_task_runner_(service_task_runner),
-      service_(service),
-      certificate_map_(certificate_map) {}
+class CertificateProviderService::ClientCertIdentity
+    : public net::ClientCertIdentity {
+ public:
+  ClientCertIdentity(
+      scoped_refptr<net::X509Certificate> cert,
+      const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
+      base::WeakPtr<CertificateProviderService> service)
+      : net::ClientCertIdentity(std::move(cert)),
+        service_task_runner_(service_task_runner),
+        service_(service) {}
+  ~ClientCertIdentity() override = default;
 
-CertificateProviderService::CertKeyProviderImpl::~CertKeyProviderImpl() {}
+  void AcquirePrivateKey(
+      const base::Callback<void(scoped_refptr<net::SSLPrivateKey>)>&
+          private_key_callback) override;
 
-bool CertificateProviderService::CertKeyProviderImpl::GetCertificateKey(
-    const net::X509Certificate& cert,
-    scoped_refptr<net::SSLPrivateKey>* private_key) {
+ private:
+  scoped_refptr<net::SSLPrivateKey> GetCertificateKeyOnServiceThread(
+      net::X509Certificate* cert);
+
+  scoped_refptr<base::SequencedTaskRunner> service_task_runner_;
+  // Must be dereferenced on |service_task_runner_| only.
+  const base::WeakPtr<CertificateProviderService> service_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientCertIdentity);
+};
+
+void CertificateProviderService::ClientCertIdentity::AcquirePrivateKey(
+    const base::Callback<void(scoped_refptr<net::SSLPrivateKey>)>&
+        private_key_callback) {
+  if (base::PostTaskAndReplyWithResult(
+          service_task_runner_.get(), FROM_HERE,
+          base::Bind(&ClientCertIdentity::GetCertificateKeyOnServiceThread,
+                     base::Unretained(this), base::Unretained(certificate())),
+          private_key_callback)) {
+    return;
+  }
+  // If the task could not be posted, behave as if there were no certificates.
+  private_key_callback.Run(nullptr);
+}
+
+scoped_refptr<net::SSLPrivateKey> CertificateProviderService::
+    ClientCertIdentity::GetCertificateKeyOnServiceThread(
+        net::X509Certificate* cert) {
+  if (!service_)
+    return nullptr;
+
   bool is_currently_provided = false;
   CertificateInfo info;
   std::string extension_id;
-  certificate_map_->LookUpCertificate(cert, &is_currently_provided, &info,
-                                      &extension_id);
+  // TODO(mattm): can the ClientCertIdentity store a handle directly to the
+  // extension instead of having to go through service_->certificate_map_ ?

[davidben, 2017/06/16 00:28:25]

Looks like you have the callback available at UpdateCertificatesAndRun, right?
That'll also fix issues if two extensions advertise the same cert. Though yeah,
maybe separate CL makes sense and probably there's more involved here. *shrug*

[mattm, 2017/06/16 22:34:55]

Yeah I think the complication will be handling extensions that get removed while
a ClientCertIdentity is in flight but before the private key is acquired. Right
now certificate_map_ handles all that. I guess it probably shouldn't be too hard
to check directly if the extension still exists. On the other hand, I also
suppose checking that is a bit of questionable benefit since there's still the
possibility that the extension gets unloaded between AcquirePrivateKey and the
actual signing operation ..

The other issue is that doing this wouldn't buy much unless we remove the other
dependencies on certificate_map_ as well.

So yeah, probably better to leave this to a separate change.

+  service_->certificate_map_.LookUpCertificate(*cert, &is_currently_provided,
+                                               &info, &extension_id);
   if (!is_currently_provided)
-    return false;
+    return nullptr;
 
-  *private_key =
-      new SSLPrivateKey(extension_id, info, service_task_runner_, service_);
-
-  return true;
+  return base::MakeRefCounted<SSLPrivateKey>(extension_id, info,
+                                             service_task_runner_, service_);
 }
 
 CertificateProviderService::CertificateProviderImpl::CertificateProviderImpl(
     const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
     const base::WeakPtr<CertificateProviderService>& service)
     : service_task_runner_(service_task_runner), service_(service) {}
 
 CertificateProviderService::CertificateProviderImpl::
     ~CertificateProviderImpl() {}
 
 void CertificateProviderService::CertificateProviderImpl::GetCertificates(
-    const base::Callback<void(const net::CertificateList&)>& callback) {
+    const base::Callback<void(net::ClientCertIdentityList)>& callback) {
   const scoped_refptr<base::TaskRunner> source_task_runner =
       base::ThreadTaskRunnerHandle::Get();
-  const base::Callback<void(const net::CertificateList&)>
-      callback_from_service_thread = base::Bind(&PostCertificatesToTaskRunner,
-                                                source_task_runner, callback);
+  const base::Callback<void(net::ClientCertIdentityList)>
+      callback_from_service_thread =
+          base::Bind(&PostIdentitiesToTaskRunner, source_task_runner, callback);
 
   service_task_runner_->PostTask(
       FROM_HERE, base::Bind(&GetCertificatesOnServiceThread, service_,
                             callback_from_service_thread));
 }
 
 std::unique_ptr<CertificateProvider>
 CertificateProviderService::CertificateProviderImpl::Copy() {
   return base::WrapUnique(
       new CertificateProviderImpl(service_task_runner_, service_));
 }
 
 // static
 void CertificateProviderService::CertificateProviderImpl::
     GetCertificatesOnServiceThread(
         const base::WeakPtr<CertificateProviderService>& service,
-        const base::Callback<void(const net::CertificateList&)>& callback) {
+        const base::Callback<void(net::ClientCertIdentityList)>& callback) {
   if (!service) {
-    callback.Run(net::CertificateList());
+    callback.Run(net::ClientCertIdentityList());
     return;
   }
   service->GetCertificatesFromExtensions(callback);
 }
 
 CertificateProviderService::SSLPrivateKey::SSLPrivateKey(
     const std::string& extension_id,
     const CertificateInfo& cert_info,
     const scoped_refptr<base::SequencedTaskRunner>& service_task_runner,
     const base::WeakPtr<CertificateProviderService>& service)
@@ skipping 62 matching lines @@
     const std::vector<uint8_t>& signature) {
   DCHECK(thread_checker_.CalledOnValidThread());
   callback.Run(error, signature);
 }
 
 CertificateProviderService::CertificateProviderService()
     : weak_factory_(this) {}
 
 CertificateProviderService::~CertificateProviderService() {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  // ClientKeyStore serializes access to |cert_key_provider_|.
-  // Once RemoveProvider() returns, it is guaranteed that there are no more
-  // accesses to |cert_key_provider_| in flight and no references to
-  // |cert_key_provider_| are remaining. This service will hold the last
-  // reference to |cert_key_provider_|.
-  net::ClientKeyStore::GetInstance()->RemoveProvider(cert_key_provider_.get());
-  cert_key_provider_.reset();
 }
 
 void CertificateProviderService::SetDelegate(
     std::unique_ptr<Delegate> delegate) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!delegate_);
   DCHECK(delegate);
 
   delegate_ = std::move(delegate);
-  cert_key_provider_.reset(
-      new CertKeyProviderImpl(base::ThreadTaskRunnerHandle::Get(),
-                              weak_factory_.GetWeakPtr(), &certificate_map_));
-  net::ClientKeyStore::GetInstance()->AddProvider(cert_key_provider_.get());
 }
 
 bool CertificateProviderService::SetCertificatesProvidedByExtension(
     const std::string& extension_id,
     int cert_request_id,
     const CertificateInfoList& certificate_infos) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool completed = false;
   if (!certificate_requests_.SetCertificates(extension_id, cert_request_id,
                                              certificate_infos, &completed)) {
     DLOG(WARNING) << "Unexpected reply of extension " << extension_id
                   << " to request " << cert_request_id;
     return false;
   }
   if (completed) {
     std::map<std::string, CertificateInfoList> certificates;
-    base::Callback<void(const net::CertificateList&)> callback;
+    base::Callback<void(net::ClientCertIdentityList)> callback;
     certificate_requests_.RemoveRequest(cert_request_id, &certificates,
                                         &callback);
     UpdateCertificatesAndRun(certificates, callback);
   }
   return true;
 }
 
 void CertificateProviderService::ReplyToSignRequest(
     const std::string& extension_id,
     int sign_request_id,
@@ skipping 30 matching lines @@
       base::ThreadTaskRunnerHandle::Get(), weak_factory_.GetWeakPtr());
 }
 
 void CertificateProviderService::OnExtensionUnloaded(
     const std::string& extension_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   for (const int cert_request_id :
        certificate_requests_.DropExtension(extension_id)) {
     std::map<std::string, CertificateInfoList> certificates;
-    base::Callback<void(const net::CertificateList&)> callback;
+    base::Callback<void(net::ClientCertIdentityList)> callback;
     certificate_requests_.RemoveRequest(cert_request_id, &certificates,
                                         &callback);
     UpdateCertificatesAndRun(certificates, callback);
   }
 
   certificate_map_.RemoveExtension(extension_id);
 
   for (auto callback : sign_requests_.RemoveAllRequests(extension_id))
     callback.Run(net::ERR_FAILED, std::vector<uint8_t>());
 
   pin_dialog_manager_.ExtensionUnloaded(extension_id);
 }
 
 void CertificateProviderService::GetCertificatesFromExtensions(
-    const base::Callback<void(const net::CertificateList&)>& callback) {
+    const base::Callback<void(net::ClientCertIdentityList)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const std::vector<std::string> provider_extensions(
       delegate_->CertificateProviderExtensions());
 
   if (provider_extensions.empty()) {
     DVLOG(2) << "No provider extensions left, clear all certificates.";
     UpdateCertificatesAndRun(std::map<std::string, CertificateInfoList>(),
                              callback);
     return;
   }
 
   const int cert_request_id = certificate_requests_.AddRequest(
       provider_extensions, callback,
       base::Bind(&CertificateProviderService::TerminateCertificateRequest,
                  base::Unretained(this)));
 
   DVLOG(2) << "Start certificate request " << cert_request_id;
   delegate_->BroadcastCertificateRequest(cert_request_id);
 }
 
 void CertificateProviderService::UpdateCertificatesAndRun(
     const std::map<std::string, CertificateInfoList>& extension_to_certificates,
-    const base::Callback<void(const net::CertificateList&)>& callback) {
+    const base::Callback<void(net::ClientCertIdentityList)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Extensions are removed from the service's state when they're unloaded.
   // Any remaining extension is assumed to be enabled.
   certificate_map_.Update(extension_to_certificates);
 
-  net::CertificateList all_certs;
+  net::ClientCertIdentityList all_certs;
   for (const auto& entry : extension_to_certificates) {
     for (const CertificateInfo& cert_info : entry.second)
-      all_certs.push_back(cert_info.certificate);
+      all_certs.push_back(base::MakeUnique<ClientCertIdentity>(
+          cert_info.certificate, base::ThreadTaskRunnerHandle::Get(),
+          weak_factory_.GetWeakPtr()));
   }
 
-  callback.Run(all_certs);
+  callback.Run(std::move(all_certs));
 }
 
 void CertificateProviderService::TerminateCertificateRequest(
     int cert_request_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   std::map<std::string, CertificateInfoList> certificates;
-  base::Callback<void(const net::CertificateList&)> callback;
+  base::Callback<void(net::ClientCertIdentityList)> callback;
   if (!certificate_requests_.RemoveRequest(cert_request_id, &certificates,
                                            &callback)) {
     DLOG(WARNING) << "Request id " << cert_request_id << " unknown.";
     return;
   }
 
   DVLOG(1) << "Time out certificate request " << cert_request_id;
   UpdateCertificatesAndRun(certificates, callback);
 }
 
 void CertificateProviderService::RequestSignatureFromExtension(
     const std::string& extension_id,
     const scoped_refptr<net::X509Certificate>& certificate,
     net::SSLPrivateKey::Hash hash,
     const std::string& digest,
     const net::SSLPrivateKey::SignCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const int sign_request_id = sign_requests_.AddRequest(extension_id, callback);
   if (!delegate_->DispatchSignRequestToExtension(extension_id, sign_request_id,
                                                  hash, certificate, digest)) {
     sign_requests_.RemoveRequest(extension_id, sign_request_id,
                                  nullptr /* callback */);
     callback.Run(net::ERR_FAILED, std::vector<uint8_t>());
   }
 }
 
 }  // namespace chromeos