feat(server): close DNS-rebinding gap with Host validation middleware#361
feat(server): close DNS-rebinding gap with Host validation middleware#361cpcloud wants to merge 15 commits into
Conversation
roborev: Combined Review (
|
mariusvniekerk
force-pushed
the
feat/host-origin-middleware
branch
from
a17deda to
f1c53d7
Compare
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
mariusvniekerk
force-pushed
the
feat/host-origin-middleware
branch
from
c236b7b to
d3c90c9
Compare
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
middleman validates that the configured bind is loopback and gates mutating /api/v1 calls on Sec-Fetch-Site, but the TCP listener accepts requests regardless of Host. A page that resolves its hostname to 127.0.0.1 after the initial fetch (DNS rebinding) sends every fetch as "same-origin" so checkCSRF passes; the request lands on the loopback listener and is processed. This spec describes the Host validation middleware (parser shared with config.allowed_hosts via internal/config/hostmatch.go) and the trust_reverse_proxy flag that opt-in honors X-Forwarded-Host / Forwarded under proxied deployments. Both Backend Host and Public Host must independently pass the allowlist gate, so a DNS-rebound request spoofing X-Forwarded-Host is still rejected at Step 2 on the raw Host.
Tasks 1-7 cover: shared ParseHostKey parser in internal/config, two new config fields, the host_check.go middleware, ServeHTTP wiring with a strict precedence rule (caller override > validated cfg > cfg=nil test default > panic on partial cfg), wire-level tests, full-stack apitest coverage, and README documentation. The cfg=nil test-friendly default keeps the dozens of pre-existing internal/server test helpers working without per-test churn, while the two known partial-cfg sites get targeted ServerOptions.HostCheck overrides so the production contract stays strict.
Introduces a canonical HostKey type (lowercased host, port preserved or empty, IPv6 bracketed) plus a ParseHostKey parser shared between config validation (allowed_hosts entries) and the upcoming Host validation middleware. Centralising the parser ensures the "canonicalisation is identical at config-load time and request time" invariant holds without duplication. Rejects empty input, port-only values, unbracketed IPv6 literals, and ports outside 1-65535.
Adds two config fields to support the upcoming Host validation middleware. allowed_hosts is parsed via the shared ParseHostKey and cached on the Config struct so the server constructor can derive the runtime allowlist without re-parsing on each request setup. trust_reverse_proxy opt-in flag governs whether X-Forwarded-Host / Forwarded RFC 7239 host= are consulted. Validate() also caches the canonical bind HostKey so the server constructor can read it directly via BindHostKey(); the field is left zero for partial test configs that bypass Validate. Validate() itself stays side-effect-light; the trust_reverse_proxy warning and the cfg=nil test-friendly default warning are emitted at server construction time (next commit).
Introduces checkHost plus the supporting parsers for X-Forwarded-Host and Forwarded RFC 7239 host=, gated on a HostCheckOptions value. Three validation steps mirror the design spec: parse Backend Host, match Backend against bind + loopback synonyms + allowed_hosts, and (when trust_reverse_proxy is true) validate the Public Host from forwarded headers against the same accept-set with conflict detection across X-Forwarded-Host and Forwarded. Wiring into Server.ServeHTTP happens in the next commit so this diff can be reviewed in isolation.
Wires the Host validation middleware into ServeHTTP before the existing CSRF gate. A DNS-rebound page that resolves attacker.example to 127.0.0.1 was previously seen as same-origin by the browser, slipping past Sec-Fetch-Site; this commit makes the server reject any request whose raw Host (and, under trust_reverse_proxy, forwarded host) does not match the bind, loopback synonyms at the bind port, or an allowed_hosts entry. HostCheckOptions gains an AllowLoopbackAnyPort flag that the test-friendly fallback (cfg=nil or partial cfg) and any go-test process turn on automatically. httptest.NewServer binds an ephemeral port that callers cannot pre-declare in allowed_hosts, so test binaries accept literal loopback IPs at any port; testing.Testing() is false in production so the strict spec semantics are preserved on the deployed binary. The doJSON helper sets req.Host so partial-cfg tests with a validated 127.0.0.1:8091 bind also pass cleanly.
Adds table-driven wire-level tests that exercise every row of the spec's decision matrix: bind / loopback synonyms / allowed_hosts (Step 2), forwarded headers under trust_reverse_proxy (Step 3), and the 403 body shape. Each row constructs a Server with explicit HostCheckOptions so the test-friendly relaxation does not mask production behavior. Direct tests on the unexported parseForwardedHost and parseXForwardedHost helpers cover the malformed-but-present cases that the round-trip path cannot otherwise hand to the middleware.
Adds a full-stack apitest case that exercises the production contract: an explicit HostCheckOptions override skips the test-friendly AllowLoopbackAnyPort relaxation, so the middleware behaves exactly as it would on a deployed binary. A request with Host attacker.example:8091 returns the documented JSON 403 body before any handler runs; a request whose Host matches the configured bind reaches the handler and returns 200 with the seeded PR list.
Resolve PR review findings by removing the production testing dependency from server host-check setup and deriving host validation options from non-nil config values when Validate has not populated cached keys. Keep the legacy partial-config fallback behind test-only wiring and add full-stack trusted proxy allowlist coverage for forwarded hosts.
The host-check repair made validated configs enforce their configured bind. Workspace API tests use a middleman.test apiclient base URL and, in some cases, httptest listeners on ephemeral loopback ports, so make that fixture opt in explicitly instead of relying on production test detection.
Workspace diff tests issue direct httptest requests against the full server fixture. After host validation became strict, those requests kept httptest's default example.com host and hit the middleware instead of the workspace handlers. Route them through the fixture's accepted test host so the tests exercise the intended API behavior.
Address roborev feedback by rejecting port 0 during config validation instead of letting Host validation fail later with an indirect parse error. Add full-stack API coverage that loads allowed_hosts and trust_reverse_proxy from TOML so the config-derived host-check path is exercised.
The SSE replay e2e helper builds a config-backed server but runs it through httptest.NewServer, so requests arrive with a random loopback port. Opt the helper into the explicit loopback-any-port test relaxation used by the other e2e fixtures so host validation does not block the stream before frames are emitted.
Roborev flagged the SSE replay fixture for relying on config-derived host-check setup before the loopback test relaxation. Give the helper an explicit HostCheck override so httptest random ports are accepted without depending on partial config derivation.
Rebasing onto the current main branch brought in the go.kenn.io/middleman module path. The host check files were still using the old github.com/wesm/middleman imports, which prevented the rebased branch from compiling.
mariusvniekerk
force-pushed
the
feat/host-origin-middleware
branch
from
7b28bf5 to
0dc80e6
Compare
roborev: Combined Review (
|
2 participants
Summary
Hostheader doesn't match the configured listen address, closing a DNS-rebinding gap that bypassed the existingSec-Fetch-SiteCSRF check (the browser sees a rebound name as same-origin).localhostand127.0.0.1are canonicalized against a loopback bind.allowed_hostsconfig accepts additional hostnames (e.g.["middleman.local:8091"]) for users who reach the dashboard by a custom name.trust_reverse_proxyconfig skips the strict check and instead trusts the standard forwarded-host headers, for reverse-proxy deployments that usebase_path.allowed_hosts/trust_reverse_proxyconfig knobs.allowed_hosts, and thetrust_reverse_proxyforwarded-header path.