From 9341f771cdf4b8c04d4928648f57c15bcdf0f4fe Mon Sep 17 00:00:00 2001 From: Sandor Molnar Date: Fri, 21 Jan 2022 22:23:59 +0200 Subject: [PATCH] Backed out 2 changesets (bug 1743022) for causing xpc failures in test_trr_blocklist. CLOSED TREE Backed out changeset 1acf0c8e8663 (bug 1743022) Backed out changeset e8822e38828f (bug 1743022) --- modules/libpref/init/StaticPrefList.yaml | 7 -- modules/libpref/init/all.js | 3 + netwerk/dns/DNSPacket.cpp | 4 - netwerk/dns/TRR.h | 2 +- netwerk/dns/TRRService.cpp | 17 +++- netwerk/dns/TRRService.h | 2 + netwerk/docs/dns/dns-over-https-trr.rst | 6 +- netwerk/test/unit/head_trr.js | 2 +- netwerk/test/unit/test_trr_blocklist.js | 81 ------------------- netwerk/test/unit/xpcshell.ini | 1 - .../test/unit_ipc/test_trr_httpssvc_wrap.js | 2 +- tools/lint/rejected-words.yml | 14 ++++ 12 files changed, 37 insertions(+), 104 deletions(-) delete mode 100644 netwerk/test/unit/test_trr_blocklist.js diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 218d147b17a6..812b9736178a 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -10149,13 +10149,6 @@ value: true mirror: always -# TRR blocklist entry expire time (in seconds). Default is one minute. -# Meant to survive basically a page load. -- name: network.trr.temp_blocklist_duration_sec - type: RelaxedAtomicUint32 - value: 60 - mirror: always - # Single TRR request timeout, in milliseconds - name: network.trr.request_timeout_ms type: RelaxedAtomicUint32 diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js index e7454ff92991..0e413c268f7e 100644 --- a/modules/libpref/init/all.js +++ b/modules/libpref/init/all.js @@ -3949,6 +3949,9 @@ pref("network.trr.custom_uri", ""); // Before TRR is widely used the NS record for this host is fetched // from the DOH end point to ensure proper configuration pref("network.trr.confirmationNS", "example.com"); +// TRR blacklist entry expire time (in seconds). Default is one minute. +// Meant to survive basically a page load. +pref("network.trr.blacklist-duration", 60); // Comma separated list of domains that we should not use TRR for pref("network.trr.excluded-domains", ""); pref("network.trr.builtin-excluded-domains", "localhost,local"); diff --git a/netwerk/dns/DNSPacket.cpp b/netwerk/dns/DNSPacket.cpp index 68565672f74f..bbd68b88c096 100644 --- a/netwerk/dns/DNSPacket.cpp +++ b/netwerk/dns/DNSPacket.cpp @@ -1014,10 +1014,6 @@ nsresult DNSPacket::DecodeInternal( return NS_ERROR_ILLEGAL_VALUE; } - if (aType == TRRTYPE_NS && rcode != 0) { - return NS_ERROR_UNKNOWN_HOST; - } - if ((aType != TRRTYPE_NS) && aCname.IsEmpty() && aResp.mAddresses.IsEmpty() && aTypeResult.is()) { // no entries were stored! diff --git a/netwerk/dns/TRR.h b/netwerk/dns/TRR.h index d24e1d0c83bd..09b2fc426e2f 100644 --- a/netwerk/dns/TRR.h +++ b/netwerk/dns/TRR.h @@ -95,7 +95,7 @@ class TRR : public Runnable, // FailData() must be called to signal that the asynch TRR resolve is // completed. For failed name resolves ("no such host"), the 'error' it // passses on in its argument must be NS_ERROR_UNKNOWN_HOST. Other errors - // (if host was blocklisted, there as a bad content-type received, etc) + // (if host was blacklisted, there as a bad content-type received, etc) // other error codes must be used. This distinction is important for the // subsequent logic to separate the error reasons. nsresult FailData(nsresult error); diff --git a/netwerk/dns/TRRService.cpp b/netwerk/dns/TRRService.cpp index 25f801e5443b..8a76caac2818 100644 --- a/netwerk/dns/TRRService.cpp +++ b/netwerk/dns/TRRService.cpp @@ -35,7 +35,8 @@ static const char kDisableIpv6Pref[] = "network.dns.disableIPv6"; #define TRR_PREF_PREFIX "network.trr." #define TRR_PREF(x) TRR_PREF_PREFIX x -namespace mozilla::net { +namespace mozilla { +namespace net { StaticRefPtr sTRRBackgroundThread; static Atomic sTRRServicePtr; @@ -355,6 +356,14 @@ nsresult TRRService::ReadPrefs(const char* name) { Preferences::GetCString(TRR_PREF("bootstrapAddr"), mBootstrapAddr); clearEntireCache = true; } + if (!name || !strcmp(name, TRR_PREF("blacklist-duration"))) { + // prefs is given in number of seconds + uint32_t secs; + if (NS_SUCCEEDED( + Preferences::GetUint(TRR_PREF("blacklist-duration"), &secs))) { + mBlocklistDurationSeconds = secs; + } + } if (!name || !strcmp(name, kDisableIpv6Pref)) { bool tmp; if (NS_SUCCEEDED(Preferences::GetBool(kDisableIpv6Pref, &tmp))) { @@ -842,8 +851,7 @@ bool TRRService::IsDomainBlocked(const nsACString& aHost, // use a unified casing for the hashkey nsAutoCString hashkey(aHost + aOriginSuffix); if (auto val = bl->Lookup(hashkey)) { - int32_t until = - *val + int32_t(StaticPrefs::network_trr_temp_blocklist_duration_sec()); + int32_t until = *val + mBlocklistDurationSeconds; int32_t expire = NowInSeconds(); if (until > expire) { LOG(("Host [%s] is TRR blocklisted\n", nsCString(aHost).get())); @@ -1306,4 +1314,5 @@ void TRRService::InitTRRConnectionInfo() { } } -} // namespace mozilla::net +} // namespace net +} // namespace mozilla diff --git a/netwerk/dns/TRRService.h b/netwerk/dns/TRRService.h index fb75bb3d006e..dea8e302f31e 100644 --- a/netwerk/dns/TRRService.h +++ b/netwerk/dns/TRRService.h @@ -119,6 +119,8 @@ class TRRService : public TRRServiceBase, void AddEtcHosts(const nsTArray&); bool mInitialized{false}; + Atomic mBlocklistDurationSeconds{60}; + Mutex mLock{"TRRService"}; nsCString mPrivateCred; // main thread only diff --git a/netwerk/docs/dns/dns-over-https-trr.rst b/netwerk/docs/dns/dns-over-https-trr.rst index 06785e624848..eaa6e68b2810 100644 --- a/netwerk/docs/dns/dns-over-https-trr.rst +++ b/netwerk/docs/dns/dns-over-https-trr.rst @@ -46,7 +46,7 @@ a DoH or a Do53 request. First it checks the effective TRR mode of the request is as requests could have a different mode from the global one. If the request may use TRR, then we dispatch a request in nsHostResolver::TrrLookup. Since we usually reolve both IPv4 and IPv6 names, a **TRRQuery** object is -created to perform and combine both responses. +created to perform and combine both responses. Once done, nsHostResolver::CompleteLookup is called. If the DoH server returned a valid response we use it, otherwise we report a failure in TRR-only mode, or @@ -62,9 +62,7 @@ main thread. Dynamic Blocklist ----------------- -In order to improve performance TRR service manages a dynamic blocklist for host names that can't be resolved with DoH but work with the native resolver. Blocklisted entries will not be retried over DoH for one minute (See `network.trr.temp_blocklist_duration_sec` pref). -When a domain is added to the blocklist, we also check if there is an NS record for its parent domain, in which case we add that to the blocklist. -This feature is controlled by the `network.trr.temp_blocklist` pref. +In order to improve performance TRR service manages a dynamic persistent blocklist for host names that can't be resolved with DoH but works with the native resolver. Blocklisted entries will not be retried over DoH for one minute. TRR confirmation ---------------- diff --git a/netwerk/test/unit/head_trr.js b/netwerk/test/unit/head_trr.js index 75b70091e316..b1a652c76b3c 100644 --- a/netwerk/test/unit/head_trr.js +++ b/netwerk/test/unit/head_trr.js @@ -81,7 +81,7 @@ function trr_clear_prefs() { Services.prefs.clearUserPref("network.trr.useGET"); Services.prefs.clearUserPref("network.trr.confirmationNS"); Services.prefs.clearUserPref("network.trr.bootstrapAddr"); - Services.prefs.clearUserPref("network.trr.temp_blocklist_duration_sec"); + Services.prefs.clearUserPref("network.trr.blacklist-duration"); Services.prefs.clearUserPref("network.trr.request_timeout_ms"); Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms"); Services.prefs.clearUserPref("network.trr.disable-ECS"); diff --git a/netwerk/test/unit/test_trr_blocklist.js b/netwerk/test/unit/test_trr_blocklist.js deleted file mode 100644 index c7a70db21c32..000000000000 --- a/netwerk/test/unit/test_trr_blocklist.js +++ /dev/null @@ -1,81 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -"use strict"; - -const dns = Cc["@mozilla.org/network/dns-service;1"].getService( - Ci.nsIDNSService -); -const override = Cc["@mozilla.org/network/native-dns-override;1"].getService( - Ci.nsINativeDNSResolverOverride -); - -function setup() { - trr_test_setup(); - Services.prefs.setBoolPref("network.trr.temp_blocklist", true); -} -setup(); - -add_task(async function checkBlocklisting() { - let trrServer = new TRRServer(); - registerCleanupFunction(async () => { - await trrServer.stop(); - }); - await trrServer.start(); - info(`port = ${trrServer.port}\n`); - - dns.clearCache(true); - Services.prefs.setCharPref( - "network.trr.uri", - `https://foo.example.com:${trrServer.port}/dns-query` - ); - Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST); - - await trrServer.registerDoHAnswers("top.test.com", "NS", {}); - - override.addIPOverride("sub.top.test.com", "2.2.2.2"); - override.addIPOverride("sub2.top.test.com", "2.2.2.2"); - await new TRRDNSListener("sub.top.test.com", { - expectedAnswer: "2.2.2.2", - }); - equal(await trrServer.requestCount("sub.top.test.com", "A"), 1); - - // Clear the cache so that we need to consult the blocklist and not simply - // return the cached DNS record. - dns.clearCache(true); - await new TRRDNSListener("sub.top.test.com", { - expectedAnswer: "2.2.2.2", - }); - equal( - await trrServer.requestCount("sub.top.test.com", "A"), - 1, - "Request should go directly to native because result is still in blocklist" - ); - - // XXX(valentin): if this ever starts intermittently failing we need to add - // a sleep here. But the check for the parent NS should normally complete - // before the second subdomain request. - equal( - await trrServer.requestCount("top.test.com", "NS"), - 1, - "Should have checked parent domain" - ); - await new TRRDNSListener("sub2.top.test.com", { - expectedAnswer: "2.2.2.2", - }); - equal(await trrServer.requestCount("sub2.top.test.com", "A"), 0); - - // The blocklist should instantly expire. - Services.prefs.setIntPref("network.trr.temp_blocklist_duration_sec", 0); - dns.clearCache(true); - await new TRRDNSListener("sub.top.test.com", { - expectedAnswer: "2.2.2.2", - }); - // blocklist expired. Do another check. - equal( - await trrServer.requestCount("sub.top.test.com", "A"), - 2, - "We should do another TRR request because the bloclist expired" - ); -}); diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini index 0802746cf03f..18b44e87ac46 100644 --- a/netwerk/test/unit/xpcshell.ini +++ b/netwerk/test/unit/xpcshell.ini @@ -595,4 +595,3 @@ skip-if = os == "android" head = head_channels.js head_cache.js head_cookies.js head_trr.js trr_common.js skip-if = os == "android" run-sequentially = node server exceptions dont replay well -[test_trr_blocklist.js] diff --git a/netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js b/netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js index e2ceb1a39997..c3d20781ba62 100644 --- a/netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js +++ b/netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js @@ -61,7 +61,7 @@ registerCleanupFunction(() => { prefs.clearUserPref("network.trr.useGET"); prefs.clearUserPref("network.trr.confirmationNS"); prefs.clearUserPref("network.trr.bootstrapAddr"); - prefs.clearUserPref("network.trr.temp_blocklist_duration_sec"); + prefs.clearUserPref("network.trr.blacklist-duration"); prefs.clearUserPref("network.trr.request-timeout"); prefs.clearUserPref("network.trr.clear-cache-on-pref-change"); }); diff --git a/tools/lint/rejected-words.yml b/tools/lint/rejected-words.yml index a6cad6c9b54c..fddd561b2e46 100644 --- a/tools/lint/rejected-words.yml +++ b/tools/lint/rejected-words.yml @@ -215,9 +215,15 @@ avoid-blacklist-and-whitelist: - netwerk/base/nsURLHelper.cpp - netwerk/cookie/CookieCommons.h - netwerk/dns/nsHostRecord.cpp + - netwerk/dns/nsHostResolver.cpp - netwerk/dns/nsIDNService.cpp - netwerk/dns/nsIDNService.h - netwerk/dns/TRR.cpp + - netwerk/dns/TRR.h + - netwerk/dns/TRRServiceChild.cpp + - netwerk/dns/TRRService.cpp + - netwerk/dns/TRRService.h + - netwerk/dns/TRRServiceParent.cpp - netwerk/ipc/DocumentLoadListener.cpp - netwerk/protocol/about/nsAboutProtocolHandler.cpp - netwerk/protocol/http/Http2Session.cpp @@ -234,13 +240,21 @@ avoid-blacklist-and-whitelist: - netwerk/protocol/websocket/BaseWebSocketChannel.cpp - netwerk/socket/nsSOCKSSocketProvider.cpp - netwerk/test/gtest/TestCookie.cpp + - netwerk/test/unit/head_trr.js + - netwerk/test/unit_ipc/test_dns_by_type_resolve_wrap.js + - netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js - netwerk/test/unit/test_bug396389.js - netwerk/test/unit/test_bug427957.js - netwerk/test/unit/test_bug464591.js - netwerk/test/unit/test_bug479413.js - netwerk/test/unit/test_cookie_blacklist.js + - netwerk/test/unit/test_dns_by_type_resolve.js - netwerk/test/unit/test_idn_blacklist.js - netwerk/test/unit/test_idn_urls.js + - netwerk/test/unit/test_odoh.js + - netwerk/test/unit/test_trr_httpssvc.js + - netwerk/test/unit/test_trr.js + - netwerk/test/unit/test_use_httpssvc.js - netwerk/url-classifier/AsyncUrlChannelClassifier.cpp - netwerk/url-classifier/nsChannelClassifier.cpp - netwerk/url-classifier/nsChannelClassifier.h