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..fd3f39bb19508469b33750422c23ac9ae833c71c 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,19 @@ 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.
 - (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 of worker task can't start.

[davidben, 2015/10/05 22:19:11]

of -> if the

[davidben, 2015/10/05 22:19:11]

As with the previous CL, the thread-hopping here gets really confusing. I think
it would be clearer if you kept the core logic on one thread. (Note that now
verifyCert must be callable on two different threads.)

You can make verifyCert's callback internally post back to the main thread
without changing anything since you're always posting back to the main thread
right now. Likewise, verifyTrust's ought to as well. It is, strictly speaking,
one extra thread hop, but only on invalid certificates and the result is the
code is much easier to reason about.

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

verifyCert: and verifyTrust: will be called for making load/no-load decision for
bad SSL certs. I do understand that it's not a general case, but I still want to
avoid bouncing between threads for performance reasons.

For load-no load decision I can avoid 2 extra hops (marked with asterisk):
UI (called on UI) -> Worker (SecTrust) -> *UI* -> IO (CertVerifier) -> *UI* ->
IO (UserDecisionPolicy) -> UI (reply callback)

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

Done.

+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType))completionHandler;
+
 @end
 
 @implementation CRWCertVerificationController
@@ -112,16 +120,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 +149,45 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }];
 }
 
+- (void)querySSLStatusForTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+                          host:(NSString*)host
+             completionHandler:(web::StatusQueryHandler)completionHandler {
+  DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI);
+
+  // Completion handler of |verifyCert:forHost:completionHandler:| will be

[davidben, 2015/10/05 22:19:10]

verifyCert:forHost -> verifyTrust?

[davidben, 2015/10/05 22:19:11]

Nit: Completion -> The completion

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

Updated comments.

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

Done.

+  // deallocated on IO 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));
+  scoped_refptr<net::X509Certificate> cert(web::CreateCertFromTrust(trust));
+  [self verifyTrust:trust
+      completionHandler:^(SecTrustResultType trustResult) {
+        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).
+        [self verifyCert:cert
+                      forHost:host

[davidben, 2015/10/05 22:19:11]

This will end up calling verifyCert if posting to the WorkerPool failed, but
that doesn't make a whole lot of sense since the certificate itself wasn't
invalid. There was just an internal error.

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

Good point. Fixed.

+            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 +244,22 @@ class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
       }));
 }
 
+- (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
+    completionHandler:(void (^)(SecTrustResultType))completionHandler {
+  DCHECK(completionHandler);
+  // SecTrustEvaluate performs trust evaluation synchronously, possibly making
+  // network requests. IO thread should not be blocked by that operation.

[davidben, 2015/10/05 22:19:11]

Nit: The IO thread

[Eugene But (OOO till 7-30), 2015/10/06 03:10:09]

Done.

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