From b67b8cea8f7efd2f33f3212041329883f963ba4e Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 12 Oct 2026 16:28:33 +0200 Subject: [PATCH] =?UTF-8?q?hub=20v0.148.0=20(code):=20SECURITY=20=E2=80=94?= =?UTF-8?q?=20the=20customer=20list=20and=20box=20reports=20need=20the=20o?= =?UTF-8?q?perator=20key=20(R-951);=20R-950=20rotated;=20R-952,=20R-953=20?= =?UTF-8?q?filed=20(97=20->=2099=20rows)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- documentation/backlog/CLOSED-ITEMS.md | 10 +++ documentation/backlog/OPEN-ITEMS.md | 4 +- hub/CHANGELOG.md | 15 +++++ hub/README.md | 6 +- hub/internal/api/customers_auth_test.go | 82 +++++++++++++++++++++++++ hub/internal/api/handler.go | 29 +++++++++ 6 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 hub/internal/api/customers_auth_test.go diff --git a/documentation/backlog/CLOSED-ITEMS.md b/documentation/backlog/CLOSED-ITEMS.md index 9d0530ee..74792e7f 100644 --- a/documentation/backlog/CLOSED-ITEMS.md +++ b/documentation/backlog/CLOSED-ITEMS.md @@ -26,6 +26,16 @@ --- +## 2026-10-12 (evening) — security review: the two hub exposures + +Found by the security review of 2026-10-12 (`documentation/audits/security-review-2026-10-12/`). Both rows were born +closed. Private evidence (DooPlex only): `/mnt/5_hdd/felhom.eu/evidence/security-review-2026-10-12-PRIVATE/`. + +| Row | What | Closed | Evidence | +|---|---|---|---| +| **R-950** | **The hub's operator/global key was readable in public git history and still worked — anyone the hub's ingress let in could act as the operator for every box (including reading the vaulted break-glass credentials).** (P1) | CLOSED 2026-10-12 — **rotated** (operator GO in session): the runbook rotation in `runbooks/secrets.md` („Operator/global bearer key", steps 1–4). Before: the old key answered 200. After: new key 200, old key 403/401; all three boxes reported normally after the restart. The only consumer was `Deployment/hub`. Whether anyone used the old key before today is R-954's question. | PRIVATE/SR-01; `runbooks/secrets.md` step 5 note | +| **R-951** | **Three hub API read routes answered with no credential: anyone the hub's ingress let in could list every customer and read each box's full report.** (P1) | CLOSED 2026-10-12 — **fixed in hub v0.148.0**: the operator/global key only (no key, a box key or another household's key → 403); the operator response is unchanged. | `TestCustomersRoutes_NoCredentialIsRefused` (red-proved), `TestCustomersRoutes_BoxKeysAreRefused`, `TestCustomersRoutes_GlobalKeyUnchanged`; PRIVATE/SR-02; live check in `audits/security-review-2026-10-12/evidence/` | + ## 2026-10-12 (evening) — the evacuated disk, delivered rows, small fixes The full text of the rows below: `git show 6c2c3dd574:documentation/backlog/OPEN-ITEMS.md`. Evidence: `documentation/audits/answers-2026-10-12/` unless a row names another place. diff --git a/documentation/backlog/OPEN-ITEMS.md b/documentation/backlog/OPEN-ITEMS.md index e394d205..b7866006 100644 --- a/documentation/backlog/OPEN-ITEMS.md +++ b/documentation/backlog/OPEN-ITEMS.md @@ -213,10 +213,12 @@ stopping line that lies. | **R-724** | Monitoring & notifications | P3 | **[P3-LOW] The status pages disagree with each other in small ways a household notices.** MEASURED 2026-09-29 on a fresh box: Beállítások reads „Mentés ütemezés 02:30 / 03:00" while Biztonsági mentés says 02:30 / 03:30 / 04:15 / 04:30–08:30; Beállítások shows a raw `2026-09-29T19:22:30Z`; its „Helyi cím (LAN)" and „Átjáró" read „nem állapítható meg" on the page the household is asked to read out for remote help; the backups overview still says „Következő mentés — 0 órája" (the age of the LAST run, under the word "next" — noted 2026-09-14, never filed); the dashboard shows the backup at 19:33 where the backup pages say 21:33 (R-500, still reproducing). **FIXED 2026-09-30 (controller v0.283.0):** Beállítások shows the real schedule and a local time; the dashboard's last backup is local time (R-500); „az előző N órája készült" replaces „0 órája". **NARROWED — remaining:** „Helyi cím (LAN)" / „Átjáró" read through the file-sharing container, so a box without it says „nem állapítható meg" — not a text fix (the read needs another path). | **NARROWED — the LAN/gateway read only; owner: CC** **2026-10-06 night: confirmed not a controller-only fix** — the only host-network container is samba (`infra/samba.go:123`) and the agent's local API has no guest-network route; the fix is a new agent route (e.g. the PVE lxc interfaces read) + a controller reader — a two-repo design. | — | — | CC | | **R-285** | Monitoring & notifications | P4 | **A planned, supervised reinstall pages the operator as if the machine had died — there is no notion of expected downtime anywhere.** During the 2026-08-09 rehearsal the hub sent, all `status: sent` to the operator channel: `host_stale` 08:58 UTC, `node_stale` 09:00, **`host_down` 09:28 (error)**, **`node_down` 09:30 (error)**, `host_leaf_changed` 09:31, `host_recovered` 09:31, `node_recovered` 09:34, `offsite_delivery_stuck` 09:34 — eight operator mails for work that was deliberate, attended and announced. **This is the OPPOSITE gap from the one R-281 filed:** the alarms are not missing, they are indiscriminate. `host_stale` at 30 min and `host_down` at 60 min (`monitor/host_staleness.go:22-23`, `downAfter = 2 * threshold`) cannot distinguish a wiped-on-purpose box from a dead one, and `host_leaf_changed` firing on a reinstall is correct-but-expected. **Note the interaction with the mute used on 2026-08-09 evening:** blocking a customer silences everything, so today the only two settings are *page me for planned work* and *tell me nothing at all*. **What is owed is a middle:** a maintenance window, or an operator-set expected-downtime flag, that suppresses staleness and leaf-change while leaving genuine faults audible | **READY (M) — NEW 2026-08-09** | — | The evidence is the operator's mailbox plus `events`/`notification_log` for 2026-08-09 | CC | -## Hub & operator — 7 rows (P3 3, P4 4) +## Hub & operator — 9 rows (P3 5, P4 4) | ID | Category | Sev | What | State | Blocked on | Next action | Owner | |---|---|---|---|---|---|---|---| +| **R-952** | Hub & operator | P3 | **Nothing fails when a new hub `/api/v1` route is added without deciding who may call it — that is how three routes stayed open with no credential until v0.148.0 (R-951).** Fix: a test that reads the API route table from source (not by text search) and fails on any route that is not classified (public with a reason, operator key, host key, customer key); every non-public route gets a request with no credential and must be refused. | OPEN — filed 2026-10-12 by the security review; not built in v0.148.0 (the session could not write it) | — | Build the test in its own task, with a red-proof | CC | +| **R-953** | Hub & operator | P3 | **A full ArgoCD sync of the `felhom` app with prune would DELETE three live Secrets and so break the hub and Umami.** Measured 2026-10-12: `Secret/gitea-creds` (read by `Deployment/hub`), `Secret/umami-config` (read by `Deployment/umami` and `Deployment/umami-db`) and `Secret/healthchecks-config` (read by nothing) are live but no longer in git since R-925 de-gitted `felhom.secret.yaml`; ArgoCD marks all three `requiresPruning`. `Deployment/umami` is also OutOfSync (not pruning). Today only the build-deploy skill's „sync only the hub" line prevents it. Fix: mark the three Secrets `argocd.argoproj.io/compare-options: IgnoreExtraneous` (or move them out of the app's scope), decide whether `healthchecks-config` is still needed, and bring `Deployment/umami` back in sync. | OPEN — filed 2026-10-12 | — | Annotate or move the three Secrets; then a whole-app diff shows nothing to prune | CC | | **R-31** | Hub & operator | P3 | **[P2-HIGH] Offsite provisioning is synchronous with no status affordance.** Save runs the Hetzner sync in-request, so the request can hit the nginx 504 **while succeeding server-side**: the operator cannot tell failed from slow, and a retry races the first attempt. **MIGRATED FROM `ROADMAP.md` 2026-08-22 (R-369) — originally filed 2026-07-21, size M, roadmap state `idea`.** Moved verbatim; nothing added or reinterpreted. The roadmap keeps its copy as history, marked moved. | **OPEN — migrated from ROADMAP 2026-08-22, rank unchanged** **Re-ranked 2026-10-03: P2→P3: operator-only; a known workaround (click once, wait, verify) exists.** **2026-10-06 night: the race half fixed on felhom.eu main (hub, unreleased):** a second Save while the first still provisions is refused with 409 („already running — wait about a minute, then reload; do not save again"); nothing saved, nothing created; per customer, in memory. `TestProvision_R31_*`, red-proof `audits/night-burndown-2026-10-06/hub/R-31-red.txt`. LEFT: the async save with a status card (the escrow-card idiom). | — | Direction: make it async + a status card, reusing the proven **awaiting-card/poll idiom** (v0.138.0 escrow card). **Interim mitigation belongs in R-3 as an operator note: click once, wait, verify — do not re-click.** | CC | | **R-882** | Hub & operator | P3 | **Longhorn on DooPlex could not grow a volume online: its `instance-manager` (116 days up) called a host process that no longer existed** — `nsenter: cannot open /host/proc/196610/ns/mnt` on every expansion retry, and an offline growth was blocked by the expansion's own attachment ticket (found 2026-10-05 growing `hub-data` to 2 Gi). A restart of the instance-manager (operator-approved) fixed it: 77/77 volumes back `attached/healthy` in 110 s. **Why the cached PID went stale was not established** (likely a containerd/k3s or iscsid restart after the instance-manager started), so it will recur after the next such restart and stay invisible until a volume needs to grow. `audits/hub-db-offsite-2026-10-05/partA/step1-*.txt` | **OPEN** | — | Find which host process the PID was and whether Longhorn 1.10.x re-resolves it; until then, before growing any volume, check the instance-manager's age against the last k3s/containerd restart | operator | | **R-883** | Hub & operator | P3 | **8 DooPlex workloads run an image by a moving tag (`:latest` or none), so any pod restart is a silent upgrade.** Measured 2026-10-05: the Longhorn restart restarted zipline on `ghcr.io/diced/zipline:latest` (pull Always), which pulled 4.8.0; 4.8.0 refused its database (`cannot safely migrate from prisma to drizzle: expected migration 20260508022000 … was not applied`) and crash-looped. Fixed for zipline by pinning `4.7.0` (homelab-manifests `90f60e4`, `4c8ec7a`; 4.7.0 applied the four missing migrations; a dump from before is kept out-of-band). The other 7 were counted, not named or changed. **Checked from source 2026-10-05 (burn-down round 2):** homelab-manifests@87dfc29 still has moving tags (grep image lines without a numeric tag): admin-system/toolbox.yaml:12 nicolaka/netshoot:latest (a bare Pod); calibre-system/cwa.yaml:826 calibre-web-automated:dev; outline-system/outline.yaml:270 minio/minio:latest; tandoor-system/recipe-importer.yaml:26 gitea.dooplex.hu/admin/recipe-importer:latest (pull Always :27); adventurelog-system/adventurelog.yaml:100 and :256 adventurelog-backend/frontend:latest (pull Always :101,:257); jarrs-system/jarr- | **OPEN** | — | List the 7 (`kubectl get deploy,sts -A` images without a fixed tag), pin each to the running version, and let Renovate move them | operator | diff --git a/hub/CHANGELOG.md b/hub/CHANGELOG.md index c09e0505..d692e4df 100644 --- a/hub/CHANGELOG.md +++ b/hub/CHANGELOG.md @@ -1,3 +1,18 @@ +## v0.148.0 — SECURITY: the customer list and box reports need the operator key (2026-10-12) + +Security review 2026-10-12 (`documentation/audits/security-review-2026-10-12/`), R-951, P1. + +- **`GET /api/v1/customers`, `/customers/{id}` and `/customers/{id}/history` answered with no credential at all.** + They returned every customer's name, dashboard address and the stored box report to anyone the ingress let + through. Each handler now calls `requireGlobalKey` first (the `/admin/wg/*` pattern): no credential, a box's own + key, or another household's key → 403. With the operator/global key the response is unchanged. Nothing in the + product called these routes (searched controller, agent, hub, scripts, website). + `TestCustomersRoutes_NoCredentialIsRefused`, `TestCustomersRoutes_BoxKeysAreRefused`, + `TestCustomersRoutes_GlobalKeyUnchanged`. Red-proof: with the check removed from `handleCustomer`, the first one + failed (`GET /customers/c1` answered 200). +- **Not in this release:** the route guard (a test that fails when a new `/api/v1` route has no login decision) — + R-952, a later task. + ## v0.147.0 — the operator's answers S4, S6, S10, S29, S30, S32, S33 (2026-10-12) Operator rulings 2026-10-12 (`09` §3 decisions 196–233). Every item has a test seen failing first. diff --git a/hub/README.md b/hub/README.md index 60ba894c..a996051e 100644 --- a/hub/README.md +++ b/hub/README.md @@ -56,9 +56,9 @@ All API endpoints require `Authorization: Bearer ` (except `/healthz` a | Method | Path | Description | |--------|------|-------------| | `POST` | `/api/v1/report` | Controller pushes periodic status report (v0.28.0+ includes `app_telemetry` field) | -| `GET` | `/api/v1/customers` | List all customers with latest report summary | -| `GET` | `/api/v1/customers/{id}` | Get latest full report for a customer | -| `GET` | `/api/v1/customers/{id}/history?period=7d` | Get report history | +| `GET` | `/api/v1/customers` | List all customers with latest report summary — **global key only** (v0.148.0, R-951) | +| `GET` | `/api/v1/customers/{id}` | Get latest full report for a customer — **global key only** (v0.148.0, R-951) | +| `GET` | `/api/v1/customers/{id}/history?period=7d` | Get report history — **global key only** (v0.148.0, R-951) | The `POST /api/v1/report` handler (v0.4.0+) automatically parses the optional `app_telemetry` JSON array from the request body and stores it in `app_telemetry` / `app_log_issues` tables. Old controllers (no `app_telemetry` key) continue to work unchanged. diff --git a/hub/internal/api/customers_auth_test.go b/hub/internal/api/customers_auth_test.go new file mode 100644 index 00000000..b3ad4ec9 --- /dev/null +++ b/hub/internal/api/customers_auth_test.go @@ -0,0 +1,82 @@ +package api + +import ( + "net/http" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-hub/internal/store" +) + +// R-951 (security review 2026-10-12): GET /customers, /customers/{id} and /customers/{id}/history answered with NO +// credential and returned every customer's name, dashboard address and stored box report. Since v0.148.0 they are +// operator/global key only. Red-proof (2026-10-12): with the requireGlobalKey call removed from handleCustomer, +// TestCustomersRoutes_NoCredentialIsRefused failed (GET /customers/c1 answered 200). + +const customersTestName = "Teszt Csalad R935" + +// newCustomersHandler: customer c1 (key ckey) with one stored report and a host h1 (key hkey) under it; a second +// customer c2 (key c2key). +func newCustomersHandler(t *testing.T) (*Handler, *store.Store) { + t.Helper() + h, st := newTwoCustomerHandler(t) + if err := st.UpsertHost(&store.Host{HostID: "h1", CustomerID: "c1", APIKey: "hkey"}); err != nil { + t.Fatalf("UpsertHost: %v", err) + } + rr := do(h, http.MethodPost, "/report", "ckey", `{"customer_id":"c1","customer_name":"`+customersTestName+`"}`) + if rr.Code != http.StatusOK { + t.Fatalf("seed report: status = %d, body=%s", rr.Code, rr.Body.String()) + } + return h, st +} + +var customersRoutes = []string{"/customers", "/customers/c1", "/customers/c1/history"} + +// Scenario A — a stranger, no Authorization header. +func TestCustomersRoutes_NoCredentialIsRefused(t *testing.T) { + h, _ := newCustomersHandler(t) + for _, p := range customersRoutes { + rr := do(h, http.MethodGet, p, "", "") + if rr.Code != http.StatusUnauthorized && rr.Code != http.StatusForbidden { + t.Errorf("GET %s with no credential: status = %d, want 401/403", p, rr.Code) + } + if strings.Contains(rr.Body.String(), customersTestName) || strings.Contains(rr.Body.String(), `"c1"`) { + t.Errorf("GET %s with no credential: body names a customer: %q", p, rr.Body.String()) + } + } +} + +// Scenario B — a box's own keys (per-customer, per-host, another household's) never list customers. +func TestCustomersRoutes_BoxKeysAreRefused(t *testing.T) { + h, _ := newCustomersHandler(t) + for _, key := range []string{"ckey", "hkey", "c2key", "not-a-key"} { + for _, p := range customersRoutes { + if rr := do(h, http.MethodGet, p, key, ""); rr.Code != http.StatusForbidden { + t.Errorf("GET %s with box key %q: status = %d, want 403", p, key, rr.Code) + } + } + } +} + +// Scenario C — the operator key gets what v0.147.0 returned to anyone. +func TestCustomersRoutes_GlobalKeyUnchanged(t *testing.T) { + h, st := newCustomersHandler(t) + + rr := do(h, http.MethodGet, "/customers", globalKey, "") + if rr.Code != http.StatusOK || !strings.Contains(rr.Body.String(), customersTestName) { + t.Fatalf("GET /customers with global key: status = %d body=%s", rr.Code, rr.Body.String()) + } + + c, err := st.GetCustomer("c1") + if err != nil || c == nil { + t.Fatalf("GetCustomer: %v %v", c, err) + } + rr = do(h, http.MethodGet, "/customers/c1", globalKey, "") + if rr.Code != http.StatusOK || rr.Body.String() != c.ReportJSON { + t.Fatalf("GET /customers/c1 with global key: status = %d, body differs from the stored report", rr.Code) + } + + if rr = do(h, http.MethodGet, "/customers/c1/history", globalKey, ""); rr.Code != http.StatusOK { + t.Fatalf("GET /customers/c1/history with global key: status = %d", rr.Code) + } +} diff --git a/hub/internal/api/handler.go b/hub/internal/api/handler.go index e481f529..bab86f5e 100644 --- a/hub/internal/api/handler.go +++ b/hub/internal/api/handler.go @@ -230,6 +230,17 @@ func (h *Handler) checkAuthHost(r *http.Request) (hostID, customerID string, isG return host.HostID, host.CustomerID, false, true } +// requireGlobalKey answers 403 and returns false unless the request carries the operator/global key. +// The same check as the /admin/wg/* handlers (wg.go), in one place for routes that read every customer. +func (h *Handler) requireGlobalKey(w http.ResponseWriter, r *http.Request) bool { + _, _, isGlobal, ok := h.checkAuthHost(r) + if !ok || !isGlobal { + http.Error(w, "Forbidden: global key required", http.StatusForbidden) + return false + } + return true +} + // ServeHTTP routes API requests. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { path := strings.TrimPrefix(r.URL.Path, "/api/v1") @@ -2415,6 +2426,12 @@ func (h *Handler) handleEvent(w http.ResponseWriter, r *http.Request) { } func (h *Handler) handleCustomers(w http.ResponseWriter, r *http.Request) { + // R-951: operator/global key only. These routes return every customer's name, dashboard address and + // stored box report; until v0.148.0 they answered with no credential at all. A box's own key must not + // list customers either (TestCustomersRoutes_*). + if !h.requireGlobalKey(w, r) { + return + } customers, err := h.store.GetCustomers() if err != nil { h.logger.Printf("[ERROR] Failed to get customers: %v", err) @@ -2458,6 +2475,12 @@ func (h *Handler) handleCustomers(w http.ResponseWriter, r *http.Request) { } func (h *Handler) handleCustomer(w http.ResponseWriter, r *http.Request, customerID string) { + // R-951: operator/global key only. These routes return every customer's name, dashboard address and + // stored box report; until v0.148.0 they answered with no credential at all. A box's own key must not + // list customers either (TestCustomersRoutes_*). + if !h.requireGlobalKey(w, r) { + return + } customer, err := h.store.GetCustomer(customerID) if err != nil { h.logger.Printf("[ERROR] Failed to get customer %s: %v", customerID, err) @@ -2475,6 +2498,12 @@ func (h *Handler) handleCustomer(w http.ResponseWriter, r *http.Request, custome } func (h *Handler) handleCustomerHistory(w http.ResponseWriter, r *http.Request, customerID string) { + // R-951: operator/global key only. These routes return every customer's name, dashboard address and + // stored box report; until v0.148.0 they answered with no credential at all. A box's own key must not + // list customers either (TestCustomersRoutes_*). + if !h.requireGlobalKey(w, r) { + return + } period := r.URL.Query().Get("period") var since time.Duration switch period {