Return the certificate chain in ClientCertStoreNSS.

net/ssl/client_cert_store_nss.cc

Index: net/ssl/client_cert_store_nss.cc
diff --git a/net/ssl/client_cert_store_nss.cc b/net/ssl/client_cert_store_nss.cc
index e46458ad99e166657007407f056cecac79ff4a5f..5d9b7ba7304515e756c597b78a92b307bf0ac81b 100644
--- a/net/ssl/client_cert_store_nss.cc
+++ b/net/ssl/client_cert_store_nss.cc
@@ -10,6 +10,7 @@
 #include <algorithm>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -18,8 +19,10 @@
 #include "base/strings/string_piece.h"
 #include "base/threading/worker_pool.h"
 #include "crypto/nss_crypto_module_delegate.h"
+#include "net/cert/scoped_nss_types.h"
 #include "net/cert/x509_util.h"
 #include "net/ssl/ssl_cert_request_info.h"
+#include "net/third_party/nss/ssl/cmpcert.h"
 
 namespace net {
 
@@ -63,26 +66,6 @@ void ClientCertStoreNSS::FilterCertsOnWorkerThread(
 
   filtered_certs->clear();
 
-  // Create a "fake" CERTDistNames structure. No public API exists to create
-  // one from a list of issuers.
-  CERTDistNames ca_names;
-  ca_names.arena = NULL;
-  ca_names.nnames = 0;
-  ca_names.names = NULL;
-  ca_names.head = NULL;
-
-  std::vector<SECItem> ca_names_items(request.cert_authorities.size());
-  for (size_t i = 0; i < request.cert_authorities.size(); ++i) {
-    const std::string& authority = request.cert_authorities[i];
-    ca_names_items[i].type = siBuffer;
-    ca_names_items[i].data =
-        reinterpret_cast<unsigned char*>(const_cast<char*>(authority.data()));
-    ca_names_items[i].len = static_cast<unsigned int>(authority.size());
-  }
-  ca_names.nnames = static_cast<int>(ca_names_items.size());
-  if (!ca_names_items.empty())
-    ca_names.names = &ca_names_items[0];
-
   size_t num_raw = 0;
   for (const auto& cert : certs) {
     ++num_raw;
@@ -96,16 +79,32 @@ void ClientCertStoreNSS::FilterCertsOnWorkerThread(
       continue;
     }
 
-    // Check if the certificate issuer is allowed by the server.
-    if (!request.cert_authorities.empty() &&
-        NSS_CmpCertChainWCANames(handle, &ca_names) != SECSuccess) {
+    std::vector<ScopedCERTCertificate> chain;
+    if (!MatchClientCertificateIssuers(handle, request.cert_authorities,
+                                       &chain)) {
       DVLOG(2) << "skipped non-matching cert: "
                << base::StringPiece(handle->nickname);
       continue;
     }
 
     DVLOG(2) << "matched cert: " << base::StringPiece(handle->nickname);
-    filtered_certs->push_back(cert);
+
+    std::vector<base::StringPiece> der_chain;
+    for (const auto& chain_cert : chain) {
+      der_chain.push_back(base::StringPiece(
+          reinterpret_cast<const char*>(chain_cert->derCert.data),
+          chain_cert->derCert.len));
+    }
+
+    scoped_refptr<X509Certificate> full_cert =
+        X509Certificate::CreateFromDERCertChain(der_chain);
+    if (!full_cert.get()) {
+      DVLOG(2) << "could not decode chain: "
+               << base::StringPiece(handle->nickname);
+      continue;
+    }

[Ryan Sleevi, 2016/07/29 00:37:44]

Why do you indirect through the DER, when we've got OSCertHandles? We're already
guaranteed below (c.f. line 140)

X509Certificate::OSCertHandles certs;
std::transform(chain.begin(), chain.end(), std::back_inserter(certs), [](const
ScopedCERTCertificate& cert) { return cert.get(); });
if (certs.empty())
  continue;
X509Certificate::OSCertHandle handle = certs[0];
certs.erase(certs.begin());

scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(handle,
certs);


(or plenty of other ways to write it)

It just seems silly to go from handle->der->handle, especially since going from
der->handle invokes a full smartcard search to see if it has an existing
CERTCertificate.

[davidben, 2016/07/29 15:14:11]

You know, the for loop is actually much much less typing than the std::transform
version. :-P

Done. In doing so, I made the cmpcert.cc helper spit out a list of intermediates
rather than the whole chain to match what X509Certificate wants.

+
+    filtered_certs->push_back(full_cert);
   }
   DVLOG(2) << "num_raw:" << num_raw
            << " num_filtered:" << filtered_certs->size();