HttpServerProperties: Pass NetworkIsolationKey to SupportsSpdy() methods

This CL adds NetworkIsolationKey as an argument to H2-support-checking
methods in HttpServerProperties, and makes consumers pass in the correct
key.

Bug: 969890
Change-Id: I8ea06693a470eef3ca24ec8d269ce5ac131e312f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753807Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688607}

net/http/http_server_properties.cc

@@ -162,12 +162,13 @@ void HttpServerProperties::Clear(base::OnceClosure callback) {
 }
 
 bool HttpServerProperties::SupportsRequestPriority(
-    const url::SchemeHostPort& server) {
+    const url::SchemeHostPort& server,
+    const net::NetworkIsolationKey& network_isolation_key) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (server.host().empty())
     return false;
 
-  if (GetSupportsSpdy(server))
+  if (GetSupportsSpdy(server, network_isolation_key))
     return true;
   const AlternativeServiceInfoVector alternative_service_info_vector =
       GetAlternativeServiceInfos(server);
@@ -180,25 +181,29 @@ bool HttpServerProperties::SupportsRequestPriority(
   return false;
 }
 
-bool HttpServerProperties::GetSupportsSpdy(const url::SchemeHostPort& server) {
+bool HttpServerProperties::GetSupportsSpdy(
+    const url::SchemeHostPort& server,
+    const net::NetworkIsolationKey& network_isolation_key) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (server.host().empty())
     return false;
 
   auto spdy_info =
-      server_info_map_.Get(CreateServerInfoKey(server, NetworkIsolationKey()));
+      server_info_map_.Get(CreateServerInfoKey(server, network_isolation_key));
   return spdy_info != server_info_map_.end() &&
          spdy_info->second.supports_spdy.value_or(false);
 }
 
-void HttpServerProperties::SetSupportsSpdy(const url::SchemeHostPort& server,
+void HttpServerProperties::SetSupportsSpdy(
+    const url::SchemeHostPort& server,
+    const net::NetworkIsolationKey& network_isolation_key,
     bool supports_spdy) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (server.host().empty())
     return;
 
   auto server_info = server_info_map_.GetOrPut(
-      CreateServerInfoKey(server, NetworkIsolationKey()));
+      CreateServerInfoKey(server, network_isolation_key));
   // If value is already the same as |supports_spdy|, or value is unset and
   // |supports_spdy| is false, don't queue a write.
   bool queue_write =

net/http/http_server_properties.h

@@ -101,7 +101,8 @@ typedef base::MRUCache<quic::QuicServerId, std::string> QuicServerInfoMap;
 // * Alternative Service support;
 // * QUIC data (like ServerNetworkStats and QuicServerInfo).
 //
