Add certificate error handling to devtools.

content/browser/ssl/ssl_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/ssl/ssl_manager.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/macros.h"
 #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"
 #include "content/browser/ssl/ssl_error_handler.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #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"
 
 namespace content {
 
 namespace {
 
 const char kSSLManagerKeyName[] = "content_ssl_manager";
 
 // Events for UMA. Do not reorder or change!
 enum SSLGoodCertSeenEvent {
   NO_PREVIOUS_EXCEPTION = 0,
   HAD_PREVIOUS_EXCEPTION = 1,
   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) {
     case CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE:
       // Note that we should not call SetMaxSecurityStyle here, because
       // the active NavigationEntry has just been deleted (in
       // HideInterstitialPage) and the new NavigationEntry will not be
       // set until DidNavigate.  This is ok, because the new
       // NavigationEntry will have its max security style set within
       // DidNavigate.
       //
       // 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());
       }
       handler->ContinueRequest();
       return;
     case CERTIFICATE_REQUEST_RESULT_TYPE_DENY:
       handler->DenyRequest();
       return;
     case CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL:
       handler->CancelRequest();
@@ skipping 281 matching lines @@
   bool overridable = (options_mask & OVERRIDABLE) != 0;
   bool strict_enforcement = (options_mask & STRICT_ENFORCEMENT) != 0;
   bool expired_previous_decision =
       (options_mask & EXPIRED_PREVIOUS_DECISION) != 0;
 
   WebContents* web_contents = handler->web_contents();
   int cert_error = handler->cert_error();
   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,
                              int add_content_status_flags,
                              int remove_content_status_flags) {
   // We don't always have a navigation entry to update, for example in the
   // case of the Web Inspector.
   if (!entry)
     return;
 
@@ skipping 47 matching lines @@
   SSLManagerSet* managers =
       static_cast<SSLManagerSet*>(context->GetUserData(kSSLManagerKeyName));
 
   for (std::set<SSLManager*>::iterator i = managers->get().begin();
        i != managers->get().end(); ++i) {
     (*i)->UpdateEntry((*i)->controller()->GetLastCommittedEntry(), 0, 0);
   }
 }
 
 }  // namespace content