Add is_secure_network_error field to ResolveErrorInfo.

This new field represents whether a host resolution error resulted
directly from a DNS-over-HTTPS attempt. It will be used in future cls to
prepare the error page for secure DNS errors and to trigger captive
portal probes. Note that if a result was obtained from the cache this
field will be false, regardless of whether the result was originally
obtained securely, because this field is intended to identify secure
DNS *network* failures.

Bug: 1037899
Change-Id: I21b8afaeb103f12d7d572f8cf8b29d2e1228d007
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978106
Commit-Queue: Katharine Daly <dalyk@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727821}

net/dns/host_resolver_manager.cc

@@ -600,7 +600,9 @@ class HostResolverManager::RequestImpl
     results_ = std::move(results);
   }
 
-  void set_error_info(int error) { error_info_ = ResolveErrorInfo(error); }
+  void set_error_info(int error, bool is_secure_network_error) {
+    error_info_ = ResolveErrorInfo(error, is_secure_network_error);
+  }
 
   void set_stale_info(HostCache::EntryStaleness stale_info) {
     // Should only be called at most once and before request is marked
@@ -634,9 +636,10 @@ class HostResolverManager::RequestImpl
   }
 
   // Cleans up Job assignment, marks request completed, and calls the completion
-  // callback.
-  void OnJobCompleted(Job* job, int error) {
-    set_error_info(error);
+  // callback. |is_secure_network_error| indicates whether |error| came from a
+  // secure DNS lookup.
+  void OnJobCompleted(Job* job, int error, bool is_secure_network_error) {
+    set_error_info(error, is_secure_network_error);
 
     DCHECK_EQ(job_, job);
     job_ = nullptr;
@@ -2591,7 +2594,9 @@ class HostResolverManager::Job : public PrioritizedDispatcher::Job,
         req->set_results(
             results.CopyWithDefaultPort(req->request_host().port()));
       }
-      req->OnJobCompleted(this, results.error());
+      req->OnJobCompleted(
+          this, results.error(),
+          secure && results.error() != OK /* is_secure_network_error */);
 
       // Check if the resolver was destroyed as a result of running the
       // callback. If it was, we could continue, but we choose to bail.
@@ -2985,7 +2990,8 @@ int HostResolverManager::Resolve(RequestImpl* request) {
       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());
+    request->set_error_info(results.error(),
+                            false /* is_secure_network_error */);
     return results.error();
   }
 

net/dns/host_resolver_manager_unittest.cc

@@ -5300,6 +5300,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic) {
       HostPortPair("automatic", 80), NetworkIsolationKey(), NetLogWithSource(),
       base::nullopt, request_context_.get(), host_cache_.get()));
   ASSERT_THAT(response_secure.result_error(), IsOk());
+  EXPECT_FALSE(
+      response_secure.request()->GetResolveErrorInfo().is_secure_network_error);
   EXPECT_THAT(
       response_secure.request()->GetAddressResults().value().endpoints(),
       testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
@@ -5318,6 +5320,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic) {
       NetLogWithSource(), base::nullopt, request_context_.get(),
       host_cache_.get()));
   ASSERT_THAT(response_insecure.result_error(), IsOk());
+  EXPECT_FALSE(response_insecure.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(
       response_insecure.request()->GetAddressResults().value().endpoints(),
       testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
@@ -5360,6 +5365,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_SecureCache) {
       NetLogWithSource(), base::nullopt, request_context_.get(),
       host_cache_.get()));
   EXPECT_THAT(response_secure_cached.result_error(), IsOk());
+  EXPECT_FALSE(response_secure_cached.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(
       response_secure_cached.request()->GetAddressResults().value().endpoints(),
       testing::ElementsAre(kExpectedSecureIP));
@@ -5387,6 +5395,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_InsecureCache) {
       NetLogWithSource(), base::nullopt, request_context_.get(),
       host_cache_.get()));
   EXPECT_THAT(response_insecure_cached.result_error(), IsOk());
