Add NetworkIsolationKey to NQE calls to HostResolver::CreateRequest.

Bug: 997049
Change-Id: I4cb68e2e9a64e84a8fe62fbc5333d0945cb400f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898614
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712837}

net/dns/mock_host_resolver.cc

@@ -630,7 +630,7 @@ int MockHostResolverBase::ResolveProc(
   if (cache_.get()) {
     HostCache::Key key(host.host(),
                        AddressFamilyToDnsQueryType(requested_address_family),
-                       flags, source);
+                       flags, source, network_isolation_key);
     // Storing a failure with TTL 0 so that it overwrites previous value.
     base::TimeDelta ttl;
     if (rv == OK) {

net/nqe/network_quality_estimator.cc

@@ -492,8 +492,8 @@ bool NetworkQualityEstimator::RequestProvidesRTTObservation(
     const URLRequest& request) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  bool private_network_request = nqe::internal::IsPrivateHost(
-      request.context()->host_resolver(), HostPortPair::FromURL(request.url()));
+  bool private_network_request =
+      nqe::internal::IsRequestForPrivateHost(request);
 
   return (use_localhost_requests_ || !private_network_request) &&
          // Verify that response headers are received, so it can be ensured that

net/nqe/network_quality_estimator_util.cc

@@ -15,21 +15,24 @@
 #include "net/dns/host_resolver.h"
 #include "net/dns/host_resolver_source.h"
 #include "net/log/net_log_with_source.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
 
 namespace net {
 
 namespace nqe {
 
-namespace internal {
+namespace {
 
 bool IsPrivateHost(HostResolver* host_resolver,
-                   const HostPortPair& host_port_pair) {
+                   const HostPortPair& host_port_pair,
+                   const NetworkIsolationKey& network_isolation_key) {
   // Try resolving |host_port_pair.host()| synchronously.
   HostResolver::ResolveHostParameters parameters;
   parameters.source = HostResolverSource::LOCAL_ONLY;
   std::unique_ptr<HostResolver::ResolveHostRequest> request =
-      host_resolver->CreateRequest(host_port_pair, NetLogWithSource(),
-                                   parameters);
+      host_resolver->CreateRequest(host_port_pair, network_isolation_key,
+                                   NetLogWithSource(), parameters);
 
   int rv = request->Start(base::BindOnce([](int error) { NOTREACHED(); }));
   DCHECK_NE(rv, ERR_IO_PENDING);
@@ -46,6 +49,24 @@ bool IsPrivateHost(HostResolver* host_resolver,
   return false;
 }
 
+}  // namespace
+
+namespace internal {
+
+bool IsRequestForPrivateHost(const URLRequest& request) {
+  // Using the request's NetworkIsolationKey isn't necessary for privacy
+  // reasons, but is needed to maximize the chances of a cache hit.
+  return IsPrivateHost(request.context()->host_resolver(),
+                       HostPortPair::FromURL(request.url()),
+                       request.network_isolation_key());
+}
+
+bool IsPrivateHostForTesting(HostResolver* host_resolver,
+                             const HostPortPair& host_port_pair,
+                             const NetworkIsolationKey& network_isolation_key) {
+  return IsPrivateHost(host_resolver, host_port_pair, network_isolation_key);
+}
+
 }  // namespace internal
 
 }  // namespace nqe

net/nqe/network_quality_estimator_util.h

@@ -13,6 +13,8 @@ namespace net {
 
 class HostPortPair;
 class HostResolver;
+class NetworkIsolationKey;
+class URLRequest;
 
 namespace nqe {
 
@@ -21,18 +23,23 @@ namespace internal {
 // A unified compact representation of an IPv6 or an IPv4 address.
 typedef uint64_t IPHash;
 
-// Returns true if the host contained in |host_port_pair| is a host in a
-// private Internet as defined by RFC 1918 or if the requests to
-// |host_port_pair| are not expected to generate useful network quality
-// information. This includes localhost, hosts on private subnets, and
-// hosts on subnets that are reserved for specific usage, and are unlikely
-// to be used by public web servers.
-// To make this determination, IsPrivateHost() makes the best
-// effort estimate including trying to resolve the host in the
-// |host_port_pair|. The method is synchronous.
-// |host_resolver| must not be null.
-NET_EXPORT_PRIVATE bool IsPrivateHost(HostResolver* host_resolver,
-                                      const HostPortPair& host_port_pair);
+// Returns true if the host contained of |request.url()| is a host in a
+// private Internet as defined by RFC 1918 or if the requests to it are not
+// expected to generate useful network quality information. This includes
+// localhost, hosts on private subnets, and hosts on subnets that are reserved
+// for specific usage, and are unlikely to be used by public web servers.
+//
+// To make this determination, this method makes the best effort estimate
+// including trying to resolve the host from the HostResolver's cache. This
+// method is synchronous.
+NET_EXPORT_PRIVATE bool IsRequestForPrivateHost(const URLRequest& request);
+
+// Provides access to the method used internally by IsRequestForPrivateHost(),
+// for testing.
+NET_EXPORT_PRIVATE bool IsPrivateHostForTesting(
+    HostResolver* host_resolver,
+    const HostPortPair& host_port_pair,
+    const NetworkIsolationKey& network_isolation_key);
 
 }  // namespace internal
 

net/nqe/network_quality_estimator_util_unittest.cc

@@ -7,7 +7,9 @@
 #include <memory>
 
 #include "base/optional.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "net/base/features.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_isolation_key.h"
@@ -18,6 +20,7 @@
 #include "net/log/test_net_log.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace net {
 
@@ -59,26 +62,31 @@ TEST(NetworkQualityEstimatorUtilTest, ReservedHost) {
 
   EXPECT_EQ(2u, mock_host_resolver.num_non_local_resolves());
 
-  EXPECT_FALSE(IsPrivateHost(&mock_host_resolver,
-                             HostPortPair("2607:f8b0:4006:819::200e", 80)));
+  EXPECT_FALSE(IsPrivateHostForTesting(
+      &mock_host_resolver, HostPortPair("2607:f8b0:4006:819::200e", 80),
+      NetworkIsolationKey()));
 
-  EXPECT_TRUE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("192.168.0.1", 443)));
+  EXPECT_TRUE(IsPrivateHostForTesting(&mock_host_resolver,
+                                      HostPortPair("192.168.0.1", 443),
+                                      NetworkIsolationKey()));
 
-  EXPECT_FALSE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("92.168.0.1", 443)));
+  EXPECT_FALSE(IsPrivateHostForTesting(&mock_host_resolver,
+                                       HostPortPair("92.168.0.1", 443),
+                                       NetworkIsolationKey()));
 
-  EXPECT_TRUE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("example1.com", 443)));
+  EXPECT_TRUE(IsPrivateHostForTesting(&mock_host_resolver,
+                                      HostPortPair("example1.com", 443),
+                                      NetworkIsolationKey()));
 
-  EXPECT_FALSE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("example2.com", 443)));
+  EXPECT_FALSE(IsPrivateHostForTesting(&mock_host_resolver,
+                                       HostPortPair("example2.com", 443),
+                                       NetworkIsolationKey()));
 
