1582472 - Exclude DHCP Search Suffixes from TRR

Status: RESOLVED FIXED

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

Description

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

mt:
The DNS domain search list (DHCPv4 option 119 or DHCPv6 option 24) provides a list of domain suffixes that clients might append to names. This enables the use of URLs with bare names (e.g., “https://go/”) in client software. These names are frequently attached to local name resolution, so we might assume that any eTLD+1 that is on this list is a suffix of a local name. Thus, if a name matches the search list and includes more labels than the provided suffix, it can be sent to the local resolver. Bare names would also be sent to the local resolver. Using eTLD+1 ensures that networks can’t configure search lists like “.com” to cause requests to be routed to a local resolver. The natural limits on the size of DHCP responses will ensure that this list is short and is sufficient encouragement to network operators to limit the list to those names that are important to them.

Comment 1

[saschmit]

Note: this would be a great way to solve most split-horizon issues (bug #1512255). I have a split-horizon DNS, and I'm not an enterprise, but when I'm on my network, the split-horizon name is in my dhcp search list. Every enterprise domain I know of does the same.

Comment 2

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

Attachment: Bug 1582472 - Exclude DHCP Search Suffixes from TRR r=kershaw,dragana

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9042c3ee2c0b
Exclude DHCP Search Suffixes from TRR r=kershaw

Comment 4

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 9042c3ee2c0b (bug 1582472) for xpcshell failures at toolkit/components/downloads/test/unit/test_DownloadIntegration.js

Backout: https://hg.mozilla.org/integration/autoland/rev/3b0c05f4e7c786618a8ba7b138dfedc73e27a4b0

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9042c3ee2c0bc43d3b65c4fe262cf77e67ab1897

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271292213&repo=autoland&lineNumber=2601

task 2019-10-15T12:37:45.685Z] 12:37:45 INFO - TEST-START | toolkit/components/downloads/test/unit/test_DownloadIntegration.js
[task 2019-10-15T12:37:46.844Z] 12:37:46 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/downloads/test/unit/test_DownloadIntegration.js | xpcshell return code: -11
[task 2019-10-15T12:37:46.844Z] 12:37:46 INFO - TEST-INFO took 1162ms
[task 2019-10-15T12:37:46.845Z] 12:37:46 INFO - >>>>>>>
[task 2019-10-15T12:37:46.845Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.845Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2632
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2019-10-15T12:37:46.846Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.847Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: 'result.isErr()', file /builds/worker/workspace/build/src/startupcache/StartupCache.cpp, line 173
[task 2019-10-15T12:37:46.847Z] 12:37:46 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-10-15T12:37:46.847Z] 12:37:46 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2019-10-15T12:37:46.847Z] 12:37:46 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-10-15T12:37:46.848Z] 12:37:46 INFO - running event loop
[task 2019-10-15T12:37:46.848Z] 12:37:46 INFO - PID 15852 | [15852, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 381
[task 2019-10-15T12:37:46.849Z] 12:37:46 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2019-10-15T12:37:46.849Z] 12:37:46 INFO - toolkit/components/downloads/test/unit/test_DownloadIntegration.js | Starting test_common_initialize
[task 2019-10-15T12:37:46.850Z] 12:37:46 INFO - (xpcshell/head.js) | test test_common_initialize pending (2)
[task 2019-10-15T12:37:46.850Z] 12:37:46 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2019-10-15T12:37:46.852Z] 12:37:46 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2019-10-15T12:37:46.852Z] 12:37:46 INFO - (xpcshell/head.js) | test test_common_initialize finished (2)
[task 2019-10-15T12:37:46.853Z] 12:37:46 INFO - toolkit/components/downloads/test/unit/test_DownloadIntegration.js | Starting test_getSystemDownloadsDirectory_exists_or_creates
[task 2019-10-15T12:37:46.853Z] 12:37:46 INFO - (xpcshell/head.js) | test test_getSystemDownloadsDirectory_exists_or_creates pending (2)
[task 2019-10-15T12:37:46.854Z] 12:37:46 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8c352c1055e
Exclude DHCP Search Suffixes from TRR r=kershaw

Comment 6

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/e8c352c1055e

Comment 7

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

Attachment: Backout patch

Approval Request Comment
[Feature/Bug causing the regression]:
This bug introduced a data race between threads that read mDNSSuffixDomains by calling IsExcludedFromTRR and the main thread that changes mDNSSuffixDomains in response to the "network:link-status-changed" notification.
This code hasn't gone through extensive QA testing: relevant discussion
[User impact if declined]:
This can potentially cause a crash when network changes occur while a DNS request is in progress.
[Is this code covered by automated tests?]:
Yes, but the unit test that exercise this path are run sequentially.
[Has the fix been verified in Nightly?]:
This is a backout request for beta only.
[Needs manual test from QE? If yes, steps to reproduce]:
No.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
Very low risk.
[Why is the change risky/not risky?]:
This patch only removes code. This code being removed has interaction with other code and is unlikely to cause any regressions.
[String changes made/needed]:

Comment 8

[Pascal Chevrel:pascalc]

Comment on attachment 9110968 [details] [diff] [review]
Backout patch

Approved for beta before RC, thanks.

Comment 9

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

(In reply to Pascal Chevrel:pascalc from comment #8)

Comment on attachment 9110968 [details] [diff] [review]
Backout patch

Approved for beta before RC, thanks.

Hey Pascal, it seems the backout patch wasn't landed.
Was there something I was supposed to do and I missed it?

Comment 10

[Pascal Chevrel:pascalc]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #9)

(In reply to Pascal Chevrel:pascalc from comment #8)

Comment on attachment 9110968 [details] [diff] [review]
Backout patch

Approved for beta before RC, thanks.

Hey Pascal, it seems the backout patch wasn't landed.
Was there something I was supposed to do and I missed it?

71 was not marked as affected, that's why sherifs didn't see it, my fault as well as I didn't notice the flag :(
Pinging you on slack to evaluate our options.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from mozilla-release (Gecko 71): https://hg.mozilla.org/releases/mozilla-release/rev/501aef7fe1d9622236600a7e53843d40d163a123