Merge pull request 'Give a mesh directory its mode and owner only through a descriptor (issue 358)' (#87) from fix/358-descriptor-only-directory-mode into main

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

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"}
+}

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
+}

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 <base>/parent/state, its parent a link to <base>/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)
+}

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 })
+}

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:

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 <base>/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 <base>/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 <base> 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)
+	}
+}