From 7545af8a2cf4478337e780f1a1afead69072481b Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 6 Jul 2026 20:07:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(localapi):=20F2=20mount-role=20fallback=20?= =?UTF-8?q?=E2=80=94=20enrolled=20user-data=20drives=20ejectable=20again?= =?UTF-8?q?=20(v0.73.0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit roleForMountPath resolved role only from the PVE storage view; a bind-mounted RAW enrolled user-data drive is not a PVE storage, so it fail-safe'd to system and the eject/decommission gates 403'd EVERY user-data drive in the standard topology (campaign F2, where=/mnt/teszt_enroll role=system). Add a mount-table fallback mirroring durableIDForMount Impl-2b: device-keyed classification with a whole-disk containment pass (new storage.SameWholeDisk) and the Observe-error early return kept BEFORE the fallback (else a blind view -> permissive). Only roleForMountPath touched. Tests A1/B1/B2/C1-C3 + 3 red-proofs; existing RoleGated tests green unmodified. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 23 +++ internal/localapi/disks.go | 44 ++++- internal/localapi/f2_role_fallback_test.go | 192 +++++++++++++++++++++ internal/storage/role.go | 11 ++ 4 files changed, 266 insertions(+), 4 deletions(-) create mode 100644 internal/localapi/f2_role_fallback_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ea77fdf..5e75ce0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,26 @@ +## v0.73.0 — F2 mount-role fallback: enrolled user-data drives are ejectable/decommissionable again (2026-07-06) + +Closes campaign finding **F2** (`felhom.eu/documentation/audits/CAMPAIGN-nomercy-2026-07-06.md` + +RERUN addendum). `roleForMountPath` (`internal/localapi/disks.go`) resolved a mount's protection role +ONLY from the PVE storage view (`Observe`), but a **bind-mounted RAW enrolled user-data drive is not +a PVE storage** → no MountPath match → fail-safe `RoleSystem` → the eject/decommission role gates +403'd **every user-data drive in the standard topology** (journal: `where=/mnt/teszt_enroll +role=system`). Customers could not eject or decommission their own drives. + +- **Fallback** mirrors `durableIDForMount`'s Impl-2b: after the MountPath loop misses (on a SUCCESSFUL + Observe), resolve the mount's backing device from the host mount table and classify **device-keyed** + — non-`/dev` source (NAS) → system; device on the same whole disk as a KNOWN target → THAT target's + role (containment, via new `storage.SameWholeDisk`, whole-disk granularity so a protected disk can't + be ejected through this path); else `RoleForRawDevice`. **Fail-safe preserved**: an Observe error + returns system BEFORE the fallback (a blind containment pass could label a backup drive user-data — + permissive), and a mount-table-read failure or an absent/NAS mount → system. +- Scope: **only** `roleForMountPath`; gates, `deviceRole`, `DecommissionExecutor`, `classify.go`, + `ReassertGuestBinds` untouched; the `deviceRole`/`roleForMountPath` unification is deferred. +- Tests (`f2_role_fallback_test.go`): A1 ejectable + decommission effects; B1 containment→403; B2 + system-disk→403; C1/C2 fail-safe; C3 Observe-error skips the fallback. Three red-proofs demonstrated + (pre-fix→A1 FAIL, containment-skip→B1 FAIL, fallback-on-error→C3 FAIL). Existing RoleGated/ + Decommission tests green unmodified. + ## v0.72.0 — OOB operator access: rendered operator /32 + dedicated felhom-sshd + port-adaptive belt + oob health (TASK H1) (2026-07-05) The agent half of the merged E1+H1 operator-SSH-access feature (hub half = felhom-hub v0.35.0). diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index 554e62e..fc0e785 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -1015,20 +1015,56 @@ func (s *Server) hostReader() storage.HostReader { // roleForMountPath resolves the AUTHORITATIVE protection role of the storage mounted at `where`, from // the agent's OWN storage view + host topology (never the caller's claim). It mirrors deviceRole but // keys on the mount path (the eject input). It FAILS SAFE to system (most-protected) on any -// ambiguity — a view error, or no storage target found at `where` — so an unresolvable eject is -// refused rather than silently unmounted. +// ambiguity — a view error, an unrecognizable mount source, or `where` absent from both the storage +// view and the mount table — so an unresolvable eject is refused rather than silently unmounted. func (s *Server) roleForMountPath(ctx context.Context, where string) storage.DeviceRole { sysDisks, sysKnown := storage.SystemDisks(s.hostReader()) targets, err := s.storage.Observe(ctx) if err != nil { - return storage.RoleSystem // can't read the view → treat as protected + // Can't read the view → protected. The mount-table fallback below must NOT run here: with + // the PVE view down its containment pass is blind, and falling through to raw classification + // could label a backup-backing device user-data — a PERMISSIVE regression. + return storage.RoleSystem } for _, t := range targets { if t.MountPath == where { return storage.RoleForStorage(t.Type, t.BackingDevice, sysDisks, sysKnown) } } - return storage.RoleSystem // no storage target at this mount → fail safe to protected + // Fallback (campaign F2, 2026-07-06 — mirrors durableIDForMount's Impl-2b): a RAW enrolled + // user-data drive is NOT a PVE storage, so it never appears in Observe — which made this + // function fail-safe every such mount to `system` and the eject/decommission gates 403 EVERY + // user-data drive in the standard topology (journal: where=/mnt/teszt_enroll role=system). + // Resolve the mount's backing device from the host mount table, then classify device-keyed: + // - non-/dev source (NAS "server:/export", tmpfs, …) → system (never drive-ejectable); + // - device on the same whole disk as a KNOWN storage target → THAT target's role (containment: + // felhom-usb/felhom-flash are dir storages with backup content — their disk must never + // become ejectable through this path); + // - otherwise RoleForRawDevice (system-disk membership; fail-safe to system). + mounts, merr := s.hostReader().Mounts() + if merr != nil { + return storage.RoleSystem // can't read the mount table → protected + } + for _, m := range mounts { + if m.MountPoint != where { + continue + } + if !strings.HasPrefix(m.Device, "/dev/") { + return storage.RoleSystem // network/virtual source — has its own lifecycle, protected here + } + // Containment pass over the ALREADY-FETCHED targets (never re-Observe: a racing error there + // would degrade to the permissive raw path). Compare at whole-disk granularity: targets carry + // the PARTITION as BackingDevice (a dir storage on /dev/sdb1) while a raw enrolled drive mounts + // the WHOLE disk (/dev/sdb) — SameWholeDisk normalizes both so a backup disk stays protected. + for _, t := range targets { + if t.BackingDevice != "" && storage.SameWholeDisk(t.BackingDevice, m.Device) { + return storage.RoleForStorage(t.Type, t.BackingDevice, sysDisks, sysKnown) + } + } + // No known target claims this device → classify the raw device by system-disk membership. + return storage.RoleForRawDevice(m.Device, sysDisks, sysKnown) + } + return storage.RoleSystem // no storage target and no mount-table entry → fail safe to protected } // deviceRole resolves a device's AUTHORITATIVE protection tier. It prefers a known storage target's diff --git a/internal/localapi/f2_role_fallback_test.go b/internal/localapi/f2_role_fallback_test.go new file mode 100644 index 0000000..0c85685 --- /dev/null +++ b/internal/localapi/f2_role_fallback_test.go @@ -0,0 +1,192 @@ +package localapi + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// f2HostReader is a configurable HostReader fixture for the roleForMountPath F2 fallback: it serves a +// mount table (device→mountpoint), resolves fs-UUIDs for the decommission bind-prune, and COUNTS +// Mounts() calls so a test can prove the fallback did/didn't run (SystemDisks calls Mounts() once per +// request; the fallback is a SECOND call — so Observe-error → exactly 1 call proves the fallback was +// skipped). +type f2HostReader struct { + mounts []storage.Mount + uuids map[string]string + mu sync.Mutex + calls int +} + +func (f *f2HostReader) Mounts() ([]storage.Mount, error) { + f.mu.Lock() + f.calls++ + f.mu.Unlock() + return f.mounts, nil +} +func (f *f2HostReader) ResolveUUID(dev string) (string, bool) { u, ok := f.uuids[dev]; return u, ok } +func (f *f2HostReader) DeviceExists(string) bool { return true } +func (f *f2HostReader) Rotational(string) (bool, bool) { return false, false } +func (f *f2HostReader) Removable(string) (bool, bool) { return false, false } +func (f *f2HostReader) mountsCalls() int { f.mu.Lock(); defer f.mu.Unlock(); return f.calls } + +// errStorage is a StorageView whose Observe always fails — for the Observe-error fail-safe (edge C3). +type errStorage struct{} + +func (errStorage) Observe(context.Context) ([]hub.StorageTarget, error) { + return nil, errors.New("proxmox unreachable") +} + +// f2EjectServer builds an eject/decommission-capable server with a custom storage view + host reader. +func f2EjectServer(t *testing.T, d *fakeDiskOps, sv StorageView, host storage.HostReader) *Server { + t.Helper() + srv := newDiskServerRaw(t, d, &fakeGate{}, sv, nil) + if host != nil { + srv.host = host + } + return srv +} + +// A1 — the campaign shape: a bind-mounted enrolled user-data drive (/mnt/teszt_enroll on /dev/sdb) is +// NOT a PVE storage, so it misses the MountPath loop. Pre-fix → fail-safe system → 403 EVERY user-data +// drive. Post-fix → the mount-table fallback classifies /dev/sdb (not the system disk /dev/sda) as +// user-data → eject 200 + decommission fires its effects. +// +// COMPANION RED-PROOF: on the pre-fix roleForMountPath (the plain "no target → RoleSystem" body) this +// eject returns 403 → the 200 assertion FAILS. +func TestRoleForMountPath_F2_BindMountedUserData_Ejectable(t *testing.T) { + // storage view: a PBS + a system dir, but NOTHING at /mnt/teszt_enroll (the raw enrolled drive). + sv := fakeStorage{targets: []hub.StorageTarget{ + {Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}, + {Name: "local", Type: "local", MountPath: "/var/lib/vz"}, + }} + host := &f2HostReader{ + mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, // system disk + {Device: "/dev/sdb", MountPoint: "/mnt/teszt_enroll"}, // the raw enrolled user-data drive + }, + uuids: map[string]string{"/dev/sdb": "f2236136-ced7"}, + } + + // eject → 200, and the raw is NOT unmounted (intermediary model). + d := &fakeDiskOps{} + h := f2EjectServer(t, d, sv, host).Handler() + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/teszt_enroll"}`); w.Code != http.StatusOK { + t.Fatalf("eject bind-mounted user-data drive: got %d want 200 (%s)", w.Code, w.Body.String()) + } + d.mu.Lock() + if len(d.unmountCalls) != 0 { + t.Fatalf("user-data eject must not unmount the raw drive: %v", d.unmountCalls) + } + d.mu.Unlock() + + // decommission → 200 + all three effects (intent, bind prune, DetachDrive), no unmount/format. + intent := newFakeIntent() + intent.SetEnrolled("uuid:f2236136-ced7") + gb := tempBindStore(t) + _ = gb.Record(8200, "uuid:f2236136-ced7") + ga := &fakeGuestAttacher{} + d2 := &fakeDiskOps{} + srv := decommServer(t, d2, sv, fakeGuestList{}, intent, gb, ga, nil) + srv.host = host // inject the teszt_enroll mount table + if w := do(t, srv.Handler(), "POST", "/disks/decommission", "A", `{"where":"/mnt/teszt_enroll"}`); w.Code != http.StatusOK { + t.Fatalf("decommission bind-mounted user-data drive: got %d want 200 (%s)", w.Code, w.Body.String()) + } + if got := intent.Get("uuid:f2236136-ced7"); got != storage.IntentDecommissioned { + t.Errorf("intent = %q, want decommissioned", got) + } + if ids := gb.Guests()[8200]; len(ids) != 0 { + t.Errorf("guest-bind not pruned: %v", ids) + } + if len(ga.detachDrives) != 1 || ga.detachDrives[0] != "/mnt/teszt_enroll" { + t.Errorf("DetachDrive calls = %v, want [/mnt/teszt_enroll]", ga.detachDrives) + } + d2.mu.Lock() + defer d2.mu.Unlock() + if len(d2.unmountCalls) != 0 || len(d2.formatCalls) != 0 { + t.Errorf("decommission must not unmount/format: unmount=%v format=%v", d2.unmountCalls, d2.formatCalls) + } +} + +// B1 (containment) — a mount whose device shares a whole disk with a KNOWN protected target must honor +// THAT target's role, not raw-classify. Here /dev/sdb1 backs a lvmthin (→ system) target; a mount on +// /dev/sdb1 that misses the MountPath loop must resolve to system (403), NOT user-data. +// +// COMPANION RED-PROOF: the containment-skipping mutation (fallback = bare RoleForRawDevice(m.Device)) +// returns user-data for /dev/sdb1 (not the system disk) → this eject returns 200 → the 403 FAILS. +func TestRoleForMountPath_F2_Containment(t *testing.T) { + sv := fakeStorage{targets: []hub.StorageTarget{ + // lvmthin on an external disk /dev/sdb → RoleForStorage(lvmthin) = system (default tier). + {Name: "extra-lvm", Type: "lvmthin", BackingDevice: "/dev/sdb1", MountPath: "/never/matched"}, + }} + host := &f2HostReader{mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, + {Device: "/dev/sdb1", MountPoint: "/mnt/onprotecteddisk"}, + }} + d := &fakeDiskOps{} + h := f2EjectServer(t, d, sv, host).Handler() + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/onprotecteddisk"}`); w.Code != http.StatusForbidden { + t.Fatalf("containment: mount on a protected target's disk must be 403, got %d (%s)", w.Code, w.Body.String()) + } + d.mu.Lock() + if len(d.unmountCalls) != 0 { + t.Fatalf("containment: no unmount on the refused mount: %v", d.unmountCalls) + } + d.mu.Unlock() +} + +// B2 — a mount on the SYSTEM disk (/dev/sda) is refused: RoleForRawDevice sees it is system-backed. +func TestRoleForMountPath_F2_SystemDiskMount(t *testing.T) { + sv := fakeStorage{targets: []hub.StorageTarget{{Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}}} + host := &f2HostReader{mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, + {Device: "/dev/sda3", MountPoint: "/mnt/onsystemdisk"}, + }} + h := f2EjectServer(t, &fakeDiskOps{}, sv, host).Handler() + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/onsystemdisk"}`); w.Code != http.StatusForbidden { + t.Fatalf("system-disk mount must be 403, got %d (%s)", w.Code, w.Body.String()) + } +} + +// C1/C2 — fail-safe: a mount absent from the table, and a non-/dev (NAS) source, both refuse. +func TestRoleForMountPath_F2_FailSafe(t *testing.T) { + sv := fakeStorage{targets: []hub.StorageTarget{{Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}}} + host := &f2HostReader{mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, + {Device: "nas.local:/export", MountPoint: "/mnt/nas"}, // NAS: own lifecycle, never drive-ejectable + }} + // C1: mount not in the table at all. + h := f2EjectServer(t, &fakeDiskOps{}, sv, host).Handler() + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/absent"}`); w.Code != http.StatusForbidden { + t.Fatalf("absent mount must be 403, got %d", w.Code) + } + // C2: NAS-backed mount → system (non-/dev source). + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/nas"}`); w.Code != http.StatusForbidden { + t.Fatalf("NAS mount must be 403 (has its own lifecycle), got %d (%s)", w.Code, w.Body.String()) + } +} + +// C3 — the CRITICAL fail-safe: when Observe() fails the fallback must NOT run (a blind containment +// pass could label a backup drive user-data — permissive). Assert 403 AND that Mounts() was called +// exactly once (by SystemDisks; the fallback — a second call — was skipped). +// +// COMPANION RED-PROOF: moving the fallback ahead of the Observe-error return (running it regardless) +// classifies /dev/sdb as user-data → this eject returns 200 → the 403 FAILS. +func TestRoleForMountPath_F2_ObserveError_NoFallback(t *testing.T) { + host := &f2HostReader{mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, + {Device: "/dev/sdb", MountPoint: "/mnt/teszt_enroll"}, // would classify user-data IF the fallback ran + }} + h := f2EjectServer(t, &fakeDiskOps{}, errStorage{}, host).Handler() + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/mnt/teszt_enroll"}`); w.Code != http.StatusForbidden { + t.Fatalf("Observe error must fail safe to 403, got %d (%s)", w.Code, w.Body.String()) + } + if c := host.mountsCalls(); c != 1 { + t.Fatalf("on Observe error the fallback must be skipped: Mounts() called %d times, want 1 (SystemDisks only)", c) + } +} diff --git a/internal/storage/role.go b/internal/storage/role.go index 8bef5af..6ab2044 100644 --- a/internal/storage/role.go +++ b/internal/storage/role.go @@ -136,3 +136,14 @@ func RoleForRawDevice(device string, sysDisks map[string]bool, sysKnown bool) De } return RoleUserData } + +// SameWholeDisk reports whether two device paths live on the same physical whole disk +// (e.g. /dev/sdb and /dev/sdb1). Storage targets carry BOTH granularities in practice (a dir +// storage's BackingDevice is the mounted PARTITION, a raw enrolled drive mounts the WHOLE disk), +// so containment checks must compare at whole-disk level — mirroring isSystemBacked. False when +// either side's whole-disk is unrecognizable (device-mapper/network) — callers fail safe. +func SameWholeDisk(a, b string) bool { + wa, oka := wholeDiskOf(a) + wb, okb := wholeDiskOf(b) + return oka && okb && wa == wb +}