Security review of 912f7c1: the mkfs wrapper trusts no sibling claim; the internal-place helper fails closed

- felhom-mkfs-guarded: the 0.158.0 exemption for a sibling part mounted at /mnt/<name> + under /mnt/felhom-drives is
  removed — the agent can make that corroborating mount itself. Every mount of every node outside /mnt/felhom-drives
  refuses (kernel mount table); an unlistable disk refuses. CandidatePartition.Formattable is false for every part of a
  disk with a mounted node, so it is never offered for formatting (attach still is).
- felhom-internal-place: a failed lvs listing, a listing without the pool, or a blkid exit other than 0/2 refuses
  (they read as "nothing promised" / "no signature" before).
Every new test was seen failing against the 0.158.0 versions.

Claude-Session: https://claude.ai/code/session_012qRErfCoiTkvDK9N5XHbzb

REUSE.md

@@ -44,7 +44,7 @@
 | `storage.IdentityResolver` / `Relation` / `MountFor` | internal/storage/identity.go | `HostPath(path) PlaceIdentity`, `Device(dev) PlaceIdentity`, `Relation(a, b) (different, proven, reason)` | physical-disk identity of a storage place (felhom.eu `16-storage-physical-identity.md` §3, v0.157.0) | Reads /sys + /run/udev/data + /proc/mounts only (no block I/O). Judge a drive ONLY on its own mount (`localapi.ownMountIdentity`) — never the containing fs, or an absent drive reads as the root disk. A shared disk key is "same, proven" even when unknown. Pinned by identity_test.go (fixture trees) + samedisk_test.go |
 | `localapi.Server.EnsureInternalPlace` / `InternalPlaceRecord` | internal/localapi/internalplace.go | `EnsureInternalPlace(ctx)` (startup + 20 s tick); `ReadInternalPlace` / `WriteInternalPlace` | the one-disk box's internal data place (felhom.eu `16` §5, v0.158.0): mounts it at /mnt/belso by fs-UUID like an enrolled drive, records it `enrolled`; the controller binds + registers it | Never takes /mnt/belso from a household drive (a foreign mount, or a unit naming another UUID, waits). Never formats/ejects (dm on the system disk → role system). The ONLY maker is the root helper `configs/felhom-internal-place` (installer + signed `add_internal_place`). Pinned by internalplace_test.go |
 | `configs/felhom-internal-place` | configs/ (bundle → /usr/local/sbin) | `plan\|create <planned GiB>` | the ruled size rule (reserve max(16, 10 %) ≤ 48 GiB; ≥ 20 GiB or nothing) + the only lvcreate/mkfs of a system-disk volume | Formats only a felhom-belso with NO signature; refuses a foreign one; never resizes/removes. Sudoers: `^(plan\|create) 0$` only. Pinned by configs/test_felhom_internal_place.py |
-| `storage.candidatePartitions` / `CandidatePartition` | internal/storage/partitions.go | `CandidateDisk.Partitions` | partitions as places (`16` §6): empty / filesystem / in_use per part of a free disk | A part with children or a member type is never listed; a raw /mnt mount counts as Felhom's only when corroborated (felhomOwnedMounts). Pinned by partitions_test.go |
+| `storage.candidatePartitions` / `CandidatePartition` | internal/storage/partitions.go | `CandidateDisk.Partitions` | partitions as places (`16` §6): empty / filesystem / in_use per part of a free disk; `Formattable` only while no node of the disk is mounted | A part with children or a member type is never listed; a raw /mnt mount counts as Felhom's only when corroborated (felhomOwnedMounts) — for LISTING only: the root mkfs wrapper trusts no such claim (v0.158.1). Pinned by partitions_test.go |
 | partition wipe id `byid:<…-partN>+fsuuid:<uuid>` | internal/storage/durable_device.go | `DeviceDurableID` / `ResolveDurableDevice` | a data-bearing PART binds disk id + part number + filesystem (`16` §6) | Both halves must resolve to the SAME device or the wipe refuses. Whole disks keep the plain id. Pinned by partition_durable_test.go |
 | `ValidateUUID/MountPath/BlockDevice/FSType/SMARTDevice/LVMName`, `UnitNameForMount` | internal/storage/validate.go | `Validate*(v) error` | EVERY arg that reaches a root shell-out | The security boundary; strict whitelists (no by-* symlinks, no dm, no traversal); `systemdEscapePath` computed in-process |
 | `ValidateNetworkMountSpec` | internal/storage/netmount.go | `ValidateNetworkMountSpec(spec) error` | NAS mount input boundary | Same discipline as validate.go; SMB requires a creds ref; mountpoint confined under `NetworkMountRoot` |

