Return the certificate chain in ClientCertStoreNSS.

net/ssl/client_cert_store_nss.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 "net/ssl/client_cert_store_nss.h"
 
 #include <nss.h>
 #include <ssl.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #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 {
 
 ClientCertStoreNSS::ClientCertStoreNSS(
     const PasswordDelegateFactory& password_delegate_factory)
     : password_delegate_factory_(password_delegate_factory) {}
 
 ClientCertStoreNSS::~ClientCertStoreNSS() {}
 
 void ClientCertStoreNSS::GetClientCerts(const SSLCertRequestInfo& request,
@@ skipping 23 matching lines @@
 
 // static
 void ClientCertStoreNSS::FilterCertsOnWorkerThread(
     const CertificateList& certs,
     const SSLCertRequestInfo& request,
     CertificateList* filtered_certs) {
   DCHECK(filtered_certs);
 
   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;
     X509Certificate::OSCertHandle handle = cert->os_cert_handle();
 
     // Only offer unexpired certificates.
     if (CERT_CheckCertValidTimes(handle, PR_Now(), PR_TRUE) !=
         secCertTimeValid) {
       DVLOG(2) << "skipped expired cert: "
                << base::StringPiece(handle->nickname);
       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();
 
   std::sort(filtered_certs->begin(), filtered_certs->end(),
             x509_util::ClientCertSorter());
 }
 
 void ClientCertStoreNSS::GetAndFilterCertsOnWorkerThread(
     std::unique_ptr<crypto::CryptoModuleBlockingPasswordDelegate>
@@ skipping 19 matching lines @@
   }
   for (CERTCertListNode* node = CERT_LIST_HEAD(found_certs);
        !CERT_LIST_END(node, found_certs); node = CERT_LIST_NEXT(node)) {
     certs->push_back(X509Certificate::CreateFromHandle(
         node->cert, X509Certificate::OSCertHandles()));
   }
   CERT_DestroyCertList(found_certs);
 }
 
 }  // namespace net