From fda0b130cd8ee19b23cb685a8bdcda842e8b040e Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 13 Jun 2026 17:44:49 +0200 Subject: [PATCH] audit: Tier-2 findings (stacks lifecycle, reconcile crash-safety, contracts) - CTRL-T2-1 (High): app.yaml persists Deployed:true before compose up -d -> ghost-deployed stuck stack on crash (verified by hand) - CTRL-T2-2/3 (Medium): no docker-compose exec timeout; protected-stack list has no fail-safe default - AGENT-T2-1..7 (Low/Info): PVE UPID node guard, WaitTask empty-status, recover side-effect, lane-panic, provision re-mint ordering, PBS WaitVerify - BUGHUNT H1/H2/H3/H4/H12 confirmed FIXED at eea235b - Contracts agentapi<->localapi (12 endpoints) + report<->hub: CLEAN Co-Authored-By: Claude Opus 4.8 (1M context) --- AUDIT-2026-06-13.md | 209 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 182 insertions(+), 27 deletions(-) diff --git a/AUDIT-2026-06-13.md b/AUDIT-2026-06-13.md index c2c9f88..1b8b4fc 100644 --- a/AUDIT-2026-06-13.md +++ b/AUDIT-2026-06-13.md @@ -19,8 +19,10 @@ - 17:25 — Phase 0 complete (both repos). Baselines below. - 17:35 — Tier 1 dispatched (4 parallel auditors): agent destructive-path, agent auth/authz, controller backup/crypto, controller web/auth/setup. All returned. - 17:50 — **Verified by hand**: CTRL-001 (import path traversal via manifest.AppName) — read restore.go:320-409 + manifest.go:36-42, confirmed no validator. AGENT-001 (inline wipe TOCTOU) — read disks.go:420-479, confirmed Format targets mutable req.Device. -- 17:55 — Wrote + ran failing evidence test `internal/appexport/traversal_audit_test.go` (CTRL-001). FAILS as expected. Committed Tier-1 findings (this checkpoint). -- (next) Tier 2: stacks deploy invariant, agentapi↔localapi contract diff, report↔hub contract. +- 17:55 — Wrote + ran failing evidence test `internal/appexport/traversal_audit_test.go` (CTRL-001). FAILS as expected. Committed + force-pushed Tier-1 checkpoint (`1f06029`). +- 18:15 — Tier 2 dispatched (3 parallel): controller stacks lifecycle, agent reconcile crash-safety + provision + proxmox/pbs, agentapi↔localapi contract diff. All returned. +- 18:30 — **Verified by hand**: CTRL-T2-1 (ghost-deployed on crash) — read deploy.go:288-358, confirmed `app.yaml` persists `Deployed:true` at :302 before `compose up -d` at :339; `Deploying` is in-memory only. BUGHUNT H1/H2/H3/H4/H12 all confirmed FIXED. agentapi↔localapi contract = CLEAN (12 endpoints diffed). Committed Tier-2. +- (next) Tier 3: templates/funcmap/XSS, goroutine-lifecycle races in surviving pkgs, `-race` run on build server, no-`:latest` sweep. ## Baseline (Phase 0) @@ -36,24 +38,28 @@ Both codebases are in good shape; the fail-safe/fail-destructive postures that matter most **hold**. No Critical found. The destructive-storage and operator-signature surfaces of the agent are unusually well-built (locked verify pipeline, hash-only token store, fsync-durable nonce store, narrow privileged fence, TLS pinning, fail-destructive ambiguity defaults — all confirmed). The controller's at-rest crypto (AES-256-GCM), restore data-key fail-closed gate, restic single-flight mutex, and (for the password-set path) full CSRF+auth coverage all hold. -The **one High** is a real path-traversal write primitive: the app-**import** path joins the attacker-controlled `manifest.AppName` from inside a `.fab` bundle straight into `filepath.Join`+`os.MkdirAll` with no validation (the archive-entry zip-slip guard exists, but the *stack-name* segment is unguarded). Verified by a failing evidence test. The remaining findings are Medium edge-cases (a TOCTOU between device-inspect and mkfs on the inline customer-confirmed wipe; decrypt-before-MAC writing transient unauthenticated plaintext) and a tail of Low/Info hardening items. +**Two Highs**, both verified by hand: (1) **CTRL-001** — the app-**import** path joins the attacker-controlled `manifest.AppName` from inside a `.fab` straight into `filepath.Join`+`os.MkdirAll` with no validation (the archive-entry zip-slip guard exists, but the *stack-name* segment is unguarded); confirmed by a failing evidence test. (2) **CTRL-T2-1** — `app.yaml` persists `Deployed:true` *before* `docker compose up -d` runs and `Deploying` is never persisted, so a crash in the pull window leaves a "ghost-deployed" stuck stack the customer can't redeploy. The remaining findings are Medium edge-cases (inline-wipe device-inspect→mkfs TOCTOU; decrypt-before-MAC transient plaintext; protected-stack list with no fail-safe default; no timeout on `docker compose` defeating the quiesce downtime bound) and a Low/Info tail. -This session covered **Tier 1 in full** for both repos plus the localapi↔agentapi contract. Tier 2/3 (stacks deploy invariants, reconcile crash-safety, report↔hub contract, templates) are largely **not yet covered** — see §"What was NOT covered". +Positive results worth recording: all five BUGHUNT stacks-concurrency Highs (H1–H4, H12) are **confirmed FIXED**; the controller↔agent (`agentapi`↔`localapi`, 12 endpoints) and controller↔hub (`report`↔ingest) contracts are **CLEAN**; agent crash-safety (`Recover()` ground-truth, defer-rollback, marker-before-mutate) and PBS/Proxmox TLS pinning **hold**. + +This session covered **Tier 1 and Tier 2 in full** for both repos plus both cross-repo contracts. **Tier 3** (templates/funcmap/XSS, goroutine-lifecycle races in surviving pkgs, `-race` run, no-`:latest` sweep) is **not yet covered** — see §"What was NOT covered". ## Top-10 action list | # | ID | Sev | Repo | Title | Effort | |---|---|---|---|---|---| -| 1 | CTRL-001 | **High** | controller | App-import path traversal via unvalidated `manifest.AppName` | S | -| 2 | AGENT-001 | Medium | agent | Inline customer-confirmed wipe formats mutable `/dev` path (classify→mkfs TOCTOU) | M | -| 3 | CTRL-002 | Medium | controller | FAB decrypt streams plaintext to disk *before* verifying HMAC tag | S | -| 4 | AGENT-003 | Medium | agent | `InspectDevice` swallows `blkid` error; `lsblk` is sole "probed" authority | S | -| 5 | AGENT-002 | Medium | agent | Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU | M | -| 6 | CTRL-005 | Low | controller | Decompression-bomb: import extraction has no size/count cap | S | -| 7 | CTRL-008 | Low | controller | `settings.json` (bcrypt hash + plaintext retrieval pw) written 0644 | S | -| 8 | CTRL-007 | Low | controller | Pre-auth setup CSRF is a forgeable double-submit cookie | M | -| 9 | AGENT-007 | Low | agent | Decommission durable-id scheme unvalidated → silent no-op intent | S | -| 10 | CTRL-009 | Low | controller | Login rate-limiter keys on spoofable `X-Forwarded-For` | S | +| 1 | CTRL-001 | **High** | controller | App-import path traversal via unvalidated `manifest.AppName` (verified-by-test) | S | +| 2 | CTRL-T2-1 | **High** | controller | `app.yaml` persists `Deployed:true` *before* `compose up -d` → ghost-deployed/stuck stack on crash | M | +| 3 | AGENT-001 | Medium | agent | Inline customer-confirmed wipe formats mutable `/dev` path (classify→mkfs TOCTOU) | M | +| 4 | CTRL-002 | Medium | controller | FAB decrypt streams plaintext to disk *before* verifying HMAC tag | S | +| 5 | CTRL-T2-3 | Medium | controller | Protected-stack list has no fail-safe default → empty config = nothing protected | S | +| 6 | AGENT-003 | Medium | agent | `InspectDevice` swallows `blkid` error; `lsblk` is sole "probed" authority | S | +| 7 | AGENT-002 | Medium | agent | Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU | M | +| 8 | CTRL-T2-2 | Medium | controller | No timeout on any `docker compose` exec → hung deploy/quiesce, defeats max-quiesce bound | M | +| 9 | CTRL-008 | Low | controller | `settings.json` (bcrypt hash + plaintext retrieval pw) written 0644 | S | +| 10 | CTRL-007 | Low | controller | Pre-auth setup CSRF is a forgeable double-submit cookie | M | + +(Low/Info tail: CTRL-005 decompression-bomb, CTRL-009 XFF rate-limit, CTRL-011 open-redirect, AGENT-007 decommission scheme, AGENT-004 no-timeout signed-op, AGENT-012 nonce-store growth, AGENT-T2-1..7, CTRL-T2-4/5/6 — all in the sections below.) --- @@ -77,6 +83,26 @@ Impact: Arbitrary-directory write as the controller process (config, restore Fix sketch: In `UnmarshalManifest` (or immediately after) reject `AppName` unless it matches a strict single-segment allowlist (`^[a-z0-9][a-z0-9-]*$`, no `/ \ . ..`). Apply the same to `ConfigFiles`/`VolumeNames`/`HDDSubdirs` entries used in joins. Verify: `cd controller && go test ./internal/appexport/ -run Traversal -v` → fails at this commit (parent-escape / deep-escape). Manual: craft a `.fab` with `"app_name":"../evil"`, import, observe `stackDir` outside `GetStacksBaseDir()`. +### [CTRL-T2-1] `app.yaml` persists `Deployed:true` *before* `compose up -d` → ghost-deployed/stuck stack on crash +Severity: High +Category: crash-safety / invariant-drift +Location: controller/internal/stacks/deploy.go:294-330, 337-358; manager.go:280-296 (commit eea235b) +Confidence: verified-static (read by hand) +Evidence: +```go +appCfg := &AppConfig{Deployed: true, DeployedAt: time.Now()...} // :295-300 +SaveAppConfig(stackDir, appCfg, ...) // :302 — DISK says deployed:true... +m.mu.Lock(); s.Deployed = true; s.AppConfig = appCfg; m.mu.Unlock() // :322-327 +go m.runComposeDeploy(req.StackName, stackDir, env, appCfg) // :330 — ...BEFORE compose runs +// runComposeDeploy: _, composeErr := m.composeExecWithEnv(stackDir, env, "up", "-d") // :339 +// revert (mem+disk) only if process is alive when composeErr returns // :344-356 +``` +Mechanism: The on-disk `app.yaml` records `deployed:true` before `docker compose up -d` is attempted. The only thing distinguishing "deploying" from "deployed" is `Stack.Deploying`, which is in-memory ONLY — there is no `deploying` field in `AppConfig`, so it is never persisted. The revert path (:344-356) runs only if the controller is alive when `composeErr` returns. A crash/OOM/host-reboot/self-update-restart during the up-window (image pull can take 30-60s per the code's own comment) skips the revert. On restart, `ScanStacks` reads `appCfg.Deployed==true` (manager.go:281) and marks the stack Deployed with zero live containers. This directly DRIFTS the controller invariant "`Deployed` set [true] only after `up -d`, reverted on failure (mem+disk)". +Trigger: Controller crash/restart during a deploy's compose-up window (large pull, host reboot, OOM, self-update restart). +Impact: Stack shows "deployed" (UI + quiesce/backup/memory accounting) but no containers exist. `DeployStack` then refuses redeploy ("already deployed; use update instead", deploy.go:133-135), so a non-technical customer is stuck — must Remove then redeploy, which they won't know to do → support burden + an app silently not running. +Fix sketch: Write `app.yaml` with `Deployed:false` (or add+persist a `deploying:true` field) before compose; flip to `Deployed:true` in `runComposeDeploy` only after success. Or on startup reconcile: a Deployed stack with zero containers + very recent `DeployedAt` → treat as failed-deploy. +Verify: Deploy a large app, `kill -9` the controller during the pull, restart → stack listed Deployed with no containers; DeployStack refuses redeploy. (Not unit-tested: needs Manager+provider+fs+crash sim; manual repro above.) + --- ## Findings — Medium / Low @@ -308,6 +334,105 @@ Impact: No unsafe wipe; possible user-data classification flapping. Fix sketch: On EvalSymlinks error for a system mount, treat the whole `SystemDisks` result as `ok=false` (all candidates → system). Verify: `SystemDisks` test with a `/boot` device that fails to resolve → assert `ok==false`. +### [CTRL-T2-3] Protected-stack list has no fail-safe default — empty/missing config = nothing protected +Severity: Medium +Category: security / invariant-drift +Location: controller/internal/config/config.go:251-301 (applyDefaults), 346-354 (IsProtectedStack) (commit eea235b) +Confidence: verified-static +Evidence: +```go +func (cfg *Config) IsProtectedStack(name string) bool { + for _, p := range cfg.Stacks.Protected { if strings.EqualFold(p, name) { return true } } + return false // empty list → everything unprotected +} +// applyDefaults sets ~40 defaults but NEVER seeds cfg.Stacks.Protected +``` +Mechanism: Protection is pure list-membership; `applyDefaults` never seeds the list. The set comes only from the setup wizard's generated yaml or the hub-pulled controller.yaml. If the hub template omits/empties `stacks.protected` (or a hand-edit/merge drops the key), `IsProtectedStack` returns false for everything, and the server-side guards (router.go:411-414, delete.go:86/289) all silently allow stop/remove/delete of traefik, cloudflared, filebrowser, and the controller itself. No fail-safe floor. +Trigger: Hub config-template regression, hand-edited controller.yaml, or a config merge dropping the key. +Impact: Loss of the core server-side protection invariant — a customer/API call could stop or remove infra (including the controller's own plane via traefik/cloudflared). +Fix sketch: In `applyDefaults`, if `len(cfg.Stacks.Protected)==0` seed `{traefik,cloudflared,felhom-controller,filebrowser}`; OR hardcode an always-on floor inside `IsProtectedStack` independent of config. +Verify: Load a controller.yaml with no `stacks:` block → `IsProtectedStack("traefik")` returns false. + +### [CTRL-T2-2] No timeout/context on any `docker compose` exec → hung deploy/stop, defeats quiesce max-downtime bound +Severity: Medium +Category: resource-leak / crash-safety +Location: controller/internal/stacks/manager.go:830-897 (composeExecCustomEnv); deploy.go:339; delete.go:137,339 (commit eea235b) +Confidence: verified-static +Evidence: +```go +if m.composeCmd == "docker compose" { cmd = exec.Command("docker", fullArgs...) } else { cmd = exec.Command("docker-compose", args...) } +if err := cmd.Run(); err != nil { ... } // bare exec.Command, no context, no deadline — can block forever +``` +Mechanism: Every compose call (deploy/start/stop/down/restart/update/delete) uses `exec.Command` with no `CommandContext`/deadline — while `getDirSizeBytes` (delete.go:589) DOES use a 30s context, proving the pattern is known. A hung `docker compose down`/`up`/`pull` blocks the calling goroutine forever, holding the `Deploying`/`infraMu`/lane locks. For quiesce: `StopStack→down` hanging blocks inside the stop loop; the max-quiesce deadline is checked only AFTER stops complete (quiesce.go:243), so a hung stop strands the app down past the downtime bound. +Trigger: docker daemon stall, registry hang during pull, a container ignoring SIGTERM. +Impact: Deploy/stop/start hang indefinitely; subsequent ops of that class blocked; quiesce's max-downtime guarantee bypassed. +Fix sketch: Add `composeExecCtx(ctx, ...)` via `exec.CommandContext` with per-op deadlines (deploy/pull ~10m, down/stop ~2m); plumb the quiesce ctx through StopStack/StartStack. +Verify: `docker compose` shim that sleeps forever → deploy/stop never returns; quiesce exceeds max-quiesce with the app down. + +### [CTRL-T2-4] Async-deploy disk revert happens outside the lock → narrow window for a stale-disk ScanStacks to resurrect Deployed +Severity: Low +Category: concurrency +Location: controller/internal/stacks/deploy.go:341-357; manager.go:287-296 (commit eea235b) +Confidence: suspected-needs-runtime +Evidence: In-memory revert clears `Deployed/Deploying` under `m.mu` (:344-350), but the disk revert `SaveAppConfig` runs AFTER `m.mu.Unlock()` (:353-356). A 2-min `stack-scan` landing in that gap reads stale `deployed:true` from disk, and since `Deploying` is now false its guard `if !existing.Deploying` (manager.go:293) lets it overwrite in-memory `Deployed` back to true until the next scan. +Impact: Transient ghost-deployed after a failed deploy; self-heals next scan. +Fix sketch: `SaveAppConfig` the reverted config BEFORE releasing the lock (or hold a deploy-scoped guard until disk is consistent). +Verify: Inject a SaveAppConfig delay on the revert path; fire ScanStacks in the gap. + +### [CTRL-T2-5] `getDirSizeHuman` (`du -sh`) has no timeout — hangs delete/remove on a slow/stale mount +Severity: Low +Category: resource-leak +Location: controller/internal/stacks/delete.go:573-585 (commit eea235b) +Confidence: verified-static +Evidence: `getDirSizeBytes` (delete.go:589) wraps `du -sb` in a 30s context; its sibling `getDirSizeHuman` (called on every delete/remove + HDD-data listing) uses bare `exec.Command("du","-sh",path)`. Traversal is already mitigated (ParseComposeHDDMounts cleans + ProtectedHDDPaths gate), so hang/DoS only. +Fix sketch: Give `getDirSizeHuman` the same `exec.CommandContext` 30s deadline. +Verify: Point an orphan's bind at a slow FUSE mount; call delete; handler blocks. + +### [AGENT-T2-1] PVE `WaitTask`/`TaskStatusOnce` never validate the UPID node (PBS side does) +Severity: Low +Category: contract-mismatch +Location: agent/internal/proxmox/task.go:70-81,110-114; upid.go:29-60 (commit d17b5ab) +Confidence: verified-static +Evidence: `ParseUPID` accepts an empty `parts[1]` → `u.Node==""` → request to `/nodes//tasks/...`; PVE queries `u.Node` not its pinned `c.node`. The sibling PBS client guards `node==""` (client.go:128-132); PVE does not. In Recover an unreadable status is treated fail-safe (left in-flight), bounding blast radius. +Fix sketch: After `ParseUPID`, reject `u.Node==""` and assert `u.Node==c.node`, mirroring PBS. +Verify: `TaskStatusOnce(ctx,"UPID::0:0:0:x:9:root@pam:")` → expect a node-validation error. + +### [AGENT-T2-2] `WaitTask` treats empty task status as "running" → burns full 10m timeout on a persistently-empty status +Severity: Low +Category: error-handling +Location: agent/internal/proxmox/task.go:134-142 (commit d17b5ab) +Confidence: verified-static +Evidence: `if st.Running() || st.Status == "" { ...backoff; continue }` — a 200 decoding to an empty `Status` polls until `opts.Timeout`. The per-guest queue lane is serial, so one stuck wait stalls that guest's later ops (≤10m). +Fix sketch: Cap consecutive empty-status polls (e.g. 5) → return a distinct "status never materialized" error. +Verify: Mock `TaskStatusOnce` returning `{Status:""}`; assert `WaitTask` returns before full Timeout. + +### [AGENT-T2-3] Recover marks a no-UPID in-flight op `failed` without verifying the POST didn't land +Severity: Low +Category: crash-safety +Location: agent/internal/reconcile/recover.go:56-63; engine.go:200-203 (commit d17b5ab) +Confidence: verified-static +Evidence: Journal order is `OpStarted`→POST→`OpTaskRunning(upid)`. A crash after the POST returns but before `OpTaskRunning` leaves an `OpStarted`-only entry whose mutation may already have taken effect; Recover assumes "never confirmed → never happened" and records `failed`. Benign for convergent reconcile ops (re-planned next pass), but the `marker-before-mutate` invariant does not strictly hold for the POST's task-id marker. +Fix sketch: For non-convergent/one-shot kinds, query live `GuestStatus/GuestConfig` before recording terminal (as Scratch/Rollback already do). +Verify: Simulate an `OpStarted`-only `ActionStart` entry; confirm Recover consults live run-state (currently it does not). + +### [AGENT-T2-5] Provision re-mint revokes the old token before the mount attach can fail (no transactional rollback) +Severity: Low +Category: resource-leak / correctness +Location: agent/internal/provision/backhalf.go:106-152 (commit d17b5ab) +Confidence: verified-static +Evidence: `Mint` is last-write-wins and revokes the guest's previous token immediately; if a re-provision's later `pct set` attach fails, the old token is already revoked while the new mount was never attached → guest left with no working local-API credential until a successful re-run. No `defer` rollback across mint↔attach. (First-time provision unaffected; file is 0600, no leak.) +Fix sketch: Mint AFTER the mount is prepared/attached, or log a WARN on attach-failure-after-remint so the revoked-token state is visible. +Verify: Inject a `pct set` failure on a 2nd Provision for the same VMID; confirm the prior token is already revoked. + +### [AGENT-T2-6] A panic in a queued reconcile job crashes the whole agent (no `recover()` in the lane) +Severity: Low +Category: crash-safety +Location: agent/internal/reconcile/bringup.go:148-155,181-187; queue.go run() ~line 135 (commit d17b5ab) +Confidence: verified-static +Evidence: The bring-up rollback `defer` fires on panic (guest is destroyed — good), but the panic then propagates out of `lane.run`'s `t.res <- t.fn()` with no `recover()`, taking down the process. On restart `Recover()` reaps the half-built guest, so no guest leak — but one job's panic = full agent outage. +Fix sketch: Wrap `t.fn()` in `lane.run` with `defer recover()` converting a panic into an error on `t.res`, isolating it to that op. +Verify: Submit a panicking job; assert the queue delivers an error and other lanes keep running. + --- ## Findings — Info @@ -354,16 +479,34 @@ Severity: Info • Category: security • Location: proxmox/mutate.go:105- Severity: Info • Category: security • Location: storage/hostops.go:64-69,449-453; reconcile/classify.go:99-112; gate.go:146-150 (d17b5ab) Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` → every device data-bearing; unknown op class → Destructive; nil verifier → refuse `pending_signature`; role default → system. The dangerous direction (ambiguity→allow-wipe) is the default nowhere. This downgrades AGENT-002/003 from Critical to Medium. +### [CTRL-T2-6] `EnsureBaseStack` idempotency is per-stack — only filebrowser preserves its compose; traefik/cloudflared re-render +Severity: Info • Category: error-handling • Location: controller/internal/stacks/infra.go:27-148 (eea235b) +Invariant #3 HOLDS for the load-bearing concern: EnsureBaseStack is non-fatal (joined error to LOG, never panics, infra.go:67-69) + single-flight (TryLock), and filebrowser explicitly does NOT regenerate its docker-compose.yml when present (infra.go:122-132), preserving web.SyncFileBrowserMounts' storage mounts. Drift note: traefik/cloudflared guard only on `containerRunning()` and re-render+overwrite their rendered files when the container is stopped-but-present (acme.json preserved separately) — so "idempotent = doesn't overwrite" is true only for filebrowser. No correctness bug; the contract is narrower than the doc comment implies. + +### [AGENT-T2-4] `pbs.WaitVerify` returns nil on ANY stopped state, incl. a task-level FAILED verify +Severity: Info • Category: contract-mismatch • Location: agent/internal/pbs/client.go:155-172 (d17b5ab) +`WaitVerify` resolves nil the moment the task is not running, never consulting `st.OK()`/`ExitStatus`. By design — the caller re-lists snapshots for the authoritative per-snapshot `VerifyState==failed` corruption signal (verify.go:107-126, documented at client.go:143-144). Gap: a verify task that fails to *run at all* (vs finding corruption) is logged only as wait-success. Fix: return a sentinel when `!st.OK()` so a task-level failure is distinct from "verified clean". + +### [AGENT-T2-7] LIVE WaitTask validation gap — characterized, deliberately NOT closed +Severity: Info • Category: test-gap • Location: agent/internal/proxmox/task.go:104-156; errors.go:66-73 (d17b5ab) +The "POST 200 ≠ success; authz can fail at task execution" contract is exercised only against the mock, not a live PVE token whose role is missing a privilege (task stops non-OK with a 403 exitstatus). The non-OK→`*TaskError` privilege-extraction regex (`permRe`) matches the documented "Permission check failed (path, Priv)" form, but whether PVE emits that exact wording in the *task exitstatus* (vs only an HTTP 403 body) for the agent's async ops is unverified live. Fail-safe either way (non-OK → error), only the structured diagnostic degrades. LEFT OPEN per the audit's hard-rule #2 (do not close the live WaitTask gap). Close only by a live under-privileged-token run on felhom-pve. + +### [CONTRACT-1] agentapi client omits the `snapshotted` phase constant (cosmetic) +Severity: Info • Category: contract-mismatch • Location: CLIENT controller/internal/agentapi/client.go:163-168 ↔ SERVER agent/internal/localapi/server.go:109-115 (emits it at server.go:490) +The agentapi package exports `PhaseIdle/Running/Done/Failed` but not `PhaseSnapshotted`, though the server's `/backup/status` emits `snapshotted` (8B.2 early-resume). No runtime impact: the actual consumer (quiesce loop) defines its own literal `phaseSnapshotted="snapshotted"` and matches the wire string directly (quiesce.go:47,265-266). Fix: add the constant for completeness. + --- ## Contract checks (controller↔agent, controller↔hub) | Contract | Status | Note | |---|---|---| -| agent `internal/localapi` ↔ controller `internal/agentapi` | **partially checked — clean so far** | Auth auditor walked all 14 localapi routes; self-scoping + envelope shape consistent. Full field-by-field JSON-tag diff vs `agentapi` NOT yet done — see "NOT covered". | -| controller `internal/report/types.go` ↔ hub ingest | **NOT checked** | Deferred to Tier 3. | +| controller `internal/agentapi` ↔ agent `internal/localapi` | **CHECKED — CLEAN** | All 12 client methods diffed field-by-field against their agent handlers + req/resp structs. Every route+method aligns; every request field present in the server struct (survives server `DisallowUnknownFields`); every response field the client reads is populated with the matching tag; status codes mapped correctly incl. destructive `/disks/format` 403 paths (needs_confirmation vs pending_op); error envelope `{ok,data,error}` shapes match; TLS leaf-SHA-256 pin enforced fail-closed. Only divergence = CONTRACT-1 (Info, cosmetic phase constant). | +| controller `internal/report/types.go` ↔ hub ingest | **CHECKED — CLEAN** | Hub `/api/host-report` stores the body as raw `ReportJSON` and structurally re-parses only `customer_id, controller_version, controller_url, customer_name, app_telemetry[]` — all re-parsed tags match controller `report.Report`/`AppTelemetry`/`metrics.LogIssue`. Tolerant ingest, no strict-struct mismatch. | | Operator-signature namespace `felhom-op-v1` (agent verifier) ↔ opsign tool | clean | verifier.go:99-181 fixed namespace; `cmd/felhom-opsign` present. | +**Endpoint coverage (client method → route → server handler):** Storage, BackupDue, StartBackup, BackupStatus, RestoreTestStatus, HostMetrics, Disks, AssignDisk, GuestAttach, GuestReboot, EjectDisk, FormatDisk — all CLEAN. Server-only `/snapshot`, `/rollback` are not called by this client (not mismatches). Full table in the contract auditor's notes. + ## Invariant checklist results **Agent** @@ -380,15 +523,24 @@ Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` **Controller** - No `:latest` anywhere — **NOT fully verified** (Tier 3 templates/infra sweep pending; spot-check clean). - Secrets never logged (keys only) — **HOLDS** spot-checked (handlers.go:56 logs presence only); CTRL-008 is at-rest file mode, not logging. -- Protected stacks unstoppable server-side — **NOT verified** (Tier 2, stacks pkg). -- `Deployed` set before `up -d`, reverted on failure (mem+disk) — **NOT verified** (Tier 2, stacks/deploy.go). +- Protected stacks unstoppable server-side — **HOLDS for the checks** @ router.go:411-414, manager.go:683-685, delete.go:86-88/289-291 (every mutating path checks); **DRIFTED on the data source** → CTRL-T2-3 (no fail-safe default; empty list = nothing protected). Restart on protected stacks intentionally allowed. +- `Deployed` set [true] only after `up -d`, reverted on failure (mem+disk) — **DRIFTED** → CTRL-T2-1 (disk persists `deployed:true` before compose; `Deploying` not persisted → ghost on crash) + CTRL-T2-4 (revert disk-write outside lock, Low). Happy/failure paths consistent *while the process lives*. - restic behind running mutex — **HOLDS** @ backup.go:144-148, restore.go:31-42 (restic moved to agent; all entry points single-flight). -- filebrowser compose preserved if present — **NOT verified** (Tier 2). -- `EnsureBaseStack` non-fatal+idempotent — **NOT verified** (Tier 2). +- filebrowser compose preserved if present — **HOLDS** @ infra.go:122-132 (does not regenerate when present). +- `EnsureBaseStack` non-fatal+idempotent — **HOLDS** @ infra.go:27-71 (non-fatal + TryLock single-flight); per-stack idempotency note CTRL-T2-6. - CSRF on every state-changing route incl. setup — **HOLDS for runtime mux** (password-set), full route×CSRF/auth table in session notes; **setup uses weaker CSRF** → CTRL-007. - Restore data-key fail-closed gate — **HOLDS** @ appbackup/restore_unit.go:38-50,113-116. - At-rest crypto AEAD — **HOLDS** @ crypto/crypto.go:50-61 (AES-256-GCM, fresh nonce). FAB export Encrypt-then-MAC HOLDS w/ CTRL-002 caveat. +**BUGHUNT (2026-02-25, v0.30.3) regression status — stacks concurrency items re-checked at eea235b:** +- H1 (double-deploy TOCTOU) — **FIXED** @ deploy.go:122-139 (atomic check-and-set of `Deploying` under one lock). +- H2 (delete-during-deploy) — **FIXED** @ delete.go:106-108, 309-311 (both reject when `Deploying`). +- H3 (ScanStacks overwrites Deployed during async deploy) — **FIXED** @ manager.go:291-296 (skips overwrite when `existing.Deploying`). +- H4 (shared appCfg pointer mutated by async goroutine) — **FIXED** @ deploy.go:344-353 (mutated under lock; in-mem `AppConfig=nil`). Residual disk-revert-outside-lock = CTRL-T2-4 (Low). +- H12 (deepCopyStack incomplete) — **FIXED** @ manager.go:556-638 (now deep-copies Containers, AppConfig+Env+LockedFields, HealthProbe, DeployFields+Options, OptionalConfig+Fields, Integrations, HealthCheck). +- Agent provision invariants: no `pct exec` — **HOLDS** (grep clean); bootstrap.json chown 100000:100000 — **HOLDS** @ backhalf.go:32,137. +- Agent crash-safety: marker-before-mutate — **MOSTLY HOLDS** (OpStarted pre-POST, fsync'd; task-id marker post-POST → AGENT-T2-3 Low); `Recover()` ground-truth via live `ListLXC` — **HOLDS** @ recover.go:103,163; defer-unquiesce/rollback — **HOLDS** @ bringup.go:182-187 (lane-panic isolation gap AGENT-T2-6). PBS fingerprint pinning — **HOLDS** @ pbs/pin.go:22-48. + ## Refactor & shared-code opportunities - **Durable-resolve-before-mutate helper (agent):** AGENT-001/002 + AGENT-007 all stem from formatting/recording a device by a *path/string* instead of re-resolving the *durable id* at the moment of action. A single `resolveAndReinspect(durableID) (path, probe, error)` used by both the inline localapi format path and `WipeExecutor` would make "act on exactly the confirmed device" structural. @@ -401,7 +553,9 @@ Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` - **appexport import path** — ZERO tests in the package (no `_test.go` existed before this audit's evidence test). The highest-severity finding (CTRL-001) lived in untested code. Import/restore/crypto-decrypt all lack unit coverage. - **agent inline format/wipe path** (localapi/disks.go format handler) — needs a swap-device-between-inspect-and-format test (would catch AGENT-001/002). - **storage classify under inspection error** — no test for blkid-error/lsblk-success (AGENT-003). -- Stacks deploy crash-safety, reconcile Recover() ground-truth — coverage not yet assessed (Tier 2). +- **stacks deploy crash-safety** — no test covers a crash between `app.yaml` write and compose success (CTRL-T2-1); the ghost-deployed state is untested. Needs a Manager+provider+fs harness or the manual repro given. +- **agent proxmox WaitTask** — `task_test.go` covers the mock happy/non-OK paths but not empty-node UPID (AGENT-T2-1) or persistent-empty-status (AGENT-T2-2); the live non-OK exitstatus wording is unverified (AGENT-T2-7, deliberately left open). +- **agent reconcile Recover()** ground-truth IS exercised (recover.go uses live `ListLXC`), but the no-UPID-in-flight side-effect-verification path (AGENT-T2-3) and lane-panic isolation (AGENT-T2-6) are untested. ## Dead code inventory (staticcheck U1000, controller) @@ -422,10 +576,11 @@ Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` ## What was NOT covered (defines next session) -Tier 1 is complete for both repos + the localapi route walk. NOT yet done: -- **Tier 2 agent:** `internal/reconcile` engine/plan/queue/journal/**recover**/normalize/bringup — crash-safety (marker-before-mutation, `ListLXC` ground truth in `Recover()`, defer-unquiesce); `internal/provision` back-half (token mint, bootstrap.json chown 100000:100000, no `pct exec`); `internal/proxmox` task/upid/WaitTask parsing; `internal/pbs` fingerprint pinning + verify semantics. -- **Tier 2 controller:** `internal/stacks` (Deployed-before-up-d invariant + rollback in mem+disk, protected-stack server-side enforcement, EnsureBaseStack idempotent, filebrowser-compose preserve) — these are the highest-value un-audited invariants; `internal/quiesce`, `internal/selfupdate`, `internal/recovery`, `internal/agentapi` (TLS/fingerprint, timeouts, error mapping). -- **Tier 3 contracts:** field-by-field `agentapi`↔`localapi` JSON-tag/status-code diff; `report/types.go`↔hub ingest types; templates funcmap completeness + XSS + leftover-emoji. -- **Tier 3 cross-cutting:** goroutine/ticker lifecycle & shared-state races (BUGHUNT flagged many in surviving pkgs — scheduler late-registration M10, watchdog deleted); panic isolation in scheduler/loop jobs; os/exec timeout hygiene across `docker`/`pct` calls. -- **`-race` run:** not executed (would need the build server; CGO/sqlite on Windows). Recommended next session in a throwaway dir on 192.168.0.180. +Tiers 1 & 2 are complete for both repos + both cross-repo contracts. NOT yet done (defines the next session, roughly in priority order): +- **Tier 3 templates (controller):** funcmap completeness (every template func referenced exists), XSS via any raw-HTML/`json` escape of user-influenced data, every container state has a label/color/icon, leftover emoji vs the minimal-UI rule, dead template blocks. `internal/web/funcmap.go` + `templates/`. +- **Tier 3 cross-cutting concurrency:** goroutine/ticker lifecycle & shared-state races in the surviving pkgs. BUGHUNT flagged several still-live ones NOT re-checked this session — **M10 scheduler late-registration** (jobs added after Start() never run), **M2 stackProvider unlocked read**, **M3 DrainPendingEvents loses events on save fail**, **M22 assets syncer holds mutex during network I/O**, **M13 SQLite WAL not verified**, **M14 sampleContainers uses context.Background**. Confirm FIXED/PRESENT at eea235b. +- **Tier 3 panic isolation:** does one bad scheduler/loop job kill the process? (agent lane-panic AGENT-T2-6 is the analogue — check controller `scheduler.go`.) +- **`-race` run:** not executed (needs the build server; CGO/sqlite won't `-race` on this Windows box). Recommended next session in a throwaway `mktemp -d` on `kisfenyo@192.168.0.180`, then delete. +- **Remaining Tier-1/2 pkgs not deeply read:** controller `internal/selfupdate` (signed-update verification, restart safety), `internal/notify`, `internal/integrations` (compose patching — OnlyOffice→FileBrowser/Nextcloud), `internal/cloudflare` (WAF rule identity), `internal/metrics` (SQLite stmt/conn handling). Agent `internal/lanresolver`, `internal/hub`, `internal/desired`, `internal/config`. +- **Evidence tests:** only CTRL-001 has a failing test. Cheap wins for next session: AGENT-T2-1 (empty-node UPID — pure unit test), AGENT-001/002 (swap-device-between-inspect-and-format), AGENT-003 (blkid-error/lsblk-success classification). - **Live read-only inspection** (docker logs/df/findmnt on demo) — skipped; static-only this session.