configs/felhom-internal-place

@@ -55,15 +55,19 @@ pool_gib=$(gib_floor "$pool_b")
 
 # Every thin volume promised in the pool (its virtual size) — except the place itself and vzdump's short-lived
 # snapshots (snap_*_vzdump), which exist only during a whole-box copy and are what the reserve is for.
-promised_b=0; have_lv=false
+# FAIL CLOSED: a failed or empty listing must never read as „nothing promised" (that would size the place too big).
+listing=$(lvs --noheadings --nosuffix --units b -o lv_name,pool_lv,lv_size "$VG" 2>/dev/null) || die "cannot list the volumes of $VG"
+pool_seen=false; promised_b=0; have_lv=false
 while read -r name pl size; do
 	[[ -n "${name:-}" ]] || continue
+	[[ "$name" == "$POOL" ]] && pool_seen=true
 	[[ "$pl" == "$POOL" ]] || continue
 	if [[ "$name" == "$LV_NAME" ]]; then have_lv=true; continue; fi
 	[[ "$name" =~ ^snap_.*_vzdump$ ]] && continue
 	[[ "$size" =~ ^[0-9]+$ ]] || die "cannot read the size of $VG/$name"
 	promised_b=$(( promised_b + size ))
-done < <(lvs --noheadings --nosuffix --units b -o lv_name,pool_lv,lv_size "$VG" 2>/dev/null)
+done <<< "$listing"
+$pool_seen || die "the listing of $VG does not contain the pool $POOL — refusing to guess what is promised"
 promised_gib=$(( (promised_b + 1073741823) / 1073741824 ))   # rounded UP: never promise a partial GiB twice
 
 reserve=$(( pool_gib / 10 )); (( reserve < 16 )) && reserve=16; (( reserve > 48 )) && reserve=48
@@ -79,6 +83,17 @@ is_block() { [[ "$(stat -L -c %F "$1" 2>/dev/null)" == "block special file" ]];
 fs_uuid() { blkid -p -s UUID -o value "$DEV" 2>/dev/null; }
 fs_type() { blkid -p -s TYPE -o value "$DEV" 2>/dev/null; }
 fs_label() { blkid -p -s LABEL -o value "$DEV" 2>/dev/null; }
+# has_signature: FAIL CLOSED. blkid -p exits 0 when it found something and 2 when it found NOTHING; any other result (an
+# error, an ambivalent probe, blkid missing) must never read as „empty" — that is the only gate in front of mkfs.
+has_signature() {
+	local rc=0
+	blkid -p -o value "$DEV" >/dev/null 2>&1 || rc=$?
+	case $rc in
+		0) return 0 ;;
+		2) return 1 ;;
+		*) die "cannot probe $DEV for a signature (blkid exit $rc) — refusing" ;;
+	esac
+}
 lv_gib() { gib_floor "$(lvs --noheadings --nosuffix --units b -o lv_size "$VG/$LV_NAME" 2>/dev/null | tr -d ' ')"; }
 
 if $have_lv; then
@@ -89,12 +104,12 @@ if $have_lv; then
 		echo "existing uuid=$(fs_uuid) gib=$(lv_gib)"; exit 0
 	fi
 	# A felhom-belso with NO signature: a crash between lvcreate and mkfs. Nothing can be on it — format it.
-	[[ -z "$(blkid -p -o value "$DEV" 2>/dev/null)" ]] || die "$DEV carries a signature blkid cannot name — left alone"
+	has_signature && die "$DEV carries a signature blkid cannot name — left alone"
 else
 	(( internal >= MIN_GIB )) || { echo "skipped only $internal GiB free for it (pool $pool_gib, reserve $reserve, promised $promised_gib, planned $PLANNED; needs $MIN_GIB)"; exit 0; }
 	lvcreate -q -y -V "${internal}G" -T "$VG/$POOL" -n "$LV_NAME" >/dev/null || die "lvcreate failed"
 	is_block "$DEV" || die "$DEV did not appear after lvcreate"
-	[[ -z "$(blkid -p -o value "$DEV" 2>/dev/null)" ]] || die "the new $DEV already carries a signature — left alone"
+	has_signature && die "the new $DEV already carries a signature — left alone"
 	made=created
 fi
 mkfs.ext4 -q -L "$LABEL" "$DEV" >/dev/null || die "mkfs.ext4 failed on $DEV"

configs/felhom-mkfs-guarded.sh

