Adds a concurrent ESNI transaction (with timeout) to the async resolver

This patch adds the ability to resolve ESNI (TLS 1.3 Encrypted
Server Name Indication, draft 4) results concurrently with A and
AAAA records during connection establishment.

This is behind a base::Feature, disabled by default. When the
feature is enabled and DNS over HTTPS is in use, the built-in
resolver will make an additional ESNI transaction concurrently with
the usual ones of type A and AAAA. It also starts a timer (initially
set to the 50ms recommended by the spec) past which to abandon
the ESNI transaction without failing the DNS task.

R=ericorth

Bug: 1003494
Change-Id: Ia73b5e910f2a466b6384c7c73f09705e770ea39d
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879430
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712131}

net/base/features.cc

@@ -45,6 +45,19 @@ const base::Feature kTLS13KeyUpdate{"TLS13KeyUpdate",
 const base::Feature kNetUnusedIdleSocketTimeout{
     "NetUnusedIdleSocketTimeout", base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kRequestEsniDnsRecords{"RequestEsniDnsRecords",
+                                           base::FEATURE_DISABLED_BY_DEFAULT};
+base::TimeDelta EsniDnsMaxAbsoluteAdditionalWait() {
+  DCHECK(base::FeatureList::IsEnabled(kRequestEsniDnsRecords));
+  return base::TimeDelta::FromMilliseconds(
+      kEsniDnsMaxAbsoluteAdditionalWaitMilliseconds.Get());
+}
+const base::FeatureParam<int> kEsniDnsMaxAbsoluteAdditionalWaitMilliseconds{
+    &kRequestEsniDnsRecords, "EsniDnsMaxAbsoluteAdditionalWaitMilliseconds",
+    10};
+const base::FeatureParam<int> kEsniDnsMaxRelativeAdditionalWaitPercent{
+    &kRequestEsniDnsRecords, "EsniDnsMaxRelativeAdditionalWaitPercent", 5};
+
 const base::Feature kSameSiteByDefaultCookies{
     "SameSiteByDefaultCookies", base::FEATURE_DISABLED_BY_DEFAULT};
 

net/base/features.h

@@ -65,6 +65,25 @@ NET_EXPORT extern const base::Feature kTLS13KeyUpdate;
 // Changes the timeout after which unused sockets idle sockets are cleaned up.
 NET_EXPORT extern const base::Feature kNetUnusedIdleSocketTimeout;
 
+// Enables the built-in resolver requesting ESNI (TLS 1.3 Encrypted
+// Server Name Indication) records alongside IPv4 and IPv6 address records
+// during DNS over HTTPS (DoH) host resolution.
+NET_EXPORT extern const base::Feature kRequestEsniDnsRecords;
+// Returns a TimeDelta of value kEsniDnsMaxAbsoluteAdditionalWaitMilliseconds
+// milliseconds (see immediately below).
+NET_EXPORT base::TimeDelta EsniDnsMaxAbsoluteAdditionalWait();
+// The following two parameters specify the amount of extra time to wait for a
+// long-running ESNI DNS transaction after the successful conclusion of
+// concurrent A and AAAA transactions. This timeout will have value
+// min{kEsniDnsMaxAbsoluteAdditionalWaitMilliseconds,
+//     (100% + kEsniDnsMaxRelativeAdditionalWaitPercent)
+//       * max{time elapsed for the concurrent A query,
+//             time elapsed for the concurrent AAAA query}}.
+NET_EXPORT extern const base::FeatureParam<int>
+    kEsniDnsMaxAbsoluteAdditionalWaitMilliseconds;
+NET_EXPORT extern const base::FeatureParam<int>
+    kEsniDnsMaxRelativeAdditionalWaitPercent;
+
 // When enabled, makes cookies without a SameSite attribute behave like
 // SameSite=Lax cookies by default, and requires SameSite=None to be specified
 // in order to make cookies available in a third-party context. When disabled,

net/dns/dns_test_util.cc

@@ -628,6 +628,18 @@ void MockDnsTransactionFactory::CompleteDelayedTransactions() {
   }
 }
 
+bool MockDnsTransactionFactory::CompleteOneDelayedTransactionOfType(
+    DnsQueryType type) {
+  for (base::WeakPtr<MockTransaction>& t : delayed_transactions_) {
+    if (t && t->GetType() == DnsQueryTypeToQtype(type)) {
+      t->FinishDelayedTransaction();
+      t.reset();
+      return true;
+    }
+  }
+  return false;
+}
+
 MockDnsClient::MockDnsClient(DnsConfig config, MockDnsClientRuleList rules)
     : config_(std::move(config)),
       factory_(new MockDnsTransactionFactory(std::move(rules))),
@@ -736,6 +748,10 @@ void MockDnsClient::CompleteDelayedTransactions() {
   factory_->CompleteDelayedTransactions();
 }
 
+bool MockDnsClient::CompleteOneDelayedTransactionOfType(DnsQueryType type) {
+  return factory_->CompleteOneDelayedTransactionOfType(type);
+}
+
 base::Optional<DnsConfig> MockDnsClient::BuildEffectiveConfig() {
   if (overrides_.OverridesEverything())
     return overrides_.ApplyOverrides(DnsConfig());

net/dns/dns_test_util.h

@@ -314,6 +314,10 @@ class MockDnsTransactionFactory : public DnsTransactionFactory {
   DnsConfig::SecureDnsMode GetSecureDnsModeForTest() override;
 
   void CompleteDelayedTransactions();
+  // If there are any pending transactions of the given type,
+  // completes one and returns true. Otherwise, returns false.
+  bool CompleteOneDelayedTransactionOfType(DnsQueryType type)
+      WARN_UNUSED_RESULT;
 
   bool doh_probes_running() { return doh_probes_running_; }
 
@@ -358,6 +362,10 @@ class MockDnsClient : public DnsClient {
 
   // Completes all DnsTransactions that were delayed by a rule.
   void CompleteDelayedTransactions();
+  // If there are any pending transactions of the given type,
+  // completes one and returns true. Otherwise, returns false.
+  bool CompleteOneDelayedTransactionOfType(DnsQueryType type)
+      WARN_UNUSED_RESULT;
 
   void set_max_fallback_failures(int max_fallback_failures) {
     max_fallback_failures_ = max_fallback_failures;

net/dns/dns_util.h

@@ -106,7 +106,7 @@ AddressListDeltaType FindAddressListDeltaType(const AddressList& a,
 NET_EXPORT std::string CreateNamePointer(uint16_t offset);
 
 // Convert a DnsQueryType enum to the wire format integer representation.
-uint16_t DnsQueryTypeToQtype(DnsQueryType dns_query_type);
+NET_EXPORT_PRIVATE uint16_t DnsQueryTypeToQtype(DnsQueryType dns_query_type);
 
 NET_EXPORT DnsQueryType
 AddressFamilyToDnsQueryType(AddressFamily address_family);

net/dns/host_resolver_manager.cc

@@ -1016,6 +1016,11 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     } else {
       transactions_needed_.push(DnsQueryType::A);
       transactions_needed_.push(DnsQueryType::AAAA);
+
+      if (secure_ &&
+          base::FeatureList::IsEnabled(features::kRequestEsniDnsRecords)) {
+        transactions_needed_.push(DnsQueryType::ESNI);
+      }
     }
     num_needed_transactions_ = transactions_needed_.size();
 
@@ -1035,7 +1040,8 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
   void StartNextTransaction() {
     DCHECK(needs_another_transaction());
 
-    if (transactions_started_.empty())
+    if (num_needed_transactions_ ==
+        static_cast<int>(transactions_needed_.size()))
       net_log_.BeginEvent(NetLogEventType::HOST_RESOLVER_IMPL_DNS_TASK);
 
     DnsQueryType type = transactions_needed_.front();
@@ -1067,12 +1073,45 @@ 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(std::all_of(
+        transactions_started_.begin(), transactions_started_.end(),
+        [&](const std::unique_ptr<DnsTransaction>& p) {
+          return p && p->GetType() == dns_protocol::kExperimentalTypeEsniDraft4;
+        }));
+
+    num_completed_transactions_ += transactions_started_.size();
+    DCHECK(num_completed_transactions_ == num_needed_transactions());
+    transactions_started_.clear();
+
+    // TODO(crbug.com/1003494): Log the failure to a histogram, along with
+    // the values of the absolute and relative additional wait parameters.
+
+    ProcessResultsOnCompletion();
+  }
+
   void OnTransactionComplete(const base::TimeTicks& start_time,
                              DnsQueryType dns_query_type,
                              DnsTransaction* transaction,
                              int net_error,
                              const DnsResponse* response) {
     DCHECK(transaction);
+
+    // Once control leaves OnTransactionComplete, there's no further
+    // need for the transaction object. On the other hand, since it owns
+    // |*response|, it should stay around while OnTransactionComplete
+    // executes.
+    std::unique_ptr<DnsTransaction> destroy_transaction_on_return;
+    {
+      auto it = transactions_started_.find(transaction);
+      DCHECK(it != transactions_started_.end());
+
+      destroy_transaction_on_return = std::move(*it);
+      transactions_started_.erase(it);
+    }
+
     if (net_error != OK && !(net_error == ERR_NAME_NOT_RESOLVED && response &&
                              response->IsValid())) {
       OnFailure(net_error, DnsResponse::DNS_PARSE_OK, base::nullopt);
@@ -1141,15 +1180,30 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
       }
     }
 
+    saved_results_ = std::move(results);
+
     // If not all transactions are complete, the task cannot yet be completed
     // and the results so far must be saved to merge with additional results.
     ++num_completed_transactions_;
     if (num_completed_transactions_ < num_needed_transactions()) {
-      saved_results_ = std::move(results);
       delegate_->OnIntermediateTransactionComplete();
+      MaybeStartEsniTimer();
       return;
     }
 
+    // Since all transactions are complete, in particular, all ESNI transactions
+    // are complete (if any were started).
+    esni_cancellation_timer_.Stop();
+
+    ProcessResultsOnCompletion();
+  }
+
+  // Postprocesses the transactions' aggregated results after all
+  // transactions have completed.
+  void ProcessResultsOnCompletion() {
+    DCHECK(saved_results_.has_value());
+    HostCache::Entry results = std::move(*saved_results_);
+
     // 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
@@ -1492,6 +1546,39 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
     delegate_->OnDnsTaskComplete(task_start_time_, results, secure_);
   }
 
+  // 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() {
+    DCHECK(!transactions_started_.empty());
+    DCHECK(saved_results_);
+    if (!esni_cancellation_timer_.IsRunning() &&
+        std::all_of(transactions_started_.begin(), transactions_started_.end(),
+                    [&](const std::unique_ptr<DnsTransaction>& p) {
+                      DCHECK(p);
+                      return p->GetType() ==
+                             dns_protocol::kExperimentalTypeEsniDraft4;
+                    })) {
+      base::TimeDelta total_time_taken_for_other_transactions =
+          tick_clock_->NowTicks() - task_start_time_;
+
+      esni_cancellation_timer_.Start(
+          FROM_HERE,
+          std::min(
+              features::EsniDnsMaxAbsoluteAdditionalWait(),
+              total_time_taken_for_other_transactions *
+                  (0.01 *
+                   features::kEsniDnsMaxRelativeAdditionalWaitPercent.Get())),
+          this, &DnsTask::OnEsniTransactionTimeout);
+    }
+  }
+
   DnsClient* client_;
   std::string hostname_;
   URLRequestContext* const request_context_;
@@ -1505,7 +1592,8 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
   const NetLogWithSource net_log_;
 
   base::queue<DnsQueryType> transactions_needed_;
-  base::flat_set<std::unique_ptr<DnsTransaction>> transactions_started_;
+  base::flat_set<std::unique_ptr<DnsTransaction>, base::UniquePtrComparator>
+      transactions_started_;
   int num_needed_transactions_;
   int num_completed_transactions_;
 
@@ -1516,6 +1604,10 @@ class HostResolverManager::DnsTask : public base::SupportsWeakPtr<DnsTask> {
   const base::TickClock* tick_clock_;
   base::TimeTicks task_start_time_;
 
+  // Timer for early abort of ESNI transactions. See comments describing
+  // the timeout parameters in net/base/features.h.
+  base::OneShotTimer esni_cancellation_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(DnsTask);
 };