-  // IsPrivateHost() should have queried only the resolver's cache.
+  // IsPrivateHostForTesting() should have queried only the resolver's cache.
   EXPECT_EQ(2u, mock_host_resolver.num_non_local_resolves());
 }
 
-// Verify that IsPrivateHost() returns false for a hostname whose DNS
+// Verify that IsPrivateHostForTesting() returns false for a hostname whose DNS
 // resolution is not cached. Further, once the resolution is cached, verify that
 // the cached entry is used.
 TEST(NetworkQualityEstimatorUtilTest, ReservedHostUncached) {
@@ -96,8 +104,9 @@ TEST(NetworkQualityEstimatorUtilTest, ReservedHostUncached) {
   mock_host_resolver.set_rules(rules.get());
 
   // Not in DNS host cache, so should not be marked as private.
-  EXPECT_FALSE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("example3.com", 443)));
+  EXPECT_FALSE(IsPrivateHostForTesting(&mock_host_resolver,
+                                       HostPortPair("example3.com", 443),
+                                       NetworkIsolationKey()));
   EXPECT_EQ(0u, mock_host_resolver.num_non_local_resolves());
 
   int rv = mock_host_resolver.LoadIntoCache(
@@ -105,14 +114,65 @@ TEST(NetworkQualityEstimatorUtilTest, ReservedHostUncached) {
   EXPECT_EQ(OK, rv);
   EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
 
-  EXPECT_TRUE(
-      IsPrivateHost(&mock_host_resolver, HostPortPair("example3.com", 443)));
+  EXPECT_TRUE(IsPrivateHostForTesting(&mock_host_resolver,
+                                      HostPortPair("example3.com", 443),
+                                      NetworkIsolationKey()));
 
-  // IsPrivateHost() should have queried only the resolver's cache.
+  // IsPrivateHostForTesting() should have queried only the resolver's cache.
   EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
 }
 
-// Verify that IsPrivateHost() returns correct results for local hosts.
+// Make sure that IsPrivateHostForTesting() uses the NetworkIsolationKey
+// provided to it.
+TEST(NetworkQualityEstimatorUtilTest,
+     ReservedHostUncachedWithNetworkIsolationKey) {
+  const url::Origin kOrigin = url::Origin::Create(GURL("https://foo.test/"));
+  const net::NetworkIsolationKey kNetworkIsolationKey(kOrigin, kOrigin);
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kSplitHostCacheByNetworkIsolationKey);
+
+  base::test::TaskEnvironment task_environment;
+
+  std::unique_ptr<BoundTestNetLog> net_log =
+      std::make_unique<BoundTestNetLog>();
+  MockCachingHostResolver mock_host_resolver;
+
+  scoped_refptr<net::RuleBasedHostResolverProc> rules(
+      new net::RuleBasedHostResolverProc(nullptr));
+
+  // Add example3.com resolution to the DNS cache.
+  rules->AddRule("example3.com", "127.0.0.3");
+  mock_host_resolver.set_rules(rules.get());
+
+  // Not in DNS host cache, so should not be marked as private.
+  EXPECT_FALSE(IsPrivateHostForTesting(&mock_host_resolver,
+                                       HostPortPair("example3.com", 443),
+                                       kNetworkIsolationKey));
+  EXPECT_EQ(0u, mock_host_resolver.num_non_local_resolves());
+
+  int rv = mock_host_resolver.LoadIntoCache(
+      HostPortPair("example3.com", 443), kNetworkIsolationKey, base::nullopt);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
+
+  EXPECT_TRUE(IsPrivateHostForTesting(&mock_host_resolver,
+                                      HostPortPair("example3.com", 443),
+                                      kNetworkIsolationKey));
+
+  // IsPrivateHostForTesting() should have queried only the resolver's cache.
+  EXPECT_EQ(1u, mock_host_resolver.num_non_local_resolves());
+
+  // IsPrivateHostForTesting should return false when using a different
+  // NetworkIsolationKey (in this case, any empty one).
+  EXPECT_FALSE(IsPrivateHostForTesting(&mock_host_resolver,
+                                       HostPortPair("example3.com", 443),
+                                       NetworkIsolationKey()));
+}
+
+// Verify that IsPrivateHostForTesting() returns correct results for local
+// hosts.
 TEST(NetworkQualityEstimatorUtilTest, Localhost) {
   base::test::TaskEnvironment task_environment;
 
@@ -129,12 +189,18 @@ TEST(NetworkQualityEstimatorUtilTest, Localhost) {
   scoped_refptr<net::RuleBasedHostResolverProc> rules(
       new net::RuleBasedHostResolverProc(nullptr));
 
-  EXPECT_TRUE(IsPrivateHost(resolver.get(), HostPortPair("localhost", 443)));
-  EXPECT_TRUE(IsPrivateHost(resolver.get(), HostPortPair("localhost6", 443)));
-  EXPECT_TRUE(IsPrivateHost(resolver.get(), HostPortPair("127.0.0.1", 80)));
-  EXPECT_TRUE(IsPrivateHost(resolver.get(), HostPortPair("0.0.0.0", 80)));
-  EXPECT_TRUE(IsPrivateHost(resolver.get(), HostPortPair("::1", 80)));
-  EXPECT_FALSE(IsPrivateHost(resolver.get(), HostPortPair("google.com", 80)));
+  EXPECT_TRUE(IsPrivateHostForTesting(
+      resolver.get(), HostPortPair("localhost", 443), NetworkIsolationKey()));
+  EXPECT_TRUE(IsPrivateHostForTesting(
+      resolver.get(), HostPortPair("localhost6", 443), NetworkIsolationKey()));
+  EXPECT_TRUE(IsPrivateHostForTesting(
+      resolver.get(), HostPortPair("127.0.0.1", 80), NetworkIsolationKey()));
+  EXPECT_TRUE(IsPrivateHostForTesting(
+      resolver.get(), HostPortPair("0.0.0.0", 80), NetworkIsolationKey()));
+  EXPECT_TRUE(IsPrivateHostForTesting(resolver.get(), HostPortPair("::1", 80),
+                                      NetworkIsolationKey()));
+  EXPECT_FALSE(IsPrivateHostForTesting(
+      resolver.get(), HostPortPair("google.com", 80), NetworkIsolationKey()));
 }
 
 }  // namespace