+  EXPECT_FALSE(response_insecure_cached.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(response_insecure_cached.request()
                   ->GetAddressResults()
                   .value()
@@ -5473,6 +5484,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_Unavailable) {
       HostPortPair("automatic", 80), NetworkIsolationKey(), NetLogWithSource(),
       base::nullopt, request_context_.get(), host_cache_.get()));
   ASSERT_THAT(response_automatic.result_error(), IsOk());
+  EXPECT_FALSE(response_automatic.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(
       response_automatic.request()->GetAddressResults().value().endpoints(),
       testing::UnorderedElementsAre(CreateExpected("127.0.0.1", 80),
@@ -5505,6 +5519,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_Unavailable_Fail) {
       HostPortPair("secure", 80), NetworkIsolationKey(), NetLogWithSource(),
       base::nullopt, request_context_.get(), host_cache_.get()));
   ASSERT_THAT(response_secure.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_FALSE(
+      response_secure.request()->GetResolveErrorInfo().is_secure_network_error);
 
   HostCache::Key secure_key = HostCache::Key(
       "secure", DnsQueryType::UNSPECIFIED, 0 /* host_resolver_flags */,
@@ -5546,6 +5562,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_Stale) {
       NetLogWithSource(), stale_allowed_parameters, request_context_.get(),
       host_cache_.get()));
   EXPECT_THAT(response_stale.result_error(), IsOk());
+  EXPECT_FALSE(
+      response_stale.request()->GetResolveErrorInfo().is_secure_network_error);
   EXPECT_THAT(response_stale.request()->GetAddressResults().value().endpoints(),
               testing::ElementsAre(kExpectedStaleIP));
   EXPECT_TRUE(response_stale.request()->GetStaleInfo()->is_stale());
@@ -5652,6 +5670,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_DotActive) {
       host_cache_.get()));
   proc_->SignalMultiple(1u);
   ASSERT_THAT(response_insecure.result_error(), IsOk());
+  EXPECT_FALSE(response_insecure.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(
       response_insecure.request()->GetAddressResults().value().endpoints(),
       testing::ElementsAre(CreateExpected("192.168.1.100", 80)));
@@ -5675,6 +5696,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Automatic_DotActive) {
       NetLogWithSource(), base::nullopt, request_context_.get(),
       host_cache_.get()));
   EXPECT_THAT(response_insecure_cached.result_error(), IsOk());
+  EXPECT_FALSE(response_insecure_cached.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_THAT(response_insecure_cached.request()
                   ->GetAddressResults()
                   .value()
@@ -5696,6 +5720,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure) {
       HostPortPair("secure", 80), NetworkIsolationKey(), NetLogWithSource(),
       base::nullopt, request_context_.get(), host_cache_.get()));
   ASSERT_THAT(response_secure.result_error(), IsOk());
+  EXPECT_FALSE(
+      response_secure.request()->GetResolveErrorInfo().is_secure_network_error);
   HostCache::Key secure_key = HostCache::Key(
       "secure", DnsQueryType::UNSPECIFIED, 0 /* host_resolver_flags */,
       HostResolverSource::ANY, NetworkIsolationKey());
@@ -5707,6 +5733,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure) {
       HostPortPair("ok", 80), NetworkIsolationKey(), NetLogWithSource(),
       base::nullopt, request_context_.get(), host_cache_.get()));
   ASSERT_THAT(response_insecure.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_TRUE(response_insecure.request()
+                  ->GetResolveErrorInfo()
+                  .is_secure_network_error);
   HostCache::Key insecure_key = HostCache::Key(
       "ok", DnsQueryType::UNSPECIFIED, 0 /* host_resolver_flags */,
       HostResolverSource::ANY, NetworkIsolationKey());
@@ -5719,6 +5748,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure) {
       base::nullopt, request_context_.get(), host_cache_.get()));
   proc_->SignalMultiple(1u);
   EXPECT_THAT(response_proc.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
+  EXPECT_TRUE(
+      response_proc.request()->GetResolveErrorInfo().is_secure_network_error);
 }
 
 TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure_InsecureAsyncDisabled) {
@@ -5768,6 +5799,9 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure_Local_CacheMiss) {
       source_none_parameters, request_context_.get(), host_cache_.get()));
   EXPECT_TRUE(cache_miss_request.complete());
   EXPECT_THAT(cache_miss_request.result_error(), IsError(ERR_DNS_CACHE_MISS));
