Refactor client cert private key handling.

net/ssl/client_cert_store_win.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_win.h"
 
 #include <algorithm>
 #include <string>
 
 #define SECURITY_WIN32  // Needs to be defined before including security.h
 #include <windows.h>
 #include <security.h>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/task_runner_util.h"
 #include "crypto/wincrypt_shim.h"
 #include "net/cert/x509_util.h"
+#include "net/ssl/ssl_platform_key_util.h"
+#include "net/ssl/ssl_platform_key_win.h"
+#include "net/ssl/ssl_private_key.h"
 
 namespace net {
 
 namespace {
 
+class ClientCertIdentityWin : public ClientCertIdentity {
+ public:
+  // Takes ownership of |cert_context|.
+  ClientCertIdentityWin(scoped_refptr<net::X509Certificate> cert,
+                        PCCERT_CONTEXT cert_context)
+      : ClientCertIdentity(std::move(cert)), cert_context_(cert_context) {}
+  ~ClientCertIdentityWin() override {
+    CertFreeCertificateContext(cert_context_);
+  }
+
+  void AcquirePrivateKey(
+      const base::Callback<void(scoped_refptr<SSLPrivateKey>)>&
+          private_key_callback) override;

[davidben, 2017/06/07 23:06:17]

Optional: What's the reason for defining this function out-of-line? I'm never
clear on what our style is w.r.t. these things.

[mattm, 2017/06/08 21:47:56]

Done.

+
+ private:
+  PCCERT_CONTEXT cert_context_;
+};
+
+void ClientCertIdentityWin::AcquirePrivateKey(
+    const base::Callback<void(scoped_refptr<SSLPrivateKey>)>&
+        private_key_callback) {
+  if (base::PostTaskAndReplyWithResult(
+          GetSSLPlatformKeyTaskRunner().get(), FROM_HERE,
+          base::Bind(&FetchClientCertPrivateKey,

[davidben, 2017/06/07 23:06:17]

Optional: Given this function isn't tested as part of
ssl_platform_key_win_unittest.cc anyway, we probably could just inline it into
ClientCertIdentityWin::AcquirePrivateKey. Dunno if that makes things any
cleaner.

[mattm, 2017/06/08 21:47:56]

hm, dunno. I kinda like leaving it in ssl_platform_key_win. 
One is just because of the weirdness of needing to choose between the CNG or
CAPI versions, kinda seems nicer to keep that hidden.
Also it keeps things more consistent between the different impls.

+                     base::Unretained(certificate()), cert_context_),
+          private_key_callback)) {
+    return;
+  }
+  // If the task could not be posted, behave as if there was no key.
+  private_key_callback.Run(nullptr);
+}
+
 // Callback required by Windows API function CertFindChainInStore(). In addition
 // to filtering by extended/enhanced key usage, we do not show expired
 // certificates and require digital signature usage in the key usage extension.
 //
 // This matches our behavior on Mac OS X and that of NSS. It also matches the
 // default behavior of IE8. See http://support.microsoft.com/kb/890326 and
 // http://blogs.msdn.com/b/askie/archive/2009/06/09/my-expired-client-certifica
 //     tes-no-longer-display-when-connecting-to-my-web-server-using-ie8.aspx
 static BOOL WINAPI ClientCertFindCallback(PCCERT_CONTEXT cert_context,
                                           void* find_arg) {
@@ skipping 25 matching lines @@
   if (!CertGetCertificateContextProperty(
           cert_context, CERT_KEY_PROV_INFO_PROP_ID, NULL, &size)) {
     return FALSE;
   }
 
   return TRUE;
 }
 
 void GetClientCertsImpl(HCERTSTORE cert_store,
                         const SSLCertRequestInfo& request,
-                        CertificateList* selected_certs) {
-  selected_certs->clear();
+                        ClientCertIdentityList* selected_identities) {
+  selected_identities->clear();
 
   const size_t auth_count = request.cert_authorities.size();
   std::vector<CERT_NAME_BLOB> issuers(auth_count);
   for (size_t i = 0; i < auth_count; ++i) {
     issuers[i].cbData = static_cast<DWORD>(request.cert_authorities[i].size());
     issuers[i].pbData = reinterpret_cast<BYTE*>(
         const_cast<char*>(request.cert_authorities[i].data()));
   }
 
   // Enumerate the client certificates.
@@ skipping 62 matching lines @@
       intermediates.pop_back();
     }
 
     // TODO(svaldez): cert currently wraps cert_context2 which may be backed
     // by a smartcard with threading difficulties. Instead, create a fresh
     // X509Certificate with CreateFromBytes and route cert_context2 into the
     // SSLPrivateKey. Probably changing CertificateList to be a
     // pair<X509Certificate, SSLPrivateKeyCallback>.
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
         cert_context2, intermediates);
-    if (cert)
-      selected_certs->push_back(std::move(cert));
-    CertFreeCertificateContext(cert_context2);
+    if (cert) {
+      selected_identities->push_back(base::MakeUnique<ClientCertIdentityWin>(
+          std::move(cert),
+          cert_context2));  // Takes ownership of |cert_context2|
+    }
     for (size_t i = 0; i < intermediates.size(); ++i)
       CertFreeCertificateContext(intermediates[i]);
   }
 
