Add certificate error handling to devtools.

content/browser/ssl/ssl_manager.cc

Index: content/browser/ssl/ssl_manager.cc
diff --git a/content/browser/ssl/ssl_manager.cc b/content/browser/ssl/ssl_manager.cc
index df2e7575b49da85f0455e6e880cc5e0f01d02318..08cfeb127d6140fce006f6eb6ee301d34d5fea69 100644
--- a/content/browser/ssl/ssl_manager.cc
+++ b/content/browser/ssl/ssl_manager.cc
@@ -11,6 +11,8 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/supports_user_data.h"
+#include "content/browser/devtools/devtools_agent_host_impl.h"
+#include "content/browser/devtools/protocol/security_handler.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
@@ -20,6 +22,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/certificate_request_result_type.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/ssl_host_state_delegate.h"
 #include "net/url_request/url_request.h"
@@ -37,8 +40,17 @@ enum SSLGoodCertSeenEvent {
   SSL_GOOD_CERT_SEEN_EVENT_MAX = 2
 };
 
+void OnAllowCertificateWithRecordDecision(
+    bool record_decision,
+    const base::Callback<void(bool, content::CertificateRequestResultType)>&
+        callback,
+    CertificateRequestResultType decision) {
+  callback.Run(record_decision, decision);
+}
+
 void OnAllowCertificate(SSLErrorHandler* handler,
                         SSLHostStateDelegate* state_delegate,
+                        bool record_decision,
                         CertificateRequestResultType decision) {
   DCHECK(handler->ssl_info().is_valid());
   switch (decision) {
@@ -53,7 +65,7 @@ void OnAllowCertificate(SSLErrorHandler* handler,
       // While AllowCert() executes synchronously on this thread,
       // ContinueRequest() gets posted to a different thread. Calling
       // AllowCert() first ensures deterministic ordering.
-      if (state_delegate) {
+      if (record_decision && state_delegate) {
         state_delegate->AllowCert(handler->request_url().host(),

[estark, 2017/03/09 00:24:31]

I'm not sure that this will work; have you tested it on a page that has
subresources? (like a cert error on www.blah.com that loads an image
www.blah.com/foo.jpeg)

I think that in order for that to work reliably, you'll either need to expose
cert errors on subresources to devtools (undoing my previous suggestion), or
you'll need to change the SSLHostStateDelegate so that it stores cert error
decisions differently for when devtools bypasses them.

Of those two options, the first is probably way simpler. So I guess I led you
astray on subresources -- if we want devtools to be able to load a full page
after clicking through a cert error without remembering the decision for a week
as we do for normal interstitials, then devtools needs to be able to bypass cert
errors on subresources.

(I assume that asking devtools to bypass a cert error on every subresource could
have performance implications, but I'm not familiar enough with this project to
determine if that's a blocker or not.)

[Eric Seckler, 2017/03/09 11:34:53]

For use in headless, I think we'll be happy with having to handle errors for
every subresource. May not be ideal, but given the effort required for the
alternative, it sounds like the better choice :)

[irisu, 2017/03/13 01:56:56]

Added an image to the test html, let me know if that's not what you had in mind
- but it seems to be working without needing an extra certificateError event for
the image.

[estark, 2017/03/14 01:22:50]

By "it seems to be working", do you mean that the test works, or did you
manually test loading a page with an image and see that it works without an
event for the subresource? If the latter, there might be some connection pooling
or some such -- the subresource might not need a new connection.

Could you please add a browser test for the subresource case? For example you
could add a test where the main page does not trigger a cert error but a
subresource on the page does.

I think you can probably adapt a test like
https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser...
to set that up.

[irisu, 2017/03/16 03:40:18]

Done. I added the subresource case and figured out the issue with my previous
approach.

                                   *handler->ssl_info().cert.get(),
                                   handler->cert_error());
@@ -356,11 +368,31 @@ void SSLManager::OnCertErrorInternal(std::unique_ptr<SSLErrorHandler> handler,
   const net::SSLInfo& ssl_info = handler->ssl_info();
   const GURL& request_url = handler->request_url();
   ResourceType resource_type = handler->resource_type();
-  GetContentClient()->browser()->AllowCertificateError(
-      web_contents, cert_error, ssl_info, request_url, resource_type,
-      overridable, strict_enforcement, expired_previous_decision,
+
+  base::Callback<void(bool, content::CertificateRequestResultType)> callback =
       base::Bind(&OnAllowCertificate, base::Owned(handler.release()),
-                 ssl_host_state_delegate_));
+                 ssl_host_state_delegate_);
+
+  if (resource_type != RESOURCE_TYPE_MAIN_FRAME) {

[estark, 2017/03/09 00:24:31]

Per my comment above, I think we need to go back on this and actually expose
subresource cert errors to devtools if it needs to be able to load a full page
(not just the main resource) without relying on cert decisions being remembered.

[irisu, 2017/03/13 01:56:56]

Done.

+    // A sub-resource has a certificate error. Deny the request without sending
+    // devtools event or showing UI interstitial.
+    callback.Run(false, CERTIFICATE_REQUEST_RESULT_TYPE_DENY);
+    return;
+  }
+
+  DevToolsAgentHostImpl* agent_host = static_cast<DevToolsAgentHostImpl*>(
+      DevToolsAgentHost::GetOrCreateFor(web_contents).get());
+  protocol::SecurityHandler* security_handler =
+      protocol::SecurityHandler::FromAgentHost(agent_host);
+  if (!security_handler ||
+      !security_handler->NotifyCertificateError(
+          cert_error, request_url,
+          base::Bind(&OnAllowCertificateWithRecordDecision, false, callback))) {
+    GetContentClient()->browser()->AllowCertificateError(
+        web_contents, cert_error, ssl_info, request_url, resource_type,
+        overridable, strict_enforcement, expired_previous_decision,
+        base::Bind(&OnAllowCertificateWithRecordDecision, true, callback));
+  }
 }
 
 void SSLManager::UpdateEntry(NavigationEntryImpl* entry,