WKWebView: Added cert verification API to web controller.

ios/web/net/cert_verifier_block_adapter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.

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

Doesn't the use of blocks in this file intrinsically make it objective-c++, and
thus should follow .mm naming schemes?

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

Clang and GCC implement blocks for C/C++ as a language extension. So there is no
need to make this file mm (unless we want to compile it with other compilers).

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ios/web/net/cert_verifier_block_adapter.h"
 
 #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"
 
 namespace net {
 
 namespace {
 
 // Resource manager which keeps CertVerifier::Request, CertVerifyResult and
 // X509Certificate alive until verification is completed.
 struct VerificationContext : public base::RefCounted<VerificationContext> {

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

With blocks, isn't there the possibility of arbitrary thread dispatch? Does this
need to be RefCountedThreadSafe?

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

Good catch. Done.

   VerificationContext(scoped_refptr<net::X509Certificate> cert) : cert(cert) {

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

Explicit

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

Done.

     result.cert_status = CERT_STATUS_INVALID;

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

Why is this?

This feels a very ad-hoc way of creating a CertVerifyResult, and runs counter to
the default ctor of a CertVerifyResult as used in all the other code.

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

Removed.

   }
   // Verification request. Must be alive until verification is completed,
   // otherwise it will be cancelled.
   scoped_ptr<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>;
   ~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_);
 }
 
 CertVerifierBlockAdapter::~CertVerifierBlockAdapter() {
 }
 
 CertVerifierBlockAdapter::Params::Params(scoped_refptr<X509Certificate> cert,
                                          const std::string& hostname)
     : cert(cert),
       hostname(hostname),
       flags(static_cast<CertVerifier::VerifyFlags>(0)) {
 }
 
 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,
                                       params.crl_set.get(), &(context->result),
                                       callback, &(context->request), net_log_);
 
   if (status == ERR_IO_PENDING) {
     // 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);

[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