Wire up SSL client authentication for OpenSSL/Android through the net/ stack

chrome/browser/ui/android/ssl_client_certificate_selector_android.cc

+// Copyright (c) 2012 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_selector_android.h"
+
+#include <string.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/callback.h"
+#include "base/logging.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_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"
+
+// On other platforms, the list of client certificates compatible with
+// the SSLCertRequestInfo is built using system APIs that do not require
+// user interaction. After this, ShowSSLClientCertificateSelector() is
+// merely used to display a tab sub-window asking the user to select
+// one of these certificates.
+
+// On Android, things are a bit different, because getting the list of
+// compatible client certificates is only possible using an API that shows
+// a system UI dialog. More precisely:
+//
+// - The application must call KeyChain.choosePrivateKeyAlias() and
+//   pass it the request parameters directly.
+//
+// - This API always launches a system activity (CertInstaller), that
+//   will display a list of compatible installed client certificates,
+//   if any, or prompt the user to install one manually otherwise.
+//
+// - Also, the first time this API is called, the CertInstaller will
+//   first prompt the user to enter the secure storage's password
+//   (which is the user's PIN code / password by default). This establishes
+//   a trust relationship between the KeyChain system application, and
+//   the application calling the API. It persists until the application
+//   is killed.
+//
+// - The client certificate selection result is sent back to the
+//   application through a UI thread callback. It only contains a
+//   string alias for the selected certificate, or 'null' to indicate
+//   that the user has canceled the selection, or couldn't unlock
+//   access to the secure storage.
+//
+// Note that:
+//
+// - There is no way, when the result if 'null', to know from the
+//   application if the user cancelled the request, or couldn't access
+//   the secure storage.
+//
+// - There is no way to cancel a request once it has started. Each call
+//   to KeyChain.choosePrivateKeyAlias() launches a new activity, which
+//   runs in a completely different process, and steals the focus from
+//   the browser.
+
+namespace {
+
+typedef crypto::ScopedOpenSSL<EVP_PKEY, EVP_PKEY_free> ScopedEVP_PKEY;
+
+}  // namespace
+
+namespace browser {

[Ryan Sleevi, 2013/02/12 00:25:17]

This should be in "namespace chrome {"

The unnamed namespace, in Chromium code, is vastly more common inside the
namespace than outside (eg: "namespace chrome { namespace browser { namespace {"
)

[digit1, 2013/02/12 15:05:25]

Done.

+
+// 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 SSLClientCertRequest::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) {

[Ryan Sleevi, 2013/02/12 00:25:17]

BUG: > rather than >= ?

[digit1, 2013/02/12 15:05:25]

That's embarrassing :) For some reason I've always been convinced that 65535 is
not a valid port number. Looking at all documentation available shows this is
not the case, so I'll fix this. Thanks.

+    LOG(ERROR) << "Invalid port number in: "
+               << request_info->host_and_port << "'";
+    return false;
+  }
+
+  return Java_SSLClientCertificateRequest_selectClientCertificate(
+      env, this_java, key_types_ref.obj(), principals_ref.obj(),
+      host_name_ref.obj(), port, NULL);
+}
+
+void SSLClientCertRequest::OnRequestCompletion(
+    JNIEnv* env,
+    jobject obj,
+    jstring private_key_alias_ref,
+    jobjectArray encoded_chain_ref,
+    jobject private_key_ref) {
+
+  net::X509Certificate* result = NULL;
+
+  do {

[Ryan Sleevi, 2013/02/12 00:25:17]

Chromium code specifically tries to avoid patterns like this (since they're
effectively goto)

You can rewrite it as:

scoped_ptr<SSLClientCertRequest> guard(this);
ScopedClosureRunner callback_guard(base::Bind(callback_, NULL));
....
if (some_error_condition)
  return;

....
// success condition here

closure_guard.Release(); /* rsleevi: This is so our error condition isn't run */
callback_.Run(result);

[digit1, 2013/02/12 15:05:25]

Thanks for the suggestion, I've applied your rewrite to the code.

+    // When the request is cancelled by the user.
+    if (private_key_alias_ref == NULL || private_key_ref == NULL) {
+      LOG(INFO) << "Client certificate request cancelled";

[Ryan Sleevi, 2013/02/12 20:12:58]

seems unnecessarily log spammy (and throughout)

[digit1, 2013/02/13 18:24:34]

Actually, given the level of platform bugs we've encountered, I'd like to keep
this in the log in order to potentially diagnose problems when this is deployed
in the wild. I expect surprises due to OEM enhancements, so the most messages
corresponding to unexpected use cases, the better, at this point.

Also, it's not really spammy. It can only be displayed when the UI dialog was
cancelled by the user, which will happen rarely in practice (versus, say
connecting to the server, which will reuse cached credentials).

[Ryan Sleevi, 2013/02/13 23:32:26]

We specifically discourage logging for the "nice to have, maybe". It comes with
a fix cost to all binaries. For desktop, we recommend DLOG or, more preferably,
DVLOG(1) - DVLOG(3). We went through mass purges of removing DLOG(INFO) and
LOG(INFO) to put them to appropriate levels.

If you look at
https://code.google.com/p/chromium/codesearch#search/&q=%5B%5ED%5DLOG%5C(INFO...

you will see this to be true.

[digit1, 2013/02/14 06:23:50]

Point taken then, I'll remove the line.

+      break;
+    }
+
+    // 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.
+    scoped_refptr<net::X509Certificate> cert(
+        net::X509Certificate::CreateFromDERCertChain(encoded_chain));
+    if (!cert.get()) {
+      LOG(ERROR) << "Could not decode client certificate chain";
+      break;
+    }
+
+    // Create an EVP_PKEY wrapper for the private key JNI reference, then
+    // ensure that it will be used for the rest of the SSL handshake that
+    // needs it. If this fails, log an error, but don't exit prematurely.
+    ScopedEVP_PKEY private_key(
+        net::android::GetOpenSSLPrivateKeyWrapper(private_key_ref));
+
+    if (!private_key.get()) {
+        LOG(ERROR) << "Could not create OpenSSL wrapper for private key";
+        break;
+    }
+
+    net::UseOpenSSLClientCertSigningPrivateKey(*cert.get(), private_key.get());
+
+    cert.swap(&result);
+
+  } while (0);
+
+  // Done, pass the certificate to the callback now.
+  // Note: the callback takes ownership of the certificate.
+  callback_.Run(result);
+  callback_.Reset();
+
+  // Delete the request now.
+  delete this;
+}
+
+bool RegisterSSLClientCertificateSelectorAndroid(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace browser
+
+namespace chrome {
+
+// Client Auth is not implemented on Android yet.

[Ryan Sleevi, 2013/02/12 20:12:58]

This function isn't correct, is it?

+void ShowSSLClientCertificateSelector(
+    content::WebContents* contents,
+    const net::HttpNetworkSession* network_session,
+    net::SSLCertRequestInfo* cert_request_info,
+    const chrome::SelectCertificateCallback& callback) {
+  DVLOG(1) << __FUNCTION__ << " " << contents;

[Ryan Sleevi, 2013/02/12 20:12:58]

This seems extra extra spammy. Minimally, why log __FUNCTION__ here? Why isn't
the dvlog, which already contains file and line, sufficient?

Why log the contents pointer?

[digit1, 2013/02/13 18:24:34]

This came directly from the version in
chrome/browser/ui/views/ssl_client_certificate_selector.cc. I've removed it
since it's not needed anyway. 

+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Create a new request, then try to start it.
+  browser::SSLClientCertRequest* request =
+      new browser::SSLClientCertRequest(cert_request_info, callback);
+  if (!request->Start()) {
+    // If the request could not start for some reason, consider
+    // that no client certificate was selected.
+    callback.Run(NULL);
+    delete request;
+  }
+  // Note: A started request will delete itself when it receives
+  //       an answer from the system.
+}
+
+}  // namespace chrome