diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9a186bf1..72bf654b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,6 +58,16 @@ jobs: deploy/remote-node/docker-compose.yml scripts/node-join.sh docs/ README.md | sort -u || true) if [ "$(printf '%s' "$caddy_pins" | grep -c .)" -ne 1 ]; then echo "::error::caddy image pins disagree: $(echo $caddy_pins)"; fail=1; fi + # OPS-002: every own-image pin across deploy/ must name one version, + # or an alternative profile silently deploys an older panel. + hpg_pins=$(grep -rhoE 'ghcr\.io/host-yt/[a-z-]+:[0-9]+\.[0-9]+\.[0-9]+' deploy/ \ + | grep -oE '[0-9]+\.[0-9]+\.[0-9]+$' | sort -u) + if [ "$(printf '%s' "$hpg_pins" | grep -c .)" -ne 1 ]; then + echo "::error::hpg image pins in deploy/ disagree: $(echo $hpg_pins) (run: make pin-version V=x.y.z)"; fail=1; fi + # The README status line names the same version operators will deploy. + readme_ver=$(grep -oE '^\*\*Status:\*\* v[0-9]+\.[0-9]+\.[0-9]+' README.md | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') + if [ "$readme_ver" != "$hpg_pins" ]; then + echo "::error::README status v$readme_ver != deploy/ image pins $hpg_pins"; fail=1; fi # The node image and the customer install script must ship the same # wstunnel, or a bump upgrades one side of the tunnel only. wst_vers=$( { grep -hoE 'WSTUNNEL_VERSION=[0-9]+\.[0-9]+\.[0-9]+' deploy/node-agent/Dockerfile; \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 71b2f45a..bfd1be81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,111 @@ This policy applies from 1.5.0 onward. Earlier history does not follow it consistently - 1.4.2, 1.4.3 and 1.4.9 shipped features as patch releases - and is deliberately left as published. +## [1.6.0] - 2026-09-21 + +Closes all 14 findings of the 2026-09-21 external system review +(`_docs/SYSTEM_REVIEW.md`): 1 critical, 3 high, 3 medium, 6 low and 1 +architectural note. + +### Security + +- **A tenant could reach the control plane through their own route.** The main + backend was screened, but additional upstreams and path-proxy location rules + got only host/port syntax validation - and all three end up as `dial` in the + generated Caddy config. A reseller or scope-restricted admin could point one + at `127.0.0.1:2019`, or at a managed node's own address, and drive requests + to the node's unauthenticated Caddy Admin API through their own public + domain: read the whole node configuration, or replace it. One central + fail-closed screener now covers all three paths at save time and again at + build time, denying port 2019, every node/panel address, the control mesh + and the tunnel gateway. Customer private ranges (RFC1918, CGNAT) remain + allowed - they are a legitimate origin. + +- **Remote nodes published an unauthenticated Caddy Admin API on the mesh.** + The remote-node profile mapped `10.66.0.2:2019`, so any peer or host with a + route to that address could read or replace the node's configuration. The + authenticated agent admin proxy is now required for remote nodes, the port + maps to loopback, and registering a node with an unauthenticated admin URL + is refused. Approving an already-registered node warns instead of blocking, + so the documented onboarding order still works. + +- **External SSO let every mutating request through.** In the default + permissive mode the forward-auth matcher covered only GET and HEAD, so + POST/PUT/PATCH/DELETE reached the origin with no check at all - and the + matcher keyed on `Sec-Fetch-Dest`, a header the client sets, so even a GET + could opt out of authentication. The header is no longer trusted, and new + routes are created strict. Existing routes keep their current mode. + +- **Tenant-supplied header values expanded Caddy placeholders.** `{env.…}`, + `{file.…}` and `{system.…}` in a custom header, rewrite URI or redirect + target were expanded by Caddy and sent to an origin the tenant controls. The + screen the privileged custom-handler path already used now covers these too, + on save and again over stored rows at build. + +- **A client could choose its own source IP.** The generated panel route + passed an inbound `True-Client-IP` / `X-Real-IP` straight through, and the + middleware preferred those over the parsed `X-Forwarded-For` chain once + Caddy was a trusted peer - so an internet client could rotate past per-IP + rate limits, poison the audit log, and potentially match an IP allowlist. + The panel route now strips both and stamps the address Caddy actually saw. + The middleware prefers the verified right-hand XFF entry and honours those + headers only for names an operator opts into with `APP_TRUST_REALIP_HEADERS`. + +- **One tenant's WAF directive could block every other tenant's changes.** + `waf_directives` was only length-capped and concatenated into the shared + node config, so a single broken directive made every later `/load` fail for + the whole node. Scoped admins are now limited to `SecRuleRemoveById` with + numeric ids; arbitrary SecLang stays with unrestricted platform admins, and + unparseable lines are dropped at emission so a legacy row cannot brick a + node. + +- **`GET /hpg-portal/logout` was a logout CSRF.** Logout is now POST with a + token bound to the portal session; the GET renders a confirmation. + +- Secret files are created `0600`. `install_state.json` kept the mode it had + when restored or synced, which was commonly `0644`. + +### Added + +- `server healthcheck` subcommand, and Compose healthchecks for the app and + Caddy - the runtime image is distroless, so there is no shell for a `curl` + probe. Start periods are sized for a long migration or certificate issuance. +- `server doctor` reports per-node admin API authentication, secret file modes + and the process umask, and names every missing or empty required environment + variable with a generated value to use. +- `make pin-version V=x.y.z` rewrites every image pin, and CI fails if the + deploy profiles or the README disagree on the version. + +### Fixed + +- **The remote-node bootstrap could not start.** It set `admin :2019` + inside a bridge container, where that address does not exist, so the Caddy + config failed to bind. Not in the review - found while fixing the admin API + exposure. +- **The lite and Portainer profiles still deployed 1.3.2.** A fresh install + from either got code six months old, with none of the 1.4 or 1.5 fixes. +- The remote GeoIP profile could not work: the agent wrote the database into + its own container and remote Caddy ran a stock image with no geolocation + module. Agent and Caddy now share a read-only volume and the profile uses + the custom edge image. +- `make run` claimed to load `.env` and did not; the config never parsed the + file either. + +### Documentation + +- README and `internal/domain/routes/placement.go` claimed Redis-backed shared + certificate storage. The module was never built into the image and the + Caddyfile line was commented out, so each node has always issued its own + certificates. The claim is removed and per-node issuance documented, + including what to budget for ACME rate limits on failover. Building the + shared store was considered and rejected: it would put private keys in Redis + and make every node depend on it. +- `docs/SECURITY.md` said rotating `APP_SECRET` needs no downtime, while + `cmd/rotate-secret` requires the panel stopped and invalidates every HMAC API + key. Replaced with a runbook that includes re-issuing the keys. +- `docs/ARCHITECTURE.md` now states the real recovery window for deferred work + after a crash (up to about five minutes) instead of implying durability. + ## [1.5.1] - 2026-09-21 Three mTLS defects found while bringing the documentation in line with 1.5.0. diff --git a/Makefile b/Makefile index 1b87dcd5..3b362259 100644 --- a/Makefile +++ b/Makefile @@ -84,8 +84,16 @@ build: gen build-css ## Build server binary (CSS first so it embeds). CGO_ENABLED=0 $(GO) build -trimpath -ldflags="-s -w" -o $(BIN) ./cmd/server .PHONY: run -run: build-css ## Run locally (loads .env). - $(GO) run ./cmd/server +run: build-css ## Run locally (sources .env into the environment when present). + @set -a; [ -f .env ] && . ./.env; set +a; $(GO) run ./cmd/server + +.PHONY: pin-version +pin-version: ## Rewrite every deploy/ image pin + README status to V=x.y.z (single version source). + @test -n "$(V)" || { echo "usage: make pin-version V=1.5.2"; exit 1; } + @grep -rlE 'ghcr\.io/host-yt/[a-z-]+:[0-9]+\.[0-9]+\.[0-9]+' deploy/ \ + | xargs perl -pi -e 's|(ghcr\.io/host-yt/[a-z-]+):\d+\.\d+\.\d+|$$1:$(V)|g' + @perl -pi -e 's|^\*\*Status:\*\* v\d+\.\d+\.\d+|**Status:** v$(V)|' README.md + @echo "pinned $(V); CI fails if anything still disagrees" .PHONY: dev dev: build-css ## Hot-reload dev (air + templ watcher). diff --git a/README.md b/README.md index f9ed9016..d2b32345 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ yourself, with WireGuard tunnels to origin and per-node failover. The control plane configures every node over WireGuard, drives Let's Encrypt issuance, runs a WAF + GeoIP, and surfaces traffic stats. -**Status:** v1.5.1. Stack: Go 1.26.3, chi, MariaDB/MySQL or SQLite, Redis, Caddy 2.11. +**Status:** v1.6.0. Stack: Go 1.26.3, chi, MariaDB/MySQL or SQLite, Redis, Caddy 2.11. Single binary ~21 MB image, ~28 MB idle RAM. ## Use cases @@ -37,9 +37,9 @@ Single binary ~21 MB image, ~28 MB idle RAM. hosts across the group automatically. Customer/origin backends reach the fleet over a dedicated WireGuard tunnel that falls back to WSS (WebSocket-over-TLS) when UDP is blocked, so backends behind NAT or a - restrictive firewall stay reachable. Certificate storage can be shared - across a group (Redis-backed) so a failover or active_active peer already - holds the cert before it needs to serve traffic. Node health is scraped + restrictive firewall stay reachable. Each node keeps its own certificate + store and issues on demand, so a peer taking over a host issues its own + certificate on the first TLS handshake it serves. Node health is scraped from Caddy's Prometheus endpoint, and automatic failover moves routes off a dead node onto a healthy sibling in the same group. diff --git a/cmd/node-agent/main.go b/cmd/node-agent/main.go index 6bdbed3b..138ad557 100644 --- a/cmd/node-agent/main.go +++ b/cmd/node-agent/main.go @@ -1556,6 +1556,14 @@ func syncGeoIP(ctx context.Context, log *slog.Logger, c config) { log.Debug("geoip: panel has no DB yet") return } + // The DB is only useful if Caddy can read it, so the directory must be a + // volume shared with the caddy container. Creating it inside the agent's + // own filesystem would "succeed" forever while GeoIP never works (OPS-005). + if st, err := os.Stat(filepath.Dir(geoipDBPath)); err != nil || !st.IsDir() { + log.Warn("geoip: target directory missing - mount a volume shared with the caddy container", + "path", filepath.Dir(geoipDBPath)) + return + } localSHA, _ := fileSHA256(geoipDBPath) if localSHA == remoteSHA { return // already current diff --git a/cmd/server/doctor.go b/cmd/server/doctor.go index 22fce219..a8f41211 100644 --- a/cmd/server/doctor.go +++ b/cmd/server/doctor.go @@ -5,11 +5,17 @@ package main import ( "context" + "crypto/rand" "database/sql" + "encoding/hex" "fmt" + "io" "net" + "net/http" "os" "os/exec" + "strconv" + "strings" "text/tabwriter" "time" @@ -18,6 +24,7 @@ import ( "github.com/host-yt/caddy-proxy-manager/internal/caddyapi" "github.com/host-yt/caddy-proxy-manager/internal/config" "github.com/host-yt/caddy-proxy-manager/internal/installstate" + "github.com/host-yt/caddy-proxy-manager/internal/security" "github.com/host-yt/caddy-proxy-manager/internal/store" ) @@ -51,6 +58,8 @@ func runDoctor() int { var checks []check checks = append(checks, doctorConfigCheck(cfgErr)) + checks = append(checks, doctorRequiredEnv()...) + checks = append(checks, doctorSecretFiles()...) dbChecks, db := doctorDB(ctx, rawCfg) checks = append(checks, dbChecks...) @@ -251,6 +260,16 @@ func doctorNodes(ctx context.Context, db *sql.DB, rawCfg *config.Config) []check checks = append(checks, check{label + ": admin API", statusPass, n.apiURL + " reachable"}) } + // SEC-002: Caddy's admin API authenticates nothing. A node addressed at + // a remote :2019 is owned by whatever can route to it. + if security.UnauthenticatedNodeAdminURL(n.apiURL) { + checks = append(checks, check{label + ": admin API auth", statusWarn, + n.apiURL + " is Caddy's unauthenticated admin API - front it with the node-agent " + + "admin proxy and repoint api_url at http://:2021 (docs/MULTI_NODE.md)"}) + } else if n.adminProxyKeyEnc.Valid && n.adminProxyKeyEnc.String != "" { + checks = append(checks, check{label + ": admin API auth", statusPass, "agent admin proxy, bearer key issued"}) + } + if !n.modulesProbedAt.Valid { checks = append(checks, check{label + ": module probe", statusWarn, "not yet probed - populates after the next health-probe cycle"}) @@ -342,3 +361,170 @@ func summarize(checks []check) int { } return 0 } + +// runHealthcheck probes the panel's own /readyz over loopback and returns a +// process exit code. This is the container HEALTHCHECK: the runtime image is +// distroless, so no shell, curl or wget exists to do it from Compose. +func runHealthcheck() int { + bind := os.Getenv("APP_BIND") + if bind == "" { + bind = "0.0.0.0:8080" + } + _, port, err := net.SplitHostPort(bind) + if err != nil { + fmt.Fprintf(os.Stderr, "healthcheck: APP_BIND %q is not host:port\n", bind) + return 1 + } + ctx, cancel := context.WithTimeout(context.Background(), 4*time.Second) + defer cancel() + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, "http://127.0.0.1:"+port+"/readyz", nil) + resp, err := (&http.Client{}).Do(req) + if err != nil { + fmt.Fprintln(os.Stderr, "healthcheck:", err) + return 1 + } + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) + if resp.StatusCode != http.StatusOK { + fmt.Fprintf(os.Stderr, "healthcheck: /readyz returned %d\n", resp.StatusCode) + return 1 + } + return 0 +} + +// requiredEnv is every variable deploy/docker-compose.yml marks `:?` plus the +// two config.Load() refuses to start without. Compose fails on the first one +// it hits, deep inside interpolation; doctor names them all at once (OPS-004). +var requiredEnv = []struct { + name string + secret bool // generate a value to paste when it is missing +}{ + {"APP_URL", false}, + {"APP_SECRET", true}, + {"DB_NAME", false}, + {"DB_USER", false}, + {"DB_PASSWORD", true}, + {"REDIS_PASSWORD", true}, + {"MARIADB_ROOT_PASSWORD", true}, + {"INSTALL_TOKEN", true}, +} + +// envFilePath is the .env doctor reads when it runs from a compose checkout +// rather than inside the container. HPG_ENV_FILE overrides it. +func envFilePath() string { + if p := os.Getenv("HPG_ENV_FILE"); p != "" { + return p + } + return ".env" +} + +// parseEnvFile reads KEY=VALUE lines. Deliberately dumb - it exists to answer +// "is this var set and non-empty", never to interpolate. +func parseEnvFile(path string) (map[string]string, error) { + b, err := os.ReadFile(path) + if err != nil { + return nil, err + } + out := map[string]string{} + for _, line := range strings.Split(string(b), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + k, v, ok := strings.Cut(line, "=") + if !ok { + continue + } + out[strings.TrimSpace(k)] = strings.Trim(strings.TrimSpace(v), `"'`) + } + return out, nil +} + +// doctorRequiredEnv fails when a required variable is missing or empty, naming +// every one of them and offering a generated value for the secrets. An empty +// required var is a failure, not a default: `docker compose up` would abort. +func doctorRequiredEnv() []check { + fileVals, ferr := parseEnvFile(envFilePath()) + var missing []string + var gen []string + for _, v := range requiredEnv { + val := fileVals[v.name] + if val == "" { + val = os.Getenv(v.name) + } + if val != "" { + continue + } + missing = append(missing, v.name) + if v.secret { + gen = append(gen, v.name+"="+randomSecret()) + } + } + src := envFilePath() + if ferr != nil { + src = "process environment (" + envFilePath() + " not readable)" + } + if len(missing) == 0 { + return []check{{"config: required variables", statusPass, "all set in " + src}} + } + detail := "missing or empty in " + src + ": " + strings.Join(missing, ", ") + if len(gen) > 0 { + detail += " | paste: " + strings.Join(gen, " ") + } + return []check{{"config: required variables", statusFail, detail}} +} + +// randomSecret returns 32 bytes of hex - the same shape as +// `openssl rand -hex 32`, which the compose file's error message suggests. +func randomSecret() string { + var b [32]byte + if _, err := rand.Read(b[:]); err != nil { + return "" + } + return hex.EncodeToString(b[:]) +} + +// doctorSecretFiles reports the permissions of the two files that hold panel +// secrets. Group/other-readable means any other local account can read the +// credentials (OPS-003). +func doctorSecretFiles() []check { + var checks []check + for _, path := range []string{envFilePath(), stateDir + "/install_state.json"} { + fi, err := os.Stat(path) + if err != nil { + continue // not this deployment's layout; nothing to report + } + mode := fi.Mode().Perm() + label := "secrets: " + path + if mode&0o077 != 0 { + checks = append(checks, check{label, statusWarn, + fmt.Sprintf("mode %04o is readable by other local accounts - chmod 600 %s (and umask 077 before creating it)", mode, path)}) + continue + } + checks = append(checks, check{label, statusPass, fmt.Sprintf("mode %04o", mode)}) + } + if u, ok := procUmask(); ok && u&0o077 != 0o077 { + checks = append(checks, check{"secrets: umask", statusWarn, + fmt.Sprintf("umask %04o lets newly created files be group/other-readable - set umask 077 in the shell that runs the installer", u)}) + } + return checks +} + +// procUmask reads the current umask from /proc/self/status (Linux). Returns +// ok=false elsewhere - reading it via syscall would mean setting it first. +func procUmask() (int, bool) { + b, err := os.ReadFile("/proc/self/status") + if err != nil { + return 0, false + } + for _, line := range strings.Split(string(b), "\n") { + if v, ok := strings.CutPrefix(line, "Umask:"); ok { + n, err := strconv.ParseInt(strings.TrimSpace(v), 8, 32) + if err != nil { + return 0, false + } + return int(n), true + } + } + return 0, false +} diff --git a/cmd/server/main.go b/cmd/server/main.go index 4f8c52da..b83b8fda 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -80,6 +80,11 @@ func main() { if len(os.Args) > 1 && os.Args[1] == "doctor" { os.Exit(runDoctor()) } + // "server healthcheck" is the container HEALTHCHECK: the image is + // distroless, so there is no curl/wget to probe /readyz with. + if len(os.Args) > 1 && os.Args[1] == "healthcheck" { + os.Exit(runHealthcheck()) + } doctor := flag.Bool("doctor", false, "run preflight diagnostics and exit") flag.Parse() if *doctor { diff --git a/deploy/caddy/Caddyfile b/deploy/caddy/Caddyfile index cf29afa7..056f260c 100644 --- a/deploy/caddy/Caddyfile +++ b/deploy/caddy/Caddyfile @@ -22,8 +22,9 @@ # burst 5 } - # Storage: filesystem by default. Switch to Redis module for HA later. - # storage redis { host redis:6379 password "{$REDIS_PASSWORD}" } + # Storage: filesystem, per node. The image builds no shared-storage + # module (see deploy/caddy/Dockerfile), so do NOT uncomment a `storage` + # directive here - Caddy would reject the whole config at load. # Use staging while testing to avoid burning Let's Encrypt rate limit. # acme_ca https://acme-staging-v02.api.letsencrypt.org/directory diff --git a/deploy/docker-compose.lite.yml b/deploy/docker-compose.lite.yml index cf30eb1a..39fd5e77 100644 --- a/deploy/docker-compose.lite.yml +++ b/deploy/docker-compose.lite.yml @@ -22,9 +22,15 @@ services: app: - image: ${IMAGE_APP:-ghcr.io/host-yt/caddy-proxy-manager:1.3.2} + image: ${IMAGE_APP:-ghcr.io/host-yt/caddy-proxy-manager:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped + healthcheck: + test: ["CMD", "/app/server", "healthcheck"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 300s security_opt: - no-new-privileges:true environment: @@ -115,6 +121,12 @@ services: image: caddy:2.11.4 pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped + healthcheck: + test: ["CMD", "wget", "-q", "-O", "/dev/null", "http://127.0.0.1:2019/config/"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 60s cap_add: - NET_BIND_SERVICE ports: @@ -139,7 +151,7 @@ services: # access-log dashboard + analytics rollups) and runs the customer tunnel. # No WAF audit log in lite (WAF is off); the access log is enough. hpg-node-agent: - image: ${IMAGE_NODE_AGENT:-ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.3.2} + image: ${IMAGE_NODE_AGENT:-ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped network_mode: host @@ -171,7 +183,7 @@ services: - node wireguard: - image: ${IMAGE_WG:-ghcr.io/host-yt/caddy-proxy-manager-wg:1.3.2} + image: ${IMAGE_WG:-ghcr.io/host-yt/caddy-proxy-manager-wg:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped cap_add: diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 547fb26c..6b787986 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -28,7 +28,7 @@ services: - internal app: - image: ${IMAGE_APP:-ghcr.io/host-yt/caddy-proxy-manager:1.5.1} + image: ${IMAGE_APP:-ghcr.io/host-yt/caddy-proxy-manager:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped # App runs as distroless nonroot; block any setuid privilege escalation. @@ -75,6 +75,15 @@ services: GEOIP_AVAILABLE: ${GEOIP_AVAILABLE:-1} RATE_LIMIT_AVAILABLE: ${RATE_LIMIT_AVAILABLE:-1} WEIGHTED_LB_AVAILABLE: ${WEIGHTED_LB_AVAILABLE:-1} + # Reports readiness; it does not restart anything on its own (Compose has + # no autoheal). start_period is generous: a first boot runs every migration + # before /readyz turns green, and a restart during that would loop forever. + healthcheck: + test: ["CMD", "/app/server", "healthcheck"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 300s depends_on: mariadb: condition: service_healthy @@ -141,7 +150,7 @@ services: # can flip CACHE_HANDLER_AVAILABLE=1 and per-route cache_enabled does # real origin caching. CI pushes deploy/caddy/Dockerfile to ghcr; local # dev falls back to `build:` when the image isn't pullable. - image: ${IMAGE_CADDY:-ghcr.io/host-yt/caddy-proxy-manager-edge:1.5.1} + image: ${IMAGE_CADDY:-ghcr.io/host-yt/caddy-proxy-manager-edge:1.6.0} build: context: ./caddy dockerfile: Dockerfile @@ -153,6 +162,16 @@ services: - "80:80" - "443:443" - "443:443/udp" # HTTP/3 + # Probes the local admin API - it answers as soon as the config is loaded + # and carries no secrets. Deliberately not the :80/:443 listeners: those + # legitimately 503 before any route is pushed, and an ACME order in flight + # must never look like a wedged process. + healthcheck: + test: ["CMD", "wget", "-q", "-O", "/dev/null", "http://127.0.0.1:2019/config/"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 60s depends_on: geoip-init: condition: service_completed_successfully @@ -183,7 +202,7 @@ services: # file is written but nobody ships it - dashboard stays empty. # Needs a one-shot node token + WG key from the panel tunnel-enable flash. hpg-node-agent: - image: ${IMAGE_NODE_AGENT:-ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.5.1} + image: ${IMAGE_NODE_AGENT:-ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped network_mode: host @@ -230,7 +249,7 @@ services: # the kernel module path may not exist; this service is intended for # the Linux VPS where the manager actually runs in production. wireguard: - image: ${IMAGE_WG:-ghcr.io/host-yt/caddy-proxy-manager-wg:1.5.1} + image: ${IMAGE_WG:-ghcr.io/host-yt/caddy-proxy-manager-wg:1.6.0} pull_policy: ${IMAGE_PULL_POLICY:-if_not_present} restart: unless-stopped cap_add: diff --git a/deploy/node-agent/docker-compose.example.yml b/deploy/node-agent/docker-compose.example.yml index 9cb35990..f8634a61 100644 --- a/deploy/node-agent/docker-compose.example.yml +++ b/deploy/node-agent/docker-compose.example.yml @@ -12,7 +12,7 @@ version: "3.9" services: hpg-node-agent: - image: ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.3.2 + image: ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.6.0 restart: unless-stopped network_mode: host # needed for wg-tun0 + nftables + UDP listen cap_add: @@ -49,26 +49,31 @@ services: # forward new lines from startup. # HPG_FORWARD_TAIL_ONLY: "1" # - # --- Optional: front this node's Caddy admin API ------------------- + # --- REQUIRED: front this node's Caddy admin API (SEC-002) ---------- # Caddy's admin API has no authentication of its own, so publishing it on # the WireGuard IP makes "can reach 10.66.0.x:2019" equivalent to root on - # this node. With these two set, the agent serves an authenticated proxy + # this node. With these set, the agent serves an authenticated proxy # instead: it checks the panel-issued key and refuses anything outside the - # small set of admin calls the control plane makes. + # small set of admin calls the control plane makes. The panel refuses to + # register or approve a node whose api_url is a raw remote :2019. # - # Migrating a node (order matters, or the panel loses it mid-way): - # 1. set both vars here and restart the agent; - # 2. in the panel, change the node's API URL to http://:2021; - # 3. re-bind Caddy's admin to 127.0.0.1: drop the ":2019:2019" - # port mapping from the caddy service and set `admin 127.0.0.1:2019` - # in its Caddyfile, then restart Caddy; - # 4. confirm with `server doctor` that the node's admin API is reachable. - # HPG_ADMIN_PROXY_LISTEN: "10.66.0.2:2021" # this node's WG IP; never 0.0.0.0 - # HPG_ADMIN_PROXY_KEY: "${HPG_ADMIN_PROXY_KEY}" # from the panel flash, one-shot - # HPG_CADDY_ADMIN_URL: "http://127.0.0.1:2019" # where Caddy's admin listens + # Migrating an EXISTING node (order matters, or the panel loses it): + # 1. set these three vars and restart the agent; + # 2. in the panel, change the node's API URL to http://:2021 + # and confirm `server doctor` still reaches it; + # 3. only then republish Caddy's admin on loopback: change the caddy + # service's port mapping from ":2019:2019" to + # "127.0.0.1:2019:2019" and restart Caddy; + # 4. re-run `server doctor` - the "admin API auth" row must be PASS. + HPG_ADMIN_PROXY_LISTEN: "10.66.0.2:2021" # this node's WG IP; never 0.0.0.0 + HPG_ADMIN_PROXY_KEY: "${HPG_ADMIN_PROXY_KEY}" # from the panel flash, one-shot + HPG_CADDY_ADMIN_URL: "http://127.0.0.1:2019" # Caddy's admin, published on host loopback volumes: # Shared with caddy: agent reads the rolling access log Caddy writes. - /opt/hostyt-node/caddy-logs:/var/log/caddy:ro + # Shared with caddy (read-only there): the agent syncs the GeoIP mmdb + # from the panel to the path Caddy's maxmind matcher reads (OPS-005). + - /opt/hostyt-node/geoip:/data/geoip # Writable state: keeps forward offsets across restarts. - /opt/hostyt-node/agent-state:/var/lib/hpg-node-agent # Healthcheck: agent prints reconcile JSON every poll; if no log diff --git a/deploy/portainer-external-db.yml b/deploy/portainer-external-db.yml index 09cd6f91..85a0b530 100644 --- a/deploy/portainer-external-db.yml +++ b/deploy/portainer-external-db.yml @@ -23,9 +23,15 @@ services: - internal app: - image: ghcr.io/host-yt/caddy-proxy-manager:1.3.2 + image: ghcr.io/host-yt/caddy-proxy-manager:1.6.0 pull_policy: if_not_present restart: unless-stopped + healthcheck: + test: ["CMD", "/app/server", "healthcheck"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 300s environment: APP_ENV: ${APP_ENV:-production} APP_URL: ${APP_URL} @@ -82,9 +88,15 @@ services: retries: 5 caddy: - image: ghcr.io/host-yt/caddy-proxy-manager-edge:1.3.2 + image: ghcr.io/host-yt/caddy-proxy-manager-edge:1.6.0 pull_policy: if_not_present restart: unless-stopped + healthcheck: + test: ["CMD", "wget", "-q", "-O", "/dev/null", "http://127.0.0.1:2019/config/"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 60s cap_add: - NET_BIND_SERVICE extra_hosts: @@ -109,7 +121,7 @@ services: - internal hpg-node-agent: - image: ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.3.2 + image: ghcr.io/host-yt/caddy-proxy-manager-node-agent:1.6.0 pull_policy: if_not_present restart: unless-stopped network_mode: host diff --git a/deploy/remote-node/Caddyfile.bootstrap b/deploy/remote-node/Caddyfile.bootstrap index c8088715..d00d53b2 100644 --- a/deploy/remote-node/Caddyfile.bootstrap +++ b/deploy/remote-node/Caddyfile.bootstrap @@ -3,9 +3,11 @@ # is superseded by the JSON config pushed via the Admin API. { - # Admin API listens on the WireGuard IP only - never publicly. - # Edit the IP below to match this node's wg0 address. - admin 10.66.0.2:2019 + # 0.0.0.0 here is the CONTAINER's namespace, not the host: the compose file + # publishes this port on host loopback only and the node-agent fronts it + # with authentication (SEC-002). A WireGuard IP cannot be bound here at all + # - it lives on the host, outside this container's netns. + admin 0.0.0.0:2019 email {$ACME_EMAIL} diff --git a/deploy/remote-node/docker-compose.yml b/deploy/remote-node/docker-compose.yml index 6960e1f0..00165089 100644 --- a/deploy/remote-node/docker-compose.yml +++ b/deploy/remote-node/docker-compose.yml @@ -1,5 +1,8 @@ # Remote Caddy node - runs on a VPS connected to the control plane over -# WireGuard. Admin API is bound to the WG interface only. +# WireGuard. Caddy's admin API is published on host loopback only; the +# hpg-node-agent sidecar (deploy/node-agent/docker-compose.example.yml) fronts +# it with an authenticated proxy on the WireGuard IP. Deploy BOTH - the panel +# refuses to register or approve a node whose api_url is a raw remote :2019. # # Do NOT run this on the same host as deploy/docker-compose.yml - that # stack's bundled `caddy` service already owns 80/443/443udp there and @@ -11,21 +14,28 @@ # See docs/MULTI_NODE.md. # 2. Public ports 80/443/443udp open. UDP 51820 open for WG. # 3. /opt/hostyt-node/ contains this file + Caddyfile.bootstrap. +# 4. mkdir -p /opt/hostyt-node/{caddy-logs,geoip,agent-state} - shared with +# the agent, which writes the GeoIP mmdb and reads the access log. +# 5. The hpg-node-agent stack is up with HPG_ADMIN_PROXY_LISTEN/_KEY set. # # Then: # cd /opt/hostyt-node && docker compose up -d services: caddy: - # Stock Caddy used only as fallback for nodes that don't run the - # custom cache-handler build. For cache_enabled routes the node MUST - # run ghcr.io/host-yt/caddy-proxy-manager-edge (deploy/caddy/Dockerfile). - image: ${IMAGE_CADDY:-caddy:2.11.4} + # The custom edge image is the default (OPS-005): stock caddy:2.11.4 has + # no cache-handler, coraza, caddy-l4, rate_limit or maxmind-geolocation, + # so cache_enabled / WAF / streams / GeoIP routes are rejected or silently + # absent there. Override with IMAGE_CADDY=caddy:2.11.4 for a plain node. + image: ${IMAGE_CADDY:-ghcr.io/host-yt/caddy-proxy-manager-edge:1.6.0} restart: unless-stopped - # IMPORTANT: bind :2019 to the WireGuard IP only. Public ports - # (80/443/443-udp) are bound on 0.0.0.0 as usual. + # SEC-002: :2019 is Caddy's admin API and it authenticates NOTHING, so it + # is published on host loopback only. The host-networked hpg-node-agent + # reaches it there and fronts it with an authenticated proxy on the + # WireGuard IP (:2021) - that is what the panel's api_url must point at. + # Never republish this on a WireGuard or public address. ports: - - "10.66.0.2:2019:2019" # ← change to this node's wg IP + - "127.0.0.1:2019:2019" - "80:80" - "443:443" - "443:443/udp" @@ -43,6 +53,9 @@ services: # Access logs: shared with hpg-node-agent, which tails + forwards them to # the panel. Host bind path so the host-networked agent can read it too. - /opt/hostyt-node/caddy-logs:/var/log/caddy + # GeoIP mmdb the node-agent syncs from the panel; Caddy's maxmind + # matcher reads it. Read-only here: only the agent writes (OPS-005). + - /opt/hostyt-node/geoip:/data/geoip:ro environment: ASK_ENDPOINT_URL: ${ASK_ENDPOINT_URL:-http://10.66.0.1:8080/internal/ask} ACME_EMAIL: ${ACME_EMAIL:-ops@example.com} diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9752b6f1..32166f8d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -76,7 +76,8 @@ mechanism. ### `internal/config/` Loads the full runtime config from environment variables into a typed -`Config` struct. No file parsing in production; `.env` is dev-only. +`Config` struct. It never parses a file: Compose interpolates `.env`, and +`make run` sources it into the shell before starting the binary. Validates required fields (e.g. `APP_SECRET` ≥ 32 chars after install, `APP_URL`, DB credentials). Exposes module-availability gates (`CACHE_HANDLER_AVAILABLE`, `LAYER4_AVAILABLE`, `WAF_MODULE_AVAILABLE`, @@ -255,6 +256,17 @@ the periodic sweeps: health probe, drift resync, alias re-check, backups. A Redis-backed queue is a candidate for the work that today survives only as long as the process does; it is not implemented. +**What that costs, honestly.** A crash between the DB commit and the push +loses only the immediate attempt - the row is already durable, and a later +sweep re-pushes it. The recovery window is bounded by those sweeps, not by a +queue: boot push runs 10s after the leader starts, `reconcile` every 60s picks +up routes left in a stuck state, and `drift` re-pushes a node whose live +config no longer matches the DB every 5 minutes. So a route change survives a +crash but can take up to ~5 minutes to reach a node, and ordering between two +changes to different nodes is not guaranteed. Email sends and webhook +deliveries are the work that can be lost outright; webhooks retry on a 30s +dispatcher, mail does not. + ### `internal/i18n/` Cookie-based language selection; templates carry translated strings. diff --git a/docs/GEOIP.md b/docs/GEOIP.md index c389ccda..05013cdd 100644 --- a/docs/GEOIP.md +++ b/docs/GEOIP.md @@ -67,6 +67,26 @@ The admin UI accepts a comma-separated list. Blank value with `allow` or `deny` is treated as "no countries" which effectively blocks or allows everything - validate the list before saving. +## Remote nodes + +A remote node does not get the mmdb from the panel's volume - the node-agent +syncs it (`/api/node/geoip/meta` + `/api/node/geoip/mmdb`, sha256-checked) and +writes `/data/geoip/GeoLite2-Country.mmdb`. Three things have to line up, and +all three ship in `deploy/remote-node/` + `deploy/node-agent/`: + +1. **Shared volume.** The agent writes and Caddy reads the same directory: + `/opt/hostyt-node/geoip:/data/geoip` on the agent, the same path `:ro` on + the caddy service. Without it the agent warns + `geoip: target directory missing` and skips the sync rather than writing + into its own container where Caddy will never see it. +2. **A Caddy build with the module.** Stock `caddy:2.11.4` has no + `maxmind_geolocation` matcher and rejects the whole config; the remote-node + profile therefore defaults to + `ghcr.io/host-yt/caddy-proxy-manager-edge`. +3. **The capability flag.** Tick **GeoIP** under Module capabilities on the + node's edit page (or set the fleet-wide `GEOIP_AVAILABLE=1`) only once the + first two are true - see the never-flip-early note below. + ## Per-node capability Admin → Caddy Nodes shows a `GeoIP` badge on nodes where the module was detected. @@ -88,5 +108,6 @@ Admin → Settings → GeoIP shows: - GeoIP accuracy depends on MaxMind's data; VPN/proxy users may appear in the wrong country. - The DB must be present on the container filesystem at `/data/geoip/`; bind-mount or - volume required in production deployments. + volume required in production deployments. On a remote node that volume has to be + shared between the node-agent (writer) and Caddy (reader). - MaxMind GeoLite2 is free but requires a license key; GeoIP2 (paid) is not tested. diff --git a/docs/INSTALL.md b/docs/INSTALL.md index 79c7ba46..d8132d54 100644 --- a/docs/INSTALL.md +++ b/docs/INSTALL.md @@ -33,10 +33,13 @@ Port 2019 (Caddy Admin API) must **not** be exposed - it is internal-only. ```bash git clone https://github.com/host-yt/caddy-proxy-manager.git hostyt-proxy-gateway cd hostyt-proxy-gateway +umask 077 # so .env is not created world-readable cp .env.example .env +chmod 600 .env ``` -Open `.env` and set the four required variables: +Open `.env` and set the required variables. Compose refuses to render without +them, and it reports only the first one it hits: ```bash # Publicly reachable URL of the panel (must match your DNS A record) @@ -45,15 +48,31 @@ APP_URL=https://panel.example.com # Random 64-character hex secret - generate with: APP_SECRET=$(openssl rand -hex 32) -# MariaDB passwords - use strong, unique values +# MariaDB + Redis passwords - use strong, unique values DB_PASSWORD=change_me_strong MARIADB_ROOT_PASSWORD=change_me_root_strong +REDIS_PASSWORD=change_me_redis_strong + +# One-shot token that unlocks the install wizard +INSTALL_TOKEN=$(openssl rand -hex 16) # Let's Encrypt contact address CADDY_ACME_EMAIL=ops@example.com ``` -### 2.2 Start the stack +`DB_NAME` and `DB_USER` are also required; `.env.example` already fills them in. + +### 2.2 Preflight + +Check the whole set at once, with generated values for anything missing, and +the file permissions on your secrets: + +```bash +docker run --rm -v "$PWD/.env:/app/.env:ro" \ + ghcr.io/host-yt/caddy-proxy-manager:1.5.1 doctor +``` + +### 2.3 Start the stack ```bash docker compose -f deploy/docker-compose.yml --env-file .env up -d @@ -68,13 +87,13 @@ Watch startup logs: docker compose -f deploy/docker-compose.yml logs -f --tail=100 ``` -### 2.3 Open the install wizard +### 2.4 Open the install wizard Navigate to `http://:8080/install` (or the URL set in `APP_URL`). The wizard is only reachable while `INSTALLED=0`. It flips itself to `1` on completion. -### 2.4 Full vs Lite stack +### 2.5 Full vs Lite stack Two compose files ship. Pick by whether you can run the custom Caddy build. diff --git a/docs/MULTI_NODE.md b/docs/MULTI_NODE.md index b028e9a4..2c10adb8 100644 --- a/docs/MULTI_NODE.md +++ b/docs/MULTI_NODE.md @@ -14,8 +14,10 @@ WireGuard mesh: - Manager gets a private WG IP (`10.66.0.1` by default). - Each remote node gets a unique private WG IP (`10.66.0.2`, `.3`, etc.). -- Caddy Admin API on every node binds to its WG IP only, never to - `0.0.0.0`. +- Caddy's Admin API is published on the node's host loopback only, and the + node-agent fronts it with an authenticated proxy on the node's WG IP + (`:2021`) - see Section 12. Legacy nodes that publish `:2019:2019` + directly still work, but `server doctor` warns about them. - All `/load`, `/config`, `/reverse_proxy` and metrics calls from the manager travel over the encrypted WG tunnel, not over the public internet. - Public traffic (HTTP/HTTPS) arrives at the node directly; WG carries @@ -296,8 +298,9 @@ wg-quick up wg0 ### Step 4 - Write Caddy compose and Caddyfile -Creates `/opt/hostyt-node/docker-compose.yml` binding Caddy's Admin API to the -WG IP only: +Creates `/opt/hostyt-node/docker-compose.yml` publishing Caddy's Admin API on +the node's WG IP (the legacy direct path - the script prints the steps to close +it, see Section 12): ``` - "10.66.0.3:2019:2019" # WG IP only - never public @@ -310,7 +313,7 @@ Creates `/opt/hostyt-node/Caddyfile.bootstrap`: ```caddyfile { - admin 10.66.0.3:2019 + admin 0.0.0.0:2019 email ops@example.com on_demand_tls { ask http://10.66.0.1:8080/internal/ask @@ -391,12 +394,12 @@ ping -c3 10.66.0.1 # should reach the manager ### 7.4 Write Caddy compose Create `/opt/hostyt-node/docker-compose.yml` using the template from -`deploy/remote-node/docker-compose.yml`. Change the Admin API bind address to -this node's WG IP: +`deploy/remote-node/docker-compose.yml`. Keep the Admin API on host loopback +and let the node-agent front it (Section 12): ```yaml ports: - - "10.66.0.X:2019:2019" # replace X with this node's last octet + - "127.0.0.1:2019:2019" # node-agent proxies it, authenticated, on :2021 - "80:80" - "443:443" - "443:443/udp" @@ -426,7 +429,8 @@ extra_hosts: ```bash cat > /opt/hostyt-node/Caddyfile.bootstrap < on_demand_tls { ask @@ -525,6 +529,29 @@ to another peer in the same group without requiring manual intervention. whether IP forwarding and iptables/nftables rules are correctly set on each node. +### Certificates on failover + +**Certificate storage is per node.** Each node keeps its own `caddy_data` +volume and there is no shared store: the edge image (`deploy/caddy/Dockerfile`) +builds no TLS-storage module, so do not add a `storage` directive to a node's +Caddyfile - Caddy would reject the whole config at load. + +What that means in practice: + +- A route on an `active_active`/`failover` group is pushed to every node in the + group, so a peer can answer the moment traffic arrives - but it holds no + certificate for that host until it serves a handshake itself. +- The first TLS handshake a peer serves triggers on-demand issuance, gated by + the panel's `ask` endpoint. That is one ACME order per node per host, and it + costs the client a slow first handshake (seconds), not an error - unless the + CA's rate limit has been hit. +- Budget ACME rate limits for `nodes x hosts`, not `hosts`. Let's Encrypt's + 50 certificates/registered-domain/week is the one that bites on a large + active_active group; move to a CA with a higher limit or a wildcard via + DNS-01 (Section on DNS providers in [DNS_PROVIDERS.md](DNS_PROVIDERS.md)) if you are near it. +- To pre-warm a peer before a planned failover, resolve the host to that node + and complete one HTTPS request against it. + **Automatic failover** is implemented. When `failover.auto_enabled` is set (`Admin → Settings → Failover`), the alert evaluator moves active routes from a dead node to a healthy sibling in the same `mode=failover` node group and @@ -599,22 +626,22 @@ was still holding. Revoked peers in the group are left alone. ### Node shows offline in the panel 1. Verify WG handshake is up (step above). -2. Check the Caddy Admin API is reachable from the manager over WG: +2. Check the node's admin endpoint is reachable from the manager over WG: ```bash - # On manager: - curl http://10.66.0.X:2019/config/ + # On manager - :2021 is the node-agent admin proxy (401 without the key is + # the healthy answer); :2019 only on a legacy, unmigrated node. + curl -o /dev/null -w '%{http_code}\n' http://10.66.0.X:2021/config/ ``` - `connection refused` - Caddy is not running on the node, or it is bound to - the wrong IP. + `connection refused` - the agent (or Caddy) is not running on the node. 3. Check Caddy is running on the node: ```bash # On node: cd /opt/hostyt-node && docker compose ps docker compose logs caddy ``` -4. Confirm the Admin API bind in `Caddyfile.bootstrap` matches the node's WG - IP. It must be `admin 10.66.0.X:2019`, not `admin localhost:2019` or - `admin :2019`. +4. Confirm the Admin API bind in `Caddyfile.bootstrap` is `admin 0.0.0.0:2019` + - that is the container's namespace. A WG address cannot be bound from + inside a bridge container; the `ports:` mapping decides reachability. ### Node is approved but receives no routes / domains return 503 @@ -823,12 +850,19 @@ panel: ## 12. Authenticating the node's admin API -Caddy's admin API has no authentication of its own. In the default topology it -is published on the node's WireGuard address, so anything that can route to -`:2019` can replace that node's entire configuration - every tenant on -it. See [SECURITY.md](SECURITY.md#caddy-admin-api---known-limitation). +Caddy's admin API has no authentication of its own. Published on a node's +WireGuard address, anything that can route to `:2019` can replace that +node's entire configuration - every tenant on it. See +[SECURITY.md](SECURITY.md#caddy-admin-api---known-limitation). + +**This is now required for remote nodes.** `deploy/remote-node/docker-compose.yml` +publishes Caddy's admin port on host loopback only, the node-agent example has +the proxy switched on, and the panel refuses to register a node whose API URL is +a raw remote `:2019` (`/admin/nodes` and `POST /api/v1/nodes`). A fleet that is +mid-migration can set `HPG_ALLOW_UNAUTHENTICATED_NODE_ADMIN=1` on the panel to +restore the old behaviour; `server doctor` warns for every node still on it. -A node can close that by putting its **node-agent in front of the admin API**: +The node-agent goes **in front of the admin API**: ``` panel ──(bearer key, over the WG mesh)──▶ node-agent :2021 ──(127.0.0.1)──▶ Caddy admin :2019 @@ -862,9 +896,12 @@ Order matters: the panel must be able to reach the node at every step. 3. **Point the panel at the agent.** Edit the node in `/admin/nodes` and set its API URL to `http://10.66.0.2:2021`. Pushes now carry the key. -4. **Close the direct port.** In the node's compose, drop the - `"10.66.0.2:2019:2019"` mapping from the `caddy` service, and set - `admin 127.0.0.1:2019` in its Caddyfile. Restart Caddy. +4. **Close the direct port.** In the node's compose, change the `caddy` + service's mapping from `"10.66.0.2:2019:2019"` to `"127.0.0.1:2019:2019"` + and restart Caddy. Leave `admin 0.0.0.0:2019` in the Caddyfile: that is the + *container's* namespace, and the host-networked agent reaches the published + loopback port. Binding `127.0.0.1` inside the container would hide the admin + API from the agent too. 5. **Verify.** `docker compose exec app /app/server doctor` should still report the node's admin API as reachable, and a **Resync** from the panel should diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 6e04b74e..8870011a 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -222,7 +222,31 @@ own streams and cannot clear a quarantine without fixing the destination. | API key hashes | `api_keys` table | Argon2id hash | | User passwords | `users` table | Argon2id hash | -`APP_SECRET` must be ≥ 32 characters. The `cmd/rotate-secret` tool re-encrypts all blobs under a new key without downtime. +`APP_SECRET` must be ≥ 32 characters. + +### Rotating `APP_SECRET` + +`cmd/rotate-secret` re-encrypts every at-rest blob under a new key. It is +**not** a zero-downtime operation, and it invalidates API keys: + +1. **Stop the panel.** The tool rewrites rows the panel reads on boot; running + it against a live instance is unsupported. +2. **Back up** the database and `data/install_state.json`. A half-applied + rotation is unrecoverable without them. +3. Run `hpg-rotate-secret --state ./data/install_state.json --old-secret + --new-secret ` (add `--apply`; without it the run prints a + dry-run summary and changes nothing). +4. Set the new `APP_SECRET` in `.env` (or the orchestrator's secret store) and + start the panel. +5. **Re-issue every API key.** `api_keys.key_hmac` is keyed off `APP_SECRET` + and cannot be re-derived from a hash, so rotation nulls it out. Existing + integration tokens stop working: create replacements in + **Admin → API keys** and distribute them before the next automated run. + +Node admin-proxy keys, WireGuard keys and TOTP secrets are re-encrypted in +place and need no operator action. A restored backup paired with a *different* +`APP_SECRET` is the failure mode to avoid - `server doctor` reports the node +keys it can no longer decrypt. --- @@ -385,10 +409,12 @@ The security model is therefore **network reachability only**: - On the manager stack, `:2019` is reachable inside the compose network (`CADDY_ADMIN_URL: http://caddy:2019`) and the compose file deliberately never publishes the port to the host. -- On a remote node it is published on the node's WireGuard address - - `deploy/remote-node/docker-compose.yml` binds `":2019:2019"` (the - shipped example is `10.66.0.2:2019:2019`). It is reachable from anything on - the control-plane mesh. +- On a remote node it is published on **host loopback only** + (`deploy/remote-node/docker-compose.yml` binds `"127.0.0.1:2019:2019"`), and + the node-agent fronts it with an authenticated proxy on the node's WireGuard + address (`:2021`). Nodes deployed before 1.5.2 published `":2019:2019"` + and are reachable from anything on the control-plane mesh until migrated - + `server doctor` flags each one. Two of the critical findings closed in 1.4.4/1.4.5 were paths into that API from tenant-controlled configuration, not from the network: @@ -423,11 +449,20 @@ panel ──(bearer key, over the WG mesh)──▶ node-agent ──(127.0.0.1) - It refuses to start bound to `0.0.0.0`, and refuses a key shorter than 32 characters, rather than serving something that only looks authenticated. -Turning it on is per node and opt-in (`HPG_ADMIN_PROXY_LISTEN` + -`HPG_ADMIN_PROXY_KEY` on the agent, then point the node's API URL at the agent -and re-bind Caddy's admin to `127.0.0.1`) - see -[MULTI_NODE.md](MULTI_NODE.md#12-authenticating-the-nodes-admin-api). A node -without the key is reached directly, exactly as before. +It is required for remote nodes: the panel refuses to register a node whose +API URL is a raw remote `:2019`, in `/admin/nodes` and in `POST /api/v1/nodes`. +Set it up with `HPG_ADMIN_PROXY_LISTEN` + `HPG_ADMIN_PROXY_KEY` on the agent, +then point the node's API URL at the agent and publish Caddy's admin port on +host loopback - see +[MULTI_NODE.md](MULTI_NODE.md#12-authenticating-the-nodes-admin-api) for the +order, which matters. + +Existing fleets are not cut off: nothing changes for nodes already registered, +the auto-join script still onboards a node on the direct path (its agent, and +therefore its key, only exists after tunnel-enable), and +`HPG_ALLOW_UNAUTHENTICATED_NODE_ADMIN=1` on the panel re-opens registration for +a fleet mid-migration. `server doctor` prints an `admin API auth` row per node +so the remaining ones are visible. **Still outstanding.** Until a node is migrated, treat reachability of `:2019` as equivalent to root on that node and keep the control-plane diff --git a/internal/caddyapi/customhandlers.go b/internal/caddyapi/customhandlers.go index b84d3b78..64ecae98 100644 --- a/internal/caddyapi/customhandlers.go +++ b/internal/caddyapi/customhandlers.go @@ -82,6 +82,20 @@ func rejectNestedHandlers(v any, depth int) error { return nil } +// ScreenReplacerValue refuses a tenant-supplied string that would make Caddy's +// replacer read the node's environment or filesystem. Exported because upstream +// header values and rewrite targets reach the same replacer as custom handlers +// and must share one rejector. +func ScreenReplacerValue(s string) error { + low := strings.ToLower(s) + for _, tok := range unsafePlaceholderTokens { + if strings.Contains(low, tok) { + return fmt.Errorf("placeholder %q is not permitted", tok+"...}") + } + } + return nil +} + // rejectUnsafePlaceholders walks every string (keys included) below a handler // property and refuses env/file/system placeholders wherever they hide. func rejectUnsafePlaceholders(v any, depth int) error { @@ -90,11 +104,8 @@ func rejectUnsafePlaceholders(v any, depth int) error { } switch t := v.(type) { case string: - low := strings.ToLower(t) - for _, tok := range unsafePlaceholderTokens { - if strings.Contains(low, tok) { - return fmt.Errorf("placeholder %q is not permitted", tok+"...}") - } + if err := ScreenReplacerValue(t); err != nil { + return err } case map[string]any: for k, vv := range t { diff --git a/internal/caddyapi/route.go b/internal/caddyapi/route.go index 5dbdd0d8..f900a1aa 100644 --- a/internal/caddyapi/route.go +++ b/internal/caddyapi/route.go @@ -39,6 +39,12 @@ type Route struct { PathPrefix string // optional, e.g. "/api" UpstreamIP string // backend IP or hostname UpstreamPort int + // IsPanelSelfRoute marks the panel's own self-bootstrap route (see + // routes.Service.panelRoute). HPG-SEC-005: the docker-bridge peer that + // reaches this route is trusted, so any inbound True-Client-IP / + // X-Real-IP is a client claim carried straight through unless we act - + // strip both and stamp a canonical one from Caddy's own client_ip. + IsPanelSelfRoute bool // BackendResolver: when UpstreamIP is a hostname, emit dynamic_upstreams.a // using this resolver IP (e.g. peer tunnel IP that runs dnsmasq). BackendResolver string @@ -121,8 +127,13 @@ type Route struct { SSOResolver string // SSOStrictMode extends the SSO gate to ALL HTTP methods (not just GET/HEAD) // and returns 401 JSON when the IdP replies with a redirect (3xx) instead of - // passing the redirect through to the client. Use for API-only routes. - // When false (default), the gate only checks GET/HEAD document loads. + // passing the redirect through to the client. This is the only mode that + // actually authenticates every request; new routes default to it + // (routes.Service.Create). When false, the gate is permissive/document-only: + // it checks GET/HEAD page loads and lets every other method and every + // XHR/fetch subresource request through with no auth check at all - it + // exists for browser apps with their own API auth layer, not as a weaker + // form of real protection (HPG-SEC-003). SSOStrictMode bool // External marks a reverse_proxy route whose upstream is an allowlisted @@ -629,6 +640,21 @@ func BuildRoute(r Route) map[string]any { "delete": []string{"X-Mtls-Subject"}, }, }) + // HPG-SEC-005: the panel self-route is reached over the trusted docker + // bridge, so nothing downstream re-checks who the caller claims to be - + // an inbound True-Client-IP/X-Real-IP would otherwise ride straight + // through to the panel's rate limiter and audit log. Drop both, then + // stamp X-Real-IP with Caddy's own client_ip so the panel always sees + // the address Caddy itself resolved, not one the client asserted. + if r.IsPanelSelfRoute { + handlers = append(handlers, map[string]any{ + "handler": "headers", + "request": map[string]any{ + "delete": []string{"True-Client-IP", "X-Real-IP"}, + "set": map[string]any{"X-Real-IP": []string{"{http.request.client_ip}"}}, + }, + }) + } // CIDR block list fires before geo check so explicit IP bans always apply. if cidrH := buildCIDRBlock(r); cidrH != nil { handlers = append(handlers, cidrH) @@ -653,7 +679,9 @@ func BuildRoute(r Route) map[string]any { sb.WriteString("Include @owasp_crs/*.conf\n") // Custom directives must come AFTER the CRS include: SecRuleRemoveById // only matches rules already parsed, so emitting them earlier is a no-op. - if extra := strings.TrimSpace(r.WAFDirectives); extra != "" { + // Structural screen at emission: a stored line Coraza cannot parse + // would fail /load for every tenant sharing this node. + if extra, _ := SanitizeWAFDirectives(r.WAFDirectives); extra != "" { sb.WriteString(extra) sb.WriteString("\n") } @@ -994,25 +1022,25 @@ func BuildRoute(r Route) map[string]any { // on top - Proxmox uses its CSRFPreventionToken + auth ticket, // Authelia-protected apps typically share the session cookie, // etc. - // Forward-auth matcher: GET/HEAD only, AND skip - // (a) common static asset extensions / asset path trees, and - // (b) browser XHR / fetch / subresource requests detected via - // Sec-Fetch-Dest (anything other than "document"/"iframe"). + // Forward-auth matcher (permissive/document-only mode): GET/HEAD only, + // AND skip common static asset extensions / asset path trees. // - // (a) Hard SPA refresh fires dozens of parallel JS/CSS/font - // requests; each one going through Authentik queues up and the - // IdP's auth endpoint serialises behind its own DB calls → - // tail-latency blows past Caddy's read timeout → 504. + // Hard SPA refresh fires dozens of parallel JS/CSS/font requests; + // each one going through Authentik queues up and the IdP's auth + // endpoint serialises behind its own DB calls → tail-latency blows + // past Caddy's read timeout → 504. // - // (b) The browser cannot follow a cross-origin 302 on an XHR / - // fetch, so if Authentik replies with a redirect to sso.example.com the - // request is killed by CORS. SPAs talking to their own backend - // API (Proxmox, internal dashboards…) thus break under SSO - // unless we bypass forward_auth for those subresource hits. The - // upstream app must own its own API auth in this mode; SSO only - // guarantees the document load is gated. Sec-Fetch-Dest is sent - // by every modern browser - non-browser clients (curl, agents) - // omit it and are still gated, which is the safer default. + // HPG-SEC-003: this mode used to also exclude requests by + // Sec-Fetch-Dest (to spare XHR/fetch from the redirect-under-CORS + // problem), but that header is entirely client-supplied - a GET to + // a sensitive document just had to claim an excluded value to skip + // the auth gate. Never base an auth decision on it. The path-based + // asset exclusion stays (it only widens what's cacheable-looking, + // not what a specific request claims to be) but SPA XHR/fetch calls + // to their own backend API are gated like any other request now; + // use SSOPaths/SSOHosts to scope the gate away from an API path if + // that breaks a specific app, or Strict mode's all-methods 401 JSON + // which browsers don't try to redirect-follow. ssoMatch := map[string]any{ "method": []string{"GET", "HEAD"}, "not": []any{ @@ -1025,16 +1053,6 @@ func BuildRoute(r Route) map[string]any { "/pve2/*", "/static/*", "/assets/*", "/_next/static/*", }, }, - map[string]any{ - "header": map[string]any{ - "Sec-Fetch-Dest": []string{ - "empty", "image", "font", "audio", "video", - "manifest", "object", "embed", "track", - "script", "style", "report", - "worker", "serviceworker", "sharedworker", - }, - }, - }, }, } // Per-route SSO scope: narrow the gate to specific paths and/or @@ -1045,8 +1063,11 @@ func BuildRoute(r Route) map[string]any { if len(r.SSOHosts) > 0 { ssoMatch["host"] = r.SSOHosts } - // In strict mode: gate all methods with no path/header exclusions. - // In default mode: gate only GET/HEAD document loads (ssoMatch restrictions apply). + // In strict mode: gate all methods with no path exclusions. + // In permissive/document-only mode: gate only GET/HEAD page loads + // (ssoMatch restrictions apply) - POST/PUT/PATCH/DELETE reach the + // origin unauthenticated, so this mode is for browser apps with + // their own API auth layer, never for APIs with none of their own. authRoute := map[string]any{ "handle": []any{ map[string]any{ diff --git a/internal/caddyapi/route_test.go b/internal/caddyapi/route_test.go index eac61d3a..8672bdf5 100644 --- a/internal/caddyapi/route_test.go +++ b/internal/caddyapi/route_test.go @@ -958,3 +958,60 @@ func TestBuildMTLSRBAC_CarriesNodeToken(t *testing.T) { t.Error("token header emitted without a token") } } + +// TestSSOPermissiveModeIgnoresSecFetchDest is the regression for HPG-SEC-003: +// permissive/document-only SSO used to skip the auth gate for any GET/HEAD +// whose Sec-Fetch-Dest claimed a subresource kind - entirely client-supplied, +// so a plain curl GET to a sensitive page could dodge auth by setting the +// header. The matcher must never reference it. +func TestSSOPermissiveModeIgnoresSecFetchDest(t *testing.T) { + r := Route{ + ID: "60", Hosts: []string{"perm.example.com"}, UpstreamIP: "10.0.0.1", UpstreamPort: 80, + SSOProviderURL: "https://sso.example.com", SSOStrictMode: false, + } + s := mustJSON(r) + if strings.Contains(s, "Sec-Fetch-Dest") { + t.Errorf("permissive-mode matcher must not key off client-supplied Sec-Fetch-Dest\nfull: %s", s) + } + // The static-asset path exclusion must still be present (unrelated skip). + if !strings.Contains(s, `"*.js"`) { + t.Errorf("permissive-mode matcher lost its static-asset exclusion\nfull: %s", s) + } +} + +// TestSSOStrictModeGatesAllMethods: strict mode's forward-auth subroute must +// carry no "match" restriction at all - every method, every path. +func TestSSOStrictModeGatesAllMethods(t *testing.T) { + r := Route{ + ID: "61", Hosts: []string{"strict.example.com"}, UpstreamIP: "10.0.0.1", UpstreamPort: 80, + SSOProviderURL: "https://sso.example.com", SSOStrictMode: true, + } + s := mustJSON(r) + if strings.Contains(s, `"method":["GET","HEAD"]`) { + t.Errorf("strict mode must not restrict to GET/HEAD\nfull: %s", s) + } +} + +// TestPanelSelfRouteStripsSpoofableRealIPHeaders is the regression for +// HPG-SEC-005: the panel self-route is reached over the trusted docker +// bridge, so an inbound client-set True-Client-IP / X-Real-IP would ride +// straight through to the panel unless Caddy strips and re-stamps them. +func TestPanelSelfRouteStripsSpoofableRealIPHeaders(t *testing.T) { + panel := Route{ + ID: "panel_self", Hosts: []string{"proxy.example.com"}, UpstreamIP: "app", UpstreamPort: 8080, + IsPanelSelfRoute: true, + } + s := mustJSON(panel) + if !strings.Contains(s, `"delete":["True-Client-IP","X-Real-IP"]`) { + t.Errorf("panel self-route must delete both spoofable headers\nfull: %s", s) + } + if !strings.Contains(s, `"X-Real-IP":["{http.request.client_ip}"]`) { + t.Errorf("panel self-route must re-stamp X-Real-IP from Caddy's own client_ip\nfull: %s", s) + } + + // A normal tenant route must be unaffected - no header strip/stamp at all. + tenant := Route{ID: "1", Hosts: []string{"tenant.example.com"}, UpstreamIP: "10.0.0.1", UpstreamPort: 80} + if s := mustJSON(tenant); strings.Contains(s, "True-Client-IP") || strings.Contains(s, "X-Real-IP") { + t.Errorf("non-panel route must not touch real-IP headers\nfull: %s", s) + } +} diff --git a/internal/caddyapi/wafdirectives.go b/internal/caddyapi/wafdirectives.go new file mode 100644 index 00000000..48da8649 --- /dev/null +++ b/internal/caddyapi/wafdirectives.go @@ -0,0 +1,104 @@ +package caddyapi + +import ( + "fmt" + "strconv" + "strings" +) + +// WAF directives are concatenated into the SINGLE Coraza config of a shared +// node, so a line Coraza cannot parse makes every later /load fail for every +// tenant on that node. Two gates follow from that: only a Sec* directive may +// be stored at all (no Include, which would pull a file into the rule set), +// and a scoped admin - who shares the node but answers for one tenant - may +// only suppress rules by id. + +// ValidateWAFDirectives screens custom SecLang on the write path. unrestricted +// marks a platform admin with no client/reseller boundary. +func ValidateWAFDirectives(raw string, unrestricted bool) error { + for _, line := range strings.Split(raw, "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if err := wafLineOK(line); err != nil { + return err + } + if unrestricted { + continue + } + if err := wafRemoveByIDOnly(line); err != nil { + return err + } + } + return nil +} + +// SanitizeWAFDirectives drops stored lines that fail the structural screen and +// reports whether anything was removed. Emission-side companion of the +// validator: legacy rows must not be able to brick a node's config push. +func SanitizeWAFDirectives(raw string) (string, bool) { + lines := strings.Split(raw, "\n") + kept := make([]string, 0, len(lines)) + dropped := false + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + if strings.HasPrefix(trimmed, "#") || wafLineOK(trimmed) == nil { + kept = append(kept, trimmed) + continue + } + dropped = true + } + return strings.Join(kept, "\n"), dropped +} + +// wafLineOK is the structural screen every stored line must pass. +func wafLineOK(line string) error { + if len(line) > 2048 { + return fmt.Errorf("WAF directive line too long (2048 max)") + } + for _, r := range line { + if r < 0x20 && r != '\t' { + return fmt.Errorf("WAF directive contains a control character") + } + } + if strings.Count(line, `"`)%2 != 0 { + return fmt.Errorf("WAF directive has an unbalanced quote: %q", line) + } + name := line + if i := strings.IndexAny(line, " \t"); i > 0 { + name = line[:i] + } + if !strings.HasPrefix(name, "Sec") || len(name) < 4 { + return fmt.Errorf("WAF directive %q is not a Sec* directive", name) + } + for _, r := range name[3:] { + if !(r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z') { + return fmt.Errorf("WAF directive %q is not a Sec* directive", name) + } + } + return nil +} + +// wafRemoveByIDOnly is the structured subset a scoped admin may write. +func wafRemoveByIDOnly(line string) error { + fields := strings.Fields(line) + if len(fields) < 2 || fields[0] != "SecRuleRemoveById" { + return fmt.Errorf("only SecRuleRemoveById with numeric rule ids is allowed here (got %q)", fields[0]) + } + for _, id := range fields[1:] { + lo, hi, isRange := strings.Cut(id, "-") + if !wafNumericID(lo) || (isRange && !wafNumericID(hi)) { + return fmt.Errorf("SecRuleRemoveById takes numeric rule ids, got %q", id) + } + } + return nil +} + +func wafNumericID(s string) bool { + n, err := strconv.Atoi(s) + return err == nil && n > 0 && n <= 999999999 +} diff --git a/internal/caddyapi/wafdirectives_test.go b/internal/caddyapi/wafdirectives_test.go new file mode 100644 index 00000000..d7e206bf --- /dev/null +++ b/internal/caddyapi/wafdirectives_test.go @@ -0,0 +1,67 @@ +package caddyapi + +import ( + "strings" + "testing" +) + +// HPG-SEC-006: a scoped admin may only suppress rule ids; nobody may store a +// line Coraza cannot parse, because the node compiles them all into one config. +func TestValidateWAFDirectives(t *testing.T) { + cases := []struct { + name string + raw string + unrestricted bool + wantErr bool + }{ + {"scoped removebyid", "SecRuleRemoveById 942100", false, false}, + {"scoped several ids and a range", "SecRuleRemoveById 942100 942110-942120\n# note", false, false}, + {"scoped arbitrary seclang", "SecRule REQUEST_URI \"@rx x\" \"id:1,deny\"", false, true}, + {"scoped rule engine off", "SecRuleEngine Off", false, true}, + {"scoped non-numeric id", "SecRuleRemoveById foo", false, true}, + {"admin arbitrary seclang", "SecRule REQUEST_URI \"@rx x\" \"id:1,deny\"", true, false}, + {"include is never a directive", "Include /etc/passwd", true, true}, + {"unbalanced quote breaks the parser", "SecRule REQUEST_URI \"@rx x", true, true}, + {"junk line", "not a directive at all", true, true}, + {"blank and comments", "\n# hello\n", false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateWAFDirectives(tc.raw, tc.unrestricted) + if tc.wantErr != (err != nil) { + t.Fatalf("ValidateWAFDirectives(%q, %v) = %v, wantErr=%v", tc.raw, tc.unrestricted, err, tc.wantErr) + } + }) + } +} + +// A poisoned legacy row must not reach the node config: it would fail /load +// for every tenant on that node, not just its own route. +func TestBuildRouteDropsUnparseableWAFDirectives(t *testing.T) { + r := Route{ + ID: "9", Hosts: []string{"w.example.com"}, UpstreamIP: "10.0.0.9", UpstreamPort: 8080, + WAFEnabled: true, WAFModuleAvailable: true, + WAFDirectives: "Include /etc/passwd\nSecRuleRemoveById 942100\ngarbage \"", + } + s := mustJSON(r) + if strings.Contains(s, "/etc/passwd") || strings.Contains(s, "garbage") { + t.Errorf("unparseable WAF directives must be dropped\nfull: %s", s) + } + if !strings.Contains(s, "SecRuleRemoveById 942100") { + t.Errorf("valid directive must survive\nfull: %s", s) + } +} + +// HPG-SEC-004: header values reach Caddy's replacer, same as custom handlers. +func TestScreenReplacerValue(t *testing.T) { + for _, bad := range []string{"{env.APP_SECRET}", "x {FILE./etc/passwd} y", "{SYSTEM.hostname}", "{$APP_SECRET}"} { + if err := ScreenReplacerValue(bad); err == nil { + t.Errorf("%q must be rejected", bad) + } + } + for _, ok := range []string{"{http.request.host}", "{http.request.remote.host}", "static-value", ""} { + if err := ScreenReplacerValue(ok); err != nil { + t.Errorf("%q must be allowed: %v", ok, err) + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 7e18fcaf..09c6bae0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,7 +1,8 @@ // Package config loads runtime configuration from environment variables. // -// Source of truth: env. .env file is loaded only in development (when present) -// to avoid surprises in production where secrets come from the orchestrator. +// Source of truth: the process environment. This package never reads a .env +// file - in production Compose interpolates it, and `make run` sources it into +// the shell first. package config import ( @@ -29,7 +30,13 @@ type AppConfig struct { Bind string Secret string TrustedProxies []string - LogLevel string + // TrustRealIPHeaders names which single-value client-IP override headers + // (True-Client-IP, X-Real-IP) a trusted peer is allowed to set (HPG-SEC-005). + // Empty (default) = neither; XFF's verified right-hand entry is always + // used instead. Opt in only when a real edge proxy overwrites one of + // these before forwarding - env APP_TRUST_REALIP_HEADERS, comma list. + TrustRealIPHeaders []string + LogLevel string // PanelInternalHost is the address Caddy nodes use to reach the panel // container on the internal network (docker compose service name, k8s @@ -195,14 +202,15 @@ func LoadUnvalidated() *Config { func loadEnv() *Config { c := &Config{ App: AppConfig{ - Env: envOr("APP_ENV", "production"), - URL: os.Getenv("APP_URL"), - Bind: envOr("APP_BIND", "0.0.0.0:8080"), - Secret: os.Getenv("APP_SECRET"), - TrustedProxies: splitCSV(os.Getenv("APP_TRUSTED_PROXIES")), - LogLevel: envOr("LOG_LEVEL", "info"), - PanelInternalHost: envOr("APP_INTERNAL_HOST", "app"), - PanelInternalPort: envInt("APP_INTERNAL_PORT", 8080), + Env: envOr("APP_ENV", "production"), + URL: os.Getenv("APP_URL"), + Bind: envOr("APP_BIND", "0.0.0.0:8080"), + Secret: os.Getenv("APP_SECRET"), + TrustedProxies: splitCSV(os.Getenv("APP_TRUSTED_PROXIES")), + TrustRealIPHeaders: splitCSV(os.Getenv("APP_TRUST_REALIP_HEADERS")), + LogLevel: envOr("LOG_LEVEL", "info"), + PanelInternalHost: envOr("APP_INTERNAL_HOST", "app"), + PanelInternalPort: envInt("APP_INTERNAL_PORT", 8080), }, Install: InstallConfig{ Installed: envBool("INSTALLED", false), diff --git a/internal/domain/routes/build.go b/internal/domain/routes/build.go index acbff437..ddea4abc 100644 --- a/internal/domain/routes/build.go +++ b/internal/domain/routes/build.go @@ -492,6 +492,14 @@ func (s *Service) buildRoutesForNode(ctx context.Context, nodeID int64) ([]caddy var headers map[string]string if headersJSON != "" { _ = json.Unmarshal([]byte(headersJSON), &headers) + // Re-screen stored values: rows written before the header screener + // existed can still carry an {env./file./system.} placeholder. + for k, v := range headers { + if err := caddyapi.ScreenReplacerValue(k + ": " + v); err != nil { + delete(headers, k) + s.Logger.Warn("unsafe custom header dropped from config", "route_id", id, "header", k, "err", err) + } + } } // Hostname-via-tunnel-DNS feature is disabled at build time. // External route: the upstream FQDN (ip) is intentionally a hostname, @@ -545,6 +553,10 @@ func (s *Service) buildRoutesForNode(ctx context.Context, nodeID int64) ([]caddy portalReady := s.PanelInternalHost != "" && s.PanelInternalPort != 0 mtlsEnforceable := sslEnabled && mtlsCACertPEM != "" && caddyapi.MTLSCAUsable(mtlsCACertPEM) if wafEnabled { + if clean, dropped := caddyapi.SanitizeWAFDirectives(wafDirectives); dropped { + s.Logger.Warn("unparseable WAF directives dropped from config", "route_id", id) + wafDirectives = clean + } wafDirectives = appendWAFDirectives(wafDirectives, wafSuppressionDirectives(wafSups, id)) } built = append(built, caddyapi.Route{ @@ -704,6 +716,12 @@ func (s *Service) buildRoutesForNode(ctx context.Context, nodeID int64) ([]caddy s.attachBasicAuthUsers(ctx, built, ids) s.attachMTLSPathRules(ctx, built, ids) s.attachRBACTokens(built, ids, nodeID) + // Fail-closed re-screen of every dial target; must run after the upstream + // and location-rule attachments, which add targets of their own. + built, ids, err = s.screenHTTPTargets(ctx, built, ids) + if err != nil { + return nil, nil, err + } // Emission order, not DB id order: a catch-all ahead of a narrower sibling // on the same host would shadow it. ids must follow the same permutation - // buildOneRoute pairs ids[i] with built[i]. @@ -913,6 +931,14 @@ func (s *Service) attachLocationRules(ctx context.Context, built []caddyapi.Rout &rule.UpstreamScheme, &rule.RedirectURL, &rule.RedirectCode, &rule.RewriteURI); err != nil { continue } + // Rewrite target and redirect Location go through Caddy's replacer; + // re-screen stored rows the way the write path now does. + if err := firstErrOf(caddyapi.ScreenReplacerValue(rule.RewriteURI), caddyapi.ScreenReplacerValue(rule.RedirectURL)); err != nil { + if s.Logger != nil { + s.Logger.Warn("unsafe location rule dropped from config", "route_id", rid, "path", rule.Path, "err", err) + } + continue + } if i, ok := idx[rid]; ok { built[i].LocationRules = append(built[i].LocationRules, rule) } diff --git a/internal/domain/routes/httpscreen.go b/internal/domain/routes/httpscreen.go new file mode 100644 index 00000000..86adba31 --- /dev/null +++ b/internal/domain/routes/httpscreen.go @@ -0,0 +1,119 @@ +// Emission-time screening of tenant-controlled HTTP proxy targets. +package routes + +import ( + "context" + "fmt" + "strings" + + "github.com/host-yt/caddy-proxy-manager/internal/audit" + "github.com/host-yt/caddy-proxy-manager/internal/caddyapi" + "github.com/host-yt/caddy-proxy-manager/internal/streamguard" +) + +// httpDrop records one target removed from the emitted config. +type httpDrop struct { + route caddyapi.Route + part string + cause error +} + +// screenHTTPTargets re-screens every dial target of the built routes at +// emission time, so a row stored before its target became control-plane +// infrastructure (or stored through a path that predates the screener) is not +// re-emitted. A deny set that cannot be loaded fails the whole push closed. +func (s *Service) screenHTTPTargets(ctx context.Context, built []caddyapi.Route, ids []int64) ([]caddyapi.Route, []int64, error) { + if len(built) == 0 { + return built, ids, nil + } + infra, err := streamguard.LoadInfraTargets(ctx, s.DB) + if err != nil { + return nil, nil, fmt.Errorf("http target screening unavailable: %w", err) + } + outRoutes, outIDs, drops := screenHTTPSet(infra, built, ids) + for _, d := range drops { + if s.Logger != nil { + s.Logger.Warn("unsafe HTTP target dropped from config", + "part", d.part, "route_id", d.route.ID, "reason", d.cause.Error()) + } + if d.part != "primary backend" { + continue + } + // A dropped primary means the host stops being served: audit it. + audit.Write(ctx, s.DB, s.Logger, nil, audit.Entry{ + ActorType: audit.ActorSystem, + Action: "route.blocked_target", + Entity: "route", + EntityID: d.route.ID, + Meta: map[string]any{"reason": d.cause.Error(), "backend": d.route.UpstreamIP}, + }) + } + return outRoutes, outIDs, nil +} + +// screenHTTPSet splits built routes into emittable and dropped. Pure, so the +// policy is testable without a DB. ids follows the same permutation as built. +func screenHTTPSet(infra *streamguard.InfraTargets, built []caddyapi.Route, ids []int64) ([]caddyapi.Route, []int64, []httpDrop) { + outRoutes := make([]caddyapi.Route, 0, len(built)) + outIDs := make([]int64, 0, len(ids)) + var drops []httpDrop + for i, r := range built { + if r.Kind == "redirect" { + outRoutes, outIDs = append(outRoutes, r), append(outIDs, idAt(ids, i)) + continue + } + if err := screenEmitted(infra, r.UpstreamIP, r.UpstreamPort); err != nil { + drops = append(drops, httpDrop{r, "primary backend", err}) + continue + } + ups := r.Upstreams[:0:0] + for _, u := range r.Upstreams { + if err := screenEmitted(infra, u.Host, u.Port); err != nil { + drops = append(drops, httpDrop{r, "upstream", err}) + continue + } + ups = append(ups, u) + } + r.Upstreams = ups + rules := r.LocationRules[:0:0] + for _, lr := range r.LocationRules { + if lr.Action == "proxy" { + if err := screenEmitted(infra, lr.UpstreamHost, lr.UpstreamPort); err != nil { + drops = append(drops, httpDrop{r, "location rule", err}) + continue + } + } + rules = append(rules, lr) + } + r.LocationRules = rules + outRoutes, outIDs = append(outRoutes, r), append(outIDs, idAt(ids, i)) + } + return outRoutes, outIDs, drops +} + +// screenEmitted applies the deny set without resolving: tunnel and container +// backends resolve node-side, and a DNS blip must not silently delete live +// routes. The write path does the resolving screen. +func screenEmitted(infra *streamguard.InfraTargets, host string, port int) error { + if strings.TrimSpace(host) == "" { + return nil + } + return infra.ScreenHTTPTargetLiteral(host, port) +} + +func idAt(ids []int64, i int) int64 { + if i < len(ids) { + return ids[i] + } + return 0 +} + +// firstErrOf returns the first non-nil error. +func firstErrOf(errs ...error) error { + for _, e := range errs { + if e != nil { + return e + } + } + return nil +} diff --git a/internal/domain/routes/httpscreen_test.go b/internal/domain/routes/httpscreen_test.go new file mode 100644 index 00000000..578dd67d --- /dev/null +++ b/internal/domain/routes/httpscreen_test.go @@ -0,0 +1,46 @@ +package routes + +import ( + "testing" + + "github.com/host-yt/caddy-proxy-manager/internal/caddyapi" +) + +// HPG-SEC-001: rows stored before HTTP target screening existed must not be +// re-emitted by boot push, resync or drift recovery - and a poisoned extra +// upstream or path rule must not take the whole route with it. +func TestScreenHTTPSetDropsInfraTargets(t *testing.T) { + infra := screenTestInfra(t) + built := []caddyapi.Route{ + {ID: "1", UpstreamIP: "10.0.0.5", UpstreamPort: 8080}, // customer origin, kept + {ID: "2", UpstreamIP: "203.0.113.7", UpstreamPort: 8080}, + {ID: "3", UpstreamIP: "10.0.0.5", UpstreamPort: 2019}, + {ID: "4", UpstreamIP: "10.0.0.5", UpstreamPort: 8080, + Upstreams: []caddyapi.Upstream{ + {Host: "10.0.0.6", Port: 8080}, + {Host: "127.0.0.1", Port: 2019}, + }, + LocationRules: []caddyapi.LocationRule{ + {Path: "/a", Action: "proxy", UpstreamHost: "10.0.0.7", UpstreamPort: 80}, + {Path: "/b", Action: "proxy", UpstreamHost: "10.66.0.4", UpstreamPort: 80}, + }}, + {ID: "5", Kind: "redirect", UpstreamIP: "0.0.0.0"}, // never dialed + } + out, ids, drops := screenHTTPSet(infra, built, []int64{1, 2, 3, 4, 5}) + if len(out) != 3 || len(ids) != 3 { + t.Fatalf("want 3 emitted routes, got %d (%v)", len(out), ids) + } + if ids[0] != 1 || ids[1] != 4 || ids[2] != 5 { + t.Fatalf("ids must stay paired with routes, got %v", ids) + } + if len(drops) != 4 { // routes 2 and 3, plus one upstream and one rule of route 4 + t.Fatalf("want 4 drops, got %d", len(drops)) + } + survivor := out[1] + if len(survivor.Upstreams) != 1 || survivor.Upstreams[0].Host != "10.0.0.6" { + t.Errorf("only the admin-API upstream may be dropped, got %+v", survivor.Upstreams) + } + if len(survivor.LocationRules) != 1 || survivor.LocationRules[0].Path != "/a" { + t.Errorf("only the control-plane path rule may be dropped, got %+v", survivor.LocationRules) + } +} diff --git a/internal/domain/routes/lifecycle.go b/internal/domain/routes/lifecycle.go index bd94df5e..3cce4dbf 100644 --- a/internal/domain/routes/lifecycle.go +++ b/internal/domain/routes/lifecycle.go @@ -349,6 +349,11 @@ func (s *Service) Create(ctx context.Context, clientID int64, in CreateInput) (i } s.Logger.Warn("anti-squat: evicted unverified route on create", "evicted_id", conflictID, "domain", domain) } + // sso_strict_mode = 1: HPG-SEC-003, a new route defaults to the only SSO + // mode that actually authenticates every request. SSO itself stays off + // (sso_provider_url empty) until the operator sets it up via edit; this + // only decides what they get once they do. Existing rows are untouched - + // this is an INSERT-time default, not a migration/backfill. res, err := tx.ExecContext(ctx, `INSERT INTO routes (service_id, caddy_node_id, domain, path_prefix, upstream_port, upstream_scheme, ssl_enabled, websocket, force_https, http2_enabled, http3_enabled, status, @@ -357,8 +362,8 @@ func (s *Service) Create(ctx context.Context, clientID int64, in CreateInput) (i wildcard_enabled, wildcard_zone, group_id, custom_fields, via_wg_peer_id, dns_resolver_via_wg_peer_id, require_client_cert, mtls_ca_id, - domain_verified, verify_token) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 1, 0, 'pending_dns', ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, NULLIF(?, 0), NULLIF(?, ''), ?, ?, ?, NULLIF(?, 0), ?, ?)`, + domain_verified, verify_token, sso_strict_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 1, 0, 'pending_dns', ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, NULLIF(?, 0), NULLIF(?, ''), ?, ?, ?, NULLIF(?, 0), ?, ?, 1)`, in.ServiceID, nodeID, domain, pathPrefix, in.UpstreamPort, scheme, in.SSL, in.WebSocket, in.ForceHTTPS, kind, redirURL, redirCode, tagVal, diff --git a/internal/domain/routes/placement.go b/internal/domain/routes/placement.go index 4a45b326..f9d6b548 100644 --- a/internal/domain/routes/placement.go +++ b/internal/domain/routes/placement.go @@ -13,8 +13,11 @@ import ( // active_active → every enabled+approved node in the group with capacity. // failover → primary (highest priority enabled+approved+healthy); // warm-secondary (next-highest) tracked for future -// promotion. We deploy to both so the cert exists when -// we need it (caddy-tlsredis shares cert storage). +// promotion. We deploy the route to both so the secondary +// can answer immediately; certificate storage is per node +// (the shipped image has no shared-storage module), so the +// secondary issues its own cert on the first handshake it +// serves. See docs/MULTI_NODE.md "Certificates on failover". // // Capacity check uses current_routes < max_routes per node. func nodePlacement(ctx context.Context, db *sql.DB, groupID int64) (primary int64, all []int64, mode string, err error) { diff --git a/internal/domain/routes/service.go b/internal/domain/routes/service.go index 216fcafa..4337baea 100644 --- a/internal/domain/routes/service.go +++ b/internal/domain/routes/service.go @@ -193,13 +193,14 @@ func (s *Service) panelRoute() *caddyapi.Route { return nil } return &caddyapi.Route{ - ID: "panel_self", - Hosts: []string{s.PanelPublicHost}, - UpstreamIP: s.PanelInternalHost, - UpstreamPort: s.PanelInternalPort, - WebSocket: true, - ForceHTTPS: true, - HTTP2: true, + ID: "panel_self", + Hosts: []string{s.PanelPublicHost}, + UpstreamIP: s.PanelInternalHost, + UpstreamPort: s.PanelInternalPort, + WebSocket: true, + ForceHTTPS: true, + IsPanelSelfRoute: true, // HPG-SEC-005: strip/re-stamp the real-IP headers + HTTP2: true, } } diff --git a/internal/domain/routes/sso_strict_default_test.go b/internal/domain/routes/sso_strict_default_test.go new file mode 100644 index 00000000..e2732d25 --- /dev/null +++ b/internal/domain/routes/sso_strict_default_test.go @@ -0,0 +1,30 @@ +package routes + +import ( + "context" + "testing" +) + +// TestCreate_DefaultsSSOStrictMode is the regression for HPG-SEC-003: a new +// route must default to the only SSO mode that authenticates every request, +// so an operator who later configures SSO gets a secure gate unless they +// deliberately opt out. This must not touch any existing row's value - +// covered separately by not running any UPDATE/migration here at all. +func TestCreate_DefaultsSSOStrictMode(t *testing.T) { + db := newPushTestDB(t) + seedCapacityFixture(t, db, 10) + + routeID, err := newMTLSCreateSvc(t, db).Create(context.Background(), 0, CreateInput{ + ServiceID: 1, UpstreamPort: 10008, Domain: "newroute.example", + }) + if err != nil { + t.Fatalf("create: %v", err) + } + var strict int + if err := db.QueryRow("SELECT sso_strict_mode FROM routes WHERE id = ?", routeID).Scan(&strict); err != nil { + t.Fatal(err) + } + if strict != 1 { + t.Errorf("sso_strict_mode = %d, want 1 (new routes must default to strict)", strict) + } +} diff --git a/internal/httpserver/handlers/admin.go b/internal/httpserver/handlers/admin.go index 460af6f9..2323a801 100644 --- a/internal/httpserver/handlers/admin.go +++ b/internal/httpserver/handlers/admin.go @@ -842,6 +842,12 @@ func (h *AdminHandlers) NodesCreate(w http.ResponseWriter, r *http.Request) { redirectWithFlash(w, r, "/admin/nodes", "", "api_url must start with http:// or https://") return } + // SEC-002: a remote node must be reached through the agent's authenticated + // admin proxy, never Caddy's own unauthenticated :2019. + if err := security.RejectUnauthenticatedNodeAdminURL(apiURL); err != nil { + redirectWithFlash(w, r, "/admin/nodes", "", err.Error()) + return + } if publicIP != "" && net.ParseIP(publicIP) == nil { redirectWithFlash(w, r, "/admin/nodes", "", "public_ip is not a valid IP") return @@ -1509,6 +1515,13 @@ func (h *AdminHandlers) NodesApprove(w http.ResponseWriter, r *http.Request) { } ctx, cancel := context.WithTimeout(r.Context(), 5_000_000_000) defer cancel() + // SEC-002: the auto-join script registers http://:2019 - Caddy's + // unauthenticated admin API - and the node-agent that fronts it is only + // installed after approval (tunnel-enable mints its key). Approval is + // therefore the first place an operator can be told, but blocking it would + // break the documented onboarding order, so this warns instead. + var joinURL string + _ = db.QueryRowContext(ctx, "SELECT api_url FROM caddy_nodes WHERE id = ?", id).Scan(&joinURL) if _, err := db.ExecContext(ctx, "UPDATE caddy_nodes SET is_enabled = 1, approved_at = NOW(), approved_by = ? WHERE id = ? AND approved_at IS NULL", approvedBy, id); err != nil { @@ -1523,7 +1536,12 @@ func (h *AdminHandlers) NodesApprove(w http.ResponseWriter, r *http.Request) { UserID: actorUserID(sess), Action: "node.approve", Entity: "node", EntityID: fmt.Sprintf("%d", id), }) - redirectWithFlash(w, r, "/admin/nodes", "Node approved", "") + msg := "Node approved" + if security.UnauthenticatedNodeAdminURL(joinURL) { + msg += ". This node is reached over Caddy's unauthenticated admin API - " + + "enable the tunnel, run the node-agent admin proxy, then repoint its API URL at :2021 (docs/MULTI_NODE.md)" + } + redirectWithFlash(w, r, "/admin/nodes", msg, "") } // NodesResync rebuilds the node's full Caddy config from DB and POSTs /load. diff --git a/internal/httpserver/handlers/admin_hosts.go b/internal/httpserver/handlers/admin_hosts.go index a6962048..76291559 100644 --- a/internal/httpserver/handlers/admin_hosts.go +++ b/internal/httpserver/handlers/admin_hosts.go @@ -30,6 +30,7 @@ import ( "github.com/host-yt/caddy-proxy-manager/internal/httpserver/middleware" "github.com/host-yt/caddy-proxy-manager/internal/security" "github.com/host-yt/caddy-proxy-manager/internal/store" + "github.com/host-yt/caddy-proxy-manager/internal/streamguard" ) // bcryptHash returns a bcrypt hash of pw with the default cost (Caddy's @@ -795,13 +796,18 @@ func (h *AdminHandlers) HostsCreate(w http.ResponseWriter, r *http.Request) { } // SSRF screen: proxy backends and external upstreams must not target - // loopback/link-local/metadata (169.254.169.254). Redirect routes use a + // loopback/link-local/metadata or the control plane. Redirect routes use a // sentinel backend (0.0.0.0) that is never dialed, so skip them. - // Tunnel routes with empty/hostname backend resolve peer-side; public - // DNS screening would false-fail them. + // Tunnel routes with a hostname backend resolve peer-side, so only the + // resolution step is waived there - the deny set still applies. tunnelPrivate := viaWGPeerID > 0 && (form.BackendIP == "" || net.ParseIP(form.BackendIP) == nil) - if (form.Kind == "proxy" || form.External) && !tunnelPrivate { - if err := screenBackendHost(ctx, form.BackendIP); err != nil { + if form.Kind == "proxy" || form.External { + infra, ierr := loadInfraOrFail(ctx, db, h.Logger) + if ierr != nil { + h.renderHostsNewErr(w, r, form, ierr.Error()) + return + } + if err := screenBackendWith(ctx, infra, form.BackendIP, port, tunnelPrivate); err != nil { h.Logger.Warn("backend host screen failed", "err", err) h.renderHostsNewErr(w, r, form, "backend host is not reachable or not allowed") return @@ -3205,6 +3211,15 @@ func (h *AdminHandlers) HostsUpdate(w http.ResponseWriter, r *http.Request) { redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit", "", "WAF: custom directives too long (16 KiB max)") return } + if wafDirectives != "" { + // One node compiles every tenant's directives into one Coraza config, + // so a boundary-scoped admin gets the structured subset only. + wafScoped, wafOK := h.selfProvisionScope(r.Context(), sess) + if err := caddyapi.ValidateWAFDirectives(wafDirectives, wafOK && !wafScoped); err != nil { + redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit#tab=waf", "", "WAF: "+sanitizeErr(err)) + return + } + } // Geo blocking. Normalize codes (uppercase, dedupe, drop junk); off when no mode. geoMode := strings.ToLower(strings.TrimSpace(r.FormValue("geo_mode"))) if geoMode != "allow" && geoMode != "deny" { @@ -3680,22 +3695,61 @@ func (h *AdminHandlers) HostsUpdate(w http.ResponseWriter, r *http.Request) { // mTLS implies HTTPS-only: the node redirects :80 for an enforced host // whatever the box says (BuildRoute derives it), so store what is served. forceHTTPS = forceHTTPS || requireClientCert - // SSRF screen: the effective backend (proxy IP/host or external FQDN) must - // not resolve to loopback/link-local/metadata. Redirect routes never dial a - // backend, so only screen proxy/external saves. Empty backend is a no-op. + // SSRF screen: EVERY tenant-controlled dial target of this save - primary + // backend, extra upstreams and path-rule upstreams - goes through the same + // fail-closed screener. Redirect routes never dial, so skip them. if kind == "proxy" { screenHost := backendIP + screenPort := port if external { screenHost = externalHost } - sctx, scancel := context.WithTimeout(r.Context(), 5*time.Second) - if err := screenBackendHost(sctx, screenHost); err != nil { + sctx, scancel := context.WithTimeout(r.Context(), 10*time.Second) + infra, ierr := loadInfraOrFail(sctx, h.DB(), h.Logger) + if ierr != nil { scancel() - h.Logger.Warn("host save: backend screen failed", "host", screenHost, "err", err) - redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit", "", "backend address blocked or unresolvable") + redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit", "", ierr.Error()) return } + // A hostname behind a tunnel is resolved node-side; waive only the + // resolution step, never the deny set. + tunnelPrivate := viaPeerID > 0 && net.ParseIP(screenHost) == nil + serr := screenBackendWith(sctx, infra, screenHost, screenPort, tunnelPrivate) + for _, u := range newUpstreams { + if serr != nil { + break + } + serr = screenBackendWith(sctx, infra, u.Host, u.Port, viaPeerID > 0 && net.ParseIP(u.Host) == nil) + } + for _, lr := range newLocationRules { + if serr != nil { + break + } + if lr.Action != "proxy" { + continue + } + serr = screenBackendWith(sctx, infra, lr.UpstreamHost, lr.UpstreamPort, viaPeerID > 0 && net.ParseIP(lr.UpstreamHost) == nil) + } + // A tenant-set Host naming infrastructure is how a screened dial gets + // re-aimed at a vhost of the control plane; nobody needs it. + for k, v := range parseHeaderMap(headersRaw) { + if !strings.EqualFold(k, "Host") { + continue + } + hv := v + if hh, _, err := net.SplitHostPort(v); err == nil { + hv = hh + } + if infra.Blocked(hv) { + serr = fmt.Errorf("Host header %q names control-plane infrastructure", v) + } + } scancel() + if serr != nil { + h.Logger.Warn("host save: backend screen failed", "host", screenHost, "err", serr) + redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit", "", "upstream address blocked or unresolvable: "+sanitizeErr(serr)) + return + } } // Cross-tenant + node-topology guard: tunnel must belong to the same // client that owns the route's service AND live on the same Caddy @@ -3774,7 +3828,11 @@ func (h *AdminHandlers) HostsUpdate(w http.ResponseWriter, r *http.Request) { } } - headersJSON := parseHeaderLines(headersRaw) + headersJSON, hdrErr := parseHeaderLines(headersRaw) + if hdrErr != nil { + redirectWithFlash(w, r, "/admin/hosts/"+strconv.FormatInt(id, 10)+"/edit", "", sanitizeErr(hdrErr)) + return + } ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) defer cancel() @@ -4504,6 +4562,9 @@ func sanitizeLocationRules(form url.Values) ([]locationRuleRow, error) { if !(strings.HasPrefix(rule.RedirectURL, "http://") || strings.HasPrefix(rule.RedirectURL, "https://") || strings.HasPrefix(rule.RedirectURL, "/")) { return nil, fmt.Errorf("%s redirect destination must be http(s):// or /relative", path) } + if err := caddyapi.ScreenReplacerValue(rule.RedirectURL); err != nil { + return nil, fmt.Errorf("%s redirect destination: %w", path, err) + } rule.RedirectCode = atoiDefault(fieldAt(codes, i), 308) switch rule.RedirectCode { case 301, 302, 307, 308: @@ -4518,6 +4579,10 @@ func sanitizeLocationRules(form url.Values) ([]locationRuleRow, error) { if len(rule.RewriteURI) > 1024 { return nil, fmt.Errorf("%s rewrite URI too long", path) } + // Same replacer as custom handlers: no env/file/system reads. + if err := caddyapi.ScreenReplacerValue(rule.RewriteURI); err != nil { + return nil, fmt.Errorf("%s rewrite URI: %w", path, err) + } } out = append(out, rule) } @@ -4889,31 +4954,30 @@ func clampInt(n, lo, hi int) int { return n } -// screenBackendHost SSRF-screens a reverse-proxy backend the same way the -// self-service client path does: IP literals go through IsDangerousProxyBackend -// (RFC1918/CGNAT allowed for the WG mesh, loopback/metadata blocked), hostnames -// resolve and every A/AAAA must pass ValidateOutboundHost. Empty host is a no-op. -func screenBackendHost(ctx context.Context, host string) error { - host = strings.TrimSpace(host) - if host == "" { +// screenBackendHost SSRF-screens a single reverse-proxy backend through the +// central screener (loopback/metadata, admin API port, managed node, panel and +// control-plane addresses). It loads the deny set itself, so use +// screenBackendWith when several targets of one save are screened together. +func screenBackendHost(ctx context.Context, db *sql.DB, host string, port int) error { + if strings.TrimSpace(host) == "" { return nil } - if ip := net.ParseIP(host); ip != nil { - if security.IsDangerousProxyBackend(ip) { - return fmt.Errorf("backend address %s is not allowed", host) - } - return nil - } - addrs, err := net.DefaultResolver.LookupNetIP(ctx, "ip", host) - if err != nil || len(addrs) == 0 { - return fmt.Errorf("backend host %s did not resolve", host) + infra, err := streamguard.LoadInfraTargets(ctx, db) + if err != nil { + return fmt.Errorf("backend screening unavailable: %w", err) } - for _, a := range addrs { - if security.IsDangerousProxyBackend(net.IP(a.AsSlice())) { - return fmt.Errorf("backend host %s resolves to a blocked address", host) - } + return screenBackendWith(ctx, infra, host, port, false) +} + +// screenBackendWith applies the central screen to one target. tolerateUnresolved +// keeps a name the panel cannot resolve (tunnel backends resolve node-side) +// usable while every deny-set check still applies. +func screenBackendWith(ctx context.Context, infra *streamguard.InfraTargets, host string, port int, tolerateUnresolved bool) error { + err := infra.ScreenHTTPBackend(ctx, host, port) + if tolerateUnresolved && errors.Is(err, streamguard.ErrUnresolved) { + return nil } - return nil + return err } func isValidUpstreamHost(h string) bool { @@ -4995,7 +5059,24 @@ func sanitizeCustomConfig(raw string) (string, error) { // into a compact JSON object the routes.custom_headers column stores. // Empty lines and lines without ":" are skipped silently. Returns "" if // no valid lines so we keep a NULL column instead of an empty {}. -func parseHeaderLines(raw string) string { +// Values are screened first: Caddy's replacer expands them on the node, so an +// {env./file./system.} placeholder here is node-secret exfiltration. +func parseHeaderLines(raw string) (string, error) { + out := parseHeaderMap(raw) + if len(out) == 0 { + return "", nil + } + for k, v := range out { + if err := caddyapi.ScreenReplacerValue(k + ": " + v); err != nil { + return "", fmt.Errorf("custom header %q: %w", k, err) + } + } + b, _ := json.Marshal(out) + return string(b), nil +} + +// parseHeaderMap splits the textarea into name/value pairs. +func parseHeaderMap(raw string) map[string]string { out := map[string]string{} for _, line := range strings.Split(raw, "\n") { line = strings.TrimSpace(line) @@ -5013,11 +5094,7 @@ func parseHeaderLines(raw string) string { } out[name] = value } - if len(out) == 0 { - return "" - } - b, _ := json.Marshal(out) - return string(b) + return out } func (h *AdminHandlers) HostGroupCreate(w http.ResponseWriter, r *http.Request) { diff --git a/internal/httpserver/handlers/admin_hosts_ssrf_test.go b/internal/httpserver/handlers/admin_hosts_ssrf_test.go new file mode 100644 index 00000000..f45c81f8 --- /dev/null +++ b/internal/httpserver/handlers/admin_hosts_ssrf_test.go @@ -0,0 +1,39 @@ +package handlers + +import ( + "net/url" + "strings" + "testing" +) + +// HPG-SEC-004: values in these fields are expanded by Caddy's replacer on the +// node, so an env/file/system placeholder is node-secret exfiltration. +func TestTenantStringsRejectUnsafePlaceholders(t *testing.T) { + if _, err := parseHeaderLines("X-Leak: {env.APP_SECRET}"); err == nil { + t.Error("custom header with {env.} must be rejected") + } + if _, err := parseHeaderLines("X-Leak: {file./etc/passwd}"); err == nil { + t.Error("custom header with {file.} must be rejected") + } + got, err := parseHeaderLines("X-Forwarded-Host: {http.request.host}") + if err != nil || !strings.Contains(got, "http.request.host") { + t.Errorf("request placeholders must stay allowed: %q %v", got, err) + } + + form := url.Values{ + "loc_path[]": {"/a"}, + "loc_action[]": {"rewrite"}, + "loc_rewrite_uri[]": {"/x?leak={env.APP_SECRET}"}, + } + if _, err := sanitizeLocationRules(form); err == nil { + t.Error("rewrite URI with {env.} must be rejected") + } + form = url.Values{ + "loc_path[]": {"/a"}, + "loc_action[]": {"redirect"}, + "loc_redirect_url[]": {"https://x.example/{env.APP_SECRET}"}, + } + if _, err := sanitizeLocationRules(form); err == nil { + t.Error("redirect destination with {env.} must be rejected") + } +} diff --git a/internal/httpserver/handlers/admin_npm_import.go b/internal/httpserver/handlers/admin_npm_import.go index 70a21f14..cede50b1 100644 --- a/internal/httpserver/handlers/admin_npm_import.go +++ b/internal/httpserver/handlers/admin_npm_import.go @@ -289,7 +289,7 @@ func (h *AdminHandlers) importProxyHosts(ctx context.Context, db *sql.DB, result continue } // SSRF: refuse forward hosts that resolve to loopback/link-local/metadata. - if err := screenBackendHost(ctx, ph.ForwardHost); err != nil { + if err := screenBackendHost(ctx, db, ph.ForwardHost, ph.ForwardPort); err != nil { result.add(kind, name, npmActionSkipped, fmt.Sprintf("forward host %s rejected: %s", ph.ForwardHost, err)) result.Errors = append(result.Errors, fmt.Sprintf("%s: %s", ph.ForwardHost, err)) continue diff --git a/internal/httpserver/handlers/admin_npm_import_test.go b/internal/httpserver/handlers/admin_npm_import_test.go index ee21063e..d6beb78b 100644 --- a/internal/httpserver/handlers/admin_npm_import_test.go +++ b/internal/httpserver/handlers/admin_npm_import_test.go @@ -26,9 +26,11 @@ func newNpmTestDB(t *testing.T) *sql.DB { t.Cleanup(func() { _ = db.Close() }) stmts := []string{ `CREATE TABLE caddy_nodes (id INTEGER PRIMARY KEY, node_group_id INTEGER, - approved_at TIMESTAMP NULL, is_enabled INTEGER)`, + approved_at TIMESTAMP NULL, is_enabled INTEGER, + public_ip TEXT, wg_ip TEXT, api_url TEXT, public_hostname TEXT, tunnel_subnet TEXT)`, + `CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT)`, `CREATE TABLE routes (id INTEGER PRIMARY KEY, domain TEXT)`, - `INSERT INTO caddy_nodes VALUES (1, 1, CURRENT_TIMESTAMP, 1)`, + `INSERT INTO caddy_nodes VALUES (1, 1, CURRENT_TIMESTAMP, 1, '', '', '', '', '')`, `INSERT INTO routes VALUES (1, 'taken.example')`, } for _, s := range stmts { diff --git a/internal/httpserver/handlers/admin_streams.go b/internal/httpserver/handlers/admin_streams.go index 7a7654b5..0bb2dad4 100644 --- a/internal/httpserver/handlers/admin_streams.go +++ b/internal/httpserver/handlers/admin_streams.go @@ -766,13 +766,14 @@ type streamCurrent struct { dest streamDestination } -// loadInfraOrFail builds the deny set, turning a lookup failure into a -// user-facing error: screening must fail closed, never be skipped. +// loadInfraOrFail builds the deny set for stream and HTTP target screening, +// turning a lookup failure into a user-facing error: screening must fail +// closed, never be skipped. func loadInfraOrFail(ctx context.Context, db *sql.DB, logger *slog.Logger) (*streamguard.InfraTargets, error) { infra, err := streamguard.LoadInfraTargets(ctx, db) if err != nil { if logger != nil { - logger.Warn("stream screen: infra deny list unavailable", "err", err) + logger.Warn("target screen: infra deny list unavailable", "err", err) } return nil, errors.New("destination screening unavailable, try again") } diff --git a/internal/httpserver/handlers/api_fossbilling.go b/internal/httpserver/handlers/api_fossbilling.go index ed579fb1..63d9d801 100644 --- a/internal/httpserver/handlers/api_fossbilling.go +++ b/internal/httpserver/handlers/api_fossbilling.go @@ -20,7 +20,6 @@ import ( "github.com/host-yt/caddy-proxy-manager/internal/auth" "github.com/host-yt/caddy-proxy-manager/internal/domain/routes" "github.com/host-yt/caddy-proxy-manager/internal/httpserver/middleware" - "github.com/host-yt/caddy-proxy-manager/internal/security" ) // FOSSBillingHandlers handles provisioning calls from a FOSSBilling instance. @@ -229,17 +228,10 @@ func (h *FOSSBillingHandlers) ProvisionService(w http.ResponseWriter, r *http.Re fbErr(w, http.StatusBadRequest, "client_id, name, backend_ip, plan_id required") return } - backendIP := net.ParseIP(in.BackendIP) - if backendIP == nil { + if net.ParseIP(in.BackendIP) == nil { fbErr(w, http.StatusBadRequest, "backend_ip invalid") return } - // SSRF screen the backend (loopback/link-local/metadata) - twin of - // CADDY-02 / API-02. - if security.IsDangerousProxyBackend(backendIP) { - fbErr(w, http.StatusBadRequest, "backend_ip not allowed (loopback/link-local/metadata)") - return - } if in.PortStart < 1 || in.PortEnd > 65535 || in.PortStart > in.PortEnd { fbErr(w, http.StatusBadRequest, "port range invalid") return @@ -257,6 +249,13 @@ func (h *FOSSBillingHandlers) ProvisionService(w http.ResponseWriter, r *http.Re fbErr(w, http.StatusForbidden, "client not in scope") return } + // Same fail-closed screen as the web path: loopback/link-local/metadata + // plus managed node and control-plane addresses. After the scope check so + // an out-of-scope caller learns nothing about the deny set. + if err := screenBackendHost(ctx, db, in.BackendIP, 0); err != nil { + fbErr(w, http.StatusBadRequest, "backend_ip not allowed: "+sanitizeErr(err)) + return + } var nodeGroupID int64 if err := db.QueryRowContext(ctx, diff --git a/internal/httpserver/handlers/api_v1.go b/internal/httpserver/handlers/api_v1.go index c0eb187a..d7304d60 100644 --- a/internal/httpserver/handlers/api_v1.go +++ b/internal/httpserver/handlers/api_v1.go @@ -276,10 +276,10 @@ func (h *APIHandlers) ServiceCreate(w http.ResponseWriter, r *http.Request) { apiErr(w, http.StatusBadRequest, "backend_ip invalid") return } - // Screen the backend for SSRF-sensitive ranges (loopback/link-local/ - // metadata) - twin of CADDY-02 on the web path (API-02). - if security.IsDangerousProxyBackend(backendIP) { - apiErr(w, http.StatusBadRequest, "backend_ip not allowed (loopback/link-local/metadata)") + // Screen the backend through the same fail-closed screener as the web path: + // loopback/link-local/metadata plus managed node and control-plane addresses. + if err := screenBackendHost(r.Context(), h.DB(), in.BackendIP, 0); err != nil { + apiErr(w, http.StatusBadRequest, "backend_ip not allowed: "+sanitizeErr(err)) return } if in.AllowedPortStart < 1 || in.AllowedPortEnd > 65535 || in.AllowedPortStart > in.AllowedPortEnd { @@ -780,6 +780,11 @@ func (h *APIHandlers) NodeCreate(w http.ResponseWriter, r *http.Request) { apiErr(w, http.StatusBadRequest, "api_url host not allowed (loopback/link-local/metadata)") return } + // SEC-002: remote nodes go through the agent's authenticated admin proxy. + if err := security.RejectUnauthenticatedNodeAdminURL(in.APIURL); err != nil { + apiErr(w, http.StatusBadRequest, err.Error()) + return + } ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) defer cancel() var pubIP sql.NullString diff --git a/internal/httpserver/handlers/client.go b/internal/httpserver/handlers/client.go index f4a9e529..f0e38b4e 100644 --- a/internal/httpserver/handlers/client.go +++ b/internal/httpserver/handlers/client.go @@ -29,7 +29,6 @@ import ( "github.com/host-yt/caddy-proxy-manager/internal/i18n" "github.com/host-yt/caddy-proxy-manager/internal/installstate" "github.com/host-yt/caddy-proxy-manager/internal/mail" - "github.com/host-yt/caddy-proxy-manager/internal/security" "github.com/host-yt/caddy-proxy-manager/internal/store" "github.com/host-yt/caddy-proxy-manager/internal/view" ) @@ -442,16 +441,15 @@ func (h *ClientHandlers) ServiceEdit(w http.ResponseWriter, r *http.Request) { } _ = r.ParseForm() backendIP := strings.TrimSpace(r.FormValue("backend_ip")) - ip := net.ParseIP(backendIP) - if ip == nil { + if net.ParseIP(backendIP) == nil { redirectWithFlash(w, r, "/app/services", "", "backend IP invalid") return } // Even on self-service npm plans, never let a client point the node at - // loopback or link-local/cloud-metadata - that would proxy to node-local - // services or leak the node's cloud credentials. - if security.IsDangerousProxyBackend(ip) { - redirectWithFlash(w, r, "/app/services", "", "backend IP not allowed (loopback / link-local / metadata)") + // loopback, link-local/cloud-metadata or the control plane - that would + // proxy to node-local services or leak the node's cloud credentials. + if err := screenBackendHost(ctx, db, backendIP, 0); err != nil { + redirectWithFlash(w, r, "/app/services", "", "backend IP not allowed: "+sanitizeErr(err)) return } // Hard rule #2: the allowed port range is the security boundary, NOT diff --git a/internal/httpserver/handlers/portal.go b/internal/httpserver/handlers/portal.go index 4918ff91..ebec7ac7 100644 --- a/internal/httpserver/handlers/portal.go +++ b/internal/httpserver/handlers/portal.go @@ -537,8 +537,39 @@ var portal2FATmpl = template.Must(template.New("portal_2fa").Parse(` `)) -// Logout destroys the portal session. +// portalLogoutViewData is the logout confirmation page payload. +type portalLogoutViewData struct { + Host string + CSPNonce string + CSRFToken string +} + +// LogoutConfirm renders a GET confirmation page. HPG-SEC-007: logout is a +// state change, so GET must never perform it - this page only asks; the +// form it renders POSTs to Logout. +func (h *PortalHandlers) LogoutConfirm(w http.ResponseWriter, r *http.Request) { + d := portalLogoutViewData{Host: portalRequestHost(r), CSPNonce: middleware.CSPNonce(r.Context())} + d.CSRFToken = h.issuePortalCSRF(w) + var buf bytes.Buffer + if err := portalLogoutTmpl.Execute(&buf, d); err != nil { + http.Error(w, "render failed", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write(buf.Bytes()) +} + +// Logout destroys the portal session. POST-only (HPG-SEC-007): a GET here +// used to log the caller out too, so a third-party page could force a logout +// with a plain image tag or link (the public portal has no panel session and +// bypasses the panel's CSRF middleware). The double-submit token below is the +// same mechanism LoginSubmit/Portal2FASubmit already use. func (h *PortalHandlers) Logout(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + if !h.verifyPortalCSRF(r) { + h.LogoutConfirm(w, r) + return + } if c, err := r.Cookie(portalCookie); err == nil && c.Value != "" { _ = h.RDB.Del(r.Context(), portalSessPrefix+c.Value).Err() } @@ -550,6 +581,28 @@ func (h *PortalHandlers) Logout(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, portalLoginURL(host, "/", h.Secure), http.StatusSeeOther) } +// portalLogoutTmpl mirrors portalLoginTmpl's minimal self-contained style. +var portalLogoutTmpl = template.Must(template.New("portal_logout").Parse(` + + +Sign out + +
+

