[Cronet] Public key pinning for Java API

components/cronet/android/cronet_url_request_context_adapter.cc

Index: components/cronet/android/cronet_url_request_context_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_context_adapter.cc b/components/cronet/android/cronet_url_request_context_adapter.cc
index 37a18cefb5069d6d39141a0d0273914781c85816..6a75eb9de6f4cb366848b431bb2d4761280e7d29 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -9,6 +9,7 @@
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/logging.h"
@@ -312,6 +313,14 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
         http_server_properties_manager.Pass());
   }
 
+// Explicitly disable the persister for Cronet to avoid persistence
+// of dynamic HPKP. This is a safety measure in case if somebody will
+// enable the persistence by specifying transport_security_persister_path
+// in the future.
+#if DCHECK_IS_ON()

[mef, 2015/11/06 18:02:17]

DCHECK_IS_ON() in debug, but not in the release.
Why is it only needed in debug?

[kapishnikov, 2015/11/06 19:45:08]

The intention was to execute it in debug mode only to avoid production code
changes since this is only a safety check for the situation when we enable the
persistence in the future. I assume that the developer who enables the
persistence in Cronet would write unit tests that will fail because of this
DCHECK. On the other hand, if persistence is enabled but there is no enough test
coverage, there is a risk that we may miss a bug during development and discover
it in release only. I have removed DCHECK to avoid situation with undiscovered
bug; however, both arguments have their merits.

+  context_builder.set_transport_security_persister_path(base::FilePath());
+#endif
+
   context_ = context_builder.Build().Pass();
 
   default_load_flags_ = net::LOAD_DO_NOT_SAVE_COOKIES |
@@ -372,6 +381,30 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
     }
   }
 
+  // Iterate through HPKP configuration for every host.
+  for (auto hpkp_itr = config->hpkp_list.begin();
+       hpkp_itr != config->hpkp_list.end(); ++hpkp_itr) {
+    const URLRequestContextConfig::Hpkp& hpkp = **hpkp_itr;
+
+    // Convert the vector of hash strings from the config to
+    // a vector of HashValue objects.
+    net::HashValueVector hash_value_vector;
+    for (const auto& hash : hpkp.pin_hashes) {
+      auto hash_value = net::HashValue(net::HASH_VALUE_SHA256);
+      bool good_hash = hash_value.FromString(*hash);
+      if (good_hash) {
+        hash_value_vector.push_back(hash_value);
+      } else {
+        LOG(WARNING) << "Unable to add hash value " << *hash;
+      }
+    }
+

[mef, 2015/11/06 18:02:17]

maybe add DCHECK here that
context->transport_security_state()->transport_security_persister_ is NULL? (I
don't see an accessor, so it could be easier said than done).

[kapishnikov, 2015/11/06 19:45:08]

Yes, the problem is that there is no accessor. We decided not to introduce one.
We could also check context_builder.set_transport_security_persister_ before
constructing the context.

+    // Add the host pinning.
+    context_->transport_security_state()->AddHPKP(
+        hpkp.host, hpkp.expiration_date, hpkp.include_subdomains,
+        hash_value_vector, GURL());

[mef, 2015/11/06 18:02:17]

GURL() -> GURL::EmptyGURL();

[kapishnikov, 2015/11/06 19:45:09]

Done.

+  }
+
   JNIEnv* env = base::android::AttachCurrentThread();
   jcronet_url_request_context_.Reset(env, jcronet_url_request_context.obj());
   Java_CronetUrlRequestContext_initNetworkThread(