618 lines
60 KiB
Markdown
618 lines
60 KiB
Markdown
> **REMEDIATION STATUS — updated 2026-06-13 (controller v0.59.0, deployed to demo guest 9201).**
|
||
> This is the authoritative copy of the deep-sweep audit; the validated findings have been actioned.
|
||
>
|
||
> | Finding | Status | Commit |
|
||
> |---|---|---|
|
||
> | CTRL-001 (import path traversal) | **FIXED** | controller `c20ff56` (v0.59.0) |
|
||
> | CTRL-T2-1 (ghost-deployed on crash) | **FIXED** | controller `5a80739` (v0.59.0) |
|
||
> | AGENT-001 (wrong-disk wipe TOCTOU) | **MERGED + DEPLOYED** (agent **v0.30.0**; verified non-destructively on demo, no rollback) | agent `main` `8572a0f` (merge of `d96e5bd`; REPORT `1c37a16`) |
|
||
> | CTRL-T2-2/3, AGENT-002/003, CTRL-002, and the Low/Info tail | OPEN (backlog) | — |
|
||
>
|
||
> All other findings remain as recorded below (severity/evidence unchanged). See also the BUGHUNT
|
||
> reconciliation (`bughunt-reconcile-2026-06-13.md`) and the merged fix list therein.
|
||
|
||
# AUDIT — felhom-controller + felhom-agent deep sweep — 2026-06-13
|
||
|
||
**Branch (both repos):** `audit/2026-06-13-deep-sweep` (off latest `main`)
|
||
**Auditor:** Claude Code (Opus 4.8, unattended overnight session)
|
||
**Mode:** read-only, evidence-based. No fixes applied. No mutations, deploys, or builds.
|
||
|
||
| Repo | HEAD commit | Version | Note |
|
||
|---|---|---|---|
|
||
| felhom-controller | `eea235bd6952184c4681b4b133396d6b8b0aaf33` | v0.58.0 | all CTRL findings cite this commit |
|
||
| felhom-agent | `d17b5ab45dc7df51836e26d7a38450cd391b94b2` | v0.29.1 | all AGENT findings cite this commit |
|
||
| felhom.eu / hub (reference) | `d59691dd826a901da39562aeaa5d989b8ea1a7ee` | hub v0.11.0 | contract reference |
|
||
|
||
**Tooling:** go1.26.0 windows/amd64; staticcheck (latest); go vet.
|
||
**Prior art:** controller `BUGHUNT.md` (2026-02-25, v0.30.3) read in full. NOT re-reported unless regressed. NOTE: BUGHUNT predates slice 8C which deleted ~12.3k LOC (internal/storage/*, restic, restore_drives, monitor/watchdog) — many BUGHUNT items (C1, C3, H9, M11-M20 partial, L8-L21 partial) reference now-deleted code and are MOOT. Agent has no BUGHUNT.
|
||
|
||
## Progress log
|
||
|
||
- 17:20 — Resumed: prior session left only a Phase-0 skeleton stale at v0.51.0 + a duplicate product commit. Reset audit branch onto current main (v0.58.0). Re-baselined.
|
||
- 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 + 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.
|
||
- 18:55 — Tier 3 dispatched (templates/funcmap/XSS + no-`:latest` + surviving BUGHUNT concurrency re-check). Returned very clean: templates correct, 5/6 BUGHUNT concurrency items FIXED (only M2→CTRL-T3-1 lingers, benign), scheduler panic-isolation present.
|
||
- 19:05 — Wrote + ran failing evidence test `agent: internal/proxmox/upid_audit_test.go` (AGENT-T2-1, empty-node UPID). FAILS as expected. Folded Tier-3 into report. Final commit + push (both branches).
|
||
- DONE for this session. Resume point if continued: deeper read of `selfupdate`/`integrations`/`metrics`/`cloudflare` (controller) + `lanresolver`/`hub`/`desired` (agent); `-race` on build server; live read-only inspection.
|
||
|
||
## Baseline (Phase 0)
|
||
|
||
| Check | Controller (module at `controller/`) | Agent (module at root) |
|
||
|---|---|---|
|
||
| `go build ./...` | PASS | PASS |
|
||
| `go vet ./...` | PASS (clean) | PASS (clean) |
|
||
| `go test ./...` | PASS (BUGHUNT's TestBackupCopiesOnPath now green) | PASS |
|
||
| `gofmt -l .` | CRLF noise only (autocrlf=true); not a finding | same |
|
||
| staticcheck | 18 reports — triaged (4× SA4010 in backup.go → CTRL-005; SA4006/SA4017 export.go → CTRL-006; 5× U1000 dead code → §Dead code) | clean |
|
||
|
||
## Executive summary
|
||
|
||
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.
|
||
|
||
**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.
|
||
|
||
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` (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.)
|
||
|
||
---
|
||
|
||
## Findings — Critical / High
|
||
|
||
### [CTRL-001] App-import path traversal via unvalidated `manifest.AppName`
|
||
Severity: High
|
||
Category: security
|
||
Location: controller/internal/appexport/restore.go:339, 365, 401 (commit eea235b); root cause manifest.go:36-42
|
||
Confidence: **verified-by-test** (`internal/appexport/traversal_audit_test.go`, this branch)
|
||
Evidence:
|
||
```go
|
||
manifest, err := UnmarshalManifest(manifestData) // AppName fully from bundle JSON, no validation
|
||
stackDir := filepath.Join(stacksDir, manifest.AppName) // :339
|
||
os.MkdirAll(stackDir, 0755) // :365
|
||
composePath := filepath.Join(stackDir, "docker-compose.yml") // :401
|
||
```
|
||
Mechanism: `manifest.AppName` is read from the untrusted `manifest.json` inside an imported `.fab`. `UnmarshalManifest` only validates JSON, no segment check; no `IsValidStackName` exists in the package. `filepath.Join` cleans `..`, so `../../etc/cron.d/x` resolves outside `stacksDir`, then `os.MkdirAll` creates it and `restoreConfig`/`SaveEncryptedAppConfig` write `app.yaml`/`docker-compose.yml` there. The archive-entry zip-slip guard (restore.go:1047) does NOT cover this — it guards tar member names, not the stack-name segment.
|
||
Trigger: Import of a crafted `.fab`. The handler `/api/export/import` is behind RequireAuth+CSRF when a dashboard password is set, but on the demo (no password) BOTH are skipped → fully open. The `.fab` must sit under a registered `exports/` dir (`isValidExportPath` validates the file *location*, not its *contents*) — reachable via FileBrowser or a shared/malicious export.
|
||
Impact: Arbitrary-directory write as the controller process (config, restored app data, re-encrypted `app.yaml`) outside the stacks namespace — corruption/escape, potential clobber of controller config or writes onto mounted drives.
|
||
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
|
||
|
||
### [AGENT-001] Inline customer-confirmed wipe formats a mutable `/dev` path (classify→mkfs TOCTOU)
|
||
Severity: Medium (data-loss consequence; narrow USB-reenumeration trigger)
|
||
Category: security / correctness
|
||
Location: agent/internal/localapi/disks.go:429-471 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
probe, err := s.disks.InspectDevice(r.Context(), req.Device) // inspect /dev/sdbN
|
||
role := s.deviceRole(r.Context(), req.Device)
|
||
deviceDurable, _ := storage.DeviceDurableID(req.Device) // derive id of /dev/sdbN NOW
|
||
dec := s.diskGate.AuthorizeWipe(WipeRequest{Role:..., DeviceDurableID: deviceDurable, Confirmed: req.Confirmed, ConfirmDurableID: req.DurableID})
|
||
if dec.Allowed { s.disks.Format(r.Context(), req.Device, req.FSType) } // FORMAT the same mutable path
|
||
```
|
||
Mechanism: The confirmed path inspects + derives + gate-binds the durable id, then formats `req.Device` (the raw `/dev` node) — never re-resolving durable→path immediately before mkfs. The durable id is *checked* but never *used as the wipe target*. A USB re-enumeration between derive and Format makes mkfs hit a different physical disk than the one confirmed. The signed-jobs `WipeExecutor` (signedjobs/wipe.go:74-99) deliberately does resolve→re-derive→re-inspect just before Format — proving the intended pattern this inline path violates.
|
||
Trigger: Customer-confirmed user-data wipe where `/dev/sdbN` is reassigned (hot-unplug/replug, udev churn, multi-USB hub) in the sub-second window.
|
||
Impact: mkfs on an unintended physical disk → data loss on the wrong drive, defeating "wipe X wipes exactly X" for the inline tier.
|
||
Fix sketch: After `AuthorizeWipe` Allowed, `storage.ResolveDurableDevice(deviceDurable)` → re-derive+match → re-inspect `DataBearing()` → Format the *resolved* path, not `req.Device` (mirror `WipeExecutor.Execute`).
|
||
Verify: localapi test that swaps the device behind `req.Device` between InspectDevice and Format; assert refuse-or-re-resolve.
|
||
|
||
### [AGENT-002] Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU
|
||
Severity: Medium
|
||
Category: security
|
||
Location: agent/internal/localapi/disks.go:430-444 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
probe, err := s.disks.InspectDevice(r.Context(), req.Device)
|
||
if err != nil { /* fall through; probe.DataBearing()==true on !Probed (fail-safe) */ }
|
||
if !probe.DataBearing() { s.disks.Format(r.Context(), req.Device, req.FSType); writeOK(...); return }
|
||
```
|
||
Mechanism: When the agent's own probe reads the device blank, it formats with no gate, no durable binding, on the mutable `req.Device`. "blank → benign" is correct in isolation, but a device blank at probe time can be replaced at the same node before mkfs. Same root cause as AGENT-001 (formatting a mutable path). `InspectDevice` itself is fail-safe (unprobed → data-bearing), so classification is sound; the residual risk is the path-vs-physical gap.
|
||
Trigger: `/dev/sdbN` reassigned between blank probe and mkfs.
|
||
Impact: mkfs on a now-data-bearing device that was never gated. Lower likelihood than AGENT-001 (no durable binding involved).
|
||
Fix sketch: Bind even the blank path to a durable id: derive at probe, resolve back immediately before Format, refuse on mismatch.
|
||
Verify: Test a device swapped blank→data-bearing between probe and format is not silently mkfs'd.
|
||
|
||
### [AGENT-003] `InspectDevice` swallows the `blkid` error; `lsblk` is the sole "probed" authority
|
||
Severity: Medium
|
||
Category: error-handling / security
|
||
Location: agent/internal/storage/hostops.go:293-331 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
bout, _, _ := h.runner.Run(ctx, h.bins.Blkid, "-p", "-o", "export", device) // err DROPPED
|
||
for k, v := range parseBlkidExport(bout) { ... }
|
||
lout, _, lerr := h.runner.Run(ctx, h.bins.Lsblk, "-J", ..., device)
|
||
if lerr == nil { probe.Probed = true; ... } // lsblk is the ONLY thing that sets Probed
|
||
```
|
||
Mechanism: `blkid`'s exit/stderr is discarded; `Probed` is set solely from `lsblk` succeeding. blkid and lsblk cover different signature classes. If blkid errors transiently (busy/partial read) and returns empty while `lsblk -J` parses the device as having no fstype/pttype/children/mount, the probe is `Probed=true` with no positive evidence → `DataBearing()` false → reachable to the un-gated AGENT-002 format. This is exactly the "error in inspection → safe-to-wipe" pattern, narrowed by lsblk's real coverage of fstype/pttype/partitions/mount (so Medium, not Critical).
|
||
Trigger: blkid errors/empties while lsblk succeeds on a device whose data signature is blkid-only (some RAID/crypto members reported via blkid USAGE that lsblk's 4 columns miss).
|
||
Impact: A device with real data classified blank and formatted without a gate.
|
||
Fix sketch: Treat a blkid hard error (non-empty stderr / non-2 exit) as a probe failure; require both reads to complete before `Probed=true`, or fold blkid success into the Probed decision.
|
||
Verify: Unit test: blkid runner errors+empty, lsblk returns a clean blank-looking device → assert `Probed==false` (data-bearing).
|
||
|
||
### [CTRL-002] FAB decrypt streams plaintext to disk *before* verifying the HMAC tag
|
||
Severity: Medium
|
||
Category: security
|
||
Location: controller/internal/appexport/crypto.go:191-227 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
stream.XORKeyStream(decrypted, buf[:n]); out.Write(decrypted) // :203-204 plaintext to disk as it goes
|
||
...
|
||
if !hmac.Equal(mac.Sum(nil), storedMAC) { os.Remove(outputPath); ... } // :222-224 MAC checked only at the end
|
||
```
|
||
Mechanism: Construction is sound (AES-256-CTR + HMAC-SHA256 Encrypt-then-MAC, distinct scrypt-derived keys, constant-time `hmac.Equal`, random salt+IV per file). But decrypt writes full plaintext to `outputPath` and verifies the tag only afterward; cleanup is a best-effort `os.Remove` whose error is ignored. A same-key bit-flipped ciphertext yields controlled plaintext deltas that briefly land on disk before rejection; a crash between write and verify leaves unauthenticated attacker-influenced data behind. Violates "no unauthenticated plaintext is ever produced."
|
||
Trigger: Importing a tampered encrypted `.fab` (decrypted tgz written to a temp file before the tag check rejects).
|
||
Impact: Transient unauthenticated plaintext on disk; ignored `os.Remove` error can leave a partial decrypted file. Not a key/confidentiality break.
|
||
Fix sketch: Decrypt to temp → fsync → verify MAC → only then rename to `outputPath`; check the `os.Remove` error.
|
||
Verify: Flip one ciphertext byte, call `DecryptFile`; instrument to confirm bytes existed at the output path mid-call.
|
||
|
||
### [CTRL-005] Decompression-bomb: import extraction has no total-size / entry-count cap
|
||
Severity: Low
|
||
Category: resource-leak
|
||
Location: controller/internal/appexport/restore.go:1021-1072 (extractTarGz), :280 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
case tar.TypeReg:
|
||
outFile, _ := os.Create(target)
|
||
io.Copy(outFile, tr) // :1064 unbounded
|
||
```
|
||
Mechanism: Each gzip-inflated entry copied with unbounded `io.Copy`, no aggregate size or file-count limit, extracted to `os.MkdirTemp` on the ~8GB guest rootfs. Zip-slip guard is present (:1047), so availability-only.
|
||
Trigger: Importing an oversized/bomb `.fab`.
|
||
Impact: Fills guest rootfs during extraction → breaks controller + other apps until `defer os.RemoveAll(tmpDir)` runs. DoS.
|
||
Fix sketch: Track cumulative bytes vs a cap and vs `system.GetDiskUsage(tmpDir)` free; abort on exceed; cap entry count.
|
||
Verify: Import a tar.gz inflating beyond rootfs free; observe `/tmp` fill.
|
||
|
||
### [CTRL-008] `settings.json` (bcrypt hash + plaintext retrieval pw + tokens) written world-readable (0644)
|
||
Severity: Low
|
||
Category: security
|
||
Location: controller/internal/settings/settings.go:241 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
os.WriteFile(tmpPath, data, 0644) // file carries PasswordHash (bcrypt) + RetrievalPassword (plaintext)
|
||
```
|
||
Mechanism: controller.yaml is correctly 0600, but settings.json — holding the dashboard bcrypt hash and the **plaintext** Hub retrieval password — is 0644. Any non-root UID or mounted-in app with read access to the data dir can read it.
|
||
Trigger: Any local read access to the data dir.
|
||
Impact: Disclosure of the retrieval password (re-pull customer config from Hub) and bcrypt hash (offline cracking). Defense-in-depth; de-privileged single-tenant container lowers exposure.
|
||
Fix sketch: Write settings.json 0600; optionally encrypt RetrievalPassword at rest with the existing AES key.
|
||
Verify: `stat -c %a settings.json` → 644.
|
||
|
||
### [CTRL-007] Pre-auth setup CSRF is a forgeable double-submit cookie
|
||
Severity: Low
|
||
Category: security
|
||
Location: controller/internal/setup/csrf.go:33-43 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
return cookie.Value == formToken // cookie==form, both attacker-controllable; no server secret/HMAC
|
||
```
|
||
Mechanism: Pure double-submit with no server-stored secret/HMAC and `HttpOnly:false`. An attacker who can set `felhom_csrf` on the victim (cookie injection over plain-HTTP `:8081`, or a same-site subdomain) can post a matching token to the pre-auth setup wizard.
|
||
Trigger: Controller in setup mode (`NeedsSetup`) + attacker can plant/predict the cookie.
|
||
Impact: Pre-auth CSRF on `/setup/manual`,`/setup/fresh` → set dashboard password, domain, git repo, Cloudflare tokens. Bounded to the one-time setup window.
|
||
Fix sketch: HMAC the token with a server secret, verify Origin/Referer on setup POSTs.
|
||
Verify: POST `/setup/manual` with matching cookie+form but no prior GET → accepted.
|
||
|
||
### [AGENT-007] Decommission durable-id scheme unvalidated → silent no-op intent
|
||
Severity: Low
|
||
Category: contract-mismatch / correctness
|
||
Location: agent/internal/signedjobs/decommission.go:63-79 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
if p.DurableID == "" { return ... "refusing an unbound decommission" }
|
||
d.intent.SetDecommissioned(p.DurableID) // any non-empty string accepted; no scheme check
|
||
```
|
||
Mechanism: Wipe requires the `byid:`/`byuuid:` scheme via `ResolveDurableDevice`; decommission accepts any non-empty string and writes it into the intent map. The watchdog keys on the *storage* scheme (`uuid:…`) at watchdog.go:178. A wrong-scheme id records an intent that never matches → decommission silently no-ops while reporting success.
|
||
Trigger: Signed decommission whose `durable_id` uses the device scheme (`byid:`) instead of the storage scheme.
|
||
Impact: Operator believes a drive is permanently decommissioned; watchdog still auto-mounts it. Requires operator id-format error; signature is valid.
|
||
Fix sketch: Validate `p.DurableID` carries the storage-scheme prefix before recording; reject unknown schemes (fail loud, not no-op).
|
||
Verify: Decommission with `byid:…` → should refuse.
|
||
|
||
### [CTRL-009] Login rate-limiter keys on spoofable `X-Forwarded-For`
|
||
Severity: Low
|
||
Category: security
|
||
Location: controller/internal/web/auth.go:127-161 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
ip := r.RemoteAddr
|
||
if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { ip = strings.Split(fwd, ",")[0] } // attacker-controlled
|
||
```
|
||
Mechanism: Limit key derived from client-supplied XFF with no trusted-proxy allowlist; rotating XFF defeats the 5/min lock. bcrypt cost-10 is the real backstop.
|
||
Trigger: Brute-force `/login` with varying XFF.
|
||
Impact: Online password brute force (hardening gap, not an immediate break).
|
||
Fix sketch: Honor XFF only from a configured trusted-proxy CIDR; else key on `r.RemoteAddr`; add a global failed-login cap.
|
||
Verify: 5 failed logins with distinct XFF each reach bcrypt (no lock).
|
||
|
||
### [CTRL-011] Open redirect via protocol-relative `?next=//host`
|
||
Severity: Low
|
||
Category: security
|
||
Location: controller/internal/web/auth.go:187-191 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
if nextURL != "" && strings.HasPrefix(nextURL, "/") { redirectTo = nextURL } // "//evil.com" passes
|
||
```
|
||
Mechanism: `next` only required to start with `/`; `//evil.com` is protocol-relative and most browsers redirect to that host.
|
||
Trigger: `POST /login?next=//evil.com` after valid credentials.
|
||
Impact: Post-login open redirect (phishing). Requires a valid login first.
|
||
Fix sketch: Reject `next` starting with `//` or `/\`.
|
||
Verify: Log in with `next=//example.com` → 302 to `//example.com`.
|
||
|
||
### [AGENT-004] Signed-op execution runs under `context.Background()` (no timeout)
|
||
Severity: Low
|
||
Category: resource-leak
|
||
Location: agent/internal/signedjobs/runner.go:81-85 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
go func() { if _, err := r.RunOnce(context.Background()); err != nil { ... } }() // no deadline → mkfs/pct can hang forever
|
||
```
|
||
Mechanism: ctx flows to `exec.CommandContext`; a wedged mkfs/pct never gets cancelled, and the single-flight `running` flag then short-circuits every later pass → the signed-jobs consumer stalls until restart.
|
||
Trigger: A destructive mkfs hangs (IO stall, DM hang).
|
||
Impact: Runner goroutine wedged; no further signed ops until restart.
|
||
Fix sketch: `context.WithTimeout` per pass in `OnEnvelope`, and/or per-op deadline around Format.
|
||
Verify: Inject a blocking Format; assert RunOnce returns within timeout.
|
||
|
||
### [AGENT-012] FileNonceStore loads expired nonces; `MemoryNonceStore` never evicts
|
||
Severity: Low
|
||
Category: resource-leak
|
||
Location: agent/internal/authz/noncestore.go:92-112 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
for _, line := range ... { ... s.idx[r.Nonce] = r.Exp } // no expiry filter on load; compaction only after CompactEvery new appends
|
||
```
|
||
Mechanism: `Open` loads all logged nonces incl. expired; compaction driven only by new appends (default 1000). Frequent restarts + long validity window keep expired nonces resident. Bounded by op-rate × window; correctness unaffected (time-window check rejects expired ops first).
|
||
Trigger: High op volume + frequent restarts.
|
||
Impact: Slow nonce-log growth on a busy host. Not a security hole.
|
||
Fix sketch: Skip `r.Exp.Before(now)` during `load()`; run one compaction on `Open`.
|
||
Verify: Open a store whose log holds only expired records → assert `len(idx)==0`.
|
||
|
||
### [AGENT-009] localapi cross-guest scope check covers only the query `vmid` (latent)
|
||
Severity: Low
|
||
Category: security
|
||
Location: agent/internal/localapi/server.go:263-308 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
if q := r.URL.Query().Get("vmid"); q != "" { if want != vmid { ...403 } }
|
||
fn(w, r, vmid) // handler ALWAYS receives the token's vmid
|
||
```
|
||
Mechanism: `withGuest` rejects a mismatching *query* vmid; a mismatching *body* vmid is caught separately by per-handler `scopedFromBody`. Self-scoping holds today on all 14 routes (handler always uses the token's vmid), but the invariant relies on per-handler discipline, not one chokepoint — a future route that reads an id from body/path and forgets `scopedFromBody` would regress it.
|
||
Trigger: New handler omitting the body-vmid check.
|
||
Impact: None today; latent cross-guest exposure on future routes.
|
||
Fix sketch: Centralize the body-vmid check, or assert handlers never read an id other than the wrapper's `vmid`.
|
||
Verify: Table test POSTing `{"vmid":<other>}` to every mutating route → expect 403.
|
||
|
||
### [AGENT-006] `wholeDiskOf` symlink-resolution failure falls back to raw string
|
||
Severity: Low
|
||
Category: correctness
|
||
Location: agent/internal/storage/role.go:69-87 (commit d17b5ab)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
if resolved, err := filepath.EvalSymlinks(device); err == nil { dev = resolved }
|
||
... return "", false // unrecognized → caller treats as system (fail-safe)
|
||
```
|
||
Mechanism: Direction is fail-safe (unrecognized → system/protected). Drift: `SystemDisks` only adds a disk when `wholeDiskOf` succeeds; an EvalSymlinks failure on a system mount's device omits that OS disk from `sysDisks`, so a sibling user-data disk could flap. No unsafe verdict, but classification of user-data disks can be unstable when topology resolution is flaky.
|
||
Trigger: Transient `/dev` churn failing EvalSymlinks on a system mount during classification.
|
||
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-by-test** (agent branch: `internal/proxmox/upid_audit_test.go`)
|
||
Evidence: `ParseUPID` (upid.go:50-59) sets `Node: parts[1]` with no empty/validity check → an empty-node UPID parses cleanly (`u.Node==""`) → `TaskStatusOnce` requests `/nodes//tasks/...` and queries `u.Node` not its pinned `c.node`. The sibling PBS client guards `node==""`; 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: `cd /e/git/felhom-agent && go test ./internal/proxmox/ -run RejectsEmptyNode -v` → FAILS at this commit (empty node accepted).
|
||
|
||
### [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.
|
||
|
||
### [CTRL-T3-1] `backup.Manager.stackProvider` read without the mutex that guards its write (BUGHUNT M2 — still present, benign)
|
||
Severity: Low
|
||
Category: concurrency
|
||
Location: controller/internal/backup/backup.go:89,122,259,331,400,491 (reads); :392 (locked write) (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence:
|
||
```go
|
||
func (m *Manager) SetStackProvider(p StackDataProvider) { m.mu.Lock(); m.stackProvider = p; m.mu.Unlock() } // :392
|
||
if m.stackProvider != nil { ... m.stackProvider.GetStackHDDPath(stackName) ... } // :89 unlocked read (×11)
|
||
```
|
||
Mechanism: The single write is locked (with a comment claiming concurrent reads) but all 11 reads are unlocked. Compensating control: `main.go:225` calls `SetStackProvider` exactly once during single-threaded init, before the scheduler/HTTP server start → write happens-before all reads. Latent: a second runtime call would introduce a real data race; the locked write + "concurrent" comment is misleading.
|
||
Fix sketch: Drop the lock on the init-only write, OR add a locked `getStackProvider()` accessor and route the 11 reads through it.
|
||
Verify: `go test -race ./internal/backup/...` with a test calling SetStackProvider concurrently with a read; or confirm it stays init-only.
|
||
|
||
---
|
||
|
||
## Findings — Info
|
||
|
||
### [CTRL-005b] DB-dump per-DB summary built then discarded (SA4010 ×4)
|
||
Severity: Low → Info (observability only)
|
||
Category: error-handling / dead-code
|
||
Location: controller/internal/backup/backup.go:179-253 (commit eea235b)
|
||
Confidence: verified-static
|
||
Evidence: `summary` accumulates `OK/SKIP/FAIL <db>` lines (`:188,:193,:204,:208`) but is never read; caller returns generic `"some database dumps failed"` (:249). SKIP reasons ("drive disconnected/decommissioned") are lost.
|
||
Impact: Operators lose the per-DB failure/skip breakdown — can't tell which app/drive is unprotected.
|
||
Fix sketch: Fold `summary` into the returned error / `m.lastDBDump` status, or remove the dead var.
|
||
Verify: Wire `summary` into the error → SA4010 clears.
|
||
|
||
### [CTRL-006] `executeExport` step-5 timing baseline dead-assigned (SA4006/SA4017)
|
||
Severity: Info • Category: dead-code • Location: appexport/export.go:337 (eea235b)
|
||
`stepStart = time.Now()` reassigned but never read; encrypt step uses its own `encStart`. Cosmetic timing-log gap. Fix: remove the dead assignment or add the missing `time.Since(stepStart)` log.
|
||
|
||
### [CTRL-010] Setup writes config to a hardcoded path, ignoring resolved `Paths`
|
||
Severity: Info • Category: correctness • Location: setup/handlers.go:372 (eea235b)
|
||
`configPath := "/opt/docker/felhom-controller/controller.yaml"` hardcoded. On a deployment whose runtime config path differs, setup could write where the runtime never reads → `NeedsSetup` stays true, re-exposing the pre-auth wizard persistently. Fix: pass the resolved config path into `setup.NewServer`.
|
||
|
||
### [CTRL-012] `os.MkdirAll`/`os.Remove`/`Sync` errors swallowed in export staging
|
||
Severity: Info • Category: error-handling • Location: appexport/export.go:280,298,319,571,605 (eea235b)
|
||
Staging dir/remove errors ignored → less precise later failures. Critical bundle paths (tar, EncryptFile, final rename) do check errors. Fix: `failJob` on staging MkdirAll errors.
|
||
|
||
### [AGENT-005] `bindsToAction` is self-referential for one-shot jobs (defense-in-depth note)
|
||
Severity: Info • Category: invariant-drift • Location: signedjobs/runner.go:125-148; reconcile/gate.go:265-276 (d17b5ab)
|
||
For hub-queued jobs the gate compares the verified blob's params against an intent copied from the same blob → `bindsToAction` is tautologically true. By design (the executor's durable resolve+re-inspect is the real binding), but the gate adds no re-binding to the agent-surfaced `PendingOp`. The executor's data-bearing re-check is the sole backstop. Document explicitly or add a sanity assert of blob durable-id vs the host's current view.
|
||
|
||
### [AGENT-010] localapi `TokenStore.Lookup` constant-time compare is a self-comparison
|
||
Severity: Info • Category: security • Location: localapi/tokenstore.go:138-154 (d17b5ab)
|
||
The secret-bearing step is the `byHash[want]` map lookup (variable-time); the subsequent `subtle.ConstantTimeCompare(want, byVMID[vmid])` compares a value to itself (always 1) — no timing benefit. Not exploitable (SHA-256 of a 256-bit random token). Risk is documentary: don't claim a timing guarantee the code doesn't provide. Hash-only invariant still HOLDS.
|
||
|
||
### [AGENT-011] Host-wide metrics/disk topology served to any guest token
|
||
Severity: Info • Category: security • Location: localapi/host_metrics.go:28-49, disks.go:109-147 (d17b5ab)
|
||
`/host/metrics` and `/disks` return host-wide CPU/temp/SMART + storage targets to any authenticated guest, by design (one-customer-per-host model, documented inline). Revisit only if multi-tenancy is ever introduced.
|
||
|
||
### [AGENT-013] `DestroyLXC` uses blanket destructive flags; narrowness is external (gated upstream)
|
||
Severity: Info • Category: security • Location: proxmox/mutate.go:105-115 (d17b5ab)
|
||
`DestroyLXC` always passes `force=1&purge=1&destroy-unreferenced-disks=1` with no internal vmid/scratch guard; safety depends entirely on the upstream `reconcile.Gate`. The only destructive consumer (signed-jobs runner) routes through the gate (verified). Add a routing test asserting no non-gated caller reaches it.
|
||
|
||
### [AGENT-008] (positive) Fail-destructive defaults are correctly wired end-to-end
|
||
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 |
|
||
|---|---|---|
|
||
| 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**
|
||
- Data-bearing classification from device inspection only (never caller claims) — **HOLDS** @ localapi/disks.go:429-435 + storage/hostops.go:283-333 (caller "blank/force" claim explicitly ignored). Caveat AGENT-003 (transient blkid-error read drop).
|
||
- Fail-destructive on ambiguity — **HOLDS** @ hostops.go:64-69, classify.go:111, role.go:91-99 (AGENT-008).
|
||
- Signature gate before destructive op; no un-gated path — **HOLDS for signed-jobs** @ runner.go:134-148; **DRIFTED for inline path** → AGENT-001/AGENT-002 (authorized resource ≠ mutated resource; blank branch un-gated).
|
||
- Token store hash-only — **HOLDS** @ tokenstore.go:106-170 (only sha256 hashes persisted; AGENT-010 note).
|
||
- localapi self-scoping (cross-guest→403) — **HOLDS** @ server.go:263-308 (latent-discipline AGENT-009).
|
||
- Privileged ops narrow + TLS pinned — **HOLDS** @ proxmox/privileged.go:11-178 (3 fenced exceptions, no shell), tls.go:33-75 (CAFile | leaf-SHA256 pin; InsecureSkipVerify off by default). Blanket primitive AGENT-013 gated upstream.
|
||
- Signed-job sig verified before destructive op + nonce-replay prevented — **HOLDS** @ verifier.go:99-181 (verify over raw bytes, allow-list by key material), noncestore.go:114-135 (fsync-durable, fail-safe). Growth note AGENT-012.
|
||
- No `pct exec` in provision back-half / bootstrap.json chown 100000:100000 — **NOT verified this session** (Tier 2, provision pkg).
|
||
- No secrets in logs — **HOLDS** (token "Never logged"; escrow logs only short FPs; pty discards passphrase echo).
|
||
|
||
**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 — **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 — **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.
|
||
|
||
**Tier-3 results (templates + surviving BUGHUNT concurrency, controller @ eea235b):**
|
||
- Templates/funcmap — **CLEAN.** Every template func invoked is registered (no parse-panic risk); no XSS bypass (only raw-HTML sites are `csrf.go:98` with explicit `HTMLEscapeString` and the `json` func returning `template.JS` via `json.Marshal`'s default HTML-escaping — `</script>` breakout neutralized); all 11 container states resolve in `stateColor`/`stateLabel`; **no `:latest`** anywhere (traefik:v3.6.7, cloudflared:2026.6.0, filebrowser:1.3.3-stable pinned); no emoji, no dead `{{define}}` blocks. (Several *unused* funcmap entries — `stateIcon, stateStr, statusText, seq, shortID, fmtDuration, pruneLabel, nextPruneLabel` — harmless dead code.)
|
||
- Surviving BUGHUNT concurrency: **M10** (scheduler late-registration) — **FIXED** @ scheduler.go:94-97,123-126; **M2** (stackProvider unlocked read) — **STILL-PRESENT** → CTRL-T3-1 (Low, benign); **M3** (DrainPendingEvents loss) — **FIXED** @ settings.go:946-950; **M22** (assets mutex during download) — **FIXED** @ syncer.go:79-91; **M13** (WAL not verified) — **FIXED** @ store.go:27-34; **M14** (sampleContainers context.Background) — **FIXED** @ collector.go:99-100. Scheduler **panic isolation PRESENT** @ scheduler.go:268-276 (each job under `defer recover()`).
|
||
|
||
## 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.
|
||
- **Single path-segment validator (controller):** CTRL-001 shows `manifest.AppName` and friends reach `filepath.Join` unvalidated while API stack routes have `extractName`. Extract one `ValidateStackName` and apply at every untrusted boundary (import manifest, archive subdir names, export filenames).
|
||
- **Decrypt-then-rename pattern (controller):** CTRL-002 — the export side already does tmp→rename; the decrypt side should too. Centralize an `atomicDecrypt(verify, then rename)`.
|
||
- (Carried from BUGHUNT, still relevant where code survived 8C) atomic-write helper unification; injected-logger consistency (`crypto.DecryptMap` global log).
|
||
|
||
## Test-gap analysis
|
||
|
||
- **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** — 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)
|
||
|
||
- `cmd/controller/main.go:1172` `fileExists` — unused (also BUGHUNT L1; still present).
|
||
- `internal/system/info.go:11` `debugf` — unused.
|
||
- `internal/web/alerts.go:233` `countLevel` — unused.
|
||
- `internal/web/handlers.go:1083` `(*Server).countAppsUsingPath` — unused.
|
||
- `internal/web/funcmap.go` — unused registered template funcs: `stateIcon, stateStr, statusText, seq, shortID, fmtDuration, pruneLabel, nextPruneLabel` (registered but never invoked by any template; harmless dead funcmap entries).
|
||
- (agent: staticcheck clean — no dead-code reports.)
|
||
|
||
## Session notes, assumptions, open questions
|
||
|
||
- Session unattended; conservative assumptions inline. Severity reflects consequence; confidence kept honest (most findings verified-static, CTRL-001 verified-by-test).
|
||
- **Assumption:** "one customer per host" deployment model holds (makes AGENT-011 Info not Medium). Documented in arch doc 03.
|
||
- **Assumption:** AGENT-001/002 trigger (USB `/dev` re-enumeration in the inspect→mkfs window) is rare on these N100 boxes → rated Medium not High; raise to High if USB churn proves common.
|
||
- gofmt noise = CRLF/autocrlf artifact, not a finding.
|
||
- Two controller Tier-1 auditors independently numbered findings CTRL-001..; merged + renumbered here into one namespace.
|
||
- **Full mutating-route × CSRF/auth coverage table** (password-set path) was produced and shows no uncovered mutating runtime route; available on request — every `/api/*` + `/` subtree wrapped in RequireAuth+CsrfProtect (main.go:703-728); only `/api/health` (GET) and `/api/host-metrics` (GET) are CSRF-exempt by nature.
|
||
|
||
## What was NOT covered (defines next session)
|
||
|
||
Tiers 1, 2 AND the Tier-3 templates + surviving-BUGHUNT-concurrency re-check are complete for both repos + both cross-repo contracts. NOT yet done (defines the next session, roughly in priority order):
|
||
- **`-race` run:** not executed (needs the build server; CGO/sqlite won't `-race` on this Windows box). Highest-value next step — would empirically confirm/deny CTRL-T3-1 (M2) and any uncaught shared-state races. Recommended 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.
|