Distinguish between enabling the insecure and secure parts of the async resolver.

In SECURE or AUTOMATIC mode, we should be able to send secure DoH queries via
the async resolver regardless of whether the async resolver is enabled for
sending insecure queries.

The network service will be updated in a follow-up cl to actually set the
insecure and secure parts separately; for now support is just being added in
HostResolverManager.

Bug: 985589
Change-Id: Ife79f70897229aadc4f7111c3ce07de47b47a32b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1497654
Commit-Queue: Katharine Daly <dalyk@google.com>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680069}

components/cronet/stale_host_resolver_unittest.cc

@@ -185,8 +185,10 @@ class StaleHostResolverTest : public testing::Test {
     if (dns_client) {
       inner_resolver->GetManagerForTesting()->SetDnsClientForTesting(
           std::move(dns_client));
+      inner_resolver->GetManagerForTesting()->SetInsecureDnsClientEnabled(true);
     } else {
-      inner_resolver->GetManagerForTesting()->SetDnsClientEnabled(false);
+      inner_resolver->GetManagerForTesting()->SetInsecureDnsClientEnabled(
+          false);
     }
     return inner_resolver;
   }

components/cronet/url_request_context_config.cc

@@ -679,7 +679,8 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
     CHECK(net_log) << "All DNS-related experiments require NetLog.";
     std::unique_ptr<net::HostResolver> host_resolver;
     net::HostResolver::ManagerOptions host_resolver_manager_options;
-    host_resolver_manager_options.dns_client_enabled = async_dns_enable;
+    host_resolver_manager_options.insecure_dns_client_enabled =
+        async_dns_enable;
     host_resolver_manager_options.check_ipv6_on_wifi = !disable_ipv6_on_wifi;
     // TODO(crbug.com/934402): Consider using a shared HostResolverManager for
     // Cronet HostResolvers.

net/dns/context_host_resolver_unittest.cc

