Skip to content

webserver: serve every configured TLS port, and DoH behind a reverse proxy - #3017

Open
DL6ER wants to merge 1 commit into
developmentfrom
new/webserver_tls_ports
Open

webserver: serve every configured TLS port, and DoH behind a reverse proxy#3017
DL6ER wants to merge 1 commit into
developmentfrom
new/webserver_tls_ports

Conversation

@DL6ER

@DL6ER DL6ER commented Aug 9, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

Three gaps in how the front TLS terminator exposes ports, the first raised in review of the terminator work.

1. Only the first TLS port was served. split_terminator_ports() captured the port under if(tls_port == 0), so every later ...s entry in webserver.port was dropped from the list handed to CivetWeb while nothing else picked it up. The port simply vanished, with no diagnostic. "80or,443os,8080,4443s" - the example in webserver.port's own help text - silently lost 4443.

The terminator now takes an array of listeners and its accept thread polls all of them, so every configured TLS port is served and reported in the API port list. Overlapping entries are collapsed first, which matters more than it sounds: the shipped default "80o,443os,[::]:80o,[::]:443os" names one dual-stack socket and then its IPv6 half, so binding both naively yields EADDRINUSE on every start. A bare port therefore supersedes any address-scoped entry for that port.

HTTP/3 stays on the first TLS port. The QUIC listener is a single socket wired to one quic_fd, and giving it N listeners is a much larger change; Alt-Svc names the HTTP/3 port explicitly (h3=":443"), so advertising it from the other TLS ports stays correct rather than misleading.

2. IPv4 and IPv6 halves of a port could not coexist. fill_bind_addr() maps 0.0.0.0 to the v4-mapped ::ffff:0.0.0.0, which accepts IPv4 only, and every socket was bound dual-stack - so "0.0.0.0:443s,[::]:443s" served IPv4 only and there was no way to express the split. IPV6_V6ONLY is now set for an explicit IPv6 literal and cleared otherwise, which lets the two halves coexist as separate sockets.

A side effect is that a configuration naming only a bracketed TLS entry, e.g. "[::]:443s", no longer answers over IPv4 through v4-mapped addresses. That is what webserver.port documents ("[::]:80 will bind to port 80 IPv6 only") and what CivetWeb already does for plaintext ports, so this aligns the terminator with both. Nothing released changes: the terminator only exists in development, so the dual-stack-always behavior was never in a release. The default port list is unaffected either way, as it names both a bare and a bracketed entry.

3. DoH could not sit behind a reverse proxy. Plaintext /dns-query was always refused with 426, so a Pi-hole behind an external TLS terminator (nginx, Traefik, Caddy, HAProxy) could not expose DoH at all. Two settings now cover that deployment:

  • webserver.proxySecret - a shared secret authenticating the proxy. It is handed to CivetWeb as proxy_protocol_secret, so a PROXY protocol v2 header carrying it is believed, and the client address and TLS status it announces replace the transport peer. FTL's own terminator then uses the same secret instead of its per-boot token. Empty by default, in which case the token stays per-boot and internal, exactly as today. Write-only, so it is masked in API output.
  • dns.dohReverseProxy - off by default; serves DoH on such an authenticated connection.

Trust rests on the proxy proving who it is, not on the hop being assumed safe. An unauthenticated plaintext /dns-query is still refused with 426, so enabling this cannot expose cleartext DNS on a port something else can reach - which matters, because the shipped default webserver.port has 80o on all interfaces.

An unauthenticated X-Forwarded-For is deliberately never consulted. It is trivially spoofable, and client identity is load-bearing in FTL: per-client rate limiting (client->rate_limit) and per-client group policy both key on it, so a spoofed header would let any host on that port exhaust a victim's rate-limit budget or select their policy. Because the PROXY v2 address is authenticated instead, queries are attributed to the real client rather than to the proxy - so there is no attribution or rate-limit-sharing caveat.

How to test the change during review

Set a port list, restart, and check both the listener lines in FTL.log and what actually answers:

