Remember user decisions on invalid certificates behind a flag

content/browser/ssl/ssl_host_state.cc

Index: content/browser/ssl/ssl_host_state.cc
diff --git a/content/browser/ssl/ssl_host_state.cc b/content/browser/ssl/ssl_host_state.cc
index 06c600205fa8b1277b5252e91cbbf56580320c21..6e61d4d8b0de3817da2793980ae8db511e2dbe60 100644
--- a/content/browser/ssl/ssl_host_state.cc
+++ b/content/browser/ssl/ssl_host_state.cc
@@ -6,16 +6,40 @@
 
 #include "base/logging.h"
 #include "base/lazy_instance.h"
+#include "base/pickle.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/ssl_host_state_decisions.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
 
 const char kKeyName[] = "content_ssl_host_state";
 
+namespace {
+
+void CloseIdleConnections(
+    const std::string& host,
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+  url_request_context_getter->GetURLRequestContext()
+      ->http_transaction_factory()
+      ->GetSession()
+      ->CloseIdleConnections();

[Ryan Sleevi, 2014/07/08 23:53:29]

I really, really, really don't like this being called. Especially because "Idle"
connections has a particular meaning that may not be doing what you want.

I'd be happy to leave this out of the first run. Otherwise, flush the socket
pools if the user changes connection state (or, ideally, just flush the affected
sockets)

[jww, 2014/07/11 00:08:42]

This was suggested by willchan (who I'm CC'ing in case he has anything to add)
as the correct way to deal with problems that we were arising since the
connection pool keeps its own state about "user decisions." That is, if a
decision is changed because of timeout (or, soon, user choice), that is not
actually reflected immediately at the socket level, which has its own state, and
this led to some real bugs that that revoking the decision, you could still
reload the page in the same tab and bypass the interstitial. 

My understanding is that the only way to avoid this is by closing idle
connections. Obviously, this is high cost, but this should be a pretty
infrequent event. Is there another method that makes more sense for this kind of
flushing that we can use instead?

[Ryan Sleevi, 2014/07/11 00:48:49]

I'm aware of the potential for bugs, I'm not aware of any 'real' bugs, but more
importantly, I think the discussion around this is worthy in and of itself its
own CL, with its own set of nuance.

Whether it should be tackled first or a follow-up is fine, but I don't think we
should be doing this in this CL.

[jww, 2014/07/11 01:52:07]

Sorry, I'm confused about the action to take at this point. I was stating that
without CloseIdleConnections(), this behavior leads to inconsistent state
between the interstitial layer (SSLHostStateDecisions) and the network layer
(unfortunately I can't reference the networking code that does this off the top
of my head). I took the heavy hammer here to simply close the connections in the
pool (at least I thought that's what I was doing). Without this close, this code
doesn't work and it leads to a potentially nonsensical browsing experience where
a cert has expired and some tabs show an interstitial error and other tabs don't
(that's what I meant by 'bugs').

Is there another heavy hammer we can use to avoid this? I don't feel
particularly attached to the CloseIdleConnections call; I used it because I
thought it was the right one, and seemed to resolve the bugs.

[Ryan Sleevi, 2014/07/11 19:43:27]

Don't use the heavy hammer here.

Yes, it has the potential for bugs. This potential already exists. Don't try to
fix a bug and refactor/introduce features in the same CL.

The bug fix has a lot of nuance, hence I don't want it detracting from the CL,
other than acknowledging that yes, in theory, there's a bug here.

We boot connections far too often, and far too unnecessarily. It makes Chrome
completely unusuable in some situations - such as when you're behind the Great
Firewall - and it's a hammer that really, really, really needs to be a
screwdriver.

But that's a battle for a different CL :)

[jww, 2014/07/14 21:21:15]

Okay, sounds good. However, just to be explicit: there's no "potential" here,
this *is* introducing a known bug. It's not going to be a common one, and even
if experienced, it shouldn't be that big of a deal, but it will exist. I'll make
a comment in the the RevokeAllowAndDenyPreferences method explaining the
problem.

+}
+
+}  // namespace
+
 namespace content {
 
 SSLHostState* SSLHostState::GetFor(BrowserContext* context) {
   SSLHostState* rv = static_cast<SSLHostState*>(context->GetUserData(kKeyName));
   if (!rv) {
     rv = new SSLHostState();
+    rv->browser_context_ = context;
+    rv->decisions_ = context->GetSSLHostStateDecisions();
+    // All non-testing contexts need to implement a certificate decision storage
+    // strategy of some sort.
+    DCHECK(rv->decisions_);
     context->SetUserData(kKeyName, rv);
   }
   return rv;
@@ -39,33 +63,48 @@ bool SSLHostState::DidHostRunInsecureContent(const std::string& host,
 }
 
 void SSLHostState::DenyCertForHost(net::X509Certificate* cert,
-                                   const std::string& host,
+                                   const GURL& url,
                                    net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Deny(cert, error);
+  decisions_->DenyCert(url, cert, error);
 }
 
 void SSLHostState::AllowCertForHost(net::X509Certificate* cert,
-                                    const std::string& host,
+                                    const GURL& url,
                                     net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Allow(cert, error);
+  decisions_->AllowCert(url, cert, error);
 }
 
-void SSLHostState::Clear() {
+void SSLHostState::RevokeAllowAndDenyPreferences(const GURL& url) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_.clear();
+  decisions_->RevokeAllowAndDenyPreferences(url);
+
+  scoped_refptr<net::URLRequestContextGetter> getter(
+      browser_context_->GetRequestContext());
+  browser_context_->GetRequestContext()->GetNetworkTaskRunner()->PostTask(
+      FROM_HERE, base::Bind(&CloseIdleConnections, url.host(), getter));
+}
+
+bool SSLHostState::HasAllowedOrDeniedCert(const GURL& url) {
+  DCHECK(CalledOnValidThread());
+
+  return decisions_->HasAllowedOrDeniedCert(url);
+}
+
+void SSLHostState::Clear() {
+  decisions_->Clear();
 }
 
 net::CertPolicy::Judgment SSLHostState::QueryPolicy(net::X509Certificate* cert,
-                                                    const std::string& host,
+                                                    const GURL& url,
                                                     net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  return cert_policy_for_host_[host].Check(cert, error);
+  return decisions_->QueryPolicy(url, cert, error);
 }
 
 }  // namespace content