Add client cert support to android_webview

android_webview/native/aw_contents_client_bridge.cc

Index: android_webview/native/aw_contents_client_bridge.cc
diff --git a/android_webview/native/aw_contents_client_bridge.cc b/android_webview/native/aw_contents_client_bridge.cc
index 348505981eb1c9df73e8ffca162acf18c9e65beb..6ebf9c9e012976972d4d6dd8682eb962905162a1 100644
--- a/android_webview/native/aw_contents_client_bridge.cc
+++ b/android_webview/native/aw_contents_client_bridge.cc
@@ -8,10 +8,14 @@
 #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;
@@ -24,6 +28,21 @@ using content::BrowserThread;
 
 namespace android_webview {
 
+typedef net::OpenSSLClientKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
+
+namespace {
+// 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());
+}
+
+}  // namespace
+
 AwContentsClientBridge::AwContentsClientBridge(JNIEnv* env, jobject obj)
     : java_ref_(env, obj) {
   DCHECK(obj);
@@ -49,7 +68,7 @@ void AwContentsClientBridge::AllowCertificateError(
     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);
@@ -73,12 +92,12 @@ void AwContentsClientBridge::AllowCertificateError(
   // 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";
@@ -88,6 +107,149 @@ void AwContentsClientBridge::ProceedSslError(JNIEnv* env, jobject obj,
   pending_cert_error_callbacks_.Remove(id);
 }
 
+// This method is inspired by SelectClientCertificate() in
+// chrome/browser/ui/android/ssl_client_certificate_request.cc
+void AwContentsClientBridge::SelectClientCertificate(
+      net::SSLCertRequestInfo* cert_request_info,
+      const SelectCertificateCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Add the callback to id map.
+  int request_id = pending_client_cert_request_callbacks_.Add(
+      new SelectCertificateCallback(callback));
+  // Make sure callback is run on error.
+  base::ScopedClosureRunner guard(base::Bind(
+      &AwContentsClientBridge::HandleErrorInClientCertificateResponse,
+      base::Unretained(this),
+      request_id));
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (obj.is_null())
+    return;
+
+  // 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.
+        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;
+  }
+
+  // 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());
+
+  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());
+
+  // Release the guard.
+  ignore_result(guard.Release());
+}
+
+// This method is inspired by OnSystemRequestCompletion() in
+// chrome/browser/ui/android/ssl_client_certificate_request.cc
+void AwContentsClientBridge::ProvideClientCertificateResponse(
+    JNIEnv* env,
+    jobject obj,
+    int request_id,
+    jobjectArray encoded_chain_ref,
+    jobject private_key_ref) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  SelectCertificateCallback* callback =
+      pending_client_cert_request_callbacks_.Lookup(request_id);
+  if (!callback || callback->is_null()) {

[boliu, 2014/04/21 16:56:45]

The only way I can think of this happening legitimately is if the client calls
the callback twice. Anything else?

Can we check for the called more than once behavior (maybe throw an exception in
java), then make this DCHECK?

[sgurun-gerrit only, 2014/04/21 23:53:29]

I think this is really safe and defensive programming. I don't see the value of
changing it to a DCHECK.
Having java side check for double calls is a good idea. Probably needs a test
too. 


On 2014/04/21 16:56:45, boliu wrote:

[boliu, 2014/04/22 00:20:55]

What if in a future change, we accidentally introduce this bug? It could go
unnoticed for a long time due to this.

You added isCalled on java side, which means we completely control how this
method is called, so we should DCHECK the conditions we expect. Defensive
programming only applies when dealing with code/data outside of our control imo.

On 2014/04/21 23:53:29, sgurun wrote:

[sgurun-gerrit only, 2014/04/22 00:44:36]

the best way to identify bugs is via tests, which I will add for this case in a
subsequent CL. The important behavior is to maintain is that we do throw
exceptions in subsequent  calls. most likely a CTS test will be proper there.

you want to be defensive not only against misuse of external APIs but also
against internal APIs. 
On 2014/04/22 00:20:55, boliu wrote:

[boliu, 2014/04/22 00:47:57]

Disagree. If you want to keep an invariant in code you control, why can't you
assert that invariant? What are you defending against here?

[sgurun-gerrit only, 2014/04/22 01:12:27]

Assertions and exceptions are very useful in debugging and correcting a problem.
Your suggestion of throwing an exception was good, because it provides some
information to embedder of webview some information that they can act on. 

In this case, however I don't see any value except making the code more brittle.
The internal APIs always change, the tests get outdated, and zillions of things
happen at once and this is a piece of code that is not in the common use case
for most. Why do you want to crash loudly in the field if we have a bug there.
Ignoring the duplicate code has no side effect and is the right behavior.

We made a similar mistake before, leaving CHECKs in the code and introducing
unnecessary crashes in autofill. it was pointless.


On 2014/04/22 00:47:57, boliu wrote:

[boliu, 2014/04/22 01:32:17]

How do tests apply to this discussion? You can test it regardless.
It's a bug in *our code* if ever fall into this path. I want it to crash loudly
so it's unlikely to have that bug slip out.
It's not the right behavior if we forget to check isCalled on a method in the
future.
It was not pointless. Even if caught in production, (which I agree is costly,)
it was caught. Without the CHECK, you would never know.

The right solution is to have enough test coverage to never let these things
slip out into production, not being lax about invariants in the code.

[sgurun-gerrit only, 2014/04/22 02:06:47]

Of course you can. The point is test coverage is not perfect (or when covers
even it can get outdated), and bugs in such rarely used code paths generally not
get caught until somebody tries it in the field.
This is exactly the problem I am trying to communicate. Why do you want to crash
when you have a good solution that does not crash?  Crashing loudly is always a
bad experience for users.
Of course it was pointless. This is why we removed such CHECKs afterwards.
You want to crash loudly but if you start crashing in every little bug that you
can handle via other types of defenses, then you are annoying your users.

In any case, this started taking way too much of time. I don't agree with the
change but I will do it tomorrow regardless.

[sgurun-gerrit only, 2014/04/22 17:41:08]

Done.

[digit1, 2014/04/22 18:11:13]

I think Bo's point is that this should be a DCHECK() so this can be detected
during testing, only a CHECK() will provide horrible experience to users :-) I
agree with him, better catch when the assertion fails as soon as possible.

