WKWebView: Added cert verification API to web controller.

ios/web/net/crw_cert_verification_controller.mm

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/web/net/crw_cert_verification_controller.h"
+
+#include "base/mac/bind_objc_block.h"
+#import "base/memory/scoped_ptr.h"
+#include "base/strings/sys_string_conversions.h"
+#include "ios/web/net/cert_verifier_block_adapter.h"
+#include "ios/web/public/browser_state.h"
+#include "ios/web/public/web_thread.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/ssl/ssl_config_service.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+
+@interface CRWCertVerificationController () {
+  // Cert verification object which wraps |net::CertVerifier|. Must be created,
+  // used and destroyed on IO Thread.
+  scoped_ptr<web::CertVerifierBlockAdapter> _certVerifier;
+
+  // URLRequestContextGetter for obtaining net layer objects.
+  net::URLRequestContextGetter* _contextGetter;
+}
+
+// Cert verification flags. Must be used on IO Thread.
+@property(nonatomic, readonly) int certVerifyFlags;
+
+// Creates _certVerifier object on IO thread.
+- (void)createCertVerifier;
+
+// Verifies the given |cert| for the given |host| and calls |completionHandler|
+// on completion. |completionHandler| cannot be null and will be called
+// synchronously or asynchronously on IO thread.
+- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler;
+
+@end
+
+@implementation CRWCertVerificationController
+
+#pragma mark - Superclass
+
+- (void)dealloc {
+  DCHECK(!_certVerifier);  // This is not a thread safe check.
+  [super dealloc];
+}
+
+#pragma mark - Public
+
+- (instancetype)initWithBrowserState:(web::BrowserState*)browserState {
+  DCHECK(browserState);
+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+  self = [super init];
+  if (self) {
+    _contextGetter = browserState->GetRequestContext();
+    DCHECK(_contextGetter);
+    [self createCertVerifier];
+  }
+  return self;
+}
+
+- (void)decidePolicyForCert:(const scoped_refptr<net::X509Certificate>&)cert
+                       host:(NSString*)host
+          completionHandler:(void (^)(web::CertAcceptPolicy))handler {
+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+  [self verifyCert:cert
+                forHost:host
+      completionHandler:^(net::CertVerifyResult result, int error) {
+        web::CertAcceptPolicy policy = web::CERT_ACCEPT_POLICY_UNKNOWN;
+        if (error == net::OK) {
+          policy = web::CERT_ACCEPT_POLICY_ALLOW;
+        } else if (net::IsCertStatusError(result.cert_status)) {

[Eugene But (OOO till 7-30), 2015/08/25 18:05:54]

David, could you please confirm that the logic is correct here.

[davidben, 2015/08/26 20:19:09]

I would maybe UNKNOWN -> ERROR to make it clear this is an error state (i.e. the
verifier hit some kind of fatal error internally), but otherwise I think this is
correct. This is Ryan's code though, so he'd know more canonically.

[Eugene But (OOO till 7-30), 2015/08/26 21:12:48]

Done.

[Ryan Sleevi, 2015/09/03 21:56:04]

Right, I don't think it's correct :) error can be any valid net::error :)

if (error == net::OK) {
  allow
} else if (net::IsCertStatusError() && net::IsCertStatusMinorError()) {
  decide whether you want to allow or reject these, based on the sort of UI
surface you can/want to expose.
} else {
  reject
}

[Eugene But (OOO till 7-30), 2015/09/03 22:26:37]

Hm... not sure if I understand this...

If cert cert has expired then net::IsCertStatusError() returns true, but
IsCertStatusMinorError() returns false,
in this case we want to show SSL interstitial and ask user's decision.

If both net::IsCertStatusError() and IsCertStatusMinorError() return true, I
think we want to allow load, w/o showing interstitial.

If error != net::OK and net::IsCertStatusError() returns false then CertVerifier
has actually failed to verify given cert and we don't even want to show
interstitial, but show net error.

Please let me know if any of these statements are incorrect.

+          // TODO(eugenebut): Check CertPolicyCache for user's decision.
+          policy = net::IsCertStatusMinorError(result.cert_status)
+                       ? web::CERT_ACCEPT_POLICY_ALLOW
+                       : web::CERT_ACCEPT_POLICY_DENY;
+        }
+
+        dispatch_async(dispatch_get_main_queue(), ^{
+          handler(policy);

[davidben, 2015/08/26 20:19:09]

Hrm. I think we *still* have the threading problem. :'-(

So, handler internally has a reference to the outer completionHandler, right?
This IO thread outer block must have a reference to handler so it may pass it
off to the main thread inner block. Which means we again can't be sure the IO
thread outer block returns before the main thread inner block runs, so the last
reference to handler may go away on the IO thread and so far down the line...

Although, given how absurdly viral this problem is when blocks get involved, I
wonder if it actually doesn't matter where objects get deleted on or something.
This basically means that absolutely everything which might end up on another
thread bleeds this into all other objects it touches. Does this mean iOS apps
just literally never use threads, or is it secretly expected that every object
has thread-safe destruction. (I'm not familiar with Obj-C conventions.)

I guess completionHandler does avoid taking a reference to self. Does it matter
that WKWebView might internally make it take references to who knows what?


If this constraint is important, I think what you want is something like... you
instead of passing the completionHandlers to the IO thread, you keep them in a
UI thread list or so and then pass a WeakPtr over to the IO thread. Then in
shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending
callback?). But that smells extremely non-idiomatic. I feel like there must be a
better way, but I'm probably not qualified to find the right Obj-C way.

[Eugene But (OOO till 7-30), 2015/08/26 21:12:48]

You are right, this very block (Line 83) retains, |handler| on IO thread
and |handler| indirectly retains WebController on UI thread.

However this block will release |handler| on the UI thread, because the dispatch
is performed on UI thread.
(I also checked that in practice). Please note that outer block (Line 72) does
not retain |handler|.

Answering your questions:
UIKit classes are not generally thread-safe and I would avoid destructing them
on background threads.
None of the blocks inside this method retain self.
WKWebView may potentially retain some references (like WebController), however I
will not lead to retain cycle, because WKWebView released in WebController's
close (not dealloc).


In any case this code should also wait for Stuart's review when he's back from
vacation (next Tuesday).

[davidben, 2015/08/26 21:20:46]

The outer block must retain |handler| so that it may pass it into the inner
block when that gets created, no? Perhaps Obj-C is clever enough to do the
equivalent of .Pass(), but otherwise both inner and outer would have a reference
I would think.

[Eugene But (OOO till 7-30), 2015/08/26 21:56:29]

Oh, right, I reproduced the case you described and handler was indeed
deallocated on IO thread.

So:
1.) |handler| can be released on IO thread
2.) handler itself does not retain web controller (and will not), but it retains
WKWebView's completion handler which may be a problem

I will take a look what can be done here.

[Eugene But (OOO till 7-30), 2015/08/27 15:57:33]

Alright, |handler| is not retained by the blocks anymore and its lifetime is now
managed by| BlockHolder|.

+        });
+      }];
+}
+
+- (void)shutDown {
+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+  web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
+    // This block captures |self| delaying its deallocation and possible causing
+    // dealloc to happen on IO thread (which is fine for this class).
+    _certVerifier.reset();
+  }));
+}
+
+#pragma mark - Private
+
+- (int)certVerifyFlags {
+  DCHECK(web::WebThread::CurrentlyOn(web::WebThread::IO));
+  DCHECK(_contextGetter);
+  // |net::URLRequestContextGetter| lifetime is expected to be at least the same
+  // or longer than |BrowserState| lifetime.
+  net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
+  DCHECK(context);
+  net::SSLConfigService* SSLConfigService = context->ssl_config_service();
+  DCHECK(SSLConfigService);
+  net::SSLConfig config;
+  SSLConfigService->GetSSLConfig(&config);
+  return config.GetCertVerifyFlags();
+}
+
+- (void)createCertVerifier {
+  web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
+    net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
+    _certVerifier.reset(new web::CertVerifierBlockAdapter(
+        context->cert_verifier(), context->net_log()));
+  }));
+}
+
+- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler {
+  DCHECK(completionHandler);
+  __block scoped_refptr<net::X509Certificate> blockCert = cert;
+  web::WebThread::PostTask(
+      web::WebThread::IO, FROM_HERE, base::BindBlock(^{
+        // WeakNSObject does not work across different threads, hence this block
+        // retains self.
+        if (!_certVerifier) {
+          completionHandler(net::CertVerifyResult(), net::ERR_FAILED);
+          return;
+        }
+
+        web::CertVerifierBlockAdapter::Params params(
+            blockCert.Pass(), base::SysNSStringToUTF8(host));
+        params.flags = self.certVerifyFlags;
+        params.crl_set = net::SSLConfigService::GetCRLSet();
+        // OCSP response is not provided by iOS API.
+        _certVerifier->Verify(params, completionHandler);
+      }));
+}
+
+@end