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

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

Index: ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
index 368587eab562249bab292466ff34a608391942d3..ed03c4d070fc220d65397e770a6226f161ac9096 100644
--- a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
+++ b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
@@ -11,6 +11,7 @@
 #include "base/json/json_reader.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/values.h"
 #import "ios/net/http_response_headers_util.h"
@@ -82,6 +83,14 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
 }  // namespace
 
+#if !defined(__IPHONE_9_0)
+// Predeclare iOS9 API so calls are compiled on iOS8 bots.
+// TODO(eugenebut): cleanup this (crbug.com/523365).
+@interface WKWebView (CRWIOS9API)
+@property(nonatomic, readonly, copy) NSArray* certificateChain;
+@end
+#endif
+
 @interface CRWWKWebViewWebController () <WKNavigationDelegate,
                                          WKScriptMessageHandler,
                                          WKUIDelegate> {
@@ -250,6 +259,12 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // Clears all activity indicator tasks for this web controller.
 - (void)clearActivityIndicatorTasks;
 
+// Obtains SSL status from given |certChain| and updates navigation items with
+// given |certID| and |host|.
+- (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID
+                                               host:(NSString*)host
+                                     usingCertChain:(NSArray*)certChain;
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 // Updates SSL status for the current navigation item based on the information
 // provided by web view.
@@ -864,12 +879,54 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
       clearNetworkTasksForGroup:[self activityIndicatorGroupID]];
 }
 