@@ -58,31 +58,23 @@ fi
 
 # 3) mounted OUTSIDE Felhom's own drive area = a live foreign filesystem → catastrophic. Mounts under
 #    /mnt/felhom-drives are our own drives (the agent detaches before a re-init) → allowed.
-#    Agent v0.158.0 (felhom.eu architecture/16 §6, partitions as places): a SIBLING partition that is Felhom's own
-#    enrolled drive is mounted TWICE — raw at /mnt/<name> and under /mnt/felhom-drives (R-220). That raw mount is
-#    allowed only when the same device is ALSO mounted under /mnt/felhom-drives, a state only Felhom's enrolment
-#    produces. The TARGET itself mounted anywhere outside /mnt/felhom-drives, or any mount elsewhere (/srv, /media,
-#    /mnt/a/b), still REFUSES. Read from the kernel's mount table, every node of the disk (partitions and children).
+#    Agent v0.158.0: read from the kernel's mount table for EVERY node of the disk (lsblk's MOUNTPOINT shows one mount
+#    per node; a bind elsewhere hid behind it), and a disk whose nodes cannot be listed REFUSES (it passed before).
+#    Deliberately NO exemption for a sibling that is Felhom's own enrolled drive (raw /mnt/<name> + /mnt/felhom-drives):
+#    the only corroboration available here is a mount the agent itself can make, so trusting it would let a compromised
+#    agent unlock a host owner's disk. Consequence (felhom.eu `16` §13): the free part next to an in-use Felhom drive
+#    cannot be formatted; the dashboard does not offer it.
 PROC_MOUNTS=/proc/mounts
 node_mounts() { awk -v d="$1" '$1 == d { print $2 }' "$PROC_MOUNTS"; }
 disk_nodes="$(lsblk -nrpo NAME "$whole" 2>/dev/null || true)"
 [[ -n "$disk_nodes" ]] || die "cannot list the nodes of $whole — refusing"
 while read -r node; do
   [[ -z "$node" ]] && continue
