Refactor client cert private key handling.

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 "base/callback.h"
 #include "base/logging.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/scoped_cftyperef.h"
+#include "base/memory/ptr_util.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"
 #include "net/cert/x509_util_ios_and_mac.h"
 #include "net/cert/x509_util_mac.h"
+#include "net/ssl/client_cert_identity_mac.h"
 
 using base::ScopedCFTypeRef;
 
 namespace net {
 
 // CSSM functions are deprecated as of OSX 10.7, but have no replacement.
 // https://bugs.chromium.org/p/chromium/issues/detail?id=590914#c1
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
 
@@ skipping 39 matching lines @@
   }
   if (result)
     return result;
   {
     base::AutoLock lock(crypto::GetMacSecurityServicesLock());
     result = SecTrustGetResult(trust, &status, out_cert_chain, &status_chain);
   }
   return result;
 }
 
-// Returns true if |*cert| is issued by an authority in |valid_issuers|
-// according to Keychain Services, rather than using |cert|'s intermediate
-// certificates. If it is, |*cert| is updated to point to the completed
+// Returns true if |*identity| is issued by an authority in |valid_issuers|
+// according to Keychain Services, rather than using |identity|'s intermediate
+// certificates. If it is, |*identity| is updated to point to the completed
 // certificate

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

The last sentence seems off. "is updated to include the intermediates"?

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

Done.

 bool IsIssuedByInKeychain(const std::vector<std::string>& valid_issuers,
-                          scoped_refptr<X509Certificate>* cert) {
-  DCHECK(cert);
-  DCHECK(cert->get());
+                          ClientCertIdentity* identity) {
+  DCHECK(identity);
 
   base::ScopedCFTypeRef<SecCertificateRef> os_cert(
-      x509_util::CreateSecCertificateFromX509Certificate(cert->get()));
+      x509_util::CreateSecCertificateFromX509Certificate(
+          identity->certificate()));

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

Changed this since I noticed now I could actually just get the SecCertificate
out of identity->sec_identity_ref() for this part. (Since this should only be
reached when using real identities due to the query_keychain condition.)

   if (!os_cert)
     return false;
   CFArrayRef cert_chain = NULL;
   OSStatus result = CopyCertChain(os_cert.get(), &cert_chain);
   if (result) {
     OSSTATUS_LOG(ERROR, result) << "CopyCertChain error";
     return false;
   }
 
   if (!cert_chain)
     return false;
 
   std::vector<SecCertificateRef> intermediates;
   for (CFIndex i = 1, chain_count = CFArrayGetCount(cert_chain);
        i < chain_count; ++i) {
-    SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(
+    SecCertificateRef sec_cert = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(cert_chain, i)));
-    intermediates.push_back(cert);
+    intermediates.push_back(sec_cert);
   }
 
   scoped_refptr<X509Certificate> new_cert(
       x509_util::CreateX509CertificateFromSecCertificate(os_cert.get(),
                                                          intermediates));
   CFRelease(cert_chain);  // Also frees |intermediates|.
 
   if (!new_cert || !new_cert->IsIssuedByEncoded(valid_issuers))
     return false;
 
-  cert->swap(new_cert);
+  identity->SetIntermediates(new_cert->GetIntermediateCertificates());

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

This feels slightly odd (you could also defer creating the ClientCertIdentity
once you have everything), but that's probably more trouble than is worth it to
reshuffle everything that way, and this way we get to reuse methods on
X509Certificate.

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

