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..28f6f075629b50d93cc27e33759456360e57d7d7 100644
--- a/ios/web/net/cert_verifier_block_adapter.cc
+++ b/ios/web/net/cert_verifier_block_adapter.cc
@@ -28,13 +28,6 @@ struct VerificationContext : public base::RefCounted<VerificationContext> {
   // 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.
@@ -43,14 +36,8 @@ struct VerificationContext : public base::RefCounted<VerificationContext> {
 };
 }
 
-CertVerifierBlockAdapter::CertVerifierBlockAdapter()
-    : CertVerifierBlockAdapter(
-          scoped_ptr<CertVerifier>(CertVerifier::CreateDefault())) {

[Ryan Sleevi, 2015/08/06 03:07:08]

Thanks for removing this; I would have nacked this constructor if I'd seen it
earlier.

Separately, it may help to make sure iOS code isn't calling this.

-}
-
-CertVerifierBlockAdapter::CertVerifierBlockAdapter(
-    scoped_ptr<CertVerifier> cert_verifier)
-    : cert_verifier_(cert_verifier.Pass()) {
+CertVerifierBlockAdapter::CertVerifierBlockAdapter(CertVerifier* cert_verifier)
+    : cert_verifier_(cert_verifier) {
   DCHECK(cert_verifier_);
 }
 
@@ -69,13 +56,17 @@ CertVerifierBlockAdapter::Params::~Params() {
 
 void CertVerifierBlockAdapter::Verify(
     const Params& params,
-    void (^completion_handler)(scoped_ptr<CertVerifyResult>, int)) {
+    void (^completion_handler)(CertVerifyResult, int)) {
   DCHECK(completion_handler);
+  if (!params.cert || !params.hostname.size()) {

[Ryan Sleevi, 2015/08/06 03:07:08]

!params.hostname.empty() for empty checks with STL containers (including
string). While for std::string it's equivalent, following STL container rules,
.empty() will be consistently faster across all storage container types, while
.size() may be O(N) [e.g. in a std::deque/std::list scenario]

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

Done.

+    completion_handler(CertVerifyResult(), ERR_INVALID_ARGUMENT);

[Ryan Sleevi, 2015/08/06 03:07:08]

While David explained how CertVerifier doesn't check for this, it doesn't seem
right that this (as an Adapter for a CertVerifier) should introduce the check.
It feels like it creates inconsistent expectations for developers about the
necessary preconditions - a CertVerifierBlockAdapter no longer adheres to the
same API contract as a CertVerifier.

Which is to say these checks should happen at the caller site when processing
external input, rather than be deferred deep down the stack.

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

Actually CertVerifier DOES check for this:
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr...
It is not documented in the header so I added this check to adapter as well.
CertVerifierBlockAdapter has very different API comparing to CertVerifier. F.e.
CertVerifierBlockAdapter::Verify does not return value, so I think that
different preconditions (though in practice they are not actually different) is
totally fine. Moreover this class has Adapter in its name which suggests the
difference in the interface.
 
Forcing clients to check for input arguments will lead to duplication of error
handling code. The whole point of CertVerifierBlockAdapter is to hide complexity
from WebController and provide simple API with a single completion block, which
has the same failure point.

I assume that there is no difference in handling error cases if iOS did not
provide a hostname vs. network error.  Please let me know if that assumption is
not correct.

+    return;
+  }
 
   scoped_refptr<VerificationContext> context(
       new VerificationContext(params.cert));
   CompletionCallback callback = base::BindBlock(^(int) {
-    completion_handler(context->scoped_result(), 0);
+    completion_handler(context->result, OK);

[Ryan Sleevi, 2015/08/06 03:07:08]

BUG? It seems that you're assuming that any return of the callback will indicate
success, but that's explicitly not the case.

You should be using the result code from the callback to pass on to your block
continuation.

CompletionCallback callback = base::BindBlock(^(int result) {
  completion_handler(context->result, result);
});

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

Done.

   });
   int status = cert_verifier_->Verify(params.cert.get(), params.hostname,

[Ryan Sleevi, 2015/08/06 03:07:08]

This is typically called |result| in //net, rather than status, because it's a
net::Error code

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

I named this |status| to differentiate from |certVerifyResult|:
typedef void (^CompletionHandler)(CertVerifyResult result, int status);

I can use the following naming:
typedef void (^CompletionHandler)(CertVerifyResult certVerifyResult, int
statusResult);

But it will look weird. Do you think it is ok to leave |status| name to avoid
confusion with |certVerifyResult|?

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

Well, you're not naming things statusResult/certVerifyResult - at least, not by
my read of line 59. So you shouldn't have any naming confusion.

In the C++ code, we tend to call this "verify_result", so CertVerifyResult
verifyResult would seem to be acceptable naming for the Objective C++ side.

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

Sorry for confusion, this file is .cc and a subject to C++ Style Guide in terms
of naming. I did no rename variables in the previous round, because wanted to
ask if you ok with olds names (result/status), but I guess you are not. Done.

                                       params.ocsp_response, params.flags,
@@ -89,7 +80,7 @@ void CertVerifierBlockAdapter::Verify(
   }
 
   // Verification has either failed or result was retrieved from the cache.
-  completion_handler(status ? nullptr : context->scoped_result(), status);
+  completion_handler(context->result, status);

[Ryan Sleevi, 2015/08/06 03:07:08]

BUG? Nothing in here seems like it actually keeps |context| alive (that is,
takes a reference). Certainly, lines 71-74 don't, nor would lines 69/83, AFAICT,
so how does this not cause use-after-frees?

Does the Block_copy of BindBlock actually bind |context| as well, thus
presumably creating a reference (although the interaction of Obj-C blocks and
C++ ctor semantics seem weird) or does it just bind to |context->result|?

If it binds to |context|, then you've seemingly made this impossible to cancel,
which further doesn't seem right. That's because |callback| will keep |context|
alive (line 68/69), and |context| will keep the overall verification alive
through context->request (line 74), and so you end up uncancelable.

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

|context| is kept alive by the block (blocks retain Objective-C objects and copy
C++ objects), block itself is kept alive by CompletionCallback.

So yes, requests were uncancelable. I added a pool of requests, so they will be
cancelled once CertVerifierAdapter is destroyed.

 }
 
 }  // net