-// Optionally retrieves and saves properties from/to disk.
+// Optionally retrieves and saves properties from/to disk. This class is not
+// threadsafe.
 class NET_EXPORT HttpServerProperties
     : public BrokenAlternativeServices::Delegate {
  public:
@@ -218,19 +219,27 @@ class NET_EXPORT HttpServerProperties
   // disk.
   void Clear(base::OnceClosure callback);
 
-  // Returns true if |server| supports a network protocol which honors
-  // request prioritization.
+  // Returns true if |server|, in the context of |network_isolation_key|, has
+  // previously supported a network protocol which honors request
+  // prioritization.
+  //
   // Note that this also implies that the server supports request
   // multiplexing, since priorities imply a relationship between
   // multiple requests.
-  bool SupportsRequestPriority(const url::SchemeHostPort& server);
+  bool SupportsRequestPriority(
+      const url::SchemeHostPort& server,
+      const net::NetworkIsolationKey& network_isolation_key);
 
   // Returns the value set by SetSupportsSpdy(). If not set, returns false.
-  bool GetSupportsSpdy(const url::SchemeHostPort& server);
-
-  // Add |server| into the persistent store. Should only be called from IO
-  // thread.
-  void SetSupportsSpdy(const url::SchemeHostPort& server, bool supports_spdy);
+  bool GetSupportsSpdy(const url::SchemeHostPort& server,
+                       const net::NetworkIsolationKey& network_isolation_key);
+
+  // Records whether |server| supports H2 or not. Information is restricted to
+  // the context of |network_isolation_key|, to prevent cross-site information
+  // leakage.
+  void SetSupportsSpdy(const url::SchemeHostPort& server,
+                       const net::NetworkIsolationKey& network_isolation_key,
+                       bool supports_spdy);
 
   // Returns true if |server| has required HTTP/1.1 via HTTP/2 error code.
   bool RequiresHTTP11(const HostPortPair& server);

net/http/http_stream_factory.cc

@@ -20,6 +20,7 @@
 #include "base/trace_event/process_memory_dump.h"
 #include "net/base/host_mapping_rules.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/network_isolation_key.h"
 #include "net/base/parse_number.h"
 #include "net/base/port_util.h"
 #include "net/http/http_network_session.h"
@@ -306,8 +307,11 @@ bool HttpStreamFactory::ProxyServerSupportsPriorities(
   url::SchemeHostPort scheme_host_port("https", host_port_pair.host(),
                                        host_port_pair.port());
 
+  // TODO(https://crbug.com/993517): Figure out what NetworkIsolationKey() to
+  // use here, and what to do about this and |preconnecting_proxy_servers_|,
+  // which leaks data across NetworkIsolationKeys.
   return session_->http_server_properties()->SupportsRequestPriority(
-      scheme_host_port);
+      scheme_host_port, NetworkIsolationKey());
 }
 
 void HttpStreamFactory::DumpMemoryStats(

net/http/http_stream_factory_job.cc

@@ -250,7 +250,8 @@ int HttpStreamFactory::Job::Preconnect(int num_streams) {
       !http_server_properties->IsInitialized() &&
       request_info_.url.SchemeIsCryptographic();
   if (connect_one_stream || http_server_properties->SupportsRequestPriority(
-                                url::SchemeHostPort(request_info_.url))) {
+                                url::SchemeHostPort(request_info_.url),
+                                request_info_.network_isolation_key)) {
     num_streams_ = 1;
   } else {
     num_streams_ = num_streams;
@@ -1143,8 +1144,11 @@ int HttpStreamFactory::Job::DoCreateStream() {
 
   HttpServerProperties* http_server_properties =
       session_->http_server_properties();
-  if (http_server_properties)
-    http_server_properties->SetSupportsSpdy(scheme_host_port, true);
+  if (http_server_properties) {
+    http_server_properties->SetSupportsSpdy(scheme_host_port,
+                                            request_info_.network_isolation_key,
+                                            true /* supports_spdy */);
+  }
 
   // Create a SpdyHttpStream or a BidirectionalStreamImpl attached to the
   // session.
@@ -1336,7 +1340,8 @@ bool HttpStreamFactory::Job::ShouldThrottleConnectForSpdy() const {
       spdy_session_key_.host_port_pair().host(),
       spdy_session_key_.host_port_pair().port());
   // Only throttle the request if the server is believed to support H2.
-  return session_->http_server_properties()->GetSupportsSpdy(scheme_host_port);
+  return session_->http_server_properties()->GetSupportsSpdy(
+      scheme_host_port, request_info_.network_isolation_key);
 }
 
 }  // namespace net

net/http/http_stream_factory_unittest.cc

@@ -22,6 +22,7 @@
 #include "build/build_config.h"
 #include "net/base/completion_once_callback.h"
 #include "net/base/features.h"
+#include "net/base/network_isolation_key.h"
 #include "net/base/port_util.h"
 #include "net/base/privacy_mode.h"
 #include "net/base/proxy_server.h"
@@ -75,6 +76,7 @@
 #include "net/third_party/quiche/src/quic/test_tools/mock_random.h"
 #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "url/origin.h"
 
 // This file can be included from net/http even though
 // it is in net/websockets because it doesn't
@@ -1229,7 +1231,8 @@ TEST_F(HttpStreamFactoryTest, OnlyOnePreconnectToProxyServer) {
       HttpServerProperties http_server_properties;
       if (set_http_server_properties) {
         url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
-        http_server_properties.SetSupportsSpdy(spdy_server, true);
+        http_server_properties.SetSupportsSpdy(spdy_server,
+                                               NetworkIsolationKey(), true);
       }
 
       SpdySessionDependencies session_deps;
@@ -1313,7 +1316,8 @@ TEST_F(HttpStreamFactoryTest, ProxyServerPreconnectDifferentPrivacyModes) {
   HttpServerProperties http_server_properties;
 
   url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
-  http_server_properties.SetSupportsSpdy(spdy_server, true);
+  http_server_properties.SetSupportsSpdy(spdy_server, NetworkIsolationKey(),
+                                         true);
 
   SpdySessionDependencies session_deps;
   HttpNetworkSession::Params session_params =
@@ -1707,7 +1711,8 @@ TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxyWithPreconnects) {
   // Set up the proxy server as a server that supports request priorities.
   auto http_server_properties = std::make_unique<HttpServerProperties>();
   url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
-  http_server_properties->SetSupportsSpdy(spdy_server, true);
+  http_server_properties->SetSupportsSpdy(spdy_server, NetworkIsolationKey(),
+                                          true);
   session_deps.http_server_properties = std::move(http_server_properties);
 
   StaticSocketDataProvider socket_data;
@@ -1986,7 +1991,8 @@ TEST_F(HttpStreamFactoryTest, RequestSpdyHttpStreamHttpURL) {
 
   HttpServerProperties* http_server_properties =
       session->spdy_session_pool()->http_server_properties();
-  EXPECT_FALSE(http_server_properties->GetSupportsSpdy(scheme_host_port));
+  EXPECT_FALSE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                       NetworkIsolationKey()));
 
   // Now request a stream.
   HttpRequestInfo request_info;
@@ -2013,7 +2019,84 @@ TEST_F(HttpStreamFactoryTest, RequestSpdyHttpStreamHttpURL) {
       0, GetSocketPoolGroupCount(session->GetSocketPool(
              HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
   EXPECT_FALSE(waiter.used_proxy_info().is_direct());
-  EXPECT_TRUE(http_server_properties->GetSupportsSpdy(scheme_host_port));
+  EXPECT_TRUE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                      NetworkIsolationKey()));
+}
+
+// Same as above, but checks HttpServerProperties is updated using the correct
+// NetworkIsolationKey. When/if NetworkIsolationKey is enabled by default, this
+// should probably be merged into the above test.
+TEST_F(HttpStreamFactoryTest,
+       RequestSpdyHttpStreamHttpURLWithNetworkIsolationKey) {
+  const url::Origin kOrigin1 = url::Origin::Create(GURL("https://foo.test/"));
+  const NetworkIsolationKey kNetworkIsolationKey1(kOrigin1, kOrigin1);
+  const url::Origin kOrigin2 = url::Origin::Create(GURL("https://bar.test/"));
+  const NetworkIsolationKey kNetworkIsolationKey2(kOrigin2, kOrigin2);
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kPartitionHttpServerPropertiesByNetworkIsolationKey);
+
+  url::SchemeHostPort scheme_host_port("http", "myproxy.org", 443);
+  auto session_deps = std::make_unique<SpdySessionDependencies>(
+      ProxyResolutionService::CreateFixedFromPacResult(
+          "HTTPS myproxy.org:443", TRAFFIC_ANNOTATION_FOR_TESTS));
+  std::unique_ptr<ProxyResolutionService> proxy_resolution_service =
+      ProxyResolutionService::CreateFixedFromPacResult(
+          "HTTPS myproxy.org:443", TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  MockRead mock_read(SYNCHRONOUS, ERR_IO_PENDING);
+  SequencedSocketData socket_data(base::make_span(&mock_read, 1),
+                                  base::span<MockWrite>());
+  socket_data.set_connect_data(MockConnect(ASYNC, OK));
+  session_deps->socket_factory->AddSocketDataProvider(&socket_data);
+
+  SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
+  ssl_socket_data.next_proto = kProtoHTTP2;
+  session_deps->socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+  session_deps->proxy_resolution_service = std::move(proxy_resolution_service);
+
+  std::unique_ptr<HttpNetworkSession> session(
+      SpdySessionDependencies::SpdyCreateSession(session_deps.get()));
+
+  HttpServerProperties* http_server_properties =
+      session->spdy_session_pool()->http_server_properties();
+  EXPECT_FALSE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                       kNetworkIsolationKey1));
+
+  // Now request a stream.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://www.google.com");
+  request_info.load_flags = 0;
+  request_info.network_isolation_key = kNetworkIsolationKey1;
+  request_info.traffic_annotation =
+      MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  SSLConfig ssl_config;
+  StreamRequestWaiter waiter;
+  std::unique_ptr<HttpStreamRequest> request(
+      session->http_stream_factory()->RequestStream(
+          request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+          /* enable_ip_based_pooling = */ true,
+          /* enable_alternative_services = */ true, NetLogWithSource()));
+  waiter.WaitForStream();
+  EXPECT_TRUE(waiter.stream_done());
+  EXPECT_TRUE(nullptr == waiter.websocket_stream());
+  ASSERT_TRUE(nullptr != waiter.stream());
+
+  EXPECT_EQ(1, GetSpdySessionCount(session.get()));
+  EXPECT_EQ(
+      0, GetSocketPoolGroupCount(session->GetSocketPool(
+             HttpNetworkSession::NORMAL_SOCKET_POOL, ProxyServer::Direct())));
+  EXPECT_FALSE(waiter.used_proxy_info().is_direct());
+  EXPECT_TRUE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                      kNetworkIsolationKey1));
+  // Other NetworkIsolationKeys should not be recorded as supporting SPDY.
+  EXPECT_FALSE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                       NetworkIsolationKey()));
+  EXPECT_FALSE(http_server_properties->GetSupportsSpdy(scheme_host_port,
+                                                       kNetworkIsolationKey2));
 }
 
 // Tests that when a new SpdySession is established, duplicated idle H2 sockets

