From e3c3bb37d0cd49c27910e8314fa15786d415048c Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 14:00:53 +0200 Subject: [PATCH] Give a mesh directory its mode and owner only through a descriptor, and gate what the walk refuses (issue 358) A mesh directory outside every home was chmodded and chowned by path, so a parent turned into a link between two applies carried root's chmod and chown to the link's target. It is now made and changed through the descriptor of the walk from / that follows a link only root alone could have made. A directory that walk refuses is a refusal like the placement guard's (review 47): judged before anything is applied, and again when the directory is applied, used as found or not; refused at either point it gates its module and refuses what is below it, so no file is written into the link's target and the report does not say everything was attempted. --- internal/apply/apply.go | 31 +++++++ internal/apply/homes_other.go | 11 +++ internal/apply/mesh_dirs_apply_linux.go | 114 ++++++++++++++++++++++++ internal/apply/mesh_dirs_gate_test.go | 95 ++++++++++++++++++++ internal/apply/mesh_dirs_test.go | 91 +++++++++++++++++++ internal/apply/placement_guard.go | 14 +++ internal/apply/replay358_test.go | 83 +++++++++++++++++ 7 files changed, 439 insertions(+) create mode 100644 internal/apply/mesh_dirs_apply_linux.go create mode 100644 internal/apply/mesh_dirs_gate_test.go create mode 100644 internal/apply/mesh_dirs_test.go create mode 100644 internal/apply/replay358_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 867293b..1ff9a5b 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -728,6 +728,13 @@ func ApplyMindingWindows( // The file guard (refusedPlaces) refuses what is below the directory whatever its module; this // keeps the module from going on as if its directory were there. Only the guard's refusals: a // directory that failed to be made is a failure like any other. + // A directory refused at the moment of use (novox/hq issue 358: a link another account could make + // found in its path by the walk) is a refusal like the guard's, from here on: it gates its module, and + // what is below it is refused whatever its module (applyOne's guard reads in.refused as it grows). + var refusal *PlacementRefusedError + if dir, ok := resource.(*declaration.Directory); ok && errors.As(err, &refusal) && in.refused[filepath.Clean(dir.Path)] == nil { + in.refused[filepath.Clean(dir.Path)] = err + } if dir, ok := resource.(*declaration.Directory); ok && in.refused[filepath.Clean(dir.Path)] != nil { if module, ours := moduleOf(resource.Identity()); ours { gated[module] = true @@ -1054,6 +1061,13 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru if err := in.refused[filepath.Clean(r.Target())]; err != nil { return begin(r), err } + // And below a directory refused during this apply (novox/hq issue 358), which the guard could not see + // before it began. + if _, isDir := r.(*declaration.Directory); !isDir { + if err := belowRefused(r.Target(), in.refused); err != nil { + return begin(r), err + } + } case *declaration.Container: if err := refuseMounts(res, in.refused); err != nil { return begin(r), err @@ -1147,6 +1161,18 @@ func applyDirectory(r *declaration.Directory, previous store.Applied, wasBefore if existed && !before.IsDir() { return out, fmt.Errorf("%s exists and is not a directory", r.Path) } + // Outside every home, nothing more is done — not even used as found, which lets what is declared below it go + // on — when the walk finds a link another account could make in its path now (novox/hq issue 358). A handed-over + // directory has its own, stricter walk below. + if meshDirsByDescriptor && existed && homeAbove(r.Path) == "" && !HandedOver()[handOverKey(r.ID, r.Path)] { + if err := judgeMeshDir(r.Path); err != nil { + var link *LinkInPathError + if errors.As(err, &link) { + return out, &PlacementRefusedError{Path: r.Path, Why: "it " + link.why()} + } + return out, err + } + } // Handed to the mesh by the operator (`nox node hand-over`, or `hand-over` at the machine): given the declared // owner and mode through a descriptor that followed no link (giveHandedOver) — **on every apply, not only the // first** (review of issue 356). After the first, the record is no longer used as found and would read as the @@ -1181,6 +1207,11 @@ func applyDirectory(r *declaration.Directory, previous store.Applied, wasBefore } } + // Outside every home, made and given its mode and owner only through a descriptor opened from /, following + // no link another account could have made (novox/hq issue 358, mesh_dirs.go). + if meshDirsByDescriptor && homeAbove(r.Path) == "" { + return applyMeshDirectory(r, out, mode, existed) + } if !existed { if err := makeDirs(r.Path, mode, r.Owner); err != nil { return out, err diff --git a/internal/apply/homes_other.go b/internal/apply/homes_other.go index cf5c92e..87cc3ec 100644 --- a/internal/apply/homes_other.go +++ b/internal/apply/homes_other.go @@ -8,6 +8,8 @@ package apply import ( "os" "path/filepath" + + "github.com/novox/mesh-host/internal/declaration" ) func chmodUnder(home, path string, mode os.FileMode) error { @@ -71,3 +73,12 @@ func judgeMeshDir(path string) error { return &PlacementRefusedError{Path: path, Why: "a mesh directory is judged only by a walk that follows no link " + "another account could make, which this system cannot do"} } + +// meshDirsByDescriptor is false where there is no openat with O_NOFOLLOW: a mesh directory is changed by path, as +// before novox/hq issue 358. The node-engine runs on Linux. +const meshDirsByDescriptor = false + +func applyMeshDirectory(r *declaration.Directory, out Outcome, _ os.FileMode, _ bool) (Outcome, error) { + return out, &PlacementRefusedError{Path: r.Path, Why: "a mesh directory is changed only through a descriptor " + + "that followed no link another account could make, which this system cannot open"} +} diff --git a/internal/apply/mesh_dirs_apply_linux.go b/internal/apply/mesh_dirs_apply_linux.go new file mode 100644 index 0000000..754c845 --- /dev/null +++ b/internal/apply/mesh_dirs_apply_linux.go @@ -0,0 +1,114 @@ +//go:build linux + +package apply + +// The apply half of mesh_dirs.go (novox/hq issue 358): a mesh directory outside every home made and given its mode +// and owner through the walk's descriptor. + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" + + "github.com/novox/mesh-host/internal/declaration" +) + +// meshDirsByDescriptor says this system can open a directory component by component without following a link. +const meshDirsByDescriptor = true + +// openMeshDir opens the directory at path, from /, one component at a time with O_DIRECTORY|O_NOFOLLOW; a missing +// component is made in its parent's descriptor with mode, and made is called with each one made and its descriptor. +// A link in a parent is followed only when couldReplace and linkReplacer find that root alone could have made it, +// and its target is walked again from /; any other link is a LinkInPathError, and the last component being one is +// refused as not placed. Answers the directory's descriptor, which the caller closes, with the walk: what it made +// and the path it reached the directory at. +func openMeshDir(path string, mode os.FileMode, made func(dir string, fd int) error) (int, *meshWalk, error) { + w := &meshWalk{want: filepath.Clean(path), create: true, mode: mode, made: made} + fd, err := w.open() + return fd, w, err +} + +// applyMeshDirectory gives a mesh directory outside every home its declared mode and owner through a descriptor +// (novox/hq issue 358): opened by openMeshDir, made there when missing, judged a directory by fstat on it, changed +// with fchmod and fchown on it, and read back from it. The parents it makes inside the owner's home are the owner's, +// as makeDirs gives them (novox/hq ADR 0182). +func applyMeshDirectory(r *declaration.Directory, out Outcome, mode os.FileMode, existed bool) (Outcome, error) { + uid, gid, chown := 0, 0, r.Owner != "" + var err error + if chown { + if uid, gid, err = idsOf(r.Owner); err != nil { + return out, fmt.Errorf("%s should belong to %q: %w", r.Path, r.Owner, err) + } + } + clean := filepath.Clean(r.Path) + home := "" + if chown { + if h, herr := homeOf(r.Owner); herr == nil && h != "" { + home = filepath.Clean(h) + } + } + giveMade := func(dir string, fd int) error { + if dir == clean || home == "" || (dir != home && !strings.HasPrefix(dir, home+string(os.PathSeparator))) { + return nil + } + if err := unix.Fchown(fd, uid, gid); err != nil { + return fmt.Errorf("cannot give %s to %q: %w", dir, r.Owner, &os.PathError{Op: "chown", Path: dir, Err: err}) + } + return nil + } + fd, walk, err := openMeshDir(clean, mode, giveMade) + if err != nil { + var link *LinkInPathError + if errors.As(err, &link) { + return out, &PlacementRefusedError{Path: r.Path, Why: "it " + link.why()} + } + return out, err + } + defer unix.Close(fd) + // Made by this walk when its last component is: at the path the walk reached it at, which root-only links in its + // path may have moved from the path declared. + created := len(walk.Made) > 0 && walk.Made[len(walk.Made)-1] == walk.At + + var st unix.Stat_t + if err := unix.Fstat(fd, &st); err != nil { + return out, &os.PathError{Op: "fstat", Path: r.Path, Err: err} + } + beforeMode, beforeUID, beforeGID := os.FileMode(st.Mode&0o777), int(st.Uid), int(st.Gid) + // Set explicitly even when it existed: a mode given at creation is not a mode maintained. + if err := unix.Fchmod(fd, uint32(mode.Perm())); err != nil { + return out, &os.PathError{Op: "chmod", Path: r.Path, Err: err} + } + ownedAlready := !chown || (beforeUID == uid && beforeGID == gid) + if !ownedAlready { + if err := unix.Fchown(fd, uid, gid); err != nil { + return out, fmt.Errorf("cannot give %s to %q: %w", r.Path, r.Owner, &os.PathError{Op: "chown", Path: r.Path, Err: err}) + } + } + + // Read back, from the descriptor that was changed. + if err := unix.Fstat(fd, &st); err != nil { + return out, fmt.Errorf("made %s and cannot stat it: %w", r.Path, err) + } + if st.Mode&unix.S_IFMT != unix.S_IFDIR { + return out, fmt.Errorf("%s is not a directory after applying", r.Path) + } + if got := os.FileMode(st.Mode & 0o777); got != mode.Perm() { + return out, fmt.Errorf("%s is mode %o after setting %o", r.Path, got, mode.Perm()) + } + + out.Action = "unchanged" + switch { + case created || !existed: + out.Action = "created" + case !ownedAlready: + out.Action = "updated" + case beforeMode != mode.Perm(): + out.Action = "updated" + out.Detail = fmt.Sprintf("mode %o to %o", beforeMode, mode.Perm()) + } + return out, nil +} diff --git a/internal/apply/mesh_dirs_gate_test.go b/internal/apply/mesh_dirs_gate_test.go new file mode 100644 index 0000000..dfb64de --- /dev/null +++ b/internal/apply/mesh_dirs_gate_test.go @@ -0,0 +1,95 @@ +//go:build linux + +package apply + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/store" +) + +// Review 47 of novox/hq issue 358: a directory the walk refuses gates its module, and nothing below it is written, +// whether the link was there when the apply began or appeared while it ran. + +// aLinkedModule is a module's directory at /parent/state, its parent a link to /elsewhere, and a file of +// the module below the directory; and another module's file elsewhere. +func aLinkedModule(t *testing.T) (target, other, decl string) { + t.Helper() + base := t.TempDir() + target = filepath.Join(base, "elsewhere", "state") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(base, "elsewhere"), filepath.Join(base, "parent")); err != nil { + t.Fatal(err) + } + dir := filepath.Join(base, "parent", "state") + other = filepath.Join(base, "other.conf") + decl = `{"id":"mod.state","type":"directory","path":"` + dir + `","mode":"0700"},` + + `{"id":"mod.conf","type":"file","path":"` + dir + `/settings.conf","content":"a=1\n"},` + + `{"id":"other.conf","type":"file","path":"` + other + `","content":"kept apart\n"}` + return target, other, decl +} + +func holdsTheGate(t *testing.T, target, other string, report Report, err error) { + t.Helper() + var refusal *PlacementRefusedError + if !errors.As(err, &refusal) { + t.Fatalf("the directory behind the link was not refused: %v", err) + } + if _, statErr := os.Stat(filepath.Join(target, "settings.conf")); statErr == nil { + t.Error("a file was written into the link's target, below the directory the engine refused") + } + if info, _ := os.Stat(target); info.Mode().Perm() != 0o755 { + t.Errorf("the link's target was given mode %o (was 755)", info.Mode().Perm()) + } + if o := outcomeOf(report, "mod.conf"); o.Action != "skipped" { + t.Errorf("the file below the refused directory is not reported skipped: %+v", o) + } + if strings.Contains(err.Error(), "everything was attempted") || !strings.Contains(err.Error(), "the rest of mod was not attempted") { + t.Errorf("the report does not say the module was gated: %v", err) + } + if got, readErr := os.ReadFile(other); readErr != nil || string(got) != "kept apart\n" { + t.Errorf("another module's file was not applied: %q, %v", got, readErr) + } +} + +// The link there when the apply begins: refused by the guard before anything is applied. +func TestADirectoryBehindALinkGatesItsModuleThroughApply(t *testing.T) { + target, other, decl := aLinkedModule(t) + report, _, err := Apply(context.Background(), archHost(t), declare(t, decl), store.State{}, store.OriginDeclared, + noServices, nil, nil) + holdsTheGate(t, target, other, report, err) +} + +// The link found only at the moment of use: refused by the walk during the apply, and still a refusal that gates its +// module and what is below it. Simulated by trusting / for the guard's walk (two walks of /: one to the link, one +// from its target) and not for the apply's. +func TestADirectoryRefusedDuringTheApplyGatesItsModule(t *testing.T) { + target, other, decl := aLinkedModule(t) + wasDirs, wasLinks := couldReplace, linkReplacer + walksOfRoot := 0 + couldReplace = func(path string, _, _, _ uint32) string { + if path == "/" { + walksOfRoot++ + if walksOfRoot > 2 { + return "every account (a test's /)" + } + } + return "" + } + linkReplacer = func(string, uint32) string { return "" } + t.Cleanup(func() { couldReplace, linkReplacer = wasDirs, wasLinks }) + + report, _, err := Apply(context.Background(), archHost(t), declare(t, decl), store.State{}, store.OriginDeclared, + noServices, nil, nil) + if walksOfRoot < 3 { + t.Fatalf("the apply did not walk again at the moment of use (%d walks)", walksOfRoot) + } + holdsTheGate(t, target, other, report, err) +} diff --git a/internal/apply/mesh_dirs_test.go b/internal/apply/mesh_dirs_test.go new file mode 100644 index 0000000..b84abea --- /dev/null +++ b/internal/apply/mesh_dirs_test.go @@ -0,0 +1,91 @@ +//go:build linux + +package apply + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends novox/hq issue 358: a mesh directory outside every home is given its mode and owner only through a +// descriptor opened from /, so a parent swapped for a link between two applies cannot carry root's chmod and +// chown to the link's target — unless root alone could have made that link (/var/run is one on every node). + +// A link only root could have made — itself root's, in directories no other account may write — is followed, as +// /var/run to /run is on every node; relative or absolute. +func TestALinkOnlyRootCouldMakeIsFollowed(t *testing.T) { + for name, to := range map[string]func(base string) string{ + "absolute": func(base string) string { return filepath.Join(base, "elsewhere") }, + "relative": func(string) string { return "elsewhere" }, + } { + t.Run(name, func(t *testing.T) { + rootAlone(t, "") + base, r, previous := aMeshDirectory(t) + target := swapParentForLink(t, base, to(base)) + + out, err := applyDirectory(r, previous, true) + if err != nil { + t.Fatalf("a directory behind a root-only link was refused: %v", err) + } + if got := permOf(t, target); got != 0o700 { + t.Errorf("the directory behind a root-only link is mode %o, want 700", got) + } + if out.Action != "updated" { + t.Errorf("the apply said %q, want updated", out.Action) + } + }) + } +} + +// A link another account owns refuses the directory even in directories root alone may write, naming the account. +func TestALinkAnotherAccountOwnsIsRefused(t *testing.T) { + rootAlone(t, "the account mallory (it owns the link)") + base, r, previous := aMeshDirectory(t) + target := swapParentForLink(t, base, filepath.Join(base, "elsewhere")) + + _, err := applyDirectory(r, previous, true) + if err == nil || !strings.Contains(err.Error(), "mallory") { + t.Fatalf("a link another account owns was not refused naming it: %v", err) + } + if got := permOf(t, target); got != 0o755 { + t.Errorf("the link's target was given mode %o (was 755)", got) + } +} + +// A directory that is missing is made through the walk, below a link root alone could make. +func TestAMissingDirectoryIsMadeBehindARootOnlyLink(t *testing.T) { + rootAlone(t, "") + base := t.TempDir() + if err := os.Mkdir(filepath.Join(base, "real"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("real", filepath.Join(base, "link")); err != nil { + t.Fatal(err) + } + r := &declaration.Directory{ID: "mod.state", Path: filepath.Join(base, "link", "a", "b"), Mode: "0750"} + out, err := applyDirectory(r, store.Applied{}, false) + if err != nil { + t.Fatal(err) + } + if out.Action != "created" { + t.Errorf("the apply said %q, want created", out.Action) + } + if got := permOf(t, filepath.Join(base, "real", "a", "b")); got != 0o750 { + t.Errorf("the made directory is mode %o, want 750", got) + } +} + +// rootAlone has every directory count as writable by root alone, as /, /var and /run are on a node, and a link's +// owner be linkOwner ("" for root): a test runs below /tmp, which every account may write. +func rootAlone(t *testing.T, linkOwner string) { + t.Helper() + wasDirs, wasLinks := couldReplace, linkReplacer + couldReplace = func(string, uint32, uint32, uint32) string { return "" } + linkReplacer = func(string, uint32) string { return linkOwner } + t.Cleanup(func() { couldReplace, linkReplacer = wasDirs, wasLinks }) +} diff --git a/internal/apply/placement_guard.go b/internal/apply/placement_guard.go index 01112d5..e23ef45 100644 --- a/internal/apply/placement_guard.go +++ b/internal/apply/placement_guard.go @@ -523,6 +523,20 @@ func refusedPlaces(resources []declaration.Resource) map[string]error { switch res := r.(type) { case *declaration.Directory: err = refusePath(res.Path, placingDirectory, moduleOfID(res.ID), res.Owner) + // And outside every home, the walk's judgement before anything is applied (novox/hq issue 358): a + // link another account could make in its path refuses it, and with it what is below it, whatever + // order they are declared in. The apply walks again at the moment of use. A directory handed over + // keeps its own, stricter walk (issue 356), which refuses any link at all. + if err == nil && meshDirsByDescriptor && homeAbove(res.Path) == "" && + !HandedOver()[handOverKey(res.ID, res.Path)] { + if judged := judgeMeshDir(res.Path); judged != nil { + err = judged + var link *LinkInPathError + if errors.As(judged, &link) { + err = &PlacementRefusedError{Path: res.Path, Why: "it " + link.why()} + } + } + } case *declaration.Access: err = refusePath(res.Path, placingAccess, moduleOfID(res.ID), "") default: diff --git a/internal/apply/replay358_test.go b/internal/apply/replay358_test.go new file mode 100644 index 0000000..4f3eef7 --- /dev/null +++ b/internal/apply/replay358_test.go @@ -0,0 +1,83 @@ +//go:build linux + +package apply + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// The replay of novox/hq issue 358, written only with what the node-engine had before its fix, so it can be laid +// over the commit before (mesh-lab's replays register): a mesh directory outside every home was given its mode +// and owner by path, so a parent swapped for a link between two applies carried root's chmod to the link's target. + +// aMeshDirectory applies a directory declared at /parent/state once, as the mesh's own, and answers it +// with the record a second apply would read. +func aMeshDirectory(t *testing.T) (base string, r *declaration.Directory, previous store.Applied) { + t.Helper() + base = t.TempDir() + r = &declaration.Directory{ID: "mod.state", Path: filepath.Join(base, "parent", "state"), Mode: "0700"} + out, err := applyDirectory(r, store.Applied{}, false) + if err != nil { + t.Fatalf("the first apply: %v", err) + } + if out.Action != "created" { + t.Fatalf("the first apply said %q, want created", out.Action) + } + return base, r, store.Applied{ID: r.ID, Type: "directory", Target: r.Path} +} + +// swapParentForLink moves /parent away and puts a link to elsewhere (as written) in its place, where +// elsewhere holds a directory state of mode 0755: what an account that can write does between two applies. +func swapParentForLink(t *testing.T, base, to string) (target string) { + t.Helper() + elsewhere := filepath.Join(base, "elsewhere") + target = filepath.Join(elsewhere, "state") + if err := os.MkdirAll(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(target, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(base, "parent"), filepath.Join(base, "moved")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(to, filepath.Join(base, "parent")); err != nil { + t.Fatal(err) + } + return target +} + +func permOf(t *testing.T, path string) os.FileMode { + t.Helper() + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + return info.Mode().Perm() +} + +// The acceptance of issue 358: a parent swapped for a link after an apply leaves the link's target untouched, and +// the refusal names the link and who could have made it. +func TestAParentSwappedForALinkLeavesItsTargetUntouched(t *testing.T) { + base, r, previous := aMeshDirectory(t) + target := swapParentForLink(t, base, filepath.Join(base, "elsewhere")) + + _, err := applyDirectory(r, previous, true) + var refusal *PlacementRefusedError + if !errors.As(err, &refusal) { + t.Fatalf("the directory reached through a link was not refused: %v", err) + } + if !strings.Contains(err.Error(), filepath.Join(base, "parent")) || !strings.Contains(err.Error(), "could replace") { + t.Errorf("the refusal does not name the link and who could replace it: %v", err) + } + if got := permOf(t, target); got != 0o755 { + t.Errorf("the link's target was given mode %o (was 755): root's chmod followed the link", got) + } +}