From: csagan5 <32685696+csagan5@users.noreply.github.com> Date: Sat, 26 Sep 2020 14:23:19 +0100 Subject: DoH improvements Enable secure mode by default Always enforce DoH even with inconsistent system DNS configuration License: GPL-3.0-only - https://spdx.org/licenses/GPL-3.0-only.html --- chrome/browser/net/secure_dns_config.cc | 2 +- chrome/browser/net/secure_dns_util.cc | 2 +- chrome/browser/net/stub_resolver_config_reader.cc | 3 +++ chrome/browser/privacy/secure_dns_bridge.cc | 2 +- .../ui/webui/settings/settings_secure_dns_handler.cc | 2 +- net/dns/dns_client.cc | 11 ++++++++--- net/dns/host_resolver_manager.cc | 1 + net/dns/public/doh_provider_entry.cc | 9 +++------ net/dns/public/doh_provider_entry.h | 4 ++-- 9 files changed, 21 insertions(+), 15 deletions(-) diff --git a/chrome/browser/net/secure_dns_config.cc b/chrome/browser/net/secure_dns_config.cc --- a/chrome/browser/net/secure_dns_config.cc +++ b/chrome/browser/net/secure_dns_config.cc @@ -27,7 +27,7 @@ std::optional SecureDnsConfig::ParseMode( if (name == kModeSecure) { return net::SecureDnsMode::kSecure; } else if (name == kModeAutomatic) { - return net::SecureDnsMode::kAutomatic; + return net::SecureDnsMode::kSecure; } else if (name == kModeOff) { return net::SecureDnsMode::kOff; } diff --git a/chrome/browser/net/secure_dns_util.cc b/chrome/browser/net/secure_dns_util.cc --- a/chrome/browser/net/secure_dns_util.cc +++ b/chrome/browser/net/secure_dns_util.cc @@ -42,7 +42,7 @@ bool EntryIsForCountry(const net::DohProviderEntry* entry, int country_id) { country_id; }); if (matches) { - DCHECK(!entry->ui_name.empty()); + DCHECK(!entry->ui_name_cromite.empty()); DCHECK(!entry->privacy_policy.empty()); } return matches; diff --git a/chrome/browser/net/stub_resolver_config_reader.cc b/chrome/browser/net/stub_resolver_config_reader.cc --- a/chrome/browser/net/stub_resolver_config_reader.cc +++ b/chrome/browser/net/stub_resolver_config_reader.cc @@ -83,6 +83,7 @@ enum class SecureDnsModeDetailsForHistogram { #if BUILDFLAG(IS_WIN) bool ShouldDisableDohForWindowsParentalControls() { + if ((true)) return false; return GetWinParentalControls().web_filter; } #endif // BUILDFLAG(IS_WIN) @@ -189,6 +190,7 @@ void StubResolverConfigReader::UpdateNetworkService(bool record_metrics) { } bool StubResolverConfigReader::ShouldDisableDohForManaged() { + if ((true)) return false; // This function ignores cloud policies which are loaded on a per-profile basis. #if BUILDFLAG(IS_ANDROID) // Check for MDM/management/owner apps. android_has_owner_ is true if either a @@ -217,6 +219,7 @@ bool StubResolverConfigReader::ShouldDisableDohForManaged() { } bool StubResolverConfigReader::ShouldDisableDohForParentalControls() { + if ((true)) return false; if (parental_controls_testing_override_.has_value()) return parental_controls_testing_override_.value(); diff --git a/chrome/browser/privacy/secure_dns_bridge.cc b/chrome/browser/privacy/secure_dns_bridge.cc --- a/chrome/browser/privacy/secure_dns_bridge.cc +++ b/chrome/browser/privacy/secure_dns_bridge.cc @@ -109,7 +109,7 @@ static ScopedJavaLocalRef JNI_SecureDnsBridge_GetProviders( providers, std::back_inserter(ret), [](const net::DohProviderEntry* entry) -> std::vector { net::DnsOverHttpsConfig config({entry->doh_server_config}); - return {base::UTF8ToUTF16(entry->ui_name), + return {base::UTF8ToUTF16(entry->ui_name_cromite.empty() ? entry->provider : entry->ui_name_cromite), base::UTF8ToUTF16(config.ToString()), base::UTF8ToUTF16(entry->privacy_policy)}; }); diff --git a/chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc b/chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc --- a/chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc +++ b/chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc @@ -120,7 +120,7 @@ base::Value::List SecureDnsHandler::GetSecureDnsResolverList() { for (const net::DohProviderEntry* entry : providers_) { net::DnsOverHttpsConfig doh_config({entry->doh_server_config}); base::Value::Dict dict; - dict.Set("name", entry->ui_name); + dict.Set("name", entry->ui_name_cromite.empty() ? entry->provider : entry->ui_name_cromite); dict.Set("value", doh_config.ToString()); dict.Set("policy", entry->privacy_policy); resolvers.Append(std::move(dict)); diff --git a/net/dns/dns_client.cc b/net/dns/dns_client.cc --- a/net/dns/dns_client.cc +++ b/net/dns/dns_client.cc @@ -249,11 +249,14 @@ class DnsClientImpl : public DnsClient { private: std::optional BuildEffectiveConfig() const { DnsConfig config; - if (config_overrides_.OverridesEverything()) { + // in Bromite it is sufficient to have secure DoH enabled to give the overrides priority + if (config_overrides_.dns_over_https_config && config_overrides_.secure_dns_mode) { config = config_overrides_.ApplyOverrides(DnsConfig()); } else { - if (!system_config_) + if (!system_config_) { + LOG(WARNING) << "BuildEffectiveConfig(): no system configuration"; return std::nullopt; + } config = config_overrides_.ApplyOverrides(system_config_.value()); } @@ -268,8 +271,10 @@ class DnsClientImpl : public DnsClient { if (config.unhandled_options) config.nameservers.clear(); - if (!config.IsValid()) + if (!config.IsValid()) { + LOG(WARNING) << "BuildEffectiveConfig(): invalid configuration"; return std::nullopt; + } return config; } diff --git a/net/dns/host_resolver_manager.cc b/net/dns/host_resolver_manager.cc --- a/net/dns/host_resolver_manager.cc +++ b/net/dns/host_resolver_manager.cc @@ -640,6 +640,7 @@ void HostResolverManager::SetDnsConfigOverrides(DnsConfigOverrides overrides) { bool changed = dns_client_->SetConfigOverrides(std::move(overrides)); if (changed) { + LOG(INFO) << "triggering non-system DNS change"; NetworkChangeNotifier::TriggerNonSystemDnsChange(); // Only invalidate cache if new overrides have resulted in a config change. diff --git a/net/dns/public/doh_provider_entry.cc b/net/dns/public/doh_provider_entry.cc --- a/net/dns/public/doh_provider_entry.cc +++ b/net/dns/public/doh_provider_entry.cc @@ -350,16 +350,13 @@ DohProviderEntry::DohProviderEntry( doh_server_config( ParseValidDohTemplate(std::move(dns_over_https_template), std::move(dns_over_https_server_ip_strs))), - ui_name(std::move(ui_name)), + ui_name_cromite(std::move(ui_name)), privacy_policy(std::move(privacy_policy)), - display_globally(display_globally), + display_globally(true), display_countries(std::move(display_countries)), logging_level(logging_level) { DCHECK(!display_globally || this->display_countries.empty()); - if (display_globally || !this->display_countries.empty()) { - DCHECK(!this->ui_name.empty()); - DCHECK(!this->privacy_policy.empty()); - } + if (this->privacy_policy.empty()) this->display_globally = false; for (const auto& display_country : this->display_countries) { DCHECK_EQ(2u, display_country.size()); } diff --git a/net/dns/public/doh_provider_entry.h b/net/dns/public/doh_provider_entry.h --- a/net/dns/public/doh_provider_entry.h +++ b/net/dns/public/doh_provider_entry.h @@ -55,7 +55,7 @@ struct NET_EXPORT DohProviderEntry { std::set ip_addresses; std::set dns_over_tls_hostnames; DnsOverHttpsServerConfig doh_server_config; - std::string ui_name; + std::string ui_name_cromite; std::string privacy_policy; bool display_globally; std::set display_countries; @@ -95,7 +95,7 @@ struct NET_EXPORT DohProviderEntry { std::set dns_over_53_server_ip_strs, std::set dns_over_tls_hostnames, std::string dns_over_https_template, - std::string ui_name, + std::string ui_name_cromite, std::string privacy_policy, bool display_globally, std::set display_countries, --