From 098c6d3ec70f43f0c008748f1ba6ef81382ce16d Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 21:19:15 +0200 Subject: [PATCH] Place pacman's state outside the package manager's own directory, and refuse a module directory in a system package's (issue 1841) pacman's state directory said place ".", which resolves to /var/lib/pacman: the package manager's database directory. The node-engine used it as found and reported it unhealthy on every machine, and wrote keeper.json into it. It is now the mesh's directory for the module (place "mesh", /var/lib/mesh/pacman), as docker's is; keeper.json, the keeper's bus secret and the root helper's socket follow through ${dir:state}. checks/owned rule 5 refuses a directory that is, lies in or holds a /var/lib directory a system package owns (checks/owned/system, read from shanks through pacman_owns and tmpfiles.d), and any resource inside one. systemd and postgres collide today and are listed in checks/owned/system-known under issue 1873; that list only shrinks. --- checks/owned/main.go | 194 ++++++++++++++++-- checks/owned/main_test.go | 103 +++++++++- checks/owned/system | 23 +++ checks/owned/system-known | 7 + merge-check.sh | 18 +- .../cmd/pacman-tools/keeper_manifest_test.go | 36 +++- modules/pacman/module.json | 2 +- 7 files changed, 364 insertions(+), 19 deletions(-) create mode 100644 checks/owned/system create mode 100644 checks/owned/system-known diff --git a/checks/owned/main.go b/checks/owned/main.go index 67762f06..6a2c7078 100644 --- a/checks/owned/main.go +++ b/checks/owned/main.go @@ -11,13 +11,23 @@ // another module, after ${dir:} is resolved within each module and both homes are read as one root. // Archives are not containers for this rule. This holds for ~/.ssh what checks/sshdir held (ADR 0415 §6). // -// go run . +// 5. a directory that is, lies inside or holds a directory a system package owns under /var/lib, or any other +// resource inside one (issue 1841: pacman's state directory, placed with "place": ".", was /var/lib/pacman, the +// package manager's own database directory). A placed directory is resolved as the mesh resolves it with the +// default data root: "." is /var/lib/, "mesh" /var/lib/mesh/, neither /var/lib//. +// The system directories are checks/owned/system, captured from a machine (pacman -Qo, /usr/lib/tmpfiles.d); +// checks/owned/system-known names the exceptions still to fix, by module, resource and issue. That list only +// shrinks: an entry that no longer collides is refused, and with -base one the trunk's copy does not hold. +// +// go run . [-base ] // // Exits 1 on a finding. package main import ( "encoding/json" + "errors" + "flag" "fmt" "os" "path" @@ -27,6 +37,12 @@ import ( "strings" ) +// The lists of rule 5, relative to the catalogue's root. +const ( + SystemList = "checks/owned/system" + SystemKnown = "checks/owned/system-known" +) + // Home is the one root both machine homes are read as. const Home = "${home}" @@ -43,9 +59,9 @@ type Finding struct { } type resource struct { - ID, Type, Path, Unknown string - Knows []string - HasKnows bool + ID, Type, Path, Unknown, Place string + Knows []string + HasKnows bool } type parsed struct { @@ -65,6 +81,7 @@ func load(file string) (*parsed, error) { ID string `json:"id"` Type string `json:"type"` Path string `json:"path"` + Place string `json:"place"` Unknown *string `json:"unknown"` Knows []string `json:"knows"` } `json:"resources"` @@ -81,7 +98,7 @@ func load(file string) (*parsed, error) { out.Module = filepath.Base(filepath.Dir(file)) } for _, r := range m.Resources { - x := resource{ID: r.ID, Type: r.Type, Path: r.Path, Knows: r.Knows, HasKnows: r.Knows != nil} + x := resource{ID: r.ID, Type: r.Type, Path: r.Path, Place: r.Place, Knows: r.Knows, HasKnows: r.Knows != nil} if r.Unknown != nil { x.Unknown = *r.Unknown if x.Unknown == "" { @@ -143,14 +160,23 @@ type declared struct { } // Check reads every manifest under root/modules and answers what it refuses. -func Check(root string) ([]Finding, error) { +func Check(root string, base []string) ([]Finding, error) { files, err := filepath.Glob(filepath.Join(root, "modules", "*", "module.json")) if err != nil { return nil, err } sort.Strings(files) + system, err := readSystem(filepath.Join(root, SystemList)) + if err != nil { + return nil, err + } + known, err := readLines(filepath.Join(root, SystemKnown)) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, err + } var out []Finding var paths []declared + collided := map[string]bool{} type container struct{ module, path string } var containers []container for _, file := range files { @@ -170,6 +196,7 @@ func Check(root string) ([]Finding, error) { dirs[r.ID] = "/placed/" + m.Module + "/" + r.ID } } + out = append(out, inSystem(m, system, known, collided)...) for _, r := range m.Resources { p := norm(r.Path, dirs) if p != "" { @@ -212,6 +239,28 @@ func Check(root string) ([]Finding, error) { } } } + for _, k := range known { + if !collided[k] { + module, id, _ := strings.Cut(k, " ") + id, _, _ = strings.Cut(id, " ") + out = append(out, Finding{5, module, id, fmt.Sprintf("is listed in %s, and no longer lies in a system "+ + "package's directory: take the line out, so the list only shrinks", SystemKnown)}) + } + } + if base != nil { + held := map[string]bool{} + for _, b := range base { + held[b] = true + } + for _, k := range known { + if !held[k] { + module, id, _ := strings.Cut(k, " ") + id, _, _ = strings.Cut(id, " ") + out = append(out, Finding{5, module, id, fmt.Sprintf("is a new line in %s, which the trunk's copy "+ + "does not hold: a module's directory is placed outside a system package's, never listed", SystemKnown)}) + } + } + } for _, d := range paths { for _, c := range containers { if c.module != d.module && (d.path == c.path || strings.HasPrefix(d.path, c.path+"/")) { @@ -222,21 +271,140 @@ func Check(root string) ([]Finding, error) { return out, nil } -func main() { - root := "." - if len(os.Args) > 1 { - root = os.Args[1] +// systemDir is one directory a system package owns, and what says so. +type systemDir struct{ Path, Source string } + +// readLines is a list's lines, without comments and blank lines. +func readLines(file string) ([]string, error) { + raw, err := os.ReadFile(file) + if err != nil { + return nil, err } - found, err := Check(root) + var lines []string + for _, line := range strings.Split(string(raw), "\n") { + if line = strings.TrimSpace(line); line != "" && !strings.HasPrefix(line, "#") { + lines = append(lines, line) + } + } + return lines, nil +} + +// readSystem is the list of system directories: " : " per line. Absent or +// empty, the check fails: a list that says nothing would let every module through. +func readSystem(file string) ([]systemDir, error) { + lines, err := readLines(file) + if err != nil { + return nil, fmt.Errorf("rule 5 reads the system packages' directories from %s: %w", file, err) + } + if len(lines) == 0 { + return nil, fmt.Errorf("%s names no system package's directory, so rule 5 would pass everything", file) + } + var out []systemDir + for _, line := range lines { + p, source, _ := strings.Cut(line, " ") + if !strings.HasPrefix(p, "/var/lib/") || strings.TrimSpace(source) == "" { + return nil, fmt.Errorf("%s: %q is not \"/var/lib/ : \"", file, line) + } + out = append(out, systemDir{path.Clean(p), strings.TrimSpace(source)}) + } + return out, nil +} + +// placedAt is where the mesh places a pathless directory with the default data root (mesh-sdk catalogue dirsFor). +func placedAt(module string, r resource) string { + switch r.Place { + case ".": + return "/var/lib/" + module + case "mesh": + return "/var/lib/mesh/" + module + } + return "/var/lib/" + module + "/" + r.ID +} + +// inSystem is rule 5 for one module: each resource whose resolved path collides with a system package's directory, +// unless system-known lists it (then it is marked collided, so a stale line is found). +func inSystem(m *parsed, system []systemDir, known []string, collided map[string]bool) []Finding { + real := map[string]string{} + for _, r := range m.Resources { + if r.Type == "directory" && r.Path == "" && r.ID != "" { + real[r.ID] = placedAt(m.Module, r) + } + } + for _, r := range m.Resources { + if r.ID != "" && r.Path != "" { + real[r.ID] = r.Path + } + } + var out []Finding + for _, r := range m.Resources { + p := "" + if r.Path != "" { + p = norm(r.Path, real) + } else if r.Type == "directory" { + p = real[r.ID] + } + if !strings.HasPrefix(p, "/") { + continue + } + for _, s := range system { + inside := p == s.Path || strings.HasPrefix(p, s.Path+"/") + holds := r.Type == "directory" && strings.HasPrefix(s.Path, p+"/") + if !inside && !holds { + continue + } + listed := "" + for _, k := range known { + if strings.HasPrefix(k, m.Module+" "+r.ID+" ") || k == m.Module+" "+r.ID { + listed = k + } + } + if listed != "" { + collided[listed] = true + break + } + how := "is or lies inside" + if holds { + how = "holds" + } + out = append(out, Finding{5, m.Module, r.ID, fmt.Sprintf("resolves to %q, which %s %s, the directory of the "+ + "system package %s; place it under /var/lib/mesh/%s (\"place\": \"mesh\") or a path of its own", p, how, + s.Path, s.Source, m.Module)}) + break + } + } + return out +} + +func main() { + baseFile := flag.String("base", "", "the trunk's copy of "+SystemKnown+": a line it does not hold is refused") + flag.Parse() + root := "." + if flag.NArg() > 0 { + root = flag.Arg(0) + } + var base []string + if *baseFile != "" { + lines, err := readLines(*baseFile) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + base = append([]string{}, lines...) + } + found, err := Check(root, base) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } for _, f := range found { - fmt.Printf("%s: %s: %s (rule %d, novox/hq ADR 0422 §8, issue 1024)\n", f.Module, f.ID, f.Why, f.Rule) + cite := "novox/hq ADR 0422 §8, issue 1024" + if f.Rule == 5 { + cite = "issue 1841" + } + fmt.Printf("%s: %s: %s (rule %d, %s)\n", f.Module, f.ID, f.Why, f.Rule, cite) } if len(found) > 0 { os.Exit(1) } - fmt.Println("owned paths: every directory and archive says how it treats unknown files; no module declares inside another's directory") + fmt.Println("owned paths: every directory and archive says how it treats unknown files; no module declares inside another's directory, or in a system package's") } diff --git a/checks/owned/main_test.go b/checks/owned/main_test.go index 6426244f..ebe3981a 100644 --- a/checks/owned/main_test.go +++ b/checks/owned/main_test.go @@ -20,7 +20,10 @@ func manifest(t *testing.T, root, module, body string) { func check(t *testing.T, root string) []Finding { t.Helper() - found, err := Check(root) + if _, err := os.Stat(filepath.Join(root, SystemList)); os.IsNotExist(err) { + lists(t, root, "/var/lib/pacman pacman: pacman -Qo\n", "") + } + found, err := Check(root, nil) if err != nil { t.Fatal(err) } @@ -35,6 +38,20 @@ func only(t *testing.T, found []Finding, rule int, module, id string) { } } +// lists writes the catalogue's list of system directories and its known exceptions. +func lists(t *testing.T, root, system, known string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(root, "checks", "owned"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, SystemList), []byte(system), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, SystemKnown), []byte(known), 0o644); err != nil { + t.Fatal(err) + } +} + func dir(id, path, rest string) string { return `{"id":"` + id + `","type":"directory","path":"` + path + `"` + rest + `}` } @@ -145,3 +162,87 @@ func TestPlacedDirectoriesOfTwoModulesAreTheirOwn(t *testing.T) { t.Fatalf("refused %+v", found) } } + +// 1841/AC18: a module's directory that resolves to a /var/lib directory a system package owns, lies inside one or +// holds one, is refused, whether placed (place ".", or by default beneath the module's own) or stated; a placed +// directory saying place "mesh" lies under /var/lib/mesh and passes (issue 1841: pacman's state was /var/lib/pacman). +func TestADirectoryOfASystemPackageIsRefused(t *testing.T) { + system := "/var/lib/pacman pacman: pacman -Qo\n/var/lib/systemd/coredump systemd: tmpfiles.d/systemd.conf\n" + for name, resources := range map[string][]string{ + "placed-own": {`{"id":"state","type":"directory","place":".","unknown":"program"}`}, + "placed-beneath": {`{"id":"coredump","type":"directory","unknown":"program"}`}, + "stated-inside": {dir("d", "/var/lib/pacman/keeper", `,"unknown":"program"`)}, + "stated-holding": {dir("d", "/var/lib/systemd", `,"unknown":"program"`)}, + "file-inside": {`{"id":"state","type":"directory","place":"mesh","unknown":"program"}`, + `{"id":"f","type":"file","path":"/var/lib/pacman/keeper.json"}`}, + } { + module := map[string]string{"placed-own": "pacman", "placed-beneath": "systemd"}[name] + if module == "" { + module = "keeper" + } + root := t.TempDir() + lists(t, root, system, "") + manifest(t, root, module, mod(module, resources...)) + found := check(t, root) + if len(found) != 1 || found[0].Rule != 5 || found[0].Module != module || + !strings.Contains(found[0].Why, "/var/lib/") || !strings.Contains(found[0].Why, "package") { + t.Errorf("%s: found %+v; want one finding of rule 5 for %s naming the path and the package", name, found, module) + } + } + root := t.TempDir() + lists(t, root, system, "") + manifest(t, root, "pacman", mod("pacman", + `{"id":"state","type":"directory","place":"mesh","unknown":"program"}`, + `{"id":"f","type":"file","path":"${dir:state}/keeper.json"}`, + dir("other", "/var/lib/pacman-other", `,"unknown":"program"`))) + if found := check(t, root); len(found) != 0 { + t.Fatalf("a directory under /var/lib/mesh/pacman was refused: %+v", found) + } +} + +// 1841/AC18: a known exception passes; one that no longer collides is refused, so the list only shrinks; one the trunk's +// copy does not hold is refused; and an absent list of system directories fails the check rather than passing it. +func TestKnownSystemExceptionsOnlyShrink(t *testing.T) { + system := "/var/lib/systemd systemd: tmpfiles.d/systemd.conf\n" + known := "# comment\nsystemd state issue 1873\n" + root := t.TempDir() + lists(t, root, system, known) + manifest(t, root, "systemd", mod("systemd", `{"id":"state","type":"directory","place":".","unknown":"program"}`)) + if found := check(t, root); len(found) != 0 { + t.Fatalf("a known exception was refused: %+v", found) + } + if found, err := Check(root, []string{"systemd state issue 1873"}); err != nil || len(found) != 0 { + t.Fatalf("a known exception held by the trunk was refused: %+v %v", found, err) + } + if found, err := Check(root, []string{}); err != nil || len(found) != 1 || found[0].Rule != 5 { + t.Fatalf("an exception the trunk does not hold passed: %+v %v", found, err) + } + + manifest(t, root, "systemd", mod("systemd", `{"id":"state","type":"directory","place":"mesh","unknown":"program"}`)) + only(t, check(t, root), 5, "systemd", "state") + + root = t.TempDir() + manifest(t, root, "a", mod("a")) + if err := os.MkdirAll(filepath.Join(root, "checks", "owned"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, SystemList), []byte("# nothing\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := Check(root, nil); err == nil { + t.Fatal("an empty list of system directories passed") + } +} + +// 1841/AC18: the catalogue as it stands passes rule 5 with its own lists. +func TestTheCatalogueKeepsOutOfSystemDirectories(t *testing.T) { + found, err := Check("../..", nil) + if err != nil { + t.Fatal(err) + } + for _, f := range found { + if f.Rule == 5 { + t.Errorf("%s: %s: %s", f.Module, f.ID, f.Why) + } + } +} diff --git a/checks/owned/system b/checks/owned/system new file mode 100644 index 00000000..2a37130f --- /dev/null +++ b/checks/owned/system @@ -0,0 +1,23 @@ +# The directories under /var/lib that a system package owns (rule 5 of checks/owned, issue 1841). A module's +# directory, placed or stated, may not be one of these, lie inside one or hold one: the package's program and the +# package manager keep their own files there, and the node-engine would judge, chmod or chown the directory. +# +# One line per directory: " : ". Captured on the workstation on 2026-10-22, through +# the mesh: pacman's own database by `pacman -Qo` (pacman_owns), the rest from the `d`, `D` and `Q` lines of +# /usr/lib/tmpfiles.d (node-reads.search for "/var/lib/"). A directory found to belong to a package is added here. +/var/lib/pacman pacman: pacman -Qo /var/lib/pacman/ (pacman 7.1.0) +/var/lib/alsa alsa-utils: tmpfiles.d/alsa-utils.conf +/var/lib/colord colord: tmpfiles.d/colord.conf +/var/lib/containers podman: tmpfiles.d/podman.conf +/var/lib/dbus dbus: tmpfiles.d/dbus.conf +/var/lib/geoclue geoclue: tmpfiles.d/geoclue.conf +/var/lib/lastlog util-linux: tmpfiles.d/lastlog2.conf +/var/lib/libuuid util-linux: tmpfiles.d/uuidd.conf +/var/lib/machines systemd: tmpfiles.d/systemd-nspawn.conf +/var/lib/openvpn/chroot networkmanager-openvpn: tmpfiles.d/nm-openvpn-tmpfiles.conf +/var/lib/portables systemd: tmpfiles.d/portables.conf +/var/lib/postgres postgresql: tmpfiles.d/postgresql.conf +/var/lib/private systemd: tmpfiles.d/systemd.conf +/var/lib/rpcbind rpcbind: tmpfiles.d/rpcbind.conf +/var/lib/systemd systemd: tmpfiles.d/systemd.conf +/var/lib/tpm2-tss/system/keystore tpm2-tss: tmpfiles.d/tpm2-tss-fapi.conf diff --git a/checks/owned/system-known b/checks/owned/system-known new file mode 100644 index 00000000..dcb8b120 --- /dev/null +++ b/checks/owned/system-known @@ -0,0 +1,7 @@ +# Modules whose directory lies in a system package's directory today (rule 5 of checks/owned, issue 1841), each with +# the issue that moves it out: " issue ". This list only shrinks: a line whose module +# no longer collides is refused, and so is a line the trunk's copy does not hold. +postgres consumers issue 1873 +postgres grants issue 1873 +postgres state issue 1873 +systemd state issue 1873 diff --git a/merge-check.sh b/merge-check.sh index 3fe55bd1..dd762c6d 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -48,7 +48,8 @@ # 10. the mail-store seat (novox/hq ADR 0378 §3, issue 800): no manifest but modules/mesh-mail claims it. # 11. owned paths (novox/hq ADR 0422 §8, issue 1024; it holds ~/.ssh for ssh-client as ADR 0415 §6 asked): # every directory and archive says `unknown` (move-out, module or program) and a valid `knows`; `move-out` -# only under a home; and no module declares a path inside a directory of another module. +# only under a home; no module declares a path inside a directory of another module; and none is, lies in or +# holds a directory a system package owns under /var/lib (checks/owned/system, issue 1841). # checks/sshdir (ADR 0415 §6) stays while main runs it; it goes under issue 1035 once main runs checks/owned. # 12. the image builder (novox/hq ADR 0487 §3, issue 1704): only modules/buildah provides rootless-image-build. # 13. the image's place (novox/hq ADR 0487 §1, issue 1707): every image builds from a Dockerfile in its own module's directory. @@ -71,8 +72,19 @@ set -eu # ~/.ssh (novox/hq ADR 0415 §6): only ssh-client may name a path under it. Main's merge check runs this line, so # checks/sshdir stays until main runs checks/owned; it goes under issue 1035. (cd checks/sshdir && go test -count=1 . && go run . ../..) -# Owned paths (novox/hq ADR 0422 §8): unknown files named per directory; no path inside another module's directory. -(cd checks/owned && go test -count=1 . && go run . ../..) +# Owned paths (novox/hq ADR 0422 §8): unknown files named per directory; no path inside another module's directory; +# none in a system package's directory under /var/lib (issue 1841), the known exceptions only shrinking: judged +# against the trunk's copy of checks/owned/system-known at the merge base; a trunk without one is the list's first copy. +owned_base="" +if merge_base=$(git merge-base HEAD "origin/${MESH_CHECK_BASE:-main}" 2>/dev/null); then + if git cat-file -e "$merge_base:checks/owned/system-known" 2>/dev/null; then + owned_base=$(mktemp) + git show "$merge_base:checks/owned/system-known" >"$owned_base" + fi +else + echo "NOT COMPARED WITH THE TRUNK: no merge base with origin/${MESH_CHECK_BASE:-main}; checks/owned/system-known is held to the list it was first written with" +fi +(cd checks/owned && go test -count=1 . && if [ -n "$owned_base" ]; then go run . -base "$owned_base" ../..; else go run . ../..; fi) # The secrets (novox/hq issue 342): a password is not a setting, so no setting can replace the vault's. The # known list only shrinks: judged against the trunk's copy at the merge base, a line it does not hold is refused. secrets_base="" diff --git a/modules/pacman/cmd/pacman-tools/keeper_manifest_test.go b/modules/pacman/cmd/pacman-tools/keeper_manifest_test.go index e0d9db80..be4304c0 100644 --- a/modules/pacman/cmd/pacman-tools/keeper_manifest_test.go +++ b/modules/pacman/cmd/pacman-tools/keeper_manifest_test.go @@ -83,7 +83,7 @@ func TestTheKeeperIsAProcessUnderItsOwnAccountWithABusAccountOfItsOwn(t *testing t.Errorf("the bus account is the keeper's own secret, owned by its account: %v", secrets) } state := m.resource(t, "state") - if state["type"] != "directory" || state["place"] != "." || state["unknown"] == nil { + if state["type"] != "directory" || state["place"] != "mesh" || state["unknown"] == nil { t.Errorf("the state directory: %v", state) } order := map[string]int{} @@ -211,3 +211,37 @@ func TestUpgradeOrderIsATrustedListOfTheMesh(t *testing.T) { t.Error("a value falls back only to a key of its own type (ADR 0348 §4)") } } + +// 1841/AC17: the keeper's state directory is the mesh's directory for the module (place "mesh", /mesh/pacman), +// never place "." or a placement by default: for this module both resolve under /var/lib/pacman, the package manager's +// own database directory (issue 1841: the node-engine used it as found and reported it unhealthy, and wrote keeper.json +// into it). What the keeper and its root helper keep there is still named through ${dir:state}. +func TestTheStateDirectoryIsOutsideThePackageManagersOwn(t *testing.T) { + m := rawManifest(t) + resources, _ := m["resources"].([]any) + var state map[string]any + for _, x := range resources { + if r, _ := x.(map[string]any); r["id"] == "state" { + state = r + } + } + if state == nil || state["type"] != "directory" { + t.Fatalf("no state directory: %v", state) + } + if state["place"] != "mesh" || state["path"] != nil { + t.Fatalf("the state directory says place %v and path %v; it is placed \"mesh\" with no path, so it resolves to "+ + "/var/lib/mesh/pacman and not into /var/lib/pacman", state["place"], state["path"]) + } + raw, err := os.ReadFile(filepath.Join(moduleDir, "module.json")) + if err != nil { + t.Fatal(err) + } + if bytes.Contains(raw, []byte("/var/lib/pacman")) { + t.Fatal("the manifest names /var/lib/pacman") + } + for _, want := range []string{"${dir:state}/keeper.json", "${dir:state}/broker-keeper.secret", RootSocketPath} { + if !bytes.Contains(raw, []byte(want)) { + t.Errorf("the manifest no longer names %s", want) + } + } +} diff --git a/modules/pacman/module.json b/modules/pacman/module.json index 69d08ff7..97eaad3f 100644 --- a/modules/pacman/module.json +++ b/modules/pacman/module.json @@ -147,7 +147,7 @@ "id": "state", "type": "directory", "mode": "0711", - "place": ".", + "place": "mesh", "unknown": "program" }, {