Add experimental HTTPS query support to DNS

Parsing and metrics left as TODOs for now.

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

net/dns/dns_test_util.cc

@@ -80,7 +80,9 @@ DnsResourceRecord BuildTestDnsRecord(std::string name,
   record.type = type;
   record.klass = dns_protocol::kClassIN;
   record.ttl = ttl.InSeconds();
-  record.SetOwnedRdata(std::move(rdata));
+
+  if (!rdata.empty())
+    record.SetOwnedRdata(std::move(rdata));
 
   return record;
 }

net/dns/dns_util.cc

@@ -291,6 +291,8 @@ uint16_t DnsQueryTypeToQtype(DnsQueryType dns_query_type) {
       return dns_protocol::kTypeSRV;
     case DnsQueryType::INTEGRITY:
       return dns_protocol::kExperimentalTypeIntegrity;
+    case DnsQueryType::HTTPS:
+      return dns_protocol::kTypeHttps;
   }
 }
 

net/dns/host_resolver.h

@@ -100,6 +100,8 @@ class NET_EXPORT HostResolver {
 
     // INTEGRITY results for an initial experiment related to HTTPSSVC. Each
     // boolean value indicates the intactness of an INTEGRITY record.
+    // TODO(crbug.com/1138620): Generalize to be used for other experimental
+    // records.
     NET_EXPORT virtual const base::Optional<std::vector<bool>>&
     GetIntegrityResultsForTesting() const;
 

net/dns/host_resolver_manager.cc

@@ -1124,15 +1124,15 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
       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);
+
+        if (features::kDnsHttpssvcUseIntegrity.Get())
+          transactions_needed_.push(DnsQueryType::INTEGRITY);
+        if (features::kDnsHttpssvcUseHttpssvc.Get())
+          transactions_needed_.push(DnsQueryType::HTTPS);
       }
     }
     num_needed_transactions_ = transactions_needed_.size();
@@ -1194,25 +1194,31 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     return trans;
   }
 
-  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));
+  void OnExperimentalQueryTimeout(base::Optional<std::string> doh_provider_id) {
+    for (std::unique_ptr<DnsTransaction>& transaction : transactions_started_) {
+      DCHECK(httpssvc_metrics_);
+      base::TimeDelta elapsed_time = tick_clock_->NowTicks() - task_start_time_;
+
+      switch (transaction->GetType()) {
+        case dns_protocol::kExperimentalTypeIntegrity:
+          httpssvc_metrics_->SaveForIntegrity(
+              doh_provider_id, HttpssvcDnsRcode::kTimedOut, {}, elapsed_time);
+          break;
+        case dns_protocol::kTypeHttps:
+          httpssvc_metrics_->SaveForHttps(
+              doh_provider_id, HttpssvcDnsRcode::kTimedOut, elapsed_time);
+          break;
+        default:
+          // The experimental query timer is only started when all other
+          // transactions have completed.
+          NOTREACHED();
+      }
+    }
 
     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();
   }
 
