WKWebView: Added cert verification API to web controller.

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 b64f4bb5890827ffdc533fedba842e435b106a79..3b7fab13d4c8d0f8cc38ba5c49a3b1e4f05f8ef1 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
@@ -17,6 +17,7 @@
 #import "ios/web/crw_network_activity_indicator_manager.h"
 #import "ios/web/navigation/crw_session_controller.h"
 #include "ios/web/navigation/web_load_params.h"
+#include "ios/web/net/cert_verifier_block_adapter.h"
 #include "ios/web/public/web_client.h"
 #import "ios/web/public/web_state/js/crw_js_injection_manager.h"
 #import "ios/web/public/web_state/ui/crw_native_content_provider.h"
@@ -35,6 +36,8 @@
 #import "ios/web/web_state/web_view_internal_creation_util.h"
 #import "ios/web/webui/crw_web_ui_manager.h"
 #import "net/base/mac/url_conversions.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/ssl/ssl_config_service.h"
 
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 #include "ios/web/public/cert_store.h"
@@ -125,6 +128,9 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   // CRWWebUIManager object for loading WebUI pages.
   base::scoped_nsobject<CRWWebUIManager> _webUIManager;
+
+  // Cert verification object which wraps net::CertVerifier.
+  net::CertVerifierBlockAdapter _certVerifier;
 }
 
 // Response's MIME type of the last known navigation.
@@ -227,6 +233,13 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // Attempts to handle a script message. Returns YES on success, NO otherwise.
 - (BOOL)respondToWKScriptMessage:(WKScriptMessage*)scriptMessage;
 
+// Verifies the given |cert| for the given |host| and calls |block| on
+// completion. |block| can not be null and may be called either synchronously or
+// asynchronously.
+- (void)verifyCert:(scoped_refptr<net::X509Certificate>)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block;

[davidben, 2015/07/31 18:58:46]

[Shouldn't these be indented such that the colons line up? I dunno, I don't
understand Obj-C style, so I'll defer to you. :-) ]

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

This indentation is correct. When the first keyword is shorter than the others,
indent the later lines by at least four spaces, maintaining colon alignment:
http://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations_...

+
 // Used to decide whether a load that generates errors with the
 // NSURLErrorCancelled code should be cancelled.
 - (BOOL)shouldAbortLoadForCancelledError:(NSError*)error;
@@ -833,6 +846,18 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
              : [super selectorToHandleJavaScriptCommand:command];
 }
 
