Refactor client cert private key handling.

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..88a70ac53a9f888788384da4710795af2ce6134b 100644
--- a/net/ssl/client_cert_store_mac.cc
+++ b/net/ssl/client_cert_store_mac.cc
@@ -17,6 +17,7 @@
 #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"
@@ -24,6 +25,7 @@
 #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;
 
@@ -88,12 +90,11 @@ OSStatus CopyCertChain(SecCertificateRef cert_handle,
 // certificates. If it is, |*cert| is updated to point to the completed
 // certificate
 bool IsIssuedByInKeychain(const std::vector<std::string>& valid_issuers,
-                          scoped_refptr<X509Certificate>* cert) {
+                          ClientCertIdentity* cert) {
   DCHECK(cert);
-  DCHECK(cert->get());
 
   base::ScopedCFTypeRef<SecCertificateRef> os_cert(
-      x509_util::CreateSecCertificateFromX509Certificate(cert->get()));
+      x509_util::CreateSecCertificateFromX509Certificate(cert->certificate()));
   if (!os_cert)
     return false;
   CFArrayRef cert_chain = NULL;
@@ -122,7 +123,10 @@ bool IsIssuedByInKeychain(const std::vector<std::string>& valid_issuers,
   if (!new_cert || !new_cert->IsIssuedByEncoded(valid_issuers))
     return false;
 
-  cert->swap(new_cert);
+  // TODO(mattm): could do this in a more efficient way (not creating
+  // new_cert), if there was a version of IsIssuedByEncoded that only took
+  // OSCertHandles.
+  cert->SetIntermediates(new_cert->GetIntermediateCertificates());
   return true;
 }
 
@@ -184,51 +188,58 @@ bool SupportsSSLClientAuth(SecCertificateRef cert) {
 // 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_cert,
+                        ClientCertIdentityList regular_certs,
                         const SSLCertRequestInfo& request,
                         bool query_keychain,
-                        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_certs) {
+  scoped_refptr<X509Certificate> preferred_cert_orig;
+  ClientCertIdentityList preliminary_list(std::move(regular_certs));
+  if (preferred_cert) {
+    preferred_cert_orig = preferred_cert->certificate();
+    preliminary_list.insert(preliminary_list.begin(),
+                            std::move(preferred_cert));
+  }
 
   selected_certs->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());
+        [&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())
       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_certs->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_certs->begin();
+  ClientCertIdentityList::iterator sort_end = selected_certs->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
@@ -247,7 +258,7 @@ void ClientCertStoreMac::GetClientCerts(
     // 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
@@ -255,14 +266,16 @@ void ClientCertStoreMac::GetClientCerts(
     // _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_identity.reset(sec_identity);
     }
   }
 
   // Now enumerate the identities in the available keychains.
-  scoped_refptr<X509Certificate> preferred_cert = NULL;
-  CertificateList regular_certs;
+  // scoped_refptr<X509Certificate> preferred_cert = NULL;
+  std::unique_ptr<ClientCertIdentity> preferred_cert;
+  ClientCertIdentityList regular_certs;
 
   SecIdentitySearchRef search = NULL;
   OSStatus err;
@@ -271,22 +284,21 @@ void ClientCertStoreMac::GetClientCerts(
     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);
@@ -300,42 +312,45 @@ void ClientCertStoreMac::GetClientCerts(
     if (!cert)
       continue;
 
-    if (preferred_identity && CFEqual(preferred_identity, identity)) {
+    if (preferred_identity && CFEqual(preferred_identity, sec_identity.get())) {
       // Only one certificate should match.
       DCHECK(!preferred_cert.get());
-      preferred_cert = cert;
+      preferred_cert = base::MakeUnique<ClientCertIdentityMac>(
+          std::move(cert), std::move(sec_identity));
     } else {
-      regular_certs.push_back(cert);
+      regular_certs.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);
+  ClientCertIdentityList selected_certs;
+  GetClientCertsImpl(std::move(preferred_cert), std::move(regular_certs),
+                     request, true, &selected_certs);
   callback.Run(std::move(selected_certs));
 }
 
 bool ClientCertStoreMac::SelectClientCertsForTesting(
-    const CertificateList& input_certs,
+    ClientCertIdentityList input_certs,
     const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
-  GetClientCertsImpl(NULL, input_certs, request, false, selected_certs);
+    ClientCertIdentityList* selected_certs) {
+  GetClientCertsImpl(NULL, std::move(input_certs), request, false,
+                     selected_certs);
   return true;
 }
 
 bool ClientCertStoreMac::SelectClientCertsGivenPreferredForTesting(
-    const scoped_refptr<X509Certificate>& preferred_cert,
-    const CertificateList& regular_certs,
+    std::unique_ptr<ClientCertIdentity> preferred_cert,
+    ClientCertIdentityList regular_certs,
     const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
-  GetClientCertsImpl(
-      preferred_cert, regular_certs, request, false, selected_certs);
+    ClientCertIdentityList* selected_certs) {
+  GetClientCertsImpl(std::move(preferred_cert), std::move(regular_certs),
+                     request, false, selected_certs);
   return true;
 }