net/nqe/throughput_analyzer.cc

@@ -404,8 +404,8 @@ int64_t ThroughputAnalyzer::CountTotalContentSizeBytes() const {
 bool ThroughputAnalyzer::DegradesAccuracy(const URLRequest& request) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  bool private_network_request = nqe::internal::IsPrivateHost(
-      request.context()->host_resolver(), HostPortPair::FromURL(request.url()));
+  bool private_network_request =
+      nqe::internal::IsRequestForPrivateHost(request);
 
   return !(use_localhost_requests_for_tests_ || !private_network_request) ||
          request.creation_time() < last_connection_change_;

net/nqe/throughput_analyzer_unittest.cc

@@ -22,11 +22,14 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_tick_clock.h"
+#include "net/base/features.h"
+#include "net/base/network_isolation_key.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/log/test_net_log.h"
 #include "net/nqe/network_quality_estimator.h"
@@ -38,6 +41,8 @@
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+#include "url/origin.h"
 
 namespace net {
 
@@ -81,11 +86,17 @@ class TestThroughputAnalyzer : public internal::ThroughputAnalyzer {
   // Uses a mock resolver to force example.com to resolve to a public IP
   // address.
   void AddIPAddressResolution(TestURLRequestContext* context) {
-    scoped_refptr<net::RuleBasedHostResolverProc> rules(
-        new net::RuleBasedHostResolverProc(nullptr));
-    // example1.com resolves to a public IP address.
+    scoped_refptr<net::RuleBasedHostResolverProc> rules =
+        base::MakeRefCounted<RuleBasedHostResolverProc>(nullptr);
+    // example.com resolves to a public IP address.
     rules->AddRule("example.com", "27.0.0.3");
+    // local.com resolves to a private IP address.
+    rules->AddRule("local.com", "127.0.0.1");
     mock_host_resolver_.set_rules(rules.get());
+    mock_host_resolver_.LoadIntoCache(HostPortPair("example.com", 80),
+                                      NetworkIsolationKey(), base::nullopt);
+    mock_host_resolver_.LoadIntoCache(HostPortPair("local.com", 80),
+                                      NetworkIsolationKey(), base::nullopt);
     context->set_host_resolver(&mock_host_resolver_);
   }
 
@@ -108,16 +119,16 @@ class TestThroughputAnalyzer : public internal::ThroughputAnalyzer {
 using ThroughputAnalyzerTest = TestWithTaskEnvironment;
 
 TEST_F(ThroughputAnalyzerTest, MaximumRequests) {
-  const struct {
-    bool use_local_requests;
-  } tests[] = {{
-                   false,
-               },
-               {
-                   true,
-               }};
+  const struct TestCase {
+    GURL url;
+    bool is_local;
+  } kTestCases[] = {
+      {GURL("http://127.0.0.1/test.html"), true /* is_local */},
+      {GURL("http://example.com/test.html"), false /* is_local */},
+      {GURL("http://local.com/test.html"), true /* is_local */},
+  };
 
-  for (const auto& test : tests) {
+  for (const auto& test_case : kTestCases) {
     const base::TickClock* tick_clock = base::DefaultTickClock::GetInstance();
     TestNetworkQualityEstimator network_quality_estimator;
     std::map<std::string, std::string> variation_params;
@@ -135,25 +146,88 @@ TEST_F(ThroughputAnalyzerTest, MaximumRequests) {
 
     // Start more requests than the maximum number of requests that can be held
     // in the memory.
-    const std::string url = test.use_local_requests
-                                ? "http://127.0.0.1/test.html"
-                                : "http://example.com/test.html";
-
-    EXPECT_EQ(
-        test.use_local_requests,
-        nqe::internal::IsPrivateHost(
-            context.host_resolver(),
-            HostPortPair(GURL(url).host(), GURL(url).EffectiveIntPort())));
+    EXPECT_EQ(test_case.is_local,
+              nqe::internal::IsPrivateHostForTesting(
+                  context.host_resolver(), HostPortPair::FromURL(test_case.url),
+                  NetworkIsolationKey()));
+    for (size_t i = 0; i < 1000; ++i) {
+      std::unique_ptr<URLRequest> request(
+          context.CreateRequest(test_case.url, DEFAULT_PRIORITY, &test_delegate,
+                                TRAFFIC_ANNOTATION_FOR_TESTS));
+      throughput_analyzer.NotifyStartTransaction(*(request.get()));
+      requests.push_back(std::move(request));
+    }
+    // Too many local requests should cause the |throughput_analyzer| to disable
+    // throughput measurements.
+    EXPECT_NE(test_case.is_local,
+              throughput_analyzer.IsCurrentlyTrackingThroughput());
+  }
+}
+
+// Make sure that the NetworkIsolationKey is respected when resolving a host
+// from the cache.
+TEST_F(ThroughputAnalyzerTest, MaximumRequestsWithNetworkIsolationKey) {
+  const url::Origin kOrigin = url::Origin::Create(GURL("https://foo.test/"));
+  const net::NetworkIsolationKey kNetworkIsolationKey(kOrigin, kOrigin);
+  const GURL kUrl = GURL("http://foo.test/test.html");
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kSplitHostCacheByNetworkIsolationKey);
+
+  for (bool use_network_isolation_key : {false, true}) {
+    const base::TickClock* tick_clock = base::DefaultTickClock::GetInstance();
+    TestNetworkQualityEstimator network_quality_estimator;
+    std::map<std::string, std::string> variation_params;
+    NetworkQualityEstimatorParams params(variation_params);
+    TestThroughputAnalyzer throughput_analyzer(&network_quality_estimator,
+                                               &params, tick_clock);
+
+    TestDelegate test_delegate;
+    TestURLRequestContext context;
+    MockCachingHostResolver mock_host_resolver;
+    context.set_host_resolver(&mock_host_resolver);
+
+    // Add an entry to the host cache mapping kUrl to non-local IP when using an
+    // empty NetworkIsolationKey.
+    scoped_refptr<net::RuleBasedHostResolverProc> rules =
+        base::MakeRefCounted<RuleBasedHostResolverProc>(nullptr);
+    rules->AddRule(kUrl.host(), "1.2.3.4");
+    mock_host_resolver.set_rules(rules.get());
+    mock_host_resolver.LoadIntoCache(HostPortPair::FromURL(kUrl),
+                                     NetworkIsolationKey(), base::nullopt);
+
+    // Add an entry to the host cache mapping kUrl to local IP when using
+    // kNetworkIsolationKey.
+    rules = base::MakeRefCounted<RuleBasedHostResolverProc>(nullptr);
+    rules->AddRule(kUrl.host(), "127.0.0.1");
+    mock_host_resolver.set_rules(rules.get());
+    mock_host_resolver.LoadIntoCache(HostPortPair::FromURL(kUrl),
+                                     kNetworkIsolationKey, base::nullopt);
+
+    ASSERT_FALSE(
+        throughput_analyzer.disable_throughput_measurements_for_testing());
+    base::circular_deque<std::unique_ptr<URLRequest>> requests;
+
+    // Start more requests than the maximum number of requests that can be held
+    // in the memory.
+    EXPECT_EQ(use_network_isolation_key,
+              nqe::internal::IsPrivateHostForTesting(
+                  context.host_resolver(), HostPortPair::FromURL(kUrl),
+                  use_network_isolation_key ? kNetworkIsolationKey
+                                            : NetworkIsolationKey()));
     for (size_t i = 0; i < 1000; ++i) {
       std::unique_ptr<URLRequest> request(
-          context.CreateRequest(GURL(url), DEFAULT_PRIORITY, &test_delegate,
+          context.CreateRequest(kUrl, DEFAULT_PRIORITY, &test_delegate,
                                 TRAFFIC_ANNOTATION_FOR_TESTS));
+      if (use_network_isolation_key)
+        request->set_network_isolation_key(kNetworkIsolationKey);
       throughput_analyzer.NotifyStartTransaction(*(request.get()));
       requests.push_back(std::move(request));
     }
     // Too many local requests should cause the |throughput_analyzer| to disable
     // throughput measurements.
-    EXPECT_NE(test.use_local_requests,
+    EXPECT_NE(use_network_isolation_key,
               throughput_analyzer.IsCurrentlyTrackingThroughput());
   }
 }