WKWebView(iOS9): correctly update SSL status for current navigation item

ios/web/net/crw_cert_verification_controller.mm

Index: ios/web/net/crw_cert_verification_controller.mm
diff --git a/ios/web/net/crw_cert_verification_controller.mm b/ios/web/net/crw_cert_verification_controller.mm
index a0180d0f978a7f525bcb3ad8fff64a55669586e0..468f70dc9e01e9663f7acfac41decac6935fea8a 100644
--- a/ios/web/net/crw_cert_verification_controller.mm
+++ b/ios/web/net/crw_cert_verification_controller.mm
@@ -9,9 +9,11 @@
 #import "base/memory/ref_counted.h"
 #import "base/memory/scoped_ptr.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/worker_pool.h"
 #include "ios/web/net/cert_verifier_block_adapter.h"
 #include "ios/web/public/browser_state.h"
 #include "ios/web/public/web_thread.h"
+#import "ios/web/web_state/wk_web_view_security_util.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/url_request/url_request_context.h"
@@ -73,13 +75,20 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
 // 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.
+// Verifies the given |cert| for the given |host| using |net::CertVerifier| and
+// calls |completionHandler| on completion. |completionHandler| cannot be null
+// and will be called asynchronously on IO thread.

[davidben, 2015/10/07 20:16:29]

Nit: If you're sticking with the callbacks as they are, add "May be called on
any thread."

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

This very callback can be called only on IO thread.

[davidben, 2015/10/12 23:21:49]

I mean verifyCert itself may be called on any thread. It used to be the case
that you could assume these would only get called on the UI thread.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Thanks for clarification. Done.

 - (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
               forHost:(NSString*)host
     completionHandler:(void (^)(net::CertVerifyResult, int))completionHandler;
 
+// Verifies the given |trust| using SecTrustRef API. |completionHandler| cannot
+// be null and will be either called asynchronously on Worker thread or
+// synchronously on current thread if the worker task can't start (in this
+// case |success| argument will be NO).

[davidben, 2015/10/07 20:16:29]

We could avoid some of the two-thread weirdness by having this function return
BOOL for whether it successfully posted or failed synchronously.

Then perhaps completionHandler should be named completionHandlerOnIO and
completionHandlerOnWorker so it's clear the callbacks are not called in the
standard way.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

I thought about it. But it would be an anti pattern for Objective-C code. In
Objective-C if there is a completion handler, it should be called for both
success and failure cases.

[davidben, 2015/10/12 23:21:49]

Mm. I would have thought, in Objective-C, if there is a completion handler it
should also always be called on the same thread. :-)

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

Ideally yes, completion handler should be called on the same thread.

In order to optimize for performance some ugliness is necessary (either this
method returns BOOL or handlers will be called on different threads). Personally
I believe that calling callback on different thread is more preferable (AFAIK in
some cases UIKit does it as well).

+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType, BOOL success))handler;
+
 @end
 
 @implementation CRWCertVerificationController
@@ -112,16 +121,16 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
 
 - (void)decidePolicyForCert:(const scoped_refptr<net::X509Certificate>&)cert
                        host:(NSString*)host
-          completionHandler:(web::PolicyDecisionHandler)handler {
+          completionHandler:(web::PolicyDecisionHandler)completionHandler {
   DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
   // completionHandler of |verifyCert:forHost:completionHandler:| is called on
   // IO thread and then bounces back to UI thread. As a result all objects
   // captured by completionHandler may be released on either UI or IO thread.
-  // Since |handler| can potentially capture multiple thread unsafe objects
-  // (like Web Controller) |handler| itself should never be released on
-  // background thread and |BlockHolder| ensures that.
+  // Since |completionHandler| can potentially capture multiple thread unsafe
+  // objects (like Web Controller) |completionHandler| itself should never be
+  // released on background thread and |BlockHolder| ensures that.
   __block scoped_refptr<BlockHolder<web::PolicyDecisionHandler>> handlerHolder(
-      new BlockHolder<web::PolicyDecisionHandler>(handler));
+      new BlockHolder<web::PolicyDecisionHandler>(completionHandler));
   [self verifyCert:cert
                 forHost:host
       completionHandler:^(net::CertVerifyResult result, int error) {
@@ -141,6 +150,54 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }];
 }
 