+- (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID

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

Yikes. What exactly do you all use cert IDs for? In Chrome, this is some
machinery for dealing with multiple processes and I need to get it out of the
SSLStatus struct for other work anyway. That doesn't seem applicable here at
all.

Context here is https://crbug.com/513315.

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

Fetching CertStatus is asynchronous operation, so by the time when it's
completed *current* NavigationItem may be changed (or even removed completely).
The only reliable way to update requested Navigation Item is to find it by
cert/host pair. Since multiple navigation items may have same cert/host pair I'm
updating them all. I can stop using cert_id, once SSLStatus has cert field, but
for now there is no other way.

[stuartmorgan, 2015/10/06 22:02:36]

Per offline discussion, let's have the navigation item's UniqueID be the primary
lookup, and then you can just validate that the answer still applies to that
item (e.g., that a redirect hasn't changed the URL).

[Eugene But (OOO till 7-30), 2015/10/07 15:27:13]

Done.

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

Oh, actually, I'd missed that you don't use content::SSLStatus and instead have
your own copy of everything. So me messing with content::SSLStatus won't affect
you. (Whether you want to switch away from cert IDs or no I'll leave to you.)

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

We want to keep web::SSLStatus as close as possible to content::SSLStatus. I
CCed myself to https://crbug.com/513315 so we know when you get rid of cert_id.

+                                               host:(NSString*)host
+                                     usingCertChain:(NSArray*)certChain {
+  base::WeakNSObject<CRWWKWebViewWebController> weakSelf(self);
+  void (^SSLStatusResponse)(web::SecurityStyle, net::CertStatus) = ^(
+      web::SecurityStyle style, net::CertStatus certStatus) {
+    base::scoped_nsobject<CRWWKWebViewWebController> strongSelf(
+        [weakSelf retain]);
+    if (!strongSelf || [strongSelf isBeingDestroyed]) {
+      return;
+    }
+
+    web::NavigationManager* navigationManager =
+        [strongSelf webStateImpl]->GetNavigationManager();
+    int currentItemIndex = navigationManager->GetCurrentEntryIndex();
+
+    bool updatedCurrentItem = false;
+    std::string GURLHost = base::SysNSStringToUTF8(host);
+    for (int i = 0; i < navigationManager->GetEntryCount(); i++) {
+      web::NavigationItem* item = navigationManager->GetItemAtIndex(i);
+      web::SSLStatus& SSLStatus = item->GetSSL();
+      if (SSLStatus.cert_id == certID && item->GetURL().host() == GURLHost) {
+        web::SSLStatus previousSSLStatus = item->GetSSL();
+        SSLStatus.cert_status = certStatus;
+        SSLStatus.security_style = style;
+        if (currentItemIndex == i && !previousSSLStatus.Equals(SSLStatus)) {
+          updatedCurrentItem = true;
+        }
+      }
+    }
+
+    if (updatedCurrentItem) {
+      [strongSelf didUpdateSSLStatusForCurrentNavigationItem];
+    }
+  };
+
+  [_certVerificationController
+      querySSLStatusForTrust:web::CreateServerTrustFromChain(certChain, host)
+                        host:host
+           completionHandler:SSLStatusResponse];
+}
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
+
 - (void)updateSSLStatusForCurrentNavigationItem {
   if ([self isBeingDestroyed])
     return;
 
-  DCHECK(self.webStateImpl);
   web::NavigationItem* item =
       self.webStateImpl->GetNavigationManagerImpl().GetLastCommittedItem();
   if (!item)
@@ -877,34 +934,58 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   web::SSLStatus previousSSLStatus = item->GetSSL();
   web::SSLStatus& SSLStatus = item->GetSSL();
-  if (item->GetURL().SchemeIsCryptographic()) {
-    // TODO(eugenebut): Do not set security style to authenticated once
-    // proceeding with bad ssl cert is implemented.
-    SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
-    SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
-                                   ? web::SSLStatus::NORMAL_CONTENT
-                                   : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
-
-    if (base::ios::IsRunningOnIOS9OrLater()) {
-      scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(
-          [_wkWebView performSelector:@selector(certificateChain)]));
-      if (cert) {
-        SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
-            cert.get(), self.certGroupID);
-      } else {
-        SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
-        SSLStatus.cert_id = 0;
+
+  // Starting from iOS9 WKWebView blocks active mixed content, so if
+  // |hasOnlySecureContent| returns NO it means passive content.
+  // On iOS8 there is no way to determine if web view has active mixed content.
+  SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
+                                 ? web::SSLStatus::NORMAL_CONTENT
+                                 : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
+
+  // Retrieve top level frame certificate.
+  scoped_refptr<net::X509Certificate> cert;
+  if (base::ios::IsRunningOnIOS9OrLater() &&
+      item->GetURL().SchemeIsCryptographic()) {
+    NSArray* chain = [_wkWebView certificateChain];
+    cert = web::CreateCertFromChain(chain);
+    if (cert) {
+      UMA_HISTOGRAM_BOOLEAN("WebController.WKWebViewHasCertForSecureConnection",
+                            true);
+      int oldCertID = SSLStatus.cert_id;
+      SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
+          cert.get(), self.certGroupID);
+      NSString* host = [_wkWebView URL].host;
+      if (oldCertID != SSLStatus.cert_id ||

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

I don't believe this can ever be false. Looking at how Chrome/iOS implements
CertStore, it appears to compare by pointers alone, and web::CreateCertFromChain
always makes a fresh on.

(See above for why you shouldn't depend on cert IDs don't interesting and below
for why you would want them to do anything interesting anyway.)

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

This can be false and w/o this check cert status will be recomputed when
unnecessary. SSLStatus does not have any other way to identify cert, so I have
to use cert_id at least for now.

web::CertStore compares certs using LessThen comparator, not pointers:
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/net/reques...

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

Ah, you're right. I'd missed that.

+          item->GetURL().host() != base::SysNSStringToUTF8(host)) {
+        [self updateSSLStatusForNavigationItemsWithCertID:SSLStatus.cert_id
+                                                     host:host
+                                           usingCertChain:chain];

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

Why update all navigation entries? This is confusing when this function's name
suggests it only updates the current one.

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

Replied above. I did not find a good way to find current Navigation Item at the
point when CertStatus is evaluated. 

[Eugene But (OOO till 7-30), 2015/10/07 15:27:13]

Actually there is a way to update only requested entry. Fixed.

       }
     }
-  } else {
-    SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+  }
+
+  if (!cert) {
     SSLStatus.cert_id = 0;
+    if (!item->GetURL().SchemeIsCryptographic()) {
+      // HTTP or other non-secure connection.
+      SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+    } else if (base::ios::IsRunningOnIOS9OrLater()) {
+      // HTTPS, iOS9 and no certificate (this use-case has not been observed).
+      UMA_HISTOGRAM_BOOLEAN("WebController.WKWebViewHasCertForSecureConnection",
+                            false);

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

UMA_HISTOGRAM macros expand to a fair amount of code. It emits a static thing
and everything. It's better to have only one instance of it and compute a
boolean somewhere.

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

Moved to a separate function.

+      SSLStatus.security_style = web::SECURITY_STYLE_UNKNOWN;
+    } else {
+      // HTTPS, iOS8.
+      // iOS8 cannot load unauthenticated HTTPS content.
+      SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
+    }
   }
 
   if (!previousSSLStatus.Equals(SSLStatus)) {
     [self didUpdateSSLStatusForCurrentNavigationItem];
   }
 }
+
 #endif  // !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
 - (void)registerLoadRequest:(const GURL&)url {