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..ca13b7b90e9949b9eef3f34dc6c4723acdd8a094 100644
--- a/chrome/browser/ui/android/ssl_client_certificate_selector.cc
+++ b/chrome/browser/ui/android/ssl_client_certificate_selector.cc
@@ -4,17 +4,202 @@
 
 #include "chrome/browser/ssl/ssl_client_certificate_selector.h"
 
+#include <openssl/evp.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 browser::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 Reset() was used before.
+class CallbackGuard {

[Ryan Sleevi, 2013/03/04 23:47:21]

We already have this, in src/base/bind_helpers.h

See ScopedClosureRunner

ScopedClosureRunner guard(base::Bind(callback_, NULL));
....
if (success) {
  guard.Release();
  callback_.Run(SomeResult);
}

[digit1, 2013/03/05 16:54:00]

I'm not sure I didn't already mentioned already, but your proposal doesn't
compile at all (even adding the trivial missing base:: prefix to
ScopedClosureRunner). The error message is unreadable, even with Clang, so I had
to design this helper instead. Let me know if you have a better proposal though.

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

Can you paste the error? I really want to avoid creating another instance of an
existing class.

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

Ok, the error was due to the NULL inside the base::Bind() call, the compiler
complained that it couldn't cast or match a net::X509Certificate* into an int. I
could solve this with the following:

  base::ScopedClosureRunner  guard(
      base::Bind(callback_, scoped_refptr<net::X509Certificate>()));

I guess that's why nullptr would be nice in the future. I'll upload a new patch
soon, I'm still experimenting with your other suggestions.

+ public:
+  explicit CallbackGuard(chrome::SelectCertificateCallback& callback)

[Ryan Sleevi, 2013/03/04 23:47:21]

I've mentioned it in past reviews, but in Google C++ & in Chrome, we do not pass
non-const references.

[digit1, 2013/03/05 16:54:00]

Sorry about that, I've fixed it.

+    : callback_(&callback) {
+  }
+
+  ~CallbackGuard() {
+    if (callback_)
+      base::ResetAndReturn(callback_).Run(NULL);
+  }
+
+  void Reset() {
+    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
+    : public base::RefCountedThreadSafe<CertificateAndKey> {

[Ryan Sleevi, 2013/03/04 23:47:21]

1) We have RefCountedData<T> for this
2) You shouldn't need to RefCount - a simple struct with a scoped_ptr<> should
be sufficient to pass between threads using base::Passed()

[digit1, 2013/03/05 16:54:00]

1) It looks like RefCountedData<> would be appropriate if there was only a
single member here. Unless you recommend using RefCountedData<std::pair<
scoped_refptr<net::X509Certificate>, ScopedEVP_PKEY> >, which seems less
readable to me (especially at use sites).

2) Thanks, I wasn't sure about that, the PostTaskAndReply() documentation uses a
RefCountedThreadSafe example, so I erred on the side of caution.

+ public:
+  CertificateAndKey() { }
+
+  scoped_refptr<net::X509Certificate> client_cert;
+  ScopedEVP_PKEY private_key;
+ private:
+  friend class base::RefCountedThreadSafe<CertificateAndKey>;
+
+  ~CertificateAndKey() { }
+
+  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 {

[Ryan Sleevi, 2013/03/04 23:47:21]

I'm not sure why you've got the inheritance route. It seems like added overhead
of a vtable for what seems like a build system issue?

[digit1, 2013/03/05 16:54:00]

This change reflect's Marcus' recommendation. He mentioned using a delegate but
this would require the same number of vtables anyway.

Would you prefer to add openssl as a dependency to the 'browser_test' target
instead? i.e. go back to the old design, or do you have something different in
mind?

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

Can you not just pass a base::Closure/Callback in the ctor of the
SSLClientCertRequest, and use that. It avoids the vtable of passing a delegate,
and avoids the vtable overhead of subclassing.

+ public:
+  explicit Request(const chrome::SelectCertificateCallback& callback)
+    : callback_(callback) {
+  }
+
+  virtual void OnCompletion(
+      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_refptr<CertificateAndKey> pair);

[Ryan Sleevi, 2013/03/04 23:47:21]

I've pointed out in past reviews, only scoped_ptr<> is passed by-value.

[digit1, 2013/03/05 16:54:00]

The above line comes from the fact that a scoped_ptr<> couldn't be used with a
refcounted type, but since CertificateAndKey no longer requires this constraint,
I've changed the code to use scoped_ptr<>.

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

I think you missed what I was saying: scoped_refptr<>'s are passed as const-ref
or pointers, not as values. scoped_ptr<>s are the only one we pass by value (in
conjunction with Pass())

+
+  // Called on the UI thread, as a reply to DoRecordClientCertificateKey(),
+  // to send the selected certificate to the callback.
+  void DoSendCertificate(scoped_refptr<net::X509Certificate> client_cert);
+
+  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::OnCompletion(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_refptr<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.Reset();
+
+  // DoRecordClientCertificateKey() must be called on the I/O thread,
+  // before DoSendCertificate() can be called on the UI thread to pass
+  // the client certificate to the callback.
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&Request::DoRecordClientCertificateKey, pair),
+      base::Bind(&Request::DoSendCertificate, this, pair->client_cert));

[Ryan Sleevi, 2013/03/04 23:47:21]

Why have a DoSendCertificate helper?

Why not simply
base::Bind(callback_, pair->client_cert) ?

You've already .Reset the guard (line 165), so you no longer have to worry about
callback_ triggering on dtor, so I'm not sure why you loop back through this
object.

[digit1, 2013/03/05 16:54:00]

That's an excellent suggestion, I didn't think about it but I've implemented it
in the latest patch. Thanks.

+}
+
+void Request::DoRecordClientCertificateKey(
+    scoped_refptr<CertificateAndKey> pair) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  net::OpenSSLClientKeyStore::GetInstance()->RecordClientCertPrivateKey(
+      pair->client_cert.get(), pair->private_key.get());
+}
+
+void Request::DoSendCertificate(
+    scoped_refptr<net::X509Certificate> client_cert) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  base::ResetAndReturn(&callback_).Run(client_cert);
+}
+
+}  // 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));

[Ryan Sleevi, 2013/03/04 23:47:21]

Not sure why the RefCounting

[digit1, 2013/03/05 16:54:00]

The refcounting is used to ensure that the request is destroy through Release()
when Start() call fails (if it succeeds, it increments the request's reference
count).

A simple scoped_ptr<> would not compile since the class is refcounted.

+  if (!request->Start(cert_request_info)) {
+    LOG(ERROR) << "Could not start client certificate request!";
+    callback.Run(NULL);
+  }
 }
 
 }  // namespace chrome