Skip to content

http: Prevent HTTP listeners from hijacking admin port (fix #7053)#7660

Open
AyushKashyapII wants to merge 3 commits intocaddyserver:masterfrom
AyushKashyapII:master
Open

http: Prevent HTTP listeners from hijacking admin port (fix #7053)#7660
AyushKashyapII wants to merge 3 commits intocaddyserver:masterfrom
AyushKashyapII:master

Conversation

@AyushKashyapII
Copy link
Copy Markdown

@AyushKashyapII AyushKashyapII commented Apr 20, 2026

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 confusing 404 Not Found when 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

  1. Context Helper: Added LocalAdminAddress() (NetworkAddress, bool, error) to caddy.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.
  2. Conflict Detection: Added a ConflictsWith(other NetworkAddress) bool method to accurately check overlaps. This handles:
    • Port range math (StartPort <= other.EndPort && EndPort >= other.StartPort).
    • Strict network protocols (e.g., isolating tcp vs udp, and strictly separating IPv4 networks like tcp4 from IPv6 networks like tcp6).
    • Host normalization (treating localhost, 127.0.0.1, ::1, and [::1] as equivalent loopbacks, and catching "", 0.0.0.0, ::, and [::] as all-interface overlaps).
  3. HTTP Provisioning Hook: In modules/caddyhttp/app.go (Provision()), each listener is now checked against the admin address after placeholder replacements (ReplaceOrErr) are processed.

Design Decisions & Reasoning

  • Why check in Provision() instead of replaceLocalAdminServer?
    Provision is 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.
  • Why use Context.LocalAdminAddress()?
    To maintain strict boundaries. Context already encapsulates state access for modules. Exporting a focused helper keeps coupling low and avoids duplicating "effective admin address" resolution logic across the codebase.
  • Why only check the local admin endpoint?
    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.
  • Why a hard error instead of a warning?
    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

  • Unit Tests: Added targeted, table-driven unit tests in listeners_test.go covering port-range overlaps, strict IPv4/IPv6 networking, and loopback/catch-all host normalization.
  • Integration Test: Ran go test ./..., which passes. Tested locally by attempting to start Caddy with the following Caddyfile:
localhost:2019 {
    respond "I stole the admin port!"
}

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.

@steadytao
Copy link
Copy Markdown
Member

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 ConflictsWith(). For a correctness check like this, the current host/network overlap logic seems too naive. For example, it only normalises localhost to 127.0.0.1 so localhost:2019 would not conflict with [::1]:2019, and it treats tcp, tcp4, and tcp6 as equivalent by prefix without really modelling the actual bind semantics.

I also notice this only checks the local admin endpoint via LocalAdminAddress(), not any configured remote admin listener. That may be intentional but if so I think the scope should be stated more explicitly.

At a minimum I think this needs targeted tests before merge, especially around localhost / 127.0.0.1 / ::1, empty host handling and port-range overlap.

@AyushKashyapII
Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks but I'm not sure about this logic.

Comment thread listeners.go Outdated
Comment on lines +90 to +95
// 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
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread listeners_test.go
Comment on lines +694 to +698
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,
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is actually allowed though. You should be able to serve a public and private port 2019.

@AyushKashyapII
Copy link
Copy Markdown
Author

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.
I have pushed an update that corrects this:

  • A wildcard/catch-all now ONLY conflicts with another wildcard.
  • A loopback ONLY conflicts with another loopback.
  • I've updated the tests to assert that 0.0.0.0:2019 vs localhost:2019 is allowed, while 0.0.0.0 vs "" correctly throws a conflict error.

@mholt
Copy link
Copy Markdown
Member

mholt commented Apr 23, 2026

Isn't this just an equality check at this point?

@AyushKashyapII
Copy link
Copy Markdown
Author

It’s conceptually an equality check.
A raw host1 == host2 check would miss conflicts where different strings map to the same kernel listener slot (for example, localhost vs 127.0.0.1 or 0.0.0.0 vs an empty string).
By grouping them into 'Catch-all' and 'Loopback' categories, we ensure that these different-but-equivalent strings are correctly identified as conflicts while still allowing the specific-vs-wildcard co-existence.

@AyushKashyapII AyushKashyapII requested a review from mholt April 25, 2026 11:12
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.

Confusing behaviour when starting a server on the admin port

3 participants