Fix client certificate authentication on Mac and Linux introduced in r178732

net/ssl/client_cert_store_impl_mac.cc

Index: net/ssl/client_cert_store_impl_mac.cc
diff --git a/net/ssl/client_cert_store_impl_mac.cc b/net/ssl/client_cert_store_impl_mac.cc
index ed10a7f38bd20037be0ea88e0a397dd4b54b1bf9..53c3a6e51d9d413f87ce76bf14fc828f393e695b 100644
--- a/net/ssl/client_cert_store_impl_mac.cc
+++ b/net/ssl/client_cert_store_impl_mac.cc
@@ -14,6 +14,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/strings/sys_string_conversions.h"
@@ -28,9 +29,96 @@ namespace net {
 
 namespace {
 
+// Gets the issuer for a given cert, starting with the cert itself and
+// including the intermediate and finally root certificates (if any).
+// This function calls SecTrust but doesn't actually pay attention to the trust
+// result: it shouldn't be used to determine trust, just to traverse the chain.
+// Caller is responsible for releasing the value stored into *out_cert_chain.
+OSStatus CopyCertChain(SecCertificateRef cert_handle,
+                       CFArrayRef* out_cert_chain) {
+  DCHECK(cert_handle);
+  DCHECK(out_cert_chain);
+
+  // Create an SSL policy ref configured for client cert evaluation.
+  SecPolicyRef ssl_policy;
+  OSStatus result = x509_util::CreateSSLClientPolicy(&ssl_policy);
+  if (result)
+    return result;
+  ScopedCFTypeRef<SecPolicyRef> scoped_ssl_policy(ssl_policy);
+
+  // Create a SecTrustRef.
+  ScopedCFTypeRef<CFArrayRef> input_certs(CFArrayCreate(
+      NULL, const_cast<const void**>(reinterpret_cast<void**>(&cert_handle)),
+      1, &kCFTypeArrayCallBacks));
+  SecTrustRef trust_ref = NULL;
+  {
+    base::AutoLock lock(crypto::GetMacSecurityServicesLock());
+    result = SecTrustCreateWithCertificates(input_certs, ssl_policy,
+                                            &trust_ref);
+  }
+  if (result)
+    return result;
+  ScopedCFTypeRef<SecTrustRef> trust(trust_ref);
+
+  // Evaluate trust, which creates the cert chain.
+  SecTrustResultType status;
+  CSSM_TP_APPLE_EVIDENCE_INFO* status_chain;
+  {
+    base::AutoLock lock(crypto::GetMacSecurityServicesLock());
+    result = SecTrustEvaluate(trust, &status);
+  }
+  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
+// certificate
+bool IsIssuedByInKeychain(const std::vector<std::string>& valid_issuers,
+                          scoped_refptr<X509Certificate>* cert) {
+  DCHECK(cert);
+  DCHECK(*cert);
+
+  X509Certificate::OSCertHandle cert_handle = (*cert)->os_cert_handle();
+  CFArrayRef cert_chain = NULL;
+  OSStatus result = CopyCertChain(cert_handle, &cert_chain);
+  if (result) {
+    OSSTATUS_LOG(ERROR, result) << "CopyCertChain error";
+    return false;
+  }
+
+  if (!cert_chain)
+    return false;
+
+  X509Certificate::OSCertHandles intermediates;
+  for (CFIndex i = 1, chain_count = CFArrayGetCount(cert_chain);
+       i < chain_count; ++i) {
+    SecCertificateRef cert = base::mac::CFCastStrict<SecCertificateRef>(
+        CFArrayGetValueAtIndex(cert_chain, i));
+    intermediates.push_back(cert);
+  }
+
+  scoped_refptr<X509Certificate> new_cert(X509Certificate::CreateFromHandle(
+      cert_handle, intermediates));
+  CFRelease(cert_chain);  // Also frees |intermediates|.
+
+  if (!new_cert->IsIssuedByEncoded(valid_issuers))

[Ryan Sleevi, 2013/04/16 02:32:50]

The downside to this approach is that |new_cert->os_cert_handle()| is checked
twice (first on line 147, since it's the same OSCertHandle, then again here). On
OS X, this forces the cert to be parsed twice into its NSS
representation/CachedCert, but I think this is a minor overhead for the code
simplicity - and for keeping this merge friendly (eg: not changing
IsIssuedByEncoded)

+    return false;
+
+  cert->swap(new_cert);
+  return true;
+}
+
 bool GetClientCertsImpl(const scoped_refptr<X509Certificate>& preferred_cert,
                         const CertificateList& regular_certs,
                         const SSLCertRequestInfo& request,
+                        bool query_keychain,

[wtc, 2013/04/16 18:50:32]

query_keychain probably should be documented.

It's not clear to me when query_keychain should be true.
Based on the three callers of this function, I can see the
rule is whether the caller is trying to *get* or *select*
client certs. But I don't know the rationale of this rule.
It would be nice to document this for future maintainers of
this code.

[Ryan Sleevi, 2013/04/16 19:00:26]

Good point. Will add comments to these functions. Also, see reply to NSS code
for more explanation.

                         CertificateList* selected_certs) {
   CertificateList preliminary_list;
   if (preferred_cert)
@@ -55,11 +143,12 @@ bool GetClientCertsImpl(const scoped_refptr<X509Certificate>& preferred_cert,
       continue;
 
     // Check if the certificate issuer is allowed by the server.
-    if (!request.cert_authorities.empty() &&
-        !cert->IsIssuedByEncoded(request.cert_authorities)) {
-      continue;
+    if (request.cert_authorities.empty() ||
+        cert->IsIssuedByEncoded(request.cert_authorities) ||
+        (query_keychain &&
+         IsIssuedByInKeychain(request.cert_authorities, &cert))) {
+      selected_certs->push_back(cert);
     }
-    selected_certs->push_back(cert);
   }
 
   // Preferred cert should appear first in the ui, so exclude it from the
@@ -147,14 +236,14 @@ bool ClientCertStoreImpl::GetClientCerts(const SSLCertRequestInfo& request,
     return false;
   }
 
-  return GetClientCertsImpl(preferred_cert, regular_certs, request,
+  return GetClientCertsImpl(preferred_cert, regular_certs, request, true,
                             selected_certs);
 }
 
 bool ClientCertStoreImpl::SelectClientCerts(const CertificateList& input_certs,
                                             const SSLCertRequestInfo& request,
                                             CertificateList* selected_certs) {
-  return GetClientCertsImpl(NULL, input_certs, request,
+  return GetClientCertsImpl(NULL, input_certs, request, false,
                             selected_certs);
 }
 
@@ -164,7 +253,7 @@ bool ClientCertStoreImpl::SelectClientCertsGivenPreferred(
       const CertificateList& regular_certs,
       const SSLCertRequestInfo& request,
       CertificateList* selected_certs) {
-  return GetClientCertsImpl(preferred_cert, regular_certs, request,
+  return GetClientCertsImpl(preferred_cert, regular_certs, request, false,
                             selected_certs);
 }
 #endif