Simplify DnsConfigOverrides support for HOSTS file config

Replace |DnsConfigOverrides::hosts| with a simple |clear_hosts| bool
that merely specifies to not use HOSTS read from the system. Shouldn't
ever be any reason to ever override with made-up HOSTS values. This
simplification cleans up a lot of unnecessary complexity and reduces
net-external dependencies.

Bug: 1129993
Change-Id: Ie86b5e0394f763dcbe22b1d8db127c84d3501045
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419536
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810693}

net/dns/dns_config_overrides.cc

@@ -25,7 +25,6 @@ DnsConfigOverrides& DnsConfigOverrides::operator=(DnsConfigOverrides&& other) =
 
 bool DnsConfigOverrides::operator==(const DnsConfigOverrides& other) const {
   return nameservers == other.nameservers && search == other.search &&
-         hosts == other.hosts &&
          append_to_multi_label_name == other.append_to_multi_label_name &&
          ndots == other.ndots && timeout == other.timeout &&
          attempts == other.attempts && doh_attempts == other.doh_attempts &&
@@ -33,7 +32,8 @@ bool DnsConfigOverrides::operator==(const DnsConfigOverrides& other) const {
          dns_over_https_servers == other.dns_over_https_servers &&
          secure_dns_mode == other.secure_dns_mode &&
          allow_dns_over_https_upgrade == other.allow_dns_over_https_upgrade &&
-         disabled_upgrade_providers == other.disabled_upgrade_providers;
+         disabled_upgrade_providers == other.disabled_upgrade_providers &&
+         clear_hosts == other.clear_hosts;
 }
 
 bool DnsConfigOverrides::operator!=(const DnsConfigOverrides& other) const {
@@ -48,7 +48,6 @@ DnsConfigOverrides::CreateOverridingEverythingWithDefaults() {
   DnsConfigOverrides overrides;
   overrides.nameservers = defaults.nameservers;
   overrides.search = defaults.search;
-  overrides.hosts = defaults.hosts;
   overrides.append_to_multi_label_name = defaults.append_to_multi_label_name;
   overrides.ndots = defaults.ndots;
   overrides.timeout = defaults.timeout;
@@ -61,15 +60,17 @@ DnsConfigOverrides::CreateOverridingEverythingWithDefaults() {
   overrides.allow_dns_over_https_upgrade =
       defaults.allow_dns_over_https_upgrade;
   overrides.disabled_upgrade_providers = defaults.disabled_upgrade_providers;
+  overrides.clear_hosts = true;
 
   return overrides;
 }
 
 bool DnsConfigOverrides::OverridesEverything() const {
-  return nameservers && search && hosts && append_to_multi_label_name &&
-         ndots && timeout && attempts && doh_attempts && rotate &&
-         use_local_ipv6 && dns_over_https_servers && secure_dns_mode &&
-         allow_dns_over_https_upgrade && disabled_upgrade_providers;
+  return nameservers && search && append_to_multi_label_name && ndots &&
+         timeout && attempts && doh_attempts && rotate && use_local_ipv6 &&
+         dns_over_https_servers && secure_dns_mode &&
+         allow_dns_over_https_upgrade && disabled_upgrade_providers &&
+         clear_hosts;
 }
 
 DnsConfig DnsConfigOverrides::ApplyOverrides(const DnsConfig& config) const {
@@ -82,8 +83,6 @@ DnsConfig DnsConfigOverrides::ApplyOverrides(const DnsConfig& config) const {
     overridden.nameservers = nameservers.value();
   if (search)
     overridden.search = search.value();
-  if (hosts)
-    overridden.hosts = hosts.value();
   if (append_to_multi_label_name)
     overridden.append_to_multi_label_name = append_to_multi_label_name.value();
   if (ndots)
@@ -108,6 +107,8 @@ DnsConfig DnsConfigOverrides::ApplyOverrides(const DnsConfig& config) const {
   }
   if (disabled_upgrade_providers)
     overridden.disabled_upgrade_providers = disabled_upgrade_providers.value();
+  if (clear_hosts)
+    overridden.hosts.clear();
 
   return overridden;
 }

net/dns/dns_config_overrides.h

@@ -12,7 +12,6 @@
 #include "base/time/time.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_export.h"
-#include "net/dns/dns_hosts.h"
 #include "net/dns/public/dns_over_https_server_config.h"
 #include "net/dns/public/secure_dns_mode.h"
 
@@ -50,7 +49,6 @@ struct NET_EXPORT DnsConfigOverrides {
   // Overriding values. See same-named fields in DnsConfig for explanations.
   base::Optional<std::vector<IPEndPoint>> nameservers;
   base::Optional<std::vector<std::string>> search;
-  base::Optional<DnsHosts> hosts;
   base::Optional<bool> append_to_multi_label_name;
   base::Optional<int> ndots;
   base::Optional<base::TimeDelta> timeout;
@@ -63,6 +61,9 @@ struct NET_EXPORT DnsConfigOverrides {
   base::Optional<bool> allow_dns_over_https_upgrade;
   base::Optional<std::vector<std::string>> disabled_upgrade_providers;
 
+  // |hosts| is not supported for overriding except to clear it.
+  bool clear_hosts = false;
+
   // Note no overriding value for |unhandled_options|. It is meta-configuration,
   // and there should be no reason to override it.
 };

net/dns/host_resolver_manager_unittest.cc

@@ -7027,6 +7027,8 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   resolver_->SetDnsClientForTesting(std::move(client));
 
   DnsConfig original_config = CreateValidDnsConfig();
+  original_config.hosts = {
+      {DnsHostsKey("host", ADDRESS_FAMILY_IPV4), IPAddress(192, 168, 1, 1)}};
   ChangeDnsConfig(original_config);
 
   // Confirm pre-override state.
@@ -7038,9 +7040,6 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   overrides.nameservers = nameservers;
   const std::vector<std::string> search = {"str"};
   overrides.search = search;
-  const DnsHosts hosts = {
-      {DnsHostsKey("host", ADDRESS_FAMILY_IPV4), IPAddress(192, 168, 1, 1)}};
-  overrides.hosts = hosts;
   overrides.append_to_multi_label_name = false;
   const int ndots = 5;
   overrides.ndots = ndots;
@@ -7060,6 +7059,7 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   overrides.allow_dns_over_https_upgrade = true;
   const std::vector<std::string> disabled_upgrade_providers = {"provider_name"};
   overrides.disabled_upgrade_providers = disabled_upgrade_providers;
+  overrides.clear_hosts = true;
 
   // This test is expected to test overriding all fields.
   EXPECT_TRUE(overrides.OverridesEverything());
@@ -7072,7 +7072,6 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   ASSERT_TRUE(overridden_config);
   EXPECT_EQ(nameservers, overridden_config->nameservers);
   EXPECT_EQ(search, overridden_config->search);
-  EXPECT_EQ(hosts, overridden_config->hosts);
   EXPECT_FALSE(overridden_config->append_to_multi_label_name);
   EXPECT_EQ(ndots, overridden_config->ndots);
   EXPECT_EQ(timeout, overridden_config->timeout);
@@ -7085,6 +7084,7 @@ TEST_F(HostResolverManagerDnsTest, SetDnsConfigOverrides) {
   EXPECT_TRUE(overridden_config->allow_dns_over_https_upgrade);
   EXPECT_EQ(disabled_upgrade_providers,
             overridden_config->disabled_upgrade_providers);
+  EXPECT_THAT(overridden_config->hosts, testing::IsEmpty());
 
   base::RunLoop().RunUntilIdle();  // Notifications are async.
   EXPECT_EQ(1, config_observer.dns_changed_calls());

services/network/public/cpp/host_resolver_mojom_traits.cc

@@ -14,9 +14,6 @@ namespace mojo {
 
 using network::mojom::DnsConfigOverrides;
 using network::mojom::DnsConfigOverridesDataView;
-using network::mojom::DnsHost;
-using network::mojom::DnsHostDataView;
-using network::mojom::DnsHostPtr;
 using network::mojom::DnsOverHttpsServer;
 using network::mojom::DnsOverHttpsServerDataView;
 using network::mojom::DnsOverHttpsServerPtr;
@@ -46,46 +43,6 @@ base::Optional<bool> FromTristate(DnsConfigOverrides::Tristate tristate) {
   }
 }
 
-bool ReadHostData(mojo::ArrayDataView<DnsHostDataView> data,
-                  base::Optional<net::DnsHosts>* out) {
-  if (data.is_null()) {
-    out->reset();
-    return true;
-  }
-
-  out->emplace();
-  for (size_t i = 0; i < data.size(); ++i) {
-    DnsHostDataView host_data;
-    data.GetDataView(i, &host_data);
-
-    std::string hostname;
-    if (!host_data.ReadHostname(&hostname))
-      return false;
-
-    net::IPAddress address;
-    if (!host_data.ReadAddress(&address) || !address.IsValid())
-      return false;
-
-    net::AddressFamily address_family;
-    if (address.IsIPv4()) {
-      address_family = net::ADDRESS_FAMILY_IPV4;
-    } else if (address.IsIPv6()) {
-      address_family = net::ADDRESS_FAMILY_IPV6;
-    } else {
-      return false;
-    }
-
-    net::DnsHostsKey key = std::make_pair(std::move(hostname), address_family);
-    if (out->value().find(key) != out->value().end()) {
-      // Each DnsHostsKey expected to be unique.
-      return false;
-    }
-    out->value()[std::move(key)] = std::move(address);
-  }
-
-  return true;
-}
-
 bool ReadDnsOverHttpsServerData(
     mojo::ArrayDataView<DnsOverHttpsServerDataView> data,
     base::Optional<std::vector<net::DnsOverHttpsServerConfig>>* out) {
@@ -140,21 +97,6 @@ base::Optional<net::SecureDnsMode> FromOptionalSecureDnsMode(
   }
 }
 
-// static
-base::Optional<std::vector<DnsHostPtr>>
-StructTraits<DnsConfigOverridesDataView, net::DnsConfigOverrides>::hosts(
-    const net::DnsConfigOverrides& overrides) {
-  if (!overrides.hosts)
-    return base::nullopt;
-
-  std::vector<DnsHostPtr> out_hosts;
-  for (const net::DnsHosts::value_type& host : overrides.hosts.value()) {
-    out_hosts.push_back(DnsHost::New(host.first.first, host.second));
-  }
-
-  return base::make_optional(std::move(out_hosts));
-}
-
 // static
 DnsConfigOverrides::Tristate
 StructTraits<DnsConfigOverridesDataView, net::DnsConfigOverrides>::
@@ -216,11 +158,6 @@ bool StructTraits<DnsConfigOverridesDataView, net::DnsConfigOverrides>::Read(
   if (!data.ReadSearch(&out->search))
     return false;
 
-  mojo::ArrayDataView<DnsHostDataView> hosts_data;
-  data.GetHostsDataView(&hosts_data);
-  if (!ReadHostData(hosts_data, &out->hosts))
-    return false;
-
   out->append_to_multi_label_name =
       FromTristate(data.append_to_multi_label_name());
 
@@ -256,6 +193,8 @@ bool StructTraits<DnsConfigOverridesDataView, net::DnsConfigOverrides>::Read(
   if (!data.ReadDisabledUpgradeProviders(&out->disabled_upgrade_providers))
     return false;
 
+  out->clear_hosts = data.clear_hosts();
+
   return true;
 }
 

services/network/public/cpp/host_resolver_mojom_traits.h

@@ -19,7 +19,6 @@
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
 #include "net/dns/dns_config_overrides.h"
-#include "net/dns/dns_hosts.h"
 #include "net/dns/host_resolver.h"
 #include "net/dns/public/dns_query_type.h"
 #include "net/dns/public/secure_dns_mode.h"
@@ -46,9 +45,6 @@ struct StructTraits<network::mojom::DnsConfigOverridesDataView,
     return overrides.search;
   }
 
-  static base::Optional<std::vector<network::mojom::DnsHostPtr>> hosts(
-      const net::DnsConfigOverrides& overrides);
-
   static network::mojom::DnsConfigOverrides_Tristate append_to_multi_label_name(
       const net::DnsConfigOverrides& overrides);
 
@@ -84,6 +80,10 @@ struct StructTraits<network::mojom::DnsConfigOverridesDataView,
     return overrides.disabled_upgrade_providers;
   }
 
+  static bool clear_hosts(const net::DnsConfigOverrides& overrides) {
+    return overrides.clear_hosts;
+  }
+
   static bool Read(network::mojom::DnsConfigOverridesDataView data,
                    net::DnsConfigOverrides* out);
 };

services/network/public/cpp/host_resolver_mojom_traits_unittest.cc

@@ -31,11 +31,6 @@ TEST(HostResolverMojomTraitsTest, DnsConfigOverridesRoundtrip_FullySpecified) {
   original.nameservers.emplace(
       {net::IPEndPoint(net::IPAddress(1, 2, 3, 4), 80)});
   original.search.emplace({std::string("str")});
-  original.hosts = net::DnsHosts(
-      {std::make_pair(net::DnsHostsKey("host1", net::ADDRESS_FAMILY_IPV4),
-                      net::IPAddress(2, 3, 4, 5)),
-       std::make_pair(net::DnsHostsKey("host2", net::ADDRESS_FAMILY_IPV4),
-                      net::IPAddress(2, 3, 4, 5))});
   original.append_to_multi_label_name = true;
   original.ndots = 2;
   original.timeout = base::TimeDelta::FromHours(4);
@@ -47,6 +42,7 @@ TEST(HostResolverMojomTraitsTest, DnsConfigOverridesRoundtrip_FullySpecified) {
   original.secure_dns_mode = net::SecureDnsMode::kSecure;
   original.allow_dns_over_https_upgrade = true;
   original.disabled_upgrade_providers.emplace({std::string("provider_name")});
+  original.clear_hosts = true;
 
   net::DnsConfigOverrides deserialized;
   EXPECT_TRUE(mojo::test::SerializeAndDeserialize<mojom::DnsConfigOverrides>(
@@ -79,42 +75,6 @@ TEST(HostResolverMojomTraitsTest, DnsConfigOverrides_OnlyDnsOverHttpsServers) {
   EXPECT_EQ(original, deserialized);
 }
 
-TEST(HostResolverMojomTraitsTest, DnsConfigOverrides_OnlyHosts) {
-  net::DnsConfigOverrides original;
-  original.hosts = net::DnsHosts(
-      {std::make_pair(net::DnsHostsKey("host", net::ADDRESS_FAMILY_IPV4),
-                      net::IPAddress(1, 1, 1, 1))});
-
-  net::DnsConfigOverrides deserialized;
-  EXPECT_TRUE(mojo::test::SerializeAndDeserialize<mojom::DnsConfigOverrides>(
-      &original, &deserialized));
-
-  EXPECT_EQ(original, deserialized);
-}
-
-TEST(HostResolverMojomTraitsTest, DnsConfigOverrides_NonUniqueHostKeys) {
-  mojom::DnsConfigOverridesPtr overrides = mojom::DnsConfigOverrides::New();
-  overrides->hosts.emplace();
-
-  // Create two different entries that share the key ("host", IPV4).
-  mojom::DnsHostPtr host_entry1 = mojom::DnsHost::New();
-  host_entry1->hostname = "host";
-  host_entry1->address = net::IPAddress(1, 1, 1, 1);
-  overrides->hosts.value().push_back(std::move(host_entry1));
-
-  mojom::DnsHostPtr host_entry2 = mojom::DnsHost::New();
-  host_entry2->hostname = "host";
-  host_entry2->address = net::IPAddress(2, 2, 2, 2);
-  overrides->hosts.value().push_back(std::move(host_entry2));
-
-  std::vector<uint8_t> serialized =
-      mojom::DnsConfigOverrides::Serialize(&overrides);
-
-  net::DnsConfigOverrides deserialized;
-  EXPECT_FALSE(
-      mojom::DnsConfigOverrides::Deserialize(serialized, &deserialized));
-}
-
 TEST(HostResolverMojomTraitsTest, ResolveErrorInfo) {
   net::ResolveErrorInfo original;
   original.error = net::ERR_NAME_NOT_RESOLVED;

services/network/public/mojom/host_resolver.mojom

@@ -6,18 +6,11 @@ module network.mojom;
 
 import "mojo/public/mojom/base/time.mojom";
 import "services/network/public/mojom/address_list.mojom";
-import "services/network/public/mojom/ip_address.mojom";
 import "services/network/public/mojom/ip_endpoint.mojom";
 import "services/network/public/mojom/network_isolation_key.mojom";
 import "services/network/public/mojom/network_param.mojom";
 import "services/network/public/mojom/url_loader.mojom";
 
-// A single entry from a HOSTS file.
-struct DnsHost {
-  string hostname;
-  IPAddress address;
-};
-
 // An HTTPS server to send DNS queries to, per the DNS Queries over HTTPS spec.
 // spec:  https://tools.ietf.org/html/draft-ietf-doh-dns-over-https-12
 struct DnsOverHttpsServer {
@@ -66,11 +59,6 @@ struct DnsConfigOverrides {
   // is less than |ndots|.
   array<string>? search;
 
-  // Entries from a HOSTS file. This array is intended for serialization to/from
-  // a lookup map, so unlike the source data from actual HOSTS files, each entry
-  // should represent a unique host query key (|hostname| and AddressFamily).
-  array<DnsHost>? hosts;
-
   // Whether suffix search should be performed for multi-label names.
   Tristate append_to_multi_label_name = Tristate.NO_OVERRIDE;
 
@@ -105,6 +93,9 @@ struct DnsConfigOverrides {
   // List of providers to exclude from upgrade mapping. See the
   // mapping in net/dns/dns_util.cc for provider ids.
   array<string>? disabled_upgrade_providers;
+
+  // Whether to clear data read from the system HOSTS file.
+  bool clear_hosts = false;
 };
 
 // Host resolution error info.