Refactor client cert private key handling.

net/ssl/client_cert_store_win.cc

Index: net/ssl/client_cert_store_win.cc
diff --git a/net/ssl/client_cert_store_win.cc b/net/ssl/client_cert_store_win.cc
index fec9c33ae67b7d108e0468e4d686e606822213a1..891161f368a1083c6219221f702aeade8b0d2061 100644
--- a/net/ssl/client_cert_store_win.cc
+++ b/net/ssl/client_cert_store_win.cc
@@ -11,15 +11,54 @@
 #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;
+
+ 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,
+                     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.
@@ -65,8 +104,8 @@ static BOOL WINAPI ClientCertFindCallback(PCCERT_CONTEXT cert_context,
 
 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);
@@ -149,15 +188,17 @@ void GetClientCertsImpl(HCERTSTORE cert_store,
     // pair<X509Certificate, SSLPrivateKeyCallback>.

[davidben, 2017/06/01 23:41:25]

I think |cert| should probably be created from bytes out of
cert_context2->pbCertEncoded/cbCertEncoded? Maybe also the intermediates for
good measure?

This is possibly a Ryan question to make sure I'm not totally confused, but the
point of this exercise, as I understand was because cert_context and
cert_context2 are potentially tied to the smartcard driver and thus not
threadsafe or anything sensible (hence all this code running on the IO thread).

On the one hand, this is needed because FetchClientCertPrivateKey might want the
original PCCERT_CONTEXT.

On the other hand, this is not particular desirable because we'd rather
net::X509Certificate be logically byte strings. This avoids any threading
concerns. Notably, although we're trying to move everything to the smartcard
thread, access of the certs themselves is going to happen on the IO thread,
since we need the actual bytes. If we create things from bytes fresh, that means
we know they're not tied to any smartcard (and eventually can safely be made
CRYPTO_BUFFERs).

(Once you do that, you can just remove this TODO.)

[mattm, 2017/06/02 04:04:20]

I was planning to leave this as is in this CL since it should be followed very
shortly by switching win to X509CertificateBytes.

[Ryan Sleevi, 2017/06/08 18:13:47]

|cert_context2| is not - line 150 handles that.

The "issue" was that |cert_context| is tied to the HCERTSTORE that it came from
(and thus the HCERTSTORE can't be closed while the cert context is still live,
which can cause smart card difficulties). So we clone the |cert_context| into
the default HCERTSTORE (and the intermediates) to break the HCERTSTORE linkage,
while still maintaining the non-serialized attributes (such as the underlying
smartcard handle). 

That said, because of X509CertificateCache::InsertOrUpdate, we wouldn't actually
keep the smartcard context in the null-store cert if we already had a
(different) null-store cert.

But totally happy to defer this cleanup; the current code isn't accomplishing
what it (originally) set out to do, but that hasn't caused issues.

     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
