fix: close all 14 findings of the 2026-09-21 system review - #21
Merged
Merged
Conversation
…te nodes Caddy's admin API authenticates nothing, so "can route to <wg-ip>:2019" was equivalent to root on a node. The agent admin proxy existed but was opt-in and the shipped remote-node profile published 2019 on the WireGuard address. - deploy/remote-node: publish the admin port on 127.0.0.1 only, default to the custom edge image, add the shared GeoIP volume; the bootstrap Caddyfile now binds 0.0.0.0 inside the container (a WG address cannot be bound from a bridge container at all - that config could never start). - deploy/node-agent example: admin proxy vars are no longer commented out. - panel: refuse to register a node whose api_url is a raw remote :2019, in the admin form and POST /api/v1/nodes; `server doctor` prints an "admin API auth" row per node; approving an auto-joined node warns instead of blocking. - scripts/node-join.sh: fix the unbindable admin address and print the steps to close the direct port. Rollout order (a wrong order cuts the panel off from the node): 1. Enable tunnel / Rotate in the panel, copy HPG_ADMIN_PROXY_KEY. 2. Start the agent with HPG_ADMIN_PROXY_LISTEN/_KEY/HPG_CADDY_ADMIN_URL. 3. Repoint the node's API URL at http://<wg-ip>:2021 and verify with doctor. 4. Only then change the caddy mapping to "127.0.0.1:2019:2019". Existing nodes are untouched; HPG_ALLOW_UNAUTHENTICATED_NODE_ADMIN=1 re-opens registration for a fleet mid-migration. (cherry picked from commit 6f0d15ba6d65b5de177c538b1c17abd61fa6b798)
…issuance README and the placement comment promised Redis-backed shared cert storage (caddy-tlsredis), but the edge image builds no TLS-storage module, the Caddyfile line was commented out, and every node has its own caddy_data. The claim was chosen for removal rather than implementation: shared TLS storage holds private keys, needs Redis reachable from every remote node over the mesh, and fails closed on a Redis outage - a much larger availability and blast-radius change than the failover latency it saves. MULTI_NODE gains a "Certificates on failover" section stating what actually happens: the peer issues its own certificate on the first handshake it serves, so plan ACME rate limits for nodes x hosts. The Caddyfile comment now warns that adding a `storage` directive breaks config load instead of inviting it. (cherry picked from commit c2bbd3743c8c632fdefdd5e63eabef81cbb91cea)
The lite and Portainer profiles still shipped 1.3.2, so an operator who picked either got a panel missing every 1.4/1.5 fix. Bumped, plus two guards in the existing CI policy block: every ghcr.io/host-yt pin under deploy/ must name one version, and the README status line must be that version. `make pin-version V=x.y.z` rewrites all of them at once so the bump cannot be partial. (cherry picked from commit 6c978d8104ffaf8b4b0da5f69c131c4ac6108813)
MariaDB, Redis and the node-agent had healthchecks; the two services that matter did not, so a wedged-but-running panel or Caddy stayed "running". The runtime image is distroless - no shell, curl or wget - so the app probes itself: `server healthcheck` GETs /readyz over loopback and exits 0/1. Caddy uses the local admin endpoint rather than :80/:443, which legitimately 503s before the first route is pushed. start_period is 300s for the app (a first boot runs every migration before /readyz turns green) and 60s for Caddy; an ACME order in flight must never read as wedged. Compose reports health but does not restart on it - that needs an external supervisor. (cherry picked from commit 1acff05ef575345810fb19816e5c04f1540e8e6f)
…le modes `docker compose up` aborted on an empty REDIS_PASSWORD deep inside interpolation, and the install guide never mentioned the variable at all. `server doctor` now lists every required variable that is missing or empty in one row and prints a generated value for each secret one, so the fix is a paste rather than a loop of compose errors. Secret files: install_state.json is chmod 600 after the rename, not only on create, so a file restored from a backup or synced by a file-sync client stops being world-readable. Doctor reports the mode of .env and install_state.json plus the process umask. INSTALL.md sets umask 077 and chmods .env. (cherry picked from commit 2011619313efc8829ec93cdd62c72b919486c346)
The agent wrote /data/geoip/GeoLite2-Country.mmdb into its own container - the example mounted only logs and state - and the remote Caddy ran stock caddy:2.11.4, which has no maxmind_geolocation matcher. The feature could not work in the profile that documents it. Agent and Caddy now share /opt/hostyt-node/geoip (read-only on the Caddy side, only the agent writes), the remote profile defaults to the custom edge image, and syncGeoIP refuses to write when the directory is not a mount instead of succeeding into a path Caddy never reads. The per-node GeoIP capability flag stays the operator's gate: Caddy's admin API exposes no module list, so there is nothing to probe it with. (cherry picked from commit adfcb0530b33f9e98c52d6d5b8587f00bdd968c5)
…le SLA - `make run` advertised loading .env but only ran `go run`, and nothing in the Go code parses a .env file. The target now sources it when present, and both the package doc and ARCHITECTURE say the process environment is the only source. - SECURITY claimed APP_SECRET rotation was downtime-free. cmd/rotate-secret requires the panel stopped and nulls api_keys.key_hmac, so the runbook now spells out stop, back up, rotate, restart, re-issue every API key. - ARCHITECTURE states the actual recovery window for deferred work instead of leaving "no durable queue" as the whole answer: boot push at +10s, reconcile at 60s, drift at 5 min, so a route change is durable but can take ~5 minutes after a crash; mail is the work that can be lost outright. No queue built. (cherry picked from commit c4db9474603b807300ed3f318b51753c3c11973b)
…ch-Dest trust HPG-SEC-003. Permissive/document-only SSO mode only ever gated GET/HEAD page loads and additionally skipped the gate for any request whose client-supplied Sec-Fetch-Dest header claimed a subresource kind - a plain GET could dodge auth entirely just by setting that header, and every POST/PUT/PATCH/DELETE already went through with zero auth check. - New routes (routes.Service.Create, the single insert path for the panel UI, the client-portal API and the admin API) now default sso_strict_mode=1, so an operator who later turns on SSO for a route gets the only mode that actually authenticates every request unless they deliberately opt out. Existing rows are untouched - this is an INSERT-time default, not a migration/backfill, so no live route's behavior changes today. - Removed the Sec-Fetch-Dest exclusion from the permissive-mode matcher. Never base an auth decision on a header the client sets. The static-asset path exclusion (SPA refresh storms) is unaffected. - Renamed the UI copy to say what permissive mode actually is (document-load-only, not a weaker form of real auth) and added a visible warning when Strict mode is unchecked. What an operator notices: a route with SSO configured for the first time now starts in Strict mode (401 JSON, all methods) instead of the old redirect-only GET/HEAD gate - if that breaks a browser app that relied on the permissive redirect flow, uncheck Strict mode and accept the warning. Existing SSO routes are unaffected either way. Permissive mode no longer exempts requests that claim a non-navigation Sec-Fetch-Dest, so an SPA's own XHR/fetch calls to its backend are now gated like everything else in that mode - scope the gate off an API path with sso_paths/sso_hosts, or switch to Strict mode, if that breaks an app that relied on the old bypass. (cherry picked from commit 5a5b6429cce465b037158d319201abbbdb3dcdbc)
… the docker bridge
HPG-SEC-005. docker-compose trusts the whole bridge subnet as a proxy
peer, and the panel self-route forwarded True-Client-IP/X-Real-IP to
the panel verbatim. TrustedRealIP then preferred those single-value,
client-settable headers over the properly parsed X-Forwarded-For
chain - so an internet client could pick its own apparent source IP,
rotating past per-IP rate limits, poisoning the audit log, or matching
an allowlisted IP for a gated endpoint.
- Panel self-route (routes.Service.panelRoute, caddyapi.BuildRoute):
delete both headers and stamp X-Real-IP from Caddy's own resolved
{http.request.client_ip} instead of relaying whatever the client
sent.
- TrustedRealIP: the verified right-hand X-Forwarded-For entry (a
well-behaved proxy appends what it saw rather than letting the
client set it) is now always preferred. True-Client-IP/X-Real-IP are
only honored for header names the operator explicitly opts into
(APP_TRUST_REALIP_HEADERS) - the same "name the proxy chain" model
CloudflareIP already uses for CF-Connecting-IP - so a real edge
appliance that legitimately overwrites one of these still works once
named, and nothing is trusted blindly by default.
What an operator notices: nothing, by default - APP_TRUST_REALIP_HEADERS
is empty out of the box, so rate limiting/audit/allowlists now derive
the client IP from XFF instead of a client-settable header, which is
what they should have been doing all along. An operator whose edge
proxy sets True-Client-IP or X-Real-IP (not XFF) before reaching the
panel needs to list that header name in APP_TRUST_REALIP_HEADERS or
those requests fall back to the docker-bridge peer address.
Left alone: docker-compose.yml's 172.18.0.0/16 trust range itself
(deploy/** is another agent's area) - this fix assumes that range stays
broad and hardens what happens once a peer is inside it.
(cherry picked from commit 4fb21745e9dbfe1fdb04219006cf2225c4739ce8)
HPG-SEC-007. /hpg-portal/logout was registered for both GET and POST, and the public forward-auth portal runs on the protected host with no panel session, so it bypasses the panel's CSRF middleware entirely. A third-party page could force a visitor's logout with a plain <img> tag or link - a logout CSRF. - GET now renders a confirmation page only (PortalHandlers.LogoutConfirm) - no session lookup, no cookie mutation, no Redis call. - POST (PortalHandlers.Logout) is the only path that destroys the session, and it now requires the same double-submit CSRF token LoginSubmit/Portal2FASubmit already use (cookie + form field, set when the confirmation page renders). A POST with a missing or mismatched token falls back to rendering the confirmation instead of logging out. Checked for other links/redirects into /hpg-portal/logout (nav templates, the SSO/OIDC flow, JS) - there are none; the portal has no UI that links to it today, so nothing else needed updating. What an operator notices: an existing bookmark, image tag, or script that GETs /hpg-portal/logout to force a sign-out now only shows a "Sign out" confirmation button instead of actually signing the user out - anything driving logout must POST with the token from that page. (cherry picked from commit c4d0ea0cf1558406bc4c8aca18c49dbe24c01f50)
… and WAF
HPG-SEC-001: a reseller or scope-restricted admin could point their own route
at the control plane. The main backend allowed any RFC1918/CGNAT address with
no infra or port check, and additional upstreams plus path-rule upstreams had
no SSRF screening at all - all three become a Caddy `dial`, so a tenant could
reach the node's unauthenticated admin API on 2019 through their own domain.
Reuse the stream deny set (streamguard.InfraTargets) as the one screener for
HTTP too: InfraTargets.ScreenHTTPBackend now refuses port 2019, managed node /
panel / control-plane addresses and the WireGuard mesh, resolves hostnames and
checks every answer. All three upstream paths call it on save, and the config
builder re-screens every emitted target (deny set only, no DNS) so a legacy or
newly-infra row is dropped instead of pushed. Customer private origins stay
allowed. A tenant Host header naming infrastructure is refused too.
HPG-SEC-004: custom_headers, rewrite URIs and redirect destinations reach
Caddy's replacer, which expands {env./file./system./$} - the same rejector the
custom-handlers path already had now guards them, on save and at build.
HPG-SEC-006: waf_directives was only length-capped, so one bad SecLang line
could fail /load for every tenant on the node. Scoped admins now get the
structured subset (SecRuleRemoveById with numeric ids); unrestricted admins
keep arbitrary Sec* directives; unparseable lines are dropped at emission.
(cherry picked from commit 0f349c2d4e61ef306ef2b6e58db7f6e119bea85c)
Closes all 14 findings of the 2026-09-21 system review. Minor rather than patch: the batch adds a healthcheck subcommand, new doctor checks, a single-source version pin target and APP_TRUST_REALIP_HEADERS. No existing configuration format changes, and the upgrade is still a docker pull plus the migrations the panel runs itself - but three behaviours tighten, and the upgrade notes say which.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security remediation. Details withheld until operators have had time to upgrade; see the CHANGELOG for impact and upgrade notes.