+- (void)verifyCert:(scoped_refptr<net::X509Certificate>)cert
+              forHost:(NSString*)host
+    completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block {
+  DCHECK(block);
+  std::string hostname = base::SysNSStringToUTF8(host);
+  net::CertVerifierBlockAdapter::Params params(cert, hostname);
+  params.ocsp_response = "";  // Not provided by iOS API.

[Ryan Sleevi, 2015/08/01 01:36:22]

= "" is unnscessary (you should have a default ctor), but even if it was, you
should use either = std::string() or .clear()

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

I just want to be explicit that ocsp_response is empty. And the code which
assigns empty string communicates that very well.

Using std::string(), clear() or "" is a style preference. Both C++ and Chromium
Style guide does now disallow assigning char* to std::string. F.e.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_and...

Having said that I would prefer to leave the code as it is, both suggested and
current approaches are simply a personal preference.

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

We developed a clang tool to excise this pattern from the codebase. It's
certainly less efficient than either of the alternatives I suggested. While I
can appreciate personal preference, efficiency seems a reasonable argument over
unnecessary code. If you do feel strongly, it seems like simply documenting it
is empty is better to calling attention than adding code.

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

Thanks, I did not know about clang changes. Replaced with a comment. 

+  params.flags = net::CertVerifier::VERIFY_CERT_IO_ENABLED;

[Ryan Sleevi, 2015/08/01 01:36:22]

This doesn't seem correct.

See
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config...

Although you can't use the socket APIs, you still should be able to get an
SSLConfig from the higher layers (e.g. the SSLConfigService) to influence
behaviour here, right?

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Done, thanks for the link.

+  params.crl_set = net::SSLConfigService::GetCRLSet().Pass();
+  _certVerifier.Verify(params, block);
+}
+
 - (BOOL)shouldAbortLoadForCancelledError:(NSError*)error {
   DCHECK_EQ(error.code, NSURLErrorCancelled);
   // Do not abort the load if it is for an app specific URL, as such errors
@@ -1166,8 +1191,21 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
                     completionHandler:
         (void (^)(NSURLSessionAuthChallengeDisposition disposition,
                   NSURLCredential *credential))completionHandler {

[davidben, 2015/07/31 18:58:46]

Isn't this supposed to check the protectionSpace.authenticationMethod and make
sure it's a NSURLAuthenticationMethodServerTrust?

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

challenge.protectionSpace.serverTrust returns nil if authenticationMethod is not
NSURLAuthenticationMethodServerTrust.
CreateCertFromTrust returns null for null SecTrustRef and CertVerifierAdapter
calls completion handler with error if cert is null.

So everything should be fine.

[davidben, 2015/08/03 18:06:32]

This is unreasonable to rely on.
As far as I can tell, it does not. The NULL gets passed all the way into the
CertVerifier which does NOT check this and shouldn't.
This needs an explicit check. This method is a common sink for multiple kinds of
authentication methods, so it's important to check which type you're dealing
with.

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

I guess auth method, other than NSURLAuthenticationMethodServerTrust can be a
special case. Done.

-  NOTIMPLEMENTED();
-  completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);
+  SecTrustRef trust = challenge.protectionSpace.serverTrust;
+  scoped_refptr<net::X509Certificate> cert = web::CreateCertFromTrust(trust);
+  [self verifyCert:cert
+                forHost:challenge.protectionSpace.host
+      completionHandler:^(scoped_ptr<net::CertVerifyResult> result,
+                          int status) {
+        DCHECK(result || status);
+        if (result && !net::IsCertStatusError(result->cert_status)) {

[Ryan Sleevi, 2015/08/01 01:36:22]

This doesn't seem right either - normally you'd also want to check
net::IsCertStatusMinorError as well

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Done.

+          // Cert is valid.
+        } else {
+          // Cert is invalid.

[davidben, 2015/07/31 18:58:46]

The docs point to this sample code:

https://developer.apple.com/library/prerelease/ios/documentation/NetworkingIn...

Which isn't quite the same API, but it does have sample code to get the
NSURLCredential out.

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

Thanks for the link. Accepting bad SSL cert is not implemented in this CL yet.
With current implementation SSL icon will be red, only if
webView:didFailProvisionalNavigation:withError: was called.
So before implementing recoverable SSL interstitial I want to fix SSL icon
coloring first, and for that I need to have cert verification method.
This design doc describes the big picture of cert verification:
https://docs.google.com/document/d/1o8wzJkxzG1fvhvW9Gqf-RdGjhm0btCIehDtbxDfbL...

+        }
+        NOTIMPLEMENTED();
+        completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);

[davidben, 2015/07/31 18:58:46]

Is this supposed to be NSURLSessionAuthChallengeRejectProtectionSpace or
NSURLSessionAuthChallengePerformDefaultHandling?

[Eugene But (OOO till 7-30), 2015/08/01 00:25:40]

NSURLSessionAuthChallengeRejectProtectionSpace is the current code, which
accepts good certs and cancels request for bad certs.

For bad certs didFailProvisionalNavigation: is called after request is
cancelled. I will investigate the difference between
NSURLSessionAuthChallengePerformDefaultHandling and
NSURLSessionAuthChallengeRejectProtectionSpace before implementing recoverable
interstitials.

But I agree that NSURLSessionAuthChallengePerformDefaultHandling will be more
appropriate.

[Eugene But (OOO till 7-30), 2015/08/05 16:13:43]

Changed to NSURLSessionAuthChallengePerformDefaultHandling.

+      }];
 }
 
 - (void)webViewWebContentProcessDidTerminate:(WKWebView*)webView {