services/network/network_context_unittest.cc

@@ -840,14 +840,15 @@ TEST_F(NetworkContextTest, HttpServerPropertiesToDisk) {
 
   // Wait for properties to load from disk, and sanity check initial state.
   task_environment_.RunUntilIdle();
-  EXPECT_FALSE(network_context->url_request_context()
+  EXPECT_FALSE(
+      network_context->url_request_context()
           ->http_server_properties()
-                   ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
 
   // Set a property.
   network_context->url_request_context()
       ->http_server_properties()
-      ->SetSupportsSpdy(kSchemeHostPort, true);
+      ->SetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey(), true);
   // Deleting the context will cause it to flush state. Wait for the pref
   // service to flush to disk.
   network_context.reset();
@@ -861,9 +862,10 @@ TEST_F(NetworkContextTest, HttpServerPropertiesToDisk) {
   // Wait for properties to load from disk.
   task_environment_.RunUntilIdle();
 
-  EXPECT_TRUE(network_context->url_request_context()
+  EXPECT_TRUE(
+      network_context->url_request_context()
           ->http_server_properties()
-                  ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
 
   // Now check that ClearNetworkingHistorySince clears the data.
   base::RunLoop run_loop2;
@@ -871,9 +873,10 @@ TEST_F(NetworkContextTest, HttpServerPropertiesToDisk) {
       base::Time::Now() - base::TimeDelta::FromHours(1),
       run_loop2.QuitClosure());
   run_loop2.Run();
-  EXPECT_FALSE(network_context->url_request_context()
+  EXPECT_FALSE(
+      network_context->url_request_context()
           ->http_server_properties()
-                   ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
 
   // Destroy the network context and let any pending writes complete before
   // destroying |temp_dir|, to avoid leaking any files.
@@ -890,24 +893,27 @@ TEST_F(NetworkContextTest, ClearHttpServerPropertiesInMemory) {
   std::unique_ptr<NetworkContext> network_context =
       CreateContextWithParams(mojom::NetworkContextParams::New());
 
-  EXPECT_FALSE(network_context->url_request_context()
+  EXPECT_FALSE(
+      network_context->url_request_context()
           ->http_server_properties()
-                   ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
   network_context->url_request_context()
       ->http_server_properties()
-      ->SetSupportsSpdy(kSchemeHostPort, true);
-  EXPECT_TRUE(network_context->url_request_context()
+      ->SetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey(), true);
+  EXPECT_TRUE(
+      network_context->url_request_context()
           ->http_server_properties()
-                  ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
 
   base::RunLoop run_loop;
   network_context->ClearNetworkingHistorySince(
       base::Time::Now() - base::TimeDelta::FromHours(1),
       run_loop.QuitClosure());
   run_loop.Run();
-  EXPECT_FALSE(network_context->url_request_context()
+  EXPECT_FALSE(
+      network_context->url_request_context()
           ->http_server_properties()
-                   ->GetSupportsSpdy(kSchemeHostPort));
+          ->GetSupportsSpdy(kSchemeHostPort, net::NetworkIsolationKey()));
 }
 
 // Checks that ClearNetworkingHistorySince() clears network quality prefs.

services/network/resource_scheduler/resource_scheduler.cc

@@ -757,10 +757,13 @@ class ResourceScheduler::Client
         url::SchemeHostPort scheme_host_port(request->url_request()->url());
         net::HttpServerProperties& http_server_properties =
             *request->url_request()->context()->http_server_properties();
-        if (!http_server_properties.SupportsRequestPriority(scheme_host_port))
+        if (!http_server_properties.SupportsRequestPriority(
+                scheme_host_port,
+                request->url_request()->network_isolation_key())) {
           attributes |= kAttributeDelayable;
         }
       }
+    }
 
     return attributes;
   }
@@ -1059,9 +1062,11 @@ class ResourceScheduler::Client
         params_for_network_quality_.delay_requests_on_multiplexed_connections;
 
     url::SchemeHostPort scheme_host_port(url_request.url());
-    bool supports_priority = url_request.context()
+    bool supports_priority =
+        url_request.context()
             ->http_server_properties()
-                                 ->SupportsRequestPriority(scheme_host_port);
+            ->SupportsRequestPriority(scheme_host_port,
+                                      url_request.network_isolation_key());
 
     if (!priority_delayable) {
       // TODO(willchan): We should really improve this algorithm as described in