From 7ae82e1d5dd74d68224f42fdb6c00f1f65af409f Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 12 Jun 2026 09:29:09 +0200 Subject: [PATCH] agent v0.24.0: role-gate the eject path (system/backup mounts unmount-protected at the agent) handleDiskEject now resolves the authoritative role of the storage at `where` and refuses 403 (no Unmount) unless it is user-data. Fails safe to protected on ambiguity. Adds roleForMountPath + an injectable HostReader seam for testability. TestEject_RoleGated asserts protected mounts are refused with no Unmount. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 21 +++++++++ cmd/felhom-agent/main.go | 2 +- internal/localapi/disks.go | 43 ++++++++++++++++- internal/localapi/disks_test.go | 83 +++++++++++++++++++++++++++++++-- internal/localapi/server.go | 13 ++++-- 5 files changed, 153 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b78ac2..c6743ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,27 @@ All notable changes to **felhom-agent** are recorded here. Update on every code change that gets pushed. +## v0.24.0 — role-gate the eject path (system/backup mounts are unmount-protected at the agent) (2026-06-12) + +Closes the eject gap in the storage-authorization redesign: `POST /disks/eject` now **refuses to +unmount a system or backup storage**, enforced at the agent — not just hidden in the controller UI. +A direct API call (or a compromised controller) trying to `eject {where:"/var/lib/vz"}` or the PBS +mount is refused 403; only `user-data` mounts are ejectable. + +- **`handleDiskEject` (`internal/localapi/disks.go`)** — before `Unmount`, resolves the AUTHORITATIVE + protection role of the storage mounted at `where` (the agent's own storage-view + host-topology + classification, never the caller's claim) via the new `roleForMountPath`. Refuses (403, no + `Unmount`) unless the role is `user-data`. **Fails SAFE**: an unresolvable mount (view error or no + storage target at that path) → treated as protected → refused (the same most-protected-on-ambiguity + default the wipe gate uses). Mirrors the wipe path's "protected — eject refused by role" logging. +- **`roleForMountPath` + `hostReader` seam** — `roleForMountPath` keys `RoleForStorage` on the mount + path (the eject input), mirroring `deviceRole`. `Options.HostReader` (optional; defaults to the + production `*storage.ProcHostReader`) injects the root-free topology reader so the role-gate is unit- + testable. `handleDisks`/`deviceRole` now share the same seam. +- **Tests** — `TestEject_RoleGated` asserts a `system` and a `backup` mount are refused with **no + `Unmount`**, a `user-data` mount ejects, and an unresolvable mount fails safe to refused (the same + non-hollowness the wipe tests use). `TestEject_UnmountAndDependents` updated to a user-data target. + ## v0.23.0 — device-ROLE classification + tiered storage-wipe gate (system/backup operator-only, user-data customer-confirmable) (2026-06-11) The storage-authorization redesign (agent half). The gate's destructive-wipe path is now **tiered by diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 735a432..44aef9e 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -43,7 +43,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.23.0" +var version = "0.24.0" func main() { var ( diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index 03fc219..5036a69 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -100,7 +100,7 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { } // Resolve the OS/system disks ONCE for this request — role classification is agent-authoritative // (the agent's own mount/topology read, never the caller's claim). - sysDisks, sysKnown := storage.SystemDisks(storage.NewProcHostReader()) + sysDisks, sysKnown := storage.SystemDisks(s.hostReader()) out := make([]DiskInfo, 0, len(targets)) for _, t := range targets { di := DiskInfo{ @@ -183,6 +183,17 @@ func (s *Server) handleDiskEject(w http.ResponseWriter, r *http.Request, vmid in writeErr(w, http.StatusBadRequest, "where (mountpoint) is required") return } + // ROLE GATE (defense in depth): eject is permitted ONLY for a user-data mount. The agent + // classifies the role of the storage at `where` from its OWN view — never the caller's claim — + // and refuses system/backup. The UI hiding the button is NOT the control: a direct API call (or a + // compromised controller) trying to unmount /var/lib/vz or the PBS mount is refused here. Fails + // SAFE: an unresolvable mount → protected → refused (most-protected-on-ambiguity, like the wipe). + if role := s.roleForMountPath(r.Context(), req.Where); role != storage.RoleUserData { + s.logger.Warn("local-api: protected — eject refused by role", + "vmid", vmid, "where", req.Where, "role", role) + writeErr(w, http.StatusForbidden, "mount is system/backup-protected — eject refused (role: "+string(role)+")") + return + } dependents := s.dependentGuests(r.Context(), req.Where) if err := s.disks.Unmount(r.Context(), req.Where); err != nil { s.logger.Error("local-api: disk eject", "vmid", vmid, "where", req.Where, "err", err) @@ -338,11 +349,39 @@ func (s *Server) handleDiskFormat(w http.ResponseWriter, r *http.Request, vmid i "device is system/backup-protected — format requires an operator signature ("+dec.Reason+")") } +// hostReader returns the injected root-free host topology reader, or the production default. The seam +// keeps the role classification (SystemDisks) testable without touching the real /proc /dev /sys. +func (s *Server) hostReader() storage.HostReader { + if s.host != nil { + return s.host + } + return storage.NewProcHostReader() +} + +// 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. +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 + } + 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 +} + // deviceRole resolves a device's AUTHORITATIVE protection tier. It prefers a known storage target's // role (so a PBS-backed device is recognized as backup), falling back to a raw-device classification // (for a fresh disk not yet a PVE storage — the init flow). Defaults to system on ambiguity. func (s *Server) deviceRole(ctx context.Context, device string) storage.DeviceRole { - sysDisks, sysKnown := storage.SystemDisks(storage.NewProcHostReader()) + sysDisks, sysKnown := storage.SystemDisks(s.hostReader()) if targets, err := s.storage.Observe(ctx); err == nil { for _, t := range targets { if t.BackingDevice != "" && t.BackingDevice == device { diff --git a/internal/localapi/disks_test.go b/internal/localapi/disks_test.go index 0ea21bf..c90714a 100644 --- a/internal/localapi/disks_test.go +++ b/internal/localapi/disks_test.go @@ -73,6 +73,22 @@ type fakeGuestList struct{ guests []proxmox.Guest } func (f fakeGuestList) ListLXC(context.Context) ([]proxmox.Guest, error) { return f.guests, nil } +// fakeHostReader is a deterministic HostReader fixture for the role classifier: "/" lives on the +// whole disk /dev/sda, so SystemDisks resolves {/dev/sda} (sysKnown=true) and any storage backed by +// a different whole-disk (e.g. /dev/sdb1) classifies as user-data without touching the real host. +type fakeHostReader struct{ mounts []storage.Mount } + +func (f fakeHostReader) Mounts() ([]storage.Mount, error) { return f.mounts, nil } +func (f fakeHostReader) ResolveUUID(string) (string, bool) { return "", false } +func (f fakeHostReader) DeviceExists(string) bool { return true } +func (f fakeHostReader) Rotational(string) (bool, bool) { return false, false } +func (f fakeHostReader) Removable(string) (bool, bool) { return false, false } + +// sysOnSDA is the default system-disk fixture (root on /dev/sda) used by the disk-server test helpers. +func sysOnSDA() fakeHostReader { + return fakeHostReader{mounts: []storage.Mount{{Device: "/dev/sda1", MountPoint: "/"}}} +} + // 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() @@ -89,6 +105,7 @@ func newDiskServer(t *testing.T, d *fakeDiskOps, g *fakeGate, sv StorageView, gl Disks: d, DiskGate: g, Guests2: gl, + HostReader: sysOnSDA(), Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), }) if err != nil { @@ -256,8 +273,9 @@ func TestAssign_EnsureMount(t *testing.T) { func TestEject_UnmountAndDependents(t *testing.T) { d := &fakeDiskOps{} - // storage view: target "bulk" is mounted at /mnt/bulk - sv := fakeStorage{targets: []hub.StorageTarget{{Name: "bulk", MountPath: "/mnt/bulk"}}} + // storage view: USER-DATA target "bulk" (usb on a non-system disk) mounted at /mnt/bulk — the + // role-gate permits ejecting it. + sv := fakeStorage{targets: []hub.StorageTarget{{Name: "bulk", Type: hub.StorageTypeUSB, BackingDevice: "/dev/sdb1", MountPath: "/mnt/bulk"}}} // guest 8200 mounts storage "bulk"; guest 9300 does not gl := fakeGuestList{guests: []proxmox.Guest{{VMID: 8200}, {VMID: 9300}}} h := newDiskServerWithGuestConfigs(t, d, sv, gl, map[int]map[string]string{ @@ -295,6 +313,65 @@ func TestEject_UnmountAndDependents(t *testing.T) { } } +// A2 (security): the eject path is ROLE-GATED at the agent. A system mount (local dir on the OS disk) +// and a backup mount (PBS) are REFUSED 403 with NO Unmount; only a user-data mount ejects. The UI +// hiding the button is not the control — a direct API call to unmount a protected storage is refused. +func TestEject_RoleGated(t *testing.T) { + sv := fakeStorage{targets: []hub.StorageTarget{ + {Name: "bulk", Type: hub.StorageTypeUSB, BackingDevice: "/dev/sdb1", MountPath: "/mnt/bulk"}, // user-data + {Name: "local", Type: "local", MountPath: "/var/lib/vz"}, // system (builtin dir) + {Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}, // backup (PBS) + }} + + // system mount → refused, no Unmount. + d := &fakeDiskOps{} + h := newDiskServer(t, d, &fakeGate{}, sv, nil) + if w := do(t, h, "POST", "/disks/eject", "A", `{"where":"/var/lib/vz"}`); w.Code != http.StatusForbidden { + t.Fatalf("eject system mount: got %d want 403 (%s)", w.Code, w.Body.String()) + } + d.mu.Lock() + if len(d.unmountCalls) != 0 { + t.Fatalf("Unmount called on a system mount — role-gate bypassed: %v", d.unmountCalls) + } + d.mu.Unlock() + + // backup mount (PBS) → refused, no Unmount. + d2 := &fakeDiskOps{} + h2 := newDiskServer(t, d2, &fakeGate{}, sv, nil) + if w := do(t, h2, "POST", "/disks/eject", "A", `{"where":"/mnt/pbs"}`); w.Code != http.StatusForbidden { + t.Fatalf("eject backup mount: got %d want 403 (%s)", w.Code, w.Body.String()) + } + d2.mu.Lock() + if len(d2.unmountCalls) != 0 { + t.Fatalf("Unmount called on a backup mount — role-gate bypassed: %v", d2.unmountCalls) + } + d2.mu.Unlock() + + // user-data mount → ejects (Unmount called once). + d3 := &fakeDiskOps{} + h3 := newDiskServer(t, d3, &fakeGate{}, sv, nil) + if w := do(t, h3, "POST", "/disks/eject", "A", `{"where":"/mnt/bulk"}`); w.Code != http.StatusOK { + t.Fatalf("eject user-data mount: got %d want 200 (%s)", w.Code, w.Body.String()) + } + d3.mu.Lock() + if len(d3.unmountCalls) != 1 || d3.unmountCalls[0] != "/mnt/bulk" { + t.Fatalf("user-data eject did not Unmount /mnt/bulk: %v", d3.unmountCalls) + } + d3.mu.Unlock() + + // fail-safe: an unknown mount (no storage target) → refused, no Unmount. + d4 := &fakeDiskOps{} + h4 := newDiskServer(t, d4, &fakeGate{}, sv, nil) + if w := do(t, h4, "POST", "/disks/eject", "A", `{"where":"/mnt/unknown"}`); w.Code != http.StatusForbidden { + t.Fatalf("eject unresolvable mount: got %d want 403 (fail-safe) (%s)", w.Code, w.Body.String()) + } + d4.mu.Lock() + if len(d4.unmountCalls) != 0 { + t.Fatalf("Unmount called on an unresolvable mount — fail-safe violated: %v", d4.unmountCalls) + } + d4.mu.Unlock() +} + // ---- auth / config ---------------------------------------------------------------------- func TestDisks_CrossGuest403(t *testing.T) { @@ -334,7 +411,7 @@ func newDiskServerWithGuestConfigs(t *testing.T, d *fakeDiskOps, sv StorageView, srv, err := NewServer(Options{ ListenAddr: "127.0.0.1:0", Guests: fg, Backups: &fakeBackups{}, Store: &fakeStore{}, Storage: sv, Tokens: staticTokens{"A": 8200, "B": 9300}, - Disks: d, DiskGate: &fakeGate{}, Guests2: gl, + Disks: d, DiskGate: &fakeGate{}, Guests2: gl, HostReader: sysOnSDA(), Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), }) if err != nil { diff --git a/internal/localapi/server.go b/internal/localapi/server.go index f29a5e7..3bbe32c 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -16,6 +16,7 @@ import ( "gitea.dooplex.hu/admin/felhom-agent/internal/hub" "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" ) // GuestAPI is the narrow Proxmox surface the local API needs. Satisfied by *proxmox.Client. @@ -80,6 +81,10 @@ type Options struct { Disks DiskOps DiskGate StorageGate Guests2 GuestLister + // HostReader is the root-free host topology reader used to classify a device/mount's protection + // ROLE (it backs SystemDisks for the eject role-gate + the /disks role hints). OPTIONAL — when nil + // it defaults to the production *storage.ProcHostReader. Injectable so the role-gate is testable. + HostReader storage.HostReader // HostMetrics serves GET /host/metrics (slice 9) — host-wide health (cpu%/mem/load/uptime/ // cpu-temp) + per-storage capacity, host-wide and token-authed (one-customer-per-host). When // nil the endpoint reports "not configured" (host still reports/reconciles). @@ -127,9 +132,10 @@ type Server struct { logger *slog.Logger now func() time.Time - disks DiskOps // slice 8C (optional) - diskGate StorageGate // slice 8C (optional) - guestList GuestLister // slice 8C (optional) + disks DiskOps // slice 8C (optional) + diskGate StorageGate // slice 8C (optional) + guestList GuestLister // slice 8C (optional) + host storage.HostReader // role classification source (optional; defaults to ProcHostReader) hostMetrics HostMetricsProvider // slice 9 (optional) hostID string // slice 10B: for the data-bearing-format pending-op hint @@ -169,6 +175,7 @@ func NewServer(o Options) (*Server, error) { disks: o.Disks, diskGate: o.DiskGate, guestList: o.Guests2, + host: o.HostReader, hostMetrics: o.HostMetrics, hostID: o.HostID, jobs: map[int]*backupJob{},