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..b4324364b428ae1c0dcd23a3cbf45d8b8fe099cc 100644
--- a/net/ssl/client_cert_store_mac.cc
+++ b/net/ssl/client_cert_store_mac.cc
@@ -284,29 +284,8 @@ 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(regular_certs, preferred_cert, preferred_identity.get(),
+                identity);
   }
 
   if (err != errSecItemNotFound) {
@@ -315,12 +294,68 @@ void ClientCertStoreMac::GetClientCerts(
     return;
   }
 
+  // macOS provides two ways to search for identities. SecIdentitySearchCreate()
+  // 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[] = {
+      kSecClass, kSecMatchLimit, kSecReturnRef, kSecAttrCanSign,
+  };
+  const void* values[] = {
+      kSecClassIdentity, kSecMatchLimitAll, kCFBooleanTrue, kCFBooleanTrue,
+  };
+  ScopedCFTypeRef<CFDictionaryRef> query(CFDictionaryCreate(
+      kCFAllocatorDefault, keys, values, sizeof(values) / sizeof(values[0]),

[mattm, 2017/06/05 23:34:28]

arraysize(values)

[agaynor, 2017/06/06 22:38:49]

Done.

+      &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
+  ScopedCFTypeRef<CFArrayRef> result;
+  err = SecItemCopyMatching(

[mattm, 2017/06/05 23:34:27]

put this in a base::AutoLock lock(crypto::GetMacSecurityServicesLock()); block
(eg, like line 281)

[agaynor, 2017/06/06 22:38:49]

Done.

+      query, reinterpret_cast<CFTypeRef*>(result.InitializeInto()));
+  if (!err) {
+    for (CFIndex i = 0; i < CFArrayGetCount(result); i++) {
+      CFTypeRef item = CFArrayGetValueAtIndex(result, i);
+      AddIdentity(regular_certs, preferred_cert, preferred_identity,
+                  (SecIdentityRef)item);

[mattm, 2017/06/05 23:34:27]

C style casts aren't allowed, should use reinterpret_cast

[agaynor, 2017/06/06 22:38:49]

Done. Although I had to also use a const_cast, which doesn't seem like it should
be necessary.

+    }
+  }
+
   CertificateList selected_certs;
   GetClientCertsImpl(preferred_cert, regular_certs, request, true,
                      &selected_certs);
   callback.Run(std::move(selected_certs));
 }
 
+void ClientCertStoreMac::AddIdentity(

[mattm, 2017/06/05 23:34:28]

It appears this could be an anonymous function instead of a private method.

[agaynor, 2017/06/06 22:38:49]

Done.

[mattm, 2017/06/07 00:24:07]

Also move it into the "namespace {}" block above.

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

Done.

+    CertificateList& regular_certs,
+    scoped_refptr<X509Certificate>& preferred_cert,

[mattm, 2017/06/05 23:34:27]

non-const reference parameters aren't allowed, these should be passed as
pointers instead.

[agaynor, 2017/06/06 22:38:49]

Done.

+    SecIdentityRef preferred_identity,
+    SecIdentityRef identity) {

[mattm, 2017/06/05 23:34:27]

Input parameters should come before output params.

[agaynor, 2017/06/06 22:38:49]

Done.

+  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());
+    preferred_cert = cert;
+  } else {
+    regular_certs.push_back(cert);
+  }
+}
+
 bool ClientCertStoreMac::SelectClientCertsForTesting(
     const CertificateList& input_certs,
     const SSLCertRequestInfo& request,