diff --git a/CHANGELOG.md b/CHANGELOG.md index e91e94c..2353e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,50 @@ +## v0.61.0 — blast-radius audit fixes B1 + D1 + D2 + D3 (2026-07-03) + +Four LOW/INFO fixes from `AUDIT-blast-radius-hostroot-localapi-2026-07-02.md` — the "second gate must +mirror the first" batch. Each shipped with a non-hollow test AND a companion red-proof (test shown +failing on the pre-fix implementation). A1 (stale-lock pool-membership) is deliberately NOT here — it +needs a pool-read spike (the role lacks `Pool.Audit`); C1/C2/A2/B2–B5/E1/E2 deferred. + +- **B1 (LOW) — random temp staging for root-installed scripts.** `internal/guesthook/install.go` + (`InstallSnippet`) and `internal/localapi/intermediary.go` (`installSharedParentUnit`, new shared + `stageTemp`) staged root-executed scripts through FIXED, predictable `/tmp` names via `os.WriteFile` + (no O_EXCL, follows symlinks) — a local TOCTOU into a root-run PVE hookscript / boot script. Both now + use the `os.CreateTemp` random-name pattern `lanresolver` already used. `configs/felhom-agent.sudoers` + (`FELHOM_GUESTHOOK`/`FELHOM_INTERMEDIARY`) install-SOURCE grants became globs + (`/tmp/felhom-guest-hook-*.sh`, `/tmp/felhom-shared-parent-*.{sh,service}`; destinations stay pinned); + `internal/capability/manifest.go` representative vectors updated to match. Tests: + `TestInstallSnippet_RandomTempName`, `TestInstallSharedParent_RandomTempName` (fake runner records the + install source: random pattern, two calls differ, content + cleanup asserted). +- **D1 (LOW) — the guarded mkfs wrapper now mirrors the classifier's member/RO classes.** + `configs/felhom-mkfs-guarded.sh` re-checked only system-disk / LVM-PV (PATH-dependent `command -v + pvs`) / foreign-mount — a bypassed agent could mkfs a ZFS/mdraid/LUKS/swap member or a read-only + disk. Added (additive; nothing removed/reordered): `/sys/block//ro` == 1 → die; an + lsblk-FSTYPE loop over the whole disk refusing exactly `claim.go`'s `memberFSTypes` + (LVM2_member/zfs_member/linux_raid_member/crypto_LUKS/swap — the FSTYPE catch works with pvs + absent); pvs resolved via absolute candidates (/usr/sbin/pvs, /sbin/pvs). Validated by the new + `scripts/mkfs-guarded-harness.sh` on felhom-pve: throwaway loop devices + PATH-shimmed lsblk + a + recorder bind-mounted over mkfs.ext4 in a private mount namespace (no real mkfs possible) — fixed + wrapper 8/8 (incl. plain-blank-disk still formats); pre-fix wrapper red-proof: 7/8 hostile fixtures + reached mkfs. +- **D2 (INFO) — `classifyClaim` empty-lsblk fail-safe.** A successful-but-empty `lsblk` + (`{"blockdevices":[]}`) skipped the member/mount loop and returned `unclaimed`. + `internal/storage/claim.go` now refuses when the node tree is empty OR the target whole-disk is + absent from it (undeterminable topology ⇒ claimed). Tests: `TestClassifyClaim_EmptyNodesRefused`, + `TestClassifyClaim_TargetAbsentFromTree`. +- **D3 (INFO) — blank-format anti-retarget (AGENT-001's benign-branch twin).** `handleDiskFormat`'s + blank branch formatted the mutable caller-supplied `req.Device` with no durable-id binding — a /dev + re-enumeration between inspect and mkfs could format a data-bearing disk that inherited the node. + The blank branch (`internal/localapi/disks.go`) now derives the device's durable id (no durable id ⇒ + 409 refuse — path-only formats are not permitted), re-resolves it via the new + `antiRetargetResolveBlank` (`wipe_reresolve.go`: shared `antiRetargetResolveExpect` core; the blank + variant asserts the device is STILL !DataBearing), and formats the RE-RESOLVED device. The format + job record (`formatjob.go`) carries `blank`; restart recovery re-checks blank jobs with the blank + variant (durable-id-bound, fail-safe refuse). Confirmed/data-bearing branch untouched. Tests: + `TestFormatBlankPath_AntiRetarget_{ReassignedDataBearingRefused,ReassignedDifferentDiskRefused,UnresolvableRefused,SameBlankProceeds}`, + `TestFormat_Blank_{FormatsReresolvedDeviceNotCallerPath,ReresolveRefusalNoMkfs,NoDurableIDRefused}`. +- Deploy note: the host's `/etc/sudoers.d/felhom-agent` MUST be updated together with the v0.61.0 + binary (the old fixed-name grants deny the new random-name installs, and vice versa). + ## v0.60.0 — proof-of-launch destroy gating + restore-test band-advance (campaign F1/F2) (2026-07-02) Fixes the pool-effects campaign's HIGH finding (F1, `CAMPAIGN-pool-effects-2026-07-01.md`): the bring-up diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..e7299fe --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,29 @@ +# CONTEXT — felhom-agent working state + +> Snapshot of the current state + open threads. Authoritative history lives in `CHANGELOG.md` (top +> entry = current); the end-of-task detail lives in `REPORT.md`. + +## Current + +- **v0.61.0** (2026-07-03) — blast-radius audit fixes **B1 + D1 + D2 + D3** from + `felhom.eu/documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md`: random temp + staging for root-installed scripts (+ sudoers/manifest glob updates), mkfs-wrapper member/RO + re-checks (validated by `scripts/mkfs-guarded-harness.sh`), classifyClaim empty-lsblk fail-safe, + and the blank-format anti-retarget (durable-id-bound, AGENT-001's benign-branch twin). +- Deployed on demo host `felhom-pve` (node `demo-felhom`), non-root `felhom-agent` service user, + pool-scoped token (`felhom` pool). + +## Open threads + +- **A1 (LOW, pending SPIKE — not fixed in v0.61.0):** stale-lock recovery + (`internal/localapi/stalelock.go`) reaps guests by a pool-blind `ListLXC` scan, not ownership. + Contained by the pool-scoped token today; **must be fixed before any broad/root-token deployment.** + It is a spike, not a patch: the role lacks `Pool.Audit`, so there is no cheap authorized + pool-membership read — which read to use is the open design question. +- Deferred audit items (housekeeping/design, all INFO): C1 (controller-swap version floor), C2 (NAS + server allowlist), A2 (gate journal cross-check), B2–B5, E1/E2. +- Drive-enrollment leftovers: (a) `runStorageInit` slow-device detached-format polling; (b) Impl-3 + shared-box operator format gate. +- BUNDLE leftover: non-root agent can't read the PBS key; migration must preserve cert/key/tokens. +- Not run (needs a supervised session): the destructive D1/D3 live proofs (real mkfs on a crafted + member; a live /dev re-enumeration race during a real format). diff --git a/README.md b/README.md index 62bf1d9..5967512 100644 --- a/README.md +++ b/README.md @@ -256,12 +256,18 @@ from the intent registry + Felhom `.mount` units (`RegistryKnownTargets`), NOT P enrolled with no PVE dir-storage is still health-tracked (`Observe()` stays for real PVE storages). The controller wizard consuming this is Impl-2b. -**Format safety (v0.54.0, Impl-1):** `Format` (mkfs) is gated by a mandatory **unclaimed-disk guard** -(`internal/storage/claim.go`) — it refuses any device not provably free for Felhom (OS disk, LVM PV, -ZFS/mdraid member, foreign mount, read-only; fail-safe on any read error), independent of `DataBearing`. -Below the agent, mkfs runs ONLY through `configs/felhom-mkfs-guarded.sh` (the sole mkfs the sudoers -permits), which re-checks the catastrophic cases as root. The pool-scoped token does NOT touch mkfs -(sudo op) — the filter + wrapper are the guard. See `SPIKE-drive-enrollment-2026-07-01.md`. +**Format safety (v0.54.0, Impl-1; hardened v0.61.0, audit D1/D2/D3):** `Format` (mkfs) is gated by a +mandatory **unclaimed-disk guard** (`internal/storage/claim.go`) — it refuses any device not provably +free for Felhom (OS disk, LVM PV, ZFS/mdraid member, foreign mount, read-only; fail-safe on any read +error, and on an empty/target-absent lsblk topology — D2), independent of `DataBearing`. Below the +agent, mkfs runs ONLY through `configs/felhom-mkfs-guarded.sh` (the sole mkfs the sudoers permits), +which re-checks the catastrophic cases as root: system disk, LVM PV (absolute-path pvs), foreign +mount, read-only device, and any LVM/ZFS/mdraid/LUKS/swap member signature (D1 — validated by +`scripts/mkfs-guarded-harness.sh`, a loop-device + recorder harness). The blank-format local-API path +binds to the device's durable id and anti-retarget re-resolves before mkfs, same as the confirmed +wipe (D3, AGENT-001's benign-branch twin). The pool-scoped token does NOT touch mkfs (sudo op) — the +filter + wrapper are the guard. See `SPIKE-drive-enrollment-2026-07-01.md` and +`felhom.eu/documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md`. ## Process model diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 64d4ff4..ca2e508 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -45,7 +45,7 @@ import ( // version is the agent version. Overridable at build time with // -ldflags "-X main.version="; defaults to the in-repo CHANGELOG version. -var version = "0.60.0" +var version = "0.61.0" // runGuestHook is the PVE pre-start hook body (`felhom-agent guest-hook `). On the // pre-start phase it creates placeholder dirs for any absent bind-mount source so the guest always boots diff --git a/configs/felhom-agent.sudoers b/configs/felhom-agent.sudoers index e924f89..251a266 100644 --- a/configs/felhom-agent.sudoers +++ b/configs/felhom-agent.sudoers @@ -68,9 +68,11 @@ Cmnd_Alias FELHOM_DNSMASQ = \ # wrapper is installed once into the PVE snippets dir (from an agent-written /tmp file) and registered # per-guest; decommission/eject DELETE the dead mountpoint slot so a missing bind source can't brick the # guest at next boot (the B3 C1 fix). The agent fine-validates the vmid (numeric) + slot (mp[0-9]+) and -# the snippet path is fixed — the wildcards are the coarse allowlist. +# the snippet path is fixed — the wildcards are the coarse allowlist. The install SOURCE is a +# random-named agent temp (os.CreateTemp, audit B1 — a fixed /tmp name was a local TOCTOU), hence the +# glob; the DESTINATION stays pinned. Cmnd_Alias FELHOM_GUESTHOOK = \ - /usr/bin/install -m 0755 -- /tmp/felhom-guest-hook.sh /var/lib/vz/snippets/felhom-guest-hook.sh, \ + /usr/bin/install -m 0755 -- /tmp/felhom-guest-hook-*.sh /var/lib/vz/snippets/felhom-guest-hook.sh, \ /usr/sbin/pct set [0-9]* --hookscript local\:snippets/felhom-guest-hook.sh, \ /usr/sbin/pct set [0-9]* --delete mp[0-9]*, \ /usr/sbin/pct reboot [0-9]* @@ -95,8 +97,8 @@ Cmnd_Alias FELHOM_INTERMEDIARY = \ /usr/bin/mount --make-private /mnt/felhom-drives, \ /usr/bin/mount --bind /mnt/*/felhom-data /mnt/felhom-drives/*, \ /usr/bin/umount /mnt/felhom-drives/*, \ - /usr/bin/install -m 0755 -- /tmp/felhom-shared-parent.sh /usr/local/sbin/felhom-shared-parent.sh, \ - /usr/bin/install -m 0644 -- /tmp/felhom-shared-parent.service /etc/systemd/system/felhom-shared-parent.service, \ + /usr/bin/install -m 0755 -- /tmp/felhom-shared-parent-*.sh /usr/local/sbin/felhom-shared-parent.sh, \ + /usr/bin/install -m 0644 -- /tmp/felhom-shared-parent-*.service /etc/systemd/system/felhom-shared-parent.service, \ /usr/bin/systemctl enable felhom-shared-parent.service, \ /usr/bin/lxc-info -n [0-9]* -p -H, \ /usr/sbin/pct set [0-9]* -mp8 /mnt/felhom-drives* diff --git a/configs/felhom-mkfs-guarded.sh b/configs/felhom-mkfs-guarded.sh index c3fa168..ed4d747 100644 --- a/configs/felhom-mkfs-guarded.sh +++ b/configs/felhom-mkfs-guarded.sh @@ -3,7 +3,8 @@ # felhom-mkfs-guarded — the ONLY mkfs path the felhom-agent sudoers permits (Impl-1 Part B, # SPIKE-drive-enrollment-2026-07-01 §SQ3). Defense-in-depth BELOW the agent: even a buggy or # compromised agent cannot mkfs a catastrophic target through this — it re-checks, as root, the -# cheap catastrophic cases (OS/system disk, LVM physical volume, a foreign mount) and refuses. +# cheap catastrophic cases (OS/system disk, LVM physical volume, a foreign mount, a read-only +# device, and any LVM/ZFS/mdraid/LUKS/swap member signature — audit D1) and refuses. # # The agent's full unclaimed-disk filter (internal/storage/claim.go) is the PRIMARY guard; this # wrapper is a deliberately minimal, auditable second gate. It is NOT the place for the full filter. @@ -38,14 +39,21 @@ while read -r src mnt _rest; do esac done < /proc/mounts -# 2) LVM physical volume anywhere on the target disk or its partitions. -if command -v pvs >/dev/null 2>&1; then +# 2) LVM physical volume anywhere on the target disk or its partitions. pvs is resolved by ABSOLUTE +# path (audit D1: `command -v pvs` silently skipped this check when pvs wasn't on the caller's +# PATH); if neither candidate exists, check 5's LVM2_member FSTYPE loop still catches a PV +# independently — pvs-absent never silently drops LVM detection. +pvsbin="" +for c in /usr/sbin/pvs /sbin/pvs; do + [[ -x "$c" ]] && { pvsbin="$c"; break; } +done +if [[ -n "$pvsbin" ]]; then while read -r pv; do pv="${pv//[[:space:]]/}"; [[ -z "$pv" ]] && continue pvpk="$(lsblk -ndo PKNAME "$pv" 2>/dev/null || true)" pvwhole="$pv"; [[ -n "$pvpk" ]] && pvwhole="/dev/$pvpk" [[ "$pvwhole" == "$whole" ]] && die "device holds an LVM physical volume ($pv)" - done < <(pvs --noheadings -o pv_name 2>/dev/null || true) + done < <("$pvsbin" --noheadings -o pv_name 2>/dev/null || true) fi # 3) mounted OUTSIDE Felhom's own drive area = a live foreign filesystem → catastrophic. Mounts under @@ -58,6 +66,26 @@ while read -r mp; do esac done < <(lsblk -nro MOUNTPOINT "$whole" 2>/dev/null || true) +# 4) read-only device (audit D1): a device the kernel marks RO is never a formattable data disk. +wbase="${whole#/dev/}" +rof="/sys/block/$wbase/ro" +if [[ -r "$rof" ]]; then + ro="$(cat "$rof" 2>/dev/null || true)" + [[ "$ro" == "1" ]] && die "read-only device ($whole)" +fi + +# 5) member/active FSTYPEs anywhere on the target disk or its partitions (audit D1). Mirrors +# claim.go memberFSTypes exactly: a member of LVM/ZFS/mdraid/LUKS or active-swap signature is +# always a claim, never a plain formattable data disk. This also independently catches an LVM PV +# when pvs is not installed (check 2's belt-and-suspenders). +while read -r fst; do + [[ -z "$fst" ]] && continue + case "$fst" in + LVM2_member|zfs_member|linux_raid_member|crypto_LUKS|swap) + die "device holds a $fst signature ($whole)" ;; + esac +done < <(lsblk -nro FSTYPE "$whole" 2>/dev/null || true) + # Passed the catastrophic checks → format. exec so the mkfs exit status is the wrapper's. case "$fstype" in ext4) exec /usr/sbin/mkfs.ext4 -F "$dev" ;; diff --git a/internal/capability/manifest.go b/internal/capability/manifest.go index a191b16..546ef9f 100644 --- a/internal/capability/manifest.go +++ b/internal/capability/manifest.go @@ -49,8 +49,8 @@ var manifest = []Capability{ {"drives-mkdir-sub", "per-drive stable dir create", "/usr/bin/mkdir", []string{"-p", "/mnt/felhom-drives/felhom-usb"}, false}, {"drives-mkdir-data", "felhom-data namespace create", "/usr/bin/mkdir", []string{"-p", "/mnt/felhom-usb/felhom-data"}, false}, {"drives-chown-data", "felhom-data guest-root chown", "/usr/bin/chown", []string{"100000:100000", "/mnt/felhom-usb/felhom-data"}, false}, - {"parent-script-install", "shared-parent boot script install", "/usr/bin/install", []string{"-m", "0755", "--", "/tmp/felhom-shared-parent.sh", "/usr/local/sbin/felhom-shared-parent.sh"}, false}, - {"parent-unit-install", "shared-parent boot unit install", "/usr/bin/install", []string{"-m", "0644", "--", "/tmp/felhom-shared-parent.service", "/etc/systemd/system/felhom-shared-parent.service"}, false}, + {"parent-script-install", "shared-parent boot script install", "/usr/bin/install", []string{"-m", "0755", "--", "/tmp/felhom-shared-parent-123456789.sh", "/usr/local/sbin/felhom-shared-parent.sh"}, false}, + {"parent-unit-install", "shared-parent boot unit install", "/usr/bin/install", []string{"-m", "0644", "--", "/tmp/felhom-shared-parent-123456789.service", "/etc/systemd/system/felhom-shared-parent.service"}, false}, {"parent-unit-enable", "shared-parent boot-persistence enable", "/usr/bin/systemctl", []string{"enable", "felhom-shared-parent.service"}, false}, {"parent-bind-mp8", "parent bind into guest at provision", "/usr/sbin/pct", []string{"set", "9201", "-mp8", "/mnt/felhom-drives"}, false}, @@ -75,7 +75,7 @@ var manifest = []Capability{ {"provision-onboot", "customer guest autostart (onboot)", "/usr/sbin/pct", []string{"set", "9201", "-onboot", "1"}, false}, // ---- Pre-start self-heal hook + guest lifecycle ---- - {"guesthook-install", "pre-start hook snippet install", "/usr/bin/install", []string{"-m", "0755", "--", "/tmp/felhom-guest-hook.sh", "/var/lib/vz/snippets/felhom-guest-hook.sh"}, false}, + {"guesthook-install", "pre-start hook snippet install", "/usr/bin/install", []string{"-m", "0755", "--", "/tmp/felhom-guest-hook-123456789.sh", "/var/lib/vz/snippets/felhom-guest-hook.sh"}, false}, {"guesthook-register", "pre-start hook register", "/usr/sbin/pct", []string{"set", "9201", "--hookscript", "local:snippets/felhom-guest-hook.sh"}, false}, {"guesthook-delete-mp", "dead mountpoint slot delete (C1 net)", "/usr/sbin/pct", []string{"set", "9201", "--delete", "mp0"}, false}, {"guest-reboot", "enroll activate-binds reboot", "/usr/sbin/pct", []string{"reboot", "9201"}, false}, diff --git a/internal/guesthook/install.go b/internal/guesthook/install.go index 4138067..0fb819a 100644 --- a/internal/guesthook/install.go +++ b/internal/guesthook/install.go @@ -36,12 +36,23 @@ exec ` + AgentBin + ` guest-hook "$1" "$2" // InstallSnippet writes the pre-start hook wrapper into the PVE snippets dir (idempotent, root-owned, // executable). The agent runs as a non-root service user, so it writes an agent-writable temp file then // `install`s it host-root (same pattern as the bootstrap mount + dnsmasq drop-ins). Safe to call repeatedly. +// The temp file is a RANDOM-named os.CreateTemp (audit B1): a fixed, predictable /tmp name could be +// pre-created by another local user and rewritten between our write and root's install (TOCTOU into a +// root-executed hookscript). The final mode comes from `install -m`, so the 0600 temp is fine. func InstallSnippet(ctx context.Context, runner proxmox.Runner) error { - tmp := filepath.Join(os.TempDir(), "felhom-guest-hook.sh") - if err := os.WriteFile(tmp, []byte(snippetBody), 0o755); err != nil { + f, err := os.CreateTemp("", "felhom-guest-hook-*.sh") + if err != nil { + return fmt.Errorf("guesthook: create temp snippet: %w", err) + } + tmp := f.Name() + defer os.Remove(tmp) + if _, err := f.WriteString(snippetBody); err != nil { + f.Close() return fmt.Errorf("guesthook: write temp snippet: %w", err) } - defer os.Remove(tmp) + if err := f.Close(); err != nil { + return fmt.Errorf("guesthook: close temp snippet: %w", err) + } if _, stderr, err := runner.Run(ctx, "install", "-m", "0755", "--", tmp, SnippetPath); err != nil { return fmt.Errorf("guesthook: install snippet to %s: %w: %s", SnippetPath, err, string(stderr)) } diff --git a/internal/guesthook/install_test.go b/internal/guesthook/install_test.go new file mode 100644 index 0000000..590977e --- /dev/null +++ b/internal/guesthook/install_test.go @@ -0,0 +1,83 @@ +package guesthook + +import ( + "context" + "io" + "os" + "regexp" + "testing" +) + +// recordingRunner is a fake proxmox.Runner that records every call and snapshots the content of the +// install SOURCE file at call time (the deferred os.Remove would erase it before the test can look). +type recordingRunner struct { + calls [][]string + srcContent []string +} + +func (r *recordingRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + if name == "install" && len(args) > 0 { + src := args[len(args)-2] + b, _ := os.ReadFile(src) + r.srcContent = append(r.srcContent, string(b)) + } + return nil, nil, nil +} + +func (r *recordingRunner) RunStdin(ctx context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) { + return r.Run(ctx, name, args...) +} + +// TestInstallSnippet_RandomTempName is the audit-B1 negative test: the staged install SOURCE must be a +// RANDOM os.CreateTemp name (felhom-guest-hook-.sh), never the fixed, pre-creatable +// /tmp/felhom-guest-hook.sh (a local TOCTOU into a root-executed hookscript), and two consecutive +// installs must stage through DIFFERENT paths. +func TestInstallSnippet_RandomTempName(t *testing.T) { + r := &recordingRunner{} + if err := InstallSnippet(context.Background(), r); err != nil { + t.Fatalf("InstallSnippet #1: %v", err) + } + if err := InstallSnippet(context.Background(), r); err != nil { + t.Fatalf("InstallSnippet #2: %v", err) + } + if len(r.calls) != 2 { + t.Fatalf("expected 2 install calls, got %d: %v", len(r.calls), r.calls) + } + + randomName := regexp.MustCompile(`felhom-guest-hook-[^/\\]+\.sh$`) + fixedName := regexp.MustCompile(`felhom-guest-hook\.sh$`) + var srcs []string + for i, call := range r.calls { + // install -m 0755 -- + if call[0] != "install" || len(call) != 6 { + t.Fatalf("call %d: unexpected vector %v", i, call) + } + src, dest := call[4], call[5] + if dest != SnippetPath { + t.Errorf("call %d: dest = %q, want %q", i, dest, SnippetPath) + } + if !randomName.MatchString(src) { + t.Errorf("call %d: source %q does not match the random felhom-guest-hook-*.sh pattern", i, src) + } + if fixedName.MatchString(src) { + t.Errorf("call %d: source %q is the FIXED predictable temp name (B1 TOCTOU)", i, src) + } + srcs = append(srcs, src) + } + if srcs[0] == srcs[1] { + t.Errorf("two consecutive installs staged through the SAME source path %q — must be random per call", srcs[0]) + } + // Non-hollow: the staged file must actually carry the snippet body at install time. + for i, c := range r.srcContent { + if c != snippetBody { + t.Errorf("call %d: staged content is not the snippet body (got %d bytes)", i, len(c)) + } + } + // And the temp is cleaned up after. + for _, src := range srcs { + if _, err := os.Stat(src); err == nil { + t.Errorf("staged temp %q left behind (defer os.Remove missing)", src) + } + } +} diff --git a/internal/localapi/blankformat_test.go b/internal/localapi/blankformat_test.go new file mode 100644 index 0000000..dd0a0a2 --- /dev/null +++ b/internal/localapi/blankformat_test.go @@ -0,0 +1,147 @@ +package localapi + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// ---- Audit D3: the blank-format branch's anti-retarget guard ------------------------------ +// +// AGENT-001 closed the classify→mkfs TOCTOU on the data-bearing branch; these tests pin the same +// guard onto the BLANK branch: the format must bind to a durable id, re-resolve it to the CURRENT +// device, and format THAT — never the mutable caller-supplied /dev path. + +// Pure-function coverage of antiRetargetResolveBlank (the injected-deps sibling; no real /dev). +func TestFormatBlankPath_AntiRetarget_ReassignedDataBearingRefused(t *testing.T) { + const durable = "byid:wwn-0xBLANK" + dataBearing := storage.DeviceProbe{Probed: true, HasFilesystem: true, FSType: "ext4"} + got, err := antiRetargetResolveBlank(durable, + func(string) (string, error) { return "/dev/sdb", nil }, + func(string) (string, error) { return durable, nil }, + func(string) (storage.DeviceProbe, error) { return dataBearing, nil }, + ) + if err == nil { + t.Fatalf("re-inspect found a DATA-BEARING disk on the blank path — must refuse, got device %q", got) + } + if !strings.Contains(err.Error(), "now data-bearing") { + t.Errorf("error %q missing the now-data-bearing explanation", err.Error()) + } +} + +func TestFormatBlankPath_AntiRetarget_ReassignedDifferentDiskRefused(t *testing.T) { + const durable = "byid:wwn-0xBLANK" + blank := storage.DeviceProbe{Probed: true} + got, err := antiRetargetResolveBlank(durable, + func(string) (string, error) { return "/dev/sdb", nil }, + func(string) (string, error) { return "byid:wwn-0xDIFFERENT", nil }, // node reassigned → different id + func(string) (storage.DeviceProbe, error) { return blank, nil }, + ) + if err == nil || !strings.Contains(err.Error(), "durable-id mismatch") { + t.Fatalf("want durable-id mismatch refusal, got device %q err %v", got, err) + } +} + +func TestFormatBlankPath_AntiRetarget_UnresolvableRefused(t *testing.T) { + blank := storage.DeviceProbe{Probed: true} + // Unresolvable durable id → refuse. + if got, err := antiRetargetResolveBlank("byid:wwn-0xGONE", + func(string) (string, error) { return "", errors.New("gone") }, + func(string) (string, error) { return "byid:wwn-0xGONE", nil }, + func(string) (storage.DeviceProbe, error) { return blank, nil }, + ); err == nil || !strings.Contains(err.Error(), "no longer resolves") { + t.Fatalf("want no-longer-resolves refusal, got device %q err %v", got, err) + } + // Empty durable id (nothing to bind) → refuse: a path-only blank format is what the guard prevents. + if got, err := antiRetargetResolveBlank("", + func(string) (string, error) { return "/dev/sdb", nil }, + func(string) (string, error) { return "", nil }, + func(string) (storage.DeviceProbe, error) { return blank, nil }, + ); err == nil || !strings.Contains(err.Error(), "path-only") { + t.Fatalf("want path-only refusal on empty durable id, got device %q err %v", got, err) + } +} + +func TestFormatBlankPath_AntiRetarget_SameBlankProceeds(t *testing.T) { + const durable = "byid:wwn-0xBLANK" + blank := storage.DeviceProbe{Probed: true} + got, err := antiRetargetResolveBlank(durable, + func(string) (string, error) { return "/dev/sdb", nil }, + func(string) (string, error) { return durable, nil }, + func(string) (storage.DeviceProbe, error) { return blank, nil }, + ) + if err != nil { + t.Fatalf("same still-blank disk must proceed, got refusal: %v", err) + } + if got != "/dev/sdb" { + t.Fatalf("device = %q, want the re-resolved /dev/sdb", got) + } +} + +// ---- handler wiring: the blank branch formats the RE-RESOLVED device, never req.Device ---- + +// The red-proof companion vs the pre-fix handler: pre-fix, the blank branch called +// startFormatDetached(req.Device, ...) directly, so the format hit the caller path even when the +// re-resolve seam says the durable id now lives at a different node. Post-fix, mkfs must run on the +// re-resolved device. +func TestFormat_Blank_FormatsReresolvedDeviceNotCallerPath(t *testing.T) { + d := &fakeDiskOps{probe: storage.DeviceProbe{Probed: true}} // blank + srv := newDiskServerRaw(t, d, &fakeGate{}, nil, nil) + var boundID string + srv.reresolveBlank = func(_ context.Context, durableID string) (string, error) { + boundID = durableID + return "/dev/sdz", nil // the durable id's CURRENT node differs from the caller path + } + h := srv.Handler() + + w := do(t, h, "POST", "/disks/format", "A", `{"device":"/dev/sdb","fstype":"ext4"}`) + if w.Code != http.StatusOK { + t.Fatalf("blank format: got %d want 200 (%s)", w.Code, w.Body.String()) + } + if boundID != "byid:wwn-sdb" { + t.Errorf("blank format bound durable id %q, want the one derived from req.Device", boundID) + } + got := d.formatted() + if len(got) != 1 || got[0] != "/dev/sdz" { + t.Fatalf("mkfs ran on %v — must run on the RE-RESOLVED device /dev/sdz, never the caller path /dev/sdb", got) + } +} + +// A blank-branch anti-retarget refusal (device changed in the window) must refuse 409 with ZERO mkfs. +func TestFormat_Blank_ReresolveRefusalNoMkfs(t *testing.T) { + d := &fakeDiskOps{probe: storage.DeviceProbe{Probed: true}} // blank + srv := newDiskServerRaw(t, d, &fakeGate{}, nil, nil) + srv.reresolveBlank = func(_ context.Context, durableID string) (string, error) { + return "", fmt.Errorf("%s is now data-bearing (target changed since blank inspection) — refusing", "/dev/sdb") + } + h := srv.Handler() + + w := do(t, h, "POST", "/disks/format", "A", `{"device":"/dev/sdb","fstype":"ext4"}`) + if w.Code != http.StatusConflict { + t.Fatalf("refused blank format: got %d want 409 (%s)", w.Code, w.Body.String()) + } + if got := d.formatted(); len(got) != 0 { + t.Fatalf("mkfs WAS called after an anti-retarget refusal: %v", got) + } +} + +// A device with no durable id cannot be bound → the blank format is refused (no path-only formats). +func TestFormat_Blank_NoDurableIDRefused(t *testing.T) { + d := &fakeDiskOps{probe: storage.DeviceProbe{Probed: true}} // blank + srv := newDiskServerRaw(t, d, &fakeGate{}, nil, nil) + srv.deviceDurableID = func(string) (string, error) { return "", errors.New("no by-id/by-uuid entry") } + h := srv.Handler() + + w := do(t, h, "POST", "/disks/format", "A", `{"device":"/dev/sdb","fstype":"ext4"}`) + if w.Code != http.StatusConflict { + t.Fatalf("unbindable blank format: got %d want 409 (%s)", w.Code, w.Body.String()) + } + if got := d.formatted(); len(got) != 0 { + t.Fatalf("mkfs WAS called with no durable-id binding: %v", got) + } +} diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index 6caee5e..554e62e 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -678,16 +678,41 @@ func (s *Server) handleDiskFormat(w http.ResponseWriter, r *http.Request, vmid i // Blank device → benign → mkfs (role is irrelevant; there is nothing to destroy). F20-BUG3: run // it DETACHED off s.baseCtx so a request/client deadline can't SIGKILL mkfs mid-write; we still // wait here to return the synchronous result (backward-compatible with the controller's client). - done := s.startFormatDetached(req.Device, "", req.FSType) - if err := s.awaitFormat(r.Context(), done, vmid, req.Device); err != nil { + // + // [audit D3, AGENT-001's benign-branch twin] anti-retarget: bind the format to the device's + // durable id and re-resolve it to the CURRENT device (re-derive + exact match + re-inspect + // STILL blank) — then format THAT device, never the mutable req.Device path. A /dev + // re-enumeration in the window could otherwise mkfs a data-bearing disk that inherited the + // node, with neither the DataBearing customer-confirm nor any durable-id binding. A device with + // no durable id cannot be bound → refused (a path-only format is what the guard prevents). + blankDurable, derr := s.deviceDurableID(req.Device) + if derr != nil || blankDurable == "" { + s.logger.Warn("local-api: blank format REFUSED — device has no durable id to bind (anti-retarget)", + "vmid", vmid, "device", req.Device, "err", derr) + writeStatus(w, http.StatusConflict, false, + FormatResponse{VMID: vmid, Device: req.Device, Formatted: false, DataBearing: false}, + "format refused: device has no durable id to bind the format to (path-only formats are not permitted)") + return + } + device, rerr := s.reresolveBlank(r.Context(), blankDurable) + if rerr != nil { + s.logger.Warn("local-api: blank format REFUSED at anti-retarget re-resolve", + "vmid", vmid, "req_device", req.Device, "durable_id", blankDurable, "err", rerr) + writeStatus(w, http.StatusConflict, false, + FormatResponse{VMID: vmid, Device: req.Device, Formatted: false, DataBearing: false, DurableID: blankDurable}, + "format refused (device may have changed since inspection): "+rerr.Error()) + return + } + done := s.startFormatDetached(device, blankDurable, req.FSType, true) + if err := s.awaitFormat(r.Context(), done, vmid, device); err != nil { if err == errFormatClientGone { return // client gone; mkfs continues detached + the job record records the outcome } - s.logger.Error("local-api: format", "vmid", vmid, "device", req.Device, "err", err) + s.logger.Error("local-api: format", "vmid", vmid, "device", device, "err", err) writeErr(w, http.StatusBadGateway, "format failed: "+err.Error()) return } - writeOK(w, FormatResponse{VMID: vmid, Device: req.Device, Formatted: true, DataBearing: false, Reason: "blank device formatted " + req.FSType}) + writeOK(w, FormatResponse{VMID: vmid, Device: device, Formatted: true, DataBearing: false, DurableID: blankDurable, Reason: "blank device formatted " + req.FSType}) return } @@ -724,7 +749,7 @@ func (s *Server) handleDiskFormat(w http.ResponseWriter, r *http.Request, vmid i // F20-BUG3: run the destructive mkfs DETACHED off s.baseCtx (bound durable id recorded for // restart-recovery), so a request/client deadline can never SIGKILL it mid-write and corrupt the // disk. We still wait to return the synchronous result (backward-compatible with the controller). - done := s.startFormatDetached(device, deviceDurable, req.FSType) + done := s.startFormatDetached(device, deviceDurable, req.FSType, false) if err := s.awaitFormat(r.Context(), done, vmid, device); err != nil { if err == errFormatClientGone { return // client gone; the wipe continues detached + survives a restart via the job record diff --git a/internal/localapi/disks_test.go b/internal/localapi/disks_test.go index db6642b..b33c5a8 100644 --- a/internal/localapi/disks_test.go +++ b/internal/localapi/disks_test.go @@ -103,6 +103,13 @@ func sysOnSDA() fakeHostReader { // newDiskServer builds a server wired with the 8C disk deps (token A → guest 8200). func newDiskServer(t *testing.T, d *fakeDiskOps, g *fakeGate, sv StorageView, gl GuestLister) http.Handler { + t.Helper() + return newDiskServerRaw(t, d, g, sv, gl).Handler() +} + +// newDiskServerRaw is newDiskServer returning the *Server, so tests can override seams +// (reresolveWipe/reresolveBlank/deviceDurableID) before taking the handler. +func newDiskServerRaw(t *testing.T, d *fakeDiskOps, g *fakeGate, sv StorageView, gl GuestLister) *Server { t.Helper() if sv == nil { sv = fakeStorage{} @@ -129,10 +136,13 @@ func newDiskServer(t *testing.T, d *fakeDiskOps, g *fakeGate, sv StorageView, gl // device the format tests use; antiRetargetResolve itself is covered directly // in wipe_reresolve_test.go. srv.reresolveWipe = func(_ context.Context, _ string) (string, error) { return "/dev/sdb", nil } + // [audit D3] the blank-path anti-retarget sibling touches /dev/disk/by-* too; stub it to a + // successful still-blank re-resolve of the same device. Covered directly in wipe_reresolve_test.go. + srv.reresolveBlank = func(_ context.Context, _ string) (string, error) { return "/dev/sdb", nil } // F20-BUG2: the wipe id derivation hits /dev/disk/by-* in production; stub it deterministically so // both the /disks list and the gate (which share this seam) resolve the same id in tests. srv.deviceDurableID = func(device string) (string, error) { return "byid:wwn-" + strings.TrimPrefix(device, "/dev/"), nil } - return srv.Handler() + return srv } // ---- the security centerpiece ----------------------------------------------------------- diff --git a/internal/localapi/formatjob.go b/internal/localapi/formatjob.go index d38e574..02acec7 100644 --- a/internal/localapi/formatjob.go +++ b/internal/localapi/formatjob.go @@ -17,9 +17,10 @@ import ( type formatJob struct { JobID string `json:"job_id"` Device string `json:"device"` - DurableID string `json:"durable_id"` // "" for a blank (benign) format — never auto-recovered + DurableID string `json:"durable_id"` // durable-id binding; "" only in legacy records (never auto-recovered) FSType string `json:"fstype"` - Phase string `json:"phase"` // running | done | failed + Blank bool `json:"blank,omitempty"` // audit D3: blank (benign) format — recovery re-checks STILL-blank, not data-bearing + Phase string `json:"phase"` // running | done | failed Error string `json:"error,omitempty"` StartedAt string `json:"started_at"` UpdatedAt string `json:"updated_at"` @@ -93,15 +94,16 @@ func (s *FormatJobStore) save(j *formatJob) error { // request context), with a long bound. It returns a channel that yields the format error (nil on // success). The caller may stop waiting (client disconnect) without killing the mkfs — the goroutine // runs to completion and records the outcome. device is the ALREADY anti-retarget-resolved device; the -// record carries durableID so a restart can re-resolve + re-run. -func (s *Server) startFormatDetached(device, durableID, fstype string) <-chan error { +// record carries durableID so a restart can re-resolve + re-run. blank marks a benign (blank-device) +// format, so restart recovery re-checks STILL-blank rather than data-bearing (audit D3). +func (s *Server) startFormatDetached(device, durableID, fstype string, blank bool) <-chan error { base := s.baseCtx if base == nil { base = context.Background() } job := &formatJob{ JobID: s.nowFn().UTC().Format("20060102T150405Z"), Device: device, DurableID: durableID, - FSType: fstype, Phase: formatPhaseRunning, + FSType: fstype, Blank: blank, Phase: formatPhaseRunning, StartedAt: s.nowFn().UTC().Format(time.RFC3339), UpdatedAt: s.nowFn().UTC().Format(time.RFC3339), } if s.formatJobs != nil { @@ -151,18 +153,26 @@ func (s *Server) RecoverFormatJob(ctx context.Context) { return } if job.DurableID == "" { - s.logger.Warn("format-job recover: interrupted blank format — marking failed (retry needed; not auto-re-running a path-bound format)", "device", job.Device) + s.logger.Warn("format-job recover: interrupted format has no durable id (legacy record) — marking failed (retry needed; not auto-re-running a path-bound format)", "device", job.Device) s.finishFormatJob(job, fmt.Errorf("interrupted by agent restart; retry the format")) return } - device, err := s.reresolveWipe(ctx, job.DurableID) + // Audit D3: a blank format authorized "nothing to destroy" — its recovery re-check must assert + // STILL-blank (an interrupted mkfs may leave partial signatures; if the re-resolved device probes + // data-bearing the blank re-check refuses fail-safe and the caller retries). The customer-confirmed + // wipe path keeps the data-bearing re-check as before. + reresolve := s.reresolveWipe + if job.Blank { + reresolve = s.reresolveBlank + } + device, err := reresolve(ctx, job.DurableID) if err != nil { - s.logger.Warn("format-job recover: durable-id no longer resolves — NOT re-formatting (anti-retarget)", "durable_id", job.DurableID, "err", err) + s.logger.Warn("format-job recover: durable-id did not re-resolve cleanly — NOT re-formatting (anti-retarget)", "durable_id", job.DurableID, "blank", job.Blank, "err", err) s.finishFormatJob(job, fmt.Errorf("durable-id %s did not re-resolve after restart: %w", job.DurableID, err)) return } - s.logger.Warn("format-job recover: re-running interrupted format detached", "durable_id", job.DurableID, "device", device, "fstype", job.FSType) - _ = s.startFormatDetached(device, job.DurableID, job.FSType) // detached; updates the record on completion + s.logger.Warn("format-job recover: re-running interrupted format detached", "durable_id", job.DurableID, "device", device, "fstype", job.FSType, "blank", job.Blank) + _ = s.startFormatDetached(device, job.DurableID, job.FSType, job.Blank) // detached; updates the record on completion } // nowFn returns the server clock (testable), defaulting to time.Now. diff --git a/internal/localapi/intermediary.go b/internal/localapi/intermediary.go index 5912d7f..42f30a3 100644 --- a/internal/localapi/intermediary.go +++ b/internal/localapi/intermediary.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "os" - "path/filepath" "strconv" "strings" ) @@ -130,6 +129,26 @@ func (b *GuestBinder) EnsureSharedParent(ctx context.Context) error { return nil } +// stageTemp writes content to a fresh random-named temp file (os.CreateTemp pattern — `*` is replaced +// by a random string) and returns its path. Caller removes it after the privileged `install`. +func stageTemp(pattern, content string) (string, error) { + f, err := os.CreateTemp("", pattern) + if err != nil { + return "", err + } + name := f.Name() + if _, err := f.WriteString(content); err != nil { + f.Close() + os.Remove(name) + return "", err + } + if err := f.Close(); err != nil { + os.Remove(name) + return "", err + } + return name, nil +} + // sharedParentInstallStale reports whether the on-disk boot script OR unit is missing or differs from // what this build ships — the trigger to (re)install both. Comparing BOTH (not the unit alone) is the // F2-a fix: a script-only change must still redeploy. Pure (path args) so it is unit-testable. @@ -144,18 +163,21 @@ func sharedParentInstallStale(unitPath, scriptPath string) bool { } // installSharedParentUnit writes the script + unit (from agent-written temps) and enables the unit so the -// shared parent is re-established on every host boot before pve-guests. Idempotent. +// shared parent is re-established on every host boot before pve-guests. Idempotent. The temps are +// RANDOM-named os.CreateTemp files (audit B1): a fixed, predictable /tmp name could be pre-created by +// another local user and rewritten between our write and root's install (TOCTOU into a root-executed +// boot script). The final modes come from `install -m`, so the 0600 temps are fine. func (b *GuestBinder) installSharedParentUnit(ctx context.Context) error { - tmpScript := filepath.Join(os.TempDir(), "felhom-shared-parent.sh") - if err := os.WriteFile(tmpScript, []byte(sharedParentScript), 0o755); err != nil { + tmpScript, err := stageTemp("felhom-shared-parent-*.sh", sharedParentScript) + if err != nil { return fmt.Errorf("write temp script: %w", err) } defer os.Remove(tmpScript) if err := b.run(ctx, "install", "-m", "0755", "--", tmpScript, sharedParentScriptPath); err != nil { return fmt.Errorf("install script: %w", err) } - tmpUnit := filepath.Join(os.TempDir(), "felhom-shared-parent.service") - if err := os.WriteFile(tmpUnit, []byte(sharedParentUnit), 0o644); err != nil { + tmpUnit, err := stageTemp("felhom-shared-parent-*.service", sharedParentUnit) + if err != nil { return fmt.Errorf("write temp unit: %w", err) } defer os.Remove(tmpUnit) diff --git a/internal/localapi/intermediary_install_test.go b/internal/localapi/intermediary_install_test.go new file mode 100644 index 0000000..4cdb4aa --- /dev/null +++ b/internal/localapi/intermediary_install_test.go @@ -0,0 +1,97 @@ +package localapi + +import ( + "context" + "io" + "os" + "regexp" + "testing" +) + +// stagingRecorderRunner is a fake proxmox.Runner recording every call vector and snapshotting each +// install SOURCE file's content at call time (the deferred os.Remove erases it afterwards). +type stagingRecorderRunner struct { + calls [][]string + srcContent map[string]string // install dest → staged source content +} + +func (r *stagingRecorderRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + if name == "install" && len(args) >= 2 { + src, dest := args[len(args)-2], args[len(args)-1] + if r.srcContent == nil { + r.srcContent = map[string]string{} + } + b, _ := os.ReadFile(src) + r.srcContent[dest] = string(b) + } + return nil, nil, nil +} + +func (r *stagingRecorderRunner) RunStdin(ctx context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) { + return r.Run(ctx, name, args...) +} + +// installSources returns the install-call source paths keyed by destination. +func (r *stagingRecorderRunner) installSources() map[string][]string { + out := map[string][]string{} + for _, c := range r.calls { + if c[0] == "install" && len(c) >= 3 { + src, dest := c[len(c)-2], c[len(c)-1] + out[dest] = append(out[dest], src) + } + } + return out +} + +// TestInstallSharedParent_RandomTempName is the audit-B1 negative test for the shared-parent boot +// persistence install: both staged install SOURCES (script + unit) must be RANDOM os.CreateTemp names +// (felhom-shared-parent-.sh / .service), never the fixed, pre-creatable /tmp names (a local +// TOCTOU into a root-executed boot script), and two consecutive installs must use DIFFERENT paths. +func TestInstallSharedParent_RandomTempName(t *testing.T) { + r := &stagingRecorderRunner{} + b := NewGuestBinder(r, nil) + if err := b.installSharedParentUnit(context.Background()); err != nil { + t.Fatalf("installSharedParentUnit #1: %v", err) + } + if err := b.installSharedParentUnit(context.Background()); err != nil { + t.Fatalf("installSharedParentUnit #2: %v", err) + } + + srcs := r.installSources() + cases := []struct { + dest string + random *regexp.Regexp + fixed *regexp.Regexp + content string + }{ + {sharedParentScriptPath, regexp.MustCompile(`felhom-shared-parent-[^/\\]+\.sh$`), + regexp.MustCompile(`felhom-shared-parent\.sh$`), sharedParentScript}, + {sharedParentUnitPath, regexp.MustCompile(`felhom-shared-parent-[^/\\]+\.service$`), + regexp.MustCompile(`felhom-shared-parent\.service$`), sharedParentUnit}, + } + for _, tc := range cases { + got := srcs[tc.dest] + if len(got) != 2 { + t.Fatalf("dest %s: expected 2 install calls, got %d (%v)", tc.dest, len(got), got) + } + for i, src := range got { + if !tc.random.MatchString(src) { + t.Errorf("dest %s call %d: source %q does not match the random temp pattern", tc.dest, i, src) + } + if tc.fixed.MatchString(src) { + t.Errorf("dest %s call %d: source %q is the FIXED predictable temp name (B1 TOCTOU)", tc.dest, i, src) + } + if _, err := os.Stat(src); err == nil { + t.Errorf("dest %s: staged temp %q left behind (defer os.Remove missing)", tc.dest, src) + } + } + if got[0] == got[1] { + t.Errorf("dest %s: two consecutive installs staged through the SAME source %q — must be random per call", tc.dest, got[0]) + } + // Non-hollow: the staged file carried the real content at install time. + if r.srcContent[tc.dest] != tc.content { + t.Errorf("dest %s: staged content mismatch (got %d bytes, want %d)", tc.dest, len(r.srcContent[tc.dest]), len(tc.content)) + } + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index cc16378..9936df1 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -186,6 +186,11 @@ type Server struct { // override it to avoid touching real /dev. reresolveWipe func(ctx context.Context, durableID string) (string, error) + // reresolveBlank is the BLANK-format sibling (audit D3): same anti-retarget + // sequence, but requires the re-inspected device to STILL be blank. Defaults + // to s.reresolveDurableForBlankFormat; tests override it. + reresolveBlank func(ctx context.Context, durableID string) (string, error) + // deviceDurableID derives the WIPE-binding durable id of a block device (the byid:/byuuid: scheme // the wipe gate resolves against). F20-BUG2: BOTH the /disks list (DiskInfo.WipeDurableID) and the // format gate use this single seam, so the id the customer copies from the list is exactly the id @@ -253,6 +258,7 @@ func NewServer(o Options) (*Server, error) { swapInFlight: map[int]bool{}, } s.reresolveWipe = s.reresolveDurableForWipe + s.reresolveBlank = s.reresolveDurableForBlankFormat s.deviceDurableID = storage.DeviceDurableID if o.ControllerSwap != nil { s.swap = NewControllerSwapper(o.ControllerSwap, o.ControllerSwapStateDir, o.Logger) diff --git a/internal/localapi/wipe_reresolve.go b/internal/localapi/wipe_reresolve.go index 8127ae4..bf3904a 100644 --- a/internal/localapi/wipe_reresolve.go +++ b/internal/localapi/wipe_reresolve.go @@ -27,6 +27,35 @@ func antiRetargetResolve( resolve func(string) (string, error), derive func(string) (string, error), inspect func(string) (storage.DeviceProbe, error), +) (string, error) { + return antiRetargetResolveExpect(durableID, true, resolve, derive, inspect) +} + +// antiRetargetResolveBlank is the BLANK-format sibling [audit D3, AGENT-001's benign-branch twin]: the +// same resolve → re-derive-match → re-inspect sequence, but it requires the re-inspected device to +// STILL be blank (!DataBearing). The blank branch authorized mkfs precisely because the agent read the +// device as having nothing to destroy; if the /dev node was reassigned in the window and now holds +// data, the target changed — refuse rather than format it with neither a customer confirm nor a bound +// durable id. +func antiRetargetResolveBlank( + durableID string, + resolve func(string) (string, error), + derive func(string) (string, error), + inspect func(string) (storage.DeviceProbe, error), +) (string, error) { + return antiRetargetResolveExpect(durableID, false, resolve, derive, inspect) +} + +// antiRetargetResolveExpect is the shared core: resolve the bound durable id to the CURRENT device, +// re-derive the device's durable id and require an exact match, then re-inspect and require the +// data-bearing state to still be what the caller authorized (expectDataBearing). Returns the +// re-resolved device to format — never a caller-supplied path. +func antiRetargetResolveExpect( + durableID string, + expectDataBearing bool, + resolve func(string) (string, error), + derive func(string) (string, error), + inspect func(string) (storage.DeviceProbe, error), ) (string, error) { if durableID == "" { // A path-only binding is exactly what the durable id exists to prevent. @@ -50,11 +79,16 @@ func antiRetargetResolve( if !probe.Probed { return "", fmt.Errorf("%s did not probe cleanly at execution — refusing", device) } - if !probe.DataBearing() { + if expectDataBearing && !probe.DataBearing() { // The customer confirmed wiping a DATA-BEARING device; if it is now blank, // the target changed since confirmation — refuse rather than wipe blindly. return "", fmt.Errorf("%s is no longer data-bearing (target changed since confirmation) — refusing", device) } + if !expectDataBearing && probe.DataBearing() { + // The agent authorized a BLANK format; the device is now data-bearing — + // the target changed since inspection (D3 re-enumeration race) — refuse. + return "", fmt.Errorf("%s is now data-bearing (target changed since blank inspection) — refusing", device) + } return device, nil } @@ -69,3 +103,14 @@ func (s *Server) reresolveDurableForWipe(ctx context.Context, durableID string) func(dev string) (storage.DeviceProbe, error) { return s.disks.InspectDevice(ctx, dev) }, ) } + +// reresolveDurableForBlankFormat wires antiRetargetResolveBlank with the real storage funcs and the +// server's disk inspector (audit D3 — the blank-format branch's pre-mkfs anti-retarget re-check). +func (s *Server) reresolveDurableForBlankFormat(ctx context.Context, durableID string) (string, error) { + return antiRetargetResolveBlank( + durableID, + storage.ResolveDurableDevice, + storage.DeviceDurableID, + func(dev string) (storage.DeviceProbe, error) { return s.disks.InspectDevice(ctx, dev) }, + ) +} diff --git a/internal/storage/claim.go b/internal/storage/claim.go index 0527ed3..bf143ec 100644 --- a/internal/storage/claim.go +++ b/internal/storage/claim.go @@ -85,6 +85,24 @@ func classifyClaim(f claimFacts) (unclaimed bool, reason string) { return false, "device is mounted at " + n.mountpoint + " (" + n.name + ")" } } + // Fail-safe backstop (audit D2): a successful-but-EMPTY lsblk (or a tree that does not even contain + // the target whole-disk) means the member/mount loop above inspected nothing — that is undeterminable + // topology, not proof of freedom. Without this, "unclaimed" rested on the untested assumption that + // lsblk always ERRORS (non-zero exit) on a bad device rather than emitting empty success. + if len(f.nodes) == 0 { + return false, "empty block topology (undeterminable) — refusing" + } + base := path.Base(f.wholeDisk) + found := false + for _, n := range f.nodes { + if n.name == base { + found = true + break + } + } + if !found { + return false, "target disk " + base + " absent from block topology (undeterminable) — refusing" + } return true, "unclaimed" } diff --git a/internal/storage/claim_test.go b/internal/storage/claim_test.go index d9e8039..6289cc8 100644 --- a/internal/storage/claim_test.go +++ b/internal/storage/claim_test.go @@ -64,6 +64,36 @@ func TestClassifyClaim(t *testing.T) { } } +// TestClassifyClaim_EmptyNodesRefused is the audit-D2 negative test: a successful-but-EMPTY lsblk +// (`{"blockdevices":[]}` → zero nodes) previously skipped the member/mount loop entirely and returned +// (true,"unclaimed") — the one hole in the "undeterminable ⇒ claimed" fail-safe. It must refuse. +func TestClassifyClaim_EmptyNodesRefused(t *testing.T) { + for _, nodes := range [][]claimNode{nil, {}} { + f := claimFacts{device: "/dev/sdd", wholeDisk: "/dev/sdd", wholeDiskOK: true, nodes: nodes} + unclaimed, reason := classifyClaim(f) + if unclaimed { + t.Fatalf("nodes=%v: empty topology classified UNCLAIMED (reason %q) — fail-safe hole", nodes, reason) + } + if !strings.Contains(reason, "empty block topology") { + t.Errorf("nodes=%v: reason %q missing the empty-topology explanation", nodes, reason) + } + } +} + +// TestClassifyClaim_TargetAbsentFromTree (audit D2): lsblk returned SOME tree, but the target +// whole-disk is not in it — the loop inspected the wrong device's signals. Undeterminable → refuse. +func TestClassifyClaim_TargetAbsentFromTree(t *testing.T) { + f := claimFacts{device: "/dev/sdd", wholeDisk: "/dev/sdd", wholeDiskOK: true, + nodes: []claimNode{{name: "sdc"}, {name: "sdc1", fstype: "ntfs"}}} // benign signals, wrong disk + unclaimed, reason := classifyClaim(f) + if unclaimed { + t.Fatalf("target-absent tree classified UNCLAIMED (reason %q)", reason) + } + if !strings.Contains(reason, "absent from block topology") { + t.Errorf("reason %q missing the target-absent explanation", reason) + } +} + func TestParseLsblkNodes(t *testing.T) { out := []byte(`{"blockdevices":[{"name":"sdd","fstype":null,"mountpoint":null,"children":[{"name":"sdd1","fstype":"ntfs","mountpoint":null}]}]}`) nodes, err := parseLsblkNodes(out) diff --git a/scripts/mkfs-guarded-harness.sh b/scripts/mkfs-guarded-harness.sh new file mode 100644 index 0000000..7fb083b --- /dev/null +++ b/scripts/mkfs-guarded-harness.sh @@ -0,0 +1,107 @@ +#!/bin/bash +#=============================================================================== +# mkfs-guarded-harness — audit-D1 test harness for configs/felhom-mkfs-guarded.sh. +# +# Runs the REAL wrapper against throwaway loop devices with: +# - a PATH-shimmed `lsblk` that fakes the member FSTYPE signal (FIXTURE_FSTYPE), +# - a private mount namespace (unshare -m) that bind-mounts a RECORDER over +# /usr/sbin/mkfs.ext4 — so a wrapper that decides to format provably "formats" +# (the recorder logs the argv) while NO real mkfs can ever run, and +# - /dev/null bind-mounted over pvs for the pvs-absent case. +# +# Nothing real is formatted; nothing leaks outside the namespace; the loop files +# are throwaway temps. Run as root on a Linux host (losetup/unshare needed). +# +# Usage: mkfs-guarded-harness.sh +# Exit 0 = all cases behave like the FIXED wrapper should. Running it against the +# pre-fix wrapper is the red-proof: the member/RO cases will FAIL (recorder fired). +#=============================================================================== +set -euo pipefail + +WRAPPER="${1:?usage: mkfs-guarded-harness.sh }" +WRAPPER="$(readlink -f "$WRAPPER")" +[[ -r "$WRAPPER" ]] || { echo "no wrapper at $WRAPPER" >&2; exit 2; } +[[ "$(id -u)" == 0 ]] || { echo "must run as root (losetup/unshare)" >&2; exit 2; } + +WORK="$(mktemp -d /tmp/mkfs-harness.XXXXXX)" +LOOP=""; LOOP_RO="" +cleanup() { + [[ -n "$LOOP" ]] && losetup -d "$LOOP" 2>/dev/null || true + [[ -n "$LOOP_RO" ]] && losetup -d "$LOOP_RO" 2>/dev/null || true + rm -rf "$WORK" +} +trap cleanup EXIT + +# --- throwaway loop devices (a plain RW one + a kernel-read-only one) --- +truncate -s 64M "$WORK/plain.img" +truncate -s 64M "$WORK/ro.img" +LOOP="$(losetup -f --show "$WORK/plain.img")" +LOOP_RO="$(losetup -f --show -r "$WORK/ro.img")" +echo "loop devices: rw=$LOOP ro=$LOOP_RO" + +# --- PATH shim: lsblk fakes only the `-nro FSTYPE` read when FIXTURE_FSTYPE is set --- +mkdir -p "$WORK/shim" +cat > "$WORK/shim/lsblk" <<'EOF' +#!/bin/bash +if [[ -n "${FIXTURE_FSTYPE:-}" && "${1:-}" == "-nro" && "${2:-}" == "FSTYPE" ]]; then + echo "$FIXTURE_FSTYPE" + exit 0 +fi +exec /usr/bin/lsblk "$@" +EOF +chmod 0755 "$WORK/shim/lsblk" + +# --- the mkfs recorder bound over /usr/sbin/mkfs.ext4 inside the namespace --- +cat > "$WORK/fake-mkfs" <<'EOF' +#!/bin/bash +echo "mkfs.ext4 $*" >> "$MKFS_MARKER" +exit 0 +EOF +chmod 0755 "$WORK/fake-mkfs" + +# run_case +pass=0; fail=0 +run_case() { + local name="$1" dev="$2" fixture="$3" hidepvs="$4" want="$5" msg="$6" + local marker="$WORK/marker.$RANDOM"; rm -f "$marker" + local hide_cmds="" + if [[ "$hidepvs" == 1 ]]; then + for p in /usr/sbin/pvs /sbin/pvs; do + [[ -e "$p" ]] && hide_cmds+="mount --bind /dev/null $p; " + done + fi + set +e + out="$(unshare -m bash -c " + set -e + mount --bind '$WORK/fake-mkfs' /usr/sbin/mkfs.ext4 + $hide_cmds + export PATH='$WORK/shim':\$PATH MKFS_MARKER='$marker' FIXTURE_FSTYPE='$fixture' + exec bash '$WRAPPER' '$dev' ext4 + " 2>&1)" + rc=$? + set -e + local fired=no; [[ -s "$marker" ]] && fired=yes + local verdict=FAIL + if [[ "$want" == refuse ]]; then + [[ $rc -ne 0 && "$fired" == no && "$out" == *"$msg"* ]] && verdict=PASS + else + [[ $rc -eq 0 && "$fired" == yes && "$(cat "$marker")" == *"$dev"* ]] && verdict=PASS + fi + if [[ "$verdict" == PASS ]]; then pass=$((pass+1)); else fail=$((fail+1)); fi + printf '%-4s %-38s rc=%-3s mkfs-fired=%-3s want=%s\n' "$verdict" "$name" "$rc" "$fired" "$want" + [[ "$verdict" == FAIL ]] && printf ' output: %s\n' "$out" + return 0 +} + +echo "--- wrapper under test: $WRAPPER ---" +run_case "refuse zfs_member" "$LOOP" "zfs_member" 0 refuse "zfs_member" +run_case "refuse linux_raid_member" "$LOOP" "linux_raid_member" 0 refuse "linux_raid_member" +run_case "refuse crypto_LUKS" "$LOOP" "crypto_LUKS" 0 refuse "crypto_LUKS" +run_case "refuse swap" "$LOOP" "swap" 0 refuse "swap" +run_case "refuse LVM2_member" "$LOOP" "LVM2_member" 0 refuse "LVM" +run_case "refuse read-only device" "$LOOP_RO" "" 0 refuse "read-only" +run_case "pvs-hidden still catches LVM" "$LOOP" "LVM2_member" 1 refuse "LVM" +run_case "plain blank disk formats" "$LOOP" "" 0 format "" + +echo "--- $pass passed, $fail failed ---" +[[ $fail -eq 0 ]]