@@ -51,6 +51,7 @@ class ContextHostResolverTest : public TestWithScopedTaskEnvironment {
         std::make_unique<MockDnsClient>(DnsConfig(), std::move(rules));
     dns_client_ = dns_client.get();
     manager_->SetDnsClientForTesting(std::move(dns_client));
+    manager_->SetInsecureDnsClientEnabled(true);
 
     scoped_refptr<HostResolverProc> proc = CreateCatchAllHostResolverProc();
     manager_->set_proc_params_for_test(ProcTaskParams(proc.get(), 1u));

net/dns/fuzzed_host_resolver_util.cc

@@ -85,6 +85,62 @@ IPAddress FuzzIPAddress(FuzzedDataProvider* data_provider) {
   return FuzzIPv6Address(data_provider);
 }
 
+DnsConfig GetFuzzedDnsConfig(FuzzedDataProvider* data_provider) {
+  // Fuzz DNS configuration.
+  DnsConfig config;
+
+  // Fuzz name servers.
+  uint32_t num_nameservers = data_provider->ConsumeIntegralInRange(0, 4);
+  for (uint32_t i = 0; i < num_nameservers; ++i) {
+    config.nameservers.push_back(
+        IPEndPoint(FuzzIPAddress(data_provider), FuzzPort(data_provider)));
+  }
+
+  // Fuzz suffix search list.
+  switch (data_provider->ConsumeIntegralInRange(0, 3)) {
+    case 3:
+      config.search.push_back("foo.com");
+      FALLTHROUGH;
+    case 2:
+      config.search.push_back("bar");
+      FALLTHROUGH;
+    case 1:
+      config.search.push_back("com");
+      FALLTHROUGH;
+    default:
+      break;
+  }
+
+  net::DnsHosts hosts;
+  // Fuzz hosts file.
+  uint8_t num_hosts_entries = data_provider->ConsumeIntegral<uint8_t>();
+  for (uint8_t i = 0; i < num_hosts_entries; ++i) {
+    const char* kHostnames[] = {"foo", "foo.com",   "a.foo.com",
+                                "bar", "localhost", "localhost6"};
+    const char* hostname = data_provider->PickValueInArray(kHostnames);
+    net::IPAddress address = FuzzIPAddress(data_provider);
+    config.hosts[net::DnsHostsKey(hostname, net::GetAddressFamily(address))] =
+        address;
+  }
+
+  config.unhandled_options = data_provider->ConsumeBool();
+  config.append_to_multi_label_name = data_provider->ConsumeBool();
+  config.randomize_ports = data_provider->ConsumeBool();
+  config.ndots = data_provider->ConsumeIntegralInRange(0, 3);
+  config.attempts = data_provider->ConsumeIntegralInRange(1, 3);
+
+  // Timeouts don't really work for fuzzing. Even a timeout of 0 milliseconds
+  // will be increased after the first timeout, resulting in inconsistent
+  // behavior.
+  config.timeout = base::TimeDelta::FromDays(10);
+
+  config.rotate = data_provider->ConsumeBool();
+
+  config.use_local_ipv6 = data_provider->ConsumeBool();
+
+  return config;
+}
+
 // HostResolverProc that returns a random set of results, and can succeed or
 // fail. Must only be run on the thread it's created on.
 class FuzzedHostResolverProc : public HostResolverProc {
@@ -311,9 +367,6 @@ class FuzzedHostResolverManager : public HostResolverManager {
         socket_factory_(data_provider_),
         net_log_(net_log),
         data_provider_weak_factory_(data_provider) {
-    // Use SetDnsClientEnabled() to ensure fuzzed client is used.
-    DCHECK(!options.dns_client_enabled);
-
     ProcTaskParams proc_task_params(
         new FuzzedHostResolverProc(data_provider_weak_factory_.GetWeakPtr()),
         // Retries are only used when the original request hangs, which this
@@ -323,16 +376,20 @@ class FuzzedHostResolverManager : public HostResolverManager {
     SetTaskRunnerForTesting(base::SequencedTaskRunnerHandle::Get());
     SetMdnsSocketFactoryForTesting(
         std::make_unique<FuzzedMdnsSocketFactory>(data_provider_));
+    std::unique_ptr<DnsClient> dns_client = DnsClient::CreateClientForTesting(
+        net_log_, &socket_factory_,
+        base::Bind(&FuzzedDataProvider::ConsumeIntegralInRange<int32_t>,
+                   base::Unretained(data_provider_)));
+    dns_client->SetConfig(GetFuzzedDnsConfig(data_provider_));
+    HostResolverManager::SetDnsClientForTesting(std::move(dns_client));
   }
 
   ~FuzzedHostResolverManager() override = default;
 
-  // Enable / disable the async resolver. When enabled, installs a
-  // DnsClient with fuzzed UDP and TCP sockets.
-  void SetDnsClientEnabled(bool enabled) override;
-
   void SetDnsClientForTesting(std::unique_ptr<DnsClient> dns_client) {
-    // Should only call SetDnsClientEnabled() to ensure a fuzzed client is used.
+    // The only DnsClient that is supported is the one created by the
+    // FuzzedHostResolverManager since that DnsClient contains the necessary
+    // fuzzing logic.
     NOTREACHED();
   }
 
@@ -362,73 +419,6 @@ class FuzzedHostResolverManager : public HostResolverManager {
   DISALLOW_COPY_AND_ASSIGN(FuzzedHostResolverManager);
 };
 
-void FuzzedHostResolverManager::SetDnsClientEnabled(bool enabled) {
-  if (!enabled) {
-    HostResolverManager::SetDnsClientEnabled(false);
-    return;
-  }
-
-  // Fuzz DNS configuration.
-
-  DnsConfig config;
-
-  // Fuzz name servers.
-  uint32_t num_nameservers = data_provider_->ConsumeIntegralInRange(0, 4);
-  for (uint32_t i = 0; i < num_nameservers; ++i) {
-    config.nameservers.push_back(
-        IPEndPoint(FuzzIPAddress(data_provider_), FuzzPort(data_provider_)));
-  }
-
-  // Fuzz suffix search list.
-  switch (data_provider_->ConsumeIntegralInRange(0, 3)) {
-    case 3:
-      config.search.push_back("foo.com");
-      FALLTHROUGH;
-    case 2:
-      config.search.push_back("bar");
-      FALLTHROUGH;
-    case 1:
-      config.search.push_back("com");
-      FALLTHROUGH;
-    default:
-      break;
-  }
-
-  net::DnsHosts hosts;
-  // Fuzz hosts file.
-  uint8_t num_hosts_entries = data_provider_->ConsumeIntegral<uint8_t>();
-  for (uint8_t i = 0; i < num_hosts_entries; ++i) {
-    const char* kHostnames[] = {"foo", "foo.com",   "a.foo.com",
-                                "bar", "localhost", "localhost6"};
-    const char* hostname = data_provider_->PickValueInArray(kHostnames);
-    net::IPAddress address = FuzzIPAddress(data_provider_);
-    config.hosts[net::DnsHostsKey(hostname, net::GetAddressFamily(address))] =
-        address;
-  }
-
-  config.unhandled_options = data_provider_->ConsumeBool();
-  config.append_to_multi_label_name = data_provider_->ConsumeBool();
-  config.randomize_ports = data_provider_->ConsumeBool();
-  config.ndots = data_provider_->ConsumeIntegralInRange(0, 3);
-  config.attempts = data_provider_->ConsumeIntegralInRange(1, 3);
-
-  // Timeouts don't really work for fuzzing. Even a timeout of 0 milliseconds
-  // will be increased after the first timeout, resulting in inconsistent
-  // behavior.
-  config.timeout = base::TimeDelta::FromDays(10);
-
-  config.rotate = data_provider_->ConsumeBool();
-
-  config.use_local_ipv6 = data_provider_->ConsumeBool();
-
-  std::unique_ptr<DnsClient> dns_client = DnsClient::CreateClientForTesting(
-      net_log_, &socket_factory_,
-      base::Bind(&FuzzedDataProvider::ConsumeIntegralInRange<int32_t>,
-                 base::Unretained(data_provider_)));
-  dns_client->SetConfig(config);
-  HostResolverManager::SetDnsClientForTesting(std::move(dns_client));
-}
-
 }  // namespace
 
 std::unique_ptr<ContextHostResolver> CreateFuzzedContextHostResolver(
@@ -436,16 +426,8 @@ std::unique_ptr<ContextHostResolver> CreateFuzzedContextHostResolver(
     NetLog* net_log,
     FuzzedDataProvider* data_provider,
     bool enable_caching) {
-  // FuzzedHostResolverManager only handles fuzzing DnsClient when enabled
-  // through SetDnsClientEnabled().
-  bool enable_dns_client = options.dns_client_enabled;
-  HostResolver::ManagerOptions filtered_options(options);
-  filtered_options.dns_client_enabled = false;
-
-  auto manager = std::make_unique<FuzzedHostResolverManager>(
-      filtered_options, net_log, data_provider);
-  manager->SetDnsClientEnabled(enable_dns_client);
-
+  auto manager = std::make_unique<FuzzedHostResolverManager>(options, net_log,
+                                                             data_provider);
   return std::make_unique<ContextHostResolver>(
       std::move(manager),
       enable_caching ? HostCache::CreateDefaultCache() : nullptr);

net/dns/fuzzed_host_resolver_util.h

@@ -20,9 +20,11 @@ class NetLog;
 // return. It inherits from ContextHostResolver, unlike MockHostResolver, so
 // more closely matches real behavior.
 //
-// By default uses a mocked out system resolver, though can be configured (using
-// SetDnsClientEnabled() on the underlying manager) to use the built-in async
-// resolver (Built in DNS stub resolver) with a fuzzed set of UDP/TCP sockets.
+// By default uses a mocked out system resolver, though can be configured to use
+// the built-in async resolver (Built in DNS stub resolver) with a fuzzed set
+// of UDP/TCP sockets by setting ManagerOptions.insecure_dns_client_enabled to
+// true or calling SetInsecureDnsClientEnabled on the underlying
+// HostResolverManager.
 //
 // To make behavior most deterministic, does not use the WorkerPool to run its
 // simulated platform host resolver calls, instead runs them on the thread it is

net/dns/host_resolver.h

@@ -69,6 +69,9 @@ class NET_EXPORT HostResolver {
     // an incompatible IP literal (e.g. IPv6 is disabled and it is an IPv6
     // literal).
     //
+    // Results in ERR_DNS_CACHE_MISS if only fast local sources are to be
+    // queried and a cache lookup attempt fails.
+    //
     // The parent HostResolver must still be alive when Start() is called,  but
     // if it is destroyed before an asynchronous result completes, the request
     // will be automatically cancelled.
@@ -131,10 +134,10 @@ class NET_EXPORT HostResolver {
     // |kDefaultRetryAttempts| for the resolver to choose a default value.
     size_t max_system_retry_attempts = kDefaultRetryAttempts;
 
-    // Initial setting for whether the built-in asynchronous DnsClient is
-    // enabled or disabled. See HostResolverManager::SetDnsClientEnabled() for
-    // details.
-    bool dns_client_enabled = false;
+    // Initial setting for whether the insecure portion of the built-in
+    // asynchronous DnsClient is enabled or disabled. See HostResolverManager::
+    // SetInsecureDnsClientEnabled() for details.
+    bool insecure_dns_client_enabled = false;
 
     // Initial configuration overrides for the built-in asynchronous DnsClient.
     // See HostResolverManager::SetDnsConfigOverrides() for details.

net/dns/host_resolver_manager_fuzzer.cc

@@ -205,7 +205,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
     net::HostResolver::ManagerOptions options;
     options.max_concurrent_resolves =
         data_provider.ConsumeIntegralInRange(1, 8);
-    options.dns_client_enabled = data_provider.ConsumeBool();
+    options.insecure_dns_client_enabled = data_provider.ConsumeBool();
     bool enable_caching = data_provider.ConsumeBool();
     std::unique_ptr<net::ContextHostResolver> host_resolver =
         net::CreateFuzzedContextHostResolver(options, &net_log, &data_provider,

services/network/host_resolver_unittest.cc

@@ -1177,6 +1177,7 @@ TEST_F(HostResolverTest, TextResults) {
       net::HostResolver::CreateStandaloneContextResolver(&net_log);
   inner_resolver->GetManagerForTesting()->SetDnsClientForTesting(
       std::move(dns_client));
+  inner_resolver->GetManagerForTesting()->SetInsecureDnsClientEnabled(true);
   inner_resolver->SetBaseDnsConfigForTesting(CreateValidDnsConfig());
 
   HostResolver resolver(inner_resolver.get(), &net_log);
@@ -1216,6 +1217,7 @@ TEST_F(HostResolverTest, HostResults) {
       net::HostResolver::CreateStandaloneContextResolver(&net_log);
   inner_resolver->GetManagerForTesting()->SetDnsClientForTesting(
       std::move(dns_client));
+  inner_resolver->GetManagerForTesting()->SetInsecureDnsClientEnabled(true);
   inner_resolver->SetBaseDnsConfigForTesting(CreateValidDnsConfig());
 
   HostResolver resolver(inner_resolver.get(), &net_log);

services/network/network_context.cc

@@ -1324,7 +1324,7 @@ void NetworkContext::CreateHostResolver(
     // different overrides.  But since this is only used for special cases for
     // now, much easier to create entirely separate net::HostResolver instances.
     net::HostResolver::ManagerOptions options;
-    options.dns_client_enabled = true;
+    options.insecure_dns_client_enabled = true;
     options.dns_config_overrides = config_overrides.value();
     private_internal_resolver =
         network_service_->host_resolver_factory()->CreateStandaloneResolver(

services/network/network_service.cc

@@ -456,9 +456,9 @@ void NetworkService::ConfigureStubHostResolver(
   DCHECK(stub_resolver_enabled || !dns_over_https_servers);
   DCHECK(!dns_over_https_servers || !dns_over_https_servers->empty());
 
-  // Enable or disable the stub resolver, as needed. "DnsClient" is class that
-  // implements the stub resolver.
-  host_resolver_manager_->SetDnsClientEnabled(stub_resolver_enabled);
+  // Enable or disable the insecure part of DnsClient. "DnsClient" is the class
+  // that implements the stub resolver.
+  host_resolver_manager_->SetInsecureDnsClientEnabled(stub_resolver_enabled);
 
   // Configure DNS over HTTPS.
   if (!dns_over_https_servers || dns_over_https_servers.value().empty()) {
@@ -472,7 +472,8 @@ void NetworkService::ConfigureStubHostResolver(
     overrides.dns_over_https_servers.value().emplace_back(
         doh_server->server_template, doh_server->use_post);
   }
-  // TODO(dalyk): Allow the secure dns mode to be set.
+  // TODO(crbug.com/985589): Allow the secure dns mode to be set independently
+  // of the insecure part of the stub resolver.
   overrides.secure_dns_mode = net::DnsConfig::SecureDnsMode::AUTOMATIC;
   host_resolver_manager_->SetDnsConfigOverrides(overrides);
 }

services/network/network_service_unittest.cc

@@ -445,15 +445,21 @@ TEST_F(NetworkServiceTest, AuthEnableNegotiatePort) {
 TEST_F(NetworkServiceTest, DnsClientEnableDisable) {
   // HostResolver::GetDnsConfigAsValue() returns nullptr if the stub resolver is
   // disabled.
-  EXPECT_FALSE(service()->host_resolver_manager()->GetDnsConfigAsValue());
+  EXPECT_FALSE(service()
+                   ->host_resolver_manager()
+                   ->GetInsecureDnsClientEnabledForTesting());
   service()->ConfigureStubHostResolver(
       true /* stub_resolver_enabled */,
       base::nullopt /* dns_over_https_servers */);
-  EXPECT_TRUE(service()->host_resolver_manager()->GetDnsConfigAsValue());
+  EXPECT_TRUE(service()
+                  ->host_resolver_manager()
+                  ->GetInsecureDnsClientEnabledForTesting());
   service()->ConfigureStubHostResolver(
       false /* stub_resolver_enabled */,
       base::nullopt /* dns_over_https_servers */);
-  EXPECT_FALSE(service()->host_resolver_manager()->GetDnsConfigAsValue());
+  EXPECT_FALSE(service()
+                   ->host_resolver_manager()
+                   ->GetInsecureDnsClientEnabledForTesting());
 }
 
 TEST_F(NetworkServiceTest, DnsOverHttpsEnableDisable) {
@@ -464,10 +470,6 @@ TEST_F(NetworkServiceTest, DnsOverHttpsEnableDisable) {
   const std::string kServer3 = "https://grapefruit/resolver/query{?dns}";
   const bool kServer3UsePost = false;
 
-  // HostResolver::GetDnsClientForTesting() returns nullptr if the stub resolver
-  // is disabled.
-  EXPECT_FALSE(service()->host_resolver_manager()->GetDnsConfigAsValue());
-
   // Create the primary NetworkContext before enabling DNS over HTTPS.
   mojom::NetworkContextPtr network_context;
   mojom::NetworkContextParamsPtr context_params = CreateContextParams();