Add client cert support to android_webview

android_webview/native/aw_contents_client_bridge.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 "android_webview/native/aw_contents_client_bridge.h"
 
 #include "android_webview/common/devtools_instrumentation.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
-#include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/AwContentsClientBridge_jni.h"
+#include "net/android/keystore_openssl.h"
 #include "net/cert/x509_certificate.h"
+#include "net/ssl/openssl_client_key_store.h"
+#include "net/ssl/ssl_cert_request_info.h"
+#include "net/ssl/ssl_client_cert_type.h"
 #include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ConvertUTF16ToJavaString;
 using base::android::JavaRef;
 using base::android::ScopedJavaLocalRef;
 using content::BrowserThread;
 
 namespace android_webview {
 
+typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;

[boliu, 2014/04/14 23:56:04]

using?

[sgurun-gerrit only, 2014/04/15 23:40:03]

nop, this is a class member. won't work.

+
+// Must be called on the I/O thread to record a client certificate
+// and its private key in the OpenSSLClientKeyStore.
+void RecordClientCertificateKey(
+    const scoped_refptr<net::X509Certificate>& client_cert,
+    ScopedEVP_PKEY private_key) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  net::OpenSSLClientKeyStore::GetInstance()->RecordClientCertPrivateKey(
+      client_cert.get(), private_key.get());
+}
+
 AwContentsClientBridge::AwContentsClientBridge(JNIEnv* env, jobject obj)
     : java_ref_(env, obj) {
   DCHECK(obj);
   Java_AwContentsClientBridge_setNativeContentsClientBridge(
       env, obj, reinterpret_cast<intptr_t>(this));
 }
 
 AwContentsClientBridge::~AwContentsClientBridge() {
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
   if (obj.is_null())
     return;
   // Clear the weak reference from the java peer to the native object since
   // it is possible that java object lifetime can exceed the AwContens.
   Java_AwContentsClientBridge_setNativeContentsClientBridge(env, obj.obj(), 0);
 }
 
 void AwContentsClientBridge::AllowCertificateError(
     int cert_error,
     net::X509Certificate* cert,
     const GURL& request_url,
     const base::Callback<void(bool)>& callback,
     bool* cancel_request) {
 
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
   if (obj.is_null())
     return;
 
   std::string der_string;
   net::X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_string);
   ScopedJavaLocalRef<jbyteArray> jcert = base::android::ToJavaByteArray(
       env,
       reinterpret_cast<const uint8*>(der_string.data()),
       der_string.length());
   ScopedJavaLocalRef<jstring> jurl(ConvertUTF8ToJavaString(
       env, request_url.spec()));
   // We need to add the callback before making the call to java side,
   // as it may do a synchronous callback prior to returning.
   int request_id = pending_cert_error_callbacks_.Add(
       new CertErrorCallback(callback));
   *cancel_request = !Java_AwContentsClientBridge_allowCertificateError(
       env, obj.obj(), cert_error, jcert.obj(), jurl.obj(), request_id);
   // if the request is cancelled, then cancel the stored callback
   if (*cancel_request) {
     pending_cert_error_callbacks_.Remove(request_id);
- }
+  }
 }
 
 void AwContentsClientBridge::ProceedSslError(JNIEnv* env, jobject obj,
                                              jboolean proceed, jint id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   CertErrorCallback* callback = pending_cert_error_callbacks_.Lookup(id);
   if (!callback || callback->is_null()) {
     LOG(WARNING) << "Ignoring unexpected ssl error proceed callback";
     return;
   }
   callback->Run(proceed);
   pending_cert_error_callbacks_.Remove(id);
 }
 
+// This method is almost same as SelectClientCertificate() in
+// chrome/browser/ui/android/ssl_client_certificate_request.cc

[boliu, 2014/04/14 23:56:04]

File a bug to componentize this? Do we have time to do this correctly this time?

[sgurun-gerrit only, 2014/04/15 23:40:03]

opened an internal bug to track this. I think this makes sense but not sure
where it should go now.
On 2014/04/14 23:56:04, boliu wrote:

[boliu, 2014/04/16 02:57:46]

General track record is we never go back and clean stuff up. And this chrome
code looks pretty bad already and doesn't entirely fit webview's case. Maybe for
now we'll just fork and fix all the crap in webview?

On 2014/04/15 23:40:03, sgurun wrote:

