ClientCertStoreChromeOS: support additional non-platform certs.

chrome/browser/chromeos/net/client_cert_store_chromeos.cc

 // Copyright 2013 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/net/client_cert_store_chromeos.h"
 
 #include <cert.h>
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 
 namespace chromeos {
 
 namespace {
 
 class CertNotAllowedPredicate {
  public:
   explicit CertNotAllowedPredicate(
       const ClientCertStoreChromeOS::CertFilter& filter)
       : filter_(filter) {}
   bool operator()(const scoped_refptr<net::X509Certificate>& cert) const {
     return !filter_.IsCertAllowed(cert);
   }
 
  private:
   const ClientCertStoreChromeOS::CertFilter& filter_;
 };
 
 }  // namespace
 
 ClientCertStoreChromeOS::ClientCertStoreChromeOS(
+    scoped_ptr<CertificateProvider> cert_provider,
     scoped_ptr<CertFilter> cert_filter,
     const PasswordDelegateFactory& password_delegate_factory)
     : ClientCertStoreNSS(password_delegate_factory),
+      cert_provider_(cert_provider.Pass()),
       cert_filter_(cert_filter.Pass()) {}
 
 ClientCertStoreChromeOS::~ClientCertStoreChromeOS() {}
 
 void ClientCertStoreChromeOS::GetClientCerts(
     const net::SSLCertRequestInfo& cert_request_info,
     net::CertificateList* selected_certs,
     const base::Closure& callback) {
-  base::Closure bound_callback = base::Bind(
-      &ClientCertStoreChromeOS::CertFilterInitialized,
-      // Caller is responsible for keeping the ClientCertStore alive
-      // until the callback is run.
-      base::Unretained(this), &cert_request_info, selected_certs, callback);
+  // Caller is responsible for keeping the ClientCertStore alive until the
+  // callback is run.
+  base::Closure get_certs_after_init = base::Bind(
+      &ClientCertStoreChromeOS::GetClientCertsAfterInit, base::Unretained(this),
+      &cert_request_info, selected_certs, callback);
 
-  if (cert_filter_->Init(bound_callback))
-    bound_callback.Run();
+  base::Closure get_additional_certs_and_continue;
+  if (cert_provider_) {
+    get_additional_certs_and_continue =
+        base::Bind(&CertificateProvider::GetCertificates,
+                   base::Unretained(cert_provider_.get()),
+                   base::Bind(&ClientCertStoreChromeOS::SetAdditionalCerts,
+                              base::Unretained(this), get_certs_after_init));
+  } else {
+    get_additional_certs_and_continue = get_certs_after_init;
+  }
+
+  if (cert_filter_->Init(get_additional_certs_and_continue))
+    get_additional_certs_and_continue.Run();
 }
 
 void ClientCertStoreChromeOS::GetClientCertsImpl(
     CERTCertList* cert_list,
     const net::SSLCertRequestInfo& request,
     bool query_nssdb,
     net::CertificateList* selected_certs) {
   net::ClientCertStoreNSS::GetClientCertsImpl(cert_list, request, query_nssdb,
                                               selected_certs);
 
   size_t pre_size = selected_certs->size();
   selected_certs->erase(
       std::remove_if(selected_certs->begin(), selected_certs->end(),
                      CertNotAllowedPredicate(*cert_filter_)),
       selected_certs->end());
   DVLOG(1) << "filtered " << pre_size - selected_certs->size() << " of "
            << pre_size << " certs";
+
+  // Additionally to the matching certs from the platform's store, allow
+  // matching certificates from |additional_certs_|.
+  CERTCertList* additional_cert_list = CERT_NewCertList();
+  for (const auto& cert : additional_certs_) {
+    CERT_AddCertToListTail(additional_cert_list,
+                           CERT_DupCertificate(cert->os_cert_handle()));
+  }
+  net::CertificateList filtered_additional_certs;
+  net::ClientCertStoreNSS::GetClientCertsImpl(
+      additional_cert_list, request, query_nssdb, &filtered_additional_certs);
+
+  selected_certs->insert(selected_certs->end(),
+                         filtered_additional_certs.begin(),
+                         filtered_additional_certs.end());
 }
 
-void ClientCertStoreChromeOS::CertFilterInitialized(
+void ClientCertStoreChromeOS::SetAdditionalCerts(
+    const base::Closure& callback,
+    const net::CertificateList& certs) {
+  additional_certs_ = certs;

[davidben, 2015/08/14 21:49:16]

Hrm. Although we never actually call GetClientCerts more than once and this
interface is a mess, it is sort of weird to implement it this way, making it
stateful. (What if we have two GetClientCerts going on in parallel?)

Actually, why not do this:

1. GetClientCertsAfterInit takes a net::CertificateList parameter.
2. Use net::ClientCertStoreNSS::GetClientCertsImpl to filter that additional
list.
3. Call net::ClientCertStoreNSS::GetClientCerts with a different callback which
concatenates your lists together and calls the real callback.

In fact, if you do this (or even as they are now), it starts to seem like this
whole subclassing thing is kind of unnecessary to begin with. Instead you could:

1. Split net::ClientCertStoreNSS::GetClientCertsImpl into a random utility
function you can just call. FilterCertificateList or something.
2. ClientCertStoreChromeOS implements ClientCertStore directly and contains a
ClientCertStoreNSS.
3. ClientCertStoreChromeOS::GetClientCerts does:
   a. Look up certificates from ClientCertStoreNSS.
   -- async --
   b. Call additional CertNotAllowedPredicate on that list.
   c. Look up certificates from CertificateProvider.
   -- async --
   d. Call FilterCertificateList on that list.
   e. Call the callback with the concatenation of those two.

Composition and all that good stuff. :-)

[pneubeck (no reviews), 2015/08/17 12:01:32]

Not as easy because a lot of the NSS stuff was executed on the worker pool. What
everyone overlooked so far, is that the complete list of matching certs must be
sorted. I fixed that in the last patch set.

I went ahead an broke the ClientCertStoreNSS into static functions and removed
any inheritance. That should make this class much simpler now.

There's some duplication now to post to the worker pool. But IMO that's
acceptable and avoiding this duplication will mean more callbacks/virtuals...

+  callback.Run();
+}
+
+void ClientCertStoreChromeOS::GetClientCertsAfterInit(
     const net::SSLCertRequestInfo* request,
     net::CertificateList* selected_certs,
     const base::Closure& callback) {
   net::ClientCertStoreNSS::GetClientCerts(*request, selected_certs, callback);
 }
 
 }  // namespace chromeos