pihole-FTL --config webserver.port '80o,443os,4443s'
curl -sk -o /dev/null -w '%{http_code} %{http_version}\n' https://127.0.0.1:443/api/info/ftl
curl -sk -o /dev/null -w '%{http_code} %{http_version}\n' https://127.0.0.1:4443/api/info/ftl

Measured here, one FTL start per row:

webserver.port listeners IPv4 IPv6
80o,443os,[::]:80o,[::]:443os (default) *#443 200 200
80o,0.0.0.0:443s,[::]:443s 0.0.0.0#443 + ::#443 200 200
80o,[::]:443s ::#443 - 200
80o,0.0.0.0:443s 0.0.0.0#443 200 -
80o,443os,4443s *#443 + *#4443 200 200

The default row is the important regression check: one dual-stack listener and no ERROR: Terminator: bind() in the log.

DoH behind a proxy needs a real PROXY v2 header, so a plain curl cannot exercise it - point an actual proxy at Pi-hole, or use a small script that prepends the header. With webserver.proxySecret set and dns.dohReverseProxy true, measured here:

request to http://127.0.0.1/dns-query result
PROXY v2 with the correct secret + PP2_TYPE_SSL 200 application/dns-message
no PROXY header (plain cleartext) 426
PROXY v2 with a wrong secret 426
PROXY v2, correct secret, no TLS marker 426

With an off-subnet announced client the request is refused with 403 by the usual dns.listeningMode policy, the same rule a plain DNS client faces. Attribution was confirmed by the query being recorded against the announced client address rather than the proxy.

Automated coverage: the full suite passes (test_suite.bats 191, dotdoh.bats 12, dotdoh_server.bats 25, test_final.bats 9, pytest 149) with only the three known version-string failures that also fail on a clean development build in the same container. test_suite.bats diffs test/pihole.toml against the regenerated config, which pins the new key, and test_openapi.py type-checks it against the updated spec. The build was also checked in the no-nghttp2, no-nghttp3, no-QUIC and TLS-only configurations, since none of those are covered by CI. No automated test binds a second TLS port or exercises plaintext DoH, so the manual steps above are worth running.

Additional information

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. My change does not modify 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 the dnsmasq-discuss mailing list first, we merge them once they are in dnsmasq master.

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repository's development branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@DL6ER
DL6ER force-pushed the new/webserver_tls_ports branch 3 times, most recently from d386dfa to 91d23ef Compare August 9, 2026 09:34
@DL6ER
DL6ER marked this pull request as ready for review August 9, 2026 09:58
@DL6ER
DL6ER requested a review from a team as a code owner August 9, 2026 09:58
Copilot AI lite review requested due to automatic review settings August 9, 2026 09:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates FTL’s embedded webserver stack to (1) correctly serve all configured TLS (“...s”) ports via the in-process TLS terminator, (2) allow explicit IPv4/IPv6 scoping for TLS listeners (supporting separate v4 and v6 sockets on the same port), and (3) optionally serve plaintext DoH behind a trusted external reverse proxy via a new dns.dohReverseProxy setting.

Changes:

  • Extend the TLS terminator to accept and poll multiple TLS listener sockets, while keeping HTTP/3 on the first TLS port.
  • Add optional plaintext DoH handling in CivetWeb when dns.dohReverseProxy=true, with existing 426 behavior unchanged by default.
  • Wire the new config key through config structs, test config fixtures, and the OpenAPI config schema.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/pihole.toml Adds dns.dohReverseProxy fixture entry and updates stats counts.