+void AwContentsClientBridge::SelectClientCertificate(
+      net::SSLCertRequestInfo* cert_request_info,
+      const SelectCertificateCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Ensure that callback(NULL) is posted as a task on the UI thread
+  // in case of an error.
+  base::Closure post_task_closure = base::Bind(
+      base::IgnoreResult(&content::BrowserThread::PostTask),

[boliu, 2014/04/14 23:56:04]

Do we really need to do the post? Can't we just call it back synchronously on
failure?

[sgurun-gerrit only, 2014/04/15 23:40:03]

Taken verbatim from chrome. posting definitely does not hurt, however, it seems
to my eyes that calling sync would also be ok. In any case I don't want to
diverge from chrome here so I can easily combine the code later. Will put a todo
to do it later while merging code.

On 2014/04/14 23:56:04, boliu wrote:

+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(callback, scoped_refptr<net::X509Certificate>()));
+
+  base::ScopedClosureRunner guard(post_task_closure);
+
+  // Build the |key_types| JNI parameter, as a String[]
+  std::vector<std::string> key_types;
+  for (size_t n = 0; n < cert_request_info->cert_key_types.size(); ++n) {
+    switch (cert_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.

[boliu, 2014/04/14 23:56:04]

This is ok because...? Should this be NOTREACHED?

[sgurun-gerrit only, 2014/04/15 23:40:03]

We ignore the other types. This is almost advisory and user does not have to
choose the provided type and things can even still work. this is fine.
On 2014/04/14 23:56:04, boliu wrote:

+        break;
+    }
+  }
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  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;
+  }
+
+  // Build the |encoded_principals| JNI parameter, as a byte[][]
+  ScopedJavaLocalRef<jobjectArray> principals_ref =
+      base::android::ToJavaArrayOfByteArray(
+          env, cert_request_info->cert_authorities);
+  if (principals_ref.is_null()) {
+    LOG(ERROR) << "Could not create principals array (byte[][])";
+    return;
+  }
+
+  // Build the |host_name| and |port| JNI parameters, as a String and
+  // a jint.
+  ScopedJavaLocalRef<jstring> host_name_ref =
+      base::android::ConvertUTF8ToJavaString(
+          env, cert_request_info->host_and_port.host());
+
+  // Create a copy of the callback on the heap so that its address
+  // and ownership can be passed through and returned from Java via JNI.
+  scoped_ptr<SelectCertificateCallback> request(
+      new SelectCertificateCallback(callback));
+
+  jlong request_id = reinterpret_cast<intptr_t>(request.get());
+
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (obj.is_null())

[boliu, 2014/04/14 23:56:04]

Feels like this null check can be moved a lot higher.

[sgurun-gerrit only, 2014/04/15 23:40:03]

Done.

+    return;
+
+  if (!Java_AwContentsClientBridge_selectClientCertificate(
+          env,
+          obj.obj(),
+          request_id,
+          key_types_ref.obj(),
+          principals_ref.obj(),
+          host_name_ref.obj(),
+          cert_request_info->host_and_port.port())) {
+    return;
+  }
+
+  ignore_result(guard.Release());
+
+  // Ownership was transferred to Java.
+  SelectCertificateCallback* ALLOW_UNUSED dummy = request.release();

[boliu, 2014/04/14 23:56:04]

This transfer of ownership from native to java (which the app might leak) is
super unsafe. Why not keep the native object in a map, give the key of the map
to java, then there is no chance of leaks?

[sgurun-gerrit only, 2014/04/15 23:40:03]

As we discussed I don't think it is super unsafe. The only leak condition that I
am aware of is when a programmer makes a pretty grave mistake of overwriting
this method without not calling one of the methods. There is nothing we can do
then, and the careless soul will have bigger issues to worry about.
Even in a map, if the user forgets calling, the method will be kept in the map
indefinitely. so not much different.

Are there any other concrete cases that you know that can cause a leak?

On 2014/04/14 23:56:04, boliu wrote:

[boliu, 2014/04/16 02:57:46]

If app holds onto the callback object, then it's an indication it's still not
made the decision. If app takes forever to make the decision, then too bad and
there's nothing we can do.

But if app just drops the callback object and it gets garbage collected, we can
just let the default behavior happen (or raise an exception)
If you put it in a map, the native object will be deleted when the webview is
destroyed.

+}
+
+// This method is almost same as OnSystemRequestCompletion() in
+// chrome/browser/ui/android/ssl_client_certificate_request.cc
+void AwContentsClientBridge::ProvideClientCertificateResponse(
+    JNIEnv* env,
+    jobject obj,
+    jlong request_id,
+    jobjectArray encoded_chain_ref,
+    jobject private_key_ref) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Take back ownership of the request object.
+  scoped_ptr<SelectCertificateCallback> callback(
+      reinterpret_cast<SelectCertificateCallback*>(request_id));
+
+  // Ensure that callback(NULL) is called in case of an error.
+  base::Closure null_closure =
+      base::Bind(*callback, scoped_refptr<net::X509Certificate>());
+
+  base::ScopedClosureRunner guard(null_closure);
+
+  if (encoded_chain_ref == NULL || private_key_ref == NULL) {
+    LOG(ERROR) << "Client certificate request cancelled";
+    return;
+  }
+  // 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> 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.
+  ScopedEVP_PKEY private_key(
+      net::android::GetOpenSSLPrivateKeyWrapper(private_key_ref));
+  if (!private_key.get()) {
+    LOG(ERROR) << "Could not create OpenSSL wrapper for private key";
+    return;
+  }
+
+  ignore_result(guard.Release());
+
+  // RecordClientCertificateKey() must be called on the I/O thread,
+  // before the callback is called with the selected certificate on
+  // the UI thread.
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&RecordClientCertificateKey,
+                 client_cert,
+                 base::Passed(&private_key)),
+      base::Bind(*callback, client_cert));
+}
+
 void AwContentsClientBridge::RunJavaScriptDialog(
     content::JavaScriptMessageType message_type,
     const GURL& origin_url,
     const base::string16& message_text,
     const base::string16& default_prompt_text,
     const content::JavaScriptDialogManager::DialogClosedCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
@@ skipping 103 matching lines @@
   }
   callback->Run(false, base::string16());
   pending_js_dialog_callbacks_.Remove(id);
 }
 
 bool RegisterAwContentsClientBridge(JNIEnv* env) {
   return RegisterNativesImpl(env) >= 0;
 }
 
 }  // namespace android_webview