From 53eb000a84fb8886402f5fc51cdcc85e69c8fed5 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 1 Sep 2026 19:36:01 +0200 Subject: [PATCH] A container may not mount a path the module never declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the half of 04-ISSUES/026 that would otherwise come back. The fourteen mounts across the forge, the mail system, the store and the object store are all declared now — but nothing said they had to be, so they were right by coincidence and the next volume added would not be. A bind mount whose source does not exist is created by the container runtime, as root, with a mode it picks. So `owner` and `mode` — which exist precisely so a module can say who its data belongs to — were silently not applied to the only directories holding data. And the rule written for exactly this case did not reach them. A directory the mesh declared and no longer wants is kept, not removed, when it holds anything the mesh did not put there (ADR 0030). That is the answer to *what happens to my data when a module goes away*, and it is written in terms of declared directories: an undeclared one sits outside it, because the mesh does not know it is there. Refused where it is written rather than on the machine, which cannot tell the difference — by the time the host sees the mount it is being asked to make a directory, which it is perfectly able to do. The fault is in the manifest, so it is named at the manifest. Same argument as the action refusal directly above it. A path under a declared directory counts as declared, as do the files a module already names: its own secrets, its grants, what it receives. Every real manifest is checked to still parse, and the refusal bites. --- internal/catalogue/machineside_test.go | 37 ++++++++++++- internal/catalogue/manifest.go | 72 ++++++++++++++++++++++++++ internal/catalogue/parseall_test.go | 23 ++++++++ 3 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 internal/catalogue/parseall_test.go diff --git a/internal/catalogue/machineside_test.go b/internal/catalogue/machineside_test.go index 553e1a5..e0c1694 100644 --- a/internal/catalogue/machineside_test.go +++ b/internal/catalogue/machineside_test.go @@ -1,6 +1,9 @@ package catalogue -import "testing" +import ( + "strings" + "testing" +) // A module with nothing that publishes binds what it binds, and the mesh may not move it. // @@ -58,3 +61,35 @@ func TestAPortNotInTheMappingIsNotFound(t *testing.T) { at, mayAssign) } } + +// A bind mount the module never declared is refused where it is written. +// +// The container runtime creates a missing bind source itself, as root, with a mode it picks. So +// the module's own owner and mode never reach the directory holding its data, and the rule that +// keeps data when a module goes away (novox/hq ADR 0030) does not cover it — that rule is written +// in terms of declared directories, and the mesh has never heard of this one. +func TestAMountNothingDeclaresIsRefused(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"store","resources":[` + + `{"id":"server","type":"container","name":"store","image":"x@sha256:` + + `0000000000000000000000000000000000000000000000000000000000000000",` + + `"volumes":["/services/store/data:/var/lib/data"]}]}`)) + if err == nil { + t.Fatal("a container mounting a path no resource declares was accepted; the runtime " + + "would create it as root and the module's owner and mode would never apply") + } + if !strings.Contains(err.Error(), "/services/store/data") { + t.Fatalf("refused without naming the path, which leaves the author guessing: %v", err) + } +} + +// And declaring it is enough — including declaring the directory above it. +func TestAMountUnderADeclaredDirectoryIsAccepted(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"store","resources":[` + + `{"id":"state","type":"directory","path":"/services/store","mode":"0700"},` + + `{"id":"server","type":"container","name":"store","image":"x@sha256:` + + `0000000000000000000000000000000000000000000000000000000000000000",` + + `"volumes":["/services/store/data:/var/lib/data"]}]}`)) + if err != nil { + t.Fatalf("a module that said where its data lives was refused anyway: %v", err) + } +} diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 910a09f..0e5c054 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -606,6 +606,78 @@ func ParseManifest(raw []byte) (Manifest, error) { "program that reads what the mesh delivered and reconciles", m.Module, r["id"])) } + // **A container may not mount a path the module never declared** (novox/hq 04-ISSUES/026). + // + // A bind mount whose source does not exist is created by the container runtime, as root, with + // whatever mode it picks. So `owner` and `mode` — which exist precisely so a module can say who + // its data belongs to — are silently not applied to the only directories that hold data. + // + // And the protection written for exactly this case does not reach them. A directory the mesh + // declared and no longer wants is kept, not removed, when it holds anything the mesh did not + // put there (novox/hq ADR 0030). That rule is the answer to *what happens to my data when a + // module goes away*, and it is written in terms of declared directories. An undeclared one is + // outside it, because the mesh does not know it is there. + // + // Checked here rather than on the machine because the machine cannot tell the difference: by + // the time it sees the mount it is being asked to create the directory, which is a thing it is + // perfectly able to do. The fault is in the manifest, so it is named at the manifest. + declared := map[string]bool{} + for _, r := range m.Resources { + switch fmt.Sprint(r["type"]) { + case "directory", "file": + if p := fmt.Sprint(r["path"]); strings.HasPrefix(p, "/") { + declared[strings.TrimRight(p, "/")] = true + } + } + } + // A secret the mesh seals onto the machine is a file this module owns; it is declared by + // `own-secrets` rather than by a resource, and mounting it is the ordinary way a container + // reads it. + for _, where := range m.OwnSecrets { + declared[strings.TrimRight(where, "/")] = true + } + for _, where := range m.Secrets { + declared[strings.TrimRight(where, "/")] = true + } + for _, where := range m.Receives { + declared[strings.TrimRight(where, "/")] = true + } + for _, where := range m.Grants { + declared[strings.TrimRight(where, "/")] = true + } + // Under a declared directory is declared: a module that says where its data lives has said so + // for what it puts inside. + covers := func(path string) bool { + for at := path; strings.HasPrefix(at, "/"); { + if declared[at] { + return true + } + cut := strings.LastIndex(at, "/") + if cut <= 0 { + return false + } + at = at[:cut] + } + return false + } + for _, r := range m.Resources { + if fmt.Sprint(r["type"]) != "container" { + continue + } + mounts, _ := r["volumes"].([]any) + for _, v := range mounts { + from, _, _ := strings.Cut(fmt.Sprint(v), ":") + if !strings.HasPrefix(from, "/") || covers(strings.TrimRight(from, "/")) { + continue + } + problems = append(problems, fmt.Sprintf( + "%s mounts %q into %v, and nothing in %s declares it: a bind mount the module did "+ + "not declare is created by the container runtime as root, so the module's own "+ + "owner and mode do not reach the directory that holds its data, and the rule "+ + "that keeps data when a module goes away (novox/hq ADR 0030) does not cover it", + m.Module, from, r["id"], m.Module)) + } + } for name, where := range m.OwnSecrets { if !strings.HasPrefix(where, "/") { problems = append(problems, fmt.Sprintf( diff --git a/internal/catalogue/parseall_test.go b/internal/catalogue/parseall_test.go new file mode 100644 index 0000000..3885307 --- /dev/null +++ b/internal/catalogue/parseall_test.go @@ -0,0 +1,23 @@ +package catalogue + +import ( + "os" + "path/filepath" + "testing" +) + +func TestEveryExampleManifestStillParses(t *testing.T) { + found, err := filepath.Glob("../../examples/modules/*.json") + if err != nil || len(found) == 0 { + t.Fatalf("no example manifests found: %v", err) + } + for _, f := range found { + raw, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + if _, err := ParseManifest(raw); err != nil { + t.Errorf("%s no longer parses: %v", filepath.Base(f), err) + } + } +}