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..d69ebbc0f1573001e6c79edf3a24e6199dbe1182 100644
--- a/net/ssl/client_cert_store_mac.cc
+++ b/net/ssl/client_cert_store_mac.cc
@@ -284,29 +284,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(regular_certs, preferred_cert, preferred_identity.get(), identity);
   }
 
   if (err != errSecItemNotFound) {
@@ -315,12 +293,74 @@ void ClientCertStoreMac::GetClientCerts(
     return;
   }
 
+  // For reasons I don't understand, macOS has two different ways of querying
+  // for client certificate identities. The way we just tried will miss some
+  // smart card based certificates, and this way misses some soft certificates.

[Ryan Sleevi, 2017/05/30 18:00:01]

We try to avoid pronouns in comments (I/we) and try to explain/document the root
cause (e.g. via discussion from opensource.apple.com - see line 251 for an
example)

More complete documentation may be:

// 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.

[agaynor, 2017/05/31 02:37:33]

Done.

+  const void *keys[] = {

[Ryan Sleevi, 2017/05/30 18:00:01]

You can use "git cl format" to ensure this is properly formatted, but Chrome
groups the * to the type rather than the identifier ("void* keys" vs "void
*keys")

https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expre...
permits both, but
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
specifies for Chromium :)

[Ryan Sleevi, 2017/05/30 18:00:01]

So, I _believe_ for correctness, that kSecAttrCanSign should be kCFBooleanTrue
(if RSA) or kSecAttrCanDerive should be kCFBooleanTrue (if ECDSA/ECDH)

Could you describe what type of key you have? I'd be willing to bet it's an
ECDSA/ECDH key, which is why the CSSM_KEYUSE_SIGN isn't sufficient. I'd be
willing to bet that if we also passed CSSM_KEYUSE_DERIVE, your cert would be
returned.

Context is that
https://opensource.apple.com/source/Security/Security-57740.51.3/OSX/libsecur...
shows that SecItemCopyMatching punts to SecItemCopyMatching_osx which then maps
the kSecAttrCan* flags into CSSM keyusage fields, and then just calls
SecIdentitySearchCreate(). The difference in your invocation here and the
existing invocation is you're not supplying a kSecAttrCan* parameter, and so
you're getting all keyUsages, by virtue of _CssmKeyUsageFromQuery

[agaynor, 2017/05/31 02:37:33]

Done.

[agaynor, 2017/05/31 02:37:33]

It's a boring old RSA key. I've added |kSecAttrCanSign=kCFBooleanTrue| and this
patch continues to fix this issue for me.

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

Looking at that code a bit, it seems that SecItemCategorizeQuery is used to
decide whether to do the SecItemCopyMatching_ios and/or SecItemCopyMatching_osx,
and the default is to do both. It looks like this query doesn't have any of the
params that would exclude either type, so that can explain how this produces
different results.

(I guess that also means that if we were confident about which versions of OSX
that translation code is fully implemented on, we could get rid of the
SecIdentitySearch... code from here entirely. I looked at the 10.9 version, it
appears to have similar stuff, though with some differences.
(https://opensource.apple.com/source/Security/Security-55471/libsecurity_keych...)
I didn't dig in enough to really be confident about it though.  Just to be safe
keeping both seems fine.)

+    kSecClass,
+    kSecMatchLimit,
+    kSecReturnRef,
+  };
+  const void *values[] = {
+    kSecClassIdentity,
+    kSecMatchLimitAll,
+    kCFBooleanTrue,
+  };
+  CFDictionaryRef query = CFDictionaryCreate(

[Ryan Sleevi, 2017/05/30 18:00:01]

You can see we used the ScopedCFTypeRef C++ helpers here to ensure RAII

[agaynor, 2017/05/31 02:37:33]

Done.

+    kCFAllocatorDefault,
+    keys,
+    values,
+    sizeof(values) / sizeof(values[0]),
+    &kCFTypeDictionaryKeyCallBacks,
+    &kCFTypeDictionaryValueCallBacks
+  );
+  CFArrayRef result = NULL;
+  err = SecItemCopyMatching(query, (CFTypeRef *)&result);

[Ryan Sleevi, 2017/05/30 18:00:00]

Note: Chromium explicitly uses C++ casts rather than C casts - see
https://google.github.io/styleguide/cppguide.html#Casting

If you're using ScopedCFTypeRef, you can use .InitializeInto() to return an
acceptable pointer for this :)

[agaynor, 2017/05/31 02:37:33]

Done.

+  if (!err) {

[Ryan Sleevi, 2017/05/30 18:00:01]

In general, we try to handle the error case first, to avoid too much nesting.
You can see this in line 273 or 284, for example.

+    for (CFIndex i = 0; i < CFArrayGetCount(result); i++) {
+      CFTypeRef item = CFArrayGetValueAtIndex(result, i);
+      AddIdentity(regular_certs, preferred_cert, preferred_identity, (SecIdentityRef)item);
+    }
+  }
+  CFRelease(query);
+  CFRelease(result);
+
   CertificateList selected_certs;
   GetClientCertsImpl(preferred_cert, regular_certs, request, true,
                      &selected_certs);
   callback.Run(std::move(selected_certs));
 }
 
+void ClientCertStoreMac::AddIdentity(
+    CertificateList& regular_certs,
+    scoped_refptr<X509Certificate>& preferred_cert,
+    SecIdentityRef preferred_identity,
+    SecIdentityRef identity) {
+  OSStatus err;
+  SecCertificateRef cert_handle;

[Ryan Sleevi, 2017/05/30 18:00:01]

.InitializeInto :)

[agaynor, 2017/05/31 02:37:33]

Done.

+  err = SecIdentityCopyCertificate(identity, &cert_handle);
+  if (err != noErr)
+    return;
+  ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle);
+
+  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,