Yeah, I noticed this oddity too, but it seemed like it would have to uglify
other things to avoid it.

   return true;
 }
 
 // Returns true if |purpose| is listed as allowed in |usage|. This
 // function also considers the "Any" purpose. If the attribute is
 // present and empty, we return false.
 bool ExtendedKeyUsageAllows(const CE_ExtendedKeyUsage* usage,
                             const CSSM_OID* purpose) {
   for (unsigned p = 0; p < usage->numPurposes; ++p) {
     if (CSSMOIDEqual(&usage->purposes[p], purpose))
@@ skipping 34 matching lines @@
   if (status == CSSM_OK && key_usage.field()) {
     const CSSM_X509_EXTENSION* ext = key_usage.GetAs<CSSM_X509_EXTENSION>();
     const CE_ExtendedKeyUsage* ext_key_usage =
         reinterpret_cast<const CE_ExtendedKeyUsage*>(ext->value.parsedValue);
     if (!ExtendedKeyUsageAllows(ext_key_usage, &CSSMOID_ClientAuth))
       return false;
   }
   return true;
 }
 
-// Examines the certificates in |preferred_cert| and |regular_certs| to find
-// all certificates that match the client certificate request in |request|,
-// storing the matching certificates in |selected_certs|.
+// Examines the certificates in |preferred_identity| and |regular_identities| to
+// find all certificates that match the client certificate request in |request|,
+// storing the matching certificates in |selected_identities|.
 // If |query_keychain| is true, Keychain Services will be queried to construct
 // full certificate chains. If it is false, only the the certificates and their
 // intermediates (available via X509Certificate::GetIntermediateCertificates())
 // will be considered.
-void GetClientCertsImpl(const scoped_refptr<X509Certificate>& preferred_cert,
-                        const CertificateList& regular_certs,
+void GetClientCertsImpl(std::unique_ptr<ClientCertIdentity> preferred_identity,
+                        ClientCertIdentityList regular_identities,
                         const SSLCertRequestInfo& request,
                         bool query_keychain,

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

I suspect the query_keychain boolean silliness could be better done with the
temporary keychain trick plus SecTrustSetKeychains, but, if we bother, that's
for a later CL. :-)

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

Well, aside from crbug.com/671889 :(
(SecTrustSetKeychains does not work on Mac OS 10.12)

-                        CertificateList* selected_certs) {
-  CertificateList preliminary_list;
-  if (preferred_cert.get())
-    preliminary_list.push_back(preferred_cert);
-  preliminary_list.insert(preliminary_list.end(), regular_certs.begin(),
-                          regular_certs.end());
+                        ClientCertIdentityList* selected_identities) {
+  scoped_refptr<X509Certificate> preferred_cert_orig;
+  ClientCertIdentityList preliminary_list(std::move(regular_identities));
+  if (preferred_identity) {
+    preferred_cert_orig = preferred_identity->certificate();
+    preliminary_list.insert(preliminary_list.begin(),
+                            std::move(preferred_identity));
+  }
 
-  selected_certs->clear();
+  selected_identities->clear();
   for (size_t i = 0; i < preliminary_list.size(); ++i) {
-    scoped_refptr<X509Certificate>& cert = preliminary_list[i];
-    if (cert->HasExpired())
+    std::unique_ptr<ClientCertIdentity>& cert = preliminary_list[i];
+    if (cert->certificate()->HasExpired())
       continue;
 
     // Skip duplicates (a cert may be in multiple keychains).
     auto cert_iter = std::find_if(
-        selected_certs->begin(), selected_certs->end(),
-        [&cert](const scoped_refptr<X509Certificate>& other_cert) {
-          return X509Certificate::IsSameOSCert(cert->os_cert_handle(),
-                                               other_cert->os_cert_handle());
+        selected_identities->begin(), selected_identities->end(),
+        [&cert](
+            const std::unique_ptr<ClientCertIdentity>& other_cert_identity) {
+          return X509Certificate::IsSameOSCert(
+              cert->certificate()->os_cert_handle(),
+              other_cert_identity->certificate()->os_cert_handle());
         });
-    if (cert_iter != selected_certs->end())
+    if (cert_iter != selected_identities->end())
       continue;
 
     // Check if the certificate issuer is allowed by the server.
     if (request.cert_authorities.empty() ||
-        cert->IsIssuedByEncoded(request.cert_authorities) ||
+        cert->certificate()->IsIssuedByEncoded(request.cert_authorities) ||
         (query_keychain &&
-         IsIssuedByInKeychain(request.cert_authorities, &cert))) {
-      selected_certs->push_back(cert);
+         IsIssuedByInKeychain(request.cert_authorities, cert.get()))) {
+      selected_identities->push_back(std::move(cert));
     }
   }
 
   // Preferred cert should appear first in the ui, so exclude it from the
-  // sorting.
-  CertificateList::iterator sort_begin = selected_certs->begin();
-  CertificateList::iterator sort_end = selected_certs->end();
-  if (preferred_cert.get() && sort_begin != sort_end &&
-      sort_begin->get() == preferred_cert.get()) {
+  // sorting.  Compare the os_cert_handle since the X509Certificate object may
+  // have changed if intermediates were added.
+  ClientCertIdentityList::iterator sort_begin = selected_identities->begin();
+  ClientCertIdentityList::iterator sort_end = selected_identities->end();
+  if (preferred_cert_orig && sort_begin != sort_end &&
+      X509Certificate::IsSameOSCert(
+          sort_begin->get()->certificate()->os_cert_handle(),
+          preferred_cert_orig->os_cert_handle())) {
     ++sort_begin;
   }
-  sort(sort_begin, sort_end, x509_util::ClientCertSorter());
+  sort(sort_begin, sort_end, ClientCertIdentitySorter());
 }
 
 }  // namespace
 
 ClientCertStoreMac::ClientCertStoreMac() {}
 
 ClientCertStoreMac::~ClientCertStoreMac() {}
 
 void ClientCertStoreMac::GetClientCerts(
     const SSLCertRequestInfo& request,
     const ClientCertListCallback& callback) {
   std::string server_domain = request.host_and_port.host();
 
-  ScopedCFTypeRef<SecIdentityRef> preferred_identity;
+  ScopedCFTypeRef<SecIdentityRef> preferred_sec_identity;
   if (!server_domain.empty()) {
     // See if there's an identity preference for this domain:
     ScopedCFTypeRef<CFStringRef> domain_str(
         base::SysUTF8ToCFStringRef("https://" + server_domain));
-    SecIdentityRef identity = NULL;
+    SecIdentityRef sec_identity = NULL;
     // While SecIdentityCopyPreferences appears to take a list of CA issuers
     // to restrict the identity search to, within Security.framework the
     // argument is ignored and filtering unimplemented. See
     // SecIdentity.cpp in libsecurity_keychain, specifically
     // _SecIdentityCopyPreferenceMatchingName().
     {
       base::AutoLock lock(crypto::GetMacSecurityServicesLock());
-      if (SecIdentityCopyPreference(domain_str, 0, NULL, &identity) == noErr)
-        preferred_identity.reset(identity);
+      if (SecIdentityCopyPreference(domain_str, 0, NULL, &sec_identity) ==
+          noErr)
+        preferred_sec_identity.reset(sec_identity);
     }
   }
 
   // Now enumerate the identities in the available keychains.
-  scoped_refptr<X509Certificate> preferred_cert = NULL;
-  CertificateList regular_certs;
+  std::unique_ptr<ClientCertIdentity> preferred_identity;
+  ClientCertIdentityList regular_identities;
 
   SecIdentitySearchRef search = NULL;
   OSStatus err;
   {
     base::AutoLock lock(crypto::GetMacSecurityServicesLock());
     err = SecIdentitySearchCreate(NULL, CSSM_KEYUSE_SIGN, &search);
   }
   if (err) {
-    callback.Run(CertificateList());
+    callback.Run(ClientCertIdentityList());
     return;
   }
   ScopedCFTypeRef<SecIdentitySearchRef> scoped_search(search);
   while (!err) {
-    SecIdentityRef identity = NULL;
+    ScopedCFTypeRef<SecIdentityRef> sec_identity;
     {
       base::AutoLock lock(crypto::GetMacSecurityServicesLock());
-      err = SecIdentitySearchCopyNext(search, &identity);
+      err = SecIdentitySearchCopyNext(search, sec_identity.InitializeInto());
     }
     if (err)
       break;
-    ScopedCFTypeRef<SecIdentityRef> scoped_identity(identity);
 
     SecCertificateRef cert_handle;
-    err = SecIdentityCopyCertificate(identity, &cert_handle);
+    err = SecIdentityCopyCertificate(sec_identity.get(), &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)) {
+    if (preferred_sec_identity &&
+        CFEqual(preferred_sec_identity, sec_identity.get())) {
       // Only one certificate should match.
-      DCHECK(!preferred_cert.get());
-      preferred_cert = cert;
+      DCHECK(!preferred_identity.get());
+      preferred_identity = base::MakeUnique<ClientCertIdentityMac>(
+          std::move(cert), std::move(sec_identity));
     } else {
-      regular_certs.push_back(cert);
+      regular_identities.push_back(base::MakeUnique<ClientCertIdentityMac>(
+          std::move(cert), std::move(sec_identity)));
     }
   }
 
   if (err != errSecItemNotFound) {
     OSSTATUS_LOG(ERROR, err) << "SecIdentitySearch error";
-    callback.Run(CertificateList());
+    callback.Run(ClientCertIdentityList());
     return;
   }
 
-  CertificateList selected_certs;
-  GetClientCertsImpl(preferred_cert, regular_certs, request, true,
-                     &selected_certs);
-  callback.Run(std::move(selected_certs));
+  ClientCertIdentityList selected_identities;
+  GetClientCertsImpl(std::move(preferred_identity),
+                     std::move(regular_identities), request, true,
+                     &selected_identities);
+  callback.Run(std::move(selected_identities));
 }
 
 bool ClientCertStoreMac::SelectClientCertsForTesting(
-    const CertificateList& input_certs,
+    ClientCertIdentityList input_identities,
     const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
-  GetClientCertsImpl(NULL, input_certs, request, false, selected_certs);
+    ClientCertIdentityList* selected_identities) {
+  GetClientCertsImpl(NULL, std::move(input_identities), request, false,
+                     selected_identities);
   return true;
 }
 
 bool ClientCertStoreMac::SelectClientCertsGivenPreferredForTesting(
-    const scoped_refptr<X509Certificate>& preferred_cert,
-    const CertificateList& regular_certs,
+    std::unique_ptr<ClientCertIdentity> preferred_identity,
+    ClientCertIdentityList regular_identities,
     const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
-  GetClientCertsImpl(
-      preferred_cert, regular_certs, request, false, selected_certs);
+    ClientCertIdentityList* selected_identities) {
+  GetClientCertsImpl(std::move(preferred_identity),
+                     std::move(regular_identities), request, false,
+                     selected_identities);
   return true;
 }
 
 #pragma clang diagnostic pop  // "-Wdeprecated-declarations"
 
 }  // namespace net