+- (void)querySSLStatusForTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+                          host:(NSString*)host
+             completionHandler:(web::StatusQueryHandler)completionHandler {

[davidben, 2015/10/07 20:16:29]

This callback currently almost always runs asynchronously, but in very rare edge
cases will run synchronously. You should the callback in the synchronous case by
a dispatch_async. Otherwise higher-level code like
updateSSLStatusForCurrentNavigationItem needs to account for
updateCurrentNavigationItemSSLStatusUsingCertChain possibly doing something and
possibly not doing something immediately.

[stuartmorgan, 2015/10/07 22:30:13]

Alternately, how about we push that down the stack into
verifyTrust:completionHandler:, so that it always calls back asynchronously (via
dispatch_async in the rare case).

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+
+  // The completion handlers of |verifyCert:forHost:completionHandler:| and
+  // |verifyTrust:completionHandler:| will be deallocated on background thread.
+  // |completionHandler| itself should never be released on background thread
+  // and |BlockHolder| ensures that.
+  __block scoped_refptr<BlockHolder<web::StatusQueryHandler>> handlerHolder(
+      new BlockHolder<web::StatusQueryHandler>(completionHandler));
+  [self verifyTrust:trust
+      completionHandler:^(SecTrustResultType trustResult, BOOL success) {
+        if (!success) {
+          // |verifyTrust:completionHandler:| was not able to start WorkerThread
+          // task, so |completionHandler| is called on UI thread.
+          DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);

[stuartmorgan, 2015/10/07 22:30:14]

Instead of this, how about we dispatch_async(dispatch_get_main_queue() here
exactly like all the other cases. Yes, it's pointless, but it makes this code
significantly less confusing IMO, and it's an exceptionally rare case so the
performance doesn't matter.

+          handlerHolder->call(web::SECURITY_STYLE_UNKNOWN, net::CertStatus());
+          return;
+        }
+
+        web::SecurityStyle securityStyle =
+            web::GetSecurityStyleFromTrustResult(trustResult);
+        if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) {
+          // SecTrust API considers this cert as valid.
+          dispatch_async(dispatch_get_main_queue(), ^{
+            handlerHolder->call(securityStyle, net::CertStatus());
+          });
+          return;
+        }
+
+        // Retrieve the net::CertStatus for invalid certificates to determine
+        // the rejection reason.
+        // TODO(eugenebut): Add UMA for CertVerifier and SecTrust verification
+        // mismatch (crbug.com/535699).

[davidben, 2015/10/07 20:16:29]

This is very possible as their implementations have nothing to do with each
other. You won't need UMA to determine if it happens.

It's probably even possible that two SecTrustEvaluate calls give different
answers. This is a very non-deterministic process and you can't make any
assumptions on behavior.

Make sure your code is okay with this. E.g. you may get didFailProvisionalLoad
and have to show an interstitial on a good certificate.

I would probably explicitly filter out the ones NSS liked because the rest of
your system, based on the design doc, isn't going to accept those certs. Perhaps
turn them into some generic error.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

We know that mismatch will happen, but we (including felt@) want to know how
often it happens. Do you think that knowing how often mismatch happens is
unnecessary? 
UX will be bad, but app can handle that gracefully.
What do you mean by filter out?

[davidben, 2015/10/12 23:21:49]

I meant you might want to turn those into a different SecurityStyle or something
right now. But I suppose you haven't lost information, so it could be done
later. Added a comment about documenting the weirdness in the public API
instead.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

I see your point. Introducing a new security style will require additional
discussions with felt@, so if you ok with current approach I would prefer to
leave the code as is, and revisit this topic later.

[davidben, 2015/10/15 17:46:12]

We mentioned this in the meeting, but UNKNOWN vs a new SecurityStyle both seem
fine to me as far as the NavigationItem not being in a confused state (half one
page, half another). Whether you want a new one is I think largely a question
for whether the UI needs to differentiate the two.

[Eugene But (OOO till 7-30), 2015/10/15 22:59:34]

Thank you for confirmation.

+        scoped_refptr<net::X509Certificate> cert(
+            web::CreateCertFromTrust(trust));
+        [self verifyCert:cert
+                      forHost:host
+            completionHandler:^(net::CertVerifyResult certVerifierResult, int) {
+              dispatch_async(dispatch_get_main_queue(), ^{
+                handlerHolder->call(securityStyle,
+                                    certVerifierResult.cert_status);
+              });
+            }];
+      }];
+}
+
 - (void)shutDown {
   DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
   web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
@@ -197,4 +254,22 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }));
 }
 
+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType, BOOL))completionHandler {
+  DCHECK(completionHandler);
+  // SecTrustEvaluate performs trust evaluation synchronously, possibly making
+  // network requests. The IO thread should not be blocked by that operation.

[davidben, 2015/10/07 20:16:29]

IO -> UI? This code I believe runs on the UI thread, so the mention of IO is
strange.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

A few patches ago this method was called on IO thread. Fixed comment.

+  bool result = base::WorkerPool::PostTask(FROM_HERE, base::BindBlock(^{
+    SecTrustResultType trustResult = kSecTrustResultInvalid;
+    if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) {
+      trustResult = kSecTrustResultInvalid;
+    }
+    completionHandler(trustResult, YES);
+  }), false /* task_is_slow */);
+
+  if (!result) {
+    completionHandler(kSecTrustResultInvalid, NO);
+  }
+}
+
 @end