diff options
Diffstat (limited to 'chromium/net/dns')
46 files changed, 2265 insertions, 2718 deletions
diff --git a/chromium/net/dns/BUILD.gn b/chromium/net/dns/BUILD.gn index 74680eccfea..3e24edd0b48 100644 --- a/chromium/net/dns/BUILD.gn +++ b/chromium/net/dns/BUILD.gn @@ -5,6 +5,10 @@ import("//net/features.gni") import("//testing/libfuzzer/fuzzer_test.gni") +if (is_android) { + import("//build/config/android/rules.gni") +} + enable_built_in_dns = !is_ios source_set("dns") { @@ -62,7 +66,6 @@ source_set("dns") { "dns_transaction.cc", "dns_udp_tracker.cc", "dns_udp_tracker.h", - "esni_content.cc", "host_cache.cc", "host_resolver.cc", "host_resolver_manager.cc", @@ -72,6 +75,8 @@ source_set("dns") { "host_resolver_mdns_task.h", "host_resolver_proc.cc", "host_resolver_proc.h", + "httpssvc_metrics.cc", + "httpssvc_metrics.h", "mapped_host_resolver.cc", "notify_watcher_mac.cc", "notify_watcher_mac.h", @@ -198,7 +203,6 @@ source_set("host_resolver") { sources += [ "dns_config.h", "dns_config_overrides.h", - "esni_content.h", "host_cache.h", "host_resolver.h", "host_resolver_source.h", @@ -371,6 +375,12 @@ source_set("mdns_client") { public_deps = [ "//net:net_public_deps" ] } +if (is_android) { + java_cpp_enum("secure_dns_mode_generated_enum") { + sources = [ "dns_config.h" ] + } +} + source_set("tests") { testonly = true sources = [ @@ -386,9 +396,9 @@ source_set("tests") { "dns_transaction_unittest.cc", "dns_udp_tracker_unittest.cc", "dns_util_unittest.cc", - "esni_content_unittest.cc", "host_cache_unittest.cc", "host_resolver_manager_unittest.cc", + "httpssvc_metrics_unittest.cc", "mapped_host_resolver_unittest.cc", "record_parsed_unittest.cc", "record_rdata_unittest.cc", diff --git a/chromium/net/dns/address_info.cc b/chromium/net/dns/address_info.cc index ec8016a33f3..67a357a112e 100644 --- a/chromium/net/dns/address_info.cc +++ b/chromium/net/dns/address_info.cc @@ -5,6 +5,7 @@ #include "net/dns/address_info.h" #include "base/logging.h" +#include "base/notreached.h" #include "base/sys_byteorder.h" #include "net/base/address_list.h" #include "net/base/net_errors.h" diff --git a/chromium/net/dns/context_host_resolver.cc b/chromium/net/dns/context_host_resolver.cc index 0ae8cc862f7..32a7a0ddf27 100644 --- a/chromium/net/dns/context_host_resolver.cc +++ b/chromium/net/dns/context_host_resolver.cc @@ -150,16 +150,6 @@ class ContextHostResolver::WrappedResolveHostRequest return inner_request_->GetHostnameResults(); } - const base::Optional<EsniContent>& GetEsniResults() const override { - if (!inner_request_) { - static const base::NoDestructor<base::Optional<EsniContent>> - nullopt_result; - return *nullopt_result; - } - - return inner_request_->GetEsniResults(); - } - net::ResolveErrorInfo GetResolveErrorInfo() const override { if (!inner_request_) { return resolve_error_info_; diff --git a/chromium/net/dns/dns_config.h b/chromium/net/dns/dns_config.h index 9e1e3e917d0..d65a31eb45d 100644 --- a/chromium/net/dns/dns_config.h +++ b/chromium/net/dns/dns_config.h @@ -51,6 +51,7 @@ struct NET_EXPORT DnsConfig { return !nameservers.empty() || !dns_over_https_servers.empty(); } + // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.net // The SecureDnsMode specifies what types of lookups (secure/insecure) should // be performed and in what order when resolving a specific query. The int // values should not be changed as they are logged. diff --git a/chromium/net/dns/dns_hosts.cc b/chromium/net/dns/dns_hosts.cc index 13ee84b7d0b..04b1596e613 100644 --- a/chromium/net/dns/dns_hosts.cc +++ b/chromium/net/dns/dns_hosts.cc @@ -7,7 +7,6 @@ #include "base/check.h" #include "base/files/file_util.h" #include "base/macros.h" -#include "base/metrics/histogram_macros.h" #include "base/strings/string_util.h" #include "net/dns/dns_util.h" @@ -199,9 +198,6 @@ bool ParseHostsFile(const base::FilePath& path, DnsHosts* dns_hosts) { if (!base::GetFileSize(path, &size)) return false; - UMA_HISTOGRAM_COUNTS_1M("AsyncDNS.HostsSize", - static_cast<base::HistogramBase::Sample>(size)); - // Reject HOSTS files larger than |kMaxHostsSize| bytes. const int64_t kMaxHostsSize = 1 << 25; // 32MB if (size > kMaxHostsSize) diff --git a/chromium/net/dns/dns_response.cc b/chromium/net/dns/dns_response.cc index 523ccc2725b..860eebbc5e7 100644 --- a/chromium/net/dns/dns_response.cc +++ b/chromium/net/dns/dns_response.cc @@ -93,8 +93,10 @@ DnsResourceRecord& DnsResourceRecord::operator=(DnsResourceRecord&& other) { } void DnsResourceRecord::SetOwnedRdata(std::string value) { + DCHECK(!value.empty()); owned_rdata = std::move(value); rdata = owned_rdata; + DCHECK_EQ(owned_rdata.data(), rdata.data()); } size_t DnsResourceRecord::CalculateRecordSize() const { @@ -570,8 +572,7 @@ bool DnsResponse::WriteQuestion(base::BigEndianWriter* writer, bool DnsResponse::WriteRecord(base::BigEndianWriter* writer, const DnsResourceRecord& record) { - if (record.rdata.data() != record.owned_rdata.data() || - record.rdata.size() != record.owned_rdata.size()) { + if (record.rdata != base::StringPiece(record.owned_rdata)) { VLOG(1) << "record.rdata should point to record.owned_rdata."; return false; } diff --git a/chromium/net/dns/dns_response.h b/chromium/net/dns/dns_response.h index 75d69707345..b7fa79aadc5 100644 --- a/chromium/net/dns/dns_response.h +++ b/chromium/net/dns/dns_response.h @@ -44,8 +44,8 @@ struct NET_EXPORT_PRIVATE DnsResourceRecord { DnsResourceRecord& operator=(const DnsResourceRecord& other); DnsResourceRecord& operator=(DnsResourceRecord&& other); - // A helper to set |owned_rdata| that also sets |rdata| to point to it. - // See the definition of |owned_rdata| below. + // A helper to set |owned_rdata| that also sets |rdata| to point to it. The + // |value| must be non-empty. See the definition of |owned_rdata| below. void SetOwnedRdata(std::string value); // NAME (variable length) + TYPE (2 bytes) + CLASS (2 bytes) + TTL (4 bytes) + @@ -208,9 +208,9 @@ class NET_EXPORT_PRIVATE DnsResponse { bool WriteHeader(base::BigEndianWriter* writer, const dns_protocol::Header& header); bool WriteQuestion(base::BigEndianWriter* writer, const DnsQuery& query); - bool WriteRecord(base::BigEndianWriter* wirter, + bool WriteRecord(base::BigEndianWriter* writer, const DnsResourceRecord& record); - bool WriteAnswer(base::BigEndianWriter* wirter, + bool WriteAnswer(base::BigEndianWriter* writer, const DnsResourceRecord& answer, const base::Optional<DnsQuery>& query); diff --git a/chromium/net/dns/dns_test_util.cc b/chromium/net/dns/dns_test_util.cc index 0497c197e4f..efcec21579d 100644 --- a/chromium/net/dns/dns_test_util.cc +++ b/chromium/net/dns/dns_test_util.cc @@ -166,77 +166,25 @@ DnsResourceRecord BuildServiceRecord(std::string name, return record; } -void AppendU16LengthPrefixed(base::StringPiece in, std::string* out) { - DCHECK(out); - char buf[2]; - base::WriteBigEndian(buf, base::checked_cast<uint16_t>(in.size())); - out->append(buf, 2); - out->insert(out->end(), in.begin(), in.end()); -} - -// Builds an ESNI (TLS 1.3 Encrypted Server Name Indication, draft 4) record. -// -// An ESNI record associates an "ESNI key object" (an opaque string used -// by the TLS library) with a collection of IP addresses. -DnsResourceRecord BuildEsniRecord(std::string name, EsniContent esni_content) { - DCHECK(!name.empty()); + + +DnsResourceRecord BuildIntegrityRecord( + std::string name, + const std::vector<uint8_t>& serialized_rdata) { + CHECK(!name.empty()); DnsResourceRecord record; record.name = std::move(name); - record.type = dns_protocol::kExperimentalTypeEsniDraft4; + record.type = dns_protocol::kExperimentalTypeIntegrity; record.klass = dns_protocol::kClassIN; record.ttl = base::TimeDelta::FromDays(1).InSeconds(); - std::string rdata; - - // An esni_content struct corresponding to a single record - // should have exactly one key object, along with zero or more addresses - // corresponding to the key object. - DCHECK_EQ(esni_content.keys().size(), 1u); - rdata += *esni_content.keys().begin(); - - if (esni_content.keys_for_addresses().empty()) { - // No addresses: leave the "dns_extensions" field of the - // ESNI record empty and conclude the rdata with the - // "dns_extensions" field's length prefix (two zero bytes). - rdata.push_back(0); - rdata.push_back(0); - record.SetOwnedRdata(std::move(rdata)); - return record; - } - - // When the "dns_extensions" field of a draft-4 ESNI record is nonempty, - // it stores an IP addresses: more specifically, it contains - // - a 16-bit length prefix, - // - the 16-bit "extension type" label of the single address_set - // extension (the only type of extension) contained in the extensions object, - // - a 16-bit length prefix for the address_set extension's contents, and - // - the contents of the address_set extension, which is just a list - // of type-prefixed network-order IP addresses. - // - // (See the draft spec for the complete definition.) - std::string dns_extensions; - - std::string address_set; - char buf[2]; - base::WriteBigEndian(buf, EsniRecordRdata::kAddressSetExtensionType); - address_set.append(buf, 2); - - std::string serialized_addresses; - for (const auto& kv : esni_content.keys_for_addresses()) { - IPAddress address = kv.first; - - uint8_t address_type = address.IsIPv4() ? 4 : 6; - serialized_addresses.push_back(address_type); - serialized_addresses.insert(serialized_addresses.end(), - address.bytes().begin(), address.bytes().end()); - } + std::string serialized_rdata_str(serialized_rdata.begin(), + serialized_rdata.end()); + record.SetOwnedRdata(std::move(serialized_rdata_str)); - AppendU16LengthPrefixed(serialized_addresses, &address_set); - AppendU16LengthPrefixed(address_set, &dns_extensions); - rdata.append(dns_extensions); + CHECK_EQ(record.rdata.data(), record.owned_rdata.data()); - record.SetOwnedRdata(std::move(rdata)); return record; } @@ -257,28 +205,6 @@ DnsResourceRecord BuildTestAddressRecord(std::string name, return record; } -const char kWellFormedEsniKeys[] = { - 0xff, 0x3, 0x0, 0x1, 0xff, 0x0, 0x24, 0x0, 0x1d, 0x0, 0x20, - 0xed, 0xed, 0xc8, 0x68, 0xc1, 0x71, 0xd6, 0x9e, 0xa9, 0xf0, 0xa2, - 0xc9, 0xf5, 0xa9, 0xdc, 0xcf, 0xf9, 0xb8, 0xed, 0x15, 0x5c, 0xc4, - 0x5a, 0xec, 0x6f, 0xb2, 0x86, 0x14, 0xb7, 0x71, 0x1b, 0x7c, 0x0, - 0x2, 0x13, 0x1, 0x1, 0x4, 0x0, 0x0}; -const size_t kWellFormedEsniKeysSize = sizeof(kWellFormedEsniKeys); - -std::string GenerateWellFormedEsniKeys(base::StringPiece custom_data) { - std::string well_formed_esni_keys(kWellFormedEsniKeys, - kWellFormedEsniKeysSize); - // Dead-reckon to the first byte after ESNIKeys.keys.group (0x001d). - // - // Overwrite at most 0x22 bytes: this is the length of the "keys" field - // in the example struct (0x0024, specified as a 16-bit big-endian value - // by the index-5 and index-6 bytes), minus 2 because the 0x0, 0x1d bytes - // will not be overwritten. - custom_data = custom_data.substr(0, 0x22); - std::copy(custom_data.begin(), custom_data.end(), - well_formed_esni_keys.begin() + 9); - return well_formed_esni_keys; -} std::unique_ptr<DnsResponse> BuildTestDnsResponse(std::string name, const IPAddress& ip) { @@ -387,28 +313,23 @@ std::unique_ptr<DnsResponse> BuildTestDnsServiceResponse( std::vector<DnsResourceRecord>() /* additional_records */, query); } -std::unique_ptr<DnsResponse> BuildTestDnsEsniResponse( +std::unique_ptr<DnsResponse> BuildTestDnsIntegrityResponse( std::string hostname, - std::vector<EsniContent> esni_records, - std::string answer_name) { - if (answer_name.empty()) - answer_name = hostname; + const std::vector<uint8_t>& serialized_rdata) { + CHECK(!hostname.empty()); - std::vector<DnsResourceRecord> answers; - answers.reserve(esni_records.size()); - for (EsniContent& c : esni_records) { - answers.push_back(BuildEsniRecord(answer_name, c)); - } + std::vector<DnsResourceRecord> answers{ + BuildIntegrityRecord(hostname, serialized_rdata)}; std::string dns_name; CHECK(DNSDomainFromDot(hostname, &dns_name)); base::Optional<DnsQuery> query(base::in_place, 0, dns_name, - dns_protocol::kExperimentalTypeEsniDraft4); + dns_protocol::kExperimentalTypeIntegrity); return std::make_unique<DnsResponse>( 0, false, std::move(answers), - std::vector<DnsResourceRecord>() /* authority_records */, - std::vector<DnsResourceRecord>() /* additional_records */, query); + std::vector<DnsResourceRecord>() /* authority_records */, + std::vector<DnsResourceRecord>() /* additional_records */, query); } MockDnsClientRule::Result::Result(ResultType type) : type(type) {} @@ -560,15 +481,17 @@ class MockDnsTransactionFactory::MockTransaction case MockDnsClientRule::NODOMAIN: case MockDnsClientRule::FAIL: std::move(callback_).Run(this, ERR_NAME_NOT_RESOLVED, - result_.response.get()); + result_.response.get(), base::nullopt); break; case MockDnsClientRule::EMPTY: case MockDnsClientRule::OK: case MockDnsClientRule::MALFORMED: - std::move(callback_).Run(this, OK, result_.response.get()); + std::move(callback_).Run(this, OK, result_.response.get(), + base::nullopt); break; case MockDnsClientRule::TIMEOUT: - std::move(callback_).Run(this, ERR_DNS_TIMED_OUT, nullptr); + std::move(callback_).Run(this, ERR_DNS_TIMED_OUT, nullptr, + base::nullopt); break; } } diff --git a/chromium/net/dns/dns_test_util.h b/chromium/net/dns/dns_test_util.h index bc056cd8a80..40c619976a0 100644 --- a/chromium/net/dns/dns_test_util.h +++ b/chromium/net/dns/dns_test_util.h @@ -23,7 +23,6 @@ #include "net/dns/dns_response.h" #include "net/dns/dns_transaction.h" #include "net/dns/dns_util.h" -#include "net/dns/esni_content.h" #include "net/dns/public/dns_protocol.h" #include "net/socket/socket_test_util.h" @@ -184,22 +183,6 @@ static const char* const kT4IpAddresses[] = {"172.217.6.195"}; static const int kT4TTL = 0x0000012b; static const unsigned kT4RecordCount = base::size(kT0IpAddresses); -//-------------------------------------------------------------------- -// A well-formed ESNI (TLS 1.3 Encrypted Server Name Indication, -// draft 4) keys object ("ESNIKeys" member of the ESNIRecord struct from -// the spec). -// -// (This is cribbed from boringssl SSLTest.ESNIKeysDeserialize (CL 37704/13).) -extern const char kWellFormedEsniKeys[]; -extern const size_t kWellFormedEsniKeysSize; - -// Returns a well-formed ESNI keys object identical to kWellFormedEsniKeys, -// except that the first 0x22 bytes of |custom_data| are written over -// fields of the keys object in a manner that leaves length prefixes -// correct and enum members valid, and so that distinct values of -// |custom_data| result in distinct returned keys. -std::string GenerateWellFormedEsniKeys(base::StringPiece custom_data = ""); - class AddressSorter; class DnsClient; class DnsSession; @@ -241,10 +224,9 @@ std::unique_ptr<DnsResponse> BuildTestDnsServiceResponse( std::vector<TestServiceRecord> service_records, std::string answer_name = ""); -std::unique_ptr<DnsResponse> BuildTestDnsEsniResponse( +std::unique_ptr<DnsResponse> BuildTestDnsIntegrityResponse( std::string hostname, - std::vector<EsniContent> esni_records, - std::string answer_name = ""); + const std::vector<uint8_t>& serialized_rdata); struct MockDnsClientRule { enum ResultType { diff --git a/chromium/net/dns/dns_transaction.cc b/chromium/net/dns/dns_transaction.cc index 8c58aeca906..60e97bf23f3 100644 --- a/chromium/net/dns/dns_transaction.cc +++ b/chromium/net/dns/dns_transaction.cc @@ -129,7 +129,7 @@ base::Value NetLogStartParams(const std::string& hostname, uint16_t qtype) { // matches. Logging is done in the socket and in the outer DnsTransaction. class DnsAttempt { public: - explicit DnsAttempt(int server_index) + explicit DnsAttempt(size_t server_index) : result_(ERR_FAILED), server_index_(server_index) {} virtual ~DnsAttempt() = default; @@ -147,10 +147,9 @@ class DnsAttempt { // Returns the net log bound to the source of the socket. virtual const NetLogWithSource& GetSocketNetLog() const = 0; - // Returns the index of the destination server within DnsConfig::nameservers. - // If the server index is -1, indicates that no request was sent and that the - // attempt was resolved synchronously with failure. - int server_index() const { return server_index_; } + // Returns the index of the destination server within DnsConfig::nameservers + // (or DnsConfig::dns_over_https_servers for secure transactions). + size_t server_index() const { return server_index_; } // Returns a Value representing the received response, along with a reference // to the NetLog source source of the UDP socket used. The request must have @@ -180,7 +179,7 @@ class DnsAttempt { // Result of last operation. int result_; - const int server_index_; + const size_t server_index_; DISALLOW_COPY_AND_ASSIGN(DnsAttempt); }; @@ -570,7 +569,7 @@ class DnsHTTPAttempt : public DnsAttempt, public URLRequest::Delegate { }; void ConstructDnsHTTPAttempt(DnsSession* session, - int doh_server_index, + size_t doh_server_index, std::string hostname, uint16_t qtype, const OptRecordRdata* opt_rdata, @@ -589,9 +588,7 @@ void ConstructDnsHTTPAttempt(DnsSession* session, query = attempts->at(0)->GetQuery()->CloneWithNewId(id); } - DCHECK_GE(doh_server_index, 0); - DCHECK_LT(doh_server_index, - (int)session->config().dns_over_https_servers.size()); + DCHECK_LT(doh_server_index, session->config().dns_over_https_servers.size()); const DnsOverHttpsServerConfig& doh_config = session->config().dns_over_https_servers[doh_server_index]; GURL gurl_without_parameters( @@ -922,7 +919,7 @@ class DnsOverHttpsProbeRunner : public DnsProbeRunner { base::WeakPtrFactory<ProbeStats> weak_factory{this}; }; - void ContinueProbe(int doh_server_index, + void ContinueProbe(size_t doh_server_index, base::WeakPtr<ProbeStats> probe_stats, bool network_change, base::TimeTicks sequence_start_time) { @@ -973,7 +970,7 @@ class DnsOverHttpsProbeRunner : public DnsProbeRunner { } void ProbeComplete(unsigned attempt_number, - int doh_server_index, + size_t doh_server_index, base::WeakPtr<ProbeStats> probe_stats, bool network_change, base::TimeTicks sequence_start_time, @@ -1187,7 +1184,14 @@ class DnsTransactionImpl : public DnsTransaction, net_log_.EndEventWithNetErrorCode(NetLogEventType::DNS_TRANSACTION, result.rv); - std::move(callback_).Run(this, result.rv, response); + base::Optional<std::string> doh_provider_id; + if (secure_ && result.attempt) { + size_t server_index = result.attempt->server_index(); + doh_provider_id = GetDohProviderIdForHistogramFromDohConfig( + session_->config().dns_over_https_servers[server_index]); + } + + std::move(callback_).Run(this, result.rv, response, doh_provider_id); } AttemptResult MakeAttempt() { @@ -1406,7 +1410,7 @@ class DnsTransactionImpl : public DnsTransaction, if (result.attempt) { resolve_context_->RecordServerFailure( result.attempt->server_index(), secure_ /* is_doh_server */, - session_.get()); + result.rv, session_.get()); } if (MoreAttemptsAllowed()) { result = MakeAttempt(); @@ -1424,14 +1428,20 @@ class DnsTransactionImpl : public DnsTransaction, default: // Server failure. DCHECK(result.attempt); + + // If attempt is not the most recent attempt, means this error is for + // an attempt that already timed out and was treated as complete but + // allowed to continue attempting in parallel with new attempts (see + // the ERR_DNS_TIMED_OUT case above). As the failure was already + // recorded at timeout time and is no longer being waited on, ignore + // this failure. if (result.attempt != attempts_.back().get()) { - // This attempt already timed out. Ignore it. - DCHECK_GE(result.attempt->server_index(), 0); - resolve_context_->RecordServerFailure( - result.attempt->server_index(), secure_ /* is_doh_server */, - session_.get()); return AttemptResult(ERR_IO_PENDING, nullptr); } + + resolve_context_->RecordServerFailure(result.attempt->server_index(), + secure_ /* is_doh_server */, + result.rv, session_.get()); if (!MoreAttemptsAllowed()) { return result; } diff --git a/chromium/net/dns/dns_transaction.h b/chromium/net/dns/dns_transaction.h index 7bb9501d09b..bc182ff7612 100644 --- a/chromium/net/dns/dns_transaction.h +++ b/chromium/net/dns/dns_transaction.h @@ -80,9 +80,14 @@ class NET_EXPORT_PRIVATE DnsTransactionFactory { // Called with the response or NULL if no matching response was received. // Note that the |GetDottedName()| of the response may be different than the // original |hostname| as a result of suffix search. + // + // The |doh_provider_id| contains the provider ID for histograms of the last + // DoH server attempted. If the name is unavailable, or this is not a DoH + // transaction, |doh_provider_id| is nullopt. typedef base::OnceCallback<void(DnsTransaction* transaction, int neterror, - const DnsResponse* response)> + const DnsResponse* response, + base::Optional<std::string> doh_provider_id)> CallbackType; DnsTransactionFactory(); diff --git a/chromium/net/dns/dns_transaction_unittest.cc b/chromium/net/dns/dns_transaction_unittest.cc index e3196cfc21e..17f6bf62e72 100644 --- a/chromium/net/dns/dns_transaction_unittest.cc +++ b/chromium/net/dns/dns_transaction_unittest.cc @@ -28,6 +28,7 @@ #include "net/base/port_util.h" #include "net/base/upload_bytes_element_reader.h" #include "net/base/url_util.h" +#include "net/cookies/cookie_inclusion_status.h" #include "net/cookies/cookie_util.h" #include "net/dns/dns_config.h" #include "net/dns/dns_query.h" @@ -324,7 +325,8 @@ class TransactionHelper { void OnTransactionComplete(DnsTransaction* t, int rv, - const DnsResponse* response) { + const DnsResponse* response, + base::Optional<std::string> doh_provider_id) { EXPECT_FALSE(completed_); EXPECT_EQ(transaction_.get(), t); @@ -1200,8 +1202,17 @@ TEST_F(DnsTransactionTestWithMockTime, ServerFallbackAndRotate) { EXPECT_TRUE(helper1.Run(transaction_factory_.get())); size_t kOrder[] = { - 0, 1, 2, 0, 1, // The first transaction. - 1, 2, 0, // The second transaction starts from the next server. + // The first transaction. + 0, + 1, + 2, + 0, + 1, + // The second transaction starts from the next server, and 0 is skipped + // because it already has 2 consecutive failures. + 1, + 2, + 1, }; CheckServerOrder(kOrder, base::size(kOrder)); } @@ -2178,14 +2189,15 @@ class CookieCallback { CookieCallback() : result_(false), loop_to_quit_(std::make_unique<base::RunLoop>()) {} - void SetCookieCallback(CanonicalCookie::CookieInclusionStatus result) { + void SetCookieCallback(CookieInclusionStatus result) { result_ = result.IsInclude(); loop_to_quit_->Quit(); } - void GetCookieListCallback(const net::CookieStatusList& list, - const net::CookieStatusList& excluded_cookies) { - list_ = cookie_util::StripStatuses(list); + void GetCookieListCallback( + const net::CookieAccessResultList& list, + const net::CookieAccessResultList& excluded_cookies) { + list_ = cookie_util::StripAccessResults(list); loop_to_quit_->Quit(); } @@ -2384,6 +2396,63 @@ TEST_F(DnsTransactionTest, HttpsPostLookupWithLog) { EXPECT_EQ(observer.dict_count(), 3); } +// Test for when a slow DoH response is delayed until after the initial timeout +// and no more attempts are configured. +TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_SingleAttempt) { + config_.doh_attempts = 1; + ConfigureDohServers(false /* use_post */); + + AddQueryAndTimeout(kT0HostName, kT0Qtype, + DnsQuery::PaddingStrategy::BLOCK_LENGTH_128); + + TransactionHelper helper(kT0HostName, kT0Qtype, true /* secure */, + ERR_DNS_TIMED_OUT, resolve_context_.get()); + ASSERT_FALSE(helper.Run(transaction_factory_.get())); + + // Only one attempt configured, so expect immediate failure after timeout + // period. + FastForwardBy(resolve_context_->NextDohTimeout(0 /* doh_server_index */, + session_.get())); + EXPECT_TRUE(helper.has_completed()); +} + +// Test for when a slow DoH response is delayed until after the initial timeout +// but a retry is configured. +TEST_F(DnsTransactionTestWithMockTime, SlowHttpsResponse_TwoAttempts) { + config_.doh_attempts = 2; + ConfigureDohServers(false /* use_post */); + + // Simulate a slow response by using an ERR_IO_PENDING read error to delay + // until SequencedSocketData::Resume() is called. + auto data = std::make_unique<DnsSocketData>( + 1 /* id */, kT1HostName, kT1Qtype, ASYNC, Transport::HTTPS, + nullptr /* opt_rdata */, DnsQuery::PaddingStrategy::BLOCK_LENGTH_128); + data->AddReadError(ERR_IO_PENDING, ASYNC); + data->AddResponseData(kT1ResponseDatagram, base::size(kT1ResponseDatagram), + ASYNC); + SequencedSocketData* sequenced_socket_data = data->GetProvider(); + AddSocketData(std::move(data)); + + TransactionHelper helper(kT1HostName, kT1Qtype, true /* secure */, + kT1RecordCount, resolve_context_.get()); + ASSERT_FALSE(helper.Run(transaction_factory_.get())); + ASSERT_TRUE(sequenced_socket_data->IsPaused()); + + // Another attempt configured, so transaction should not fail after initial + // timeout. Setup the second attempt to never receive a response. + AddQueryAndTimeout(kT1HostName, kT1Qtype, + DnsQuery::PaddingStrategy::BLOCK_LENGTH_128); + FastForwardBy(resolve_context_->NextDohTimeout(0 /* doh_server_index */, + session_.get())); + EXPECT_FALSE(helper.has_completed()); + + // Expect first attempt to continue in parallel with retry, so expect the + // transaction to complete when the first query is allowed to resume. + sequenced_socket_data->Resume(); + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(helper.has_completed()); +} + TEST_F(DnsTransactionTest, TCPLookup) { AddAsyncQueryAndRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeNOERROR | dns_protocol::kFlagTC); @@ -3100,8 +3169,9 @@ TEST_F(DnsTransactionTestWithMockTime, RestartFinishedProbe) { // Mark server unavailabe and restart runner. for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) { - resolve_context_->RecordServerFailure( - 0u /* server_index */, true /* is_doh_server */, session_.get()); + resolve_context_->RecordServerFailure(0u /* server_index */, + true /* is_doh_server */, ERR_FAILED, + session_.get()); } ASSERT_FALSE(resolve_context_->GetDohServerAvailability( 0u /* doh_server_index */, session_.get())); @@ -3149,8 +3219,9 @@ TEST_F(DnsTransactionTestWithMockTime, FastProbeRestart) { // becoming unavailable and might as well replecate real behavior for the // test. for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) { - resolve_context_->RecordServerFailure( - 0u /* server_index */, true /* is_doh_server */, session_.get()); + resolve_context_->RecordServerFailure(0u /* server_index */, + true /* is_doh_server */, ERR_FAILED, + session_.get()); } ASSERT_FALSE(resolve_context_->GetDohServerAvailability( 0u /* doh_server_index */, session_.get())); diff --git a/chromium/net/dns/dns_util.cc b/chromium/net/dns/dns_util.cc index 49bfdfc8323..9ad7d53637c 100644 --- a/chromium/net/dns/dns_util.cc +++ b/chromium/net/dns/dns_util.cc @@ -21,7 +21,7 @@ #include "net/base/address_list.h" #include "net/base/url_util.h" #include "net/dns/public/dns_protocol.h" -#include "net/dns/public/doh_provider_list.h" +#include "net/dns/public/doh_provider_entry.h" #include "net/dns/public/util.h" #include "net/third_party/uri_template/uri_template.h" #include "url/url_canon.h" @@ -113,21 +113,21 @@ bool DNSDomainFromDot(const base::StringPiece& dotted, return true; } -std::vector<const DohProviderEntry*> GetDohProviderEntriesFromNameservers( +DohProviderEntry::List GetDohProviderEntriesFromNameservers( const std::vector<IPEndPoint>& dns_servers, const std::vector<std::string>& excluded_providers) { - const std::vector<DohProviderEntry>& providers = GetDohProviderList(); - std::vector<const DohProviderEntry*> entries; + const DohProviderEntry::List& providers = DohProviderEntry::GetList(); + DohProviderEntry::List entries; for (const auto& server : dns_servers) { - for (const auto& entry : providers) { - if (base::Contains(excluded_providers, entry.provider)) + for (const auto* entry : providers) { + if (base::Contains(excluded_providers, entry->provider)) continue; // DoH servers should only be added once. - if (base::Contains(entry.ip_addresses, server.address()) && - !base::Contains(entries, &entry)) { - entries.push_back(&entry); + if (base::Contains(entry->ip_addresses, server.address()) && + !base::Contains(entries, entry)) { + entries.push_back(entry); } } } @@ -289,8 +289,8 @@ uint16_t DnsQueryTypeToQtype(DnsQueryType dns_query_type) { return dns_protocol::kTypePTR; case DnsQueryType::SRV: return dns_protocol::kTypeSRV; - case DnsQueryType::ESNI: - return dns_protocol::kExperimentalTypeEsniDraft4; + case DnsQueryType::INTEGRITY: + return dns_protocol::kExperimentalTypeIntegrity; } } @@ -311,23 +311,21 @@ DnsQueryType AddressFamilyToDnsQueryType(AddressFamily address_family) { std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromDotHostname( const std::string& dot_server, const std::vector<std::string>& excluded_providers) { - const std::vector<DohProviderEntry>& entries = GetDohProviderList(); std::vector<DnsOverHttpsServerConfig> doh_servers; if (dot_server.empty()) return doh_servers; - for (const auto& entry : entries) { - if (base::Contains(excluded_providers, entry.provider)) + for (const auto* entry : DohProviderEntry::GetList()) { + if (base::Contains(excluded_providers, entry->provider)) continue; - if (base::Contains(entry.dns_over_tls_hostnames, dot_server)) { + if (base::Contains(entry->dns_over_tls_hostnames, dot_server)) { std::string server_method; - CHECK(dns_util::IsValidDohTemplate(entry.dns_over_https_template, + CHECK(dns_util::IsValidDohTemplate(entry->dns_over_https_template, &server_method)); - doh_servers.push_back(DnsOverHttpsServerConfig( - entry.dns_over_https_template, server_method == "POST")); - break; + doh_servers.emplace_back(entry->dns_over_https_template, + server_method == "POST"); } } return doh_servers; @@ -336,38 +334,35 @@ std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromDotHostname( std::vector<DnsOverHttpsServerConfig> GetDohUpgradeServersFromNameservers( const std::vector<IPEndPoint>& dns_servers, const std::vector<std::string>& excluded_providers) { - std::vector<const DohProviderEntry*> entries = + const auto entries = GetDohProviderEntriesFromNameservers(dns_servers, excluded_providers); std::vector<DnsOverHttpsServerConfig> doh_servers; - std::string server_method; - for (const auto* entry : entries) { - CHECK(dns_util::IsValidDohTemplate(entry->dns_over_https_template, - &server_method)); - doh_servers.push_back(DnsOverHttpsServerConfig( - entry->dns_over_https_template, server_method == "POST")); - } + doh_servers.reserve(entries.size()); + std::transform(entries.begin(), entries.end(), + std::back_inserter(doh_servers), [](const auto* entry) { + std::string server_method; + CHECK(dns_util::IsValidDohTemplate( + entry->dns_over_https_template, &server_method)); + return DnsOverHttpsServerConfig( + entry->dns_over_https_template, server_method == "POST"); + }); return doh_servers; } std::string GetDohProviderIdForHistogramFromDohConfig( const DnsOverHttpsServerConfig& doh_server) { - const std::vector<DohProviderEntry>& entries = GetDohProviderList(); - for (const auto& entry : entries) { - if (doh_server.server_template == entry.dns_over_https_template) { - return entry.provider; - } - } - return "Other"; + const auto& entries = DohProviderEntry::GetList(); + const auto it = + std::find_if(entries.begin(), entries.end(), [&](const auto* entry) { + return entry->dns_over_https_template == doh_server.server_template; + }); + return it != entries.end() ? (*it)->provider : "Other"; } std::string GetDohProviderIdForHistogramFromNameserver( const IPEndPoint& nameserver) { - std::vector<const DohProviderEntry*> entries = - GetDohProviderEntriesFromNameservers({nameserver}, {}); - if (entries.size() == 0) - return "Other"; - else - return entries[0]->provider; + const auto entries = GetDohProviderEntriesFromNameservers({nameserver}, {}); + return entries.empty() ? "Other" : entries[0]->provider; } std::string SecureDnsModeToString( diff --git a/chromium/net/dns/dns_util_unittest.cc b/chromium/net/dns/dns_util_unittest.cc index f07c342e442..39a89880b4f 100644 --- a/chromium/net/dns/dns_util_unittest.cc +++ b/chromium/net/dns/dns_util_unittest.cc @@ -6,6 +6,7 @@ #include "base/stl_util.h" #include "net/dns/public/dns_protocol.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" namespace net { @@ -183,20 +184,24 @@ TEST_F(DNSUtilTest, GetDohUpgradeServersFromNameservers) { doh_servers = GetDohUpgradeServersFromNameservers(nameservers, std::vector<std::string>()); - EXPECT_EQ(3u, doh_servers.size()); - EXPECT_EQ("https://chrome.cloudflare-dns.com/dns-query", - doh_servers[0].server_template); - EXPECT_EQ("https://doh.cleanbrowsing.org/doh/family-filter{?dns}", - doh_servers[1].server_template); - EXPECT_EQ("https://doh.cleanbrowsing.org/doh/security-filter{?dns}", - doh_servers[2].server_template); + EXPECT_THAT( + doh_servers, + testing::ElementsAre( + DnsOverHttpsServerConfig( + "https://chrome.cloudflare-dns.com/dns-query", true), + DnsOverHttpsServerConfig( + "https://doh.cleanbrowsing.org/doh/family-filter{?dns}", false), + DnsOverHttpsServerConfig( + "https://doh.cleanbrowsing.org/doh/security-filter{?dns}", + false))); doh_servers = GetDohUpgradeServersFromNameservers( nameservers, std::vector<std::string>( {"CleanBrowsingSecure", "Cloudflare", "Unexpected"})); - EXPECT_EQ(1u, doh_servers.size()); - EXPECT_EQ("https://doh.cleanbrowsing.org/doh/family-filter{?dns}", - doh_servers[0].server_template); + EXPECT_THAT( + doh_servers, + testing::ElementsAre(DnsOverHttpsServerConfig( + "https://doh.cleanbrowsing.org/doh/family-filter{?dns}", false))); } TEST_F(DNSUtilTest, GetDohProviderIdForHistogramFromDohConfig) { diff --git a/chromium/net/dns/esni_content.cc b/chromium/net/dns/esni_content.cc deleted file mode 100644 index 014d492942b..00000000000 --- a/chromium/net/dns/esni_content.cc +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "net/dns/esni_content.h" - -namespace net { - -EsniContent::EsniContent() = default; -EsniContent::EsniContent(const EsniContent& other) { - MergeFrom(other); -} -EsniContent::EsniContent(EsniContent&& other) = default; -EsniContent& EsniContent::operator=(const EsniContent& other) { - MergeFrom(other); - return *this; -} -EsniContent& EsniContent::operator=(EsniContent&& other) = default; -EsniContent::~EsniContent() = default; - -bool operator==(const EsniContent& c1, const EsniContent& c2) { - return c1.keys() == c2.keys() && - c1.keys_for_addresses() == c2.keys_for_addresses(); -} - -const std::set<std::string, EsniContent::StringPieceComparator>& -EsniContent::keys() const { - return keys_; -} - -const std::map<IPAddress, std::set<base::StringPiece>>& -EsniContent::keys_for_addresses() const { - return keys_for_addresses_; -} - -void EsniContent::AddKey(base::StringPiece key) { - if (keys_.find(key) == keys_.end()) - keys_.insert(std::string(key)); -} - -void EsniContent::AddKeyForAddress(const IPAddress& address, - base::StringPiece key) { - auto key_it = keys_.find(key); - if (key_it == keys_.end()) { - bool key_was_added; - std::tie(key_it, key_was_added) = keys_.insert(std::string(key)); - DCHECK(key_was_added); - } - keys_for_addresses_[address].insert(base::StringPiece(*key_it)); -} - -void EsniContent::MergeFrom(const EsniContent& other) { - for (const auto& kv : other.keys_for_addresses()) { - const IPAddress& address = kv.first; - const auto& keys_for_address = kv.second; - for (base::StringPiece key : keys_for_address) - AddKeyForAddress(address, key); - } - for (const std::string& key : other.keys()) - AddKey(key); -} - -} // namespace net diff --git a/chromium/net/dns/esni_content.h b/chromium/net/dns/esni_content.h deleted file mode 100644 index 8c6a8e0d0cc..00000000000 --- a/chromium/net/dns/esni_content.h +++ /dev/null @@ -1,84 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef NET_DNS_ESNI_CONTENT_H_ -#define NET_DNS_ESNI_CONTENT_H_ - -#include <map> -#include <set> -#include <string> - -#include "base/strings/string_piece.h" -#include "net/base/ip_address.h" -#include "net/base/net_export.h" - -namespace net { - -// An EsniContent struct represents an aggregation of the -// content of several ESNI (TLS 1.3 Encrypted Server Name Indication, -// draft 4) resource records. -// -// This aggregation contains: -// (1) The ESNI key objects from each of the ESNI records, and -// (2) A collection of IP addresses, each of which is associated -// with one or more of the key objects. (Each key will likely also -// be associated with several destination addresses.) -class NET_EXPORT EsniContent { - public: - EsniContent(); - EsniContent(const EsniContent& other); - EsniContent(EsniContent&& other); - EsniContent& operator=(const EsniContent& other); - EsniContent& operator=(EsniContent&& other); - ~EsniContent(); - - // Key objects (which might be up to ~50K in length) are stored - // in a collection of std::string; use transparent comparison - // to allow checking whether a given base::StringPiece is in - // the collection without making copies. - struct StringPieceComparator { - using is_transparent = int; - - bool operator()(const base::StringPiece lhs, - const base::StringPiece rhs) const { - return lhs < rhs; - } - }; - - const std::set<std::string, StringPieceComparator>& keys() const; - const std::map<IPAddress, std::set<base::StringPiece>>& keys_for_addresses() - const; - - // Adds |key| (if it is not already stored) without associating it - // with any particular addresss; if this addition is performed, it - // copies the underlying string. - void AddKey(base::StringPiece key); - - // Associates a key with an address, copying the underlying string to - // the internal collection of keys if it is not already stored. - void AddKeyForAddress(const IPAddress& address, base::StringPiece key); - - // Merges the contents of |other|: - // 1. unions the collection of stored keys with |other.keys()| and - // 2. unions the stored address-key associations with - // |other.keys_for_addresses()|. - void MergeFrom(const EsniContent& other); - - private: - // In order to keep the StringPieces in |keys_for_addresses_| valid, - // |keys_| must be of a collection type guaranteeing stable pointers. - std::set<std::string, StringPieceComparator> keys_; - - std::map<IPAddress, std::set<base::StringPiece>> keys_for_addresses_; -}; - -// Two EsniContent structs are equal if they have the same set of keys, the -// same set of IP addresses, and the same subset of the keys corresponding to -// each IP address. -NET_EXPORT_PRIVATE -bool operator==(const EsniContent& c1, const EsniContent& c2); - -} // namespace net - -#endif // NET_DNS_ESNI_CONTENT_H_ diff --git a/chromium/net/dns/esni_content_unittest.cc b/chromium/net/dns/esni_content_unittest.cc deleted file mode 100644 index 50bc5d81dca..00000000000 --- a/chromium/net/dns/esni_content_unittest.cc +++ /dev/null @@ -1,170 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "net/dns/esni_content.h" - -#include "base/strings/string_number_conversions.h" - -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace net { - -namespace { - -IPAddress MakeIPAddress() { - // Introduce some (deterministic) variation in the IP addresses - // generated. - static uint8_t next_octet = 0; - next_octet += 4; - - return IPAddress(next_octet, next_octet + 1, next_octet + 2, next_octet + 3); -} - -// Make sure we can add keys. -TEST(EsniContentTest, AddKey) { - EsniContent c1; - c1.AddKey("a"); - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a")); - c1.AddKey("a"); - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a")); - c1.AddKey("b"); - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a", "b")); -} - -// Make sure we can add key-address pairs. -TEST(EsniContentTest, AddKeyForAddress) { - EsniContent c1; - auto address = MakeIPAddress(); - c1.AddKeyForAddress(address, "a"); - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a")); - EXPECT_THAT(c1.keys_for_addresses(), - ::testing::UnorderedElementsAre(::testing::Pair( - address, ::testing::UnorderedElementsAre("a")))); -} - -TEST(EsniContentTest, AssociateAddressWithExistingKey) { - EsniContent c1; - auto address = MakeIPAddress(); - c1.AddKey("a"); - c1.AddKeyForAddress(address, "a"); - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a")); - EXPECT_THAT(c1.keys_for_addresses(), - ::testing::UnorderedElementsAre(::testing::Pair( - address, ::testing::UnorderedElementsAre("a")))); -} - -// Merging to an empty EsniContent should make the result equal the source of -// the merge. -TEST(EsniContentTest, MergeToEmpty) { - EsniContent c1; - c1.AddKey("c"); - IPAddress address = MakeIPAddress(); - - c1.AddKeyForAddress(address, "a"); - c1.AddKeyForAddress(address, "b"); - EsniContent empty; - empty.MergeFrom(c1); - EXPECT_EQ(c1, empty); -} - -TEST(EsniContentTest, MergeFromEmptyNoOp) { - EsniContent c1, c2; - c1.AddKey("a"); - c2.AddKey("a"); - EsniContent empty; - c1.MergeFrom(empty); - EXPECT_EQ(c1, c2); -} - -// Test that merging multiple keys corresponding to a single address works. -TEST(EsniContentTest, MergeKeysForSingleHost) { - EsniContent c1, c2; - IPAddress address = MakeIPAddress(); - - c1.AddKeyForAddress(address, "a"); - c1.AddKeyForAddress(address, "b"); - c2.AddKeyForAddress(address, "b"); - c2.AddKeyForAddress(address, "c"); - c1.MergeFrom(c2); - - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a", "b", "c")); - EXPECT_THAT(c1.keys_for_addresses(), - ::testing::UnorderedElementsAre(::testing::Pair( - address, ::testing::UnorderedElementsAre("a", "b", "c")))); -} - -// Test that merging multiple addresss corresponding to a single key works. -TEST(EsniContentTest, MergeHostsForSingleKey) { - EsniContent c1, c2; - IPAddress address = MakeIPAddress(); - IPAddress second_address = MakeIPAddress(); - c1.AddKeyForAddress(address, "a"); - c2.AddKeyForAddress(second_address, "a"); - c1.MergeFrom(c2); - - EXPECT_THAT(c1.keys(), ::testing::UnorderedElementsAre("a")); - EXPECT_THAT( - c1.keys_for_addresses(), - ::testing::UnorderedElementsAre( - ::testing::Pair(address, ::testing::UnorderedElementsAre("a")), - ::testing::Pair(second_address, - ::testing::UnorderedElementsAre("a")))); -} - -// Test merging some more complex instances of the class. -TEST(EsniContentTest, MergeSeveralHostsAndKeys) { - EsniContent c1, c2, expected; - for (int i = 0; i < 50; ++i) { - IPAddress address = MakeIPAddress(); - std::string key = base::NumberToString(i); - switch (i % 3) { - case 0: - c1.AddKey(key); - expected.AddKey(key); - break; - case 1: - c2.AddKey(key); - expected.AddKey(key); - break; - } - // Associate each address with a subset of the keys seen so far - { - int j = 0; - for (auto key : c1.keys()) { - if (j % 2) { - c1.AddKeyForAddress(address, key); - expected.AddKeyForAddress(address, key); - } - ++j; - } - } - { - int j = 0; - for (auto key : c2.keys()) { - if (j % 3 == 1) { - c2.AddKeyForAddress(address, key); - expected.AddKeyForAddress(address, key); - } - ++j; - } - } - } - { - EsniContent merge_dest = c1; - EsniContent merge_src = c2; - merge_dest.MergeFrom(merge_src); - EXPECT_EQ(merge_dest, expected); - } - { - EsniContent merge_dest = c2; - EsniContent merge_src = c1; - merge_dest.MergeFrom(merge_src); - EXPECT_EQ(merge_dest, expected); - } -} - -} // namespace - -} // namespace net diff --git a/chromium/net/dns/host_cache.cc b/chromium/net/dns/host_cache.cc index 647d44ae427..70805898089 100644 --- a/chromium/net/dns/host_cache.cc +++ b/chromium/net/dns/host_cache.cc @@ -13,6 +13,7 @@ #include "base/strings/string_number_conversions.h" #include "base/time/default_tick_clock.h" #include "base/trace_event/trace_event.h" +#include "net/base/address_family.h" #include "net/base/ip_endpoint.h" #include "net/base/trace_constants.h" #include "net/dns/host_resolver.h" @@ -47,7 +48,6 @@ const char kAddressesKey[] = "addresses"; const char kTextRecordsKey[] = "text_records"; const char kHostnameResultsKey[] = "hostname_results"; const char kHostPortsKey[] = "host_ports"; -const char kEsniDataKey[] = "esni_data"; bool AddressListFromListValue(const base::ListValue* value, base::Optional<AddressList>* out_list) { @@ -69,67 +69,6 @@ bool AddressListFromListValue(const base::ListValue* value, return true; } -// Serializes the cache's ESNI content as -// { -// key 0: [addresses for key 0], -// ..., -// key N: [address for key N] -// } -base::Value EsniContentToValue(const EsniContent& content) { - base::Value addresses_for_keys_value(base::Value::Type::DICTIONARY); - - for (const auto& key : content.keys()) { - addresses_for_keys_value.SetKey(key, base::Value(base::Value::Type::LIST)); - } - - for (const auto& kv : content.keys_for_addresses()) { - const IPAddress& address = kv.first; - const auto& keys_for_address = kv.second; - for (base::StringPiece key : keys_for_address) { - base::Value* addresses_for_key = addresses_for_keys_value.FindKey(key); - DCHECK(addresses_for_key); - addresses_for_key->Append(address.ToString()); - } - } - - return addresses_for_keys_value; -} - -bool EsniContentFromValue(const base::Value& esni_content_value, - base::Optional<EsniContent>* out_esni_content) { - EsniContent content_for_cache; - - // The esni_data cache member is encoded as a - // { key: list of associated IP addresses } dictionary. - if (!esni_content_value.is_dict()) - return false; - - for (const auto& kv : esni_content_value.DictItems()) { - const std::string& key = kv.first; - const base::Value& serialized_addresses = kv.second; - if (!serialized_addresses.is_list()) - return false; - if (serialized_addresses.GetList().empty()) { - content_for_cache.AddKey(key); - } else { - for (const base::Value& serialized_address_value : - serialized_addresses.GetList()) { - if (!serialized_address_value.is_string()) - return false; - const std::string& serialized_address = - serialized_address_value.GetString(); - IPAddress address; - if (!address.AssignFromIPLiteral(serialized_address)) - return false; - content_for_cache.AddKeyForAddress(address, key); - } - } - } - - *out_esni_content = std::move(content_for_cache); - return true; -} - template <typename T> void MergeLists(base::Optional<T>* target, const base::Optional<T>& source) { if (target->has_value() && source) { @@ -220,11 +159,7 @@ HostCache::Entry HostCache::Entry::MergeEntries(Entry front, Entry back) { front.MergeAddressesFrom(back); MergeLists(&front.text_records_, back.text_records()); MergeLists(&front.hostnames_, back.hostnames()); - if (back.esni_data_ && !front.esni_data_) { - front.esni_data_ = std::move(back.esni_data_); - } else if (front.esni_data_ && back.esni_data_) { - front.esni_data_->MergeFrom(back.esni_data_.value()); - } + MergeLists(&front.integrity_data_, back.integrity_data()); // Use canonical name from |back| iff empty in |front|. if (front.addresses() && front.addresses().value().canonical_name().empty() && @@ -298,7 +233,7 @@ HostCache::Entry::Entry(const HostCache::Entry& entry, addresses_(entry.addresses()), text_records_(entry.text_records()), hostnames_(entry.hostnames()), - esni_data_(entry.esni_data()), + integrity_data_(entry.integrity_data()), source_(entry.source()), ttl_(entry.ttl()), expires_(now + ttl), @@ -308,7 +243,7 @@ HostCache::Entry::Entry(int error, const base::Optional<AddressList>& addresses, base::Optional<std::vector<std::string>>&& text_records, base::Optional<std::vector<HostPortPair>>&& hostnames, - base::Optional<EsniContent>&& esni_data, + base::Optional<std::vector<bool>>&& integrity_data, Source source, base::TimeTicks expires, int network_changes) @@ -316,11 +251,15 @@ HostCache::Entry::Entry(int error, addresses_(addresses), text_records_(std::move(text_records)), hostnames_(std::move(hostnames)), - esni_data_(std::move(esni_data)), + integrity_data_(std::move(integrity_data)), source_(source), expires_(expires), network_changes_(network_changes) {} +void HostCache::Entry::PrepareForCacheInsertion() { + integrity_data_.reset(); +} + bool HostCache::Entry::IsStale(base::TimeTicks now, int network_changes) const { EntryStaleness stale; stale.expired_by = now - expires_; @@ -355,28 +294,11 @@ void HostCache::Entry::MergeAddressesFrom(const HostCache::Entry& source) { addresses_->Deduplicate(); - auto has_keys = [&](const IPEndPoint& e) { - return (esni_data() && - esni_data()->keys_for_addresses().count(e.address())) || - (source.esni_data() && - source.esni_data()->keys_for_addresses().count(e.address())); - }; - std::stable_sort(addresses_->begin(), addresses_->end(), - [&](const IPEndPoint& lhs, const IPEndPoint& rhs) { - // Prefer an address with ESNI keys to one without; - // break ties by address family. - - // Store one lookup's result to avoid repeating the lookup. - bool lhs_has_keys = has_keys(lhs); - if (lhs_has_keys != has_keys(rhs)) - return lhs_has_keys; - - if ((lhs.GetFamily() == ADDRESS_FAMILY_IPV6) != - (rhs.GetFamily() == ADDRESS_FAMILY_IPV6)) - return (lhs.GetFamily() == ADDRESS_FAMILY_IPV6); - - return false; + [](const IPEndPoint& lhs, const IPEndPoint& rhs) { + // Return true iff |lhs < rhs|. + return lhs.GetFamily() == ADDRESS_FAMILY_IPV6 && + rhs.GetFamily() == ADDRESS_FAMILY_IPV4; }); } @@ -433,10 +355,6 @@ base::DictionaryValue HostCache::Entry::GetAsValue( entry_dict.SetKey(kHostnameResultsKey, std::move(hostnames_value)); entry_dict.SetKey(kHostPortsKey, std::move(host_ports_value)); } - - if (esni_data()) { - entry_dict.SetKey(kEsniDataKey, EsniContentToValue(*esni_data())); - } } return entry_dict; @@ -632,7 +550,9 @@ void HostCache::Set(const Key& key, EvictOneEntry(now); } - AddEntry(key, Entry(entry, now, ttl, network_changes_)); + Entry entry_for_cache(entry, now, ttl, network_changes_); + entry_for_cache.PrepareForCacheInsertion(); + AddEntry(key, std::move(entry_for_cache)); if (delegate_ && result_changed) delegate_->ScheduleWrite(); @@ -801,7 +721,6 @@ bool HostCache::RestoreFromListValue(const base::ListValue& old_cache) { const base::ListValue* text_records_value = nullptr; const base::ListValue* hostname_records_value = nullptr; const base::ListValue* host_ports_value = nullptr; - const base::Value* esni_content_value = nullptr; if (!entry_dict->GetInteger(kErrorKey, &error)) { entry_dict->GetList(kAddressesKey, &addresses_value); entry_dict->GetList(kTextRecordsKey, &text_records_value); @@ -810,8 +729,6 @@ bool HostCache::RestoreFromListValue(const base::ListValue& old_cache) { entry_dict->GetList(kHostPortsKey, &host_ports_value)) { return false; } - - entry_dict->Get(kEsniDataKey, &esni_content_value); } int64_t time_internal; @@ -860,15 +777,12 @@ bool HostCache::RestoreFromListValue(const base::ListValue& old_cache) { } } - base::Optional<EsniContent> esni_content; - if (esni_content_value && - !EsniContentFromValue(*esni_content_value, &esni_content)) { - return false; - } + // We do not intend to serialize INTEGRITY records with the host cache. + base::Optional<std::vector<bool>> integrity_data; // Assume an empty address list if we have an address type and no results. if (IsAddressType(dns_query_type) && !address_list && !text_records && - !hostname_records && !esni_content) { + !hostname_records) { address_list.emplace(); } @@ -882,9 +796,9 @@ bool HostCache::RestoreFromListValue(const base::ListValue& old_cache) { auto found = entries_.find(key); if (found == entries_.end()) { AddEntry(key, Entry(error, address_list, std::move(text_records), - std::move(hostname_records), std::move(esni_content), - Entry::SOURCE_UNKNOWN, expiration_time, - network_changes_ - 1)); + std::move(hostname_records), + std::move(integrity_data), Entry::SOURCE_UNKNOWN, + expiration_time, network_changes_ - 1)); restore_size_++; } } diff --git a/chromium/net/dns/host_cache.h b/chromium/net/dns/host_cache.h index 11f92932edd..a74f7dc5ba9 100644 --- a/chromium/net/dns/host_cache.h +++ b/chromium/net/dns/host_cache.h @@ -15,8 +15,8 @@ #include <utility> #include <vector> +#include "base/check.h" #include "base/gtest_prod_util.h" -#include "base/logging.h" #include "base/macros.h" #include "base/numerics/clamped_math.h" #include "base/optional.h" @@ -31,7 +31,6 @@ #include "net/base/net_export.h" #include "net/base/network_isolation_key.h" #include "net/dns/dns_util.h" -#include "net/dns/esni_content.h" #include "net/dns/host_resolver_source.h" #include "net/dns/public/dns_query_type.h" #include "net/log/net_log_capture_mode.h" @@ -161,10 +160,13 @@ class NET_EXPORT HostCache { void set_hostnames(base::Optional<std::vector<HostPortPair>> hostnames) { hostnames_ = std::move(hostnames); } - const base::Optional<EsniContent>& esni_data() const { return esni_data_; } - void set_esni_data(base::Optional<EsniContent> esni_data) { - esni_data_ = std::move(esni_data); + const base::Optional<std::vector<bool>>& integrity_data() const { + return integrity_data_; } + void set_integrity_data(base::Optional<std::vector<bool>> integrity_data) { + integrity_data_ = std::move(integrity_data); + } + Source source() const { return source_; } bool has_ttl() const { return ttl_ >= base::TimeDelta(); } base::TimeDelta ttl() const { return ttl_; } @@ -176,16 +178,14 @@ class NET_EXPORT HostCache { // Public for the net-internals UI. int network_changes() const { return network_changes_; } - // Merge |front| and |back|, representing results from multiple - // transactions for the same overall host resolution query. + // Merge |front| and |back|, representing results from multiple transactions + // for the same overall host resolution query. // - // - When merging result hostname and text record lists, result - // elements from |front| will be merged in front of elements from |back|. - // - Merging address lists deduplicates addresses and sorts them in a stable - // manner by (breaking ties by continuing down the list): - // 1. Addresses with associated ESNI keys precede addresses without - // 2. IPv6 addresses precede IPv4 addresses - // - Fields that cannot be merged take precedence from |front|. + // Merges lists, placing elements from |front| before elements from |back|. + // Further, dedupes address lists and moves IPv6 addresses before IPv4 + // addresses (maintaining stable order otherwise). + // + // Fields that cannot be merged take precedence from |front|. static Entry MergeEntries(Entry front, Entry back); // Creates a value representation of the entry for use with NetLog. @@ -207,11 +207,13 @@ class NET_EXPORT HostCache { const base::Optional<AddressList>& addresses, base::Optional<std::vector<std::string>>&& text_results, base::Optional<std::vector<HostPortPair>>&& hostnames, - base::Optional<EsniContent>&& esni_data, + base::Optional<std::vector<bool>>&& integrity_data, Source source, base::TimeTicks expires, int network_changes); + void PrepareForCacheInsertion(); + void SetResult(AddressList addresses) { addresses_ = std::move(addresses); } void SetResult(std::vector<std::string> text_records) { text_records_ = std::move(text_records); @@ -219,7 +221,9 @@ class NET_EXPORT HostCache { void SetResult(std::vector<HostPortPair> hostnames) { hostnames_ = std::move(hostnames); } - void SetResult(EsniContent esni_data) { esni_data_ = std::move(esni_data); } + void SetResult(std::vector<bool> integrity_data) { + integrity_data_ = std::move(integrity_data); + } int total_hits() const { return total_hits_; } int stale_hits() const { return stale_hits_; } @@ -230,20 +234,11 @@ class NET_EXPORT HostCache { int network_changes, EntryStaleness* out) const; - // Combines the addresses of |source| with those already stored, - // resulting in the following order: - // - // 1. IPv6 addresses associated with ESNI keys - // 2. IPv4 addresses associated with ESNI keys - // 3. IPv6 addresses not associated with ESNI keys - // 4. IPv4 addresses not associated with ESNI keys - // - // - Conducts the merge in a stable fashion (other things equal, addresses - // from |*this| will precede those from |source|, and addresses earlier in - // one entry's list will precede other addresses from later in the same - // list). - // - Deduplicates the entries during the merge so that |*this|'s - // address list will not contain duplicates after the call. + // Merges addresses from |source| into the stored list of addresses and + // deduplicates. The address list can be accessed with |addresses()|. This + // method performs a stable sort to ensure IPv6 addresses precede IPv4 + // addresses. IP versions being equal, addresses from |*this| will precede + // those from |source|. void MergeAddressesFrom(const HostCache::Entry& source); base::DictionaryValue GetAsValue(bool include_staleness) const; @@ -253,7 +248,7 @@ class NET_EXPORT HostCache { base::Optional<AddressList> addresses_; base::Optional<std::vector<std::string>> text_records_; base::Optional<std::vector<HostPortPair>> hostnames_; - base::Optional<EsniContent> esni_data_; + base::Optional<std::vector<bool>> integrity_data_; // Where results were obtained (e.g. DNS lookup, hosts file, etc). Source source_ = SOURCE_UNKNOWN; // TTL obtained from the nameserver. Negative if unknown. diff --git a/chromium/net/dns/host_cache_unittest.cc b/chromium/net/dns/host_cache_unittest.cc index b78a51f854c..61939a71a2c 100644 --- a/chromium/net/dns/host_cache_unittest.cc +++ b/chromium/net/dns/host_cache_unittest.cc @@ -939,7 +939,6 @@ TEST(HostCacheTest, SerializeAndDeserialize) { EXPECT_TRUE(result1->first.secure); ASSERT_TRUE(result1->second.addresses()); EXPECT_FALSE(result1->second.text_records()); - EXPECT_FALSE(result1->second.esni_data()); EXPECT_FALSE(result1->second.hostnames()); EXPECT_EQ(1u, result1->second.addresses().value().size()); EXPECT_EQ(address_ipv4, @@ -1106,105 +1105,6 @@ TEST(HostCacheTest, SerializeAndDeserialize_Text) { EXPECT_EQ(text_records, result->second.text_records().value()); } -TEST(HostCacheTest, SerializeAndDeserialize_Esni) { - base::TimeTicks now; - - base::TimeDelta ttl = base::TimeDelta::FromSeconds(99); - HostCache::Key key("example.com", DnsQueryType::A, 0, HostResolverSource::DNS, - NetworkIsolationKey()); - key.secure = true; - - const std::string kEsniKey = "a"; - const std::string kAddresslessEsniKey = "b"; - const IPAddress kAddressBack(0x20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0); - EsniContent esni_content; - esni_content.AddKeyForAddress(kAddressBack, kEsniKey); - esni_content.AddKey(kAddresslessEsniKey); - HostCache::Entry entry(OK, esni_content, HostCache::Entry::SOURCE_DNS, ttl); - ASSERT_TRUE(entry.esni_data()); - - HostCache cache(kMaxCacheEntries); - cache.Set(key, entry, now, ttl); - EXPECT_EQ(1u, cache.size()); - - base::ListValue serialized_cache; - cache.GetAsListValue(&serialized_cache, false /* include_staleness */, - HostCache::SerializationType::kRestorable); - HostCache restored_cache(kMaxCacheEntries); - restored_cache.RestoreFromListValue(serialized_cache); - - ASSERT_EQ(1u, restored_cache.size()); - HostCache::EntryStaleness staleness; - const std::pair<const HostCache::Key, HostCache::Entry>* result = - restored_cache.LookupStale(key, now, &staleness); - ASSERT_TRUE(result); - EXPECT_TRUE(result->first.secure); - - EXPECT_FALSE(result->second.addresses()); - EXPECT_FALSE(result->second.text_records()); - EXPECT_FALSE(result->second.hostnames()); - EXPECT_THAT(result->second.esni_data(), Optional(esni_content)); -} - -class HostCacheMalformedEsniSerializationTest : public ::testing::Test { - public: - HostCacheMalformedEsniSerializationTest() - : serialized_cache_(), - // We'll only need one entry. - restored_cache_(1) {} - - protected: - void SetUp() override { - base::TimeTicks now; - - base::TimeDelta ttl = base::TimeDelta::FromSeconds(99); - HostCache::Key key("example.com", DnsQueryType::A, 0, - HostResolverSource::DNS, NetworkIsolationKey()); - key.secure = true; - - const std::string esni_key = "a"; - const IPAddress kAddressBack(0x20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0); - EsniContent esni_content; - esni_content.AddKeyForAddress(kAddressBack, esni_key); - HostCache::Entry entry(OK, esni_content, HostCache::Entry::SOURCE_DNS, ttl); - ASSERT_TRUE(entry.esni_data()); - HostCache cache(kMaxCacheEntries); - cache.Set(key, entry, now, ttl); - EXPECT_EQ(1u, cache.size()); - cache.GetAsListValue(&serialized_cache_, true /* include_staleness */, - HostCache::SerializationType::kRestorable); - } - - base::ListValue serialized_cache_; - HostCache restored_cache_; -}; - -// The key corresponds to kEsniDataKey from host_cache.cc. -const char kEsniDataKey[] = "esni_data"; - -TEST_F(HostCacheMalformedEsniSerializationTest, RejectsNonDictElement) { - base::Value non_dict_element(base::Value::Type::LIST); - - base::Value::ListStorage cache_entries = serialized_cache_.TakeList(); - cache_entries[0].SetKey(kEsniDataKey, std::move(non_dict_element)); - serialized_cache_ = base::ListValue(std::move(cache_entries)); - - EXPECT_FALSE(restored_cache_.RestoreFromListValue(serialized_cache_)); -} - -TEST_F(HostCacheMalformedEsniSerializationTest, RejectsNonStringAddress) { - base::Value dict_with_non_string_value(base::Value::Type::DICTIONARY); - dict_with_non_string_value.SetKey("a", base::Value(1)); - - base::Value::ListStorage cache_entries = serialized_cache_.TakeList(); - cache_entries[0].SetKey(kEsniDataKey, std::move(dict_with_non_string_value)); - serialized_cache_ = base::ListValue(std::move(cache_entries)); - - EXPECT_FALSE(restored_cache_.RestoreFromListValue(serialized_cache_)); -} - TEST(HostCacheTest, SerializeAndDeserialize_Hostname) { base::TimeTicks now; @@ -1234,7 +1134,6 @@ TEST(HostCacheTest, SerializeAndDeserialize_Hostname) { EXPECT_FALSE(result->first.secure); EXPECT_FALSE(result->second.addresses()); EXPECT_FALSE(result->second.text_records()); - EXPECT_FALSE(result->second.esni_data()); ASSERT_TRUE(result->second.hostnames()); EXPECT_EQ(hostnames, result->second.hostnames().value()); } @@ -1394,136 +1293,25 @@ TEST(HostCacheTest, SortsAndDeduplicatesAddresses) { ElementsAreArray(MakeEndpoints({"::3", "0.0.0.1", "0.0.0.2"}))))); } -TEST(HostCacheTest, PrefersAddressesWithEsniContent) { - IPAddressList front_addresses = MakeIPList({"0.0.0.2", "0.0.0.4"}); +TEST(HostCacheTest, PrefersAddressesWithIpv6) { + IPAddressList front_addresses = MakeIPList({"::1", "0.0.0.2", "0.0.0.4"}); IPAddressList back_addresses = MakeIPList({"0.0.0.2", "0.0.0.2", "::3", "::3", "0.0.0.4"}); - EsniContent esni_content_front, esni_content_back; - esni_content_front.AddKeyForAddress(MakeIP("0.0.0.4"), "key for 0.0.0.4"); - esni_content_back.AddKeyForAddress(MakeIP("::3"), "key for ::3"); - HostCache::Entry front( OK, AddressList::CreateFromIPAddressList(front_addresses, "front"), HostCache::Entry::SOURCE_DNS); - front.set_esni_data(esni_content_front); HostCache::Entry back( OK, AddressList::CreateFromIPAddressList(back_addresses, "back"), HostCache::Entry::SOURCE_DNS); - back.set_esni_data(esni_content_back); - - HostCache::Entry result = - HostCache::Entry::MergeEntries(std::move(front), std::move(back)); - - EXPECT_THAT( - result.addresses(), - Optional(Property( - &AddressList::endpoints, - ElementsAreArray(MakeEndpoints({"::3", "0.0.0.4", "0.0.0.2"}))))); - - EXPECT_THAT(result.esni_data(), - Optional(Property( - &EsniContent::keys_for_addresses, - UnorderedElementsAre( - Pair(MakeIP("::3"), UnorderedElementsAre("key for ::3")), - Pair(MakeIP("0.0.0.4"), - UnorderedElementsAre("key for 0.0.0.4")))))); -} - -TEST(HostCacheTest, MergesManyEntriesWithEsniContent) { - IPAddressList front_addresses, back_addresses; - EsniContent esni_content_front, esni_content_back; - - // Add several IPv4 and IPv6 addresses to both the front and - // back ESNI structs and address_lists, and associate some of each - // with ESNI keys. - const std::string ipv4_prefix = "1.2.3.", ipv6_prefix = "::"; - for (int i = 0; i < 50; ++i) { - IPAddress next = - MakeIP((i % 2 ? ipv4_prefix : ipv6_prefix) + base::NumberToString(i)); - bool is_front = !!(i % 3); - if (is_front) { - front_addresses.push_back(next); - } else { - back_addresses.push_back(next); - } - if (i % 5) { - std::string key = base::NumberToString(i % 5); - if (is_front) { - esni_content_front.AddKeyForAddress(next, key); - } else { - esni_content_back.AddKeyForAddress(next, key); - } - } - } - - HostCache::Entry front( - OK, - AddressList::CreateFromIPAddressList(front_addresses, "front_canonname"), - HostCache::Entry::SOURCE_DNS); - front.set_esni_data(esni_content_front); - - HostCache::Entry back( - OK, - AddressList::CreateFromIPAddressList(back_addresses, "back_canonname"), - HostCache::Entry::SOURCE_DNS); - back.set_esni_data(esni_content_back); HostCache::Entry result = HostCache::Entry::MergeEntries(std::move(front), std::move(back)); - ASSERT_TRUE(result.addresses()); - EXPECT_EQ(result.addresses()->canonical_name(), "front_canonname"); - - EXPECT_EQ(result.addresses()->size(), - std::set<IPEndPoint>(result.addresses()->begin(), - result.addresses()->end()) - .size()) - << "Addresses should have been deduplicated."; - - ASSERT_TRUE(result.esni_data()); - - auto has_keys = [&](const IPEndPoint& e) { - return !!result.esni_data()->keys_for_addresses().count(e.address()); - }; - - // Helper for determining whether the resulting addresses are correctly - // ordered. Returns true if it's an error for |e2| to come before |e1| in - // *results.addresses(). - auto address_must_precede = [&](const IPEndPoint& e1, - const IPEndPoint& e2) -> bool { - if (has_keys(e1) != has_keys(e2)) { - return has_keys(e1) && !has_keys(e2); - } - if (e1.address().IsIPv6() != e2.address().IsIPv6()) { - return e1.address().IsIPv6() && !e2.address().IsIPv6(); - } - - // If e1 and e2 were in the same input entry, and they're otherwise - // tied in the precedence ordering, then their order in the input entry - // should be preserved in the output. - bool e1_in_front = base::Contains(front_addresses, e1.address()); - bool e2_in_front = base::Contains(front_addresses, e2.address()); - bool e1_in_back = base::Contains(back_addresses, e1.address()); - bool e2_in_back = base::Contains(back_addresses, e2.address()); - if (e1_in_front == e2_in_front && e1_in_front != e1_in_back && - e2_in_front != e2_in_back) { - const IPAddressList common_list = - e1_in_front ? front_addresses : back_addresses; - return std::find(common_list.begin(), common_list.end(), e1.address()) < - std::find(common_list.begin(), common_list.end(), e2.address()); - } - return false; - }; - - for (size_t i = 0; i < result.addresses()->size() - 1; ++i) { - EXPECT_FALSE(address_must_precede((*result.addresses())[i + 1], - (*result.addresses())[i])); - } - - auto esni_content_merged = esni_content_front; - esni_content_merged.MergeFrom(esni_content_back); - EXPECT_THAT(result.esni_data(), Optional(esni_content_merged)); + EXPECT_THAT(result.addresses(), + Optional(Property(&AddressList::endpoints, + ElementsAreArray(MakeEndpoints( + {"::1", "::3", "0.0.0.2", "0.0.0.4"}))))); } TEST(HostCacheTest, MergeEntries_frontEmpty) { @@ -1536,10 +1324,6 @@ TEST(HostCacheTest, MergeEntries_frontEmpty) { HostCache::Entry::SOURCE_DNS, base::TimeDelta::FromHours(4)); back.set_text_records(std::vector<std::string>{"text2"}); - EsniContent esni_content_back; - const std::string esni_key = "a"; - esni_content_back.AddKeyForAddress(kAddressBack, esni_key); - back.set_esni_data(esni_content_back); const HostPortPair kHostnameBack("host", 2); back.set_hostnames(std::vector<HostPortPair>{kHostnameBack}); @@ -1554,7 +1338,6 @@ TEST(HostCacheTest, MergeEntries_frontEmpty) { ElementsAre(kEndpointBack)); EXPECT_THAT(result.text_records(), Optional(ElementsAre("text2"))); EXPECT_THAT(result.hostnames(), Optional(ElementsAre(kHostnameBack))); - EXPECT_THAT(result.esni_data(), Optional(esni_content_back)); EXPECT_EQ(base::TimeDelta::FromHours(4), result.ttl()); } @@ -1566,10 +1349,6 @@ TEST(HostCacheTest, MergeEntries_backEmpty) { HostCache::Entry::SOURCE_DNS, base::TimeDelta::FromMinutes(5)); front.set_text_records(std::vector<std::string>{"text1"}); - EsniContent esni_content_front; - const std::string esni_key = "a"; - esni_content_front.AddKeyForAddress(kAddressFront, esni_key); - front.set_esni_data(esni_content_front); const HostPortPair kHostnameFront("host", 1); front.set_hostnames(std::vector<HostPortPair>{kHostnameFront}); @@ -1586,7 +1365,6 @@ TEST(HostCacheTest, MergeEntries_backEmpty) { ElementsAre(kEndpointFront)); EXPECT_THAT(result.text_records(), Optional(ElementsAre("text1"))); EXPECT_THAT(result.hostnames(), Optional(ElementsAre(kHostnameFront))); - EXPECT_THAT(result.esni_data(), Optional(esni_content_front)); EXPECT_EQ(base::TimeDelta::FromMinutes(5), result.ttl()); } @@ -1604,7 +1382,6 @@ TEST(HostCacheTest, MergeEntries_bothEmpty) { EXPECT_FALSE(result.addresses()); EXPECT_FALSE(result.text_records()); EXPECT_FALSE(result.hostnames()); - EXPECT_FALSE(result.esni_data()); EXPECT_FALSE(result.has_ttl()); } diff --git a/chromium/net/dns/host_resolver.cc b/chromium/net/dns/host_resolver.cc index 4d7dee3f90c..2fb635a7a28 100644 --- a/chromium/net/dns/host_resolver.cc +++ b/chromium/net/dns/host_resolver.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/check.h" +#include "base/immediate_crash.h" #include "base/macros.h" #include "base/no_destructor.h" #include "base/notreached.h" @@ -56,11 +57,6 @@ class FailingRequestImpl : public HostResolver::ResolveHostRequest, return *nullopt_result; } - const base::Optional<EsniContent>& GetEsniResults() const override { - static const base::NoDestructor<base::Optional<EsniContent>> nullopt_result; - return *nullopt_result; - } - ResolveErrorInfo GetResolveErrorInfo() const override { return ResolveErrorInfo(error_); } @@ -80,6 +76,11 @@ class FailingRequestImpl : public HostResolver::ResolveHostRequest, } // namespace +const base::Optional<std::vector<bool>>& +HostResolver::ResolveHostRequest::GetIntegrityResultsForTesting() const { + IMMEDIATE_CRASH(); +} + const size_t HostResolver::ManagerOptions::kDefaultRetryAttempts = static_cast<size_t>(-1); @@ -107,14 +108,6 @@ HostResolver::ResolveHostParameters::ResolveHostParameters( HostResolver::~HostResolver() = default; -std::unique_ptr<HostResolver::ResolveHostRequest> HostResolver::CreateRequest( - const HostPortPair& host, - const NetLogWithSource& net_log, - const base::Optional<ResolveHostParameters>& optional_parameters) { - return CreateRequest(host, NetworkIsolationKey(), net_log, - optional_parameters); -} - std::unique_ptr<HostResolver::ProbeRequest> HostResolver::CreateDohProbeRequest() { // Should be overridden in any HostResolver implementation where this method diff --git a/chromium/net/dns/host_resolver.h b/chromium/net/dns/host_resolver.h index 264ab583500..87db841cf71 100644 --- a/chromium/net/dns/host_resolver.h +++ b/chromium/net/dns/host_resolver.h @@ -98,12 +98,10 @@ class NET_EXPORT HostResolver { virtual const base::Optional<std::vector<HostPortPair>>& GetHostnameResults() const = 0; - // TLS 1.3 Encrypted Server Name Indication, draft 4 (ESNI, - // https://tools.ietf.org/html/draft-ietf-tls-esni-04) - // results of the request. Should only be called after - // Start() signals completion, either by invoking the callback or by - // returning a result other than |ERR_IO_PENDING|. - virtual const base::Optional<EsniContent>& GetEsniResults() const = 0; + // INTEGRITY results for an initial experiment related to HTTPSSVC. Each + // boolean value indicates the intactness of an INTEGRITY record. + NET_EXPORT virtual const base::Optional<std::vector<bool>>& + GetIntegrityResultsForTesting() const; // Error info for the request. // @@ -312,15 +310,6 @@ class NET_EXPORT HostResolver { const NetLogWithSource& net_log, const base::Optional<ResolveHostParameters>& optional_parameters) = 0; - // Deprecated version of above method that uses an empty NetworkIsolationKey. - // - // TODO(mmenke): Once all consumers have been updated to use the other - // overload instead, remove this method and make above method pure virtual. - virtual std::unique_ptr<ResolveHostRequest> CreateRequest( - const HostPortPair& host, - const NetLogWithSource& net_log, - const base::Optional<ResolveHostParameters>& optional_parameters); - // Creates a request to probe configured DoH servers to find which can be used // successfully. virtual std::unique_ptr<ProbeRequest> CreateDohProbeRequest(); diff --git a/chromium/net/dns/host_resolver_manager.cc b/chromium/net/dns/host_resolver_manager.cc index 87bc925b8a0..82347fd6bf8 100644 --- a/chromium/net/dns/host_resolver_manager.cc +++ b/chromium/net/dns/host_resolver_manager.cc @@ -82,6 +82,7 @@ #include "net/dns/host_resolver_mdns_listener_impl.h" #include "net/dns/host_resolver_mdns_task.h" #include "net/dns/host_resolver_proc.h" +#include "net/dns/httpssvc_metrics.h" #include "net/dns/mdns_client.h" #include "net/dns/public/dns_protocol.h" #include "net/dns/public/resolve_error_info.h" @@ -499,7 +500,8 @@ class HostResolverManager::RequestImpl const base::Optional<ResolveHostParameters>& optional_parameters, ResolveContext* resolve_context, HostCache* host_cache, - base::WeakPtr<HostResolverManager> resolver) + base::WeakPtr<HostResolverManager> resolver, + const base::TickClock* tick_clock) : source_net_log_(source_net_log), request_host_(request_host), network_isolation_key_( @@ -516,7 +518,8 @@ class HostResolverManager::RequestImpl priority_(parameters_.initial_priority), job_(nullptr), resolver_(resolver), - complete_(false) {} + complete_(false), + tick_clock_(tick_clock) {} ~RequestImpl() override { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); @@ -544,7 +547,7 @@ class HostResolverManager::RequestImpl } else { DCHECK(!job_); complete_ = true; - LogFinishRequest(rv); + LogFinishRequest(rv, false /* async_completion */); } resolver_ = nullptr; @@ -573,10 +576,12 @@ class HostResolverManager::RequestImpl return results_ ? results_.value().hostnames() : *nullopt_result; } - const base::Optional<EsniContent>& GetEsniResults() const override { + const base::Optional<std::vector<bool>>& GetIntegrityResultsForTesting() + const override { DCHECK(complete_); - static const base::NoDestructor<base::Optional<EsniContent>> nullopt_result; - return results_ ? results_.value().esni_data() : *nullopt_result; + static const base::NoDestructor<base::Optional<std::vector<bool>>> + nullopt_result; + return results_ ? results_.value().integrity_data() : *nullopt_result; } net::ResolveErrorInfo GetResolveErrorInfo() const override { @@ -649,7 +654,7 @@ class HostResolverManager::RequestImpl DCHECK(!complete_); complete_ = true; - LogFinishRequest(error); + LogFinishRequest(error, true /* async_completion */); DCHECK(callback_); std::move(callback_).Run(HostResolver::SquashErrorCode(error)); @@ -679,19 +684,12 @@ class HostResolverManager::RequestImpl bool complete() const { return complete_; } - base::TimeTicks request_time() const { - DCHECK(!request_time_.is_null()); - return request_time_; - } - void set_request_time(base::TimeTicks request_time) { - DCHECK(request_time_.is_null()); - DCHECK(!request_time.is_null()); - request_time_ = request_time; - } - private: - // Logs when a request has just been started. + // Logging and metrics for when a request has just been started. void LogStartRequest() { + DCHECK(request_time_.is_null()); + request_time_ = tick_clock_->NowTicks(); + source_net_log_.BeginEvent( NetLogEventType::HOST_RESOLVER_IMPL_REQUEST, [this] { base::Value dict(base::Value::Type::DICTIONARY); @@ -708,10 +706,20 @@ class HostResolverManager::RequestImpl }); } - // Logs when a request has just completed (before its callback is run). - void LogFinishRequest(int net_error) { + // Logging and metrics for when a request has just completed (before its + // callback is run). + void LogFinishRequest(int net_error, bool async_completion) { source_net_log_.EndEventWithNetErrorCode( NetLogEventType::HOST_RESOLVER_IMPL_REQUEST, net_error); + + if (!parameters_.is_speculative) { + DCHECK(!request_time_.is_null()); + base::TimeDelta duration = tick_clock_->NowTicks() - request_time_; + + UMA_HISTOGRAM_MEDIUM_TIMES("Net.DNS.Request.TotalTime", duration); + if (async_completion) + UMA_HISTOGRAM_MEDIUM_TIMES("Net.DNS.Request.TotalTimeAsync", duration); + } } // Logs when a request has been cancelled. @@ -744,6 +752,7 @@ class HostResolverManager::RequestImpl base::Optional<HostCache::EntryStaleness> stale_info_; ResolveErrorInfo error_info_; + const base::TickClock* const tick_clock_; base::TimeTicks request_time_; SEQUENCE_CHECKER(sequence_checker_); @@ -1107,9 +1116,21 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { transactions_needed_.push(DnsQueryType::A); transactions_needed_.push(DnsQueryType::AAAA); - if (secure_ && - base::FeatureList::IsEnabled(features::kRequestEsniDnsRecords)) { - transactions_needed_.push(DnsQueryType::ESNI); + // Queue up an INTEGRITY query if we are allowed to. + const bool is_httpssvc_experiment_domain = + httpssvc_domain_cache_.IsExperimental(hostname); + const bool is_httpssvc_control_domain = + httpssvc_domain_cache_.IsControl(hostname); + if (base::FeatureList::IsEnabled(features::kDnsHttpssvc) && + features::kDnsHttpssvcUseIntegrity.Get() && + (secure_ || features::kDnsHttpssvcEnableQueryOverInsecure.Get()) && + (is_httpssvc_experiment_domain || is_httpssvc_control_domain)) { + // We should not be configured to query HTTPSSVC *and* INTEGRITY. + DCHECK(!features::kDnsHttpssvcUseHttpssvc.Get()); + + httpssvc_metrics_.emplace( + is_httpssvc_experiment_domain /* expect_intact */); + transactions_needed_.push(DnsQueryType::INTEGRITY); } } num_needed_transactions_ = transactions_needed_.size(); @@ -1170,15 +1191,25 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { return trans; } - void OnEsniTransactionTimeout() { - // Currently, the ESNI transaction timer only gets started - // when all non-ESNI transactions have completed. - DCHECK(TaskIsCompleteOrOnlyEsniTransactionsRemain()); + void OnExperimentalQueryTimeout(uint16_t qtype, + base::Optional<std::string> doh_provider_id) { + // The experimental query timer is only started when all other transactions + // have completed. + DCHECK(TaskIsCompleteOrOnlyQtypeTransactionsRemain(qtype)); num_completed_transactions_ += transactions_started_.size(); DCHECK(num_completed_transactions_ == num_needed_transactions()); transactions_started_.clear(); + if (qtype == dns_protocol::kExperimentalTypeIntegrity) { + DCHECK(httpssvc_metrics_); + + // Record that this INTEGRITY query timed out in the metrics. + base::TimeDelta elapsed_time = tick_clock_->NowTicks() - task_start_time_; + httpssvc_metrics_->SaveForIntegrity( + doh_provider_id, HttpssvcDnsRcode::kTimedOut, {}, elapsed_time); + } + ProcessResultsOnCompletion(); } @@ -1186,7 +1217,8 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { DnsQueryType dns_query_type, DnsTransaction* transaction, int net_error, - const DnsResponse* response) { + const DnsResponse* response, + base::Optional<std::string> doh_provider_id) { DCHECK(transaction); // Once control leaves OnTransactionComplete, there's no further @@ -1202,10 +1234,30 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { transactions_started_.erase(it); } + base::TimeDelta elapsed_time = tick_clock_->NowTicks() - task_start_time_; + enum HttpssvcDnsRcode rcode_for_httpssvc = HttpssvcDnsRcode::kNoError; + if (httpssvc_metrics_) { + if (net_error == ERR_DNS_TIMED_OUT) { + rcode_for_httpssvc = HttpssvcDnsRcode::kTimedOut; + } else if (net_error == ERR_NAME_NOT_RESOLVED) { + rcode_for_httpssvc = HttpssvcDnsRcode::kNoError; + } else if (response == nullptr) { + rcode_for_httpssvc = HttpssvcDnsRcode::kMissingDnsResponse; + } else { + rcode_for_httpssvc = + TranslateDnsRcodeForHttpssvcExperiment(response->rcode()); + } + } + if (net_error != OK && !(net_error == ERR_NAME_NOT_RESOLVED && response && response->IsValid())) { - OnFailure(net_error, DnsResponse::DNS_PARSE_OK, base::nullopt); - return; + if (dns_query_type == DnsQueryType::INTEGRITY) { + // Do not allow an INTEGRITY query to fail the whole DnsTask. + response = nullptr; + } else { + OnFailure(net_error, DnsResponse::DNS_PARSE_OK, base::nullopt); + return; + } } DnsResponse::Result parse_result = DnsResponse::DNS_PARSE_RESULT_MAX; @@ -1228,8 +1280,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { case DnsQueryType::SRV: parse_result = ParseServiceDnsResponse(response, &results); break; - case DnsQueryType::ESNI: - parse_result = ParseEsniDnsResponse(response, &results); + case DnsQueryType::INTEGRITY: + // Parse the INTEGRITY records, condensing them into a vector<bool>. + parse_result = ParseIntegrityDnsResponse(response, &results); break; } DCHECK_LT(parse_result, DnsResponse::DNS_PARSE_RESULT_MAX); @@ -1239,6 +1292,21 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { return; } + if (httpssvc_metrics_) { + if (dns_query_type != DnsQueryType::INTEGRITY) { + httpssvc_metrics_->SaveForNonIntegrity(doh_provider_id, elapsed_time, + rcode_for_httpssvc); + } else { + const base::Optional<std::vector<bool>>& condensed = + results.integrity_data(); + CHECK(condensed.has_value()); + // INTEGRITY queries can time out the normal way (here), or when the + // experimental query timer runs out (OnExperimentalQueryTimeout). + httpssvc_metrics_->SaveForIntegrity(doh_provider_id, rcode_for_httpssvc, + *condensed, elapsed_time); + } + } + // Merge results with saved results from previous transactions. if (saved_results_) { DCHECK_LE(2, num_needed_transactions()); @@ -1257,10 +1325,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { results = HostCache::Entry::MergeEntries( std::move(results), std::move(saved_results_).value()); break; - case DnsQueryType::ESNI: - // It doesn't matter whether the ESNI record is the "front" - // or the "back" argument to the merge, since the logic for - // merging addresses from ESNI records is the same in each case. + case DnsQueryType::INTEGRITY: results = HostCache::Entry::MergeEntries( std::move(results), std::move(saved_results_).value()); break; @@ -1277,13 +1342,16 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { ++num_completed_transactions_; if (num_completed_transactions_ < num_needed_transactions()) { delegate_->OnIntermediateTransactionComplete(); - MaybeStartEsniTimer(); + // If the experimental query times out, blame the provider that gave the + // last A/AAAA result. If we were being 100% correct, we would blame the + // provider associated with the experimental query. + MaybeStartExperimentalQueryTimer(doh_provider_id); return; } - // Since all transactions are complete, in particular, all ESNI transactions - // are complete (if any were started). - esni_cancellation_timer_.Stop(); + // Since all transactions are complete, in particular, all experimental + // transactions are complete (if any were started). + experimental_query_cancellation_timer_.Stop(); ProcessResultsOnCompletion(); } @@ -1296,20 +1364,13 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { // If there are multiple addresses, and at least one is IPv6, need to // sort them. - // When there are no ESNI keys in the record, IPv6 addresses are always - // put before IPv4 ones, so it's sufficient to just check the family of - // the first address. - // When there are ESNI keys, there could be ESNI-equipped - // IPv4 addresses preceding the first IPv6 address, so it's necessary to - // scan the list. bool at_least_one_ipv6_address = results.addresses() && !results.addresses().value().empty() && (results.addresses().value()[0].GetFamily() == ADDRESS_FAMILY_IPV6 || - (results.esni_data() && - std::any_of(results.addresses().value().begin(), - results.addresses().value().end(), [](auto& e) { - return e.GetFamily() == ADDRESS_FAMILY_IPV6; - }))); + std::any_of(results.addresses().value().begin(), + results.addresses().value().end(), [](auto& e) { + return e.GetFamily() == ADDRESS_FAMILY_IPV6; + })); if (at_least_one_ipv6_address) { // Sort addresses if needed. Sort could complete synchronously. @@ -1326,6 +1387,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { DnsResponse::Result ParseAddressDnsResponse(const DnsResponse* response, HostCache::Entry* out_results) { + DCHECK(response); AddressList addresses; base::TimeDelta ttl; DnsResponse::Result parse_result = @@ -1346,6 +1408,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { DnsResponse::Result ParseTxtDnsResponse(const DnsResponse* response, HostCache::Entry* out_results) { + DCHECK(response); std::vector<std::unique_ptr<const RecordParsed>> records; base::Optional<base::TimeDelta> response_ttl; DnsResponse::Result parse_result = ParseAndFilterResponseRecords( @@ -1371,6 +1434,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { DnsResponse::Result ParsePointerDnsResponse(const DnsResponse* response, HostCache::Entry* out_results) { + DCHECK(response); std::vector<std::unique_ptr<const RecordParsed>> records; base::Optional<base::TimeDelta> response_ttl; DnsResponse::Result parse_result = ParseAndFilterResponseRecords( @@ -1399,6 +1463,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { DnsResponse::Result ParseServiceDnsResponse(const DnsResponse* response, HostCache::Entry* out_results) { + DCHECK(response); std::vector<std::unique_ptr<const RecordParsed>> records; base::Optional<base::TimeDelta> response_ttl; DnsResponse::Result parse_result = ParseAndFilterResponseRecords( @@ -1428,55 +1493,39 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { return DnsResponse::DNS_PARSE_OK; } - DnsResponse::Result ParseEsniDnsResponse(const DnsResponse* response, - HostCache::Entry* out_results) { - std::vector<std::unique_ptr<const RecordParsed>> records; + DnsResponse::Result ParseIntegrityDnsResponse(const DnsResponse* response, + HostCache::Entry* out_results) { base::Optional<base::TimeDelta> response_ttl; + const HostCache::Entry default_entry( + OK, std::vector<bool>(), HostCache::Entry::SOURCE_DNS, response_ttl); + + if (response == nullptr) { + *out_results = default_entry; + return DnsResponse::Result::DNS_PARSE_OK; + } + + std::vector<std::unique_ptr<const RecordParsed>> records; DnsResponse::Result parse_result = ParseAndFilterResponseRecords( - response, dns_protocol::kExperimentalTypeEsniDraft4, &records, + response, dns_protocol::kExperimentalTypeIntegrity, &records, &response_ttl); if (parse_result != DnsResponse::DNS_PARSE_OK) { - *out_results = GetMalformedResponseResult(); - return parse_result; + *out_results = default_entry; + return DnsResponse::Result::DNS_PARSE_OK; } - // Glom the ESNI response records into a single EsniContent; - // this also dedups keys and (key, address) associations. - EsniContent content; + // Condense results into a list of booleans. We do not cache the results, + // but this enables us to write some unit tests. + std::vector<bool> condensed_results; for (const auto& record : records) { - const EsniRecordRdata& rdata = *record->rdata<EsniRecordRdata>(); - - for (const IPAddress& address : rdata.addresses()) - content.AddKeyForAddress(address, rdata.esni_keys()); - } - - // As a first pass, deliberately ignore ESNI records with no addresses - // included. Later, the implementation can be extended to handle "at-large" - // ESNI keys not specifically associated with collections of addresses. - // (We're declining the "...clients MAY initiate..." choice in ESNI draft 4, - // Section 4.2.2 Step 2.) - if (content.keys_for_addresses().empty()) { - *out_results = - HostCache::Entry(ERR_NAME_NOT_RESOLVED, EsniContent(), - HostCache::Entry::SOURCE_DNS, response_ttl); - } else { - AddressList addresses, ipv4_addresses_temporary; - addresses.set_canonical_name(hostname_); - for (const auto& kv : content.keys_for_addresses()) - (kv.first.IsIPv6() ? addresses : ipv4_addresses_temporary) - .push_back(IPEndPoint(kv.first, 0)); - addresses.insert(addresses.end(), ipv4_addresses_temporary.begin(), - ipv4_addresses_temporary.end()); - - // Store the addresses separately from the ESNI key-address - // associations, so that the addresses can be merged later with - // addresses from A and AAAA records. - *out_results = HostCache::Entry( - OK, std::move(content), HostCache::Entry::SOURCE_DNS, response_ttl); - out_results->set_addresses(std::move(addresses)); + const IntegrityRecordRdata& rdata = + *record->rdata<IntegrityRecordRdata>(); + condensed_results.push_back(rdata.IsIntact()); } + *out_results = HostCache::Entry(OK, std::move(condensed_results), + HostCache::Entry::SOURCE_DNS, response_ttl); + DCHECK_EQ(parse_result, DnsResponse::DNS_PARSE_OK); return parse_result; } @@ -1605,6 +1654,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { void OnFailure(int net_error, DnsResponse::Result parse_result, base::Optional<base::TimeDelta> ttl) { + if (httpssvc_metrics_) + httpssvc_metrics_->SaveNonIntegrityFailure(); + DCHECK_NE(OK, net_error); HostCache::Entry results(net_error, HostCache::Entry::SOURCE_UNKNOWN); @@ -1636,52 +1688,57 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { delegate_->OnDnsTaskComplete(task_start_time_, results, secure_); } - // Returns whether all transactions left to execute are of transaction - // type ESNI. (In particular, this is the case if all transactions are - // complete.) - // Used for logging and starting the ESNI transaction timer (see - // MaybeStartEsniTimer). - bool TaskIsCompleteOrOnlyEsniTransactionsRemain() const { - // Since DoH runs all transactions concurrently and - // DnsQueryType::UNSPECIFIED-with-ESNI tasks are only run using DoH, - // this method only needs to check the transactions in transactions_started_ - // because transactions_needed_ is empty from the time the first - // transaction is started. + // Returns whether all transactions left to execute are of transaction type + // |qtype|. (In particular, this is the case if all transactions are + // complete.) Used for logging and starting the experimental query timer (see + // MaybeStartExperimentalQueryTimer). + bool TaskIsCompleteOrOnlyQtypeTransactionsRemain(uint16_t qtype) const { + // Since DoH runs all transactions concurrently and experimental types are + // only queried over DoH, this method only needs to check the transactions + // in transactions_started_ because transactions_needed_ is empty from the + // time the first transaction is started. DCHECK(transactions_needed_.empty()); return std::all_of( transactions_started_.begin(), transactions_started_.end(), [&](const std::unique_ptr<DnsTransaction>& p) { DCHECK(p); - return p->GetType() == dns_protocol::kExperimentalTypeEsniDraft4; + return p->GetType() == qtype; }); } - // If ESNI transactions are being executed as part of this task - // and all transactions except the ESNI transactions have finished, and the - // ESNI transactions have not finished, starts a timer after which to abort - // the ESNI transactions. - // - // This timer has duration equal to the shorter of two parameterized values: - // - a fixed, absolute duration - // - a relative duration (as a proportion of the total time taken for - // the task's other transactions). - void MaybeStartEsniTimer() { + + void MaybeStartExperimentalQueryTimer( + base::Optional<std::string> doh_provider_id) { DCHECK(!transactions_started_.empty()); - DCHECK(saved_results_); - if (!esni_cancellation_timer_.IsRunning() && - TaskIsCompleteOrOnlyEsniTransactionsRemain()) { - base::TimeDelta total_time_taken_for_other_transactions = + + // Abort if neither HTTPSSVC nor INTEGRITY querying is enabled. + if (!base::FeatureList::IsEnabled(features::kDnsHttpssvc) || + (!features::kDnsHttpssvcUseIntegrity.Get() && + !features::kDnsHttpssvcUseHttpssvc.Get())) { + return; + } + + if (!experimental_query_cancellation_timer_.IsRunning() && + TaskIsCompleteOrOnlyQtypeTransactionsRemain( + dns_protocol::kExperimentalTypeIntegrity)) { + const base::TimeDelta kExtraTimeAbsolute = + features::dns_httpssvc_experiment::GetExtraTimeAbsolute(); + const int kExtraTimePercent = + features::kDnsHttpssvcExtraTimePercent.Get(); + + base::TimeDelta total_time_for_other_transactions = tick_clock_->NowTicks() - task_start_time_; + base::TimeDelta relative_timeout = + total_time_for_other_transactions * kExtraTimePercent / 100; - esni_cancellation_timer_.Start( - FROM_HERE, - std::min( - features::EsniDnsMaxAbsoluteAdditionalWait(), - total_time_taken_for_other_transactions * - (0.01 * - features::kEsniDnsMaxRelativeAdditionalWaitPercent.Get())), - this, &DnsTask::OnEsniTransactionTimeout); + base::TimeDelta timeout = std::min(kExtraTimeAbsolute, relative_timeout); + + experimental_query_cancellation_timer_.Start( + FROM_HERE, timeout, + base::BindOnce( + &DnsTask::OnExperimentalQueryTimeout, base::Unretained(this), + dns_protocol::kExperimentalTypeIntegrity, doh_provider_id)); } } @@ -1711,16 +1768,12 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> { const base::TickClock* tick_clock_; base::TimeTicks task_start_time_; - // In order to histogram the relative end-to-end elapsed times of - // a task's ESNI and non-ESNI transactions, store the end-to-end time - // elapsed from task start to the end of the task's ESNI transaction - // (if any) and its final non-ESNI transaction. - base::TimeDelta esni_elapsed_for_logging_; - base::TimeDelta non_esni_elapsed_for_logging_; + HttpssvcExperimentDomainCache httpssvc_domain_cache_; + base::Optional<HttpssvcMetrics> httpssvc_metrics_; - // Timer for early abort of ESNI transactions. See comments describing - // the timeout parameters in net/base/features.h. - base::OneShotTimer esni_cancellation_timer_; + // Timer for early abort of experimental queries. See comments describing the + // timeout parameters in net/base/features.h. + base::OneShotTimer experimental_query_cancellation_timer_; DISALLOW_COPY_AND_ASSIGN(DnsTask); }; @@ -2396,7 +2449,7 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job, query_types.push_back(query_type_); } - MDnsClient* client; + MDnsClient* client = nullptr; int rv = resolver_->GetOrCreateMdnsClient(&client); mdns_task_ = std::make_unique<HostResolverMdnsTask>(client, hostname_, query_types); @@ -2454,23 +2507,6 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job, if (had_non_speculative_request_) { category = RESOLVE_SUCCESS; UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveSuccessTime", duration); - switch (query_type_) { - case DnsQueryType::A: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveSuccessTime.IPV4", - duration); - break; - case DnsQueryType::AAAA: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveSuccessTime.IPV6", - duration); - break; - case DnsQueryType::UNSPECIFIED: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveSuccessTime.UNSPEC", - duration); - break; - default: - // No histogram for other query types. - break; - } } else { category = RESOLVE_SPECULATIVE_SUCCESS; } @@ -2482,23 +2518,6 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job, if (had_non_speculative_request_) { category = RESOLVE_FAIL; UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveFailureTime", duration); - switch (query_type_) { - case DnsQueryType::A: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveFailureTime.IPV4", - duration); - break; - case DnsQueryType::AAAA: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveFailureTime.IPV6", - duration); - break; - case DnsQueryType::UNSPECIFIED: - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.ResolveFailureTime.UNSPEC", - duration); - break; - default: - // No histogram for other query types. - break; - } } else { category = RESOLVE_SPECULATIVE_FAIL; } @@ -2514,6 +2533,13 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job, else base::UmaHistogramSparse("Net.DNS.ResolveError.Slow", std::abs(error)); } + + if (had_non_speculative_request_) { + UmaHistogramMediumTimes( + base::StringPrintf("Net.DNS.SecureDnsMode.%s.ResolveTime", + SecureDnsModeToString(secure_dns_mode_).c_str()), + duration); + } } void MaybeCacheResult(const HostCache::Entry& results, @@ -2587,13 +2613,7 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job, RequestImpl* req = requests_.head()->value(); req->RemoveFromList(); DCHECK_EQ(this, req->job()); - // Update the net log and notify registered observers. - if (results.did_complete()) { - // Record effective total time from creation to completion. - resolver_->RecordTotalTime( - req->parameters().is_speculative, false /* from_cache */, - secure_dns_mode_, tick_clock_->NowTicks() - req->request_time()); - } + if (results.error() == OK && !req->parameters().is_speculative) { req->set_results( results.CopyWithDefaultPort(req->request_host().port())); @@ -2808,7 +2828,7 @@ HostResolverManager::CreateRequest( return std::make_unique<RequestImpl>( net_log, host, network_isolation_key, optional_parameters, - resolve_context, host_cache, weak_ptr_factory_.GetWeakPtr()); + resolve_context, host_cache, weak_ptr_factory_.GetWeakPtr(), tick_clock_); } std::unique_ptr<HostResolverManager::CancellableProbeRequest> @@ -2971,8 +2991,6 @@ int HostResolverManager::Resolve(RequestImpl* request) { ResolveHostParameters::CacheUsage::ALLOWED); DCHECK(!invalidation_in_progress_); - request->set_request_time(tick_clock_->NowTicks()); - DnsQueryType effective_query_type; HostResolverFlags effective_host_resolver_flags; DnsConfig::SecureDnsMode effective_secure_dns_mode; @@ -2996,8 +3014,6 @@ int HostResolverManager::Resolve(RequestImpl* request) { } if (stale_info && !request->parameters().is_speculative) request->set_stale_info(std::move(stale_info).value()); - RecordTotalTime(request->parameters().is_speculative, true /* from_cache */, - effective_secure_dns_mode, base::TimeDelta()); request->set_error_info(results.error(), false /* is_secure_network_error */); return HostResolver::SquashErrorCode(results.error()); @@ -3317,24 +3333,6 @@ void HostResolverManager::CacheResult(HostCache* cache, cache->Set(key, entry, tick_clock_->NowTicks(), ttl); } -// Record time from Request creation until a valid DNS response. -void HostResolverManager::RecordTotalTime( - bool speculative, - bool from_cache, - DnsConfig::SecureDnsMode secure_dns_mode, - base::TimeDelta duration) const { - if (!speculative) { - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.TotalTime", duration); - UmaHistogramMediumTimes( - base::StringPrintf("Net.DNS.SecureDnsMode.%s.TotalTime", - SecureDnsModeToString(secure_dns_mode).c_str()), - duration); - - if (!from_cache) - UMA_HISTOGRAM_LONG_TIMES_100("Net.DNS.TotalTimeNotCached", duration); - } -} - std::unique_ptr<HostResolverManager::Job> HostResolverManager::RemoveJob( JobMap::iterator job_it) { DCHECK(job_it != jobs_.end()); diff --git a/chromium/net/dns/host_resolver_manager.h b/chromium/net/dns/host_resolver_manager.h index 4c4799ea00f..c712824c3d1 100644 --- a/chromium/net/dns/host_resolver_manager.h +++ b/chromium/net/dns/host_resolver_manager.h @@ -414,12 +414,6 @@ class NET_EXPORT HostResolverManager const HostCache::Entry& entry, base::TimeDelta ttl); - // Record time from Request creation until a valid DNS response. - void RecordTotalTime(bool speculative, - bool from_cache, - DnsConfig::SecureDnsMode secure_dns_mode, - base::TimeDelta duration) const; - // Removes |job_it| from |jobs_| and return. std::unique_ptr<Job> RemoveJob(JobMap::iterator job_it); diff --git a/chromium/net/dns/host_resolver_manager_fuzzer.cc b/chromium/net/dns/host_resolver_manager_fuzzer.cc index 8380657fef5..a6ae1ddaf81 100644 --- a/chromium/net/dns/host_resolver_manager_fuzzer.cc +++ b/chromium/net/dns/host_resolver_manager_fuzzer.cc @@ -17,6 +17,7 @@ #include "net/base/address_family.h" #include "net/base/address_list.h" #include "net/base/net_errors.h" +#include "net/base/network_isolation_key.h" #include "net/base/request_priority.h" #include "net/dns/context_host_resolver.h" #include "net/dns/fuzzed_host_resolver_util.h" @@ -161,7 +162,8 @@ class DnsRequest { const char* hostname = data_provider_->PickValueInArray(kHostNames); request_ = host_resolver_->CreateRequest( - net::HostPortPair(hostname, 80), net::NetLogWithSource(), parameters); + net::HostPortPair(hostname, 80), net::NetworkIsolationKey(), + net::NetLogWithSource(), parameters); int rv = request_->Start( base::BindOnce(&DnsRequest::OnCallback, base::Unretained(this))); if (rv != net::ERR_IO_PENDING) diff --git a/chromium/net/dns/host_resolver_manager_unittest.cc b/chromium/net/dns/host_resolver_manager_unittest.cc index 16d70163982..f467b8e1eac 100644 --- a/chromium/net/dns/host_resolver_manager_unittest.cc +++ b/chromium/net/dns/host_resolver_manager_unittest.cc @@ -86,6 +86,7 @@ using ::testing::AllOf; using ::testing::Between; using ::testing::ByMove; using ::testing::Eq; +using ::testing::IsEmpty; using ::testing::Optional; using ::testing::Pair; using ::testing::Property; @@ -2779,7 +2780,7 @@ TEST_F(HostResolverManagerTest, Mdns) { CreateExpected("000a:0000:0000:0000:0001:0002:0003:0004", 80))); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerTest, Mdns_AaaaOnly) { @@ -2831,7 +2832,7 @@ TEST_F(HostResolverManagerTest, Mdns_Txt) { EXPECT_THAT(response.request()->GetTextResults(), testing::Optional(testing::ElementsAre("foo", "bar"))); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerTest, Mdns_Ptr) { @@ -2856,7 +2857,7 @@ TEST_F(HostResolverManagerTest, Mdns_Ptr) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); EXPECT_THAT( response.request()->GetHostnameResults(), testing::Optional(testing::ElementsAre(HostPortPair("foo.com", 83)))); @@ -2884,7 +2885,7 @@ TEST_F(HostResolverManagerTest, Mdns_Srv) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); EXPECT_THAT( response.request()->GetHostnameResults(), testing::Optional(testing::ElementsAre(HostPortPair("foo.com", 8265)))); @@ -2912,7 +2913,7 @@ TEST_F(HostResolverManagerTest, Mdns_Srv_Unrestricted) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); EXPECT_THAT( response.request()->GetHostnameResults(), testing::Optional(testing::ElementsAre(HostPortPair("foo.com", 8265)))); @@ -2941,7 +2942,7 @@ TEST_F(HostResolverManagerTest, Mdns_Srv_Result_Unrestricted) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); EXPECT_THAT(response.request()->GetHostnameResults(), testing::Optional( testing::ElementsAre(HostPortPair("foo bar.local", 8265)))); @@ -3006,7 +3007,7 @@ TEST_F(HostResolverManagerTest, Mdns_NoResponse) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); test_task_runner->FastForwardUntilNoTasksRemain(); } @@ -3050,7 +3051,7 @@ TEST_F(HostResolverManagerTest, Mdns_WrongType) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); test_task_runner->FastForwardUntilNoTasksRemain(); } @@ -7815,7 +7816,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Order between separate DNS records is undefined, but each record should // stay in order as that order may be meaningful. @@ -7868,7 +7869,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_NonexistentDomain) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_Failure) { @@ -7895,7 +7896,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_Failure) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_Timeout) { @@ -7922,7 +7923,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_Timeout) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_Empty) { @@ -7949,7 +7950,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_Empty) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_Malformed) { @@ -7976,7 +7977,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_Malformed) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_MismatchedName) { @@ -8000,7 +8001,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_MismatchedName) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, TxtQuery_WrongType) { @@ -8025,7 +8026,7 @@ TEST_F(HostResolverManagerDnsTest, TxtQuery_WrongType) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } // Same as TxtQuery except we specify DNS HostResolverSource instead of relying @@ -8058,7 +8059,7 @@ TEST_F(HostResolverManagerDnsTest, TxtDnsQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Order between separate DNS records is undefined, but each record should // stay in order as that order may be meaningful. @@ -8092,7 +8093,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Order between separate records is undefined. EXPECT_THAT(response.request()->GetHostnameResults(), @@ -8119,7 +8120,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_Ip) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Order between separate records is undefined. EXPECT_THAT(response.request()->GetHostnameResults(), @@ -8151,7 +8152,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_NonexistentDomain) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_Failure) { @@ -8178,7 +8179,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_Failure) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_Timeout) { @@ -8205,7 +8206,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_Timeout) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_Empty) { @@ -8232,7 +8233,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_Empty) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_Malformed) { @@ -8259,7 +8260,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_Malformed) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_MismatchedName) { @@ -8283,7 +8284,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_MismatchedName) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, PtrQuery_WrongType) { @@ -8308,7 +8309,7 @@ TEST_F(HostResolverManagerDnsTest, PtrQuery_WrongType) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } // Same as PtrQuery except we specify DNS HostResolverSource instead of relying @@ -8335,7 +8336,7 @@ TEST_F(HostResolverManagerDnsTest, PtrDnsQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Order between separate records is undefined. EXPECT_THAT(response.request()->GetHostnameResults(), @@ -8366,7 +8367,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Expect ordered by priority, and random within a priority. base::Optional<std::vector<HostPortPair>> results = @@ -8411,7 +8412,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_ZeroWeight) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Expect ordered by priority, and random within a priority. EXPECT_THAT(response.request()->GetHostnameResults(), @@ -8443,7 +8444,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_NonexistentDomain) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_Failure) { @@ -8470,7 +8471,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_Failure) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_Timeout) { @@ -8497,7 +8498,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_Timeout) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_Empty) { @@ -8524,7 +8525,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_Empty) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_Malformed) { @@ -8551,7 +8552,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_Malformed) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_MismatchedName) { @@ -8575,7 +8576,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_MismatchedName) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } TEST_F(HostResolverManagerDnsTest, SrvQuery_WrongType) { @@ -8600,7 +8601,7 @@ TEST_F(HostResolverManagerDnsTest, SrvQuery_WrongType) { EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); } // Same as SrvQuery except we specify DNS HostResolverSource instead of relying @@ -8631,7 +8632,7 @@ TEST_F(HostResolverManagerDnsTest, SrvDnsQuery) { EXPECT_THAT(response.result_error(), IsOk()); EXPECT_FALSE(response.request()->GetAddressResults()); EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); + EXPECT_FALSE(response.request()->GetIntegrityResultsForTesting()); // Expect ordered by priority, and random within a priority. base::Optional<std::vector<HostPortPair>> results = @@ -8653,6 +8654,542 @@ TEST_F(HostResolverManagerDnsTest, SrvDnsQuery) { HostPortPair("google.com", 5))); } +class HostResolverManagerDnsTestIntegrity : public HostResolverManagerDnsTest { + public: + HostResolverManagerDnsTestIntegrity() + : HostResolverManagerDnsTest( + base::test::TaskEnvironment::TimeSource::MOCK_TIME) { + const base::FieldTrialParams params = { + {"DnsHttpssvcUseIntegrity", "true"}, + {"DnsHttpssvcExperimentDomains", "host"}, + {"DnsHttpssvcControlDomains", ""}, + {"DnsHttpssvcEnableQueryOverInsecure", "false"}, + }; + scoped_feature_list_.InitAndEnableFeatureWithParameters( + features::kDnsHttpssvc, params); + } + + protected: + struct IntegrityAddRulesOptions { + bool add_a = true; + bool add_aaaa = true; + bool add_integrity = true; + bool integrity_mangled = false; + + bool secure_a = true; + bool secure_aaaa = true; + bool secure_integrity = true; + + bool delay_a = false; + bool delay_aaaa = false; + bool delay_integrity = false; + }; + + std::vector<uint8_t> GetValidIntegrityRdata() { + const IntegrityRecordRdata kValidRecord({'f', 'o', 'o'}); + base::Optional<std::vector<uint8_t>> valid_serialized = + kValidRecord.Serialize(); + CHECK(valid_serialized); + return *valid_serialized; + } + + std::vector<uint8_t> GetMangledIntegrityRdata() { + std::vector<uint8_t> rdata = GetValidIntegrityRdata(); + constexpr size_t kOffset = 2u; + CHECK_GT(rdata.size(), kOffset); + // Create a mangled version of |kValidRecord| by erasing a byte. + rdata.erase(rdata.begin() + kOffset); + return rdata; + } + + void AddRules(MockDnsClientRuleList rules, + const IntegrityAddRulesOptions& options) { + if (options.add_a) { + rules.emplace_back("host", dns_protocol::kTypeA, options.secure_a, + MockDnsClientRule::Result(MockDnsClientRule::OK), + options.delay_a); + } + + if (options.add_aaaa) { + rules.emplace_back("host", dns_protocol::kTypeAAAA, options.secure_aaaa, + MockDnsClientRule::Result(MockDnsClientRule::OK), + options.delay_aaaa); + } + + if (options.add_integrity) { + std::vector<uint8_t> integrity_rdata = options.integrity_mangled + ? GetMangledIntegrityRdata() + : GetValidIntegrityRdata(); + rules.emplace_back( + "host", dns_protocol::kExperimentalTypeIntegrity, + options.secure_integrity, + MockDnsClientRule::Result(BuildTestDnsIntegrityResponse( + "host", std::move(integrity_rdata))), + options.delay_integrity); + } + + CreateResolver(); + UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); + } + + std::unique_ptr<ResolveHostResponseHelper> DoIntegrityQuery(bool use_secure) { + if (use_secure) { + DnsConfigOverrides overrides; + overrides.secure_dns_mode = DnsConfig::SecureDnsMode::SECURE; + resolver_->SetDnsConfigOverrides(overrides); + } + + return std::make_unique<ResolveHostResponseHelper>(resolver_->CreateRequest( + HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), + HostResolver::ResolveHostParameters(), resolve_context_.get(), + resolve_context_->host_cache())); + } + + base::test::ScopedFeatureList scoped_feature_list_; +}; + +TEST_F(HostResolverManagerDnsTestIntegrity, IntegrityQuery) { + AddRules(CreateDefaultDnsRules(), IntegrityAddRulesOptions()); + + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + EXPECT_THAT(response->result_error(), IsOk()); + base::Optional<std::vector<bool>> results = + response->request()->GetIntegrityResultsForTesting(); + + EXPECT_TRUE(response->request()->GetAddressResults()); + EXPECT_FALSE(response->request()->GetTextResults()); + EXPECT_THAT(results, Optional(UnorderedElementsAre(true))); +} + +TEST_F(HostResolverManagerDnsTestIntegrity, IntegrityQueryMangled) { + IntegrityAddRulesOptions options; + options.integrity_mangled = true; + AddRules(CreateDefaultDnsRules(), options); + + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + EXPECT_THAT(response->result_error(), IsOk()); + base::Optional<std::vector<bool>> results = + response->request()->GetIntegrityResultsForTesting(); + + EXPECT_TRUE(response->request()->GetAddressResults()); + EXPECT_FALSE(response->request()->GetTextResults()); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(UnorderedElementsAre(false))); +} + +TEST_F(HostResolverManagerDnsTestIntegrity, IntegrityQueryOnlyOverSecure) { + IntegrityAddRulesOptions rules_options; + rules_options.secure_a = false; + rules_options.secure_aaaa = false; + rules_options.secure_integrity = false; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(false /* use_secure */); + + EXPECT_THAT(response->result_error(), IsOk()); + base::Optional<std::vector<bool>> results = + response->request()->GetIntegrityResultsForTesting(); + + EXPECT_FALSE(results); +} + +// Ensure that the address results are preserved, even when the INTEGRITY query +// completes last. +TEST_F(HostResolverManagerDnsTestIntegrity, IntegrityQueryCompletesLast) { + IntegrityAddRulesOptions rules_options; + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(1); + + FastForwardBy(100 * kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_TRUE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + // Above, the A/AAAA queries took 100 time units. We only fast forward by 1 + // time unit (1%) before answering the INTEGRITY query, to avoid triggering + // the timeout logic. This should work, assuming + // (1) the relative timeout is > 1% and + // (2) the absolute timeout is < (101 * kQuantum). + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); + // If this expectation fails, the INTEGRITY query was probably timed out. + // Check the |kDnsHttpssvcExtraTimeMs| and |kDnsHttpssvcExtraTimePercent| + // feature params in relation to this test's FastForward steps. + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(UnorderedElementsAre(true))); +} + +// For symmetry with |IntegrityQueryCompletesLast|, test the case where the +// INTEGRITY query completes first. +TEST_F(HostResolverManagerDnsTestIntegrity, IntegrityQueryCompletesFirst) { + IntegrityAddRulesOptions rules_options; + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(10); + + FastForwardBy(kQuantum); + + ASSERT_TRUE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + FastForwardBy(kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(UnorderedElementsAre(true))); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); +} + +// Ensure that the address results are preserved, even when the INTEGRITY query +// completes last and fails. +TEST_F(HostResolverManagerDnsTestIntegrity, + IntegrityQueryCompletesLastWithError) { + IntegrityAddRulesOptions rules_options; + rules_options.add_a = true; + rules_options.add_aaaa = true; + rules_options.add_integrity = false; + + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(1); + + FastForwardBy(100 * kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_FALSE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(IsEmpty())); +} + +// Ensure that the address results are preserved, even when the INTEGRITY query +// completes first and fails. +TEST_F(HostResolverManagerDnsTestIntegrity, + IntegrityQueryCompletesFirstWithError) { + IntegrityAddRulesOptions rules_options; + rules_options.add_a = true; + rules_options.add_aaaa = true; + rules_options.add_integrity = false; + + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(10); + + FastForwardBy(kQuantum); + + // This fails because there is no rule for the INTEGRITY query. + ASSERT_FALSE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + FastForwardBy(kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(IsEmpty())); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); +} + +TEST_F(HostResolverManagerDnsTestIntegrity, + IntegrityQueryCompletesLastMangled) { + IntegrityAddRulesOptions rules_options; + rules_options.integrity_mangled = true; + + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(1); + + FastForwardBy(100 * kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_TRUE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(UnorderedElementsAre(false))); +} + +TEST_F(HostResolverManagerDnsTestIntegrity, + IntegrityQueryCompletesFirstMangled) { + IntegrityAddRulesOptions rules_options; + rules_options.integrity_mangled = true; + + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + constexpr base::TimeDelta kQuantum = base::TimeDelta::FromMilliseconds(10); + + FastForwardBy(kQuantum); + + ASSERT_TRUE(dns_client_->CompleteOneDelayedTransactionOfType( + DnsQueryType::INTEGRITY)); + + FastForwardBy(kQuantum); + + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + FastForwardBy(kQuantum); + + ASSERT_THAT(response->result_error(), IsOk()); + EXPECT_THAT(response->request()->GetIntegrityResultsForTesting(), + Optional(UnorderedElementsAre(false))); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); +} + +// Make sure that INTEGRITY queries don't get cancelled *before* the configured +// timeout, but do get cancelled after it, in the case where the absolute +// timeout dominates. +TEST_F(HostResolverManagerDnsTestIntegrity, RespectsAbsoluteTimeout) { + IntegrityAddRulesOptions rules_options; + rules_options.delay_a = true; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + // relative_timeout + // ┌────────────────────────────────┤ + // │ + // │ absolute_timeout + // ├────────────────────┤ + // a_aaaa_elapsed │ time + // ├────────────────────┼─────────────────────────────────────────────────> + // Now └ (moment when A and AAAA complete) + // + // When the A and AAAA queries complete, and only INTEGRITY remains, we start + // running the INTEGRITY timeout clock. This moment is |Now + a_aaaa_elapsed|, + // or just |a_aaaa_elapsed| if we let Now = 0. The INTEGRITY query is + // cancelled at the moment that |absolute_timeout| or |relative_timeout| runs + // out. + // + // The TimeDelta values of |absolute_timeout| and |relative_timeout| are + // computed from feature params. + // + // absolute_timeout = a_aaaa_elapsed + ExtraMs. + // + // relative_timeout = a_aaaa_elapsed * (1 + (ExtraPercent/100)). + // + // Assume ExtraMs > 0 and 0 < ExtraPercent < 100. + // + // For this test, we want the absolute timeout to happen *before* the relative + // timeout. Compute a value for a_aaaa_elapsed such that absolute_timeout + // comes before relative_timeout. + // + // Assuming ExtraPercent is not zero, we know that these two lines intersect + // for some value of a_aaaa_elapsed. Let's find it. + // + // Assume absolute_timeout = relative_timeout. + // a_aaaa_elapsed + ExtraMs = a_aaaa_elapsed * (1 + (ExtraPercent / 100)). + // ExtraMs = a_aaaa_elapsed * (1 + (ExtraPercent / 100)) - a_aaaa_elapsed. + // ExtraMs = a_aaaa_elapsed * ((1 + (ExtraPercent / 100)) - 1). + // ExtraMs / ((1 + (ExtraPercent / 100)) - 1) = a_aaaa_elapsed. + // Simplified: + // a_aaaa_elapsed = 100 * ExtraMs / ExtraPercent. + // + // For values of a_aaaa_elapsed < 100 * ExtraMs / ExtraPercent, + // relative_timeout < absolute_timeout. For larger values, absolute_timeout > + // relative_timeout. + + base::TimeDelta absolute_timeout = base::TimeDelta::FromMilliseconds( + features::kDnsHttpssvcExtraTimeMs.Get()); + base::TimeDelta intersection = + 100 * absolute_timeout / features::kDnsHttpssvcExtraTimePercent.Get(); + + // Let enough time pass during the A and AAAA transactions that the + // absolute timeout will be less than the relative timeout. + base::TimeDelta a_aaaa_elapsed = 50 * intersection; + + FastForwardBy(a_aaaa_elapsed); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + // Since the A and AAAA queries have only just completed, we shouldn't + // have timed out the INTEGRITY query. + EXPECT_FALSE(response->complete()); + + // After half of the absolute timeout, the query should still be alive. + FastForwardBy(absolute_timeout / 2); + + // Since the absolute timeout has not yet elapsed, and it is shorter by + // design than the relative timeout, we shouldn't + // have timed out the INTEGRITY transaction. + EXPECT_FALSE(response->complete()); + + // After (more than) the timeout has passed, we should have cancelled + // the INTEGRITY transaction. + FastForwardBy(absolute_timeout); + ASSERT_THAT(response->result_error(), IsOk()); + + // Since we cancelled the transaction, we shouldn't have any INTEGRITY + // results. + EXPECT_FALSE(response->request()->GetIntegrityResultsForTesting()); + + // Out of paranoia, pass some more time to ensure no crashes occur. + FastForwardBy(base::TimeDelta::FromMilliseconds(100)); +} + +TEST_F(HostResolverManagerDnsTestIntegrity, RespectsRelativeTimeout) { + IntegrityAddRulesOptions rules_options; + rules_options.delay_a = false; + rules_options.delay_aaaa = true; + rules_options.delay_integrity = true; + + AddRules(CreateDefaultDnsRules(), rules_options); + + std::unique_ptr<ResolveHostResponseHelper> response = + DoIntegrityQuery(true /* use_secure */); + + base::TimeDelta absolute_timeout = base::TimeDelta::FromMilliseconds( + features::kDnsHttpssvcExtraTimeMs.Get()); + base::TimeDelta intersection = + 100 * absolute_timeout / features::kDnsHttpssvcExtraTimePercent.Get(); + + // Let little enough time pass during the A and AAAA transactions that the + // relative timeout will be less than the absolute timeout. + base::TimeDelta a_aaaa_elapsed = 0.05 * intersection; + + // Since the A and AAAA queries haven't both completed yet, we shouldn't time + // out the INTEGRITY query. + FastForwardBy(a_aaaa_elapsed); + + // Upon completing the AAAA transaction, the INTEGRITY timer should start + ASSERT_TRUE( + dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); + + base::TimeDelta relative_timeout = + a_aaaa_elapsed * features::kDnsHttpssvcExtraTimePercent.Get() / 100; + + // After *less* than the relative timeout, the query shouldn't have concluded. + FastForwardBy(relative_timeout * 0.5); + + EXPECT_FALSE(response->complete()); + + // After more than the relative timeout, the query should conclude by aborting + // the INTEGRITY query. + FastForwardBy(relative_timeout); + + // The task should have completed with a cancelled INTEGRITY query. + ASSERT_THAT(response->result_error(), IsOk()); + EXPECT_FALSE(response->request()->GetIntegrityResultsForTesting()); + ASSERT_TRUE(response->request()->GetAddressResults()); + EXPECT_THAT(response->request()->GetAddressResults()->endpoints(), + testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 108), + CreateExpected("::1", 108))); + + // Out of paranoia, pass some more time to ensure no crashes occur. + FastForwardBy(base::TimeDelta::FromMilliseconds(100)); +} + TEST_F(HostResolverManagerDnsTest, DohProbeRequest) { ChangeDnsConfig(CreateValidDnsConfig()); @@ -8787,349 +9324,6 @@ TEST_F(HostResolverManagerDnsTest, MultipleDohProbeRequests) { EXPECT_FALSE(dns_client_->factory()->doh_probes_running()); } -TEST_F(HostResolverManagerDnsTest, EsniQuery) { - EsniContent c1, c2, c3; - IPAddress a1(1, 2, 3, 4), a2(5, 6, 7, 8); - IPAddress a3(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1); - - std::string kKey1 = GenerateWellFormedEsniKeys("a"); - std::string kKey2 = GenerateWellFormedEsniKeys("b"); - std::string kKey3 = GenerateWellFormedEsniKeys("c"); - - c1.AddKey(kKey1); - - c2.AddKeyForAddress(a1, kKey2); - c2.AddKeyForAddress(a2, kKey2); - c2.AddKeyForAddress(a3, kKey2); - - c3.AddKeyForAddress(a1, kKey3); - - std::vector<EsniContent> esni_records = {c1, c2, c3}; - - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result(BuildTestDnsEsniResponse( - "host", std::move(esni_records))), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsOk()); - - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - - // The IPv6 address |a3| should come first, and the other - // addresses should have been deduplicated. - EXPECT_THAT( - response.request()->GetAddressResults(), - Optional(AllOf(Property(&AddressList::endpoints, - UnorderedElementsAre(IPEndPoint(a3, 108), - IPEndPoint(a1, 108), - IPEndPoint(a2, 108))), - Property(&AddressList::front, IPEndPoint(a3, 108))))); - - // During aggregation of ESNI query results, we drop ESNI keys - // with no associated addresses, like key 1 here. (This is an implementation - // decision declining a "MAY" behavior from the spec.) - // So, we require that only keys 2 and 3 are surfaced. - // - // The Eq() wrappers are necessary here because keys_for_addresses - // returns a container of StringPieces. - EXPECT_THAT( - response.request()->GetEsniResults(), - Optional(AllOf( - Property(&EsniContent::keys, - UnorderedElementsAre(Eq(kKey2), Eq(kKey3))), - Property(&EsniContent::keys_for_addresses, - UnorderedElementsAre( - Pair(a1, UnorderedElementsAre(Eq(kKey2), Eq(kKey3))), - Pair(a2, UnorderedElementsAre(Eq(kKey2))), - Pair(a3, UnorderedElementsAre(Eq(kKey2)))))))); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_InvalidConfig) { - set_allow_fallback_to_proctask(false); - // Set empty DnsConfig. - InvalidateDnsConfig(); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_DNS_CACHE_MISS)); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_NonexistentDomain) { - // Setup fallback to confirm it is not used for non-address results. - set_allow_fallback_to_proctask(true); - proc_->AddRuleForAllFamilies("host", "192.168.1.102"); - proc_->SignalMultiple(1u); - - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::NODOMAIN), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_Failure) { - // Setup fallback to confirm it is not used for non-address results. - set_allow_fallback_to_proctask(true); - proc_->AddRuleForAllFamilies("host", "192.168.1.102"); - proc_->SignalMultiple(1u); - - MockDnsClientRuleList rules; - rules.emplace_back( - "host", dns_protocol::kExperimentalTypeEsniDraft4, false /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::FAIL), false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_Timeout) { - // Setup fallback to confirm it is not used for non-address results. - set_allow_fallback_to_proctask(true); - proc_->AddRuleForAllFamilies("host", "192.168.1.102"); - proc_->SignalMultiple(1u); - - MockDnsClientRuleList rules; - rules.emplace_back( - "host", dns_protocol::kExperimentalTypeEsniDraft4, false /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::TIMEOUT), false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_DNS_TIMED_OUT)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_Empty) { - // Setup fallback to confirm it is not used for non-address results. - set_allow_fallback_to_proctask(true); - proc_->AddRuleForAllFamilies("host", "192.168.1.102"); - proc_->SignalMultiple(1u); - - MockDnsClientRuleList rules; - rules.emplace_back( - "host", dns_protocol::kExperimentalTypeEsniDraft4, false /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::EMPTY), false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_Malformed) { - // Setup fallback to confirm it is not used for non-address results. - set_allow_fallback_to_proctask(true); - proc_->AddRuleForAllFamilies("host", "192.168.1.102"); - proc_->SignalMultiple(1u); - - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::MALFORMED), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_DNS_MALFORMED_RESPONSE)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_MismatchedName) { - EsniContent content; - IPAddress address(1, 2, 3, 4); - std::string key = GenerateWellFormedEsniKeys("a"); - content.AddKeyForAddress(address, key); - - std::vector<EsniContent> esni_records = {content}; - - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result(BuildTestDnsEsniResponse( - "host", std::move(esni_records), "not.host")), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_DNS_MALFORMED_RESPONSE)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -TEST_F(HostResolverManagerDnsTest, EsniQuery_WrongType) { - // Respond to an ESNI query with an A response. - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result( - BuildTestDnsResponse("host", IPAddress(1, 2, 3, 4))), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.dns_query_type = DnsQueryType::ESNI; - - // Responses for the wrong type should be ignored. - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("ok", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsError(ERR_NAME_NOT_RESOLVED)); - EXPECT_FALSE(response.request()->GetAddressResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -// Same as EsniQuery except we specify DNS HostResolverSource instead of relying -// on automatic determination. Expect same results since DNS should be what we -// automatically determine, but some slightly different logic paths are -// involved. -TEST_F(HostResolverManagerDnsTest, EsniDnsQuery) { - EsniContent c1, c2, c3; - IPAddress a1(1, 2, 3, 4), a2(5, 6, 7, 8); - - const std::string kKey1 = GenerateWellFormedEsniKeys("a"); - const std::string kKey2 = GenerateWellFormedEsniKeys("b"); - const std::string kKey3 = GenerateWellFormedEsniKeys("c"); - - c1.AddKey(kKey1); - - c2.AddKeyForAddress(a1, kKey2); - c2.AddKeyForAddress(a2, kKey2); - - c3.AddKeyForAddress(a1, kKey3); - - std::vector<EsniContent> esni_records = {c1, c2, c3}; - - MockDnsClientRuleList rules; - rules.emplace_back("host", dns_protocol::kExperimentalTypeEsniDraft4, - false /* secure */, - MockDnsClientRule::Result(BuildTestDnsEsniResponse( - "host", std::move(esni_records))), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - HostResolver::ResolveHostParameters parameters; - parameters.source = HostResolverSource::DNS; - parameters.dns_query_type = DnsQueryType::ESNI; - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 108), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - EXPECT_THAT(response.result_error(), IsOk()); - EXPECT_FALSE(response.request()->GetHostnameResults()); - EXPECT_FALSE(response.request()->GetTextResults()); - - // The multiple ESNI records should have been merged when parsing - // the results. - c1.MergeFrom(c2); - c1.MergeFrom(c3); - - // The ESNI records' addresses should have been merged into - // the address list. - ASSERT_TRUE(response.request()->GetAddressResults()); - EXPECT_THAT( - response.request()->GetAddressResults()->endpoints(), - testing::UnorderedElementsAre(IPEndPoint(a1, 108), IPEndPoint(a2, 108))); - - ASSERT_TRUE(response.request()->GetEsniResults().has_value()); - - // During aggregation of ESNI query results, we drop ESNI keys - // with no associated addresses, like key 1 here. (This is an implementation - // decision declining a "MAY" behavior from the spec.) So, we require that - // only keys 2 and 3 are surfaced. - EXPECT_THAT(response.request()->GetEsniResults()->keys(), - testing::UnorderedElementsAre(kKey2, kKey3)); - EXPECT_EQ(response.request()->GetEsniResults()->keys_for_addresses(), - c1.keys_for_addresses()); -} - // Test that a newly-registered ResolveContext is immediately usable with a DNS // configuration loaded before the context registration. TEST_F(HostResolverManagerDnsTest, @@ -9202,353 +9396,4 @@ TEST_F(HostResolverManagerDnsTest, resolver_->DeregisterResolveContext(&context); } -class HostResolverManagerEsniTest : public HostResolverManagerDnsTest { - public: - HostResolverManagerEsniTest() - : HostResolverManagerDnsTest( - base::test::TaskEnvironment::TimeSource::MOCK_TIME) { - scoped_feature_list_.InitAndEnableFeature(features::kRequestEsniDnsRecords); - } - - protected: - base::test::ScopedFeatureList scoped_feature_list_; - - // Adds a rule returning a collection of ESNI records such that - // - there is a lone key with no associated addresses - // - there is an address associated with multiple keys - // - there is a key associated with multiple addresses - // - // Returns a pair containing: - // (1) a single merged EsniContent object which should be contained in - // the eventual response. - // (2) the collection of IPEndPoints corresponding to the - // ESNI records' contained addresses; these are expected to - // be contained in the eventual response's address list (assuming - // no addresses are pruned by the address sorter, which will - // be the case in the test, because MockAddressSorter no-ops) - struct AddEsniRecordsRuleOptions { - bool secure = true, delay = false; - }; - std::pair<EsniContent, std::vector<IPEndPoint>> AddEsniRecordsRule( - base::StringPiece hostname, - AddEsniRecordsRuleOptions options, - MockDnsClientRuleList* rules) { - EsniContent c1, c2, c3; - IPAddress a1(1, 2, 3, 4); - IPAddress a2(5, 6, 7, 8); - - const std::string kKey1 = GenerateWellFormedEsniKeys("a"); - const std::string kKey2 = GenerateWellFormedEsniKeys("b"); - const std::string kKey3 = GenerateWellFormedEsniKeys("c"); - - c1.AddKey(kKey1); - - c2.AddKeyForAddress(a1, kKey2); - c2.AddKeyForAddress(a2, kKey2); - - c3.AddKeyForAddress(a1, kKey3); - - std::vector<EsniContent> esni_records = {c1, c2, c3}; - rules->emplace_back(std::string(hostname), - dns_protocol::kExperimentalTypeEsniDraft4, - options.secure, - MockDnsClientRule::Result(BuildTestDnsEsniResponse( - std::string(hostname), std::move(esni_records))), - options.delay); - - // Key 1 will be dropped because it corresponds to no addresses; - // section 4.2.2 of ESNI draft 4 gives implementors the option to associate - // these with all IP addresses received in concurrent A and AAAA responses, - // and we choose not to do this. - c2.MergeFrom(c3); - return std::make_pair( - c2, std::vector<IPEndPoint>{IPEndPoint(a1, 80), IPEndPoint(a2, 80)}); - } -}; - -// Check that resolving ESNI queries alongside A and AAAA queries -// results in a correct aggregation of addresses. -TEST_F(HostResolverManagerEsniTest, AggregatesResults) { - MockDnsClientRuleList rules; - - EsniContent esni_expectation; - std::vector<IPEndPoint> expected_addresses; - std::tie(esni_expectation, expected_addresses) = - AddEsniRecordsRule("host", AddEsniRecordsRuleOptions(), &rules); - - rules.emplace_back("host", dns_protocol::kTypeA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - false /* delay */); - rules.emplace_back("host", dns_protocol::kTypeAAAA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - false /* delay */); - // Even though the A and AAAA results' addresses won't have any - // associated ESNI keys, they should still be surfaced in GetAddressResults(). - expected_addresses.push_back(CreateExpected("127.0.0.1", 80)); - expected_addresses.push_back(CreateExpected("::1", 80)); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - DnsConfigOverrides overrides; - overrides.secure_dns_mode = DnsConfig::SecureDnsMode::AUTOMATIC; - resolver_->SetDnsConfigOverrides(overrides); - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 80), NetworkIsolationKey(), NetLogWithSource(), - HostResolver::ResolveHostParameters(), resolve_context_.get(), - resolve_context_->host_cache())); - - ASSERT_THAT(response.result_error(), IsOk()); - EXPECT_THAT(response.request()->GetEsniResults(), - testing::Optional(testing::Eq(esni_expectation))); - // GetAddressResults() should surface addresses with and without - // associated ESNI keys. - ASSERT_THAT(response.request()->GetAddressResults()->endpoints(), - testing::UnorderedElementsAreArray(expected_addresses)); -} - -// Test that addresses with associated ESNI keys are placed -// first in the order provided to the address sorter. -// (This corresponds to the order of the address list in the results -// because MockAddressSorter's sort is a no-op.) -TEST_F(HostResolverManagerEsniTest, EsniAddressesFirstInOrder) { - MockDnsClientRuleList rules; - - EsniContent esni_expectation; - std::vector<IPEndPoint> esni_addresses; - std::tie(esni_expectation, esni_addresses) = - AddEsniRecordsRule("host", AddEsniRecordsRuleOptions(), &rules); - - rules.emplace_back("host", dns_protocol::kTypeA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - false /* delay */); - rules.emplace_back("host", dns_protocol::kTypeAAAA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - false /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - DnsConfigOverrides overrides; - overrides.secure_dns_mode = DnsConfig::SecureDnsMode::AUTOMATIC; - resolver_->SetDnsConfigOverrides(overrides); - - HostResolver::ResolveHostParameters parameters; - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 80), NetworkIsolationKey(), NetLogWithSource(), - parameters, resolve_context_.get(), resolve_context_->host_cache())); - - // Check that the IP addresses with associated - // ESNI key objects occupy the initial entries of the - // address list returned by the DNS query. - ASSERT_THAT(response.result_error(), IsOk()); - ASSERT_TRUE(response.request()->GetAddressResults()); - const auto& result_addresses = - response.request()->GetAddressResults()->endpoints(); - for (const IPEndPoint& address_with_esni_keys : esni_addresses) { - int index = std::find(result_addresses.begin(), result_addresses.end(), - address_with_esni_keys) - - result_addresses.begin(); - - // Since this address has associated ESNI keys, it should be in - // the first esni_addresses.size() many entries of the result's - // address list. - ASSERT_TRUE(base::IsValueInRangeForNumericType<size_t>(index)); - EXPECT_LT(static_cast<size_t>(index), esni_addresses.size()); - } -} - -TEST_F(HostResolverManagerEsniTest, OnlyMakesRequestOverSecureDns) { - // Add some insecurely-accessible ESNI results alongside - // the default (insecurely-accessible) IPv4 and IPv6 results - // for the "ok" hostname. - MockDnsClientRuleList rules = CreateDefaultDnsRules(); - AddEsniRecordsRuleOptions options; - options.secure = false; - AddEsniRecordsRule("ok", options, &rules); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("ok", 80), NetworkIsolationKey(), NetLogWithSource(), - HostResolver::ResolveHostParameters(), resolve_context_.get(), - resolve_context_->host_cache())); - - ASSERT_THAT(response.result_error(), IsOk()); - - // Since the request wasn't secure, we shouldn't have - // queried for any ESNI results. - ASSERT_FALSE(response.request()->GetEsniResults()); -} - -// Make sure that ESNI queries don't get cancelled *before* the -// configured timeout, but do get cancelled after it, -// in the case where the absolute timeout dominates. -TEST_F(HostResolverManagerEsniTest, RespectsAbsoluteTimeout) { - // Add some delayed ESNI, IPv4, and IPv6 results - MockDnsClientRuleList rules = CreateDefaultDnsRules(); - AddEsniRecordsRuleOptions options; - options.delay = true; - AddEsniRecordsRule("host", options, &rules); - rules.emplace_back("host", dns_protocol::kTypeA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - true /* delay */); - rules.emplace_back("host", dns_protocol::kTypeAAAA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - true /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - DnsConfigOverrides overrides; - overrides.secure_dns_mode = DnsConfig::SecureDnsMode::AUTOMATIC; - resolver_->SetDnsConfigOverrides(overrides); - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 80), NetworkIsolationKey(), NetLogWithSource(), - HostResolver::ResolveHostParameters(), resolve_context_.get(), - resolve_context_->host_cache())); - - base::TimeDelta absolute_timeout = - features::EsniDnsMaxAbsoluteAdditionalWait(); - - // Let enough time pass during the A and AAAA transactions that the - // absolute timeout will be less than the relative timeout. - base::TimeDelta a_aaaa_elapsed = - 50 * (100.0 / features::kEsniDnsMaxRelativeAdditionalWaitPercent.Get()) * - absolute_timeout; - - FastForwardBy(a_aaaa_elapsed); - ASSERT_TRUE( - dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::A)); - ASSERT_TRUE( - dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); - - // Since the A and AAAA queries have only just completed, we shouldn't - // have timed out the ESNI query. - EXPECT_FALSE(response.complete()); - - // After half of the absolute timeout, the query should still be alive. - FastForwardBy(0.5 * absolute_timeout); - - // Since the absolute timeout has not yet elapsed, and it is shorter by - // design than the relative timeout, we shouldn't - // have timed out the ESNI transaction. - EXPECT_FALSE(response.complete()); - - // After (more than) the timeout has passed, we should have cancelled - // the ESNI transaction. - FastForwardBy(absolute_timeout); - ASSERT_THAT(response.result_error(), IsOk()); - - // Since we cancelled the transaction, we shouldn't have any ESNI results. - EXPECT_FALSE(response.request()->GetEsniResults()); -} - -// Make sure that ESNI queries don't get cancelled *before* the -// configured timeout, but do get cancelled after it, -// in the case where the relative timeout dominates. -TEST_F(HostResolverManagerEsniTest, RespectsRelativeTimeout) { - // Add some delayed ESNI, IPv4, and IPv6 results - MockDnsClientRuleList rules = CreateDefaultDnsRules(); - AddEsniRecordsRuleOptions options; - options.delay = true; - AddEsniRecordsRule("host", options, &rules); - rules.emplace_back("host", dns_protocol::kTypeA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - false /* delay */); - rules.emplace_back("host", dns_protocol::kTypeAAAA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - true /* delay */); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - DnsConfigOverrides overrides; - overrides.secure_dns_mode = DnsConfig::SecureDnsMode::AUTOMATIC; - resolver_->SetDnsConfigOverrides(overrides); - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 80), NetworkIsolationKey(), NetLogWithSource(), - HostResolver::ResolveHostParameters(), resolve_context_.get(), - resolve_context_->host_cache())); - - // Let little enough time pass during the A and AAAA transactions that the - // relative timeout will be less than the absolute timeout. - base::TimeDelta a_aaaa_elapsed = - 0.05 * features::EsniDnsMaxAbsoluteAdditionalWait() * - (100 / features::kEsniDnsMaxRelativeAdditionalWaitPercent.Get()); - - // Since the A and AAAA queries haven't both completed yet, we shouldn't time - // out the ESNI query. - FastForwardBy(a_aaaa_elapsed); - - // Upon completing the AAAA transaction, the ESNI timer should start - ASSERT_TRUE( - dns_client_->CompleteOneDelayedTransactionOfType(DnsQueryType::AAAA)); - - base::TimeDelta relative_timeout = - 0.01 * features::kEsniDnsMaxRelativeAdditionalWaitPercent.Get() * - a_aaaa_elapsed; - - // After *less* than the relative timeout, the query shouldn't have concluded. - FastForwardBy(relative_timeout * 0.5); - - EXPECT_FALSE(response.complete()); - - // After more than the relative timeout, the query should conclude by aborting - // the ESNI query. - FastForwardBy(relative_timeout * 0.5); - - // The task should have completed with a cancelled ESNI query. - ASSERT_THAT(response.result_error(), IsOk()); - EXPECT_FALSE(response.request()->GetEsniResults()); - ASSERT_TRUE(response.request()->GetAddressResults()); - EXPECT_THAT(response.request()->GetAddressResults()->endpoints(), - testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80), - CreateExpected("::1", 80))); -} - -// Test that we still receive delayed A/AAAA records -// that arrive after a successful (non-delayed) ESNI transaction. -TEST_F(HostResolverManagerEsniTest, WaitsForSlowAccompanyingQueries) { - MockDnsClientRuleList rules; - - EsniContent esni_expectation; - std::vector<IPEndPoint> expected_addresses; - std::tie(esni_expectation, expected_addresses) = - AddEsniRecordsRule("host", AddEsniRecordsRuleOptions(), &rules); - - rules.emplace_back("host", dns_protocol::kTypeA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - true /* delay */); - expected_addresses.push_back(CreateExpected("127.0.0.1", 80)); - - rules.emplace_back("host", dns_protocol::kTypeAAAA, true /* secure */, - MockDnsClientRule::Result(MockDnsClientRule::OK), - true /* delay */); - expected_addresses.push_back(CreateExpected("::1", 80)); - - CreateResolver(); - UseMockDnsClient(CreateValidDnsConfig(), std::move(rules)); - DnsConfigOverrides overrides; - overrides.secure_dns_mode = DnsConfig::SecureDnsMode::AUTOMATIC; - resolver_->SetDnsConfigOverrides(overrides); - - ResolveHostResponseHelper response(resolver_->CreateRequest( - HostPortPair("host", 80), NetworkIsolationKey(), NetLogWithSource(), - HostResolver::ResolveHostParameters(), resolve_context_.get(), - resolve_context_->host_cache())); - - // Wait quite a long time. (If the timer were erroneously to have been - // started, it should expire by the end of this elapsed window.) - FastForwardBy(features::EsniDnsMaxAbsoluteAdditionalWait() * 10); - dns_client_->CompleteDelayedTransactions(); - - EXPECT_THAT(response.result_error(), IsOk()); - EXPECT_THAT(response.request()->GetEsniResults(), - testing::Optional(testing::Eq(esni_expectation))); - ASSERT_TRUE(response.request()->GetAddressResults()); - EXPECT_THAT(response.request()->GetAddressResults()->endpoints(), - testing::UnorderedElementsAreArray(expected_addresses)); -} - } // namespace net diff --git a/chromium/net/dns/host_resolver_mdns_listener_impl.cc b/chromium/net/dns/host_resolver_mdns_listener_impl.cc index 88102dff25e..066cc635774 100644 --- a/chromium/net/dns/host_resolver_mdns_listener_impl.cc +++ b/chromium/net/dns/host_resolver_mdns_listener_impl.cc @@ -73,7 +73,7 @@ void HostResolverMdnsListenerImpl::OnRecordUpdate( switch (query_type_) { case DnsQueryType::UNSPECIFIED: - case DnsQueryType::ESNI: + case DnsQueryType::INTEGRITY: NOTREACHED(); break; case DnsQueryType::A: diff --git a/chromium/net/dns/host_resolver_mdns_task.cc b/chromium/net/dns/host_resolver_mdns_task.cc index 551ab7ae25c..745b847eede 100644 --- a/chromium/net/dns/host_resolver_mdns_task.cc +++ b/chromium/net/dns/host_resolver_mdns_task.cc @@ -198,8 +198,8 @@ HostCache::Entry HostResolverMdnsTask::ParseResult( switch (query_type) { case DnsQueryType::UNSPECIFIED: // Should create two separate transactions with specified type. - case DnsQueryType::ESNI: - // ESNI queries are not expected to be useful in mDNS, so they're not + case DnsQueryType::INTEGRITY: + // INTEGRITY queries are not expected to be useful in mDNS, so they're not // supported. NOTREACHED(); return HostCache::Entry(ERR_FAILED, HostCache::Entry::SOURCE_UNKNOWN); diff --git a/chromium/net/dns/httpssvc_metrics.cc b/chromium/net/dns/httpssvc_metrics.cc new file mode 100644 index 00000000000..453e2a32ed2 --- /dev/null +++ b/chromium/net/dns/httpssvc_metrics.cc @@ -0,0 +1,254 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/dns/httpssvc_metrics.h" + +#include "base/feature_list.h" +#include "base/metrics/histogram.h" +#include "base/metrics/histogram_base.h" +#include "base/metrics/histogram_functions.h" +#include "base/notreached.h" +#include "base/strings/string_split.h" +#include "base/strings/string_util.h" +#include "net/base/features.h" +#include "net/dns/dns_util.h" +#include "net/dns/public/dns_protocol.h" + +namespace net { + +enum HttpssvcDnsRcode TranslateDnsRcodeForHttpssvcExperiment(uint8_t rcode) { + switch (rcode) { + case dns_protocol::kRcodeNOERROR: + return HttpssvcDnsRcode::kNoError; + case dns_protocol::kRcodeFORMERR: + return HttpssvcDnsRcode::kFormErr; + case dns_protocol::kRcodeSERVFAIL: + return HttpssvcDnsRcode::kServFail; + case dns_protocol::kRcodeNXDOMAIN: + return HttpssvcDnsRcode::kNxDomain; + case dns_protocol::kRcodeNOTIMP: + return HttpssvcDnsRcode::kNotImp; + case dns_protocol::kRcodeREFUSED: + return HttpssvcDnsRcode::kRefused; + default: + return HttpssvcDnsRcode::kUnrecognizedRcode; + } + NOTREACHED(); +} + +HttpssvcExperimentDomainCache::HttpssvcExperimentDomainCache() = default; +HttpssvcExperimentDomainCache::~HttpssvcExperimentDomainCache() = default; + +bool HttpssvcExperimentDomainCache::ListContainsDomain( + const std::string& domain_list, + base::StringPiece domain, + base::Optional<base::flat_set<std::string>>& in_out_cached_list) { + if (!in_out_cached_list) { + in_out_cached_list = base::SplitString( + domain_list, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); + } + return in_out_cached_list->find(domain) != in_out_cached_list->end(); +} + +bool HttpssvcExperimentDomainCache::IsExperimental(base::StringPiece domain) { + if (!base::FeatureList::IsEnabled(features::kDnsHttpssvc)) + return false; + return ListContainsDomain(features::kDnsHttpssvcExperimentDomains.Get(), + domain, experimental_list_); +} + +bool HttpssvcExperimentDomainCache::IsControl(base::StringPiece domain) { + std::vector<base::StringPiece> control_domains; + if (!base::FeatureList::IsEnabled(features::kDnsHttpssvc)) + return false; + if (features::kDnsHttpssvcControlDomainWildcard.Get()) + return !IsExperimental(domain); + return ListContainsDomain(features::kDnsHttpssvcControlDomains.Get(), domain, + control_list_); +} + +HttpssvcMetrics::HttpssvcMetrics(bool expect_intact) + : expect_intact_(expect_intact) {} + +HttpssvcMetrics::~HttpssvcMetrics() { + RecordIntegrityMetrics(); +} + +void HttpssvcMetrics::SaveForNonIntegrity( + base::Optional<std::string> new_doh_provider_id, + base::TimeDelta resolve_time, + enum HttpssvcDnsRcode rcode) { + set_doh_provider_id(new_doh_provider_id); + + non_integrity_resolve_times_.push_back(resolve_time); + + if (rcode != HttpssvcDnsRcode::kNoError) + disqualified_ = true; +} + +void HttpssvcMetrics::SaveNonIntegrityFailure() { + disqualified_ = true; +} + +void HttpssvcMetrics::SaveForIntegrity( + base::Optional<std::string> new_doh_provider_id, + enum HttpssvcDnsRcode rcode_integrity, + const std::vector<bool>& condensed_records, + base::TimeDelta integrity_resolve_time) { + DCHECK(!rcode_integrity_.has_value()); + set_doh_provider_id(new_doh_provider_id); + + rcode_integrity_ = rcode_integrity; + + num_integrity_records_ = condensed_records.size(); + + // We only record one "Integrity" sample per INTEGRITY query. In case + // multiple matching records are in present in the response, we + // combine their intactness values with logical AND. + const bool intact = + std::all_of(condensed_records.cbegin(), condensed_records.cend(), + [](bool b) { return b; }); + + DCHECK(!is_integrity_intact_.has_value()); + is_integrity_intact_ = intact; + + DCHECK(!integrity_resolve_time_.has_value()); + integrity_resolve_time_ = integrity_resolve_time; +} + +void HttpssvcMetrics::set_doh_provider_id( + base::Optional<std::string> new_doh_provider_id) { + // "Other" never gets updated. + if (doh_provider_id_.has_value() && *doh_provider_id_ == "Other") + return; + + // If provider IDs mismatch, downgrade the new provider ID to "Other". + if ((doh_provider_id_.has_value() && !new_doh_provider_id.has_value()) || + (doh_provider_id_.has_value() && new_doh_provider_id.has_value() && + *doh_provider_id_ != *new_doh_provider_id)) { + new_doh_provider_id = "Other"; + } + + doh_provider_id_ = new_doh_provider_id; +} + +std::string HttpssvcMetrics::BuildMetricName( + base::StringPiece leaf_name) const { + // Build shared pieces of the metric names. + const base::StringPiece expectation = + expect_intact_ ? "ExpectIntact" : "ExpectNoerror"; + const std::string provider_id = doh_provider_id_.value_or("Other"); + + // Example INTEGRITY metric name: + // Net.DNS.HTTPSSVC.RecordIntegrity.CleanBrowsingAdult.ExpectIntact.DnsRcode + return base::JoinString({"Net.DNS.HTTPSSVC.RecordIntegrity", + provider_id.c_str(), expectation, leaf_name}, + "."); +} + +void HttpssvcMetrics::RecordIntegrityMetrics() { + // The HTTPSSVC experiment and its feature param indicating INTEGRITY must + // both be enabled. + DCHECK(base::FeatureList::IsEnabled(features::kDnsHttpssvc)); + DCHECK(features::kDnsHttpssvcUseIntegrity.Get()); + + DCHECK(in_progress_); + in_progress_ = false; + + // We really have no metrics to record without |integrity_resolve_time_| and + // |non_integrity_resolve_times_|. If this HttpssvcMetrics is in an + // inconsistent state, disqualify any metrics from being recorded. + if (!integrity_resolve_time_.has_value() || + non_integrity_resolve_times_.empty()) { + disqualified_ = true; + } + if (disqualified_) + return; + + // Record the metrics that the "ExpectIntact" and "ExpectNoerror" branches + // have in common. + RecordIntegrityCommonMetrics(); + + if (expect_intact_) { + // Record metrics that are unique to the "ExpectIntact" branch. + RecordIntegrityExpectIntactMetrics(); + } else { + // Record metrics that are unique to the "ExpectNoerror" branch. + RecordIntegrityExpectNoerrorMetrics(); + } +} + +void HttpssvcMetrics::RecordIntegrityCommonMetrics() { + base::UmaHistogramMediumTimes(BuildMetricName("ResolveTimeIntegrityRecord"), + *integrity_resolve_time_); + + const std::string kMetricResolveTimeNonIntegrityRecord = + BuildMetricName("ResolveTimeNonIntegrityRecord"); + for (base::TimeDelta resolve_time_other : non_integrity_resolve_times_) { + base::UmaHistogramMediumTimes(kMetricResolveTimeNonIntegrityRecord, + resolve_time_other); + } + + // ResolveTimeRatio is the INTEGRITY resolve time divided by the slower of the + // A or AAAA resolve times. Arbitrarily choosing precision at two decimal + // places. + std::vector<base::TimeDelta>::iterator slowest_non_integrity_resolve = + std::max_element(non_integrity_resolve_times_.begin(), + non_integrity_resolve_times_.end()); + DCHECK(slowest_non_integrity_resolve != non_integrity_resolve_times_.end()); + + // Compute a percentage showing how much larger the INTEGRITY resolve time was + // compared to the slowest A or AAAA query. + // + // Computation happens on TimeDelta objects, which use CheckedNumeric. This + // will crash if the system clock leaps forward several hundred millennia + // (numeric_limits<int64_t>::max() microseconds ~= 292,000 years). + const int64_t resolve_time_percent = + (100 * *integrity_resolve_time_) / *slowest_non_integrity_resolve; + + // Scale the value of |resolve_time_percent| by dividing by |kPercentScale|. + // Sample values are bounded between 1 and 20. A recorded sample of 10 means + // that the INTEGRITY resolve time took 100% of the slower A/AAAA resolve + // time. A sample of 20 means that the INTEGRITY resolve time was 200% + // relative to the A/AAAA resolve time, twice as long. + constexpr int64_t kMaxRatio = 20; + constexpr int64_t kPercentScale = 10; + base::UmaHistogramExactLinear(BuildMetricName("ResolveTimeRatio"), + resolve_time_percent / kPercentScale, + kMaxRatio); +} + +void HttpssvcMetrics::RecordIntegrityExpectIntactMetrics() { + // Without |rocde_integrity_|, we can't make progress on any of these metrics. + DCHECK(rcode_integrity_.has_value()); + + // The ExpectIntact variant of the "DnsRcode" metric is only recorded when no + // records are received. + if (num_integrity_records_ == 0) { + base::UmaHistogramEnumeration(BuildMetricName("DnsRcode"), + *rcode_integrity_); + } + if (num_integrity_records_ > 0) { + if (*rcode_integrity_ == HttpssvcDnsRcode::kNoError) { + base::UmaHistogramBoolean(BuildMetricName("Integrity"), + is_integrity_intact_.value_or(false)); + } else if (*rcode_integrity_ != HttpssvcDnsRcode::kNoError) { + // Record boolean indicating whether we received an INTEGRITY record and + // an error simultaneously. + base::UmaHistogramBoolean(BuildMetricName("RecordWithError"), true); + } + } +} + +void HttpssvcMetrics::RecordIntegrityExpectNoerrorMetrics() { + if (rcode_integrity_.has_value()) { + base::UmaHistogramEnumeration(BuildMetricName("DnsRcode"), + *rcode_integrity_); + } + if (num_integrity_records_ > 0) { + base::UmaHistogramBoolean(BuildMetricName("RecordReceived"), true); + } +} + +} // namespace net diff --git a/chromium/net/dns/httpssvc_metrics.h b/chromium/net/dns/httpssvc_metrics.h new file mode 100644 index 00000000000..76e4795cafc --- /dev/null +++ b/chromium/net/dns/httpssvc_metrics.h @@ -0,0 +1,112 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef NET_DNS_HTTPSSVC_METRICS_H_ +#define NET_DNS_HTTPSSVC_METRICS_H_ + +#include <string> +#include <vector> + +#include "base/containers/flat_set.h" +#include "base/optional.h" +#include "base/strings/string_piece_forward.h" +#include "base/time/time.h" +#include "net/base/net_export.h" + +namespace net { + +// These values are persisted to logs. Entries should not be renumbered and +// numeric values should never be reused. (See HttpssvcDnsRcode in +// tools/metrics/histograms/enums.xml.) +enum HttpssvcDnsRcode { + kTimedOut = 0, + kUnrecognizedRcode, + kMissingDnsResponse, + kNoError, + kFormErr, + kServFail, + kNxDomain, + kNotImp, + kRefused, + kMaxValue = kRefused, +}; + +// Helper that classifies domains as experimental, control, or other. Queries +// feature params and caches result to avoid repeated parsing. +class NET_EXPORT_PRIVATE HttpssvcExperimentDomainCache { + public: + HttpssvcExperimentDomainCache(); + ~HttpssvcExperimentDomainCache(); + bool IsExperimental(base::StringPiece domain); + bool IsControl(base::StringPiece domain); + + private: + bool ListContainsDomain( + const std::string& domain_list, + base::StringPiece domain, + base::Optional<base::flat_set<std::string>>& in_out_cached_list); + + base::Optional<base::flat_set<std::string>> experimental_list_; + base::Optional<base::flat_set<std::string>> control_list_; +}; + +// Translate an RCODE value to the |HttpssvcDnsRcode| enum, which is used for +// HTTPSSVC experimentation. The goal is to keep these values in a small, +// contiguous range in order to satisfy the UMA enumeration function's +// requirements. This function never returns |kTimedOut| |kUnrecognizedRcode|, +// or |kMissingDnsResponse|. +enum HttpssvcDnsRcode TranslateDnsRcodeForHttpssvcExperiment(uint8_t rcode); + +// Tool for aggregating HTTPSSVC and INTEGRITY metrics. Accumulates metrics via +// the Save* methods. Records metrics to UMA on destruction. +class NET_EXPORT_PRIVATE HttpssvcMetrics { + public: + explicit HttpssvcMetrics(bool expect_intact); + ~HttpssvcMetrics(); + HttpssvcMetrics(HttpssvcMetrics&) = delete; + HttpssvcMetrics(HttpssvcMetrics&&) = delete; + + // May be called many times. + void SaveForNonIntegrity(base::Optional<std::string> doh_provider_id, + base::TimeDelta resolve_time, + enum HttpssvcDnsRcode rcode); + + // Save the fact that the non-integrity queries failed. Prevents metrics from + // being recorded. + void SaveNonIntegrityFailure(); + + // Must only be called once. + void SaveForIntegrity(base::Optional<std::string> doh_provider_id, + enum HttpssvcDnsRcode rcode, + const std::vector<bool>& condensed_records, + base::TimeDelta integrity_resolve_time); + + private: + std::string BuildMetricName(base::StringPiece leaf_name) const; + + // Records all the aggregated metrics to UMA. + void RecordIntegrityMetrics(); + void RecordIntegrityCommonMetrics(); + void RecordIntegrityExpectIntactMetrics(); + void RecordIntegrityExpectNoerrorMetrics(); + + void set_doh_provider_id(base::Optional<std::string> doh_provider_id); + + // RecordIntegrityMetrics() will do nothing when |disqualified_| is true. + bool disqualified_ = false; + const bool expect_intact_; + bool in_progress_ = true; + base::Optional<std::string> doh_provider_id_; + base::Optional<enum HttpssvcDnsRcode> rcode_integrity_; + size_t num_integrity_records_ = 0; + base::Optional<bool> is_integrity_intact_; + // We never make multiple INTEGRITY queries per DnsTask, so we only need + // one TimeDelta for the INTEGRITY query. + base::Optional<base::TimeDelta> integrity_resolve_time_; + std::vector<base::TimeDelta> non_integrity_resolve_times_; +}; + +} // namespace net + +#endif // NET_DNS_HTTPSSVC_METRICS_H_ diff --git a/chromium/net/dns/httpssvc_metrics_unittest.cc b/chromium/net/dns/httpssvc_metrics_unittest.cc new file mode 100644 index 00000000000..1ce51cf7608 --- /dev/null +++ b/chromium/net/dns/httpssvc_metrics_unittest.cc @@ -0,0 +1,554 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/dns/httpssvc_metrics.h" + +#include <string> +#include <tuple> + +#include "base/feature_list.h" +#include "base/strings/strcat.h" +#include "base/strings/string_number_conversions.h" +#include "base/strings/string_util.h" +#include "base/test/metrics/histogram_tester.h" +#include "base/test/scoped_feature_list.h" +#include "net/base/features.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace net { + +// int: number of domains +// bool: extra leading comma +// bool: extra trailing comma +using DomainListQuirksTuple = std::tuple<int, bool, bool>; + +// bool: DnsHttpssvc feature is enabled +// bool: DnsHttpssvcUseIntegrity feature param +// bool: DnsHttpssvcUseHttpssvc feature param +// bool: DnsHttpssvcControlDomainWildcard feature param +using HttpssvcFeatureTuple = std::tuple<bool, bool, bool, bool>; + +// DomainListQuirksTuple: quirks for the experimental domain list. +// DomainListQuirksTuple: quirks for the control domain list. +// HttpssvcFeatureTuple: config for the whole DnsHttpssvc feature. +using ParsingTestParamTuple = std:: + tuple<DomainListQuirksTuple, DomainListQuirksTuple, HttpssvcFeatureTuple>; + +// bool: whether we are querying for an experimental domain or a control domain +// HttpssvcFeatureTuple: config for the whole DnsHttpssvc feature. +using MetricsTestParamTuple = std::tuple<bool, HttpssvcFeatureTuple>; + +// Create a comma-separated list of |domains| with the given |quirks|. +std::string FlattenDomainList(const std::vector<std::string>& domains, + DomainListQuirksTuple quirks) { + int num_domains; + bool leading_comma, trailing_comma; + std::tie(num_domains, leading_comma, trailing_comma) = quirks; + + CHECK_EQ(static_cast<size_t>(num_domains), domains.size()); + std::string flattened = base::JoinString(domains, ","); + if (leading_comma) + flattened.insert(flattened.begin(), ','); + if (trailing_comma) + flattened.push_back(','); + return flattened; +} + +// Intermediate representation constructed from test parameters. +struct HttpssvcFeatureConfig { + HttpssvcFeatureConfig() = default; + + explicit HttpssvcFeatureConfig(const HttpssvcFeatureTuple& feature_tuple, + base::StringPiece experiment_domains, + base::StringPiece control_domains) + : experiment_domains(experiment_domains.as_string()), + control_domains(control_domains.as_string()) { + std::tie(enabled, use_integrity, use_httpssvc, control_domain_wildcard) = + feature_tuple; + } + + void Apply(base::test::ScopedFeatureList* scoped_feature_list) const { + if (!enabled) { + scoped_feature_list->InitAndDisableFeature(features::kDnsHttpssvc); + return; + } + auto stringify = [](bool b) -> std::string { return b ? "true" : "false"; }; + scoped_feature_list->InitAndEnableFeatureWithParameters( + features::kDnsHttpssvc, + { + {"DnsHttpssvcUseHttpssvc", stringify(use_httpssvc)}, + {"DnsHttpssvcUseIntegrity", stringify(use_integrity)}, + {"DnsHttpssvcEnableQueryOverInsecure", "false"}, + {"DnsHttpssvcExperimentDomains", experiment_domains}, + {"DnsHttpssvcControlDomains", control_domains}, + {"DnsHttpssvcControlDomainWildcard", + stringify(control_domain_wildcard)}, + }); + } + + bool enabled = false; + bool use_integrity = false; + bool use_httpssvc = false; + bool control_domain_wildcard = false; + std::string experiment_domains; + std::string control_domains; +}; + +std::vector<std::string> GenerateDomainList(base::StringPiece label, int n) { + std::vector<std::string> domains; + for (int i = 0; i < n; i++) { + domains.push_back(base::StrCat( + {"domain", base::NumberToString(i), ".", label, ".example"})); + } + return domains; +} + +// Base for testing domain list parsing functions in +// net::features::dns_httpssvc_experiment. +class HttpssvcDomainParsingTest + : public ::testing::TestWithParam<ParsingTestParamTuple> { + public: + void SetUp() override { + DomainListQuirksTuple domain_quirks_experimental; + DomainListQuirksTuple domain_quirks_control; + HttpssvcFeatureTuple httpssvc_feature; + std::tie(domain_quirks_experimental, domain_quirks_control, + httpssvc_feature) = GetParam(); + + expected_experiment_domains_ = GenerateDomainList( + "experiment", std::get<0>(domain_quirks_experimental)); + expected_control_domains_ = + GenerateDomainList("control", std::get<0>(domain_quirks_control)); + + config_ = HttpssvcFeatureConfig( + httpssvc_feature, + FlattenDomainList(expected_experiment_domains_, + domain_quirks_experimental), + FlattenDomainList(expected_control_domains_, domain_quirks_control)); + config_.Apply(&scoped_feature_list_); + } + + const HttpssvcFeatureConfig& config() { return config_; } + + protected: + // The expected results of parsing the comma-separated domain lists in + // |experiment_domains| and |control_domains|, respectively. + std::vector<std::string> expected_experiment_domains_; + std::vector<std::string> expected_control_domains_; + + private: + HttpssvcFeatureConfig config_; + base::test::ScopedFeatureList scoped_feature_list_; +}; + +// This instantiation tests the domain list parser against various quirks, +// e.g. leading comma. +INSTANTIATE_TEST_SUITE_P( + HttpssvcMetricsTestDomainParsing, + HttpssvcDomainParsingTest, + testing::Combine( + // DomainListQuirksTuple for experimental domains. To fight back + // combinatorial explosion of tests, this tuple is pared down more than + // the one for control domains. This should not significantly hurt test + // coverage because |IsExperimentDomain| and |IsControlDomain| rely on a + // shared helper function. + testing::Combine(testing::Values(0, 1), + testing::Values(false), + testing::Values(false)), + // DomainListQuirksTuple for control domains. + testing::Combine(testing::Range(0, 3), + testing::Bool(), + testing::Bool()), + // HttpssvcFeatureTuple + testing::Combine( + testing::Bool() /* DnsHttpssvc feature enabled? */, + testing::Bool() /* DnsHttpssvcUseIntegrity */, + testing::Values(false) /* DnsHttpssvcUseHttpssvc */, + testing::Values(false) /* DnsHttpssvcControlDomainWildcard */))); + +// Base for testing the metrics collection code in |HttpssvcMetrics|. +class HttpssvcMetricsTest + : public ::testing::TestWithParam<MetricsTestParamTuple> { + public: + void SetUp() override { + HttpssvcFeatureTuple httpssvc_feature; + std::tie(querying_experimental_, httpssvc_feature) = GetParam(); + config_ = HttpssvcFeatureConfig(httpssvc_feature, "", ""); + config_.Apply(&scoped_feature_list_); + } + + std::string BuildMetricNamePrefix() const { + return base::StrCat( + {"Net.DNS.HTTPSSVC.RecordIntegrity.", doh_provider_, "."}); + } + + template <typename T> + void ExpectSample(base::StringPiece name, base::Optional<T> sample) const { + if (sample) + histo().ExpectUniqueSample(name, *sample, 1); + else + histo().ExpectTotalCount(name, 0); + } + + void ExpectSample(base::StringPiece name, + base::Optional<base::TimeDelta> sample) const { + base::Optional<int64_t> sample_ms; + if (sample) + sample_ms = {sample->InMilliseconds()}; + ExpectSample<int64_t>(name, sample_ms); + } + + void VerifyMetricsForExpectIntact( + base::Optional<HttpssvcDnsRcode> rcode, + base::Optional<bool> integrity, + base::Optional<bool> record_with_error, + base::Optional<base::TimeDelta> resolve_time_integrity, + base::Optional<base::TimeDelta> resolve_time_non_integrity, + base::Optional<int> resolve_time_ratio) const { + const std::string kPrefix = + base::StrCat({BuildMetricNamePrefix(), "ExpectIntact."}); + const std::string kMetricDnsRcode = base::StrCat({kPrefix, "DnsRcode"}); + const std::string kMetricIntegrity = base::StrCat({kPrefix, "Integrity"}); + const std::string kMetricRecordWithError = + base::StrCat({kPrefix, "RecordWithError"}); + const std::string kMetricResolveTimeIntegrity = + base::StrCat({kPrefix, "ResolveTimeIntegrityRecord"}); + const std::string kMetricResolveTimeNonIntegrity = + base::StrCat({kPrefix, "ResolveTimeNonIntegrityRecord"}); + const std::string kMetricResolveTimeRatio = + base::StrCat({kPrefix, "ResolveTimeRatio"}); + + ExpectSample(kMetricDnsRcode, rcode); + ExpectSample(kMetricIntegrity, integrity); + ExpectSample(kMetricRecordWithError, record_with_error); + ExpectSample(kMetricResolveTimeIntegrity, resolve_time_integrity); + ExpectSample(kMetricResolveTimeNonIntegrity, resolve_time_non_integrity); + ExpectSample(kMetricResolveTimeRatio, resolve_time_ratio); + } + + void VerifyMetricsForExpectNoerror( + base::Optional<HttpssvcDnsRcode> rcode, + base::Optional<int> record_received, + base::Optional<base::TimeDelta> resolve_time_integrity, + base::Optional<base::TimeDelta> resolve_time_non_integrity, + base::Optional<int> resolve_time_ratio) const { + const std::string kPrefix = + base::StrCat({BuildMetricNamePrefix(), "ExpectNoerror."}); + const std::string kMetricDnsRcode = base::StrCat({kPrefix, "DnsRcode"}); + const std::string kMetricRecordReceived = + base::StrCat({kPrefix, "RecordReceived"}); + const std::string kMetricResolveTimeIntegrity = + base::StrCat({kPrefix, "ResolveTimeIntegrityRecord"}); + const std::string kMetricResolveTimeNonIntegrity = + base::StrCat({kPrefix, "ResolveTimeNonIntegrityRecord"}); + const std::string kMetricResolveTimeRatio = + base::StrCat({kPrefix, "ResolveTimeRatio"}); + + ExpectSample(kMetricDnsRcode, rcode); + ExpectSample(kMetricRecordReceived, record_received); + ExpectSample(kMetricResolveTimeIntegrity, resolve_time_integrity); + ExpectSample(kMetricResolveTimeNonIntegrity, resolve_time_non_integrity); + ExpectSample(kMetricResolveTimeRatio, resolve_time_ratio); + } + + void VerifyMetricsForExpectIntact() { + VerifyMetricsForExpectIntact(base::nullopt, base::nullopt, base::nullopt, + base::nullopt, base::nullopt, base::nullopt); + } + + void VerifyMetricsForExpectNoerror() { + VerifyMetricsForExpectNoerror(base::nullopt, base::nullopt, base::nullopt, + base::nullopt, base::nullopt); + } + + const base::HistogramTester& histo() const { return histogram_; } + const HttpssvcFeatureConfig& config() const { return config_; } + + protected: + bool querying_experimental_; + + private: + HttpssvcFeatureConfig config_; + base::test::ScopedFeatureList scoped_feature_list_; + base::HistogramTester histogram_; + std::string doh_provider_ = "Other"; +}; + +// This instantiation focuses on whether the correct metrics are recorded. The +// domain list parser is already tested against encoding quirks in +// |HttpssvcMetricsTestDomainParsing|, so we fix the quirks at false. +INSTANTIATE_TEST_SUITE_P( + HttpssvcMetricsTestSimple, + HttpssvcMetricsTest, + testing::Combine( + // Whether we are querying an experimental domain. + testing::Bool(), + // HttpssvcFeatureTuple + testing::Combine( + testing::Values(true) /* DnsHttpssvc feature enabled? */, + testing::Values(true) /* DnsHttpssvcUseIntegrity */, + testing::Values(false) /* DnsHttpssvcUseHttpssvc */, + testing::Values(false) /* DnsHttpssvcControlDomainWildcard */))); + +TEST_P(HttpssvcDomainParsingTest, ParseFeatureParamIntegrityDomains) { + HttpssvcExperimentDomainCache domain_cache; + + // We are not testing this feature param yet. + CHECK(!config().use_httpssvc); + + const std::string kReservedDomain = "neither.example"; + EXPECT_FALSE(domain_cache.IsExperimental(kReservedDomain)); + EXPECT_EQ(domain_cache.IsControl(kReservedDomain), + config().enabled && config().control_domain_wildcard); + + // If |config().use_integrity| is true, then we expect all domains in + // |expected_experiment_domains_| to be experimental (same goes for + // control domains). Otherwise, no domain should be considered experimental or + // control. + + if (!config().enabled) { + // When the HTTPSSVC feature is disabled, no domain should be considered + // experimental or control. + for (const std::string& experiment_domain : expected_experiment_domains_) { + EXPECT_FALSE(domain_cache.IsExperimental(experiment_domain)); + EXPECT_FALSE(domain_cache.IsControl(experiment_domain)); + } + for (const std::string& control_domain : expected_control_domains_) { + EXPECT_FALSE(domain_cache.IsExperimental(control_domain)); + EXPECT_FALSE(domain_cache.IsControl(control_domain)); + } + } else if (config().use_integrity) { + for (const std::string& experiment_domain : expected_experiment_domains_) { + EXPECT_TRUE(domain_cache.IsExperimental(experiment_domain)); + EXPECT_FALSE(domain_cache.IsControl(experiment_domain)); + } + for (const std::string& control_domain : expected_control_domains_) { + EXPECT_FALSE(domain_cache.IsExperimental(control_domain)); + EXPECT_TRUE(domain_cache.IsControl(control_domain)); + } + return; + } +} + +// Only record metrics for a non-integrity query. +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityMissing) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); +} + +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityIntact) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + const base::TimeDelta kResolveTimeIntegrity = + base::TimeDelta::FromMilliseconds(15); + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForIntegrity(base::nullopt, HttpssvcDnsRcode::kNoError, {true}, + kResolveTimeIntegrity); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + if (querying_experimental_) { + VerifyMetricsForExpectIntact( + base::nullopt /* rcode */, {true} /* integrity */, + base::nullopt /* record_with_error */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); + + VerifyMetricsForExpectNoerror(); + return; + } + + VerifyMetricsForExpectIntact(); + + VerifyMetricsForExpectNoerror( + {HttpssvcDnsRcode::kNoError} /* rcode */, {1} /* record_received */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); +} + +// This test simulates an INTEGRITY response that includes no INTEGRITY records, +// but does have an error value for the RCODE. +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityMissingWithRcode) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + const base::TimeDelta kResolveTimeIntegrity = + base::TimeDelta::FromMilliseconds(15); + + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForIntegrity(base::nullopt, HttpssvcDnsRcode::kNxDomain, {}, + kResolveTimeIntegrity); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + if (querying_experimental_) { + VerifyMetricsForExpectIntact( + {HttpssvcDnsRcode::kNxDomain} /* rcode */, + base::nullopt /* integrity */, base::nullopt /* record_with_error */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); + + VerifyMetricsForExpectNoerror(); + return; + } + + VerifyMetricsForExpectIntact(); + + VerifyMetricsForExpectNoerror( + {HttpssvcDnsRcode::kNxDomain} /* rcode */, + base::nullopt /* record_received */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); +} + +// This test simulates an INTEGRITY response that includes an intact INTEGRITY +// record, but also has an error RCODE. +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityIntactWithRcode) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + const base::TimeDelta kResolveTimeIntegrity = + base::TimeDelta::FromMilliseconds(15); + + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForIntegrity(base::nullopt, HttpssvcDnsRcode::kNxDomain, {true}, + kResolveTimeIntegrity); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + if (querying_experimental_) { + VerifyMetricsForExpectIntact( + // "DnsRcode" metric is omitted because we received an INTEGRITY record. + base::nullopt /* rcode */, + // "Integrity" metric is omitted because the RCODE is not NOERROR. + base::nullopt /* integrity */, {true} /* record_with_error */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); + + VerifyMetricsForExpectNoerror(); + return; + } + + VerifyMetricsForExpectIntact(); + + VerifyMetricsForExpectNoerror( + {HttpssvcDnsRcode::kNxDomain} /* rcode */, {true} /* record_received */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); +} + +// This test simulates an INTEGRITY response that includes a mangled INTEGRITY +// record *and* has an error RCODE. +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityMangledWithRcode) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + const base::TimeDelta kResolveTimeIntegrity = + base::TimeDelta::FromMilliseconds(15); + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForIntegrity(base::nullopt, HttpssvcDnsRcode::kNxDomain, {false}, + kResolveTimeIntegrity); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + if (querying_experimental_) { + VerifyMetricsForExpectIntact( + // "DnsRcode" metric is omitted because we received an INTEGRITY record. + base::nullopt /* rcode */, + // "Integrity" metric is omitted because the RCODE is not NOERROR. + base::nullopt /* integrity */, {true} /* record_with_error */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); + + VerifyMetricsForExpectNoerror(); + return; + } + + VerifyMetricsForExpectIntact(); + + VerifyMetricsForExpectNoerror( + {HttpssvcDnsRcode::kNxDomain} /* rcode */, {true} /* record_received */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); +} + +// This test simulates successful address queries and an INTEGRITY query that +// timed out. +TEST_P(HttpssvcMetricsTest, AddressAndIntegrityTimedOut) { + if (!config().enabled || !config().use_integrity) { + VerifyMetricsForExpectIntact(); + VerifyMetricsForExpectNoerror(); + return; + } + const base::TimeDelta kResolveTime = base::TimeDelta::FromMilliseconds(10); + const base::TimeDelta kResolveTimeIntegrity = + base::TimeDelta::FromMilliseconds(15); + base::Optional<HttpssvcMetrics> metrics(querying_experimental_); + metrics->SaveForIntegrity(base::nullopt, HttpssvcDnsRcode::kTimedOut, {}, + kResolveTimeIntegrity); + metrics->SaveForNonIntegrity(base::nullopt, kResolveTime, + HttpssvcDnsRcode::kNoError); + metrics.reset(); // Record the metrics to UMA. + + if (querying_experimental_) { + VerifyMetricsForExpectIntact( + {HttpssvcDnsRcode::kTimedOut} /* rcode */, + // "Integrity" metric is omitted because the RCODE is not NOERROR. + base::nullopt /* integrity */, base::nullopt /* record_with_error */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); + + VerifyMetricsForExpectNoerror(); + return; + } + + VerifyMetricsForExpectIntact(); + + VerifyMetricsForExpectNoerror( + {HttpssvcDnsRcode::kTimedOut} /* rcode */, + base::nullopt /* record_received */, + {kResolveTimeIntegrity} /* resolve_time_integrity */, + {kResolveTime} /* resolve_time_non_integrity */, + {15} /* resolve_time_ratio */); +} + +} // namespace net diff --git a/chromium/net/dns/mock_host_resolver.cc b/chromium/net/dns/mock_host_resolver.cc index d66b8e4a1fb..e0a23ae727b 100644 --- a/chromium/net/dns/mock_host_resolver.cc +++ b/chromium/net/dns/mock_host_resolver.cc @@ -139,12 +139,6 @@ class MockHostResolverBase::RequestImpl return *nullopt_result; } - const base::Optional<EsniContent>& GetEsniResults() const override { - DCHECK(complete_); - static const base::NoDestructor<base::Optional<EsniContent>> nullopt_result; - return *nullopt_result; - } - net::ResolveErrorInfo GetResolveErrorInfo() const override { DCHECK(complete_); return resolve_error_info_; @@ -1028,10 +1022,6 @@ class HangingHostResolver::RequestImpl IMMEDIATE_CRASH(); } - const base::Optional<EsniContent>& GetEsniResults() const override { - IMMEDIATE_CRASH(); - } - net::ResolveErrorInfo GetResolveErrorInfo() const override { IMMEDIATE_CRASH(); } diff --git a/chromium/net/dns/public/BUILD.gn b/chromium/net/dns/public/BUILD.gn index 04f2a9b57b3..832312ddeee 100644 --- a/chromium/net/dns/public/BUILD.gn +++ b/chromium/net/dns/public/BUILD.gn @@ -19,8 +19,8 @@ source_set("public") { "dns_protocol.h", "dns_query_type.cc", "dns_query_type.h", - "doh_provider_list.cc", - "doh_provider_list.h", + "doh_provider_entry.cc", + "doh_provider_entry.h", "resolve_error_info.cc", "resolve_error_info.h", "util.cc", @@ -35,7 +35,7 @@ source_set("public") { source_set("tests") { testonly = true sources = [ - "doh_provider_list_unittest.cc", + "doh_provider_entry_unittest.cc", "util_unittest.cc", ] diff --git a/chromium/net/dns/public/dns_protocol.h b/chromium/net/dns/public/dns_protocol.h index ea9112feb00..c77dbaa7cd4 100644 --- a/chromium/net/dns/public/dns_protocol.h +++ b/chromium/net/dns/public/dns_protocol.h @@ -153,11 +153,6 @@ static const uint16_t kTypeANY = 255; // Experimental DNS record types pending IANA assignment. // -// Record type proposed for TLS Encrypted Server Name Indication -// (ESNI, draft 4) records: -// https://tools.ietf.org/html/draft-ietf-tls-esni-04#section-8.3 -static const uint16_t kExperimentalTypeEsniDraft4 = 65439; - // The INTEGRITY RR type exists purely for measuring how the DNS ecosystem // handles new RR types. // https://docs.google.com/document/d/14eCqVyT_3MSj7ydqNFl1Yl0yg1fs6g24qmYUUdi5V-k/edit?usp=sharing diff --git a/chromium/net/dns/public/dns_query_type.h b/chromium/net/dns/public/dns_query_type.h index 7d407a62d01..ecc61ea597d 100644 --- a/chromium/net/dns/public/dns_query_type.h +++ b/chromium/net/dns/public/dns_query_type.h @@ -20,14 +20,15 @@ enum class DnsQueryType { TXT, PTR, SRV, - ESNI, - MAX = ESNI + INTEGRITY, + MAX = INTEGRITY }; const DnsQueryType kDnsQueryTypes[] = { DnsQueryType::UNSPECIFIED, DnsQueryType::A, DnsQueryType::AAAA, DnsQueryType::TXT, DnsQueryType::PTR, DnsQueryType::SRV, - DnsQueryType::ESNI}; + DnsQueryType::INTEGRITY, +}; static_assert(base::size(kDnsQueryTypes) == static_cast<unsigned>(DnsQueryType::MAX) + 1, diff --git a/chromium/net/dns/public/doh_provider_list.cc b/chromium/net/dns/public/doh_provider_entry.cc index b5b2a50ef89..0422c8c6a7a 100644 --- a/chromium/net/dns/public/doh_provider_list.cc +++ b/chromium/net/dns/public/doh_provider_entry.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "net/dns/public/doh_provider_list.h" +#include "net/dns/public/doh_provider_entry.h" #include <utility> @@ -12,54 +12,28 @@ namespace net { -DohProviderEntry::DohProviderEntry( - std::string provider, - base::Optional<DohProviderIdForHistogram> provider_id_for_histogram, - std::set<std::string> ip_strs, - std::set<std::string> dns_over_tls_hostnames, - std::string dns_over_https_template, - std::string ui_name, - std::string privacy_policy, - bool display_globally, - std::set<std::string> display_countries) - : provider(std::move(provider)), - provider_id_for_histogram(std::move(provider_id_for_histogram)), - dns_over_tls_hostnames(std::move(dns_over_tls_hostnames)), - dns_over_https_template(std::move(dns_over_https_template)), - ui_name(std::move(ui_name)), - privacy_policy(std::move(privacy_policy)), - display_globally(display_globally), - display_countries(std::move(display_countries)) { - DCHECK(!this->dns_over_https_template.empty()); - DCHECK(dns_util::IsValidDohTemplate(this->dns_over_https_template, - nullptr /* server_method */)); +namespace { - DCHECK(!display_globally || this->display_countries.empty()); - if (display_globally || !this->display_countries.empty()) { - DCHECK(!this->ui_name.empty()); - DCHECK(!this->privacy_policy.empty()); - DCHECK(this->provider_id_for_histogram.has_value()); - } - for (const auto& display_country : this->display_countries) { - DCHECK_EQ(2u, display_country.size()); - } - for (const std::string& ip_str : ip_strs) { +std::set<IPAddress> ParseIPs(const std::set<base::StringPiece>& ip_strs) { + std::set<IPAddress> ip_addresses; + for (base::StringPiece ip_str : ip_strs) { IPAddress ip_address; bool success = ip_address.AssignFromIPLiteral(ip_str); DCHECK(success); - ip_addresses.insert(ip_address); + ip_addresses.insert(std::move(ip_address)); } + return ip_addresses; } -DohProviderEntry::DohProviderEntry(const DohProviderEntry& other) = default; -DohProviderEntry::~DohProviderEntry() = default; +} // namespace -const std::vector<DohProviderEntry>& GetDohProviderList() { +// static +const DohProviderEntry::List& DohProviderEntry::GetList() { // The provider names in these entries should be kept in sync with the // DohProviderId histogram suffix list in // tools/metrics/histograms/histograms.xml. - static const base::NoDestructor<std::vector<DohProviderEntry>> providers{{ - DohProviderEntry( + static const base::NoDestructor<DohProviderEntry::List> providers{{ + new DohProviderEntry( "CleanBrowsingAdult", base::nullopt /* provider_id_for_histogram */, {"185.228.168.10", "185.228.169.11", "2a0d:2a00:1::1", "2a0d:2a00:2::1"}, @@ -67,7 +41,7 @@ const std::vector<DohProviderEntry>& GetDohProviderList() { "https://doh.cleanbrowsing.org/doh/adult-filter{?dns}", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "CleanBrowsingFamily", DohProviderIdForHistogram::kCleanBrowsingFamily, {"185.228.168.168", "185.228.169.168", @@ -77,7 +51,7 @@ const std::vector<DohProviderEntry>& GetDohProviderList() { "CleanBrowsing (Family Filter)" /* ui_name */, "https://cleanbrowsing.org/privacy" /* privacy_policy */, true /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "CleanBrowsingSecure", base::nullopt /* provider_id_for_histogram */, {"185.228.168.9", "185.228.169.9", "2a0d:2a00:1::2", "2a0d:2a00:2::2"}, @@ -85,7 +59,7 @@ const std::vector<DohProviderEntry>& GetDohProviderList() { "https://doh.cleanbrowsing.org/doh/security-filter{?dns}", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "Cloudflare", DohProviderIdForHistogram::kCloudflare, {"1.1.1.1", "1.0.0.1", "2606:4700:4700::1111", "2606:4700:4700::1001"}, @@ -96,63 +70,64 @@ const std::vector<DohProviderEntry>& GetDohProviderList() { "https://developers.cloudflare.com/1.1.1.1/privacy/" "public-dns-resolver/" /* privacy_policy */, true /* display_globally */, {} /* display_countries */), - DohProviderEntry("Comcast", base::nullopt /* provider_id_for_histogram */, - {"75.75.75.75", "75.75.76.76", "2001:558:feed::1", - "2001:558:feed::2"}, - {"dot.xfinity.com"} /* dns_over_tls_hostnames */, - "https://doh.xfinity.com/dns-query{?dns}", - "" /* ui_name */, "" /* privacy_policy */, - false /* display_globally */, - {} /* display_countries */), - DohProviderEntry("Cznic", base::nullopt /* provider_id_for_histogram */, - {"185.43.135.1", "2001:148f:fffe::1"}, - {"odvr.nic.cz"} /* dns_over_tls_hostnames */, - "https://odvr.nic.cz/doh", "" /* ui_name */, - "" /* privacy_policy */, false /* display_globally */, - {} /* display_countries */), + new DohProviderEntry( + "Comcast", base::nullopt /* provider_id_for_histogram */, + {"75.75.75.75", "75.75.76.76", "2001:558:feed::1", + "2001:558:feed::2"}, + {"dot.xfinity.com"} /* dns_over_tls_hostnames */, + "https://doh.xfinity.com/dns-query{?dns}", "" /* ui_name */, + "" /* privacy_policy */, false /* display_globally */, + {} /* display_countries */), + new DohProviderEntry( + "Cznic", base::nullopt /* provider_id_for_histogram */, + {"185.43.135.1", "2001:148f:fffe::1"}, + {"odvr.nic.cz"} /* dns_over_tls_hostnames */, + "https://odvr.nic.cz/doh", "" /* ui_name */, "" /* privacy_policy */, + false /* display_globally */, {} /* display_countries */), // Note: DNS.SB has separate entries for autoupgrade and settings UI to // allow the extra |no_ecs| parameter for autoupgrade. This parameter // disables EDNS Client Subnet (ECS) handling in order to match the // behavior of the upgraded-from classic DNS server. - DohProviderEntry( + new DohProviderEntry( "Dnssb", base::nullopt /* provider_id_for_histogram */, {"185.222.222.222", "185.184.222.222", "2a09::", "2a09::1"}, {"dns.sb"} /* dns_over_tls_hostnames */, "https://doh.dns.sb/dns-query?no_ecs=true{&dns}", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "DnssbUserSelected", DohProviderIdForHistogram::kDnsSb, {} /* ip_strs */, {} /* dns_over_tls_hostnames */, "https://doh.dns.sb/dns-query{?dns}", "DNS.SB" /* ui_name */, "https://dns.sb/privacy/" /* privacy_policy */, false /* display_globally */, {"EE", "DE"} /* display_countries */), - DohProviderEntry("Google", DohProviderIdForHistogram::kGoogle, - {"8.8.8.8", "8.8.4.4", "2001:4860:4860::8888", - "2001:4860:4860::8844"}, - {"dns.google", "dns.google.com", - "8888.google"} /* dns_over_tls_hostnames */, - "https://dns.google/dns-query{?dns}", - "Google (Public DNS)" /* ui_name */, - "https://developers.google.com/speed/public-dns/" - "privacy" /* privacy_policy */, - true /* display_globally */, {} /* display_countries */), - DohProviderEntry("Iij", DohProviderIdForHistogram::kIij, {} /* ip_strs */, - {} /* dns_over_tls_hostnames */, - "https://public.dns.iij.jp/dns-query", - "IIJ (Public DNS)" /* ui_name */, - "https://public.dns.iij.jp/" /* privacy_policy */, - false /* display_globally */, - {"JP"} /* display_countries */), - DohProviderEntry("OpenDNS", base::nullopt /* provider_id_for_histogram */, - {"208.67.222.222", "208.67.220.220", "2620:119:35::35", - "2620:119:53::53"}, - {""} /* dns_over_tls_hostnames */, - "https://doh.opendns.com/dns-query{?dns}", - "" /* ui_name */, "" /* privacy_policy */, - false /* display_globally */, - {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry("Google", DohProviderIdForHistogram::kGoogle, + {"8.8.8.8", "8.8.4.4", "2001:4860:4860::8888", + "2001:4860:4860::8844"}, + {"dns.google", "dns.google.com", + "8888.google"} /* dns_over_tls_hostnames */, + "https://dns.google/dns-query{?dns}", + "Google (Public DNS)" /* ui_name */, + "https://developers.google.com/speed/public-dns/" + "privacy" /* privacy_policy */, + true /* display_globally */, + {} /* display_countries */), + new DohProviderEntry("Iij", DohProviderIdForHistogram::kIij, + {} /* ip_strs */, {} /* dns_over_tls_hostnames */, + "https://public.dns.iij.jp/dns-query", + "IIJ (Public DNS)" /* ui_name */, + "https://public.dns.iij.jp/" /* privacy_policy */, + false /* display_globally */, + {"JP"} /* display_countries */), + new DohProviderEntry( + "OpenDNS", base::nullopt /* provider_id_for_histogram */, + {"208.67.222.222", "208.67.220.220", "2620:119:35::35", + "2620:119:53::53"}, + {""} /* dns_over_tls_hostnames */, + "https://doh.opendns.com/dns-query{?dns}", "" /* ui_name */, + "" /* privacy_policy */, false /* display_globally */, + {} /* display_countries */), + new DohProviderEntry( "OpenDNSFamily", base::nullopt /* provider_id_for_histogram */, {"208.67.222.123", "208.67.220.123", "2620:119:35::123", "2620:119:53::123"}, @@ -160,36 +135,94 @@ const std::vector<DohProviderEntry>& GetDohProviderList() { "https://doh.familyshield.opendns.com/dns-query{?dns}", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "Quad9Cdn", base::nullopt /* provider_id_for_histogram */, {"9.9.9.11", "149.112.112.11", "2620:fe::11", "2620:fe::fe:11"}, {"dns11.quad9.net"} /* dns_over_tls_hostnames */, "https://dns11.quad9.net/dns-query", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "Quad9Insecure", base::nullopt /* provider_id_for_histogram */, {"9.9.9.10", "149.112.112.10", "2620:fe::10", "2620:fe::fe:10"}, {"dns10.quad9.net"} /* dns_over_tls_hostnames */, "https://dns10.quad9.net/dns-query", "" /* ui_name */, "" /* privacy_policy */, false /* display_globally */, {} /* display_countries */), - DohProviderEntry( + new DohProviderEntry( "Quad9Secure", DohProviderIdForHistogram::kQuad9Secure, {"9.9.9.9", "149.112.112.112", "2620:fe::fe", "2620:fe::9"}, {"dns.quad9.net", "dns9.quad9.net"} /* dns_over_tls_hostnames */, "https://dns.quad9.net/dns-query", "Quad9 (9.9.9.9)" /* ui_name */, "https://www.quad9.net/home/privacy/" /* privacy_policy */, true /* display_globally */, {} /* display_countries */), - DohProviderEntry("Switch", base::nullopt /* provider_id_for_histogram */, - {"130.59.31.251", "130.59.31.248", "2001:620:0:ff::2", - "2001:620:0:ff::3"}, - {"dns.switch.ch"} /* dns_over_tls_hostnames */, - "https://dns.switch.ch/dns-query", "" /* ui_name */, - "" /* privacy_policy */, false /* display_globally */, - {} /* display_countries */), + new DohProviderEntry( + "Switch", base::nullopt /* provider_id_for_histogram */, + {"130.59.31.251", "130.59.31.248", "2001:620:0:ff::2", + "2001:620:0:ff::3"}, + {"dns.switch.ch"} /* dns_over_tls_hostnames */, + "https://dns.switch.ch/dns-query", "" /* ui_name */, + "" /* privacy_policy */, false /* display_globally */, + {} /* display_countries */), }}; return *providers; } +// static +DohProviderEntry DohProviderEntry::ConstructForTesting( + std::string provider, + base::Optional<DohProviderIdForHistogram> provider_id_for_histogram, + std::set<base::StringPiece> ip_strs, + std::set<std::string> dns_over_tls_hostnames, + std::string dns_over_https_template, + std::string ui_name, + std::string privacy_policy, + bool display_globally, + std::set<std::string> display_countries) { + return DohProviderEntry(provider, provider_id_for_histogram, ip_strs, + dns_over_tls_hostnames, dns_over_https_template, + ui_name, privacy_policy, display_globally, + display_countries); +} + +DohProviderEntry::DohProviderEntry(DohProviderEntry&& other) = default; +DohProviderEntry& DohProviderEntry::operator=(DohProviderEntry&& other) = + default; + +DohProviderEntry::~DohProviderEntry() = default; + +DohProviderEntry::DohProviderEntry( + std::string provider, + base::Optional<DohProviderIdForHistogram> provider_id_for_histogram, + std::set<base::StringPiece> ip_strs, + std::set<std::string> dns_over_tls_hostnames, + std::string dns_over_https_template, + std::string ui_name, + std::string privacy_policy, + bool display_globally, + std::set<std::string> display_countries) + : provider(std::move(provider)), + provider_id_for_histogram(std::move(provider_id_for_histogram)), + ip_addresses(ParseIPs(ip_strs)), + dns_over_tls_hostnames(std::move(dns_over_tls_hostnames)), + dns_over_https_template(std::move(dns_over_https_template)), + ui_name(std::move(ui_name)), + privacy_policy(std::move(privacy_policy)), + display_globally(display_globally), + display_countries(std::move(display_countries)) { + DCHECK(!this->dns_over_https_template.empty()); + DCHECK(dns_util::IsValidDohTemplate(this->dns_over_https_template, + nullptr /* server_method */)); + + DCHECK(!display_globally || this->display_countries.empty()); + if (display_globally || !this->display_countries.empty()) { + DCHECK(!this->ui_name.empty()); + DCHECK(!this->privacy_policy.empty()); + DCHECK(this->provider_id_for_histogram.has_value()); + } + for (const auto& display_country : this->display_countries) { + DCHECK_EQ(2u, display_country.size()); + } +} + } // namespace net diff --git a/chromium/net/dns/public/doh_provider_list.h b/chromium/net/dns/public/doh_provider_entry.h index 5b3050874a3..150ea48bef5 100644 --- a/chromium/net/dns/public/doh_provider_list.h +++ b/chromium/net/dns/public/doh_provider_entry.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_ -#define NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_ +#ifndef NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_ +#define NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_ #include <set> #include <string> @@ -41,37 +41,56 @@ enum class DohProviderIdForHistogram { // codes, if any, where the entry is eligible for being displayed in the // dropdown menu. struct NET_EXPORT DohProviderEntry { - DohProviderEntry( + public: + using List = std::vector<const DohProviderEntry*>; + + std::string provider; + // A provider_id_for_histogram is required for entries that are intended to + // be visible in the UI. + base::Optional<DohProviderIdForHistogram> provider_id_for_histogram; + std::set<IPAddress> ip_addresses; + std::set<std::string> dns_over_tls_hostnames; + std::string dns_over_https_template; + std::string ui_name; + std::string privacy_policy; + bool display_globally; + std::set<std::string> display_countries; + + // Returns the full list of DoH providers. A subset of this list may be used + // to support upgrade in automatic mode or to populate the dropdown menu for + // secure mode. + static const List& GetList(); + + static DohProviderEntry ConstructForTesting( std::string provider, base::Optional<DohProviderIdForHistogram> provider_id_for_histogram, - std::set<std::string> ip_strs, + std::set<base::StringPiece> ip_strs, std::set<std::string> dns_over_tls_hostnames, std::string dns_over_https_template, std::string ui_name, std::string privacy_policy, bool display_globally, std::set<std::string> display_countries); - DohProviderEntry(const DohProviderEntry& other); + + // Entries are move-only. This allows tests to construct a List but ensures + // that |const DohProviderEntry*| is a safe type for application code. + DohProviderEntry(DohProviderEntry&& other); + DohProviderEntry& operator=(DohProviderEntry&& other); ~DohProviderEntry(); - const std::string provider; - // A provider_id_for_histogram is required for entries that are intended to - // be visible in the UI. - const base::Optional<DohProviderIdForHistogram> provider_id_for_histogram; - std::set<IPAddress> ip_addresses; - const std::set<std::string> dns_over_tls_hostnames; - const std::string dns_over_https_template; - const std::string ui_name; - const std::string privacy_policy; - bool display_globally; - std::set<std::string> display_countries; + private: + DohProviderEntry( + std::string provider, + base::Optional<DohProviderIdForHistogram> provider_id_for_histogram, + std::set<base::StringPiece> ip_strs, + std::set<std::string> dns_over_tls_hostnames, + std::string dns_over_https_template, + std::string ui_name, + std::string privacy_policy, + bool display_globally, + std::set<std::string> display_countries); }; -// Returns the full list of DoH providers. A subset of this list may be used -// to support upgrade in automatic mode or to populate the dropdown menu for -// secure mode. -NET_EXPORT const std::vector<DohProviderEntry>& GetDohProviderList(); - } // namespace net -#endif // NET_DNS_PUBLIC_DOH_PROVIDER_LIST_H_ +#endif // NET_DNS_PUBLIC_DOH_PROVIDER_ENTRY_H_ diff --git a/chromium/net/dns/public/doh_provider_list_unittest.cc b/chromium/net/dns/public/doh_provider_entry_unittest.cc index 60750e6d86a..e5cf5b79cf5 100644 --- a/chromium/net/dns/public/doh_provider_list_unittest.cc +++ b/chromium/net/dns/public/doh_provider_entry_unittest.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "net/dns/public/doh_provider_list.h" +#include "net/dns/public/doh_provider_entry.h" #include "testing/gtest/include/gtest/gtest.h" @@ -10,7 +10,7 @@ namespace net { namespace { TEST(DohProviderListTest, GetDohProviderList) { - const std::vector<DohProviderEntry>& list = GetDohProviderList(); + const DohProviderEntry::List& list = DohProviderEntry::GetList(); EXPECT_FALSE(list.empty()); } diff --git a/chromium/net/dns/record_parsed.cc b/chromium/net/dns/record_parsed.cc index 1ca18d8d22c..23c2f2eb4bc 100644 --- a/chromium/net/dns/record_parsed.cc +++ b/chromium/net/dns/record_parsed.cc @@ -62,9 +62,6 @@ std::unique_ptr<const RecordParsed> RecordParsed::CreateFrom( case OptRecordRdata::kType: rdata = OptRecordRdata::Create(record.rdata, *parser); break; - case EsniRecordRdata::kType: - rdata = EsniRecordRdata::Create(record.rdata, *parser); - break; case IntegrityRecordRdata::kType: rdata = IntegrityRecordRdata::Create(record.rdata); break; diff --git a/chromium/net/dns/record_rdata.cc b/chromium/net/dns/record_rdata.cc index 74200808b57..08d89d4fc15 100644 --- a/chromium/net/dns/record_rdata.cc +++ b/chromium/net/dns/record_rdata.cc @@ -18,88 +18,12 @@ namespace net { -namespace { - -// Helper function for parsing ESNI (TLS 1.3 Encrypted -// Server Name Indication, draft 4) RDATA. -// -// Precondition: |reader| points to the beginning of an -// incoming ESNI-type RecordRdata's data -// -// If the ESNIRecord contains a well-formed -// ESNIKeys field, advances |reader| immediately past the field -// and returns true. Otherwise, returns false. -WARN_UNUSED_RESULT bool AdvancePastEsniKeysField( - base::BigEndianReader* reader) { - DCHECK(reader); - - // Skip |esni_keys.version|. - if (!reader->Skip(2)) - return false; - - // Within esni_keys, skip |public_name|, - // |keys|, and |cipher_suites|. - base::StringPiece piece_for_skipping; - for (int i = 0; i < 3; ++i) { - if (!reader->ReadU16LengthPrefixed(&piece_for_skipping)) - return false; - } - - // Skip the |esni_keys.padded_length| field. - if (!reader->Skip(2)) - return false; - - // Skip the |esni_keys.extensions| field. - return reader->ReadU16LengthPrefixed(&piece_for_skipping); -} - -// Parses a single ESNI address set, appending the addresses to |out|. -WARN_UNUSED_RESULT bool ParseEsniAddressSet(base::StringPiece address_set, - std::vector<IPAddress>* out) { - DCHECK(out); - - IPAddressBytes address_bytes; - base::BigEndianReader address_set_reader(address_set.data(), - address_set.size()); - while (address_set_reader.remaining()) { - // enum AddressType, section 4.2.1 of ESNI draft 4 - // values: 4 (IPv4), 6 (IPv6) - uint8_t address_type = 0; - if (!address_set_reader.ReadU8(&address_type)) - return false; - - switch (address_type) { - case 4: { - address_bytes.Resize(IPAddress::kIPv4AddressSize); - break; - } - case 6: { - address_bytes.Resize(IPAddress::kIPv6AddressSize); - break; - } - default: // invalid address type - return false; - } - if (!address_set_reader.ReadBytes(address_bytes.data(), - address_bytes.size())) - return false; - out->emplace_back(address_bytes); - } - return true; -} - -} // namespace - static const size_t kSrvRecordMinimumSize = 6; -// Source: https://tools.ietf.org/html/draft-ietf-tls-esni-04, section 4.1 -// (This isn't necessarily a tight bound, but it doesn't need to be: -// |HasValidSize| is just used for sanity-check validation.) -// - ESNIKeys field: 8 bytes of length prefixes, 4 bytes of mandatory u16 -// fields, >=7 bytes of length-prefixed fields' contents -// - ESNIRecord field = ESNIKeys field + 2 bytes for |dns_extensions|'s -// length prefix -static const size_t kEsniDraft4MinimumSize = 21; +// The simplest INTEGRITY record is a U16-length-prefixed nonce (containing zero +// bytes) followed by its SHA256 digest. +static constexpr size_t kIntegrityMinimumSize = + sizeof(uint16_t) + IntegrityRecordRdata::kDigestLen; bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) { switch (type) { @@ -109,8 +33,8 @@ bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) { return data.size() == IPAddress::kIPv4AddressSize; case dns_protocol::kTypeAAAA: return data.size() == IPAddress::kIPv6AddressSize; - case dns_protocol::kExperimentalTypeEsniDraft4: - return data.size() >= kEsniDraft4MinimumSize; + case dns_protocol::kExperimentalTypeIntegrity: + return data.size() >= kIntegrityMinimumSize; case dns_protocol::kTypeCNAME: case dns_protocol::kTypePTR: case dns_protocol::kTypeTXT: @@ -449,72 +373,6 @@ bool OptRecordRdata::Opt::operator==(const OptRecordRdata::Opt& other) const { return code_ == other.code_ && data_ == other.data_; } -EsniRecordRdata::EsniRecordRdata() = default; - -EsniRecordRdata::~EsniRecordRdata() = default; - -// static -std::unique_ptr<EsniRecordRdata> EsniRecordRdata::Create( - base::StringPiece data, - const DnsRecordParser& parser) { - base::BigEndianReader reader(data.data(), data.size()); - - // TODO: Once BoringSSL CL 37704 lands, replace - // this with Boring's SSL_parse_esni_record, - // which does the same thing. - if (!AdvancePastEsniKeysField(&reader)) - return nullptr; - - size_t esni_keys_len = reader.ptr() - data.data(); - - base::StringPiece dns_extensions; - if (!reader.ReadU16LengthPrefixed(&dns_extensions) || reader.remaining() > 0) - return nullptr; - - // Check defensively that we're not about to read OOB. - CHECK_LT(esni_keys_len, data.size()); - auto rdata = base::WrapUnique(new EsniRecordRdata); - rdata->esni_keys_ = std::string(data.begin(), esni_keys_len); - - base::BigEndianReader dns_extensions_reader(dns_extensions.data(), - dns_extensions.size()); - if (dns_extensions_reader.remaining() == 0) - return rdata; - - // ESNI Draft 4 only permits one extension type, address_set, - // so reject if we see any other extension type. - uint16_t dns_extension_type = 0; - if (!dns_extensions_reader.ReadU16(&dns_extension_type) || - dns_extension_type != kAddressSetExtensionType) - return nullptr; - - base::StringPiece address_set; - if (!dns_extensions_reader.ReadU16LengthPrefixed(&address_set) || - !ParseEsniAddressSet(address_set, &rdata->addresses_)) - return nullptr; - - // In TLS, it's forbidden to send the same extension more than once in an - // extension block; assuming that the same restriction applies here, the - // record is ill-formed if any bytes follow the first (and only) extension. - if (dns_extensions_reader.remaining() > 0) - return nullptr; - - return rdata; -} - -uint16_t EsniRecordRdata::Type() const { - return EsniRecordRdata::kType; -} - -bool EsniRecordRdata::IsEqual(const RecordRdata* other) const { - if (other->Type() != Type()) - return false; - const EsniRecordRdata* esni_other = - static_cast<const EsniRecordRdata*>(other); - return esni_keys_ == esni_other->esni_keys_ && - addresses_ == esni_other->addresses_; -} - IntegrityRecordRdata::IntegrityRecordRdata(Nonce nonce) : nonce_(std::move(nonce)), digest_(Hash(nonce_)), is_intact_(true) {} diff --git a/chromium/net/dns/record_rdata.h b/chromium/net/dns/record_rdata.h index 42d7c1c6111..d652eb7e7ab 100644 --- a/chromium/net/dns/record_rdata.h +++ b/chromium/net/dns/record_rdata.h @@ -11,8 +11,8 @@ #include <string> #include <vector> +#include "base/check_op.h" #include "base/compiler_specific.h" -#include "base/logging.h" #include "base/macros.h" #include "base/optional.h" #include "base/strings/string_piece.h" @@ -269,49 +269,6 @@ class NET_EXPORT_PRIVATE OptRecordRdata : public RecordRdata { DISALLOW_COPY_AND_ASSIGN(OptRecordRdata); }; -// TLS 1.3 Encrypted Server Name Indication -// record format (https://tools.ietf.org/id/draft-ietf-tls-esni-04.txt) -// struct { -// ESNIKeys esni_keys; // see spec -// Extension dns_extensions<0..2 ^ 16 - 1>; -// } ESNIRecord; -class NET_EXPORT EsniRecordRdata : public RecordRdata { - public: - static constexpr uint16_t kType = dns_protocol::kExperimentalTypeEsniDraft4; - static constexpr uint16_t kAddressSetExtensionType = 0x1001u; - - ~EsniRecordRdata() override; - - // Parsing an ESNIRecord Rdata succeeds when all of the following hold: - // 1. The esni_keys field is well-formed. - // 2. The dns_extensions field is well-formed and, additionally, valid - // in the sense that its enum members have values allowed by the spec. - // 3. The Rdata field contains no data beyond the ESNIKeys and, optionally, - // one DNS extension of type address_set. - static std::unique_ptr<EsniRecordRdata> Create(base::StringPiece data, - const DnsRecordParser& parser); - - // Two EsniRecordRdatas compare equal if their ESNIKeys fields agree - // and their address sets contain the same addresses in the same order. - bool IsEqual(const RecordRdata* other) const override; - uint16_t Type() const override; - - // Returns the ESNIKeys field of the record. This is an opaque bitstring - // passed to the SSL library. - base::StringPiece esni_keys() const { return esni_keys_; } - - // Returns the IP addresses parsed from the address_set DNS extension, if any. - const std::vector<IPAddress>& addresses() const { return addresses_; } - - private: - EsniRecordRdata(); - - std::string esni_keys_; - std::vector<IPAddress> addresses_; - - DISALLOW_COPY_AND_ASSIGN(EsniRecordRdata); -}; - // This class parses and serializes the INTEGRITY DNS record. // // This RR was invented for a preliminary HTTPSSVC experiment. See the public diff --git a/chromium/net/dns/record_rdata_unittest.cc b/chromium/net/dns/record_rdata_unittest.cc index c70e674a2d9..88f4c2bd96f 100644 --- a/chromium/net/dns/record_rdata_unittest.cc +++ b/chromium/net/dns/record_rdata_unittest.cc @@ -136,472 +136,6 @@ TEST(RecordRdataTest, ParseCnameRecord) { ASSERT_TRUE(record_obj->IsEqual(record_obj.get())); } -// Appends a well-formed ESNIKeys struct to the stream owned by "writer". -// Returns the length, in bytes, of this struct, or 0 on error. -// -// (There is no ambiguity in the return value because a well-formed -// ESNIKeys struct has positive length.) -void AppendWellFormedEsniKeys(base::BigEndianWriter* writer) { - CHECK(writer); - writer->WriteBytes(kWellFormedEsniKeys, kWellFormedEsniKeysSize); -} - -// This helper checks |keys| against the well-formed sample ESNIKeys -// struct; it's necessary because we can't implicitly convert -// kWellFormedEsniKeys to a StringPiece (it's a byte array, not a -// null-terminated string). -void ExpectMatchesSampleKeys(base::StringPiece keys) { - EXPECT_EQ(keys, - base::StringPiece(kWellFormedEsniKeys, kWellFormedEsniKeysSize)); -} - -// Appends an IP address in network byte order, prepended by one byte -// containing its version number, to |*serialized_addresses|. Appends -// the corresponding IPAddress object to |*address_objects|. -void AppendAnotherIPAddress(std::vector<uint8_t>* serialized_addresses, - std::vector<IPAddress>* address_objects, - int ip_version) { - CHECK(serialized_addresses); - CHECK(address_objects); - - // To make the addresses vary, but in a deterministic manner, assign octets - // in increasing order as they're requested, potentially eventually wrapping - // to 0. - static uint8_t next_octet; - - CHECK(ip_version == 4 || ip_version == 6); - const int address_num_bytes = ip_version == 4 ? 4 : 16; - - std::vector<uint8_t> address_bytes; - for (int i = 0; i < address_num_bytes; ++i) - address_bytes.push_back(next_octet++); - IPAddress address(address_bytes.data(), address_num_bytes); - - serialized_addresses->push_back(ip_version); - serialized_addresses->insert(serialized_addresses->end(), - address_bytes.begin(), address_bytes.end()); - address_objects->push_back(address); -} - -// Writes a dns_extensions ESNIRecord block containing given the address -// set to |writer|. This involves: -// - writing the 16-bit length prefix for the dns_extensions block -// - writing the 16-bit extension type (0x1001 "address_set") -// - writing the 16-bit length prefix for the address set extension -// - writing the extension itself -void AppendDnsExtensionsBlock(base::BigEndianWriter* writer, - const std::vector<uint8_t>& address_list) { - CHECK(writer); - // 2 bytes for the DNS extension type - writer->WriteU16(4 + - address_list.size()); // length of the dns_extensions field - writer->WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer->WriteU16(address_list.size()); // length of the address set - writer->WriteBytes(address_list.data(), address_list.size()); -} - -// Test parsing a well-formed ESNI record with no DNS extensions. -TEST(RecordRdataTest, ParseEsniRecordNoExtensions) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(0); // dns_extensions length - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - ASSERT_THAT(record_obj, NotNull()); - EXPECT_TRUE(record_obj->IsEqual(record_obj.get())); - EXPECT_EQ(record_obj->esni_keys(), - std::string(kWellFormedEsniKeys, kWellFormedEsniKeysSize)); - EXPECT_EQ(record_obj->Type(), dns_protocol::kExperimentalTypeEsniDraft4); -} - -// Test parsing a well-formed ESNI record bearing an address_set extension -// containing a single IPv4 address. -TEST(RecordRdataTest, ParseEsniRecordOneIPv4Address) { - // ESNI record: - // well-formed ESNI keys - // extensions length - // extension - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - std::vector<uint8_t> address_list; - std::vector<IPAddress> addresses_for_validation; - - AppendAnotherIPAddress(&address_list, &addresses_for_validation, - 4 /* ip_version */); - - AppendDnsExtensionsBlock(&writer, address_list); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - ASSERT_THAT(record_obj, NotNull()); - - const auto& addresses = record_obj->addresses(); - - EXPECT_EQ(addresses, addresses_for_validation); - - EXPECT_TRUE(record_obj->IsEqual(record_obj.get())); - ExpectMatchesSampleKeys(record_obj->esni_keys()); -} - -// Test parsing a well-formed ESNI record bearing an address_set extension -// containing a single IPv6 address. -TEST(RecordRdataTest, ParseEsniRecordOneIPv6Address) { - // ESNI record: - // well-formed ESNI keys - // extensions length - // extension - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - std::vector<uint8_t> address_list; - std::vector<IPAddress> addresses_for_validation; - - AppendAnotherIPAddress(&address_list, &addresses_for_validation, - 6 /* ip_version */); - - AppendDnsExtensionsBlock(&writer, address_list); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - ASSERT_THAT(record_obj, NotNull()); - - const auto& addresses = record_obj->addresses(); - - EXPECT_EQ(addresses, addresses_for_validation); - - EXPECT_TRUE(record_obj->IsEqual(record_obj.get())); - ExpectMatchesSampleKeys(record_obj->esni_keys()); -} - -// Test parsing a well-formed ESNI record bearing an address_set extension -// containing several IPv4 and IPv6 addresses. -TEST(RecordRdataTest, ParseEsniRecordManyAddresses) { - // ESNI record: - // well-formed ESNI keys - // extensions length - // extension - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - std::vector<uint8_t> address_list; - std::vector<IPAddress> addresses_for_validation; - - for (int i = 0; i < 100; ++i) - AppendAnotherIPAddress(&address_list, &addresses_for_validation, - (i % 3) ? 4 : 6 /* ip_version */); - - AppendDnsExtensionsBlock(&writer, address_list); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - ASSERT_THAT(record_obj, NotNull()); - - const auto& addresses = record_obj->addresses(); - - EXPECT_EQ(addresses, addresses_for_validation); - - EXPECT_TRUE(record_obj->IsEqual(record_obj.get())); - ExpectMatchesSampleKeys(record_obj->esni_keys()); -} - -// Test that we correctly reject a record with an ill-formed ESNI keys field. -// -// This test makes sure that the //net-side record parser is able -// correctly to handle the case where an external ESNI keys validation -// subroutine reports that the keys are ill-formed; because this validation -// will eventually be performed by BoringSSL once the corresponding -// BSSL code lands, it's out of scope here to exercise the -// validation logic itself. -TEST(RecordRdataTest, EsniMalformedRecord_InvalidEsniKeys) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - - // Oops! This otherwise well-formed ESNIKeys struct is missing its - // final byte, and the reader contains no content after this incomplete - // struct. - const char ill_formed_esni_keys[] = { - 0xff, 0x3, 0x0, 0x0, 0x0, 0x24, 0x0, 0x1d, 0x0, 0x20, - 0xed, 0xed, 0xc8, 0x68, 0xc1, 0x71, 0xd6, 0x9e, 0xa9, 0xf0, - 0xa2, 0xc9, 0xf5, 0xa9, 0xdc, 0xcf, 0xf9, 0xb8, 0xed, 0x15, - 0x5c, 0xc4, 0x5a, 0xec, 0x6f, 0xb2, 0x86, 0x14, 0xb7, 0x71, - 0x1b, 0x7c, 0x0, 0x2, 0x13, 0x1, 0x1, 0x4, 0x0}; - writer.WriteBytes(ill_formed_esni_keys, sizeof(ill_formed_esni_keys)); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an empty address_set extension is correctly accepted. -TEST(RecordRdataTest, ParseEsniRecord_EmptyAddressSet) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(4); // length of the dns_extensions field - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(0); // length of the (empty) address_set extension - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, NotNull()); -} - -// Test that we correctly reject a record invalid due to having extra -// data within its dns_extensions block but after its last extension. -TEST(RecordRdataTest, EsniMalformedRecord_TrailingDataWithinDnsExtensions) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(5); // length of the dns_extensions field - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(0); // length of the (empty) address_set extension - - // Pad the otherwise-valid extensions block with one byte of garbage. - writer.WriteBytes(&"a", 1); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that we correctly reject a record with two well-formed -// DNS extensions (only one extension of each type is permitted). -TEST(RecordRdataTest, EsniMalformedRecord_TooManyExtensions) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(8); // length of the dns_extensions field - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(0); // length of the (empty) address_set extension - // Write another (empty, but completely valid on its own) extension, - // rendering the record invalid. - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(0); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNIRecord with an extension of invalid type -// is correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_InvalidExtensionType) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - // 2 bytes for the DNS extension type - writer.WriteU16(2); // length of the dns_extensions field - writer.WriteU16(0xdead); // invalid address type - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an address_set extension missing a length field -// is correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_MalformedAddressSetLength) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - // 3 bytes: 2 for the DNS extension type, and one for our - // too-short address_set length - writer.WriteU16(3); // length of the dns_extensions field - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - // oops! need two bytes for the address length - writer.WriteU8(57); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with malformed dns_extensions length is -// correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_MalformedDnsExtensionsLength) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - // Oops! Length field of dns_extensions should be 2 bytes. - writer.WriteU8(57); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with invalid dns_extensions length is -// correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_BadDnsExtensionsLength) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - // Length-prepend the dns_extensions field with value 5, even though - // the extensions object will have length 4 (two U16's): this should - // make the record be rejected as malformed. - writer.WriteU16(5); - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(0); // length of the address_set extension - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with invalid address_set extension length is -// correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_BadAddressSetLength) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(4); // 2 bytes for each of the U16s to be written - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - // Oops! Length-prepending the empty address_set field with the value 1. - writer.WriteU16(1); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with an address_set entry of bad address -// type is correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_InvalidAddressType) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - writer.WriteU16(9); // dns_extensions length: two U16's and a 5-byte address - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - - std::vector<uint8_t> address_list; - IPAddress ipv4; - ASSERT_TRUE(net::ParseURLHostnameToAddress("192.168.1.1", &ipv4)); - address_list.push_back(5); // Oops! "5" isn't a valid AddressType. - std::copy(ipv4.bytes().begin(), ipv4.bytes().end(), - std::back_inserter(address_list)); - - writer.WriteU16(address_list.size()); - writer.WriteBytes(address_list.data(), address_list.size()); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with an address_set entry of bad address -// type is correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_NotEnoughAddressData_IPv4) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - std::vector<uint8_t> address_list; - std::vector<IPAddress> addresses_for_validation_unused; - AppendAnotherIPAddress(&address_list, &addresses_for_validation_unused, 4); - - // dns_extensions length: 2 bytes for address type, 2 for address_set length - // Subtract 1 because we're deliberately writing one byte too few for the - // purposes of this test. - writer.WriteU16(address_list.size() - 1 + 4); - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(address_list.size() - 1); - // oops! missing the last byte of our IPv4 address - writer.WriteBytes(address_list.data(), address_list.size() - 1); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - -// Test that an ESNI record with an address_set entry of bad address -// type is correctly rejected. -TEST(RecordRdataTest, EsniMalformedRecord_NotEnoughAddressData_IPv6) { - char record[10000] = {}; - base::BigEndianWriter writer(record, sizeof(record)); - AppendWellFormedEsniKeys(&writer); - - std::vector<uint8_t> address_list; - std::vector<IPAddress> addresses_for_validation_unused; - AppendAnotherIPAddress(&address_list, &addresses_for_validation_unused, 6); - - // dns_extensions length: 2 bytes for address type, 2 for address_set length - // Subtract 1 because we're deliberately writing one byte too few for the - // purposes of this test. - writer.WriteU16(address_list.size() - 1 + 4); - writer.WriteU16(EsniRecordRdata::kAddressSetExtensionType); - writer.WriteU16(address_list.size() - 1); - // oops! missing the last byte of our IPv6 address - writer.WriteBytes(address_list.data(), address_list.size() - 1); - - auto record_size = writer.ptr() - record; - DnsRecordParser parser(record, record_size, 0 /* offset */); - std::unique_ptr<EsniRecordRdata> record_obj = - EsniRecordRdata::Create(std::string(record, record_size), parser); - - ASSERT_THAT(record_obj, IsNull()); -} - TEST(RecordRdataTest, ParsePtrRecord) { // These are just the rdata portions of the DNS records, rather than complete // records, but it works well enough for this test. diff --git a/chromium/net/dns/resolve_context.cc b/chromium/net/dns/resolve_context.cc index fd7f7c6f17f..3b15cf320ec 100644 --- a/chromium/net/dns/resolve_context.cc +++ b/chromium/net/dns/resolve_context.cc @@ -7,7 +7,6 @@ #include <algorithm> #include <cstdlib> #include <limits> -#include <string> #include <utility> #include "base/check_op.h" @@ -15,6 +14,7 @@ #include "base/metrics/histogram.h" #include "base/metrics/histogram_base.h" #include "base/metrics/histogram_functions.h" +#include "base/metrics/histogram_macros.h" #include "base/metrics/sample_vector.h" #include "base/no_destructor.h" #include "base/numerics/safe_conversions.h" @@ -150,10 +150,27 @@ size_t ResolveContext::NumAvailableDohServers(const DnsSession* session) const { void ResolveContext::RecordServerFailure(size_t server_index, bool is_doh_server, + int rv, const DnsSession* session) { + DCHECK(rv != OK && rv != ERR_NAME_NOT_RESOLVED && rv != ERR_IO_PENDING); + if (!IsCurrentSession(session)) return; + // "FailureError" metric is only recorded for secure queries. + if (is_doh_server) { + std::string query_type = + GetQueryTypeForUma(server_index, true /* is_doh_server */, session); + DCHECK_NE(query_type, "Insecure"); + std::string provider_id = + GetDohProviderIdForUma(server_index, true /* is_doh_server */, session); + + base::UmaHistogramSparse( + base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureError", + query_type.c_str(), provider_id.c_str()), + std::abs(rv)); + } + size_t num_available_doh_servers_before = NumAvailableDohServers(session); ServerStats* stats = GetServerStats(server_index, is_doh_server); @@ -200,10 +217,11 @@ void ResolveContext::RecordRtt(size_t server_index, if (!IsCurrentSession(session)) return; - RecordRttForUma(server_index, is_doh_server, rtt, rv, session); - ServerStats* stats = GetServerStats(server_index, is_doh_server); + base::TimeDelta base_timeout = NextTimeoutHelper(stats, 0 /* num_backoffs */); + RecordRttForUma(server_index, is_doh_server, rtt, rv, base_timeout, session); + // RTT values shouldn't be less than 0, but it shouldn't cause a crash if // they are anyway, so clip to 0. See https://crbug.com/753568. if (rtt < base::TimeDelta()) @@ -367,43 +385,71 @@ void ResolveContext::RecordRttForUma(size_t server_index, bool is_doh_server, base::TimeDelta rtt, int rv, + base::TimeDelta base_timeout, const DnsSession* session) { DCHECK(IsCurrentSession(session)); - std::string query_type; - std::string provider_id; - if (is_doh_server) { - // Secure queries are validated if the DoH server state is available. - if (GetDohServerAvailability(server_index, session)) - query_type = "SecureValidated"; - else - query_type = "SecureNotValidated"; - provider_id = GetDohProviderIdForHistogramFromDohConfig( - current_session_->config().dns_over_https_servers[server_index]); - } else { - query_type = "Insecure"; - provider_id = GetDohProviderIdForHistogramFromNameserver( - current_session_->config().nameservers[server_index]); - } + std::string query_type = + GetQueryTypeForUma(server_index, is_doh_server, session); + std::string provider_id = + GetDohProviderIdForUma(server_index, is_doh_server, session); + if (rv == OK || rv == ERR_NAME_NOT_RESOLVED) { base::UmaHistogramMediumTimes( base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.SuccessTime", query_type.c_str(), provider_id.c_str()), rtt); + if (query_type == "SecureValidated") { + DCHECK(is_doh_server); + + // Only for SecureValidated requests, record the ratio between successful + // RTT and the base timeout for the server. Note that RTT could be much + // longer than the timeout as previous attempts are often allowed to + // continue in parallel with new attempts made by the transaction. Scale + // the ratio up by 10 for sub-integer granularity. + // TODO(crbug.com/1105138): Remove after determining good timeout logic. + int timeout_ratio = 10 * rtt / base_timeout; + UMA_HISTOGRAM_COUNTS_1000( + "Net.DNS.DnsTransaction.SecureValidated.SuccessTimeoutRatio", + timeout_ratio); + } } else { base::UmaHistogramMediumTimes( base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureTime", query_type.c_str(), provider_id.c_str()), rtt); - if (is_doh_server) { - base::UmaHistogramSparse( - base::StringPrintf("Net.DNS.DnsTransaction.%s.%s.FailureError", - query_type.c_str(), provider_id.c_str()), - std::abs(rv)); - } } } +std::string ResolveContext::GetQueryTypeForUma(size_t server_index, + bool is_doh_server, + const DnsSession* session) { + DCHECK(IsCurrentSession(session)); + + if (!is_doh_server) + return "Insecure"; + + // Secure queries are validated if the DoH server state is available. + if (GetDohServerAvailability(server_index, session)) + return "SecureValidated"; + + return "SecureNotValidated"; +} + +std::string ResolveContext::GetDohProviderIdForUma(size_t server_index, + bool is_doh_server, + const DnsSession* session) { + DCHECK(IsCurrentSession(session)); + + if (is_doh_server) { + return GetDohProviderIdForHistogramFromDohConfig( + session->config().dns_over_https_servers[server_index]); + } + + return GetDohProviderIdForHistogramFromNameserver( + session->config().nameservers[server_index]); +} + void ResolveContext::NotifyDohStatusObserversOfSessionChanged() { for (auto& observer : doh_status_observers_) observer.OnSessionChanged(); diff --git a/chromium/net/dns/resolve_context.h b/chromium/net/dns/resolve_context.h index 8b3ec4fd388..bcbac646da5 100644 --- a/chromium/net/dns/resolve_context.h +++ b/chromium/net/dns/resolve_context.h @@ -6,6 +6,7 @@ #define NET_DNS_RESOLVE_CONTEXT_H_ #include <memory> +#include <string> #include <vector> #include "base/memory/weak_ptr.h" @@ -92,9 +93,12 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver { // Record that server failed to respond (due to SRV_FAIL or timeout). If // |is_doh_server| and the number of failures has surpassed a threshold, // sets the DoH probe state to unavailable. Noop if |session| is not the - // current session. + // current session. Should only be called with with server failure |rv|s, + // not eg OK, ERR_NAME_NOT_RESOLVED (which at the transaction level is + // expected to be nxdomain), or ERR_IO_PENDING. void RecordServerFailure(size_t server_index, bool is_doh_server, + int rv, const DnsSession* session); // Record that server responded successfully. Noop if |session| is not the @@ -199,7 +203,14 @@ class NET_EXPORT_PRIVATE ResolveContext : public base::CheckedObserver { bool is_doh_server, base::TimeDelta rtt, int rv, + base::TimeDelta base_timeout, const DnsSession* session); + std::string GetQueryTypeForUma(size_t server_index, + bool is_doh_server, + const DnsSession* session); + std::string GetDohProviderIdForUma(size_t server_index, + bool is_doh_server, + const DnsSession* session); void NotifyDohStatusObserversOfSessionChanged(); void NotifyDohStatusObserversOfUnavailable(bool network_change); diff --git a/chromium/net/dns/resolve_context_unittest.cc b/chromium/net/dns/resolve_context_unittest.cc index 4701feff6db..5de6a10e3f3 100644 --- a/chromium/net/dns/resolve_context_unittest.cc +++ b/chromium/net/dns/resolve_context_unittest.cc @@ -197,7 +197,7 @@ TEST_F(ResolveContextTest, DohServerAvailability_DifferentSession) { ASSERT_TRUE(context.GetDohServerAvailability(1u, session2.get())); for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) { context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session1.get()); + ERR_FAILED, session1.get()); } EXPECT_TRUE(context.GetDohServerAvailability(1u, session2.get())); } @@ -300,7 +300,7 @@ TEST_F(ResolveContextTest, DohServerAvailabilityNotification) { for (int i = 0; i < ResolveContext::kAutomaticModeFailureLimit; ++i) { ASSERT_EQ(2u, context.NumAvailableDohServers(session.get())); context.RecordServerFailure(0u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); base::RunLoop().RunUntilIdle(); // Notifications are async. EXPECT_EQ(1, config_observer.dns_changed_calls()); } @@ -313,7 +313,7 @@ TEST_F(ResolveContextTest, DohServerAvailabilityNotification) { EXPECT_EQ(1, config_observer.dns_changed_calls()); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } ASSERT_EQ(0u, context.NumAvailableDohServers(session.get())); base::RunLoop().RunUntilIdle(); // Notifications are async. @@ -418,7 +418,8 @@ TEST_F(ResolveContextTest, Failures_Consecutive) { EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u); context.RecordServerFailure(1u /* server_index */, - false /* is_doh_server */, session.get()); + false /* is_doh_server */, ERR_FAILED, + session.get()); } { @@ -464,7 +465,8 @@ TEST_F(ResolveContextTest, Failures_NonConsecutive) { EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u); context.RecordServerFailure(1u /* server_index */, - false /* is_doh_server */, session.get()); + false /* is_doh_server */, ERR_FAILED, + session.get()); } { @@ -491,7 +493,7 @@ TEST_F(ResolveContextTest, Failures_NonConsecutive) { // Expect server stay preferred through non-consecutive failures. context.RecordServerFailure(1u /* server_index */, false /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); { std::unique_ptr<DnsServerIterator> classic_itr = context.GetClassicDnsIterator(session->config(), session.get()); @@ -518,7 +520,8 @@ TEST_F(ResolveContextTest, Failures_NoSession) { EXPECT_FALSE(classic_itr->AttemptAvailable()); context.RecordServerFailure(1u /* server_index */, - false /* is_doh_server */, session.get()); + false /* is_doh_server */, ERR_FAILED, + session.get()); } std::unique_ptr<DnsServerIterator> classic_itr = context.GetClassicDnsIterator(session->config(), session.get()); @@ -551,7 +554,8 @@ TEST_F(ResolveContextTest, Failures_DifferentSession) { EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 1u); context.RecordServerFailure(1u /* server_index */, - false /* is_doh_server */, session1.get()); + false /* is_doh_server */, ERR_FAILED, + session1.get()); } std::unique_ptr<DnsServerIterator> classic_itr = context.GetClassicDnsIterator(session2->config(), session2.get()); @@ -586,9 +590,11 @@ TEST_F(ResolveContextTest, TwoFailures) { EXPECT_EQ(classic_itr->GetNextAttemptIndex(), 2u); context.RecordServerFailure(0u /* server_index */, - false /* is_doh_server */, session.get()); + false /* is_doh_server */, ERR_FAILED, + session.get()); context.RecordServerFailure(1u /* server_index */, - false /* is_doh_server */, session.get()); + false /* is_doh_server */, ERR_FAILED, + session.get()); } { std::unique_ptr<DnsServerIterator> classic_itr = @@ -661,7 +667,7 @@ TEST_F(ResolveContextTest, DohFailures_Consecutive) { EXPECT_EQ(1u, context.NumAvailableDohServers(session.get())); EXPECT_EQ(0, observer.server_unavailable_notifications()); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator( session->config(), DnsConfig::SecureDnsMode::AUTOMATIC, session.get()); @@ -696,7 +702,7 @@ TEST_F(ResolveContextTest, DohFailures_NonConsecutive) { EXPECT_EQ(doh_itr->GetNextAttemptIndex(), 1u); EXPECT_EQ(1u, context.NumAvailableDohServers(session.get())); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } { std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator( @@ -721,7 +727,7 @@ TEST_F(ResolveContextTest, DohFailures_NonConsecutive) { // Expect a single additional failure should not make a DoH server unavailable // because the success resets failure tracking. context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); { std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator( session->config(), DnsConfig::SecureDnsMode::AUTOMATIC, session.get()); @@ -752,7 +758,7 @@ TEST_F(ResolveContextTest, DohFailures_SuccessAfterFailures) { for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) { context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } ASSERT_EQ(0u, context.NumAvailableDohServers(session.get())); EXPECT_EQ(1, observer.server_unavailable_notifications()); @@ -787,7 +793,7 @@ TEST_F(ResolveContextTest, DohFailures_NoSession) { for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) { EXPECT_EQ(0u, context.NumAvailableDohServers(session.get())); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } EXPECT_EQ(0u, context.NumAvailableDohServers(session.get())); } @@ -814,7 +820,7 @@ TEST_F(ResolveContextTest, DohFailures_DifferentSession) { for (size_t i = 0; i < ResolveContext::kAutomaticModeFailureLimit; i++) { EXPECT_EQ(1u, context.NumAvailableDohServers(session2.get())); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session1.get()); + ERR_FAILED, session1.get()); } EXPECT_EQ(1u, context.NumAvailableDohServers(session2.get())); } @@ -849,9 +855,9 @@ TEST_F(ResolveContextTest, TwoDohFailures) { EXPECT_EQ(doh_itr->GetNextAttemptIndex(), 2u); context.RecordServerFailure(0u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); context.RecordServerFailure(1u /* server_index */, true /* is_doh_server */, - session.get()); + ERR_FAILED, session.get()); } std::unique_ptr<DnsServerIterator> doh_itr = context.GetDohIterator( diff --git a/chromium/net/dns/test_dns_config_service.h b/chromium/net/dns/test_dns_config_service.h index 3b61c361c6c..1b63ec766f6 100644 --- a/chromium/net/dns/test_dns_config_service.h +++ b/chromium/net/dns/test_dns_config_service.h @@ -7,7 +7,7 @@ #include <utility> -#include "base/logging.h" +#include "base/check.h" #include "base/optional.h" #include "net/dns/dns_config_service.h" |