WKWebView: Added cert verification API to web controller.

ios/web/net/cert_verifier_block_adapter.cc

Index: ios/web/net/cert_verifier_block_adapter.cc
diff --git a/ios/web/net/cert_verifier_block_adapter.cc b/ios/web/net/cert_verifier_block_adapter.cc
index 31901000e9552f2f347735e6937008c50289b3e9..3d701b3c2a60101dcbe2c8aadc3803abfc5e41ae 100644
--- a/ios/web/net/cert_verifier_block_adapter.cc
+++ b/ios/web/net/cert_verifier_block_adapter.cc
@@ -6,7 +6,6 @@
 
 #include "base/mac/bind_objc_block.h"
 #include "net/base/net_errors.h"
-#include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
 #include "net/cert/x509_certificate.h"
 
@@ -14,43 +13,32 @@ namespace net {
 
 namespace {
 
-// Resource manager which keeps CertVerifier::Request, CertVerifyResult and
-// X509Certificate alive until verification is completed.
-struct VerificationContext : public base::RefCounted<VerificationContext> {
-  VerificationContext(scoped_refptr<net::X509Certificate> cert) : cert(cert) {
-    result.cert_status = CERT_STATUS_INVALID;
-  }
-  // Verification request. Must be alive until verification is completed,
-  // otherwise it will be cancelled.
-  scoped_ptr<CertVerifier::Request> request;
+// Resource manager which keeps CertVerifyResult and X509Certificate alive until
+// verification is completed. Also holds unowned pointer to
+// |CertVerifier::Request|.
+struct VerificationContext
+    : public base::RefCountedThreadSafe<VerificationContext> {
+  explicit VerificationContext(scoped_refptr<net::X509Certificate> cert)
+      : request(nullptr), cert(cert) {}
+  // Unowned verification request.
+  CertVerifier::Request* request;
   // The result of certificate verification.
   CertVerifyResult result;
   // Certificate being verificated.
   scoped_refptr<net::X509Certificate> cert;
 
-  // Copies CertVerifyResult and wraps it into a scoped_ptr.
-  scoped_ptr<CertVerifyResult> scoped_result() {
-    scoped_ptr<CertVerifyResult> scoped_result(new CertVerifyResult());
-    scoped_result->CopyFrom(result);
-    return scoped_result.Pass();
-  }
-
  private:
   VerificationContext() = delete;
   // Required by base::RefCounted.
-  friend class base::RefCounted<VerificationContext>;
+  friend class base::RefCountedThreadSafe<VerificationContext>;
   ~VerificationContext() {}
 };
 }
 
-CertVerifierBlockAdapter::CertVerifierBlockAdapter()
-    : CertVerifierBlockAdapter(
-          scoped_ptr<CertVerifier>(CertVerifier::CreateDefault())) {
-}
-
-CertVerifierBlockAdapter::CertVerifierBlockAdapter(
-    scoped_ptr<CertVerifier> cert_verifier)
-    : cert_verifier_(cert_verifier.Pass()) {
+CertVerifierBlockAdapter::CertVerifierBlockAdapter(CertVerifier* cert_verifier,
+                                                   NetLog* net_log)
+    : cert_verifier_(cert_verifier),
+      net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_URL_REQUEST)) {

[Eugene But (OOO till 7-30), 2015/08/07 02:27:20]

Not sure if SOURCE_URL_REQUEST is ok here, or I should create a separate
SOURCE_CERT_VERIFICATION

[Ryan Sleevi, 2015/08/07 21:52:12]

I don't understand why you're creating it, versus taking it from the context of
the caller.

Is this because it's not compatible with the Block adapter? Then we should fix
that, because you really want to use the BoundNetLog that is the callers, since
that will contextually be associated with the right URLRequest flow.

Of course, for WKWebView, not using //net, we have to decide how we want NetLog
to behave. I don't know what's been done by the iOS team in this respect, but
having a NetLog with any information you can get from the WKWebView seems
_extremely_ valuable for diagnostics and debugging in the field.

When using Chrome's network stack, the NetLog comes from the Socket (
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...
/
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...
), which is setup here -
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc...

Although I'll readily admit, I'm not sure the full implications of how this
should look for iOS, so I'm gonna tag in mmenke@ for this.

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Caller (Web Controller) does not have BoundNetLog, but it has NetLog, so I
passed |NetLog| here.

There is no value in sharing BoundNetLog between multiple WebViews, because
MultiThreaderCertVerifier will recreate BoundNet log anyway:
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr...

Per discussion with mmenke@ I will create one BoundNetLog per
CertVerifier::Verify invocation, rather than share them. BoundNetLog just groups
related events together, and we don't want events from a bunch of different
verify invocations all to be lumped together without being sorted. 

   DCHECK(cert_verifier_);
 }
 