-  own=false
-  while read -r mp; do
-    case "$mp" in /mnt/felhom-drives/*) own=true ;; esac
-  done < <(node_mounts "$node")
   while read -r mp; do
     [[ -z "$mp" ]] && continue
     case "$mp" in
       /mnt/felhom-drives|/mnt/felhom-drives/*) : ;;
-      *)
-        if [[ "$node" != "$dev" && "$own" == true && "$mp" =~ ^/mnt/[A-Za-z0-9_][A-Za-z0-9_.-]*$ ]]; then
-          : # a sibling that is Felhom's own enrolled drive (raw mount + managed mount)
-        else
-          die "device (or a partition) is mounted at $mp ($node)"
-        fi ;;
+      *) die "device (or a partition) is mounted at $mp ($node)" ;;
     esac
   done < <(node_mounts "$node")
 done <<< "$disk_nodes"

configs/test_felhom_internal_place.py

@@ -14,12 +14,16 @@ GIB = 1 << 30
 STUB_LVS = r'''#!/usr/bin/env python3
 import json, os, sys
 st = json.load(open(os.environ["IP_STATE"]))
+if st.get("lvs_fail_listing") and "lv_name,pool_lv,lv_size" in sys.argv: sys.exit(5)
 args = [a for a in sys.argv[1:] if not a.startswith("-") and a not in ("b",)]
 fields = sys.argv[sys.argv.index("-o") + 1].split(",")
 target = args[-1]
+listing = target == st["vg"]
 for lv in st["lvs"]:
     full = st["vg"] + "/" + lv["name"]
-    if target == st["vg"] or target == full:
+    if listing and st.get("hide_pool_in_listing") and lv.get("pool", "") == "":
+        continue   # a listing that silently lost the pool row (the pool's own size stays readable)
+    if listing or target == full:
         row = {"lv_name": lv["name"], "pool_lv": lv.get("pool", ""), "lv_size": str(lv["size"])}
         print("  " + " ".join(row[f] for f in fields))
 '''
@@ -35,15 +39,18 @@ if st.get("dirty_new"): st["fs"]["felhom-belso"] = {"TYPE": "", "RAW": "junk"}
 json.dump(st, open(p, "w"))
 '''
 STUB_BLKID = r'''#!/usr/bin/env python3
+# like blkid -p: exit 0 when something is found, 2 when nothing is; st["blkid_rc"] forces an error exit
 import json, os, sys
 st = json.load(open(os.environ["IP_STATE"]))
+if st.get("blkid_rc"): sys.exit(st["blkid_rc"])
 fs = st["fs"].get(os.path.basename(sys.argv[-1]), {})
 if "-s" in sys.argv:
     v = fs.get(sys.argv[sys.argv.index("-s") + 1], "")
     if v: print(v)
-else:
-    vals = [fs.get(k, "") for k in ("TYPE", "LABEL", "UUID", "RAW") if fs.get(k)]
-    if vals: print("\n".join(vals))
+    sys.exit(0 if v else 2)
+vals = [fs.get(k, "") for k in ("TYPE", "LABEL", "UUID", "RAW") if fs.get(k)]
+if vals: print("\n".join(vals))
+sys.exit(0 if vals else 2)
 '''
 STUB_MKFS = r'''#!/usr/bin/env python3
 import json, os, sys
@@ -213,5 +220,36 @@ class Create(unittest.TestCase):
             b.close()
 
 
+class FailsClosed(unittest.TestCase):
+    """A read that fails must never read as „empty" or „nothing promised". Positive control: Create (same setups)."""
+
+    def test_a_failed_volume_listing_creates_nothing(self):
+        b = Box(349, lvs_fail_listing=True)
+        try:
+            rc, out, err = b.run("create", "132")
+            self.assertEqual(rc, 1, out)
+            self.assertEqual(b.calls(), [])
+        finally:
+            b.close()
+
+    def test_a_listing_without_the_pool_creates_nothing(self):
+        b = Box(349, [thin("vm-9201-disk-1", 250)], hide_pool_in_listing=True)
+        try:
+            self.assertEqual(b.run("create", "132")[0], 1)
+            self.assertEqual(b.calls(), [])
+        finally:
+            b.close()
+
+    def test_a_blkid_error_never_reads_as_empty(self):
+        for setup in ({"lvs": [thin("felhom-belso", 183)], "fs": {}}, {}):
+            b = Box(349, **({"lvs": setup["lvs"], "fs": setup["fs"]} if setup else {}), blkid_rc=4)
+            try:
+                rc, out, err = b.run("create", "0" if setup else "132")
+                self.assertEqual(rc, 1, (setup, out))
+                self.assertNotIn("mkfs", " ".join(b.calls()), setup)
+            finally:
+                b.close()
+
+
 if __name__ == "__main__":
     unittest.main()

configs/test_felhom_mkfs_guarded.py

@@ -1,9 +1,10 @@
 #!/usr/bin/env python3
 """Tests for configs/felhom-mkfs-guarded.sh — the ONLY mkfs path the agent's sudoers permits.
 
-First tests of this wrapper (agent v0.158.0), written with its change for partitions as places (felhom.eu
-architecture/16 §6): a SIBLING partition that is Felhom's own enrolled drive (mounted raw at /mnt/<name> AND under
-/mnt/felhom-drives) no longer blocks formatting the empty partition next to it; every other mount still refuses.
+First tests of this wrapper (agent v0.158.0), written for partitions as places (felhom.eu architecture/16 §6). Every
+mount of every node of the disk outside /mnt/felhom-drives refuses — INCLUDING a sibling that is Felhom's own drive
+(its raw /mnt/<name> mount): the agent can make that corroborating mount itself, so the root wrapper must not trust
+it (security review of 912f7c1). An unlistable disk refuses.
 
 HOW IT RUNS ANYWHERE: the wrapper is copied with five literal host paths replaced (each replacement is asserted, so a
 drifted wrapper fails here instead of silently testing nothing): /proc/mounts → a fake mount table, the block-device
@@ -110,8 +111,12 @@ class Partitions(unittest.TestCase):
         finally:
             d.close()
 
-    def test_the_empty_part_next_to_felhoms_own_drive_is_formatted(self):
-        self.check(OWN_SDB1)
+    def test_a_free_part_on_an_unmounted_disk_is_formatted(self):
+        self.check([])   # positive control for every refusal below
+
+    def test_the_free_part_next_to_felhoms_own_drive_is_refused(self):
+        # The agent could make the „corroborating" managed mount itself: the root wrapper does not trust it.
+        self.check(OWN_SDB1, want_ok=False)
 
     def test_a_sibling_mounted_raw_only_is_refused(self):
         self.check([("/dev/sdb1", "/mnt/hdd_1")], want_ok=False)
@@ -131,9 +136,9 @@ class Partitions(unittest.TestCase):
 
     def test_the_catastrophic_cases_still_refuse(self):
         self.check([("/dev/sdb1", "/")], want_ok=False)                     # system disk
-        self.check(OWN_SDB1, nodes=disk_sdb(sdb1_fstype="LVM2_member"), want_ok=False)  # a member signature
-        self.check(OWN_SDB1, pvs="/dev/sdb1", want_ok=False)                # an LVM PV (pvs)
-        self.check(OWN_SDB1, ro="1", want_ok=False)                         # read-only
+        self.check([], nodes=disk_sdb(sdb1_fstype="LVM2_member"), want_ok=False)  # a member signature
+        self.check([], pvs="/dev/sdb1", want_ok=False)                      # an LVM PV (pvs)
+        self.check([], ro="1", want_ok=False)                               # read-only
 
 
 if __name__ == "__main__":

internal/storage/partitions.go

@@ -13,7 +13,8 @@ import (
 // A disk that passed the WHOLE-disk unclaimed filter (classifyClaim: not the system disk, no LVM/ZFS/RAID/LUKS/swap
 // member on any node, no foreign mount on any node) also lists its partitions, each with its own state:
 //
-//	empty       no filesystem, no children           → „format this part" (self-serve: the agent's probe says blank)
+//	empty       no filesystem, no children           → „format this part" (self-serve: the agent's probe says blank) —
+//	                                                   only while NO part of the disk is mounted (Formattable)
 //	filesystem  ext4/xfs (mountable) or another fs   → „use this part" when mountable; „format this part" only through
 //	                                                   the customer-confirmed data-bearing path (the agent's own probe)
 //	in_use      already Felhom's own drive           → shown, never offered (it is a registered drive already)
@@ -32,6 +33,10 @@ type CandidatePartition struct {
 	Mountable bool   `json:"mountable"`
 	DurableID string `json:"durable_id,omitempty"` // uuid:<fs-uuid> — the attach key ("" when empty)
 	MountedAt string `json:"mounted_at,omitempty"` // in_use: Felhom's own mount
+	// Formattable is false when ANY node of the disk is mounted (another part in use): the root mkfs wrapper refuses
+	// every mount outside /mnt/felhom-drives on the disk and deliberately does not trust a sibling that claims to be
+	// Felhom's own (a compromised agent could fake that). Such a part can still be USED (attach erases nothing).
+	Formattable bool `json:"formattable"`
 }
 
 const (
@@ -55,6 +60,19 @@ func candidatePartitions(lsblkJSON []byte, wholeName string, sizeOf func(string)
 		if d.Name != wholeName {
 			continue
 		}
+		diskMounted := d.MountPoint != ""
+		var walk func(n lsblkDev)
+		walk = func(n lsblkDev) {
+			if n.MountPoint != "" {
+				diskMounted = true
+			}
+			for _, c := range n.Children {
+				walk(c)
+			}
+		}
+		for _, c := range d.Children {
+			walk(c)
+		}
 		for _, p := range d.Children {
 			if len(p.Children) > 0 || memberFSTypes[p.FSType] {
 				continue // LVM/mapper on it, or a member: not a place (the disk filter refuses these anyway)
@@ -74,6 +92,7 @@ func candidatePartitions(lsblkJSON []byte, wholeName string, sizeOf func(string)
 					c.DurableID = "uuid:" + u
 				}
 			}
+			c.Formattable = c.State != PartInUse && !diskMounted
 			out = append(out, c)
 		}
 	}

internal/storage/partitions_test.go

@@ -71,3 +71,21 @@ func TestPartitions_UncorroboratedMountIsNotOffered(t *testing.T) {
 		}
 	}
 }
+
+// The root wrapper refuses any format on a disk with a mounted node (and trusts no „it is Felhom's own" claim), so a
+// part next to a mounted one is never offered for formatting — only for use. Positive control: an unmounted disk.
+func TestPartitions_FormattableOnlyWhenNothingOnTheDiskIsMounted(t *testing.T) {
+	m := listParts(map[string]bool{"/mnt/films": true})
+	for dev, p := range m {
+		if p.Formattable {
+			t.Fatalf("%s: a part of a disk with a mounted part must not be formattable: %+v", dev, p)
+		}
+	}
+	free := `{"blockdevices":[{"name":"sdc","fstype":null,"mountpoint":null,"children":[
+	  {"name":"sdc1","fstype":null,"mountpoint":null},{"name":"sdc2","fstype":"ext4","mountpoint":null}]}]}`
+	got := candidatePartitions([]byte(free), "sdc", func(string) int64 { return 1 << 30 }, func(string) int { return 1 },
+		func(string) (string, bool) { return "u", true }, nil)
+	if len(got) != 2 || !got[0].Formattable || !got[1].Formattable {
+		t.Fatalf("parts of an unmounted disk are formattable, got %+v", got)
+	}
+}