home: add IPv6 support for encrypted listeners - #8490
Conversation
21a45a0 to
48e9c9d
Compare
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
There is a bind-scope compatibility issue in internal/home/web.go: getBindAddr maps both 0.0.0.0 and :: to :port, and that result is used for HTTP, HTTPS, and HTTP/3. Existing installations commonly persist an explicit http.address: 0.0.0.0:3000; with Go wildcard listeners, :port becomes dual-stack where the platform supports IPv4-mapped IPv6. An upgrade can therefore make an explicitly IPv4-only admin/DoH listener accept IPv6 connections, broadening exposure beyond the configured address. Issue #8363 requires a dual-stack option/default, but does not require changing the meaning of an explicit IPv4 wildcard. Please preserve the configured address family and handle the new/default dual-stack case separately (using explicit listener-family semantics where needed). A regression test should start a real listener on an IPv6-capable host and verify that 0.0.0.0 remains IPv4-only while the intended dual-stack/default case accepts both families.
822b00d to
d22b1e2
Compare
Done, force-pushed. 0.0.0.0 now binds with tcp4/udp4 so it really stays IPv4-only, and only :: gets the dual-stack :port form. Had to create the listeners manually since ListenAndServe hardcodes "tcp". Added a test that binds real listeners and dials them over both families to check 0.0.0.0 rejects IPv6 and :: accepts both. While testing I noticed [0.0.0.0, ::] in bind_hosts breaks dnsproxy with EADDRINUSE, so I kept the DNS default at 0.0.0.0. |
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
Thanks for preserving explicit 0.0.0.0 as IPv4-only. I re-reviewed exact
head d22b1e23922a8c2249b38d999f824d6f36c98a12; the family mapping and real
TCP listener test now address my earlier bind-scope concern. One HTTP/3
socket-lifecycle blocker remains.
internal/home/web.go:563-565 creates a net.PacketConn and passes it to
http3.Server.Serve, but no code closes that caller-owned connection. In
quic-go v0.60.0, http3.Server.Serve explicitly documents that closing the
server does not close a connection supplied by the caller
(http3/server.go:208-211).
Exact trigger: enable HTTP/3, then reconfigure TLS so shutdownSrv3 closes the
HTTP/3 server and the service starts again on the same UDP address. Serve
returns, but the old UDP socket remains bound. The next net.ListenPacket
fails with EADDRINUSE; mustStartHTTP3 then panics and its recovery path exits
AdGuard Home. An early Serve error leaks the socket for the same reason.
I reproduced the ownership behavior independently on loopback:
rebind after Server.Close: bind: address already in use
rebind after PacketConn.Close: success
The smallest fix is to close the owned PacketConn on every return path after
the successful net.ListenPacket call, preferably with the repository's
close-and-log helper, before entering http3.Server.Serve.
Please add a regression that starts the HTTP/3 helper with an owned UDP
connection, closes the server, waits for Serve to return, and then proves the
same address can be rebound. Repeating an actual TLS/HTTP3 reconfiguration on
the same port would provide even stronger end-to-end coverage.
Validation performed on the clean exact head:
PASS: focused bind-family tests under -race
PASS: go test -race -count=1 ./internal/home
PASS: make go-check (including full race suite and govulncheck)
PASS: make go-os-check
PASS: git diff --check
EXPECTED FAIL: HTTP/3 close/rebind reproducer until PacketConn.Close
One coverage note: TestNewServerConfig_Issue8363BindHosts also passes when
transplanted unchanged onto the base commit, so it does not prove a listener
regression for #8363. The current real family test covers TCP/web only. It
would be useful to add actual IPv4/IPv6 dials for every DoT/DoQ/HTTPS/DoH
protocol claimed by the PR, especially UDP/HTTP3, and to clarify whether the
issue's existing-installation case is expected to require changing persisted
http.address: 0.0.0.0 to ::. I am treating that as a coverage/design note;
the unclosed HTTP/3 socket above is the confirmed blocker.
bbc4b2f to
fb2173d
Compare
Thanks for the detailed review. The HTTP/3 socket-lifecycle blocker is fixed The listen-and-serve part of Regression coverage: Validation on the new head: On the coverage/design note: you are right that the current real-connection |
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
I re-reviewed exact head fb2173d5a8063da8cdccc4896c763404d1da61bf.
The requested HTTP/3 socket-lifecycle fix is complete: after a successful net.ListenPacket, serveHTTP3 now owns the connection and defers slogutil.CloseAndLog, covering normal shutdown and every error or panic return path. The new regression waits for the UDP listener, closes the HTTP/3 server, waits for Serve to return, and proves the same address can be rebound.
Independent validation on the clean exact head:
PASS: TestServeHTTP3_connClose, 20 times under -race
PASS: go test -race -count=1 ./internal/home
PASS: make go-check
PASS: make go-os-check
PASS: git diff --check
The current build and lint workflow runs show action_required with zero jobs, which is the upstream fork-approval gate rather than a test failure.
The additional per-protocol IPv4/IPv6 dial coverage and migration documentation remain useful follow-up or maintainer-choice improvements, but they are not blockers for the confirmed socket leak. My requested change is addressed.
|
Thanks for the re-review. I'll leave the dial coverage and migration note as |
fb2173d to
0aae4a8
Compare
|
Rebased onto master to fix the conflicts from the recent TLS refactor. |
Fixes #8363
DoH, DoT, DoQ, and the HTTPS admin panel could bind only to IPv4 when using
default or unspecified addresses.
What changed
semantics: an explicitly configured
0.0.0.0stays IPv4-only(
tcp4/udp4), while::binds dual-stack via:port, so existingIPv4-wildcard configs keep their meaning after upgrade
[::]:3000, enabling dual-stack bydefault
::1;pproflistens on both127.0.0.1and[::1]bind_hostsdefault is left as0.0.0.0: adding::alongside itcauses
EADDRINUSE, since Go wildcard listeners on0.0.0.0are alreadydual-stack where the platform supports IPv4-mapped IPv6
net.PacketConnon everyreturn path (new
serveHTTP3helper usingslogutil.CloseAndLog), sincehttp3.Server.Servedoes not close caller-provided connections; previouslya TLS reconfiguration leaked the UDP socket and the restart failed with
EADDRINUSE, exiting AdGuard HomeTesting
TestGetBindAddr_families, a regression test that starts reallisteners and verifies via actual connections that
0.0.0.0remainsIPv4-only while
::accepts both familiesTestGetBindAddrfor the network/address mapping (TCP and UDP)TestServeHTTP3_connClose, a regression test that starts the HTTP/3serving helper with an owned UDP connection, closes the server, waits for
Serveto return, and verifies the same address can be rebound; it failswith
EADDRINUSEwithout the connection closeTestNewServerConfig_DefaultHostsandTestNewServerConfig_Issue8363BindHostsmake go-checkpasses