Sign out

+

{{.Host}}

+
+ + +
+
+`)) + func (h *PortalHandlers) createPortalSession(ctx context.Context, w http.ResponseWriter, userID int64, email, username string, rememberMe bool) error { idb := make([]byte, 32) if _, err := rand.Read(idb); err != nil { diff --git a/internal/httpserver/handlers/portal_logout_test.go b/internal/httpserver/handlers/portal_logout_test.go new file mode 100644 index 00000000..dbe40bf7 --- /dev/null +++ b/internal/httpserver/handlers/portal_logout_test.go @@ -0,0 +1,96 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +// TestPortalLogoutConfirm_GETNoSideEffects is the regression for HPG-SEC-007: +// a GET to /hpg-portal/logout must only render a confirmation, never touch +// Redis or clear the session cookie itself. +func TestPortalLogoutConfirm_GETNoSideEffects(t *testing.T) { + h := &PortalHandlers{} + req := httptest.NewRequest(http.MethodGet, "https://tenant.example.com/hpg-portal/logout", nil) + rec := httptest.NewRecorder() + + h.LogoutConfirm(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + body := rec.Body.String() + if !strings.Contains(body, `method="POST"`) || !strings.Contains(body, `action="/hpg-portal/logout"`) { + t.Errorf("confirmation page must POST to /hpg-portal/logout\nbody: %s", body) + } + if !strings.Contains(body, `name="csrf_token"`) { + t.Errorf("confirmation page must carry a csrf_token field\nbody: %s", body) + } + // The confirm page must not itself clear the portal session cookie. + for _, c := range rec.Result().Cookies() { + if c.Name == portalCookie { + t.Errorf("GET must not touch the portal session cookie, got %+v", c) + } + } +} + +// TestPortalLogout_GETIsRejected proves the state-changing handler itself +// requires POST + a valid CSRF token - a bare GET (as the old route wiring +// allowed) must not destroy the session. +func TestPortalLogout_GETIsRejected(t *testing.T) { + h := &PortalHandlers{} + req := httptest.NewRequest(http.MethodGet, "https://tenant.example.com/hpg-portal/logout", nil) + rec := httptest.NewRecorder() + + // Logout() itself has no method check (routing enforces POST); calling + // it directly with no csrf_token/cookie proves the CSRF gate, not routing. + h.Logout(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (falls back to the confirmation page)", rec.Code) + } + body := rec.Body.String() + if !strings.Contains(body, "Sign out") { + t.Errorf("missing-CSRF logout must fall back to the confirmation page, got: %s", body) + } +} + +// TestPortalLogout_CSRFMismatchRejected: a forged POST with no matching +// double-submit cookie must not destroy the session or redirect. +func TestPortalLogout_CSRFMismatchRejected(t *testing.T) { + h := &PortalHandlers{} + form := url.Values{"csrf_token": {"attacker-guess"}} + req := httptest.NewRequest(http.MethodPost, "https://tenant.example.com/hpg-portal/logout", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // No portalCSRFCookie set - double-submit has nothing to match against. + rec := httptest.NewRecorder() + + h.Logout(rec, req) + + if rec.Code == http.StatusSeeOther { + t.Fatalf("forged POST without a matching CSRF cookie must not redirect (would-be logout), got %d", rec.Code) + } +} + +// TestPortalLogout_ValidCSRFSucceeds: a same-origin POST carrying the +// double-submit cookie + matching form token clears the session and +// redirects to login - the legitimate flow must keep working. +func TestPortalLogout_ValidCSRFSucceeds(t *testing.T) { + h := &PortalHandlers{} + const tok = "real-token-value" + form := url.Values{"csrf_token": {tok}} + req := httptest.NewRequest(http.MethodPost, "https://tenant.example.com/hpg-portal/logout", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(&http.Cookie{Name: portalCSRFCookie, Value: tok}) + // No portalCookie set, so Logout skips the RDB.Del call entirely (RDB is + // nil here) - this isolates the CSRF-gate behavior from Redis. + rec := httptest.NewRecorder() + + h.Logout(rec, req) + + if rec.Code != http.StatusSeeOther { + t.Fatalf("status = %d, want 303 redirect to login", rec.Code) + } +} diff --git a/internal/httpserver/middleware/trusted_realip.go b/internal/httpserver/middleware/trusted_realip.go index 33e068e0..f70d4a82 100644 --- a/internal/httpserver/middleware/trusted_realip.go +++ b/internal/httpserver/middleware/trusted_realip.go @@ -17,10 +17,19 @@ import ( // the safer default: operators must explicitly opt-in by listing their // edge proxy CIDR. // -// For XFF specifically, only the LEFT-MOST entry that itself comes from -// outside the trusted set is used - preventing a chain of trusted proxies -// from concealing a downstream spoof. -func TrustedRealIP(trustedCIDRs []*net.IPNet) func(http.Handler) http.Handler { +// HPG-SEC-005: a trusted peer is trusted to relay XFF correctly, not to +// hand us an arbitrary client-set header verbatim. X-Forwarded-For is an +// append-only chain a well-behaved proxy (including our own bundled Caddy) +// grows by adding the address it actually saw, so walking it right-to-left +// past every trusted hop yields a value the client cannot dictate even +// though the header itself started life client-settable. True-Client-IP and +// X-Real-IP are single-value headers with no such chain - a client sets the +// whole thing - so they are honored only for the specific header names the +// operator names in `trustHeaders` (nil/empty = none), the same "model the +// proxy chain explicitly" approach CloudflareIP already uses for +// CF-Connecting-IP (see cf_ip.go): a real edge proxy that overwrites one of +// these before forwarding is opted in by name, nothing is trusted blindly. +func TrustedRealIP(trustedCIDRs []*net.IPNet, trustHeaders map[string]bool) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { peer := stripPort(r.RemoteAddr) @@ -34,36 +43,34 @@ func TrustedRealIP(trustedCIDRs []*net.IPNet) func(http.Handler) http.Handler { next.ServeHTTP(w, r) return } - if ip := strings.TrimSpace(r.Header.Get("True-Client-IP")); ip != "" { - if net.ParseIP(ip) != nil { - r.RemoteAddr = ip - next.ServeHTTP(w, r) - return - } - } - if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" { - if net.ParseIP(ip) != nil { - r.RemoteAddr = ip - next.ServeHTTP(w, r) - return - } - } if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - // Walk right-to-left: first non-trusted hop is the originator. + // Walk right-to-left: the first hop (from the right) that is + // NOT itself a trusted proxy is the verified originator - a + // chain of trusted proxies cannot conceal a downstream spoof. parts := strings.Split(xff, ",") for i := len(parts) - 1; i >= 0; i-- { cand := strings.TrimSpace(parts[i]) - if cand == "" { - continue - } - if net.ParseIP(cand) == nil { + if cand == "" || net.ParseIP(cand) == nil { continue } if inAnyCIDR(cand, trustedCIDRs) { continue } r.RemoteAddr = cand - break + next.ServeHTTP(w, r) + return + } + } + if trustHeaders["True-Client-IP"] { + if ip := strings.TrimSpace(r.Header.Get("True-Client-IP")); ip != "" && net.ParseIP(ip) != nil { + r.RemoteAddr = ip + next.ServeHTTP(w, r) + return + } + } + if trustHeaders["X-Real-IP"] { + if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" && net.ParseIP(ip) != nil { + r.RemoteAddr = ip } } next.ServeHTTP(w, r) @@ -71,6 +78,23 @@ func TrustedRealIP(trustedCIDRs []*net.IPNet) func(http.Handler) http.Handler { } } +// ParseTrustHeaders builds the trustHeaders set TrustedRealIP takes from the +// operator-supplied header name list (env APP_TRUST_REALIP_HEADERS). Only +// True-Client-IP and X-Real-IP are recognized; anything else is ignored - +// there is no handler for a third one to enable. +func ParseTrustHeaders(names []string) map[string]bool { + out := make(map[string]bool, len(names)) + for _, n := range names { + switch { + case strings.EqualFold(strings.TrimSpace(n), "True-Client-IP"): + out["True-Client-IP"] = true + case strings.EqualFold(strings.TrimSpace(n), "X-Real-IP"): + out["X-Real-IP"] = true + } + } + return out +} + // ParseCIDRList parses a list of CIDR strings into []*net.IPNet. Invalid // entries are silently skipped (config-load logging is the caller's job). func ParseCIDRList(in []string) []*net.IPNet { diff --git a/internal/httpserver/middleware/trusted_realip_test.go b/internal/httpserver/middleware/trusted_realip_test.go new file mode 100644 index 00000000..39fbf825 --- /dev/null +++ b/internal/httpserver/middleware/trusted_realip_test.go @@ -0,0 +1,93 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func trustedPeerReq(headers map[string]string) *http.Request { + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = "172.18.0.5:12345" // inside the trusted docker bridge + for k, v := range headers { + r.Header.Set(k, v) + } + return r +} + +func finalRemoteAddr(t *testing.T, mwFn func(http.Handler) http.Handler, r *http.Request) string { + t.Helper() + var got string + h := mwFn(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.RemoteAddr + })) + h.ServeHTTP(httptest.NewRecorder(), r) + return got +} + +// TestTrustedRealIP_IgnoresClientHeadersByDefault is the regression for +// HPG-SEC-005: from a trusted peer, an attacker-controlled True-Client-IP or +// X-Real-IP must not override RemoteAddr unless the operator explicitly +// opted that exact header in. +func TestTrustedRealIP_IgnoresClientHeadersByDefault(t *testing.T) { + cidrs := ParseCIDRList([]string{"172.18.0.0/16"}) + mwFn := TrustedRealIP(cidrs, nil) + + r := trustedPeerReq(map[string]string{ + "True-Client-IP": "9.9.9.9", + "X-Real-IP": "8.8.8.8", + }) + got := finalRemoteAddr(t, mwFn, r) + if got == "9.9.9.9" || got == "8.8.8.8" { + t.Fatalf("RemoteAddr = %q, must not trust client-set True-Client-IP/X-Real-IP by default", got) + } +} + +// TestTrustedRealIP_PrefersVerifiedXFFEntry: the right-hand non-trusted XFF +// hop wins even when a spoofable header is also present. +func TestTrustedRealIP_PrefersVerifiedXFFEntry(t *testing.T) { + cidrs := ParseCIDRList([]string{"172.18.0.0/16"}) + mwFn := TrustedRealIP(cidrs, nil) + + r := trustedPeerReq(map[string]string{ + "X-Forwarded-For": "203.0.113.7, 172.18.0.9", + "True-Client-IP": "9.9.9.9", // attacker-set, must lose + }) + got := finalRemoteAddr(t, mwFn, r) + if got != "203.0.113.7" { + t.Fatalf("RemoteAddr = %q, want the verified XFF entry 203.0.113.7", got) + } +} + +// TestTrustedRealIP_HonorsExplicitlyTrustedHeader: an operator who names +// X-Real-IP (their edge appliance overwrites it) gets it honored - but only +// that header, not True-Client-IP, which stays untrusted. +func TestTrustedRealIP_HonorsExplicitlyTrustedHeader(t *testing.T) { + cidrs := ParseCIDRList([]string{"172.18.0.0/16"}) + mwFn := TrustedRealIP(cidrs, ParseTrustHeaders([]string{"X-Real-IP"})) + + r := trustedPeerReq(map[string]string{"X-Real-IP": "198.51.100.4"}) + if got := finalRemoteAddr(t, mwFn, r); got != "198.51.100.4" { + t.Fatalf("RemoteAddr = %q, want the explicitly trusted X-Real-IP", got) + } + + r2 := trustedPeerReq(map[string]string{"True-Client-IP": "198.51.100.4"}) + if got := finalRemoteAddr(t, mwFn, r2); got == "198.51.100.4" { + t.Fatalf("RemoteAddr = %q, True-Client-IP must stay untrusted (only X-Real-IP was opted in)", got) + } +} + +// TestTrustedRealIP_UntrustedPeerIgnoresEverything: a peer outside the +// trusted CIDRs never gets any header honored, regardless of opt-in. +func TestTrustedRealIP_UntrustedPeerIgnoresEverything(t *testing.T) { + cidrs := ParseCIDRList([]string{"172.18.0.0/16"}) + mwFn := TrustedRealIP(cidrs, ParseTrustHeaders([]string{"True-Client-IP", "X-Real-IP"})) + + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = "203.0.113.50:1234" // outside the trusted range + r.Header.Set("True-Client-IP", "9.9.9.9") + got := finalRemoteAddr(t, mwFn, r) + if got != "203.0.113.50:1234" { + t.Fatalf("RemoteAddr = %q, untrusted peer must keep its own address", got) + } +} diff --git a/internal/httpserver/server.go b/internal/httpserver/server.go index cf068ae9..59781ed0 100644 --- a/internal/httpserver/server.go +++ b/internal/httpserver/server.go @@ -137,8 +137,10 @@ func (s *Server) routes() { r.Use(mw.TraceID) // echo request id into X-Request-Id response header // TrustedRealIP replaces chi's blind RealIP - only honors XFF / X-Real-IP // / True-Client-IP when the immediate peer is in APP_TRUSTED_PROXIES. - // Empty list = headers ignored, RemoteAddr stays as the raw peer. - r.Use(mw.TrustedRealIP(mw.ParseCIDRList(s.deps.Config.App.TrustedProxies))) + // Empty list = headers ignored, RemoteAddr stays as the raw peer. XFF's + // verified entry is always preferred; True-Client-IP/X-Real-IP need an + // explicit per-header opt-in (APP_TRUST_REALIP_HEADERS, HPG-SEC-005). + r.Use(mw.TrustedRealIP(mw.ParseCIDRList(s.deps.Config.App.TrustedProxies), mw.ParseTrustHeaders(s.deps.Config.App.TrustRealIPHeaders))) r.Use(mw.CloudflareIP(s.deps.TrustCFIP)) r.Use(chimw.Recoverer) r.Use(mw.SecurityHeaders) @@ -329,8 +331,10 @@ func (s *Server) routes() { r.Get("/verify", s.deps.Portal.Verify) r.Get("/login", s.deps.Portal.LoginPage) r.Post("/login", s.deps.Portal.LoginSubmit) + // HPG-SEC-007: logout is a state change, POST-only + CSRF-checked + // (Portal.Logout). GET only renders a confirmation that posts to it. r.Post("/logout", s.deps.Portal.Logout) - r.Get("/logout", s.deps.Portal.Logout) + r.Get("/logout", s.deps.Portal.LogoutConfirm) r.Get("/2fa", s.deps.Portal.Portal2FAPage) r.Post("/2fa", s.deps.Portal.Portal2FASubmit) // OAuth2 social login for the portal (provider-agnostic). diff --git a/internal/installstate/state.go b/internal/installstate/state.go index 27285d09..69506f47 100644 --- a/internal/installstate/state.go +++ b/internal/installstate/state.go @@ -179,6 +179,12 @@ func (m *Manager) Save(s *State) error { if err := os.Rename(tmp, m.path); err != nil { return fmt.Errorf("rename: %w", err) } + // The 0600 above only applies when WriteFile creates the temp file; a file + // restored from a backup, synced by a file-sync client, or written by an + // older build can still be world-readable. This holds the invariant. + if err := os.Chmod(m.path, 0o600); err != nil { + return fmt.Errorf("chmod: %w", err) + } m.cache = s return nil } diff --git a/internal/installstate/state_perm_test.go b/internal/installstate/state_perm_test.go new file mode 100644 index 00000000..fef72eaa --- /dev/null +++ b/internal/installstate/state_perm_test.go @@ -0,0 +1,31 @@ +package installstate + +import ( + "os" + "path/filepath" + "testing" +) + +// Save must leave the state file unreadable by other local accounts even when +// an earlier, looser file is already in place (OPS-003). +func TestSaveTightensPermissions(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "install_state.json") + if err := os.WriteFile(path, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + m, err := New(dir, "0123456789abcdef0123456789abcdef") + if err != nil { + t.Fatal(err) + } + if err := m.Save(&State{CurrentStep: StepWelcome}); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if mode := fi.Mode().Perm(); mode&0o077 != 0 { + t.Fatalf("state file mode %04o is group/other readable", mode) + } +} diff --git a/internal/security/nodeadmin.go b/internal/security/nodeadmin.go new file mode 100644 index 00000000..492eba6b --- /dev/null +++ b/internal/security/nodeadmin.go @@ -0,0 +1,48 @@ +package security + +import ( + "fmt" + "net" + "net/url" + "os" + "strings" +) + +// caddyAdminPort is Caddy's admin API default port. That API has no +// authentication of any kind, so being able to open a TCP connection to it is +// equivalent to root on the node. +const caddyAdminPort = "2019" + +// AllowUnauthenticatedNodeAdminEnv re-opens the legacy path for a fleet that is +// mid-migration to the node-agent admin proxy. +const AllowUnauthenticatedNodeAdminEnv = "HPG_ALLOW_UNAUTHENTICATED_NODE_ADMIN" + +// UnauthenticatedNodeAdminURL reports whether apiURL addresses a raw Caddy +// admin endpoint on a remote machine (SEC-002). +// +// Only a literal, non-loopback IP on :2019 counts. The manager's own bundled +// Caddy is addressed by compose service name (http://caddy:2019) on a bridge +// that publishes nothing, and a node-agent admin proxy listens on a different +// port - neither is flagged. +func UnauthenticatedNodeAdminURL(apiURL string) bool { + u, err := url.Parse(strings.TrimSpace(apiURL)) + if err != nil || u.Host == "" || u.Port() != caddyAdminPort { + return false + } + ip := net.ParseIP(u.Hostname()) + return ip != nil && !ip.IsLoopback() +} + +// RejectUnauthenticatedNodeAdminURL refuses to register a node against a raw +// remote Caddy admin API. Remote nodes must be reached through the node-agent +// admin proxy, which authenticates the panel with a per-node key. +func RejectUnauthenticatedNodeAdminURL(apiURL string) error { + if !UnauthenticatedNodeAdminURL(apiURL) || os.Getenv(AllowUnauthenticatedNodeAdminEnv) == "1" { + return nil + } + host, _, _ := net.SplitHostPort(strings.TrimPrefix(strings.TrimPrefix(apiURL, "http://"), "https://")) + return fmt.Errorf("api_url points straight at Caddy's unauthenticated admin API on :%s - "+ + "run the node-agent admin proxy on that node (HPG_ADMIN_PROXY_LISTEN/HPG_ADMIN_PROXY_KEY) "+ + "and register http://%s:2021 instead, or set %s=1 to keep the legacy path while migrating "+ + "(see docs/MULTI_NODE.md)", caddyAdminPort, host, AllowUnauthenticatedNodeAdminEnv) +} diff --git a/internal/security/nodeadmin_test.go b/internal/security/nodeadmin_test.go new file mode 100644 index 00000000..4fdb0d6a --- /dev/null +++ b/internal/security/nodeadmin_test.go @@ -0,0 +1,39 @@ +package security + +import "testing" + +func TestUnauthenticatedNodeAdminURL(t *testing.T) { + cases := []struct { + url string + want bool + }{ + {"http://10.66.0.2:2019", true}, // the shipped remote-node topology + {"http://10.66.0.2:2021", false}, // node-agent admin proxy + {"http://caddy:2019", false}, // compose-local, bridge only + {"http://127.0.0.1:2019", false}, // node-local, agent side + {"http://[::1]:2019", false}, // IPv6 loopback + {"http://[fd00::2]:2019", true}, // remote IPv6 + {"http://10.66.0.2", false}, // not the admin port + {"https://node.example.com", false}, // hostname, no admin port + {"", false}, // caller validates shape + {"::not a url", false}, // + } + for _, c := range cases { + if got := UnauthenticatedNodeAdminURL(c.url); got != c.want { + t.Errorf("UnauthenticatedNodeAdminURL(%q) = %v, want %v", c.url, got, c.want) + } + } +} + +func TestRejectUnauthenticatedNodeAdminURL(t *testing.T) { + if err := RejectUnauthenticatedNodeAdminURL("http://10.66.0.2:2019"); err == nil { + t.Fatal("expected a raw remote admin URL to be refused") + } + if err := RejectUnauthenticatedNodeAdminURL("http://10.66.0.2:2021"); err != nil { + t.Fatalf("admin-proxy URL must be accepted: %v", err) + } + t.Setenv(AllowUnauthenticatedNodeAdminEnv, "1") + if err := RejectUnauthenticatedNodeAdminURL("http://10.66.0.2:2019"); err != nil { + t.Fatalf("escape hatch must restore the legacy path: %v", err) + } +} diff --git a/internal/streamguard/httpbackend.go b/internal/streamguard/httpbackend.go new file mode 100644 index 00000000..d0e73b9f --- /dev/null +++ b/internal/streamguard/httpbackend.go @@ -0,0 +1,108 @@ +package streamguard + +import ( + "context" + "errors" + "fmt" + "net" + "os" + "strings" + + "github.com/host-yt/caddy-proxy-manager/internal/security" +) + +// ErrUnresolved marks a hostname the panel could not resolve. It is a hard +// failure on the write path, but callers whose target is resolved node-side +// (tunnel backends, config emission during a DNS blip) may tolerate exactly +// this one case without losing the deny-set checks that ran before it. +var ErrUnresolved = errors.New("host did not resolve") + +// ScreenHTTPBackend is the single fail-closed screen for every tenant-controlled +// HTTP reverse-proxy target: the primary backend, extra upstreams and path-rule +// upstreams all end up as a Caddy `dial`, so they all come through here. +// Customer RFC1918/CGNAT origins stay allowed on purpose; what is refused is +// the control plane itself - the admin API port, managed node and panel +// addresses, and the control-plane mesh. port 0 means "not dialed yet" +// (caller validates it separately) and skips only the port check. +func (t *InfraTargets) ScreenHTTPBackend(ctx context.Context, host string, port int) error { + return t.screenHTTP(ctx, host, port, true) +} + +// ScreenHTTPTargetLiteral is ScreenHTTPBackend without the DNS step: the deny +// set, the admin API port and IP literals are still enforced. Used at config +// emission, which runs on every push over every route - resolving there would +// cost a lookup per route and turn a DNS blip into dropped live routes. +func (t *InfraTargets) ScreenHTTPTargetLiteral(host string, port int) error { + return t.screenHTTP(context.Background(), host, port, false) +} + +func (t *InfraTargets) screenHTTP(ctx context.Context, host string, port int, resolve bool) error { + host = strings.TrimSpace(host) + if host == "" { + return nil + } + if port != 0 { + if port < 0 || port > 65535 { + return fmt.Errorf("invalid port %d", port) + } + if _, bad := streamDeniedPorts[port]; bad { + return fmt.Errorf("port %d is reserved for the node admin API", port) + } + } + if t.Blocked(host) { + return fmt.Errorf("%s is a managed node or control-plane address", host) + } + if ip := net.ParseIP(host); ip != nil { + if security.IsDangerousProxyBackend(ip) { + return fmt.Errorf("address %s is not allowed", host) + } + return nil + } + if !resolve { + return nil + } + addrs, err := lookupAddrs(ctx, host) + if err != nil || len(addrs) == 0 { + return fmt.Errorf("%w: %s", ErrUnresolved, host) + } + for _, a := range addrs { + lit := a.Unmap().String() + if security.IsDangerousProxyBackend(net.IP(a.Unmap().AsSlice())) { + return fmt.Errorf("host %s resolves to a blocked address", host) + } + if t.Blocked(lit) { + return fmt.Errorf("host %s resolves to %s, a managed node or control-plane address", host, lit) + } + } + return nil +} + +// addEnvInfra denies the control-plane services this process itself talks to. +// They sit on the same bridge as the data plane, so a tenant route pointed at +// them would turn Caddy into a proxy for the panel's own dependencies. +func (t *InfraTargets) addEnvInfra() { + t.AddURLHost(os.Getenv("CADDY_ADMIN_URL")) + t.Add(os.Getenv("APP_INTERNAL_HOST")) + // DB/Redis only by service name: on a single-box install their address can + // be a LAN IP that also hosts legitimate customer backends, and denying a + // whole host over a database port would break them. + addNamed(t, os.Getenv("DB_HOST")) + addNamed(t, hostOnly(os.Getenv("REDIS_ADDR"))) +} + +func addNamed(t *InfraTargets, v string) { + v = strings.TrimSpace(v) + if v == "" || net.ParseIP(v) != nil { + return + } + t.Add(v) +} + +// hostOnly strips an optional :port. +func hostOnly(addr string) string { + addr = strings.TrimSpace(addr) + if h, _, err := net.SplitHostPort(addr); err == nil { + return h + } + return addr +} diff --git a/internal/streamguard/httpbackend_test.go b/internal/streamguard/httpbackend_test.go new file mode 100644 index 00000000..23382598 --- /dev/null +++ b/internal/streamguard/httpbackend_test.go @@ -0,0 +1,93 @@ +package streamguard + +import ( + "context" + "errors" + "net/netip" + "testing" +) + +// HPG-SEC-001: the three HTTP upstream paths share this screener, so the whole +// policy is asserted here once. +func TestScreenHTTPBackend(t *testing.T) { + infra := testInfra(t) + lookupAddrs = func(_ context.Context, host string) ([]netip.Addr, error) { + switch host { + case "rebind.example.com": + return []netip.Addr{netip.MustParseAddr("198.51.100.9"), netip.MustParseAddr("127.0.0.1")}, nil + case "sneaky.example.com": + return []netip.Addr{netip.MustParseAddr("10.66.0.4")}, nil // control mesh + case "origin.example.com": + return []netip.Addr{netip.MustParseAddr("198.51.100.10")}, nil + } + return nil, errors.New("nxdomain") + } + t.Cleanup(func() { + lookupAddrs = func(ctx context.Context, host string) ([]netip.Addr, error) { return nil, errors.New("disabled") } + }) + + cases := []struct { + name string + host string + port int + wantErr bool + }{ + {"caddy admin api port", "198.51.100.10", 2019, true}, + {"loopback literal", "127.0.0.1", 8080, true}, + {"cloud metadata", "169.254.169.254", 80, true}, + {"node public ip", "203.0.113.7", 8080, true}, + {"control mesh member", "10.66.0.9", 8080, true}, + {"node hostname", "node1.example.com", 8080, true}, + {"tunnel gateway", "100.96.0.1", 8080, true}, + {"hostname resolving to node mesh", "sneaky.example.com", 8080, true}, + {"multi-record with loopback", "rebind.example.com", 8080, true}, + {"customer rfc1918 origin", "10.0.0.5", 8080, false}, + {"customer tunnel peer", "100.96.7.20", 8080, false}, + {"public origin", "origin.example.com", 443, false}, + {"empty host", "", 0, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := infra.ScreenHTTPBackend(context.Background(), tc.host, tc.port) + if tc.wantErr != (err != nil) { + t.Fatalf("ScreenHTTPBackend(%q,%d) = %v, wantErr=%v", tc.host, tc.port, err, tc.wantErr) + } + }) + } + + // An unresolvable name is a distinct, tolerable outcome for callers whose + // target is resolved node-side; everything else is a hard block. + err := infra.ScreenHTTPBackend(context.Background(), "only-on-the-node", 8080) + if !errors.Is(err, ErrUnresolved) { + t.Fatalf("unresolvable host: got %v, want ErrUnresolved", err) + } + if err := infra.ScreenHTTPTargetLiteral("only-on-the-node", 8080); err != nil { + t.Fatalf("literal screen must not resolve: %v", err) + } + if err := infra.ScreenHTTPTargetLiteral("node1.example.com", 8080); err == nil { + t.Fatal("literal screen must still deny a node hostname") + } +} + +// The deny set must also carry the services the panel itself talks to, by +// service name - a tenant route to "caddy" would reach the admin API vhost. +func TestEnvInfraDeniesControlPlaneServices(t *testing.T) { + t.Setenv("CADDY_ADMIN_URL", "http://caddy:2019") + t.Setenv("APP_INTERNAL_HOST", "app") + t.Setenv("DB_HOST", "mariadb") + t.Setenv("REDIS_ADDR", "redis:6379") + infra := New() + infra.addEnvInfra() + for _, h := range []string{"caddy", "app", "mariadb", "redis"} { + if !infra.Blocked(h) { + t.Errorf("%q must be denied", h) + } + } + // A DB on a shared LAN box must not take the whole host out of service. + t.Setenv("DB_HOST", "192.168.1.10") + shared := New() + shared.addEnvInfra() + if shared.Blocked("192.168.1.10") { + t.Error("a bare DB IP must not deny the whole host") + } +} diff --git a/internal/streamguard/streamguard.go b/internal/streamguard/streamguard.go index 56b54970..2280c2de 100644 --- a/internal/streamguard/streamguard.go +++ b/internal/streamguard/streamguard.go @@ -1,6 +1,7 @@ -// Package streamguard screens L4 stream destinations against the control-plane -// deny set. It lives outside the HTTP handlers because emission (config push) -// must apply the same check as the write path. +// Package streamguard screens tenant-controlled proxy destinations (L4 streams +// and HTTP backends) against the control-plane deny set. It lives outside the +// HTTP handlers because emission (config push) must apply the same check as +// the write path. package streamguard import ( @@ -45,6 +46,7 @@ func LoadInfraTargets(ctx context.Context, db *sql.DB) (*InfraTargets, error) { return nil, errors.New("no db") } t := New() + t.addEnvInfra() rows, err := db.QueryContext(ctx, `SELECT COALESCE(public_ip,''), COALESCE(wg_ip,''), COALESCE(api_url,''), COALESCE(public_hostname,''), COALESCE(tunnel_subnet,'') diff --git a/internal/view/admin/hosts_edit.html.tmpl b/internal/view/admin/hosts_edit.html.tmpl index b81f3f4b..deec2704 100644 --- a/internal/view/admin/hosts_edit.html.tmpl +++ b/internal/view/admin/hosts_edit.html.tmpl @@ -540,7 +540,7 @@