From 24347ea5a02b53a9b2a7f44bcf4ca503ff23a1ca Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 17:08:55 +0200 Subject: [PATCH] Step back from a rendered resolver file only, and compare a declared path cleaned (issue 412, review 80) A resource on the resolver file was left out by the check and still composed by the declaration, so an uplink declaring it as a resource beside the resolver passed with two writers; it is refused again, as before. And /etc//hosts or /etc/../etc/hosts is /etc/hosts to the machine, so it is compared cleaned. A seat names no root or top-level directory. --- internal/catalogue/node_resolver.go | 21 ++++----- internal/catalogue/seat_paths.go | 12 ++++- internal/catalogue/seat_paths_test.go | 67 ++++++++++++++++++++++----- 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/internal/catalogue/node_resolver.go b/internal/catalogue/node_resolver.go index 0f6515da..a628cba7 100644 --- a/internal/catalogue/node_resolver.go +++ b/internal/catalogue/node_resolver.go @@ -1,5 +1,7 @@ package catalogue +import "path" + // The machine's own resolver (novox/hq ADR 0247). // // **Most machines have none.** Every machine lists the mesh's resolvers in /etc/resolv.conf and nothing @@ -66,10 +68,13 @@ func holdsSeat(m Manifest, seat string) bool { return false } -// stepsBack is the module as it composes among the modules of one machine (novox/hq ADR 0287, ADR 0247): +// stepsBack is the module as it composes among the modules of one machine (novox/hq ADR 0287, +// ADR 0247): // **where it holds a seat that names a path stepping back for another seat, and that other seat's holder -// is on the machine and declares the path, its own declaration of the path is left out**, and nothing else -// of it changes. The stepping back is the seat's definition — node-uplink stepping back from the resolver +// is on the machine and declares the path, its own rendering of the path is left out**, and nothing else +// of it changes. Only a file the mesh renders for it steps back, as before: a resource on the path is +// composed as declared (the declaration copies a module's resources whole), so an uplink holder declaring +// the resolver file as a resource beside the resolver's holder is two owners of one path, and refused. The stepping back is the seat's definition — node-uplink stepping back from the resolver // file for node-resolver — so any other module declaring the path beside the owner's is still two owners // of one path, and is refused as before. func stepsBack(m Manifest, modules []Manifest) Manifest { @@ -95,20 +100,12 @@ func stepsBack(m Manifest, modules []Manifest) Manifest { } facts := make(map[string]RosterFile, len(m.Facts)) for name, f := range m.Facts { - if !f.Home && drop[f.Path] { + if !f.Home && drop[path.Clean(f.Path)] { continue } facts[name] = f } m.Facts = facts - var resources []map[string]any - for _, r := range m.Resources { - if p, ok := r["path"].(string); ok && drop[p] { - continue - } - resources = append(resources, r) - } - m.Resources = resources return m } diff --git a/internal/catalogue/seat_paths.go b/internal/catalogue/seat_paths.go index 3d68e448..1b74e6f6 100644 --- a/internal/catalogue/seat_paths.go +++ b/internal/catalogue/seat_paths.go @@ -45,6 +45,10 @@ func seatPathProblem(seat, scope, p string) string { case !strings.HasPrefix(p, "/") || path.Clean(p) != p: return fmt.Sprintf("the seat %s names the path %q, which is not a clean absolute path (novox/hq ADR 0287)", seat, p) + case path.Dir(p) == "/": + // The root or a top-level directory is every role's, never one role's own (ADR 0287). + return fmt.Sprintf("the seat %s names the path %q, the root or a top-level directory, which no one "+ + "role owns (novox/hq ADR 0287)", seat, p) } return "" } @@ -113,16 +117,20 @@ func namesWithoutSteppingBack(s Seat, p string) bool { // declaredPaths is every machine path a module declares it owns: a resource's literal path, and a file // the mesh renders for it outside the operator's home. An access is not here — reading is not owning. +// +// **Cleaned, because the machine writes the cleaned path**: /etc//hosts, /etc/./hosts, /etc/../etc/hosts and +// /etc/hosts/ are all /etc/hosts to it, and a seat's path is clean by its own rule, so a declaration compared +// as written would pass for a path nobody named. func declaredPaths(m Manifest) map[string]bool { out := map[string]bool{} for _, r := range m.Resources { if p, ok := r["path"].(string); ok && strings.HasPrefix(p, "/") { - out[p] = true + out[path.Clean(p)] = true } } for _, f := range m.Facts { if !f.Home && strings.HasPrefix(f.Path, "/") { - out[f.Path] = true + out[path.Clean(f.Path)] = true } } return out diff --git a/internal/catalogue/seat_paths_test.go b/internal/catalogue/seat_paths_test.go index 536bcb00..a6f5df50 100644 --- a/internal/catalogue/seat_paths_test.go +++ b/internal/catalogue/seat_paths_test.go @@ -137,6 +137,12 @@ func TestOnlyAClaimantDeclaresASeatsPath(t *testing.T) { if !strings.Contains(got, `stranger declares the path "/etc/hosts", which the seat node-hostname names`) { t.Errorf("a module declaring /etc/hosts without claiming node-hostname was not refused: %q", got) } + // However the path is spelled (novox/hq issue 412 review). + unclean := Manifest{Module: "unclean", Version: "1", Resources: []map[string]any{ + {"id": "hosts", "type": "file", "path": "/etc/../etc/hosts", "content": "10.0.0.1 x\n"}}} + if got := strings.Join(CatalogueProblems(Shelf{"unclean": unclean}), "; "); !strings.Contains(got, "node-hostname") { + t.Errorf("/etc/../etc/hosts from a module not claiming node-hostname was not refused: %q", got) + } reader := Manifest{Module: "reader", Version: "1", Accesses: []Access{{ID: "hosts", Path: "/etc/hosts"}}} if got := CatalogueProblems(Shelf{"reader": reader}); len(got) != 0 { t.Errorf("reading /etc/hosts was refused: %v", got) @@ -237,19 +243,56 @@ func TestPathOwnersApplyTheSteppingBack(t *testing.T) { } } -// The stepping back comes from the seat: an uplink holder declaring the resolver file as a resource rather -// than a rendered file steps back too, and keeps everything else. -func TestTheUplinkStepsBackByTheSeatsDefinition(t *testing.T) { - uplink := Manifest{Module: "uplink", Version: "1", Claims: []Claim{{Name: "node-uplink"}}, - Resources: []map[string]any{ - {"id": "resolvers", "type": "file", "path": ResolverFile, "content": "nameserver 10.42.0.1\n"}, - {"id": "conf", "type": "file", "path": "/etc/uplink.conf", "content": "x"}}} - got := stepsBack(uplink, []Manifest{uplink, localResolver()}) - if len(got.Resources) != 1 || got.Resources[0]["id"] != "conf" { - t.Errorf("the uplink's holder composes %v beside the resolver", got.Resources) +// The stepping back comes from the seat, and is of a rendered file only: an uplink holder declaring the +// resolver file as a resource beside a node-resolver holder is two writers of one path, refused when the +// machine is resolved — never composed twice — as before the seat named its paths. +func TestAnUplinkDeclaringTheResolverFileAsAResourceIsRefusedBesideTheResolver(t *testing.T) { + shelf := resolverShelf(t) + uplink := shelf["systemd-networkd"] + uplink.Facts = nil + uplink.Resources = append(append([]map[string]any(nil), uplink.Resources...), + map[string]any{"id": "resolvers", "type": "file", "path": ResolverFile, "content": "nameserver 10.42.0.1\n"}) + shelf["systemd-networkd"] = uplink + shelf["local-resolver"] = localResolver() + _, err := Resolve(shelf, []string{"dnsmasq", "systemd-networkd", "local-resolver"}, splitDNSLaptop(), World{}) + if err == nil || !strings.Contains(err.Error(), `both declare the path "/etc/resolv.conf"`) { + t.Fatalf("an uplink declaring the resolver file as a resource beside the resolver was not refused: %v", err) } - if p := checkResources([]Manifest{uplink, localResolver()}); len(p) != 0 { - t.Errorf("the uplink's holder and the resolver's were refused together: %v", p) + // Its rendered file steps back, by the seat's definition, and nothing else of it does. + rendered := Manifest{Module: "uplink", Version: "1", Claims: []Claim{{Name: "node-uplink"}}, + Facts: map[string]RosterFile{"resolvers": {Path: "/etc//resolv.conf", Template: "x"}, + "other": {Path: "/etc/uplink.conf", Template: "x"}}} + got := stepsBack(rendered, []Manifest{rendered, localResolver()}) + if len(got.Facts) != 1 || got.Facts["other"].Path == "" { + t.Errorf("the uplink's holder renders %v beside the resolver", got.Facts) + } +} + +// A path is compared as the machine writes it: cleaned. Each spelling of /etc/hosts is /etc/hosts. +func TestAnUncleanSpellingOfASeatsPathIsTheSeatsPath(t *testing.T) { + for _, spelled := range []string{"/etc//hosts", "/etc/./hosts", "/etc/../etc/hosts", "/etc/hosts/"} { + stranger := Manifest{Module: "stranger", Version: "1", Resources: []map[string]any{ + {"id": "hosts", "type": "file", "path": spelled, "content": "10.0.0.1 x\n"}}} + if got := strings.Join(CatalogueProblems(Shelf{"stranger": stranger}), "; "); !strings.Contains(got, + `stranger declares the path "/etc/hosts", which the seat node-hostname names`) { + t.Errorf("%s from a module not claiming node-hostname was not refused: %q", spelled, got) + } + holder := hostnameHolder(map[string]any{"id": "hosts", "type": "file", "path": spelled, "content": "x"}, + map[string]any{"id": "name", "type": "file", "path": "/etc/hostname", "content": "x"}) + if err := CanHold(holder, hostnameSeat(t)); err != nil { + t.Errorf("a holder declaring %s was refused: %v", spelled, err) + } + } +} + +// A seat names a role's own path, never the root or a top-level directory. +func TestASeatNamesNoRootOrTopLevelDirectory(t *testing.T) { + for _, p := range []string{"/", "/etc"} { + m := Manifest{Module: "a", Version: "1", DefinesSeats: []SeatDeclaration{ + {Name: "a-seat", Scope: ScopeNode, Paths: []string{p}}}} + if got := strings.Join(declaredSeatProblems(m), "; "); !strings.Contains(got, "top-level directory") { + t.Errorf("a module's seat naming %s was accepted: %q", p, got) + } } }