+  EXPECT_FALSE(cache_miss_request.request()
+                   ->GetResolveErrorInfo()
+                   .is_secure_network_error);
   EXPECT_FALSE(cache_miss_request.request()->GetAddressResults());
   EXPECT_FALSE(cache_miss_request.request()->GetStaleInfo());
 }
@@ -5796,6 +5830,8 @@ TEST_F(HostResolverManagerDnsTest, SecureDnsMode_Secure_Local_CacheHit) {
       base::nullopt, request_context_.get(), host_cache_.get()));
   EXPECT_TRUE(response_cached.complete());
   EXPECT_THAT(response_cached.result_error(), IsOk());
+  EXPECT_FALSE(
+      response_cached.request()->GetResolveErrorInfo().is_secure_network_error);
   EXPECT_THAT(
       response_cached.request()->GetAddressResults().value().endpoints(),
       testing::ElementsAre(kExpectedSecureIP));

net/dns/public/resolve_error_info.cc

@@ -8,8 +8,10 @@ namespace net {
 
 ResolveErrorInfo::ResolveErrorInfo() {}
 
-ResolveErrorInfo::ResolveErrorInfo(int resolve_error) {
-  error = resolve_error;
+ResolveErrorInfo::ResolveErrorInfo(int resolve_error,
+                                   bool is_secure_network_error)
+    : error(resolve_error), is_secure_network_error(is_secure_network_error) {
+  DCHECK(!(is_secure_network_error && resolve_error == net::OK));
 }
 
 ResolveErrorInfo::ResolveErrorInfo(const ResolveErrorInfo& resolve_error_info) =
@@ -24,7 +26,8 @@ ResolveErrorInfo& ResolveErrorInfo::operator=(ResolveErrorInfo&& other) =
     default;
 
 bool ResolveErrorInfo::operator==(const ResolveErrorInfo& other) const {
-  return error == other.error;
+  return error == other.error &&
+         is_secure_network_error == other.is_secure_network_error;
 }
 
 bool ResolveErrorInfo::operator!=(const ResolveErrorInfo& other) const {

net/dns/public/resolve_error_info.h

@@ -13,7 +13,7 @@ namespace net {
 // Host resolution error info.
 struct NET_EXPORT ResolveErrorInfo {
   ResolveErrorInfo();
-  ResolveErrorInfo(int resolve_error);
+  ResolveErrorInfo(int resolve_error, bool is_secure_network_error = false);
   ResolveErrorInfo(const ResolveErrorInfo& resolve_error_info);
   ResolveErrorInfo(ResolveErrorInfo&& other);
 
@@ -24,6 +24,12 @@ struct NET_EXPORT ResolveErrorInfo {
   bool operator!=(const ResolveErrorInfo& other) const;
 
   int error = net::OK;
+  // Whether |error| resulted from a DNS-over-HTTPS lookup. If an answer was
+  // obtained from the cache this field will be false, regardless of whether the
+  // answer was originally obtained securely, because this field is intended to
+  // identify secure DNS *network* failures. This field will also always be
+  // false if |error| is net::OK.
+  bool is_secure_network_error = false;
 };
 
 }  // namespace net

net/url_request/http_with_dns_over_https_unittest.cc

@@ -287,5 +287,37 @@ TEST_F(HttpWithDnsOverHttpsTest, EndToEnd) {
   EXPECT_EQ(d.data_received(), kTestBody);
 }
 
+TEST_F(HttpWithDnsOverHttpsTest, EndToEndFail) {
+  // Shutdown the DoH server so that all DoH requests are refused.
+  EXPECT_TRUE(doh_server_.ShutdownAndWaitUntilComplete());
+
+  // Make a request that will trigger a DoH query.
+  TestDelegate d;
+  GURL main_url = test_server_.GetURL("fail.example.com", "/test");
+  std::unique_ptr<URLRequest> req(context()->CreateRequest(
+      main_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+  req->Start();
+  base::RunLoop().Run();
+  EXPECT_TRUE(test_server_.ShutdownAndWaitUntilComplete());
+
+  // The two DoH lookups for "fail.example.com" (both A and AAAA
+  // records are queried) should both have failed to reach the DoH server
+  // since it was not running.
+  EXPECT_EQ(doh_queries_served_, 0u);
+  // The requests to the DoH server are pooled, so there should only be one
+  // insecure lookup for the DoH server hostname.
+  EXPECT_EQ(host_resolver_proc_->insecure_queries_served(), 1u);
+  // No HTTPS connection to the test server will be attempted due to the
+  // host resolution error.
+  EXPECT_EQ(test_https_requests_served_, 0u);
+
+  EXPECT_TRUE(d.response_completed());
+  EXPECT_EQ(d.request_status(), net::ERR_CONNECTION_REFUSED);
+
+  const auto& resolve_error_info = req->response_info().resolve_error_info;
+  EXPECT_TRUE(resolve_error_info.is_secure_network_error);
+  EXPECT_EQ(resolve_error_info.error, net::ERR_CONNECTION_REFUSED);
+}
+
 }  // namespace
 }  // namespace net

services/network/public/cpp/host_resolver_mojom_traits.cc

@@ -422,11 +422,16 @@ bool EnumTraits<network::mojom::SecureDnsMode, net::DnsConfig::SecureDnsMode>::
   return false;
 }
 
+// static
 bool StructTraits<
     network::mojom::ResolveErrorInfoDataView,
     net::ResolveErrorInfo>::Read(network::mojom::ResolveErrorInfoDataView data,
                                  net::ResolveErrorInfo* out) {
-  *out = net::ResolveErrorInfo(data.error());
+  // There should not be a secure network error if the error code indicates no
+  // error.
+  if (data.error() == net::OK && data.is_secure_network_error())
+    return false;
+  *out = net::ResolveErrorInfo(data.error(), data.is_secure_network_error());
   return true;
 }
 

services/network/public/cpp/host_resolver_mojom_traits.h

@@ -131,6 +131,11 @@ class StructTraits<network::mojom::ResolveErrorInfoDataView,
     return resolve_error_info.error;
   }
 
+  static bool is_secure_network_error(
+      net::ResolveErrorInfo resolve_error_info) {
+    return resolve_error_info.is_secure_network_error;
+  }
+
   static bool Read(network::mojom::ResolveErrorInfoDataView data,
                    net::ResolveErrorInfo* out);
 };

services/network/public/cpp/net_ipc_param_traits.cc

@@ -317,11 +317,13 @@ void ParamTraits<net::OCSPVerifyResult>::Log(const param_type& p,
 void ParamTraits<net::ResolveErrorInfo>::Write(base::Pickle* m,
                                                const param_type& p) {
   WriteParam(m, p.error);
+  WriteParam(m, p.is_secure_network_error);
 }
 bool ParamTraits<net::ResolveErrorInfo>::Read(const base::Pickle* m,
                                               base::PickleIterator* iter,
                                               param_type* r) {
-  return ReadParam(m, iter, &r->error);
+  return ReadParam(m, iter, &r->error) &&
+         ReadParam(m, iter, &r->is_secure_network_error);
 }
 void ParamTraits<net::ResolveErrorInfo>::Log(const param_type& p,
                                              std::string* l) {

services/network/public/mojom/host_resolver.mojom

@@ -117,6 +117,11 @@ struct ResolveErrorInfo {
   // Underlying network error code. See net/base/net_error_list.h for error
   // descriptions.
   int32 error;
+
+  // Whether |error| came from a DNS-over-HTTPS lookup. This will be false if
+  // the answer was obtained from the cache or if |error| is net::OK since this
+  // field is intended to identify secure DNS *network* failures.
+  bool is_secure_network_error = false;
 };
 
 // Control handle used to control outstanding NetworkContext::ResolveHost