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

chrome/browser/ui/android/ssl_client_certificate_selector.cc

Index: chrome/browser/ui/android/ssl_client_certificate_selector.cc
diff --git a/chrome/browser/ui/android/ssl_client_certificate_selector.cc b/chrome/browser/ui/android/ssl_client_certificate_selector.cc
index 7d91dc10c7f32e026fe187eb5760abed49f040c6..c0e918b3201f1beafdd723837f535141d4dcda64 100644
--- a/chrome/browser/ui/android/ssl_client_certificate_selector.cc
+++ b/chrome/browser/ui/android/ssl_client_certificate_selector.cc
@@ -4,17 +4,197 @@
 
 #include "chrome/browser/ssl/ssl_client_certificate_selector.h"
 
+#include <openssl/evp.h>
+
+#include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/ui/android/ssl_client_certificate_request.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/android/keystore_openssl.h"
+#include "net/base/openssl_client_key_store.h"
+#include "net/base/ssl_cert_request_info.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.
+//
 
 namespace chrome {
 
-// Client Auth is not implemented on Android yet.
+using chrome::android::SSLClientCertificateRequest;
+
+namespace {
+
+typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
+
+// Helper class used to guarantee that a callback.Run(NULL) is called on
+// scope exit, unless Release() was used before.
+class CallbackGuard {
+ public:
+  explicit CallbackGuard(chrome::SelectCertificateCallback* callback)
+    : callback_(callback) {
+  }
+
+  ~CallbackGuard() {
+    if (callback_)
+      base::ResetAndReturn(callback_).Run(NULL);
+  }
+
+  void Release() {
+    callback_ = NULL;
+  }
+
+ private:
+  chrome::SelectCertificateCallback* callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackGuard);
+};
+
+// Helper class used to pass a (certificate,private key) pair between
+// threads.
+class CertificateAndKey {

[Ryan Sleevi, 2013/03/05 18:02:41]

Why is this a class and not a struct

[digit1, 2013/03/06 01:48:33]

It used to require to be a class when if was refcounted. I'll simplify further.

+ public:
+  CertificateAndKey() { }
+  ~CertificateAndKey() { }

[Ryan Sleevi, 2013/03/05 18:02:41]

no spaces in braces

+  scoped_refptr<net::X509Certificate> client_cert;
+  ScopedEVP_PKEY private_key;

[Ryan Sleevi, 2013/03/05 18:02:41]

It's still not clear to me why you need this class - ScopedEVP_PKEY derives from
scoped_ptr<>, so you should get .Pass() (and base::Passed) for free. You
shouldn't need to wrap this in a helper at all.

[digit1, 2013/03/06 01:48:33]

I've originally tried passing a ScopedEVP_PKEY directly, but this fails to
compile with a cryptic error message, i.e.:

....
../../base/bind_internal.h:2590:9: error: no matching constructor for
initialization of 'scoped_ptr<evp_pkey_st,
net::OpenSSLClientKeyStore::EVP_PKEY_Deleter>'
        p2_(p2) {

It looks like the problem is that ScopedEVP_PKEY doesn't have a
copy-constructor, but it's hard to understand why. Hence, the use of a helper
class/struct to wrap it.

[Ryan Sleevi, 2013/03/06 01:55:29]

You have to use base::Passed() here, which will create an object that will use
scoped_ptr<>::Pass() [which is our C++11 move emulation - aka, almost-as-good-as
a copy ctor)

base::Bind(
   &DoRecordClientCertificateKey, certificate, base::Passed(private_key));

// private_key is now NULL, ownership is transferred to the callback

This is covered in
http://dev.chromium.org/developers/coding-style/important-abstractions-and-da...

[digit1, 2013/03/06 21:34:11]

thanks, this works and has been implemented in the latest patch. The latter also
contains multiple simplifications that gets rid of the helper struct, or any
extra C++ classes (it's now purely based on callbacks).

+ private:
+  DISALLOW_COPY_AND_ASSIGN(CertificateAndKey);
+};
+
+// A SSLClientCertificateRequest used to pass the client
+// certificate to the proper callback, after recording its private
+// key into the net::OpenSSLClientKeyStore.
+class Request : public SSLClientCertificateRequest {
+ public:
+  explicit Request(const chrome::SelectCertificateCallback& callback)
+    : callback_(callback) {
+  }
+
+  virtual void OnCertificateSelected(
+      std::vector<base::StringPiece>* encoded_chain,
+      jobject private_key) OVERRIDE;
+
+ protected:
+  ~Request() { }
+
+ private:
+  Request();
+
+  // Called on the I/O thread to record a (certificate,private_key) pair
+  // into the net::OpenSSLClientKeyStore.
+  static void DoRecordClientCertificateKey(
+      scoped_ptr<CertificateAndKey> pair);
+
+  chrome::SelectCertificateCallback callback_;
+};
+
+// Called on the UI thread once the user has selected a client certificate
+// using the system-provided dialog, or failed to select one.
+// |encoded_chain| is a pointer to the encoded client certificate chain.
+// Each item in the list is a DER-encoded X.509 certificate.
+// |private_key| is a pointer to a JNI local reference to the platform
+// PrivateKey object for this client certificate chain.
+void Request::OnCertificateSelected(
+    std::vector<base::StringPiece>* encoded_chain,
+    jobject private_key_ref) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Ensure callback_.Run(NULL) is called on error exit.
+  CallbackGuard guard(&callback_);
+
+  if (encoded_chain == NULL || private_key_ref == NULL) {
+    LOG(ERROR) << "Client certificate selection cancelled by the user";
+    return;
+  }
+
+  scoped_ptr<CertificateAndKey> pair(new CertificateAndKey);
+
+  // Create the X509Certificate object from the encoded chain.
+  pair->client_cert =
+      net::X509Certificate::CreateFromDERCertChain(*encoded_chain);
+  if (!pair->client_cert.get()) {
+    LOG(ERROR) << "Could not decode client certificate chain";
+    return;
+  }
+
+  // Create an EVP_PKEY wrapper for the private key JNI reference.
+  pair->private_key.reset(
+      net::android::GetOpenSSLPrivateKeyWrapper(private_key_ref));
+  if (!pair->private_key.get()) {
+      LOG(ERROR) << "Could not create OpenSSL wrapper for private key";
+      return;
+  }
+
+  guard.Release();
+
+  // DoRecordClientCertificateKey() must be called on the I/O thread,
+  // before the callback is called with the selected certificate on
+  // the UI thread.
+  //
+  // Note that the UI thread closure is computed first, because |pair|
+  // becomes NULL after the base::Passed() call, and the order of
+  // parameter evaluation is unspecified in C++.
+  base::Closure on_ui_thread =
+      base::Bind(callback_, pair->client_cert);
+
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&Request::DoRecordClientCertificateKey,
+                 base::Passed(&pair)),
+      on_ui_thread);
+}
+
+void Request::DoRecordClientCertificateKey(
+    scoped_ptr<CertificateAndKey> pair) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  net::OpenSSLClientKeyStore::GetInstance()->RecordClientCertPrivateKey(
+      pair->client_cert.get(), pair->private_key.get());
+}
+
+}  // namespace
+
 void ShowSSLClientCertificateSelector(
     content::WebContents* contents,
     const net::HttpNetworkSession* network_session,
     net::SSLCertRequestInfo* cert_request_info,
-    const base::Callback<void(net::X509Certificate*)>& callback) {
-  NOTIMPLEMENTED();
+    const chrome::SelectCertificateCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  scoped_refptr<Request> request(new Request(callback));
+  if (!request->Start(cert_request_info)) {
+    LOG(ERROR) << "Could not start client certificate request!";
+    callback.Run(NULL);

[Ryan Sleevi, 2013/03/05 18:02:41]

DESIGN: In general, we avoid recursing directly into callbacks that have
asynchronous lifetimes, to avoid infinite stack recursion. This is why we follow
the pattern of "Return a pending error or result".

Since this API is defined as purely asynchronous (eg: no immediate result) -
which, to clarify, **is fine** - then the pattern should be that we don't
immediately run |callback|, but instead defer it (eg: via PostTask).

[digit1, 2013/03/06 01:48:33]

This has been addressed in the last patch, thanks.

+  }
 }
 
 }  // namespace chrome