From 41b20b27823fd75238e55c49e0ed411e19eb5277 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 12:21:49 +0200 Subject: [PATCH 1/2] A grant secret belongs to whoever provisions, and the sweep skips what it will not address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue 225. The mesh seals one credential per consumer beside the provider's contributions file, and wrote it root-owned. That was right while a module's own code ran in a container as root; ADR 0198 moved that code under the node's runtime, as the node's account, and the secret stayed root's. On the control machine two consumers went unprovisioned for three hours and the only sign was a line reading 'secret not readable yet', 4330 times. The same sentence is already written for a module's own secrets a few hundred lines above — 'a root-owned 0600 file is one that process cannot read'. This is that rule reaching the other kind of secret the mesh writes for a module. Issue 226. The sweep met a reference recorded with the store's old address, read 'I will not address this' as 'the store refuses everything', and collected none of the 1681 it had found. Two changes: references from build records are read through Recorded, where the provenance is known — not in LetGo, which cannot tell one registry host from another and must stay strict — and a reference the sweep will not address is now ErrNotOurs, skipped, never a reason to stop. Only the store refusing ends a sweep. make check: the two failures both fail on main as well — the resolver test (hq 202/203) and the service-manager test, which reads this machine's own shell environment. --- cmd/mesh-controller/collect.go | 25 ++++- internal/artifacts/store.go | 22 +++- internal/artifacts/store_test.go | 23 ++++ internal/catalogue/declaration.go | 26 ++++- internal/catalogue/grant_secret_owner_test.go | 103 ++++++++++++++++++ internal/inventory/collection.go | 38 +++++-- internal/inventory/collection_test.go | 45 ++++++++ 7 files changed, 263 insertions(+), 19 deletions(-) create mode 100644 internal/catalogue/grant_secret_owner_test.go diff --git a/cmd/mesh-controller/collect.go b/cmd/mesh-controller/collect.go index 5464800..6d01e2a 100644 --- a/cmd/mesh-controller/collect.go +++ b/cmd/mesh-controller/collect.go @@ -60,7 +60,7 @@ func collect(ctx context.Context, inv *inventory.Inventory) { store := artifacts.Store{Address: address} var done []string - var left int + var left, skipped int for i, reference := range references { if i >= mostPerSweep || within.Err() != nil { left = len(references) - i @@ -73,10 +73,22 @@ func collect(ctx context.Context, inv *inventory.Inventory) { done = append(done, reference) continue } - // **Stopped at the first refusal, not pushed through.** A store that refuses one refuses - // all of them — deletion disabled, the store down, the network gone — so going on would - // be a hundred identical failures and a hundred identical log lines in front of whoever - // was building something. + if errors.Is(err, artifacts.ErrNotOurs) { + // **A fact about this record, so this record is skipped** (novox/hq issue 226). Not + // marked collected — the mesh did not remove it and should not claim to — and not a + // reason to stop, because the store was never asked. One of these at the front of + // the oldest-first order ended every sweep until this. + skipped++ + if skipped == 1 { + fmt.Fprintf(os.Stderr, + "the sweep will not address %s and went on: %v\n", reference, err) + } + continue + } + // **Stopped at the first refusal by the STORE, not pushed through.** A store that refuses + // one refuses all of them — deletion disabled, the store down, the network gone — so + // going on would be a hundred identical failures and a hundred identical log lines in + // front of whoever was building something. fmt.Fprintf(os.Stderr, "the artifact store kept %s, so nothing more was asked of it: %v\n", reference, err) left = len(references) - i @@ -97,6 +109,9 @@ func collect(ctx context.Context, inv *inventory.Inventory) { if left > 0 { fmt.Fprintf(os.Stderr, "%d more to collect; the next build asks again\n", left) } + if skipped > 0 { + fmt.Fprintf(os.Stderr, "%d artifact(s) the sweep will not address were skipped\n", skipped) + } } // mostPerSweep is how many artifacts one sweep will ask about. Enough that a mesh building diff --git a/internal/artifacts/store.go b/internal/artifacts/store.go index 02b79d8..ca80e65 100644 --- a/internal/artifacts/store.go +++ b/internal/artifacts/store.go @@ -8,6 +8,7 @@ package artifacts import ( "context" + "errors" "fmt" "net/http" "strings" @@ -26,7 +27,15 @@ type Store struct { } // Gone is the answer when the store does not hold it: the outcome wanted, already true. -var Gone = fmt.Errorf("the store does not hold it") +var Gone = errors.New("the store does not hold it") + +// ErrNotOurs is a reference this sweep will not address: not the mesh's own, or naming nothing +// the store holds by digest. +// +// **A fact about the record, not about the store** (novox/hq issue 226). The two deserve opposite +// responses — skip one and go on, abandon the sweep for the other — and collapsing them into "an +// error" is how a cautious loop became one that did nothing while reporting the right number. +var ErrNotOurs = errors.New("not a reference into the mesh's artifact store") // LetGo asks the store to drop one artifact the mesh recorded making. // @@ -38,12 +47,17 @@ var Gone = fmt.Errorf("the store does not hold it") // wants the artifact absent, and it is. It is distinguished from success only so a caller can say // which of the two happened. func (s Store) LetGo(ctx context.Context, reference string) error { + // **Strict, and deliberately** (novox/hq issue 226). Only a reference the mesh keeps in its + // own vocabulary is addressed here. `Recorded` would read `docker.io/library/registry@sha256:…` + // as the mesh's too — it cannot tell one registry host from another — so normalising belongs + // where the provenance is known, which is the sweep reading its own build records, not here + // where the only job is to refuse anything that is not plainly ours. path, kept := catalogue.InArtifactStore(reference) if !kept { // Nothing the mesh put in its own store. Refused rather than attempted: composing a // delete for a reference of unknown shape is how a sweep reaches something that is not - // the mesh's. - return fmt.Errorf("%s is not a reference into the mesh's artifact store", reference) + // the mesh's. Distinguished from a store that refuses, so a sweep skips this and goes on. + return fmt.Errorf("%w: %s", ErrNotOurs, reference) } if s.Address == "" { return fmt.Errorf("this mesh has no artifact store on its network to ask about %s", reference) @@ -95,5 +109,5 @@ func split(path string) (repository, kind, digest string, err error) { if before, after, ok := strings.Cut(path, "/blobs/sha256:"); ok { return before, "blobs", "sha256:" + after, nil } - return "", "", "", fmt.Errorf("%q names nothing the store holds by digest", path) + return "", "", "", fmt.Errorf("%w: %q names nothing the store holds by digest", ErrNotOurs, path) } diff --git a/internal/artifacts/store_test.go b/internal/artifacts/store_test.go index 3b433c8..ee09086 100644 --- a/internal/artifacts/store_test.go +++ b/internal/artifacts/store_test.go @@ -92,3 +92,26 @@ func TestAReferenceThatIsNotTheMeshsOwnIsNeverAsked(t *testing.T) { t.Fatalf("the store was asked about %v", *asked) } } + +// A reference this sweep will not address says so as ErrNotOurs, which is a fact about the +// record and not about the store (novox/hq issue 226). +// +// The sweep skips one and abandons itself for the other, so they cannot be the same error. The +// first live run met a reference recorded with the store's old address, read the refusal as "the +// store refuses everything", and collected none of the 1681 it had found. +func TestAReferenceThisSweepWillNotAddressIsToldApartFromAStoreRefusing(t *testing.T) { + store, asked := fakeStore(t, http.StatusAccepted) + for _, reference := range []string{ + "docker.io/library/registry@sha256:abc123", + "127.0.0.1:5100/mesh-tools/build@sha256:abc123", + "1.4.2", + } { + err := store.LetGo(context.Background(), reference) + if !errors.Is(err, ErrNotOurs) { + t.Errorf("%s answered %v; a sweep must be able to skip it and go on", reference, err) + } + } + if len(*asked) != 0 { + t.Fatalf("the store was asked about %v", *asked) + } +} diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 473f8fe..6303bcb 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -610,14 +610,14 @@ func (r Resolution) compose(with Rendering, owner map[string]string, // and nothing would say so. continue } - first = append(first, map[string]any{ + first = append(first, ownedBy(r.provisionsAs(m), map[string]any{ // One file per holder — the consumer's module with its local name after it // where it keeps several (ADR 0094); the lab found two files with one id. "id": GrantID(to, g.Consumer+"."+holderAs(g.From, g.Local)), "type": "file", "path": grantPath(m.Grants[to], g.Consumer, holderAs(g.From, g.Local)), "sealed": g.Sealed, - }) + })) } } for _, to := range sortedKeys(m.Binds) { @@ -1240,6 +1240,28 @@ type Contribution struct { Derived map[string]any `json:"derived,omitempty"` } +// provisionsAs is the account that reads what the mesh writes for this provider: the one secret +// per consumer it must open to set that consumer's password (novox/hq issue 225). +// +// **A root-owned 0600 file is one that process cannot read**, which is the same sentence already +// written above for a module's own secrets — and the grant secret is the other kind of secret +// the mesh writes for a module, so it is the same rule. +// +// Which account depends on where the module's code runs. A module whose code is a bundle is run +// by the node's tool runtime, as the node's account ([ADR 0198](0198)); one still in a container +// is whatever it declares as its secrets owner. Nothing names these paths, so the rule that +// claims a bundle's other files by the words that name them (givenTo) cannot reach them: the +// harness composes a grant secret's path from the contributions file, not from a word. +// +// Empty is root, which is what it was and what a module with no bundle and no declared owner +// still wants. +func (r Resolution) provisionsAs(m Manifest) string { + if len(m.Bundles) > 0 && r.Account != "" { + return r.Account + } + return m.SecretsOwner +} + // grantPath is where one consumer's sealed credential lands on the providing machine. // // Suffixed, so the directory can also hold whatever the module writing it keeps there and so a diff --git a/internal/catalogue/grant_secret_owner_test.go b/internal/catalogue/grant_secret_owner_test.go new file mode 100644 index 0000000..5fdf7fe --- /dev/null +++ b/internal/catalogue/grant_secret_owner_test.go @@ -0,0 +1,103 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// A grant secret is read by whatever provisions, and that stopped being root (novox/hq issue 225). +// +// The mesh seals one credential per consumer beside the provider's contributions file. The +// provider's harness reads both: the file to learn who asked, the secret to set their password. +// While a module's own code ran in a container as root, a root-owned 0600 file was readable by +// the thing that needed it. ADR 0198 moved that code under the node's runtime, which runs as the +// operator's account — and the secret stayed root's. +// +// **The cost was silence.** The harness says `secret not readable yet`, which is true and +// ordinary on the first pass, so four thousand refusals in three hours read as patience. No user +// was ever created, and two consumers crash-looped against a database that had never heard of +// them. +// +// The same reasoning is already written for a module's *own* secrets, three hundred lines above: +// "a root-owned 0600 file is one that process cannot read". This is that rule reaching the other +// kind of secret the mesh writes for a module. + +// aProviderWithABundle is a provider whose code is a bundle the node's runtime runs — the shape +// every TypeScript provisioner has since ADR 0198. +func aProviderWithABundle() Manifest { + return Manifest{ + Module: "mongodb", Version: "1", + Provides: FromAnywhere("mongodb-database"), + Receives: map[string]string{"mongodb-database": "/var/lib/mongodb/grants/mesh.json"}, + Grants: map[string]string{"mongodb-database": "/var/lib/mongodb/grants"}, + Bundles: []Bundle{{Name: "code", Language: "typescript"}}, + Resources: []map[string]any{{ + "id": "server", "type": "container", "name": "mongodb-server", + "image": "mongo@sha256:" + strings.Repeat("a", 64), + }}, + } +} + +func TestAGrantSecretIsOwnedByTheAccountThatProvisions(t *testing.T) { + r, err := Resolve(shelf(aProviderWithABundle()), []string{"mongodb"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + r.Account = "operator" + out, err := r.Declaration(Rendering{Grants: []Grant{{ + Provision: "mongodb-database", Consumer: "workstation", From: "photos", Slug: "photos", + Values: map[string]any{}, Sealed: "c2VhbGVk", + }}}) + if err != nil { + t.Fatal(err) + } + + var secret map[string]any + for _, res := range out { + if res["type"] == "file" && strings.HasSuffix(fmtPath(res), ".secret") { + secret = res + } + } + if secret == nil { + t.Fatalf("no grant secret was composed at all: %v", out) + } + if got := secret["owner"]; got != "operator" { + t.Fatalf("the grant secret at %v belongs to %v; the provisioner runs as %q and a "+ + "root-owned 0600 file is one it cannot read — which is silent, because the harness "+ + "calls it \"not readable yet\"", fmtPath(secret), got, "operator") + } +} + +// And a provider whose code still runs in a container keeps the owner it declares, so this +// changes nothing for the modules the runtime has not taken. +func TestAContainerProvidersGrantSecretKeepsItsDeclaredOwner(t *testing.T) { + m := aProviderWithABundle() + m.Bundles = nil + m.SecretsOwner = "65534:65534" + r, err := Resolve(shelf(m), []string{"mongodb"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + r.Account = "operator" + out, err := r.Declaration(Rendering{Grants: []Grant{{ + Provision: "mongodb-database", Consumer: "workstation", From: "photos", Slug: "photos", + Values: map[string]any{}, Sealed: "c2VhbGVk", + }}}) + if err != nil { + t.Fatal(err) + } + for _, res := range out { + if res["type"] == "file" && strings.HasSuffix(fmtPath(res), ".secret") { + if got := res["owner"]; got != "65534:65534" { + t.Fatalf("a container provider's grant secret belongs to %v, not what it declares", got) + } + return + } + } + t.Fatal("no grant secret was composed") +} + +func fmtPath(r map[string]any) string { + p, _ := r["path"].(string) + return p +} diff --git a/internal/inventory/collection.go b/internal/inventory/collection.go index 4b5bf67..3f6cde4 100644 --- a/internal/inventory/collection.go +++ b/internal/inventory/collection.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "strings" + + "github.com/novox/mesh-controller/internal/catalogue" ) // What the artifact store keeps, and what it may let go (novox/hq ADR 0189, issue 108). @@ -71,11 +73,12 @@ func (i *Inventory) ToCollect(ctx context.Context) ([]string, error) { continue } for _, a := range made { - if a.Reference == "" || keep[a.Reference] || collected[a.Reference] || seen[a.Reference] { + reference := asRecorded(a.Reference) + if reference == "" || keep[reference] || collected[reference] || seen[reference] { continue } - seen[a.Reference] = true - out = append(out, a.Reference) + seen[reference] = true + out = append(out, reference) } } return out, rows.Err() @@ -127,8 +130,8 @@ func (i *Inventory) keptReferences(ctx context.Context) (map[string]bool, error) continue } for _, a := range made { - if a.Reference != "" { - keep[a.Reference] = true + if reference := asRecorded(a.Reference); reference != "" { + keep[reference] = true } } } @@ -186,16 +189,35 @@ func (i *Inventory) everyReferenceMade(ctx context.Context) ([]string, error) { continue } for _, a := range made { - if a.Reference == "" || seen[a.Reference] { + reference := asRecorded(a.Reference) + if reference == "" || seen[reference] { continue } - seen[a.Reference] = true - out = append(out, a.Reference) + seen[reference] = true + out = append(out, reference) } } return out, rows.Err() } +// asRecorded is an artifact reference in the one vocabulary the sweep speaks (novox/hq issue 226). +// +// **Every reference here came from a build record, so every one of them is the mesh's own.** That +// is what makes it safe to normalise: references kept before the store's address stopped being +// written are `:/@sha256:…` (04-ISSUES/102), and `Recorded` reads those as the +// `artifact-store://` references the rest of the mesh uses. Done here rather than when the store +// is asked, because `Recorded` cannot tell one registry host from another — only the provenance +// can, and the provenance is here. +// +// The oldest artifacts are exactly the ones recorded the old way, and exactly the ones a +// sweep reaches first. Untranslated, the first of them ended every sweep. +func asRecorded(reference string) string { + if reference == "" { + return "" + } + return catalogue.Recorded(reference) +} + // digestIn is the `sha256:` a reference names, empty when it names none. func digestIn(reference string) string { for _, marker := range []string{"@sha256:", "/sha256:"} { diff --git a/internal/inventory/collection_test.go b/internal/inventory/collection_test.go index b28c3ec..c7aaf77 100644 --- a/internal/inventory/collection_test.go +++ b/internal/inventory/collection_test.go @@ -145,3 +145,48 @@ func TestAFailedBuildNamesNothingToCollectAndEachModuleIsCountedOnItsOwn(t *test t.Fatalf("offered %v; want only web's oldest — db's three are all within its five", go_) } } + +// An artifact recorded with the store's old address is offered for collection, in the vocabulary +// the rest of the mesh speaks (novox/hq issue 226). +// +// Before references were kept without an address the mesh recorded +// `:/@sha256:…` (04-ISSUES/102). Those are the oldest artifacts, which makes +// them exactly the ones an oldest-first sweep reaches first — and the first live run met one, +// read "I will not address this" as "the store refuses everything", and collected none of 1681. +func TestAnArtifactRecordedWithAnAddressIsOfferedAsTheMeshRecordsOne(t *testing.T) { + inv := fresh(t) + ctx := context.Background() + + // The oldest build published the old way; five newer ones fill the module's five. + old := aBuild("a00", "tools", "") + old.Made = []Artifact{{Name: "build", Kind: "image", + Reference: "127.0.0.1:5100/tools/build@sha256:" + fmt.Sprintf("%064x", 1)}} + if err := inv.RecordBuild(ctx, old); err != nil { + t.Fatal(err) + } + for i := 2; i <= 6; i++ { + built(t, inv, fmt.Sprintf("a%02d", i), "tools", i) + } + + go_, err := inv.ToCollect(ctx) + if err != nil { + t.Fatal(err) + } + want := ref("tools", "build", 1) + if len(go_) != 1 || go_[0] != want { + t.Fatalf("offered %v; want %q — the address is a route to the artifact, not part of its "+ + "name, and the sweep speaks the name", go_, want) + } + // And marking it collected uses that same name, so the next sweep does not offer it again + // under a spelling it has not seen. + if err := inv.MarkCollected(ctx, go_); err != nil { + t.Fatal(err) + } + again, err := inv.ToCollect(ctx) + if err != nil { + t.Fatal(err) + } + if len(again) != 0 { + t.Fatalf("offered %v again after collecting it", again) + } +} From 74b0dab34c7d4fdbf2b1d62af6517636d8d57b5a Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 12:25:27 +0200 Subject: [PATCH 2/2] A container publishes only a port its module declares (hq issue 227) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The short form is a question the mesh answers: "80" means publish what the software calls 80, and the mesh fills in the machine's half from the port it assigned. It can only assign one for a port the module declared, so a number appearing nowhere in listens gets no assignment and reaches the machine as written — which is how the photo module asked for port 80 on the node whose reverse proxy holds it. Four modules publish 80 quite safely, because they declare 80. The difference is the declaration, not the number. A catalogue-wide test now says so; it names all three offenders against the catalogue as it was. --- internal/catalogue/published_ports_test.go | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 internal/catalogue/published_ports_test.go diff --git a/internal/catalogue/published_ports_test.go b/internal/catalogue/published_ports_test.go new file mode 100644 index 0000000..48036d2 --- /dev/null +++ b/internal/catalogue/published_ports_test.go @@ -0,0 +1,75 @@ +package catalogue + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// A container publishes only a port its module declares (novox/hq issue 227). +// +// **The short form is a question the mesh answers.** `"80"` means *publish what the software +// calls 80*, and the mesh fills in the machine's half from the port it assigned +// ([ADR 0038](0038)). It can only assign one for a port the module declared in `listens` — so a +// container publishing a number that appears nowhere in `listens` gets no assignment, and +// `publishedOn` falls back to the number as written. It escapes to the machine. +// +// That is how the photo module asked for port 80 on the control node, where the reverse proxy +// holds it: it declared its web endpoint at 4001, published a bare 80, and the container never +// started. Four other modules publish 80 quite safely — because they declare 80, so the mesh +// gives them a machine port for it. The difference is the declaration, not the number. +// +// A mapping written the long way is a module pinning both halves on purpose and is left alone. +func TestEveryPublishedPortIsOneItsModuleDeclares(t *testing.T) { + root := catalogueRoot(t) + entries, err := os.ReadDir(filepath.Join(root, "modules")) + if err != nil { + t.Fatal(err) + } + var escaped []string + for _, entry := range entries { + if !entry.IsDir() { + continue + } + raw, err := os.ReadFile(filepath.Join(root, "modules", entry.Name(), "module.json")) + if err != nil { + continue + } + m, err := ParseManifest(raw) + if err != nil { + // Whether every manifest parses is TestEveryCatalogueManifestParses's question. + continue + } + declared := map[int]bool{} + for _, l := range m.Listens { + declared[l.Port] = true + } + for _, r := range m.Resources { + if fmt.Sprint(r["type"]) != "container" { + continue + } + listed, _ := r["ports"].([]any) + for _, p := range listed { + written := strings.Split(fmt.Sprint(p), "/")[0] + if strings.Contains(written, ":") { + continue // pinned by hand, both halves, on purpose + } + port, err := strconv.Atoi(strings.TrimSpace(written)) + if err != nil || declared[port] { + continue + } + escaped = append(escaped, fmt.Sprintf( + "%s's %v publishes %d, and %s declares no such port — the mesh has nothing "+ + "to assign, so %d reaches the machine as written", + m.Module, r["id"], port, m.Module, port)) + } + } + } + if len(escaped) > 0 { + t.Fatalf("a container may publish only a port its module declares:\n - %s", + strings.Join(escaped, "\n - ")) + } +}