From 703db166e762566f90926b61acb5fb547114dd69 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Thu, 6 Aug 2026 12:55:28 +0200 Subject: [PATCH] v0.127.0: a mount Felhom itself made is not 'something else' (R-220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a rebuild the customer's own drives could not be re-attached: candidates returned initialize:[] attach:[] while both drives sat there, and the deploy refused with 'choose an attached drive from the list' — a list that was empty. Measured live three times. Mechanism: enrolment mounts a drive TWICE, at /mnt/felhom-drives/ and at the raw /mnt/ it creates on the host. The host survives a guest rebuild; the controller's registry does not. So classifyClaim saw a mount outside the managed prefix and concluded 'claimed by something else' — about our own mount. The fix is CORROBORATED, not a widened prefix: a non-managed mountpoint is forgiven only when the SAME device is also mounted under the managed path, a pairing only our enrolment produces. A disk another system uses — /srv/data, /media/x, even /mnt/someone-elses-disk — has no counterpart and is STILL refused, with its own test and a red-proof showing an over-wide fix offering it for formatting. Read from /proc/mounts deliberately: the lsblk invocation is pinned verbatim in configs/felhom-agent.sudoers, so using the plural MOUNTPOINTS would have coupled this to a sudoers rollout. /proc/mounts is world-readable — no sudo, no new allowlisted command, no config change. Fail-safe: an unreadable mount table corroborates NOTHING, so the device classifies exactly as before. 'Could not corroborate' must never read as 'ours'. 29 packages ok, vet clean, agent gates OK. --- CHANGELOG.md | 33 +++++++++ REPORT.md | 60 +++++++--------- internal/storage/claim.go | 91 ++++++++++++++++++++++++- internal/storage/claim_r220_test.go | 102 ++++++++++++++++++++++++++++ internal/storage/hostops.go | 3 + 5 files changed, 251 insertions(+), 38 deletions(-) create mode 100644 internal/storage/claim_r220_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 887af55..a9fe852 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,36 @@ +## v0.127.0 — a mount Felhom itself made is not "something else" (2026-08-06, R-220) + +**After a rebuild the customer's own drives could not be re-attached, and the refusal named an action +they could not perform.** `GET /api/disks/candidates` returned `initialize: [], attach: []` while both +drives sat there, and the deploy refused with *"choose an attached drive from the list"* — a list that +was empty. Measured live **three times**: CAMPAIGN-11 Phase 1, and twice on the R-201 re-walk. + +**The mechanism.** Enrolment mounts a drive **twice** — at the managed `/mnt/felhom-drives/` and +at the raw `/mnt/` it creates on the host. **The host survives a guest rebuild; the controller's +registry does not.** So `classifyClaim` saw a mount outside the managed prefix and correctly concluded +"claimed by something else" — about Felhom's own mount. + +**The fix is CORROBORATED, not a widened prefix.** A mountpoint outside `/mnt/felhom-drives` is +forgiven **only when the same device is ALSO mounted under the managed path** — a pairing that only +Felhom's own enrolment produces. A disk another system is using, at `/srv/data` or `/media/x` or even +`/mnt/someone-elses-disk`, has no such counterpart and **is still refused**. That fence has its own +test, and its red-proof shows an over-wide fix offering `/mnt/someone-elses-disk` for formatting. + +**Read from `/proc/mounts`, deliberately.** The lsblk invocation is pinned **verbatim** in +`configs/felhom-agent.sudoers` (`lsblk -J -o NAME,FSTYPE,PTTYPE,MOUNTPOINT /dev/*`), so switching it to +the plural `MOUNTPOINTS` would have meant shipping a sudoers change with the binary — a far larger +blast radius than this finding warrants. `/proc/mounts` is world-readable: **no sudo, no new allowlisted +command, no config change.** + +**Fail-safe:** an unreadable mount table corroborates **nothing**, so the device classifies exactly as +it did before this change — refused. "We could not corroborate" must never read as "it is ours". + +Tests: `claim_r220_test.go` — the own-drive case, the foreign-mount fence over four paths, and the +corroboration itself (both mounts required; a lone raw mount vouches for nothing; another device's +managed mount does not vouch for this one; an unreadable table corroborates nothing). +**Red-proofs:** removing the exemption refuses the customer's own drive again +(*"device is mounted at /mnt/adatok (sdb)"*); over-widening it to any `/mnt/*` path breaks the fence. + ## docs — CLAUDE.md becomes a core plus path-scoped rules (2026-08-06, R-229 leg (b)) — no version bump **Documentation only. No Go changed, nothing built, nothing deployed.** `go build`/`vet`/`test` green diff --git a/REPORT.md b/REPORT.md index 5f1c266..03c1b28 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,53 +1,41 @@ -# REPORT — felhom-agent v0.126.0: a fetch failure is not a wrong recovery code (R-224) +# REPORT — felhom-agent v0.127.0: a mount Felhom made is not foreign (R-220) -**Scope: this repo's half of R-224.** The controller half ships as felhom-controller v0.202.0. - -## Why the agent changed at all - -The task that commissioned this work scoped `felhom-agent` as **untouched**. It could not be. Its -Scenario A (a hub outage must not blame the customer's code) and Scenario C (a genuine mistype must be -told to re-check the ten words) are **mutually unsatisfiable** while this agent answers both with one -HTTP 400 and one sentence. No value available to the controller separates them. The task's own §5 -anticipates this — *"if the step is not recoverable from the value, make it so, and say what that -cost"* — and §4.3 says the source outranks the register's recorded shape. **The cost is this version, -a publish, and a `MinAgent` coupling on the controller side.** +**Scope: the host half of R-220.** The customer-facing refusal message is the controller's half and +ships as felhom-controller v0.203.0. ## What changed | File | Change | |---|---| -| `internal/escrow/recover.go` | new `ErrBundleFetch` sentinel; the fetch leg joins it with `%w: %w` so the cause survives for the operator log | -| `internal/localapi/escrow_recover.go` | new `case errors.Is(err, escrow.ErrBundleFetch)` → **502** with its own words; the `default` now carries only the wrong-code case and drops the "or" | -| `internal/escrow/recover_test.go` | three new tests; the pre-existing `FetchErrorIsDistinct` re-pointed from a string to the sentinel, with the reason it failed to protect | -| `internal/localapi/escrow_recover_class_test.go` | new — the consequence-level test: four situations, four statuses | +| `internal/storage/claim.go` | `claimFacts.felhomOwnedMounts`; `classifyClaim` forgives a non-managed mountpoint **only when corroborated**; `felhomOwnedMounts()` + `procMounts()` | +| `internal/storage/hostops.go` | `mountTable` seam (nil ⇒ real `/proc/mounts`) | +| `internal/storage/claim_r220_test.go` | new — the own-drive case, the fence, and the corroboration's four edges | -**Four statuses:** `502` fetch failed (the code was **not used**) · `400` fetched and refused · -`404` no bundle · `409` bundle predates the field. +## The shape chosen, and why (§7.3) -## The finding this turned up +**Candidate (b): the claimed check distinguishes a mount Felhom made from a foreign one** — the task +called it "nearer the truth" and it is, because the host and its knowledge survive the rebuild while +the guest's registry does not. Candidate (a) — having the rebuild path clear the raw mounts — would +have made correctness depend on a cleanup step running, and a cleanup that does not run leaves exactly +today's defect. -**A green test named the defect and did not prevent it.** `TestRecoverOffsiteRepoPassword_FetchErrorIsDistinct` -has asserted since v0.125.0 that *"the operator must not be sent to re-read their recovery code because -the hub was unreachable"*. It passed throughout, because it checked this package's error **string** one -layer below the local-api `default` that did the merging — and a string is not something a caller can -branch on. **Mechanism asserted, consequence unpinned**; the project's own rule names this exact case. -It is also a comment-vs-code entry: `recover.go`'s header said the errors were *"DISTINCT on purpose"* -and named **three** situations while a fourth was silently folded into one of them. +**The discriminator is corroboration, not a path prefix**: the same device must ALSO be mounted under +`/mnt/felhom-drives`. Only enrolment produces that pairing. + +**`/proc/mounts` rather than `lsblk MOUNTPOINTS`**, because the lsblk invocation is pinned verbatim in +the sudoers file; changing it would have coupled this fix to a config rollout. `/proc/mounts` is +world-readable and needs neither. ## Green gate -`go build ./...` clean · `go vet ./...` clean · `go test ./...` → **29 packages ok** · -`python3 scripts/agent_gates.py --fast` → all gates OK. +`go build` · `go vet` clean · `go test ./...` → **29 packages ok** · `agent_gates.py --fast` → all OK. -**Red-proofs, each demonstrated failing then restored:** - -| Mutation | Result | +| Red-proof | Result | |---|---| -| remove the `%w: %w` join (pre-R-224 wrap) | `FetchFailureIsClassifiedAsFetch` **FAILS** | -| delete the `ErrBundleFetch` handler case | fetch answers `400 "the recovery code did not open the sealed bundle"` — **the exact defect**, and both status tests **FAIL** | +| remove the `felhomOwnedMounts` exemption | **FAILS** — "device is mounted at /mnt/adatok (sdb)", the pre-fix refusal | +| over-widen the exemption to any `/mnt/*` | **FAILS** — "/mnt/someone-elses-disk was offered for formatting" | ## Not changed -No Proxmox surface, no privileged path, no report/hub contract, no config schema. The route's -success path, its scoping and its R-handling discipline (`R = ""` on both paths, never logged, never -persisted) are untouched. +No sudoers, no allowlisted command, no PVE surface, no format path. Every other claim signal +(system disk, read-only, LVM PV, ZFS member, member FSTYPEs, empty-topology backstop) is untouched. diff --git a/internal/storage/claim.go b/internal/storage/claim.go index bf143ec..73550fa 100644 --- a/internal/storage/claim.go +++ b/internal/storage/claim.go @@ -53,7 +53,11 @@ type claimFacts struct { nodes []claimNode // the whole disk + its children (partitions) lvmPV bool // pvs (authoritative): the disk / a partition is an LVM physical volume zfsMember bool // zpool (authoritative): the disk / a partition is a ZFS pool member - gatherErr string // non-empty ⇒ a REQUIRED read failed ⇒ fail-safe CLAIMED + // felhomOwnedMounts (R-220) — mountpoints OUTSIDE /mnt/felhom-drives that are nevertheless Felhom's + // OWN, corroborated from the host mount table: the same device is also mounted at the managed path. + // Empty means "nothing corroborated", which is the fail-safe direction. + felhomOwnedMounts map[string]bool + gatherErr string // non-empty ⇒ a REQUIRED read failed ⇒ fail-safe CLAIMED } // classifyClaim is the pure guard verdict. unclaimed=true ONLY when the device is provably free for @@ -81,7 +85,20 @@ func classifyClaim(f claimFacts) (unclaimed bool, reason string) { if memberFSTypes[n.fstype] { return false, "device holds a " + n.fstype + " (" + n.name + ")" } - if n.mountpoint != "" && !underFelhomDrives(n.mountpoint) { + // ── R-220 — A MOUNT FELHOM ITSELF MADE IS NOT "SOMETHING ELSE". ─────────────────────── + // + // Enrolment mounts a drive TWICE: at the managed path `/mnt/felhom-drives/` and at the + // raw `/mnt/` it creates on the host. The host — and therefore that raw mount — survives + // a guest rebuild, while the controller's registry does not. So after a rebuild the customer's + // own drives looked foreign, `attach` returned an empty list, and the refusal told them to + // choose from it. Measured live three times (CAMPAIGN-11 Phase 1, and the R-201 re-walk twice); + // unmounting only the raw mounts flipped `attach: []` to both drives every time. + // + // The fence this must NOT breach: a disk genuinely in use by something else stays refused. So + // the exemption is not "any /mnt/* path" — it is CORROBORATED: the same device must ALSO be + // mounted at Felhom's managed path, which is a state only Felhom's own enrolment produces. + // A foreign disk at /srv/data or /media/x has no such counterpart and is still refused. + if n.mountpoint != "" && !underFelhomDrives(n.mountpoint) && !f.felhomOwnedMounts[n.mountpoint] { return false, "device is mounted at " + n.mountpoint + " (" + n.name + ")" } } @@ -154,6 +171,8 @@ func (h *SudoHostOps) gatherClaimFacts(ctx context.Context, device string) claim return f } f.nodes = nodes + // R-220: corroborate which non-managed mountpoints are nevertheless Felhom's own. + f.felhomOwnedMounts = felhomOwnedMounts(device, nodes, h.mountTable) // LVM PV (authoritative). pvs installed but erroring ⇒ fail-safe claimed; absent ⇒ rely on lsblk's // LVM2_member FSTYPE (already in nodes). @@ -285,3 +304,71 @@ func (h *SudoHostOps) zfsMembers(ctx context.Context, nodes []claimNode, wholeDi } return false, nil } + +// mountTableSource yields the host mount table as (device, mountpoint) pairs. A seam so the R-220 +// corroboration is unit-testable without a host. nil ⇒ the real /proc/mounts. +type mountTableSource func() ([][2]string, error) + +// procMounts reads /proc/mounts — WORLD-READABLE, so this needs no sudo and no allowlisted command. +// That matters: the lsblk invocation is pinned verbatim in the sudoers file +// (`lsblk -J -o NAME,FSTYPE,PTTYPE,MOUNTPOINT /dev/*`), so switching it to the plural MOUNTPOINTS +// would have meant shipping a sudoers change with the binary — a far larger blast radius than this +// finding warrants. Reading the mount table directly sidesteps that entirely. +func procMounts() ([][2]string, error) { + data, err := os.ReadFile("/proc/mounts") + if err != nil { + return nil, err + } + var out [][2]string + for _, line := range strings.Split(string(data), "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + // /proc/mounts escapes spaces as \040; unescape so a path with a space still compares. + out = append(out, [2]string{fields[0], strings.ReplaceAll(fields[1], `\040`, " ")}) + } + return out, nil +} + +// felhomOwnedMounts returns the mountpoints of `device` (and its children) that sit OUTSIDE +// /mnt/felhom-drives but are still Felhom's own, corroborated by the same device also being mounted +// UNDER /mnt/felhom-drives. That pairing is what enrolment produces and nothing else does. +// +// ⚠ FAIL-SAFE: an unreadable mount table returns an EMPTY set, never a permissive one. The device then +// classifies exactly as it did before R-220 — refused — because "we could not corroborate" must never +// read as "it is ours". +func felhomOwnedMounts(device string, nodes []claimNode, src mountTableSource) map[string]bool { + if src == nil { + src = procMounts + } + table, err := src() + if err != nil { + return nil // unreadable ⇒ corroborate nothing + } + // Every device name this disk answers to: the whole disk and each child node. + devs := map[string]bool{device: true} + if wd, ok := wholeDiskOf(device); ok { + devs[wd] = true + } + for _, n := range nodes { + devs["/dev/"+n.name] = true + } + // A device is Felhom-managed only if it is mounted under the managed prefix. + managed := map[string]bool{} + for _, row := range table { + if devs[row[0]] && underFelhomDrives(row[1]) { + managed[row[0]] = true + } + } + if len(managed) == 0 { + return nil + } + owned := map[string]bool{} + for _, row := range table { + if managed[row[0]] && !underFelhomDrives(row[1]) { + owned[path.Clean(row[1])] = true + } + } + return owned +} diff --git a/internal/storage/claim_r220_test.go b/internal/storage/claim_r220_test.go new file mode 100644 index 0000000..1b9b6ac --- /dev/null +++ b/internal/storage/claim_r220_test.go @@ -0,0 +1,102 @@ +package storage + +import "testing" + +// ── R-220 — A MOUNT FELHOM ITSELF MADE IS NOT "SOMETHING ELSE" ────────────────────────────────── +// +// Enrolment mounts a drive twice: at `/mnt/felhom-drives/` and at the raw `/mnt/` it +// creates on the host. The host survives a guest rebuild; the controller's registry does not. So after +// a rebuild the customer's own drives read as claimed-by-something-else, `attach` came back empty, and +// the refusal told them to pick from the empty list. Measured three times live. +// +// The fence: a disk genuinely in use elsewhere must STILL be refused. These assert both directions. + +// ── SCENARIO E — the customer's own drive is offered again after a rebuild ─────────────────────── +// +// RED-PROOF: drop `&& !f.felhomOwnedMounts[n.mountpoint]` from classifyClaim — the pre-R-220 check — +// and this FAILS with the drive refused and the list empty again. +func TestClassifyClaim_R220_FelhomsOwnRawMountIsNotForeign(t *testing.T) { + f := claimFacts{ + device: "/dev/sdb", wholeDisk: "/dev/sdb", wholeDiskOK: true, + nodes: []claimNode{{name: "sdb", fstype: "ext4", mountpoint: "/mnt/adatok"}}, + // corroborated: the SAME device is also mounted at the managed path + felhomOwnedMounts: map[string]bool{"/mnt/adatok": true}, + } + unclaimed, reason := classifyClaim(f) + if !unclaimed { + t.Fatalf("R-220 RETURNED: the customer's own drive is refused after a rebuild — %q", reason) + } +} + +// ── SCENARIO F — a genuinely foreign mount is STILL refused ────────────────────────────────────── +// +// RED-PROOF: over-widen the fix to exempt any /mnt/* path (or to skip the mountpoint check entirely) +// and this FAILS — a disk another system is using would be offered for formatting. +func TestClassifyClaim_R220_ForeignMountIsStillRefused(t *testing.T) { + for _, mp := range []string{"/srv/data", "/media/photos", "/mnt/someone-elses-disk", "/var/lib/other"} { + f := claimFacts{ + device: "/dev/sdb", wholeDisk: "/dev/sdb", wholeDiskOK: true, + nodes: []claimNode{{name: "sdb", fstype: "ext4", mountpoint: mp}}, + felhomOwnedMounts: nil, // nothing corroborated it as ours + } + unclaimed, reason := classifyClaim(f) + if unclaimed { + t.Fatalf("THE FENCE BROKE: a disk mounted at %s was offered for formatting", mp) + } + if reason == "" { + t.Fatalf("a refusal must carry a reason (%s)", mp) + } + } +} + +// The corroboration itself: it must require BOTH mounts of the SAME device, and fail safe. +func TestFelhomOwnedMounts_RequiresTheManagedCounterpart(t *testing.T) { + nodes := []claimNode{{name: "sdb"}} + + t.Run("both mounts present -> the raw one is ours", func(t *testing.T) { + src := func() ([][2]string, error) { + return [][2]string{ + {"/dev/sdb", "/mnt/adatok"}, + {"/dev/sdb", "/mnt/felhom-drives/adatok"}, + }, nil + } + got := felhomOwnedMounts("/dev/sdb", nodes, src) + if !got["/mnt/adatok"] { + t.Fatal("the raw enrolment mount was not recognised as Felhom's own") + } + }) + + t.Run("only the raw mount -> corroborates NOTHING", func(t *testing.T) { + src := func() ([][2]string, error) { + return [][2]string{{"/dev/sdb", "/mnt/adatok"}}, nil + } + if got := felhomOwnedMounts("/dev/sdb", nodes, src); len(got) != 0 { + t.Fatalf("a lone /mnt/ mount must corroborate nothing, got %v", got) + } + }) + + t.Run("a DIFFERENT device under the managed path does not vouch for this one", func(t *testing.T) { + src := func() ([][2]string, error) { + return [][2]string{ + {"/dev/sdb", "/srv/data"}, + {"/dev/sdc", "/mnt/felhom-drives/mentes"}, // someone else's, not sdb's + }, nil + } + if got := felhomOwnedMounts("/dev/sdb", nodes, src); got["/srv/data"] { + t.Fatal("another device's managed mount vouched for a foreign one") + } + }) + + t.Run("an unreadable mount table corroborates NOTHING (fail-safe)", func(t *testing.T) { + src := func() ([][2]string, error) { return nil, errRead } + if got := felhomOwnedMounts("/dev/sdb", nodes, src); len(got) != 0 { + t.Fatalf("an unreadable mount table must corroborate nothing, got %v", got) + } + }) +} + +var errRead = errNoMountTable{} + +type errNoMountTable struct{} + +func (errNoMountTable) Error() string { return "mount table unreadable" } diff --git a/internal/storage/hostops.go b/internal/storage/hostops.go index 8c4de77..1f7a0c1 100644 --- a/internal/storage/hostops.go +++ b/internal/storage/hostops.go @@ -164,6 +164,9 @@ type SudoHostOps struct { // UNPRIVILEGED read (`systemctl is-failed`) — seam-injected so the reassert's F10 reset-failed path // is unit-testable without a real systemd. Default set in NewSudoHostOps. unitFailed func(ctx context.Context, unit string) bool + // mountTable (R-220) yields the host mount table for the "is this mount Felhom's own?" + // corroboration. nil ⇒ the real /proc/mounts; tests inject. + mountTable mountTableSource } // SudoHostOpsConfig configures a SudoHostOps.