config: add dns.hostsLocal to keep custom DNS records local - #3037
Conversation
|
I think this is a sensible change. Do you think we should expose this setting via UI at the 'Local DNS records' page? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Since dnsmasq 2.86, a query for a record type that has no local answer is forwarded upstream even when the name itself is defined locally. For a custom DNS record under a public domain that means the `AAAA` query `getaddrinfo()` sends alongside the `A` query comes back with the *public* address, which glibc then prefers - so the override silently does not override. `dig` only ever sends `A` and looks correct, which is what makes this so hard to spot. We now emit one `local=/<name>/` line per hostname in `dns.hosts`, which tells dnsmasq that the name is ours and stops the forwarding. This is scoped to the configured names and their subdomains, so everything else in the same zone keeps resolving from upstream as before. Set `dns.hostsLocal = false` for the previous behavior. Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Conflicts have been resolved. |
1 similar comment
|
Conflicts have been resolved. |
There was a problem hiding this comment.
Pull request overview
Adds the default-enabled dns.hostsLocal option to keep configured custom DNS hostnames and subdomains local.
Changes:
- Generates dnsmasq
local=directives. - Adds configuration, API documentation, and test fixture support.
- Adds tests for generated directives.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Summary |
|---|---|
test/test_suite.bats |
Tests generated local rules; coverage should include enabled/disabled behavior and runtime updates. |
test/pihole.toml |
Adds the new test configuration entry. |
src/config/dnsmasq_config.c |
Generates local DNS rules; host mutations must regenerate or reload these rules. |
src/config/config.h |
Adds the configuration field. |
src/config/config.c |
Registers the new setting and default. |
src/api/docs/content/specs/config.yaml |
Documents the API property. |
Suppressed comments (2)
src/config/dnsmasq_config.c:838
- When
dns.expandHostsis enabled with a non-emptydns.domain.name, dnsmasq creates a second hosts entry such asfoo.lanfor a dotlessfoo(src/dnsmasq/cache.c:1513-1525).local=/foo/does not matchfoo.lan, so queries such as AAAA or HTTPS for the expanded name can still be forwarded, leaving the override vulnerable on the exact path this setting is meant to close. Emit a local rule for each expanded name as well, taking the effective expansion suffix into account.
fprintf(pihole_conf, "local=/%s/\n", name);
src/config/dnsmasq_config.c:838
- The
dns.hostsvalidator permits a final label longer than 63 characters because it only checks label length when it encounters a dot, while dnsmasq'scanonicalise()rejects labels overMAXLABEL(63). Thus an otherwise accepted entry such as1.2.3.4 <64-character-label>now emits an invalidlocal=directive, causing generated-config validation or a later restart to fail. Validate/canonicalize the hostname before emitting it, or fix the host validation path.
fprintf(pihole_conf, "local=/%s/\n", name);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // for a type we have no local record for (e.g., AAAA when only an A | ||
| // address is defined) is forwarded and a public answer for the same | ||
| // name shadows the local address | ||
| if(conf->dns.hostsLocal.v.b && cJSON_GetArraySize(conf->dns.hosts.v.json) > 0) |
| @test "Custom DNS records are treated as local (dns.hostsLocal)" { | ||
| # One local= per hostname configured in dns.hosts, so a type we have no | ||
| # local record for is not forwarded and answered upstream instead | ||
| run bash -c 'grep -c "^local=/abc-custom.com/$" /etc/pihole/dnsmasq.conf' | ||
| assert_line --index 0 "1" | ||
| run bash -c 'grep -c "^local=/def-custom.de/$" /etc/pihole/dnsmasq.conf' | ||
| assert_line --index 0 "1" |
The `local=` lines come from `write_dnsmasq_config()`, but `dns.hosts` is not a dnsmasq option, so a record added through the API or the CLI kept being forwarded until an unrelated restart, and a deleted one kept its rule. Both API paths, the CLI and the `pihole.toml` re-read rewrite the config now. dnsmasq reads `local=` at startup only, so this restarts the resolver; `dns.hostsLocal = false` keeps the old behavior. While here: the CLI wrote `custom.list` from the old records, before the new config was installed. The changed test counters follow the two new queries and that restart, which rebuilds the domain table from the database. Signed-off-by: DL6ER <dl6er@dl6er.de>
`valid_domain()` measured a label when it found the terminating dot, so the last one was never checked and `1.2.3.4 test.<64 characters>` passed. dnsmasq rejects labels above 63 characters, so with `dns.hostsLocal` such an entry produced a `local=` line that makes the resolver refuse to start. Signed-off-by: DL6ER <dl6er@dl6er.de>
What does this implement/fix?
A custom DNS record only ever defines an address. Any other record type for that name -
AAAAwhen you configured anA, or theHTTPSquery browsers send - has no local answer, so dnsmasq forwards it upstream. When the name also exists publicly, the public answer comes back andgetaddrinfo()prefers theAAAAover our localA. The override silently stops overriding, and only for some clients:digsendsAalone and looks perfectly fine, which is what makes this so hard to diagnose.We now write one
local=/<name>/line per hostname indns.hosts, telling dnsmasq the name is ours and that nothing about it should leave the box. The scope is the configured names and their subdomains, so split DNS keeps working - withsub1.example.comandsub2.example.comdefined,sub3.example.comandexample.comitself still resolve from upstream as before.The new
dns.hostsLocalis on by default. What it changes is a name you have explicitly claimed, and the old behavior is one setting away, whereas the failure it prevents is silent and client-dependent. Setdns.hostsLocal = falseto get it back.How to test the change during review
Point a custom DNS record at a name that also exists publicly with an
AAAArecord, e.g.,dns.hosts = [ "10.0.0.1 sub1.example.com" ], then from a client:grep '^local=' /etc/pihole/dnsmasq.confshows one line per configured hostname, andpihole-FTL --config dns.hostsLocal falserestores the previous behavior.Automated coverage:
test/test_suite.batsasserts the generatedlocal=lines ("Custom DNS records are treated as local"), andtest/pihole.tomlcarries the new key so the template comparison and the config-write accounting intest_final.batsstay exact.Additional information
Related issue or feature (if applicable): #2841
Pull request in docs with documentation (if applicable): N/A
By submitting this pull request, I confirm the following:
git rebase)src/dnsmasq/. That tree is a verbatim copy of upstream dnsmasq and we do not carry anything in it that deviates from upstream. Fixes have to go through thednsmasq-discussmailing list first, we merge them once they are in dnsmasq master.Checklist:
developmentbranch.