Improved support for loading client certificates on smart cards on macOS

net/ssl/client_cert_store_mac.cc

Index: net/ssl/client_cert_store_mac.cc
diff --git a/net/ssl/client_cert_store_mac.cc b/net/ssl/client_cert_store_mac.cc
index 7fadb337f1e7735ae8ffd340c81c93c95588d7c4..64e1e3fa076e83e93f90d6166d152966c1129761 100644
--- a/net/ssl/client_cert_store_mac.cc
+++ b/net/ssl/client_cert_store_mac.cc
@@ -12,6 +12,8 @@
 
 #include <algorithm>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/logging.h"
@@ -237,6 +239,14 @@ ClientCertStoreMac::ClientCertStoreMac() {}
 
 ClientCertStoreMac::~ClientCertStoreMac() {}
 
+// Given an |identity|, identifies its corresponding certificate, and either
+// adds it to |regular_certs| or assigns it to |preferred_cert|, if the
+// |identity| matches the |preferred_identity|.
+void AddIdentity(SecIdentityRef identity,
+                 SecIdentityRef preferred_identity,
+                 CertificateList* regular_certs,
+                 scoped_refptr<X509Certificate>* preferred_cert);
+
 void ClientCertStoreMac::GetClientCerts(
     const SSLCertRequestInfo& request,
     const ClientCertListCallback& callback) {
@@ -284,29 +294,7 @@ void ClientCertStoreMac::GetClientCerts(
     if (err)
       break;
     ScopedCFTypeRef<SecIdentityRef> scoped_identity(identity);
-
-    SecCertificateRef cert_handle;
-    err = SecIdentityCopyCertificate(identity, &cert_handle);
-    if (err != noErr)
-      continue;
-    ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle);
-
-    if (!SupportsSSLClientAuth(cert_handle))
-      continue;
-
-    scoped_refptr<X509Certificate> cert(
-        x509_util::CreateX509CertificateFromSecCertificate(
-            cert_handle, std::vector<SecCertificateRef>()));
-    if (!cert)
-      continue;
-
-    if (preferred_identity && CFEqual(preferred_identity, identity)) {
-      // Only one certificate should match.
-      DCHECK(!preferred_cert.get());
-      preferred_cert = cert;
-    } else {
-      regular_certs.push_back(cert);
-    }
+    AddIdentity(identity, preferred_identity, &regular_certs, &preferred_cert);
   }
 
   if (err != errSecItemNotFound) {
@@ -315,12 +303,70 @@ void ClientCertStoreMac::GetClientCerts(
     return;
   }
 
+  // macOS provides two ways to search for identities. SecIdentitySearchCreate()

[awong, 2017/06/07 00:35:24]

macOS -> MacOS 

codesearch shows it's about 8x more prevalent to capitalize the first letter
there.

[mattm, 2017/06/07 01:07:56]

Well, Apple did change the official name to "macOS" some time ago.

If you limit it to comments only, macOS seems to be more common than MacOS. (I'd
agree with capitalizing the M when it's used in method names.)

[awong, 2017/06/07 19:40:14]

garagh. okay, ignore my comment then.

+  // is deprecated, as it relies on CSSM_KEYUSE_SIGN (part of the deprecated
+  // CDSM/CSSA implementation), but is necessary to return some certificates
+  // that would otherwise not be returned by SecItemCopyMatching(), which is the
+  // non-deprecated way. However, SecIdentitySearchCreate() will not return all
+  // items, particularly smart-card based identities, so it's necessary to call
+  // both functions.
+  const void* keys[] = {

[awong, 2017/06/07 00:35:24]

Should these really be stack allocated?  Make them static?

Otherwise, you're gonna incur a O(n) copy per call.

[agaynor, 2017/06/07 21:19:47]

Done.

+      kSecClass, kSecMatchLimit, kSecReturnRef, kSecAttrCanSign,
+  };
+  const void* values[] = {
+      kSecClassIdentity, kSecMatchLimitAll, kCFBooleanTrue, kCFBooleanTrue,
+  };
+  ScopedCFTypeRef<CFDictionaryRef> query(CFDictionaryCreate(
+      kCFAllocatorDefault, keys, values, arraysize(values),
+      &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
+  ScopedCFTypeRef<CFArrayRef> result;
+  {
+    base::AutoLock lock(crypto::GetMacSecurityServicesLock());
+    err = SecItemCopyMatching(
+        query, reinterpret_cast<CFTypeRef*>(result.InitializeInto()));
+  }
+  if (!err) {
+    for (CFIndex i = 0; i < CFArrayGetCount(result); i++) {
+      void* item = const_cast<void*>(CFArrayGetValueAtIndex(result, i));
+      AddIdentity(reinterpret_cast<SecIdentityRef>(item), preferred_identity,
+                  &regular_certs, &preferred_cert);
+    }
+  }
+
   CertificateList selected_certs;
   GetClientCertsImpl(preferred_cert, regular_certs, request, true,
                      &selected_certs);
   callback.Run(std::move(selected_certs));
 }
 
+void AddIdentity(SecIdentityRef identity,
+                 SecIdentityRef preferred_identity,
+                 CertificateList* regular_certs,
+                 scoped_refptr<X509Certificate>* preferred_cert) {
+  OSStatus err;
+  ScopedCFTypeRef<SecCertificateRef> cert_handle;
+  err = SecIdentityCopyCertificate(identity, cert_handle.InitializeInto());
+  if (err != noErr)
+    return;
+
+  if (!SupportsSSLClientAuth(cert_handle))
+    return;
+
+  scoped_refptr<X509Certificate> cert(
+      x509_util::CreateX509CertificateFromSecCertificate(
+          cert_handle, std::vector<SecCertificateRef>()));
+  if (!cert)
+    return;
+
+  if (preferred_identity && CFEqual(preferred_identity, identity)) {
+    // Only one certificate should match.
+    DCHECK(!preferred_cert->get());

[awong, 2017/06/07 00:35:24]

If 2 match in production, and this just uses the last one, what happens?

[mattm, 2017/06/07 01:07:56]

It's which one will be listed first in the client cert selector dialog (assuming
it passes the filter), so if 2 matched, the 2nd one would win and get listed
first, and the 1st would be forgotten. However, if they are both equal to
preferred_identity, that means they would be duplicates anyway, so ignoring one
would be fine.

+    *preferred_cert = cert;
+  } else {
+    regular_certs->push_back(cert);
+  }
+}
+
 bool ClientCertStoreMac::SelectClientCertsForTesting(
     const CertificateList& input_certs,
     const SSLCertRequestInfo& request,