Improved support for loading client certificates on smart cards on macOS

net/ssl/client_cert_store_mac.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_mac.h"
 
 #include <CommonCrypto/CommonDigest.h>
 #include <CoreFoundation/CFArray.h>
 #include <CoreServices/CoreServices.h>
 #include <Security/SecBase.h>
 #include <Security/Security.h>
 
 #include <algorithm>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "crypto/mac_security_services_lock.h"
 #include "net/base/host_port_pair.h"
 #include "net/cert/x509_util.h"
@@ skipping 205 matching lines @@
   }
   sort(sort_begin, sort_end, x509_util::ClientCertSorter());
 }
 
 }  // namespace
 
 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) {
   std::string server_domain = request.host_and_port.host();
 
   ScopedCFTypeRef<SecIdentityRef> preferred_identity;
   if (!server_domain.empty()) {
     // See if there's an identity preference for this domain:
     ScopedCFTypeRef<CFStringRef> domain_str(
         base::SysUTF8ToCFStringRef("https://" + server_domain));
@@ skipping 27 matching lines @@
   ScopedCFTypeRef<SecIdentitySearchRef> scoped_search(search);
   while (!err) {
     SecIdentityRef identity = NULL;
     {
       base::AutoLock lock(crypto::GetMacSecurityServicesLock());
       err = SecIdentitySearchCopyNext(search, &identity);
     }
     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) {
     OSSTATUS_LOG(ERROR, err) << "SecIdentitySearch error";
     callback.Run(CertificateList());
     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,
     CertificateList* selected_certs) {
   GetClientCertsImpl(NULL, input_certs, request, false, selected_certs);
   return true;
 }
 
 bool ClientCertStoreMac::SelectClientCertsGivenPreferredForTesting(
     const scoped_refptr<X509Certificate>& preferred_cert,
     const CertificateList& regular_certs,
     const SSLCertRequestInfo& request,
     CertificateList* selected_certs) {
   GetClientCertsImpl(
       preferred_cert, regular_certs, request, false, selected_certs);
   return true;
 }
 
 #pragma clang diagnostic pop  // "-Wdeprecated-declarations"
 
 }  // namespace net