From cdaf7c0e032bc8ea17249dfdea4606dae5bf93f4 Mon Sep 17 00:00:00 2001 From: Edwin <18327273+thinkbig1979@users.noreply.github.com> Date: Fri, 31 Jul 2026 22:48:53 +0200 Subject: [PATCH] fix(health): split liveness from readiness and make reachability configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capstan is a separate process from Docker, but /health returned 503 whenever the daemon was unreachable — a readiness signal on a liveness channel. Anything that restarts on failed health checks would bounce Capstan for an outage a restart cannot fix. - /health is liveness: 200 whenever the process is serving, no Docker call. Body keeps {"status":"healthy"} so existing monitors still match. - /health/ready is readiness: reports dependencies, 503 naming what is degraded, with a 2s timeout on the Docker probe so a hung daemon cannot pile up goroutines across repeated probes. Uses Ping, one round-trip, rather than the GetContainerList("") the old handler ran every 30 seconds. - Handler moved out of main.go into internal/handlers/health.go, the last inline route, with 12 tests covering every branch. - Reachability is HEALTH_ALLOWED_NETWORKS, deliberately NOT TRUSTED_NETWORKS: that value is Gin's trusted-proxy list and the AUTH_DISABLED bypass list, so reusing it would force an operator to grant an uptime monitor X-Forwarded-For spoofing and auth bypass just to read a health endpoint. Empty means loopback only, so no upgrade widens exposure silently. The whole loopback range is accepted, matching the net.IP.IsLoopback the old handler used rather than narrowing to the literal 127.0.0.1. - DockerService.Ping tolerates a nil receiver and nil client: main leaves dockerService nil when the daemon was unreachable at startup, and a nil *DockerService in an interface is a non-nil interface value. The HEALTHCHECK was also fixed, and this was not in the issue. It used `wget --spider`, which sends HEAD; no HEAD route is registered, so every HEAD fell through to the SPA catch-all and returned 200. Measured on the pre-change image with Docker unreachable: GET /health 503, HEAD /health 200, the healthcheck command exit 0, and the container reporting healthy with FailingStreak=0 through four intervals. So the outage the issue describes never marked the container unhealthy in the first place — the check was a bare port probe. It now issues a real GET, verified to exit 8 against a 503 and 0 against liveness. Verified on a rebuilt image against a killable socat proxy in front of the real socket, so the host daemon and its unrelated containers were never touched, with the proxy's port confirmed closed before measuring: Docker reachable /health 200 healthy /health/ready 200 ready Docker unreachable /health 200 healthy /health/ready 503 degraded ["docker"] container health healthy, streak 0, across 110s / >3 intervals And over bridge networking, so the caller is genuinely non-loopback: default 403 on both endpoints HEALTH_ALLOWED_NETWORKS=172.16.0.0/12 200 Closes agent-os-69a Co-Authored-By: Claude Opus 5 --- .env.example | 10 + README.md | 44 +++- backend/cmd/server/main.go | 44 +--- backend/internal/config/config.go | 9 + backend/internal/handlers/health.go | 144 ++++++++++++ backend/internal/handlers/health_test.go | 277 +++++++++++++++++++++++ backend/internal/middleware/auth.go | 19 +- backend/internal/middleware/logging.go | 4 +- backend/internal/services/docker.go | 20 ++ docker-compose.prod.yaml | 5 +- docker/Dockerfile | 9 +- docker/compose.yaml | 5 +- 12 files changed, 544 insertions(+), 46 deletions(-) create mode 100644 backend/internal/handlers/health.go create mode 100644 backend/internal/handlers/health_test.go diff --git a/.env.example b/.env.example index 932e8cf..c675e1f 100644 --- a/.env.example +++ b/.env.example @@ -47,6 +47,16 @@ AUTH_DISABLED=false # Comma-separated CIDR ranges allowed when AUTH_DISABLED=true # TRUSTED_NETWORKS=172.16.0.0/12,10.0.0.0/8,192.168.0.0/16,127.0.0.1,::1 +# Comma-separated CIDR ranges, beyond loopback, allowed to reach /health +# (liveness) and /health/ready (readiness). Empty means loopback only, which is +# all the container's own HEALTHCHECK needs. Set this to let an external uptime +# monitor or load balancer probe Capstan. +# +# Deliberately separate from TRUSTED_NETWORKS: that value is also the +# trusted-proxy list and the AUTH_DISABLED bypass list, so reusing it would +# grant a monitor X-Forwarded-For spoofing and auth bypass just to read health. +# HEALTH_ALLOWED_NETWORKS=10.1.0.0/16 + # CORS origins (comma-separated, empty = same-origin only) # CORS_ORIGINS=https://your-domain.com diff --git a/README.md b/README.md index bc6c73c..c58a25e 100644 --- a/README.md +++ b/README.md @@ -574,11 +574,49 @@ npm run build # production build ## API Endpoints All routes are under `/api/v1` and require authentication (unless -`AUTH_DISABLED=true`), except `/health` and `/api/v1/version`. The web UI is the -primary interface; these are the main REST routes. +`AUTH_DISABLED=true`), except `/health`, `/health/ready` and `/api/v1/version`. +The web UI is the primary interface; these are the main REST routes. ### Health -- `GET /health` — health check (restricted to localhost) + +Liveness and readiness are separate endpoints, because Capstan is a separate +process from Docker. Point restart-on-failure checks at liveness and dependency +monitoring at readiness. + +- `GET /health` — **liveness**. 200 whenever the process is up and serving. Makes + no Docker call, so a Docker daemon restart cannot mark the container unhealthy + and get Capstan bounced for an outage restarting it would not fix. This is what + the container `HEALTHCHECK`, and any orchestrator liveness probe, should use. +- `GET /health/ready` — **readiness**. Reports dependencies, 503 when any is + degraded, naming which. The Docker probe is bounded by a 2-second timeout so a + hung daemon cannot pile up goroutines across repeated probes. Point an uptime + monitor, a load balancer, or a Kubernetes readiness probe here. + +```console +$ curl -s localhost:5001/health +{"status":"healthy"} + +$ curl -s localhost:5001/health/ready # Docker up +{"checks":{"docker":{"status":"ok"}},"status":"ready"} + +$ curl -s localhost:5001/health/ready # Docker daemon stopped -> HTTP 503 +{"checks":{"docker":{"error":"Cannot connect to the Docker daemon at unix:///var/run/docker.sock.", +"status":"unavailable"}},"degraded":["docker"],"status":"degraded"} +``` + +**Reachability.** Loopback reaches both with no configuration, so the container's +own healthcheck needs no setup. Every other address is denied by default — +upgrading does not silently widen exposure. To let an external monitor in, list +its network in `HEALTH_ALLOWED_NETWORKS`: + +```bash +HEALTH_ALLOWED_NETWORKS=10.1.0.0/16,192.168.50.7 +``` + +This is deliberately **not** `TRUSTED_NETWORKS`. That value is Gin's +trusted-proxy list *and* the `AUTH_DISABLED` bypass list, so reusing it would +mean granting an uptime monitor `X-Forwarded-For` spoofing and authentication +bypass just to let it read a health endpoint. ### Version - `GET /api/v1/version` — build identity of the running binary. Public, so an diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fba3c23..809d38b 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -7,7 +7,6 @@ import ( "fmt" "log" "log/slog" - "net" "net/http" "os" "os/signal" @@ -79,14 +78,6 @@ func SecurityHeaders(cfg *config.Config) gin.HandlerFunc { } } -func isLocalhost(c *gin.Context) bool { - ip := net.ParseIP(c.ClientIP()) - if ip == nil { - return false - } - return ip.IsLoopback() -} - func main() { // Bootstrap the logger from the environment before config.Load, so that // config's own startup lines — including the volume-path-identity warning — @@ -224,32 +215,15 @@ func main() { r.Use(middleware.ValidateInput()) r.Use(SecurityHeaders(cfg)) - r.GET("/health", func(c *gin.Context) { - if !isLocalhost(c) { - c.JSON(http.StatusForbidden, gin.H{"error": "Health endpoint restricted to localhost"}) - return - } - - dockerHealthy := false - - if dockerService != nil { - _, err := dockerService.GetContainerList("") - if err == nil { - dockerHealthy = true - } - } - - if !dockerHealthy { - c.JSON(http.StatusServiceUnavailable, gin.H{ - "status": "unhealthy", - }) - return - } - - c.JSON(http.StatusOK, gin.H{ - "status": "healthy", - }) - }) + // Liveness and readiness. dockerService is a typed nil when the daemon was + // unreachable at startup; hand the handler an untyped nil so it reports the + // outage rather than calling through a non-nil interface holding a nil + // pointer (agent-os-69a). + var dockerPinger handlers.DockerPinger + if dockerService != nil { + dockerPinger = dockerService + } + handlers.NewHealthHandler(dockerPinger, cfg.HealthNetworks).RegisterRoutes(r) api := r.Group("/api/v1") diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index a4915b2..9d06506 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -30,6 +30,14 @@ type Config struct { AuthDisabled bool CORSOrigins string TrustedNetworks string + // HealthNetworks lists the CIDRs, beyond loopback, that may reach /health + // and /health/ready. Deliberately separate from TrustedNetworks: that value + // is Gin's trusted-proxy list *and* the AUTH_DISABLED bypass list, so + // reusing it would force an operator to grant an uptime monitor + // X-Forwarded-For spoofing and auth bypass just to read a health endpoint + // (agent-os-69a). Empty means loopback only, which is the pre-split + // behaviour. + HealthNetworks string ExtraStacksDirs []string // Backup / restic env-var fallbacks (DB settings take precedence at runtime). @@ -57,6 +65,7 @@ func Load() (*Config, error) { GitHTTPSUser: "git", AuthDisabled: os.Getenv("AUTH_DISABLED") == "true", TrustedNetworks: os.Getenv("TRUSTED_NETWORKS"), + HealthNetworks: os.Getenv("HEALTH_ALLOWED_NETWORKS"), } if stacksDir := os.Getenv("STACKS_DIR"); stacksDir != "" { diff --git a/backend/internal/handlers/health.go b/backend/internal/handlers/health.go new file mode 100644 index 0000000..bc70a48 --- /dev/null +++ b/backend/internal/handlers/health.go @@ -0,0 +1,144 @@ +package handlers + +import ( + "context" + "log/slog" + "net" + "net/http" + "time" + + "github.com/gin-gonic/gin" + "github.com/thinkbig1979/capstan/backend/internal/middleware" +) + +// readinessProbeTimeout bounds the dependency check. A hung Docker daemon +// otherwise parks a goroutine per probe, and repeated probes pile them up. +const readinessProbeTimeout = 2 * time.Second + +// DockerPinger is the readiness handler's view of the Docker service: one cheap, +// context-aware round-trip. Narrow on purpose, so the handler is testable +// without a daemon. +type DockerPinger interface { + Ping(ctx context.Context) error +} + +// HealthHandler serves liveness and readiness. +// +// The split exists because Capstan is a separate process from Docker. The old +// combined endpoint returned 503 whenever the daemon was unreachable, so a +// daemon restart marked the container unhealthy and anything that restarts on +// failed health checks would bounce Capstan — which does nothing to fix Docker +// (agent-os-69a). +// +// GET /health liveness — the process is up and serving. No Docker call. +// GET /health/ready readiness — reports dependencies; 503 names what is degraded. +type HealthHandler struct { + // docker is nil when the Docker service could not be created at startup. + // That is a legitimate degraded state, not an error to hide. + docker DockerPinger + allowedNetworks string + probeTimeout time.Duration +} + +// NewHealthHandler builds the handler. Pass a nil docker when the Docker service +// is unavailable; readiness then reports Docker as degraded rather than +// pretending to be ready. +// +// allowedNetworks is HEALTH_ALLOWED_NETWORKS: comma-separated CIDRs, beyond +// loopback, permitted to reach either endpoint. Empty means loopback only, which +// is the pre-split behaviour — an upgrade must not silently widen exposure. +func NewHealthHandler(docker DockerPinger, allowedNetworks string) *HealthHandler { + return &HealthHandler{ + docker: docker, + allowedNetworks: allowedNetworks, + probeTimeout: readinessProbeTimeout, + } +} + +// RegisterRoutes mounts both endpoints on the root router. They sit outside the +// protected group deliberately: a probe has no session. Both paths are listed in +// middleware.PublicPaths so auth and CSRF agree with that. +func (h *HealthHandler) RegisterRoutes(r gin.IRoutes) { + r.GET("/health", h.Live) + r.GET("/health/ready", h.Ready) +} + +// allowed enforces the network policy shared by both endpoints, writing the 403 +// itself. Loopback always passes, so the container's own HEALTHCHECK needs no +// configuration. +func (h *HealthHandler) allowed(c *gin.Context) bool { + clientIP := c.ClientIP() + + // The whole loopback range, not just the literal 127.0.0.1/::1 that + // middleware.IsTrustedIP matches. The handler this replaced used + // net.IP.IsLoopback, and narrowing that here would deny a probe bound to, + // say, 127.0.0.2 that works today. + if ip := net.ParseIP(clientIP); ip != nil && ip.IsLoopback() { + return true + } + + if middleware.IsTrustedIP(clientIP, h.allowedNetworks) { + return true + } + c.JSON(http.StatusForbidden, gin.H{ + "error": "Health endpoint restricted; add this network to HEALTH_ALLOWED_NETWORKS to permit it", + }) + return false +} + +// Live answers whether the process is up and serving. It must not touch any +// dependency: the container HEALTHCHECK points here, and a dependency failure +// answered from this endpoint gets the container restarted for someone else's +// outage. +// +// The body keeps the pre-split {"status":"healthy"} so existing monitors that +// match on it keep working. What changed is that it no longer returns 503. +func (h *HealthHandler) Live(c *gin.Context) { + if !h.allowed(c) { + return + } + c.JSON(http.StatusOK, gin.H{"status": "healthy"}) +} + +// Ready reports the dependencies, 503 when any is degraded, naming which. +func (h *HealthHandler) Ready(c *gin.Context) { + if !h.allowed(c) { + return + } + + timeout := h.probeTimeout + if timeout <= 0 { + timeout = readinessProbeTimeout + } + ctx, cancel := context.WithTimeout(c.Request.Context(), timeout) + defer cancel() + + dockerCheck := gin.H{"status": "ok"} + var degraded []string + + switch { + case h.docker == nil: + degraded = append(degraded, "docker") + dockerCheck = gin.H{"status": "unavailable", "error": "docker service not initialized at startup"} + default: + if err := h.docker.Ping(ctx); err != nil { + degraded = append(degraded, "docker") + dockerCheck = gin.H{"status": "unavailable", "error": err.Error()} + } + } + + body := gin.H{ + "status": "ready", + "checks": gin.H{"docker": dockerCheck}, + } + + if len(degraded) > 0 { + body["status"] = "degraded" + body["degraded"] = degraded + slog.Warn("Readiness probe degraded", "degraded", degraded) + c.JSON(http.StatusServiceUnavailable, body) + return + } + + c.JSON(http.StatusOK, body) +} diff --git a/backend/internal/handlers/health_test.go b/backend/internal/handlers/health_test.go new file mode 100644 index 0000000..0704484 --- /dev/null +++ b/backend/internal/handlers/health_test.go @@ -0,0 +1,277 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/thinkbig1979/capstan/backend/internal/middleware" +) + +// fakePinger records how often it was called, so a test can assert that +// liveness touched no dependency at all. +type fakePinger struct { + calls atomic.Int32 + err error + // block, when non-nil, holds Ping until ctx is done or block is closed — + // the stand-in for a hung daemon. + block chan struct{} +} + +func (f *fakePinger) Ping(ctx context.Context) error { + f.calls.Add(1) + if f.block != nil { + select { + case <-f.block: + case <-ctx.Done(): + return ctx.Err() + } + } + return f.err +} + +func healthRouter(h *HealthHandler) *gin.Engine { + r := gin.New() + h.RegisterRoutes(r) + return r +} + +// requestFrom issues a GET to path as if it came from remoteIP. IPv6 literals +// need bracketing in RemoteAddr or net.SplitHostPort cannot parse them, and gin +// falls back to an empty ClientIP. +func requestFrom(r *gin.Engine, path, remoteIP string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, path, nil) + if strings.Contains(remoteIP, ":") { + req.RemoteAddr = "[" + remoteIP + "]:40000" + } else { + req.RemoteAddr = remoteIP + ":40000" + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func decodeHealthBody(t *testing.T, w *httptest.ResponseRecorder) map[string]any { + t.Helper() + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decoding %s: %v", w.Body.String(), err) + } + return body +} + +// ── Liveness ──────────────────────────────────────────────────────────────── + +// TestLivenessMakesNoDockerCall is the core of the split. The container +// HEALTHCHECK points here; if liveness consulted Docker, a daemon restart would +// mark the container unhealthy and get Capstan bounced for someone else's +// outage. +func TestLivenessMakesNoDockerCall(t *testing.T) { + docker := &fakePinger{} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", w.Code, http.StatusOK) + } + if got := docker.calls.Load(); got != 0 { + t.Errorf("liveness called Docker %d time(s); it must call it none", got) + } + if body := decodeHealthBody(t, w); body["status"] != "healthy" { + t.Errorf("status = %v, want %q", body["status"], "healthy") + } +} + +// TestLivenessStays200WhenDockerIsDown is the behaviour change the bead asks +// for: the old combined endpoint returned 503 here. +func TestLivenessStays200WhenDockerIsDown(t *testing.T) { + docker := &fakePinger{err: errors.New("cannot connect to the Docker daemon")} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d — a Docker outage must not mark the process dead", w.Code, http.StatusOK) + } +} + +// TestLivenessStays200WhenDockerServiceIsAbsent covers the nil case: the daemon +// was already unreachable when the process started. +func TestLivenessStays200WhenDockerServiceIsAbsent(t *testing.T) { + w := requestFrom(healthRouter(NewHealthHandler(nil, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", w.Code, http.StatusOK) + } +} + +// ── Readiness ─────────────────────────────────────────────────────────────── + +func TestReadinessReportsReadyWhenDockerAnswers(t *testing.T) { + docker := &fakePinger{} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d (body %s)", w.Code, http.StatusOK, w.Body.String()) + } + body := decodeHealthBody(t, w) + if body["status"] != "ready" { + t.Errorf("status = %v, want %q", body["status"], "ready") + } + if docker.calls.Load() != 1 { + t.Errorf("Docker pinged %d time(s), want 1", docker.calls.Load()) + } +} + +// TestReadinessNamesDockerWhenDegraded — 503 alone is not enough; an operator +// reading the probe output has to be able to tell *what* is degraded. +func TestReadinessNamesDockerWhenDegraded(t *testing.T) { + docker := &fakePinger{err: errors.New("cannot connect to the Docker daemon socket")} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + if body["status"] != "degraded" { + t.Errorf("status = %v, want %q", body["status"], "degraded") + } + + names, ok := body["degraded"].([]any) + if !ok || len(names) != 1 || names[0] != "docker" { + t.Fatalf("degraded = %v, want [docker] — 503 must name the failing dependency", body["degraded"]) + } + + checks, _ := body["checks"].(map[string]any) + dockerCheck, _ := checks["docker"].(map[string]any) + if dockerCheck["status"] != "unavailable" { + t.Errorf("checks.docker.status = %v, want %q", dockerCheck["status"], "unavailable") + } + if msg, _ := dockerCheck["error"].(string); msg == "" { + t.Error("checks.docker.error is empty; the reason is what makes the probe actionable") + } +} + +// TestReadinessDegradesWhenDockerServiceIsAbsent covers the nil-service branch. +// A nil *DockerService stored in an interface is a non-nil interface value, so +// this is also the regression guard against pinging through it and panicking. +func TestReadinessDegradesWhenDockerServiceIsAbsent(t *testing.T) { + w := requestFrom(healthRouter(NewHealthHandler(nil, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + names, ok := body["degraded"].([]any) + if !ok || len(names) != 1 || names[0] != "docker" { + t.Fatalf("degraded = %v, want [docker]", body["degraded"]) + } +} + +// TestReadinessTimesOutOnAHungDaemon: without a bound on the probe, repeated +// 30-second checks against a hung daemon pile up goroutines indefinitely. +func TestReadinessTimesOutOnAHungDaemon(t *testing.T) { + blocked := make(chan struct{}) + t.Cleanup(func() { close(blocked) }) + + h := NewHealthHandler(&fakePinger{block: blocked}, "") + h.probeTimeout = 50 * time.Millisecond + + done := make(chan *httptest.ResponseRecorder, 1) + go func() { done <- requestFrom(healthRouter(h), "/health/ready", "127.0.0.1") }() + + select { + case w := <-done: + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + if body["status"] != "degraded" { + t.Errorf("status = %v, want %q", body["status"], "degraded") + } + case <-time.After(3 * time.Second): + t.Fatal("readiness never returned against a hung daemon: the probe has no timeout") + } +} + +// ── Reachability ──────────────────────────────────────────────────────────── + +// TestLoopbackReachesBothWithNoConfiguration — the container's own HEALTHCHECK +// must work out of the box. +func TestLoopbackReachesBothWithNoConfiguration(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "")) + + for _, tc := range []struct{ path, ip string }{ + {"/health", "127.0.0.1"}, + {"/health/ready", "127.0.0.1"}, + {"/health", "::1"}, + {"/health/ready", "::1"}, + // The replaced handler used net.IP.IsLoopback, so the whole 127.0.0.0/8 + // range reached it. Keep that rather than narrowing to the literal + // 127.0.0.1 that middleware.IsTrustedIP matches. + {"/health", "127.0.0.2"}, + {"/health/ready", "127.0.0.2"}, + } { + if w := requestFrom(r, tc.path, tc.ip); w.Code != http.StatusOK { + t.Errorf("GET %s from %s: status = %d, want %d", tc.path, tc.ip, w.Code, http.StatusOK) + } + } +} + +// TestNonLoopbackDeniedByDefault pins the no-silent-exposure-change requirement: +// an upgrade with no new configuration must keep the pre-split restriction. +func TestNonLoopbackDeniedByDefault(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "")) + + for _, path := range []string{"/health", "/health/ready"} { + w := requestFrom(r, path, "10.1.2.3") + if w.Code != http.StatusForbidden { + t.Errorf("GET %s from 10.1.2.3: status = %d, want %d — the default must not widen exposure", + path, w.Code, http.StatusForbidden) + } + } +} + +// TestNonLoopbackAllowedOnceItsNetworkIsListed is the other half: the endpoint +// is reachable by an uptime monitor once its network is configured. +func TestNonLoopbackAllowedOnceItsNetworkIsListed(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "10.1.0.0/16")) + + for _, path := range []string{"/health", "/health/ready"} { + if w := requestFrom(r, path, "10.1.2.3"); w.Code != http.StatusOK { + t.Errorf("GET %s from 10.1.2.3 with 10.1.0.0/16 allowed: status = %d, want %d", + path, w.Code, http.StatusOK) + } + } + + // A host outside the listed range is still denied. + if w := requestFrom(r, "/health", "10.9.2.3"); w.Code != http.StatusForbidden { + t.Errorf("GET /health from 10.9.2.3: status = %d, want %d", w.Code, http.StatusForbidden) + } +} + +// TestDeniedRequestSkipsTheDockerProbe — a rejected caller must not be able to +// drive daemon round-trips. +func TestDeniedRequestSkipsTheDockerProbe(t *testing.T) { + docker := &fakePinger{} + requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "10.1.2.3") + + if got := docker.calls.Load(); got != 0 { + t.Errorf("denied request pinged Docker %d time(s), want 0", got) + } +} + +// TestHealthPathsArePublic keeps middleware.PublicPaths truthful: both routes +// are mounted outside the protected group, and that list is read by the CSRF +// middleware as well as auth. +func TestHealthPathsArePublic(t *testing.T) { + for _, path := range []string{"/health", "/health/ready"} { + if !middleware.IsPublicPath(path) { + t.Errorf("%s is served outside the protected group but is not in middleware.PublicPaths", path) + } + } +} diff --git a/backend/internal/middleware/auth.go b/backend/internal/middleware/auth.go index 945cdb4..8e788d2 100644 --- a/backend/internal/middleware/auth.go +++ b/backend/internal/middleware/auth.go @@ -20,7 +20,10 @@ var PublicPaths = []string{ // conversation needs to answer "what is running here?" without a session. // This list is consulted by the CSRF middleware too, so keep the two in step. "/api/v1/version", + // Liveness and readiness. Both carry their own network policy + // (HEALTH_ALLOWED_NETWORKS, loopback always allowed) rather than a session. "/health", + "/health/ready", } func IsPublicPath(path string) bool { @@ -32,16 +35,24 @@ func IsPublicPath(path string) bool { return false } -func isTrustedIP(clientIP string, trustedNetworks string) bool { +// IsTrustedIP reports whether clientIP is loopback or falls inside one of the +// comma-separated CIDRs (or literal addresses) in networks. Loopback is always +// allowed, and an empty list means loopback only. +// +// Two callers with deliberately different lists: the AUTH_DISABLED bypass uses +// TRUSTED_NETWORKS, the health endpoints use HEALTH_ALLOWED_NETWORKS. Only the +// matching logic is shared — see config.Config.HealthNetworks for why the lists +// are not. +func IsTrustedIP(clientIP string, networks string) bool { if clientIP == "127.0.0.1" || clientIP == "::1" || clientIP == "localhost" { return true } - if trustedNetworks == "" { + if networks == "" { return false } - for _, networkStr := range strings.Split(trustedNetworks, ",") { + for _, networkStr := range strings.Split(networks, ",") { networkStr = strings.TrimSpace(networkStr) if networkStr == "" { continue @@ -91,7 +102,7 @@ func AuthMiddleware(db *database.DB, jwtSecret string, authDisabled bool, truste return func(c *gin.Context) { if authDisabled { clientIP := c.ClientIP() - if !isTrustedIP(clientIP, trustedNetworks) { + if !IsTrustedIP(clientIP, trustedNetworks) { slog.Warn("Untrusted IP attempt with auth disabled", "ip", clientIP, "trusted_networks", trustedNetworks) c.JSON(403, models.NewAppError(403, "FORBIDDEN", "Authentication disabled - only local connections allowed")) c.Abort() diff --git a/backend/internal/middleware/logging.go b/backend/internal/middleware/logging.go index 48f7f75..ac7be57 100644 --- a/backend/internal/middleware/logging.go +++ b/backend/internal/middleware/logging.go @@ -13,7 +13,9 @@ func LoggingMiddleware() gin.HandlerFunc { start := time.Now() path := c.Request.URL.Path - if path == "/health" { + // Probes run on a timer; logging every one buries the real traffic. + // Exact matches, not a prefix, so a real route under /health* still logs. + if path == "/health" || path == "/health/ready" { c.Next() return } diff --git a/backend/internal/services/docker.go b/backend/internal/services/docker.go index 8428805..05f0648 100644 --- a/backend/internal/services/docker.go +++ b/backend/internal/services/docker.go @@ -51,6 +51,26 @@ func NewDockerService(cfg *config.Config) (*DockerService, error) { }, nil } +// Ping reports whether the Docker daemon is reachable, honouring ctx's deadline. +// +// It is the readiness probe's dependency check. Ping is a single cheap +// round-trip, unlike the GetContainerList("") the old inline /health handler ran +// every 30 seconds. +// +// The nil receiver and nil client are handled rather than dereferenced: main +// leaves dockerService nil when the daemon was unreachable at startup, and a nil +// *DockerService stored in an interface is a non-nil interface value — calling +// through it would panic instead of reporting the outage it exists to report. +func (s *DockerService) Ping(ctx context.Context) error { + if s == nil || s.client == nil { + return fmt.Errorf("docker client not initialized") + } + if _, err := s.client.Ping(ctx); err != nil { + return err + } + return nil +} + func (s *DockerService) Logs(stack models.Stack, tail int) (string, error) { args := s.buildComposeArgs(stack, "logs", []string{"--tail", fmt.Sprintf("%d", tail), "--timestamps"}) diff --git a/docker-compose.prod.yaml b/docker-compose.prod.yaml index 391a786..0b507aa 100644 --- a/docker-compose.prod.yaml +++ b/docker-compose.prod.yaml @@ -43,7 +43,10 @@ services: labels: - "com.centurylinklabs.watchtower.enable=true" healthcheck: - test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:5001/health"] + # Liveness (no Docker call), and a real GET rather than --spider: --spider + # sends HEAD, which has no route and falls through to the SPA catch-all, + # so it returned 200 even when GET /health returned 503 (agent-os-69a). + test: ["CMD", "wget", "--no-verbose", "--tries=1", "-O", "/dev/null", "http://localhost:5001/health"] interval: 30s timeout: 10s start_period: 5s diff --git a/docker/Dockerfile b/docker/Dockerfile index 85216cf..a4b8ceb 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -169,8 +169,15 @@ ENV PGID=1000 EXPOSE 5001 +# Liveness, not readiness: /health makes no Docker call, so a Docker daemon +# outage cannot mark this container unhealthy and get it restarted for an outage +# a restart would not fix (agent-os-69a). +# +# A real GET, not --spider. --spider sends HEAD, no HEAD route is registered, so +# every HEAD falls through to the SPA catch-all and returns 200 — the check +# passed even when GET /health returned 503, making it a bare port check. HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ - CMD wget --no-verbose --tries=1 --spider http://localhost:5001/health || exit 1 + CMD wget --no-verbose --tries=1 -O /dev/null http://localhost:5001/health || exit 1 ENTRYPOINT ["/entrypoint.sh"] CMD ["/app/server"] diff --git a/docker/compose.yaml b/docker/compose.yaml index e51b6ff..fb2d416 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -49,7 +49,10 @@ services: memory: 512M pids_limit: 100 healthcheck: - test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:5001/health"] + # Liveness (no Docker call), and a real GET rather than --spider: --spider + # sends HEAD, which has no route and falls through to the SPA catch-all, + # so it returned 200 even when GET /health returned 503 (agent-os-69a). + test: ["CMD", "wget", "--no-verbose", "--tries=1", "-O", "/dev/null", "http://localhost:5001/health"] interval: 30s timeout: 10s retries: 3