@@ -1254,8 +1260,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
 
     if (net_error != OK && !(net_error == ERR_NAME_NOT_RESOLVED && response &&
                              response->IsValid())) {
-      if (dns_query_type == DnsQueryType::INTEGRITY) {
-        // Do not allow an INTEGRITY query to fail the whole DnsTask.
+      if (dns_query_type == DnsQueryType::INTEGRITY ||
+          dns_query_type == DnsQueryType::HTTPS) {
+        // Do not allow an experimental query to fail the whole DnsTask.
         response = nullptr;
       } else {
         OnFailure(net_error, DnsResponse::DNS_PARSE_OK, base::nullopt);
@@ -1287,6 +1294,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
         // Parse the INTEGRITY records, condensing them into a vector<bool>.
         parse_result = ParseIntegrityDnsResponse(response, &results);
         break;
+      case DnsQueryType::HTTPS:
+        parse_result = ParseHttpsDnsResponse(response, &results);
+        break;
     }
     DCHECK_LT(parse_result, DnsResponse::DNS_PARSE_RESULT_MAX);
 
@@ -1296,10 +1306,7 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     }
 
     if (httpssvc_metrics_) {
-      if (dns_query_type != DnsQueryType::INTEGRITY) {
-        httpssvc_metrics_->SaveForNonIntegrity(doh_provider_id, elapsed_time,
-                                               rcode_for_httpssvc);
-      } else {
+      if (dns_query_type == DnsQueryType::INTEGRITY) {
         const base::Optional<std::vector<bool>>& condensed =
             results.integrity_data();
         CHECK(condensed.has_value());
@@ -1307,6 +1314,12 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
         // experimental query timer runs out (OnExperimentalQueryTimeout).
         httpssvc_metrics_->SaveForIntegrity(doh_provider_id, rcode_for_httpssvc,
                                             *condensed, elapsed_time);
+      } else if (dns_query_type == DnsQueryType::HTTPS) {
+        httpssvc_metrics_->SaveForHttps(doh_provider_id, rcode_for_httpssvc,
+                                        elapsed_time);
+      } else {
+        httpssvc_metrics_->SaveForNonIntegrity(doh_provider_id, elapsed_time,
+                                               rcode_for_httpssvc);
       }
     }
 
@@ -1332,6 +1345,9 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
           results = HostCache::Entry::MergeEntries(
               std::move(results), std::move(saved_results_).value());
           break;
+        case DnsQueryType::HTTPS:
+          results = std::move(saved_results_).value();
+          break;
         default:
           // Only expect address query types with multiple transactions.
           NOTREACHED();
@@ -1534,6 +1550,14 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     return parse_result;
   }
 
+  DnsResponse::Result ParseHttpsDnsResponse(const DnsResponse* response,
+                                            HostCache::Entry* out_results) {
+    // TODO(crbug.com/1138620): Add actual parse implementation.
+    *out_results = HostCache::Entry(
+        ERR_NAME_NOT_RESOLVED, HostCache::Entry::SOURCE_DNS, base::nullopt);
+    return DnsResponse::Result::DNS_PARSE_OK;
+  }
+
   // Sort service targets per RFC2782.  In summary, sort first by |priority|,
   // lowest first.  For targets with the same priority, secondary sort randomly
   // using |weight| with higher weighted objects more likely to go first.
@@ -1705,7 +1729,8 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
   // |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 {
+  bool TaskIsCompleteOrOnlyQtypeTransactionsRemain(
+      std::initializer_list<uint16_t> qtypes) 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
@@ -1714,13 +1739,14 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
 
     return std::all_of(
         transactions_started_.begin(), transactions_started_.end(),
-        [&](const std::unique_ptr<DnsTransaction>& p) {
-          DCHECK(p);
-          return p->GetType() == qtype;
+        [&](const std::unique_ptr<DnsTransaction>& transaction) {
+          DCHECK(transaction);
+          return std::any_of(qtypes.begin(), qtypes.end(), [&](uint16_t qtype) {
+            return transaction->GetType() == qtype;
+          });
         });
   }
 
-
   void MaybeStartExperimentalQueryTimer(
       base::Optional<std::string> doh_provider_id) {
     DCHECK(!transactions_started_.empty());
@@ -1734,7 +1760,8 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
 
     if (!experimental_query_cancellation_timer_.IsRunning() &&
         TaskIsCompleteOrOnlyQtypeTransactionsRemain(
-            dns_protocol::kExperimentalTypeIntegrity)) {
+            {dns_protocol::kExperimentalTypeIntegrity,
+             dns_protocol::kTypeHttps})) {
       const base::TimeDelta kExtraTimeAbsolute =
           features::dns_httpssvc_experiment::GetExtraTimeAbsolute();
       const int kExtraTimePercent =
@@ -1744,14 +1771,16 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
           tick_clock_->NowTicks() - task_start_time_;
       base::TimeDelta relative_timeout =
           total_time_for_other_transactions * kExtraTimePercent / 100;
+      // Use at least 1ms to ensure timeout doesn't occur immediately in tests.
+      relative_timeout =
+          std::max(relative_timeout, base::TimeDelta::FromMilliseconds(1));
 
       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));
+          base::BindOnce(&DnsTask::OnExperimentalQueryTimeout,
+                         base::Unretained(this), doh_provider_id));
     }
   }
 

net/dns/host_resolver_mdns_listener_impl.cc