@@ -59,37 +47,46 @@ CertVerifierBlockAdapter::~CertVerifierBlockAdapter() {
 
 CertVerifierBlockAdapter::Params::Params(scoped_refptr<X509Certificate> cert,
                                          const std::string& hostname)
-    : cert(cert),
-      hostname(hostname),
-      flags(static_cast<CertVerifier::VerifyFlags>(0)) {
-}
+    : cert(cert), hostname(hostname), flags(0) {}
 
 CertVerifierBlockAdapter::Params::~Params() {
 }
 
 void CertVerifierBlockAdapter::Verify(

[Ryan Sleevi, 2015/08/07 21:52:12]

To confirm: This method will always and only be called on the IO thread,
correct?

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Technically it *can* be called on any thread, as long as if its the same thread
where CertVerifier was created.
In unit tests it is called on main thread, but in web controller it will
*always* be called on IO thread.

     const Params& params,
-    void (^completion_handler)(scoped_ptr<CertVerifyResult>, int)) {
+    void (^completion_handler)(CertVerifyResult, int)) {
   DCHECK(completion_handler);
+  if (!params.cert || params.hostname.empty()) {
+    completion_handler(CertVerifyResult(), ERR_INVALID_ARGUMENT);
+    return;
+  }
 
   scoped_refptr<VerificationContext> context(
       new VerificationContext(params.cert));
-  CompletionCallback callback = base::BindBlock(^(int) {
-    completion_handler(context->scoped_result(), 0);
+  CompletionCallback callback = base::BindBlock(^(int status) {
+    completion_handler(context->result, status);

[Ryan Sleevi, 2015/08/07 21:52:12]

BUG?

In //net, callbacks are always the terminal state for a return; this allows
callbacks to delete the relevant objects, if appropriate, for example.

The implications of this is that invoking a callback should always be the final
state of the function. I believe you want to delete the pending request _prior_
to invoking the completion_handler.

Note: It's perfectly OK to delete context->request in the CompletionCallback of
Verify(), as CertVerifier::Verify() adheres to this contract (that is, that
CompletionCallback is the last thing called and all state is cleaned up)

[Eugene But (OOO till 7-30), 2015/08/12 22:00:38]

Done.

+    // Remove pending request.
+    auto request_iterator = std::find(
+        pending_requests_.begin(), pending_requests_.end(), context->request);
+    DCHECK(pending_requests_.end() != request_iterator);
+    pending_requests_.erase(request_iterator);
   });
-  int status = cert_verifier_->Verify(params.cert.get(), params.hostname,
-                                      params.ocsp_response, params.flags,
-                                      params.crl_set.get(), &(context->result),
-                                      callback, &(context->request), net_log_);
-
+  scoped_ptr<CertVerifier::Request> request(new CertVerifier::Request);
+  int status = cert_verifier_->Verify(
+      params.cert.get(), params.hostname, params.ocsp_response, params.flags,
+      params.crl_set.get(), &(context->result), callback, &request, net_log_);
   if (status == ERR_IO_PENDING) {
+    // Make sure that |CertVerifier::Request| is alive until either verification
+    // is completed or CertVerifierBlockAdapter is destroyed.
+    pending_requests_.push_back(request.Pass());
+    context->request = pending_requests_.back();
     // Completion handler will be called from |callback| when verification
     // request is completed.
     return;
   }
 
   // Verification has either failed or result was retrieved from the cache.
-  completion_handler(status ? nullptr : context->scoped_result(), status);
+  completion_handler(context->result, status);
 }
 
 }  // net