@@ -174,13 +215,13 @@ ClientCertStoreWin::~ClientCertStoreWin() {}
 void ClientCertStoreWin::GetClientCerts(
     const SSLCertRequestInfo& request,
     const ClientCertListCallback& callback) {

[davidben, 2017/06/01 23:41:25]

This is probably a Ryan question, but in the same vein as not worrying about the
smartcard's thread-safety, perhaps GetSSLPlatformKeyTaskRunner() should be used
for GetClientCertsImpl too? That way accesses of the smartcard are all on one
thread. (Which perhaps suggests that FetchClientCertPrivateKey should take a
TaskRunner of some sort so we don't need to rely on two files picking the same
one...)

So originally, we had:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - IO thread
SSLPlatformKey - IO thread

This avoids threading troubles, but blocks the IO thread. Then we pushed the
signing off-thread and got:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - IO thread
SSLPlatformKey - smartcard thread

When we did this, Ryan pointed out that we're now risking problems with buggy
smartcards (though it seems to have been fine?). I assume that's why
GetClientCertsImpl didn't run on a worker thread way back.

Your CL makes it:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - smartcard thread
SSLPlatformKey - smartcard thread

But my understanding is we actually want:

GetClientCertsImpl - smartcard thread
FetchClientCertPrivateKey - smartcard thread
SSLPlatformKey - smartcard thread

As a bonus, GetClientCertsImpl won't block the IO thread!

(How this should interact with the non-NULL cert_store_ case, I have no idea!)

[davidben, 2017/06/01 23:46:14]

Likewise, should we try to keep thread affinity throughout like this for Mac and
NSS too? It seems a little odd to be happy using the worker pool most of it, but
then switching to GetSSLPlatformKeyTaskRunner once the actual signing kicks in.
Though I suppose nothing's broken yet...

[mattm, 2017/06/02 04:04:20]

Sounds reasonable but I'll wait and see if Ryan chimes in on these questions.
Also probably the GetClientCerts threading changes could be left to a followup
CL?

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

Fair enough. I guess the main reason we might want this (and the above) done
sooner is if we newly expose some threading risks with this change.

(We're already assuming the smartcard can tolerate
CryptAcquireCertificatePrivateKey and usage of the private key on different
threads, but I don't think we're currently assuming that the
potentially-smartcard-associated PCCERT_CONTEXT may be acquired on the IO thread
and then CryptAcquireCertificatePrivateKey'd on the smartcard thread.)

[Ryan Sleevi, 2017/06/08 18:13:47]

Right, so the risk I tried to capture in previous contexts was this:
- Prompting can happen in the CryptAcquire() call for some smart cards
- Prompting can happen in the CryptSignHash() call for other smart cards

Either of which can trigger UI and need to spin message pumps and all that
goodness.

HCERTSTORE is "supposed" to be thread-safe (and for MSFT it is, for others, it's
not). NCRYPT_KEY_HANDLE does not have such guarantees (IIRC). PCCERT_CONTEXT
does not (since it's backed by an HCERTSTORE)

I think the thread jumping may be better deferred until post-OSCertHandle, so
that we don't have to run the risk of using/accessing the PCCERT_CONTEXT (which
is held/lives on the IO thread) on other threads.

I missed where FetchClientCertPrivateKey moved off the IO thread, but if we're
using a PCCERT_CONTEXT, then we may be in trouble in doing so because of the
X509CertificateCache and the underlying HCERTSTORE affinity.

[davidben, 2017/06/08 21:40:48]

This CL currently moves FetchClientCertPrivateKey to the background thread up at
the top of the file. So that means this CL makes the PCCERT_CONTEXT accessd on
the IO thread (GetClientCertsImpl) and then the background thread
(FetchClientCertPrivateKey). I take it then this is "supposed" to work but does
not work for other smartcards.

I didn't realize NCRYPT_KEY_HANDLE simply didn't work at all. Even before this
CL we were doing FetchClientCertPrivateKey on IO thread and the signing on the
background thread.

How about this then: In the interest of not doing everything in one already
giant CL, let's make ClientCertIdentityWin, for now, call
FetchClientCertPrivateKey on the IO thread. (Which probably means
ClientCertIdentityWin needs to be passed in a TaskRunner since AcquirePrivateKey
is driven by the UI code now.) This means the threads are the same as before.
It's apparently violating NCRYPTO_SIGN_HANDLE's guarantees, but no differently
then we always were.

Then we can have a follow-up CL that fixes all the threading in one go: make
both GetClientCertsImpl and FetchClientCertPrivateKey join the signing code on
the smartcard thread and avoid introducing awkward intermediate stages.

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

So for context:

Before this CL, net::FetchClientCertPrivateKey is called by 
ResourceLoader::ContinueWithCertificate on the IO thread.

In the CL (up to patch set 14):
ClientCertIdentity::AcquirePrivateKey is called by
SSLClientCertificateSelector::Accept on the UI thread, 
(or directly by ChromeContentBrowserClient::SelectClientCertificate in the case
of policy auto select, also on UI thread).
AcquirePrivateKey then posts to FetchClientCertPrivateKey on the smartcard
thread.


I've changed it (in patchset 15) to instead have GetClientCertsImpl get a handle
for the thread it's on and pass that to the ClientCertIdentityWin, so that the
PCCERT_CONTEXT will only be accessed on the same thread it was created. 
That means currently it'll stay on the IO thread, but later, after the
use_byte_certs change, when GetClientCertsImpl gets moved to the smartcard
thread it'll go there too.

[mattm, 2017/06/12 21:28:30]

FYI, I have a followup CL (after this CL and after the CL for
use_byte_certs+=win) which moves the client cert lookup (and key acquisition)
onto the SSLPlatformKeyTaskRunner:
https://codereview.chromium.org/2927193003/ 

-  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;
   }
 
@@ -191,18 +232,18 @@ void ClientCertStoreWin::GetClientCerts(
   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)
@@ -232,7 +273,7 @@ bool ClientCertStoreWin::SelectClientCertsForTesting(
       return false;
   }
 
-  GetClientCertsImpl(test_store.get(), request, selected_certs);
+  GetClientCertsImpl(test_store.get(), request, selected_identities);
   return true;
 }