1743022 - Add tests for TRR temporary blocklisting and ensure we clear the temp blocklist when clearing the cache

Status: RESOLVED FIXED

(Core :: Networking: DNS, task, P2)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

So I discovered while debugging some tests for bug 1737198 that names appeared to stay temporarily blocklisted between separate tasks - i.e. that calling dnsService.clearCache() did not clear the blocklist. Upon investigation it seems clear that the temp blocklisting code does not have test coverage - can be seen by digging into the commits that added it as well as by the fact that returning false early from TRRService::IsTemporarilyBlocked() does not cause any failures in any of the test_trr*.js test files.

We should add tests for this functionality and make sure it's not interfering with our existing tests and possibly masking any bugs.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

I won't have time to dig into this. For posterity, here's what I know:

1. The blocklisting code activates when seeing a failure.
2. We check each parent of the original name for failure (via an NS lookup) and if we see failure, we blocklist the parent names too.
3. The key point here is that adding to the blocklist can trigger further DoH lookups, and asynchronously mutates a cache map.
4. If blocklisting is left turned on, it's possible and likely that such lookups can be pending when we transition from one task to the next, in tests. This means that there is scope for unexpected mutation of possibly relevant state during the next task, and can interfere with assertions.
5. So far this hasn't manifested in failures, but with the introduction of strict mode and retried lookups, counting the number of performed lookups for example would fail if a blocklist lookup leaks into the task.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Valentin, is this something you can pick up?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1743022 - Rename network.trr.blacklist-duration to network.trr.temp_blocklist_durations_sec r=#necko

Also turns it into a static pref r=#necko

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1743022 - Add tests for TRR temporary blocklisting r=#necko

• DNSPacket::Decode now returns an error code for NS responses with a non-zero
  RCODE. Previously, when we'd do the check for the parent domain, we'd treat
  any DoH response as a valid NS, making the entire check for parents useless.
• Changes the documentation for this feature to mention the prefs used by this
  feature.
• I don't think we need to worry about clearing the blocklist when the DNS
  cache is cleared. For testing we can simply disable the blocklist. In real
  life the blocklist is only 60 seconds and it's unlikely to cause problems
  for users.

Depends on D136530

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8822e38828f
Rename network.trr.blacklist-duration to network.trr.temp_blocklist_durations_sec r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/1acf0c8e8663
Add tests for TRR temporary blocklisting r=necko-reviewers,kershaw

Comment 6

[Sandor Molnar[:smolnar]]

Backed out 2 changesets (bug 1743022) for causing xpc failures in test_trr_blocklist.

Backout link: https://hg.mozilla.org/integration/autoland/rev/bd2e82e136f6e30411c382c49f49e02b604ea6c9

Push with failures

Failure log

TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_trr_blocklist.js | checkBlocklisting - [checkBlocklisting : 59] Should have checked parent domain - 2 == 1
[task 2022-01-21T20:00:56.649Z] 20:00:56     INFO -  Z:/task_164279311528973/build/tests/xpcshell/tests/netwerk/test/unit/test_trr_blocklist.js:checkBlocklisting:59
[task 2022-01-21T20:00:56.650Z] 20:00:56     INFO -  Z:\task_164279311528973\build\tests\xpcshell\head.js:_do_main:240
[task 2022-01-21T20:00:56.650Z] 20:00:56     INFO -  Z:\task_164279311528973\build\tests\xpcshell\head.js:_execute_test:604
[task 2022-01-21T20:00:56.650Z] 20:00:56     INFO -  -e:null:1
[task 2022-01-21T20:00:56.650Z] 20:00:56     INFO -  exiting test
[task 2022-01-21T20:00:56.650Z] 20:00:56     INFO -  Unexpected exception NS_ERROR_ABORT:
[task 2022-01-21T20:00:56.651Z] 20:00:56     INFO -  _abort_failed_test@Z:\task_164279311528973\build\tests\xpcshell\head.js:875:20
[task 2022-01-21T20:00:56.651Z] 20:00:56     INFO -  do_report_result@Z:\task_164279311528973\build\tests\xpcshell\head.js:976:5
[task 2022-01-21T20:00:56.651Z] 20:00:56     INFO -  Assert<@Z:\task_164279311528973\build\tests\xpcshell\head.js:75:21
[task 2022-01-21T20:00:56.651Z] 20:00:56     INFO -  proto.report@resource://testing-common/Assert.jsm:228:10
[task 2022-01-21T20:00:56.652Z] 20:00:56     INFO -  equal@resource://testing-common/Assert.jsm:270:8
[task 2022-01-21T20:00:56.652Z] 20:00:56     INFO -  checkBlocklisting@Z:/task_164279311528973/build/tests/xpcshell/tests/netwerk/test/unit/test_trr_blocklist.js:59:8
[task 2022-01-21T20:00:56.652Z] 20:00:56     INFO -  _do_main@Z:\task_164279311528973\build\tests\xpcshell\head.js:240:6
[task 2022-01-21T20:00:56.652Z] 20:00:56     INFO -  _execute_test@Z:\task_164279311528973\build\tests\xpcshell\head.js:604:5
[task 2022-01-21T20:00:56.652Z] 20:00:56     INFO -  @-e:1:1
[task 2022-01-21T20:00:56.653Z] 20:00:56     INFO -  exiting test

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1743022 - Only call AddToBlocklist once r=#necko

On windows getaddrinfo does not return TTL info for the DNS record. so we do a
second DNS resolution that uses DNSQueryA to get the TTL. We should only call
AddToBlocklist after the first DNS resolution completes. Doing so again for
the DNSQueryA refresh is unnecessary and possibly extends the lifetime of the
blocklist past the 60s that is default.

This patch also reenables the blocklist when TRR strict mode is enabled.

Depends on D136531

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07b24d87f3b2
Rename network.trr.blacklist-duration to network.trr.temp_blocklist_durations_sec r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/fea39912fecb
Add tests for TRR temporary blocklisting r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/c056411bccbd
Only call AddToBlocklist once r=necko-reviewers,kershaw

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/07b24d87f3b2
https://hg.mozilla.org/mozilla-central/rev/fea39912fecb
https://hg.mozilla.org/mozilla-central/rev/c056411bccbd