src/webserver/webserver.c Implements plaintext DoH handler behind reverse proxy; parses and forwards multiple TLS terminator listeners; reports additional TLS ports.
src/webserver/terminator.h Introduces terminator_listener and multi-listener terminator_start() API.
src/webserver/terminator.c Implements multi-listener binding + poll-based accept loop; adjusts IPv6-only binding behavior for explicit v6 literals.
src/config/config.h Adds dns.dohReverseProxy to the config struct.
src/config/config.c Registers dns.dohReverseProxy option (bool, default false) with help text.
src/api/docs/content/specs/config.yaml Adds dohReverseProxy to the OpenAPI config schema and example.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/webserver/webserver.c Outdated
Comment thread src/webserver/webserver.c
Comment thread src/webserver/webserver.c
Comment thread src/webserver/terminator.c
@DL6ER
DL6ER marked this pull request as draft August 9, 2026 10:13
@DL6ER
DL6ER force-pushed the new/webserver_tls_ports branch 2 times, most recently from 46a770d to afaf417 Compare August 9, 2026 11:45
@DL6ER
DL6ER marked this pull request as ready for review August 9, 2026 12:35
@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please resolve those before we can evaluate the pull request.

…proxy

Three gaps in how the front terminator exposes TLS:

1. `split_terminator_ports()` only ever captured the *first* secure entry of
   `webserver.port`. Every further "...s" entry was still dropped from the list
   handed to CivetWeb, so nothing served it and nothing reported it. A
   documented configuration such as "80or,443os,8080,4443s" - the example in the
   `webserver.port` help text itself - silently lost 4443.

   The terminator now takes an array of listeners and its accept thread polls
   all of them, so every configured TLS port is served and reported in the API
   port list. Entries that would bind the same socket are collapsed first: the
   default "443os,[::]:443os" names one dual-stack socket and then its IPv6
   half, and binding both is simply `EADDRINUSE`. A port outside 1-65535 is
   rejected here, as a secure entry never reaches CivetWeb's own syntax check.
   HTTP/3 stays on one port, the first that actually bound - Alt-Svc names the
   HTTP/3 port explicitly, so advertising it from the others remains correct.

2. The IPv4 and IPv6 halves of one port could not coexist. `fill_bind_addr()`
   maps "0.0.0.0" to the v4-mapped `::ffff:0.0.0.0`, which accepts IPv4 only,
   while every socket was bound dual-stack - so "0.0.0.0:443s,[::]:443s" served
   IPv4 only, with no way to express the split. `IPV6_V6ONLY` is now set for an
   explicit IPv6 literal and cleared otherwise, so the two halves bind as
   separate sockets.

   A configuration naming *only* a bracketed TLS entry, e.g. "[::]:443s", no
   longer answers over IPv4 through v4-mapped addresses. That is what
   `webserver.port` documents ("[::]:80" is IPv6 only) and what CivetWeb does
   for plaintext ports, and the terminator has never been in a release, so no
   released behavior changes. The default port list is unaffected, as it names
   both a bare and a bracketed entry.

3. Plaintext `/dns-query` was always refused with 426, leaving no way to put
   Pi-hole's DoH behind an external TLS-terminating reverse proxy. Two new
   settings cover that deployment:

   - `webserver.proxySecret` is a shared secret authenticating such a proxy. It
     is passed to CivetWeb as `proxy_protocol_secret`, so a PROXY protocol v2
     header carrying it is believed and the client address and TLS status it
     announces replace the transport peer. Our own terminator uses the same
     secret instead of its per-boot token. Empty by default, in which case the
     token stays per-boot and internal. Installing it does not depend on a local
     TLS port, as the deployment it serves has none.
   - `dns.dohReverseProxy` (off by default) serves DoH on such a connection.

   Trust therefore rests on the proxy authenticating itself, not on the hop
   being assumed safe: an unauthenticated plaintext `/dns-query` is still
   refused with 426, so enabling this cannot expose cleartext DNS on a port
   reachable by anything else. An unauthenticated `X-Forwarded-For` is never
   consulted, as it is trivially spoofable and would let any host on that port
   attribute its queries to a victim, taking over their rate-limit budget and
   group policy. Because the announced address is authenticated, queries are
   attributed to the real client rather than to the proxy.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
DL6ER force-pushed the new/webserver_tls_ports branch from afaf417 to d08abe2 Compare August 21, 2026 19:23
@github-actions

Copy link
Copy Markdown

Conflicts have been resolved.

@github-actions

Copy link
Copy Markdown

Conflicts have been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants