feat(serve): support multiple --host arguments for dual-stack binding - #73849
feat(serve): support multiple --host arguments for dual-stack binding#73849alrcatraz wants to merge 6 commits into
Conversation
972608e to
3528249
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for pursuing dual-stack binding; current main still accepts only one --host (hermes_cli/subcommands/dashboard.py:30), so the feature addresses a real gap.
Problems
- Blocking security: the new
app.state.auth_required = should_require_auth(_primary)inhermes_cli/web_server.py:20185evaluates only the first host.--host 127.0.0.1 --host 0.0.0.0would expose a non-loopback listener without the auth gate, contrary to the current contract inhermes_cli/web_server.py:442-461. - The new single
app.state.bound_host = _primarydoes not cover additional listeners. Existing HTTP and WS Host/Origin checks consume one host athermes_cli/web_server.py:464-505and14349-14372. _create_server_sockets()binds each host independently with the supplied port. For--port 0, this can allocate different ports while startup reports only one.start_server(host=...)callers remain intests/test_web_server.py:75-76andtests/hermes_cli/test_dashboard_auth_gate.py:130-131; this PR removes that keyword and adds no tests.
Suggested changes
- Model auth and bound-host validation over the complete host set, including interactive setup and WS checks.
- Make port-0 select one port then reuse it for every listener.
- Preserve/update
host=callers and add behavioral coverage for repeated hosts, host-order-independent auth, Host/Origin/WS checks, and shared port-0 binding.
Automated hermes-sweeper review.
| @@ -20152,19 +20185,21 @@ def start_server( | |||
| # injection / WS-auth paths can branch on it consistently. Phase 3.5 | |||
There was a problem hiding this comment.
_primary is insufficient for the auth decision. With --host 127.0.0.1 --host 0.0.0.0, this sets auth_required=False even though the second listener is public. The current security contract requires auth for every non-loopback bind; compute this from all requested hosts before binding.
There was a problem hiding this comment.
Fixed in 6322b052e. Changed to any(should_require_auth(h) for h in hosts) so auth is required when any bound host is non-loopback. --host 127.0.0.1 --host 0.0.0.0 now correctly evaluates to True.
| ", ".join(p.name for p in list_providers()), | ||
| ) | ||
|
|
||
| # Record the bound host so host_header_middleware can validate incoming | ||
| # Host headers against it. Defends against DNS rebinding (GHSA-ppp5-vxwm-4cf7). | ||
| app.state.bound_host = host | ||
| app.state.bound_host = _primary |
There was a problem hiding this comment.
The existing HTTP and WebSocket Host/Origin guards read one app.state.bound_host. Storing only _primary means the second explicit listener is not accepted by that model. Store and validate the complete normalized bind set before enabling multiple listeners.
There was a problem hiding this comment.
Fixed in d74dfccb0. Now stores app.state.bound_hosts = frozenset(hosts) alongside the legacy bound_host for backward compat. host_header_middleware, CORS origin check, _ws_host_origin_reason, and the WS client/auth guards all iterate the full set (_is_accepted_host now accepts frozenset[str]).
Allow passing `--host` multiple times (e.g. `--host 0.0.0.0 --host ::`) to bind both IPv4 and IPv6 on the same port. Each address gets its own pre-bound TCP socket with SO_REUSEADDR; IPv6 sockets get IPV6_V6ONLY=1 so they coexist with the v4 listener. Internally start_server() accepts a `hosts: list[str]` parameter and pre-creates sockets via the new _create_server_sockets() helper, passing them to uvicorn.Server.startup(sockets=...).
- Store all requested --host values in app.state.bound_hosts alongside the legacy bound_host for backward compatibility. - Update _is_accepted_host to accept frozenset[str] and iterate over all hosts. - Fix IPv6 host_only parsing (::1 vs ::1:9119 edge case with port detection). - Update host_header_middleware, CORS origin check, and WS host/origin guard to validate against the full set of bound hosts. - Ensures DNS-rebinding protection works when multiple --host values are specified (e.g. IPv4 + IPv6 dual-stack).
- Previously only _primary (the first --host argument) was checked. With --host 127.0.0.1 --host 0.0.0.0, auth_required was incorrectly False despite the second listener being public. - Change to any(should_require_auth(h) for h in hosts): if ANY bound interface is non-loopback, auth is required.
- When port=0, bind the first socket to an ephemeral port, read the OS-assigned port number, and reuse it for all remaining listeners. - Guarantees a single port number for all --host bindings instead of getting N different ephemeral ports.
- Update _ws_client_reason, _ws_client_is_allowed, and _ws_auth_mode to use bound_hosts (frozenset) with bound_host fallback for backward compat. - These functions determine whether the bind is loopback-only for WS peer restrictions; without this, a dual-stack bind with multiple --host args could leave the WS path unprotected.
…tion - Update start_server(host=...) to start_server(hosts=[...]) in all test calls. - Update frozenset signature in host header validator tests. - Add auth_required=False in WS tests to prevent cross-test pollution from dashboard_auth_gate tests that set auth_required=True. - Add dual-stack loopback and mixed loopback+public test scenarios.
3528249 to
5adb279
Compare
Thanks for the detailed review! All four issues are addressed in the follow-up commits on feat/dual-stack-serve:
Commits are grouped by the fix they implement rather than by review comment. I'm happy to adjust anything else! |
Allow passing
--hostmultiple times (e.g.--host 0.0.0.0 --host ::)to bind both IPv4 and IPv6 on the same port. Each address gets its own
pre-bound TCP socket with
SO_REUSEADDR; IPv6 sockets getIPV6_V6ONLY=1so they coexist with the v4 listener.
Changes
hermes_cli/subcommands/dashboard.py:--hostchanged from single-value toaction="append"hermes_cli/main.py: Allargs.hostusages adapted to list semantics; re-exec generates multiple--hostargshermes_cli/web_server.py: New_create_server_sockets()helper;start_server(hosts=…)with pre-bound socket list passed touvicorn.Server.startup(sockets=…)Verification
_all_loopbackhosts=Nonedefaults to["127.0.0.1"]Closes #73785