Add Android support for SSL client authentication to the browser layer.

chrome/browser/ui/android/ssl_client_certificate_request.cc

+// Copyright (c) 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 "chrome/browser/ui/android/ssl_client_certificate_request.h"
+
+#include <openssl/evp.h>
+#include <openssl/x509.h>
+
+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/ssl/ssl_client_auth_observer.h"
+#include "content/public/browser/browser_thread.h"
+#include "crypto/openssl_util.h"
+#include "jni/SSLClientCertificateRequest_jni.h"
+#include "net/android/keystore_openssl.h"
+#include "net/base/host_port_pair.h"
+#include "net/base/openssl_client_key_store.h"
+#include "net/base/ssl_cert_request_info.h"
+#include "net/base/ssl_client_cert_type.h"
+#include "net/base/x509_certificate.h"
+
+namespace browser {
+namespace android {
+
+using content::BrowserThread;
+
+namespace {
+
+typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
+
+}  // namespace
+
+SSLClientCertificateRequest::SSLClientCertificateRequest(
+    net::SSLCertRequestInfo* cert_request_info,
+    const chrome::SelectCertificateCallback& callback)
+    : cert_request_info_(cert_request_info),
+      client_cert_(NULL),
+      private_key_(NULL),
+      callback_(callback) {
+}
+
+SSLClientCertificateRequest::~SSLClientCertificateRequest() {
+  if (!callback_.is_null()) {

[Ryan Sleevi, 2013/02/28 19:42:42]

Under what situations is this possible?

I understand that it's a catch for when DoSendClientCertificate() has not been
called (which would run and reset the callback), but it's not clear when that
can actually happen.

I'd also recommend you use a temporary for your .Run/.Reset() paradigm. I'm
concerned about a situation where a .Run of a callback causes the current
SSLClientCertificateRequest to be deleted. In the existing paradigm (.Run() then
.Reset()), it's possible for the callback to be run twice
(DoSendClientCertificate -> Run() -> dtor -> Run())

A safer pattern would be to use base::ResetAndReturn(&callback_).Run(NULL),
which will swap out with a temporary before running.

However, you should only need that in the DoSend path, AFAICT.

[digit1, 2013/03/04 19:03:20]

This can happen when an error exit happened in OnRequestCompletion().
Note that this is not part of the destructor in the newest version of the code.
I don't think this is totally necessary, because the callback object don't know
anything about the request object, but I'll add that to the code for safety.

+    callback_.Run(NULL);
+    callback_.Reset();
+  }
+}
+
+// Start a new client certificate request. This launches a system
+// UI dialog to let the user choose a certificate matching the
+// SSLCertRequestInfo.
+// |request_info| is the SSL client certificate request info.
+// Returns true on success, or false otherwise.
+// Note that success simply means that the system activity that
+// implements client certificate selection has been launched. The only
+// way to know if the user has properly selected a certificate (versus
+// no certificate being available, or the user cancelling the operation)
+// is to look at the value sent back in nativeOnRequestCompletion()
+// below.
+bool SSLClientCertificateRequest::Start() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  net::SSLCertRequestInfo* request_info = cert_request_info_;
+
+  // Convert the object's address into a pointer that will be passed
+  // to the Java method through JNI.
+  jint this_java = reinterpret_cast<jint>(this);
+
+  // Build the |key_types| JNI parameter, as a String[]
+  std::vector<std::string> key_types;
+  for (size_t n = 0; n < request_info->cert_key_types.size(); ++n) {
+    switch (request_info->cert_key_types[n]) {
+      case net::CLIENT_CERT_RSA_SIGN:
+        key_types.push_back("RSA");
+        break;
+      case net::CLIENT_CERT_DSS_SIGN:
+        key_types.push_back("DSA");
+        break;
+      case net::CLIENT_CERT_ECDSA_SIGN:
+        key_types.push_back("ECDSA");
+        break;
+      default:
+        // Ignore unknown types.
+        break;
+    }
+  }
+  ScopedJavaLocalRef<jobjectArray> key_types_ref =
+      base::android::ToJavaArrayOfStrings(env, key_types);
+  if (key_types_ref.is_null()) {
+    LOG(ERROR) << "Could not create key types array (String[])";
+    return false;
+  }
+
+  // Build the |encoded_principals| JNI parameter, as a byte[][]
+  ScopedJavaLocalRef<jobjectArray> principals_ref =
+      base::android::ToJavaArrayOfByteArray(
+          env, request_info->cert_authorities);
+  if (principals_ref.is_null()) {
+    LOG(ERROR) << "Could not create principals array (byte[][])";
+    return false;
+  }
+
+  // Build the |host_name| and |port| JNI parameters, as a String and
+  // a jint.
+  net::HostPortPair host_and_port =
+      net::HostPortPair::FromString(request_info->host_and_port);
+
+  ScopedJavaLocalRef<jstring> host_name_ref =
+      base::android::ConvertUTF8ToJavaString(env, host_and_port.host());
+  if (host_name_ref.is_null()) {
+    LOG(ERROR) << "Could not extract host name from: '"
+               << request_info->host_and_port << "'";
+    return false;
+  }
+
+  jint port = host_and_port.port();
+  if (port <= 0 || port > 65535) {
+    LOG(ERROR) << "Invalid port number in: "
+               << request_info->host_and_port << "'";
+    return false;
+  }
+
+  // Increment reference count to ensure request object is not deleted
+  // before OnRequestCompletion is called.
+  this->AddRef();
+
+  return Java_SSLClientCertificateRequest_selectClientCertificate(
+      env, this_java, key_types_ref.obj(), principals_ref.obj(),
+      host_name_ref.obj(), port);
+}
+
+void SSLClientCertificateRequest::OnRequestCompletion(
+    JNIEnv* env,
+    jobject obj,
+    jstring private_key_alias_ref,
+    jobjectArray encoded_chain_ref,
+    jobject private_key_ref) {
+
+  // Ensure that the request object is destroyed when the
+  // function leaves on error. Note that the destructor calls
+  // callback_.Run(NULL) automatically in this case.
+  scoped_refptr<SSLClientCertificateRequest> guard(this);
+
+  // Undo the AddRef() from Start().
+  this->Release();
+
+  // When the request is cancelled by the user.
+  if (private_key_alias_ref == NULL || private_key_ref == NULL) {
+    LOG(ERROR) << "Client certificate request cancelled";
+    return;
+  }
+
+  // Convert private key alias JNI reference to string.
+  std::string private_key_alias;
+  if (private_key_alias_ref) {
+    private_key_alias = base::android::ConvertJavaStringToUTF8(
+        env, private_key_alias_ref);
+  }
+
+  // Convert the encoded chain to a vector of strings.
+  std::vector<std::string> encoded_chain_strings;
+  if (encoded_chain_ref) {
+    base::android::JavaArrayOfByteArrayToStringVector(
+        env, encoded_chain_ref, &encoded_chain_strings);
+  }
+
+  std::vector<base::StringPiece> encoded_chain;
+  for (size_t n = 0; n < encoded_chain_strings.size(); ++n)
+    encoded_chain.push_back(encoded_chain_strings[n]);
+
+  // Create the X509Certificate object from the encoded chain.
+  client_cert_ =
+      net::X509Certificate::CreateFromDERCertChain(encoded_chain);
+  if (!client_cert_.get()) {
+    LOG(ERROR) << "Could not decode client certificate chain";
+    return;
+  }
+
+  // Create an EVP_PKEY wrapper for the private key JNI reference.
+  private_key_.reset(
+      net::android::GetOpenSSLPrivateKeyWrapper(private_key_ref));
+  if (!private_key_.get()) {
+      LOG(ERROR) << "Could not create OpenSSL wrapper for private key";
+      return;
+  }
+
+  // The next step must happen in the I/O thread.
+  // This transfers ownership of the request object to the closure.
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(
+          &SSLClientCertificateRequest::DoRecordClientCertificateKey,
+          this));

[Ryan Sleevi, 2013/02/28 19:42:42]

Because this object is RefCounted, you're forcing it to continue living on the
multiple threads (IO and UI).

Was that intentional? It seems like you could use a PostTaskAndReply idiom with
a helper for DoRecordClientCertificateKey in the implementation, rather than
having this be both ref-counted and living on multiple threads.

[digit1, 2013/03/04 19:03:20]

Indeed, the object doesn't have to live in multiple threads. I've changed the
code to use PostTaskAndReply() which allows it to live exclusively in the UI
thread. Only the certificate and the private key need to be sent to the I/O
thread, so I've created a small helper class to do that (ScopedEVP_PKEY is not a
real scoped_refptr<> so cannot be passed directly in a base::Bind() call).

+}
+
+// Must be called on the I/O thread to add the client certificate's
+// private key to the OpenSSLClientKeyStore.
+// |request| is the target request object.
+// |cert| is the client certificate.
+// |private_key| is the client certificate's private key.
+void SSLClientCertificateRequest::DoRecordClientCertificateKey() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  net::OpenSSLClientKeyStore* key_store =
+      net::OpenSSLClientKeyStore::GetInstance();
+  if (!key_store->RecordClientCertPrivateKey(client_cert_.get(),
+                                             private_key_.get())) {
+      LOG(ERROR) << "Could not add key to OpenSSL private key store";
+      client_cert_ = NULL;
+  }
+
+  // Request ownership is transfered to new closure.
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(
+          &SSLClientCertificateRequest::DoSendClientCertificate, this));
+}
+
+
+void SSLClientCertificateRequest::DoSendClientCertificate() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  callback_.Run(client_cert_.get());
+  callback_.Reset();
+
+  // Request object will be destroyed when this method exits.
+}
+
+}  // namespace android
+}  // namespace browser
+
+bool RegisterSSLClientCertificateRequestAndroid(JNIEnv* env) {
+  return browser::android::RegisterNativesImpl(env);
+}