-  std::sort(selected_certs->begin(), selected_certs->end(),
-            x509_util::ClientCertSorter());
+  std::sort(selected_identities->begin(), selected_identities->end(),
+            ClientCertIdentitySorter());
 }
 
 }  // namespace
 
 ClientCertStoreWin::ClientCertStoreWin() {}
 
 ClientCertStoreWin::ClientCertStoreWin(HCERTSTORE cert_store) {
   DCHECK(cert_store);
   cert_store_.reset(cert_store);
 }
 
 ClientCertStoreWin::~ClientCertStoreWin() {}
 
 void ClientCertStoreWin::GetClientCerts(
     const SSLCertRequestInfo& request,
     const ClientCertListCallback& callback) {
-  CertificateList selected_certs;
+  ClientCertIdentityList selected_identities;
   if (cert_store_) {
     // Use the existing client cert store. Note: Under some situations,
     // it's possible for this to return certificates that aren't usable
     // (see below).
-    GetClientCertsImpl(cert_store_, request, &selected_certs);
-    callback.Run(std::move(selected_certs));
+    GetClientCertsImpl(cert_store_, request, &selected_identities);
+    callback.Run(std::move(selected_identities));
     return;
   }
 
   // Always open a new instance of the "MY" store, to ensure that there
   // are no previously cached certificates being reused after they're
   // no longer available (some smartcard providers fail to update the "MY"
   // store handles and instead interpose CertOpenSystemStore).
   ScopedHCERTSTORE my_cert_store(CertOpenSystemStore(NULL, L"MY"));
   if (!my_cert_store) {
     PLOG(ERROR) << "Could not open the \"MY\" system certificate store: ";
-    callback.Run(CertificateList());
+    callback.Run(ClientCertIdentityList());
     return;
   }
 
-  GetClientCertsImpl(my_cert_store, request, &selected_certs);
-  callback.Run(std::move(selected_certs));
+  GetClientCertsImpl(my_cert_store, request, &selected_identities);
+  callback.Run(std::move(selected_identities));
 }
 
 bool ClientCertStoreWin::SelectClientCertsForTesting(
     const CertificateList& input_certs,
     const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
+    ClientCertIdentityList* selected_identities) {
   ScopedHCERTSTORE test_store(CertOpenStore(CERT_STORE_PROV_MEMORY, 0, NULL, 0,
                                             NULL));
   if (!test_store)
     return false;
 
   // Add available certificates to the test store.
   for (size_t i = 0; i < input_certs.size(); ++i) {
     // Add the certificate to the test store.
     PCCERT_CONTEXT cert = NULL;
     if (!CertAddCertificateContextToStore(test_store,
                                           input_certs[i]->os_cert_handle(),
                                           CERT_STORE_ADD_NEW, &cert)) {
       return false;
     }
     // Add dummy private key data to the certificate - otherwise the certificate
     // would be discarded by the filtering routines.
     CRYPT_KEY_PROV_INFO private_key_data;
     memset(&private_key_data, 0, sizeof(private_key_data));
     if (!CertSetCertificateContextProperty(cert,
                                            CERT_KEY_PROV_INFO_PROP_ID,
                                            0, &private_key_data)) {
       return false;
     }
     // Decrement the reference count of the certificate (since we requested a
     // copy).
     if (!CertFreeCertificateContext(cert))
       return false;
   }
 
-  GetClientCertsImpl(test_store.get(), request, selected_certs);
+  GetClientCertsImpl(test_store.get(), request, selected_identities);
   return true;
 }
 
 }  // namespace net