@@ -74,6 +74,7 @@ void HostResolverMdnsListenerImpl::OnRecordUpdate(
   switch (query_type_) {
     case DnsQueryType::UNSPECIFIED:
     case DnsQueryType::INTEGRITY:
+    case DnsQueryType::HTTPS:
       NOTREACHED();
       break;
     case DnsQueryType::A:

net/dns/host_resolver_mdns_task.cc

@@ -198,6 +198,10 @@ HostCache::Entry HostResolverMdnsTask::ParseResult(
   switch (query_type) {
     case DnsQueryType::UNSPECIFIED:
       // Should create two separate transactions with specified type.
+    case DnsQueryType::HTTPS:
+      // Not supported.
+      // TODO(ericorth@chromium.org): Consider support for HTTPS in mDNS if it
+      // is ever decided to support HTTPS via non-DoH.
     case DnsQueryType::INTEGRITY:
       // INTEGRITY queries are not expected to be useful in mDNS, so they're not
       // supported.

net/dns/httpssvc_metrics.cc

@@ -116,6 +116,12 @@ void HttpssvcMetrics::SaveForIntegrity(
   integrity_resolve_time_ = integrity_resolve_time;
 }
 
+void HttpssvcMetrics::SaveForHttps(base::Optional<std::string> doh_provider_id,
+                                   enum HttpssvcDnsRcode rcode,
+                                   base::TimeDelta https_resolve_time) {
+  // TODO(crbug.com/1138620): Implement.
+}
+
 void HttpssvcMetrics::set_doh_provider_id(
     base::Optional<std::string> new_doh_provider_id) {
   // "Other" never gets updated.
@@ -150,7 +156,8 @@ 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(features::kDnsHttpssvcUseIntegrity.Get() ||
+         features::kDnsHttpssvcUseHttpssvc.Get());
 
   DCHECK(!already_recorded_);
   already_recorded_ = true;

net/dns/httpssvc_metrics.h

@@ -81,6 +81,9 @@ class NET_EXPORT_PRIVATE HttpssvcMetrics {
                         enum HttpssvcDnsRcode rcode,
                         const std::vector<bool>& condensed_records,
                         base::TimeDelta integrity_resolve_time);
+  void SaveForHttps(base::Optional<std::string> doh_provider_id,
+                    enum HttpssvcDnsRcode rcode,
+                    base::TimeDelta https_resolve_time);
 
  private:
   std::string BuildMetricName(base::StringPiece leaf_name) const;

net/dns/public/dns_protocol.h

@@ -149,6 +149,7 @@ static const uint16_t kTypeAAAA = 28;
 static const uint16_t kTypeSRV = 33;
 static const uint16_t kTypeOPT = 41;
 static const uint16_t kTypeNSEC = 47;
+static const uint16_t kTypeHttps = 65;
 static const uint16_t kTypeANY = 255;
 
 // Experimental DNS record types pending IANA assignment.

net/dns/public/dns_query_type.h

@@ -21,14 +21,14 @@ enum class DnsQueryType {
   PTR,
   SRV,
   INTEGRITY,
-  MAX = INTEGRITY
+  HTTPS,
+  MAX = HTTPS
 };
 
 const DnsQueryType kDnsQueryTypes[] = {
-    DnsQueryType::UNSPECIFIED, DnsQueryType::A,   DnsQueryType::AAAA,
-    DnsQueryType::TXT,         DnsQueryType::PTR, DnsQueryType::SRV,
-    DnsQueryType::INTEGRITY,
-};
+    DnsQueryType::UNSPECIFIED, DnsQueryType::A,    DnsQueryType::AAAA,
+    DnsQueryType::TXT,         DnsQueryType::PTR,  DnsQueryType::SRV,
+    DnsQueryType::INTEGRITY,   DnsQueryType::HTTPS};
 
 static_assert(base::size(kDnsQueryTypes) ==
                   static_cast<unsigned>(DnsQueryType::MAX) + 1,

net/dns/record_rdata.cc

@@ -35,6 +35,9 @@ bool RecordRdata::HasValidSize(const base::StringPiece& data, uint16_t type) {
       return data.size() == IPAddress::kIPv6AddressSize;
     case dns_protocol::kExperimentalTypeIntegrity:
       return data.size() >= kIntegrityMinimumSize;
+    case dns_protocol::kTypeHttps:
+      // TODO(crbug.com/1138620): Implement actual size minimum.
+      return data.size() == 0;
     case dns_protocol::kTypeCNAME:
     case dns_protocol::kTypePTR:
     case dns_protocol::kTypeTXT:

services/network/public/cpp/host_resolver_mojom_traits.cc

@@ -214,6 +214,8 @@ DnsQueryType EnumTraits<DnsQueryType, net::DnsQueryType>::ToMojom(
       return DnsQueryType::PTR;
     case net::DnsQueryType::SRV:
       return DnsQueryType::SRV;
+    case net::DnsQueryType::HTTPS:
+      return DnsQueryType::HTTPS;
     case net::DnsQueryType::INTEGRITY:
       NOTIMPLEMENTED();
       return DnsQueryType::UNSPECIFIED;
@@ -243,6 +245,9 @@ bool EnumTraits<DnsQueryType, net::DnsQueryType>::FromMojom(
     case DnsQueryType::SRV:
       *output = net::DnsQueryType::SRV;
       return true;
+    case DnsQueryType::HTTPS:
+      *output = net::DnsQueryType::HTTPS;
+      return true;
   }
 }
 

services/network/public/mojom/host_resolver.mojom

@@ -163,6 +163,7 @@ enum DnsQueryType {
   TXT,
   PTR,
   SRV,
+  HTTPS,
 };
 
 // Parameter-grouping struct for additional optional parameters for