Sometimes, hiding the bug with a soft-fallback just makes things more difficult
to debug.

+    LOG(WARNING) << "Ignoring unexpected client certificate response callback";
+    return;
+  }
+  // Make sure callback is run on error.
+  base::ScopedClosureRunner guard(base::Bind(
+      &AwContentsClientBridge::HandleErrorInClientCertificateResponse,
+      base::Unretained(this),
+      request_id));
+  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;
+  }
+
+  // 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));
+  pending_client_cert_request_callbacks_.Remove(request_id);
+
+  // Release the guard.
+  ignore_result(guard.Release());
+}
+
 void AwContentsClientBridge::RunJavaScriptDialog(
     content::JavaScriptMessageType message_type,
     const GURL& origin_url,
@@ -206,6 +368,15 @@ void AwContentsClientBridge::CancelJsResult(JNIEnv*, jobject, int id) {
   pending_js_dialog_callbacks_.Remove(id);
 }
 
+// Use to cleanup if there is an error in client certificate response.
+void AwContentsClientBridge::HandleErrorInClientCertificateResponse(
+    int request_id) {
+  SelectCertificateCallback* callback =
+      pending_client_cert_request_callbacks_.Lookup(request_id);
+  callback->Run(scoped_refptr<net::X509Certificate>());
+  pending_client_cert_request_callbacks_.Remove(request_id);
+}
+
 bool RegisterAwContentsClientBridge(JNIEnv* env) {
   return RegisterNativesImpl(env) >= 0;
 }