http: Prevent HTTP listeners from hijacking admin port (fix #7053)#7660
http: Prevent HTTP listeners from hijacking admin port (fix #7053)#7660AyushKashyapII wants to merge 3 commits intocaddyserver:masterfrom
Conversation
|
Firstly, please sign the CLA. Secondly, please ensure the AI disclosure is a bit easier to read since its collapsed in a code block at the moment, suggested format is in the PR template, and; The overall direction makes sense to me but I do not think this is ready to merge yet. My main concern is I also notice this only checks the local admin endpoint via At a minimum I think this needs targeted tests before merge, especially around |
|
Thank you for the review! I really appreciate the feedback on the network overlap edge cases. Regarding the scope of checking only the local admin endpoint: Yes, this was intentional. Remote admin endpoints require explicit, advanced configuration by the user, making accidental overlaps much less likely. The primary issue reported in #7053 stems from typical web configurations accidentally matching the silent default local port (2019). I have added a comment in app.go explicitly stating this scope constraint, and updated the PR description. I’ve pushed a new commit that improves ConflictsWith to properly handle IPv4/IPv6 strictly, various loopback formats (::1, [::1], 127.0.0.1), and catch-all hosts ("", 0.0.0.0, ::). I also added targeted table-driven tests for all of these scenarios in listeners_test.go. |
mholt
left a comment
There was a problem hiding this comment.
Thanks but I'm not sure about this logic.
| // A blank host, 0.0.0.0, or :: means "all interfaces", which conflicts with everything. | ||
| isAny1 := host1 == "" || host1 == "0.0.0.0" || host1 == "::" || host1 == "[::]" | ||
| isAny2 := host2 == "" || host2 == "0.0.0.0" || host2 == "::" || host2 == "[::]" | ||
| if isAny1 || isAny2 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I don't think this is always true. Or ever? Kernels have some logic for same port, different interfaces. You can bind a port on the everything interface and to a specific interface and it will work in both cases IIRC.
| name: "Catch-all (empty) vs localhost", | ||
| addr1: NetworkAddress{Network: "tcp", Host: "", StartPort: 2019, EndPort: 2019}, | ||
| addr2: NetworkAddress{Network: "tcp", Host: "localhost", StartPort: 2019, EndPort: 2019}, | ||
| want: true, | ||
| }, |
There was a problem hiding this comment.
I think this is actually allowed though. You should be able to serve a public and private port 2019.
|
Great catch @mholt! You are completely right. I was conflating logical overlap with how OS kernels actually handle socket binding hierarchies. A wildcard listener and a specific localhost listener can perfectly co-exist, so my previous logic was over-blocking valid configurations.
|
|
Isn't this just an equality check at this point? |
|
It’s conceptually an equality check. |
Fixes #7053
Description
Currently, if a user mistakenly configures a site to listen on the same port as the Admin API (e.g.,
:2019), the HTTP app can silently hijack the port on systems that allow socket sharing (Linux/macOS), resulting in a confusing404 Not Foundwhen accessing the admin endpoint. On Windows, it throws a raw OS bind error.This PR introduces a deterministic, configuration-time check to catch this conflict before sockets are opened, failing fast with a human-readable error.
What changed
LocalAdminAddress() (NetworkAddress, bool, error)tocaddy.Context. This allows modules to safely resolve the effective local admin address (accounting for defaults, overrides, or if it is disabled) without leaking internal config structs.ConflictsWith(other NetworkAddress) boolmethod to accurately check overlaps. This handles:StartPort <= other.EndPort && EndPort >= other.StartPort).tcpvsudp, and strictly separating IPv4 networks liketcp4from IPv6 networks liketcp6).localhost,127.0.0.1,::1, and[::1]as equivalent loopbacks, and catching"",0.0.0.0,::, and[::]as all-interface overlaps).modules/caddyhttp/app.go(Provision()), each listener is now checked against the admin address after placeholder replacements (ReplaceOrErr) are processed.Design Decisions & Reasoning
Provision()instead ofreplaceLocalAdminServer?Provisionis where listener strings are normalized and placeholders are replaced. It is the earliest point where all HTTP app listeners are available in their structured, final form. Validating here prevents the server from attempting to start at all.Context.LocalAdminAddress()?To maintain strict boundaries.
Contextalready encapsulates state access for modules. Exporting a focused helper keeps coupling low and avoids duplicating "effective admin address" resolution logic across the codebase.Remote admin listeners require explicit, advanced configuration by the user, making accidental overlaps much less likely. The primary issue reported stems from typical web configurations accidentally matching the silent default local port (2019). Checking the local endpoint specifically resolves the default-safety vulnerability.
Listener/admin overlap is a correctness and safety issue. A warning would still allow startup in a compromised state, which could lead to phantom outages in automated deployment environments.
Testing
listeners_test.gocovering port-range overlaps, strict IPv4/IPv6 networking, and loopback/catch-all host normalization.go test ./..., which passes. Tested locally by attempting to start Caddy with the followingCaddyfile:I consulted an AI (LLM) as a mentor to understand Caddy's architecture and to help refine edge cases (like port range math and host normalization). I authored the code, drove the implementation, and verified its correctness through local testing.