From f4bcb320fecbb5c178152387c41942711029e707 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 24 Sep 2026 18:36:08 +0200 Subject: [PATCH 01/14] contributes: a module may answer one requirement several times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A module's contributes was map[string]map[string]any — one JSON object key per requirement, structurally exactly one contribution to "route" ever. minio needs two public hostnames (the S3 API and the console), which is two different contributions to route from one module, and nothing let it say so. This is the same shape of problem ADR 0094 solved for secrets (a module needing several values from one provider that gives one per pair): contributes now accepts either the ordinary {label, port} object, or an object of local names to several such objects. Detected per requirement key by what's inside, since (unlike secrets' string-vs-object split) both shapes are JSON objects: an ordinary contribution's fields are scalars, the several-instance shape is local-name -> object. Confirmed against every module.json in mesh-catalog before relying on that split. Both route-proxy and the migration-era route-adapter already key generated routers off the composed hostname (Values["name"]), not the module name, so two contributions with the same From reach them as two independent routes with no changes needed on the receiving side. --- internal/catalogue/declaration.go | 15 +++ internal/catalogue/manifest.go | 118 ++++++++++++++++- .../catalogue/several_contributions_test.go | 124 ++++++++++++++++++ 3 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 internal/catalogue/several_contributions_test.go diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index fefd2ff..c0ccc82 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -914,6 +914,21 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, composeName(values, r.PublicDomain) out[to] = append(out[to], Contribution{From: m.Module, Values: values}) } + // Several contributions to one requirement (ADR 0094's sibling for `contributes`): an + // object store's data API and its console are two different public names from one module, + // not one. Never in `granted` — a route names a host, not a credential — so every local + // name always reaches the provider from here. + for _, to := range sortedKeys(m.ContributesMany) { + for _, local := range sortedKeys(m.ContributesMany[to]) { + values, err := settle(m.ContributesMany[to][local], settings[m.Module], nil, + m.Module+" contributing "+local+" to "+to) + if err != nil { + return nil, fmt.Errorf("%s contributing %s to %s: %w", m.Module, local, to, err) + } + composeName(values, r.PublicDomain) + out[to] = append(out[to], Contribution{From: m.Module, Values: values}) + } + } } return out, nil } diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index e39fa17..6e42dd9 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -233,6 +233,18 @@ type Manifest struct { // module that had to say both would eventually say one. Contributes map[string]map[string]any `json:"contributes,omitempty"` + // ContributesMany is the same key, `contributes`, where a module tells one provider several + // things under local names — `"route": {"api": {"label": "files-api", "port": 9000}, "console": + // {"label": "files", "port": 9001}}` — because a module may answer one requirement more than + // once: an object store with a data API and a console are two different public names, not one + // (novox/hq ADR 0094's sibling for `contributes` rather than `secrets` — "a module may need more + // than one value from a provider that gives one per pair" applies exactly as well to what a + // module gives a provider as to what it keeps from one). Each local name is a contribution of + // its own, reaching the provider as its own entry in the file it receives. + // + // Filled from the manifest's `contributes` object by UnmarshalJSON; never written by hand. + ContributesMany map[string]map[string]map[string]any `json:"-"` + // Receives is where this module wants its consumers' contributions written, per requirement // it provides. // @@ -615,16 +627,24 @@ func AccessID(path string) string { return "access-" + strings.TrimPrefix(path, // it contributes to. func (m Manifest) Wants() []string { out := append([]string{}, m.Requires...) - for to := range m.Contributes { - var already bool + add := func(to string) { for _, r := range m.Requires { if r == to { - already = true + return } } - if !already { - out = append(out, to) + for _, already := range out { + if already == to { + return + } } + out = append(out, to) + } + for to := range m.Contributes { + add(to) + } + for to := range m.ContributesMany { + add(to) } sort.Strings(out) return out @@ -679,6 +699,47 @@ func (m *Manifest) UnmarshalJSON(raw []byte) error { } delete(keys, "secrets") } + contributesPlain := map[string]map[string]any{} + contributesMany := map[string]map[string]map[string]any{} + if contributes, ok := keys["contributes"]; ok && string(contributes) != "null" { + var byTo map[string]json.RawMessage + if err := json.Unmarshal(contributes, &byTo); err != nil { + return fmt.Errorf("contributes: an object of requirement to values, or to {local name: values}: %w", err) + } + for to, v := range byTo { + // Both shapes are JSON objects, unlike secrets' path-vs-object split, so the shapes are + // told apart by what is INSIDE: an ordinary contribution's fields are scalars (a label, + // a port); the several-instance shape is an object of local names, each itself an + // object of fields. Confirmed against the whole catalogue before relying on it — no + // contribution anywhere has an object-valued field. + var fields map[string]json.RawMessage + if err := json.Unmarshal(v, &fields); err != nil { + return fmt.Errorf("contributes.%s: an object of values, or of local name to values: %w", to, err) + } + many := len(fields) > 0 + for _, field := range fields { + trimmed := bytes.TrimSpace(field) + if len(trimmed) == 0 || trimmed[0] != '{' { + many = false + break + } + } + if many { + var locals map[string]map[string]any + if err := json.Unmarshal(v, &locals); err != nil { + return fmt.Errorf("contributes.%s: an object of local name to values: %w", to, err) + } + contributesMany[to] = locals + continue + } + var values map[string]any + if err := json.Unmarshal(v, &values); err != nil { + return fmt.Errorf("contributes.%s: an object of values: %w", to, err) + } + contributesPlain[to] = values + } + delete(keys, "contributes") + } rest, err := json.Marshal(keys) if err != nil { return err @@ -696,22 +757,46 @@ func (m *Manifest) UnmarshalJSON(raw []byte) error { if len(many) > 0 { m.SecretsMany = many } + if len(contributesPlain) > 0 { + m.Contributes = contributesPlain + } + if len(contributesMany) > 0 { + m.ContributesMany = contributesMany + } return nil } -// MarshalJSON writes `secrets` back in the shape it was read: paths, and objects of local names. +// MarshalJSON writes `secrets` and `contributes` back in the shape they were read: single values, +// and objects of local names. func (m Manifest) MarshalJSON() ([]byte, error) { raw, err := json.Marshal(manifestFields(m)) if err != nil { return nil, err } - if len(m.SecretsMany) == 0 { + if len(m.SecretsMany) == 0 && len(m.ContributesMany) == 0 { return raw, nil } var keys map[string]json.RawMessage if err := json.Unmarshal(raw, &keys); err != nil { return nil, err } + if len(m.ContributesMany) > 0 { + mergedContributes := map[string]any{} + for to, values := range m.Contributes { + mergedContributes[to] = values + } + for to, locals := range m.ContributesMany { + mergedContributes[to] = locals + } + contributes, err := json.Marshal(mergedContributes) + if err != nil { + return nil, err + } + keys["contributes"] = contributes + } + if len(m.SecretsMany) == 0 { + return json.Marshal(keys) + } merged := map[string]any{} for to, path := range m.Secrets { merged[to] = path @@ -872,6 +957,25 @@ func ParseManifest(raw []byte) (Manifest, error) { "%s contributes nothing to %q; if it only needs one, require it", m.Module, to)) } } + for to, locals := range m.ContributesMany { + if !name.MatchString(to) { + problems = append(problems, fmt.Sprintf("%q is not a usable name to contribute to", to)) + } + if len(locals) == 0 { + problems = append(problems, fmt.Sprintf( + "%s contributes nothing to %q; if it only needs one, require it", m.Module, to)) + } + for local, values := range locals { + if !name.MatchString(local) { + problems = append(problems, fmt.Sprintf( + "%s contributes to %q under %q, which is not a usable name", m.Module, to, local)) + } + if len(values) == 0 { + problems = append(problems, fmt.Sprintf( + "%s contributes nothing to %q under %q", m.Module, to, local)) + } + } + } problems = append(problems, m.Build.problems(m.Module)...) // **What provides the artifact store cannot be delivered through it** (novox/hq 04-ISSUES/029). // diff --git a/internal/catalogue/several_contributions_test.go b/internal/catalogue/several_contributions_test.go new file mode 100644 index 0000000..61b04c0 --- /dev/null +++ b/internal/catalogue/several_contributions_test.go @@ -0,0 +1,124 @@ +package catalogue + +import ( + "encoding/json" + "testing" +) + +// A module may answer one requirement more than once, the sibling of ADR 0094 for `contributes` +// rather than `secrets`: an object store's data API and its console are two different public +// names, not one. `contributes` maps a requirement to several sets of values under local names, +// each reaching the provider as its own entry — the same "several from one" shape ADR 0094 gave +// `secrets`, applied to the other half of an edge. + +const twoRoutes = `{"module":"minio","version":"1","requires":["route"], + "contributes":{"route":{"api":{"label":"files-api","port":9000},"console":{"label":"files","port":9001}}}}` + +func TestContributesReadsBothShapesAndWritesThemBack(t *testing.T) { + m, err := ParseManifest([]byte(twoRoutes)) + if err != nil { + t.Fatal(err) + } + locals := m.ContributesMany["route"] + if len(locals) != 2 || locals["api"]["label"] != "files-api" || locals["console"]["port"] != float64(9001) { + t.Fatalf("two contributions under local names: %+v", locals) + } + plain, err := ParseManifest([]byte(`{"module":"board","version":"1","requires":["route"], + "contributes":{"route":{"label":"board","port":8080}}}`)) + if err != nil { + t.Fatal(err) + } + if got := plain.Contributes["route"]; got["label"] != "board" || len(plain.ContributesMany) != 0 { + t.Fatalf("the plain shape is one contribution with no local names: %+v / %+v", got, plain.ContributesMany) + } + // Written back in the shape it was read, so a built manifest keeps its local names. + raw, err := json.Marshal(m) + if err != nil { + t.Fatal(err) + } + again, err := ParseManifest(raw) + if err != nil { + t.Fatalf("what was written does not read: %v\n%s", err, raw) + } + if len(again.ContributesMany["route"]) != 2 { + t.Fatalf("the local names did not survive a round trip:\n%s", raw) + } +} + +func TestAContributionLocalNameMustBeUsable(t *testing.T) { + for _, bad := range []string{ + // Not a usable name. + `{"module":"minio","version":"1","requires":["route"], + "contributes":{"route":{"Not OK":{"label":"files","port":9000}}}}`, + // A local contribution with nothing in it. + `{"module":"minio","version":"1","requires":["route"], + "contributes":{"route":{"api":{}}}}`, + } { + if _, err := ParseManifest([]byte(bad)); err == nil { + t.Errorf("accepted:\n%s", bad) + } + } +} + +func minimalRouteProxy() Manifest { + return Manifest{Module: "route-proxy", Version: "1", + Provides: FromAnywhere("route"), + Receives: map[string]string{"route": "/var/lib/route-proxy/routes/mesh.json"}, + } +} + +func TestAModuleWithTwoRoutesGivesTheProviderTwoContributions(t *testing.T) { + minio, err := ParseManifest([]byte(twoRoutes)) + if err != nil { + t.Fatal(err) + } + got, err := Resolve(shelf(minimalRouteProxy(), minio), []string{"route-proxy", "minio"}, workstation(), World{}) + if err != nil { + t.Fatal(err) + } + out, err := got.Declaration(Rendering{}) + if err != nil { + t.Fatal(err) + } + var given []Contribution + for _, r := range out { + if r["path"] != "/var/lib/route-proxy/routes/mesh.json" { + continue + } + var parsed struct { + Given []Contribution `json:"given"` + } + if err := json.Unmarshal([]byte(r["content"].(string)), &parsed); err != nil { + t.Fatal(err) + } + given = parsed.Given + } + if len(given) != 2 { + t.Fatalf("two named routes from one module are two contributions: %+v", given) + } + byPort := map[float64]string{} + for _, g := range given { + if g.From != "minio" { + t.Fatalf("both contributions are minio's: %+v", g) + } + port, _ := g.Values["port"].(float64) + label, _ := g.Values["label"].(string) + byPort[port] = label + } + if byPort[9000] != "files-api" || byPort[9001] != "files" { + t.Fatalf("the two routes did not both survive: %+v", given) + } +} + +// A module with the ordinary, single-contribution shape resolves exactly as it did before — +// ContributesMany being empty must change nothing about it. +func TestASingleRouteStillResolvesTheOrdinaryWay(t *testing.T) { + got, err := Resolve(shelf(proxy(), published("board", "board", 8080)), []string{"board"}, workstation(), World{}) + if err != nil { + t.Fatal(err) + } + given := received(t, mustDeclare(t, got)) + if len(given) != 1 || given[0].From != "board" { + t.Fatalf("the plain shape regressed: %+v", given) + } +} From 524cc2a3ecedcd36c33cec15f133f3cdd0aa8997 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 24 Sep 2026 18:40:30 +0200 Subject: [PATCH 02/14] plan: show what a module would open and why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every module.json already declares a why for each port under listens, but plan only ever used it to build the firewall's rule set — nothing printed it. An operator deciding whether to assign a module had no way to see what it would open without reading the manifest by hand. plan now prints each assigned module's listens entries — port, protocol, source, and its why — right under the module line, so the same text that feeds the firewall is visible at the point someone is actually deciding whether to open it. --- cmd/mesh-controller/plan.go | 19 ++++++++++++++++ cmd/mesh-controller/plan_test.go | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 cmd/mesh-controller/plan_test.go diff --git a/cmd/mesh-controller/plan.go b/cmd/mesh-controller/plan.go index 23a97fa..b6bd92b 100644 --- a/cmd/mesh-controller/plan.go +++ b/cmd/mesh-controller/plan.go @@ -800,6 +800,22 @@ func grantsFor(ctx context.Context, open *stores, node string) ([]catalogue.Gran return out, nil } +// listensLines is what a person is told about what this module would open, and why — the same +// `why` every listens entry already carries for the firewall it also feeds (novox/hq ADR 0007), so +// deciding whether to assign a module can see what it would open before it opens it, not only +// after. A module with nothing to listen on prints nothing extra, same as today. +func listensLines(m catalogue.Manifest) []string { + var out []string + for _, l := range m.Listens { + if l.Why == "" { + out = append(out, fmt.Sprintf(" listens %d/%s from %s", l.Port, l.At(), l.From)) + continue + } + out = append(out, fmt.Sprintf(" listens %d/%s from %s — %s", l.Port, l.At(), l.From, l.Why)) + } + return out +} + func planCommand(ctx context.Context, args []string) error { set := flag.NewFlagSet("plan", flag.ContinueOnError) // Because "one resource" does not tell you whether the settings landed. Being able to read @@ -853,6 +869,9 @@ func planCommand(ctx context.Context, args []string) error { fmt.Printf("%s would run:\n", args[0]) for _, m := range plan.Modules { fmt.Printf(" %-20s %s\n", m.Module, plan.Because[m.Module]) + for _, line := range listensLines(m) { + fmt.Println(line) + } } // What was assigned here and cannot run here. Said with the rest rather than as a refusal: it is // one module on the wrong machine, the others still run, and the remedy is to move this one. diff --git a/cmd/mesh-controller/plan_test.go b/cmd/mesh-controller/plan_test.go new file mode 100644 index 0000000..35a86b8 --- /dev/null +++ b/cmd/mesh-controller/plan_test.go @@ -0,0 +1,37 @@ +package main + +import ( + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/catalogue" +) + +// `plan` tells a person what a module would open and why, from the same `why` every listens +// entry already carries for the firewall (novox/hq ADR 0007) — so deciding whether to assign a +// module does not need reading its manifest first. +func TestListensLinesShowWhatAModuleWouldOpenAndWhy(t *testing.T) { + m := catalogue.Manifest{Module: "minio", Listens: []catalogue.Listening{ + {Port: 9000, From: catalogue.FromMesh, Why: "the S3 endpoint"}, + {Port: 9001, From: catalogue.FromMesh}, + }} + got := listensLines(m) + if len(got) != 2 { + t.Fatalf("two listens entries, got %d: %v", len(got), got) + } + if !strings.Contains(got[0], "9000/tcp") || !strings.Contains(got[0], "the S3 endpoint") { + t.Errorf("the port and its why did not both appear: %q", got[0]) + } + if strings.Contains(got[1], "—") { + t.Errorf("a listens entry with no why should not print a dash: %q", got[1]) + } + if !strings.Contains(got[1], "9001/tcp") { + t.Errorf("the port still appears without a why: %q", got[1]) + } +} + +func TestListensLinesAreEmptyForAModuleWithNothingToListenOn(t *testing.T) { + if got := listensLines(catalogue.Manifest{Module: "board"}); len(got) != 0 { + t.Errorf("a module with no listens should print nothing, got %v", got) + } +} From 8fa5443862e299114bcaa5da41a0ba20cf1536ea Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 24 Sep 2026 18:54:35 +0200 Subject: [PATCH 03/14] contributes: a module's grant carries no value where it contributed several times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ContributionsFrom settled to whichever of a module's several contributions to one requirement sorted first, arbitrarily — the grant minted for it then carried that contribution's label and port under a credential the OTHER contribution's consumer never sees, and collided with that same contribution's own entry from contributions() besides. Confirmed live: minio's two route contributions (files-api, files) produced three entries in route-adapter's received file — files-api twice, once credentialed and once not, files not credentialed at all. Every single-contribution module (gitea, keycloak, umami) already mints an unused credential for `route` too — route never needs one, by its own documentation — but with exactly one contribution to match there was nothing to collide with, so it never surfaced. Where a module contributes more than once, there is no single value to settle on. The module still asks, still gets its one credential — a pair credential is not a place for a label or a port anyway — and each named contribution reaches the provider on its own, unchanged. No cleanup needed for the secret already minted live for minio+route: the sealed blob is a random pair credential unrelated to Values, which is recomputed fresh on every plan/push regardless. --- internal/catalogue/declaration.go | 19 +++++++- .../catalogue/several_contributions_test.go | 43 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index c0ccc82..5d3c077 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -1120,17 +1120,34 @@ func boundFile(n Needed, path, as string) (map[string]any, error) { // arrangement refused is the ordinary one. A node running eight services against one database is // not an edge case; it is what a machine looks like. Now each consumer has its own credential and // there is nothing left to refuse. +// +// **One credential, even where a module contributes several times.** A module may answer one +// requirement more than once (ADR 0094's sibling for `contributes`) — an object store's data API +// and its console are two different names, not one. There is still only one `Needed` for it, one +// credential minted, one grant to settle: a pair credential is not a place to put a label or a +// port. So where several of this module's contributions reach the same requirement, none of them +// is "the" value — settling to the first, arbitrarily, would hand the grant one contribution's +// values under a credential the OTHER contribution's consumer never sees, and would collide with +// that contribution's own entry from contributions() besides. Empty values, still granted: the +// module asked, gets its credential, and each named contribution reaches the provider on its own. func (r Resolution) ContributionsFrom(requirement, module string, settings SettingsBy) ( map[string]any, bool, error) { all, err := r.contributions(settings, nil, nil) if err != nil { return nil, false, err } + var mine []map[string]any for _, g := range all[requirement] { if g.From == module { - return g.Values, true, nil + mine = append(mine, g.Values) } } + if len(mine) == 1 { + return mine[0], true, nil + } + if len(mine) > 1 { + return map[string]any{}, true, nil + } // It contributes no payload — but a require-only consumer of a parameterless provision (one whose // `serves` names no consumer-supplied key: `redis-cache`, `amqp`) still ASKS for it and must be // granted a credential. Keying "asks" on contributions alone marked those grants withdrawn diff --git a/internal/catalogue/several_contributions_test.go b/internal/catalogue/several_contributions_test.go index 61b04c0..b0fe881 100644 --- a/internal/catalogue/several_contributions_test.go +++ b/internal/catalogue/several_contributions_test.go @@ -122,3 +122,46 @@ func TestASingleRouteStillResolvesTheOrdinaryWay(t *testing.T) { t.Fatalf("the plain shape regressed: %+v", given) } } + +// ContributionsFrom is what mints the ONE pair credential a requiring module is granted +// (cmd/mesh-controller/plan.go's grantsFor) — a separate path from Declaration()'s raw file, and +// the one the two-routes test above never exercised. Where a module contributes several times, +// there is no single "the" value: settling to whichever sorts first would both misrepresent the +// grant and collide with that same contribution's own entry from contributions(), which is +// exactly the duplicate a live plan against minio surfaced (files-api appearing once with a +// credential, once without, while files got neither). +func TestContributionsFromHasNoSingleValueWhenAModuleContributesSeveralTimes(t *testing.T) { + minio, err := ParseManifest([]byte(twoRoutes)) + if err != nil { + t.Fatal(err) + } + got, err := Resolve(shelf(minimalRouteProxy(), minio), []string{"route-proxy", "minio"}, workstation(), World{}) + if err != nil { + t.Fatal(err) + } + values, asks, err := got.ContributionsFrom("route", "minio", nil) + if err != nil { + t.Fatal(err) + } + if !asks { + t.Fatal("minio still requires route, so it still asks") + } + if len(values) != 0 { + t.Fatalf("no single value represents two contributions, got %+v", values) + } +} + +// The ordinary, single-contribution case is unchanged: exactly one match still settles to it. +func TestContributionsFromReturnsTheOneValueForAnOrdinaryContribution(t *testing.T) { + got, err := Resolve(shelf(proxy(), published("board", "board", 8080)), []string{"board"}, workstation(), World{}) + if err != nil { + t.Fatal(err) + } + values, asks, err := got.ContributionsFrom("reverse-proxy", "board", nil) + if err != nil { + t.Fatal(err) + } + if !asks || values["host"] != "board" { + t.Fatalf("the ordinary single contribution should still settle to its own value: %+v", values) + } +} From 008ce39ec0ca2e886922ff8ce4121d0f6758f3ce Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 25 Sep 2026 14:00:57 +0200 Subject: [PATCH 04/14] route-proxy: a route carries the policy applied to a request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements novox/hq ADR 0108, closing issue 116. The proxy's request path was a host lookup and a forward, so it applied nothing — while the ingress it replaces relies on four things it had none of. Path scoping came first because it is a prerequisite, not a sibling. The table mapped a host to one target, so a host could not be routed two ways, and the refusal this issue turns on matches a path on a host already routed to a workload. No amount of authentication or source filtering would have made it expressible. The table is now host to an ordered list of rules, matched on path prefix. The order is total, not just by priority. Sorting on priority alone leaves rules that share one in whatever order the map produced, so the same declaration would serve differently between restarts — a fault that works, and works differently each time, which is the hardest kind to believe when reported. Within a priority the longer path wins, which is also the intuitive reading. auth names a secret and never holds one. A declaration carrying a credential is refused whole rather than served unprotected, so the option ADR 0108 rejected cannot return by accident. A secret that cannot be read makes the route refuse and say so, rather than serve the workload unprotected — a gate that cannot check is not a gate that opens, and the alternative turns a missing file into a silently public admin surface. Authentication costs one bcrypt comparison on every path including an unknown user, so an unknown user is not measurably faster than a known one with a wrong password. That difference is a way to enumerate a route's users from outside it. Redirects keep the request's own path and query, or canonicalising one name onto another would land every deep link on the front page and raise no error doing it. Eleven tests, four of them for the capabilities and two for the failure modes that rot quietly: the credential-in-a-declaration refusal, and the unreadable secret failing closed. Nothing else breaks if those stop working, so nothing else would report it. No new dependency: bcrypt comes from the x/crypto module already required. --- examples/route-proxy/main.go | 356 +++++++++++++++++++++++++--- examples/route-proxy/policy_test.go | 233 ++++++++++++++++++ examples/route-proxy/routes_test.go | 43 +++- 3 files changed, 581 insertions(+), 51 deletions(-) create mode 100644 examples/route-proxy/policy_test.go diff --git a/examples/route-proxy/main.go b/examples/route-proxy/main.go index 2962a85..10d7217 100644 --- a/examples/route-proxy/main.go +++ b/examples/route-proxy/main.go @@ -10,10 +10,31 @@ // program. What lives here is that contract, written as something that runs so it can be read // rather than described. // +// **A route also carries what a request arriving at it may do** (novox/hq ADR 0108). The grant used +// to say only where to send traffic, so this proxy applied nothing; the four things the ingress it +// replaces actually relies on are now part of the contribution. The set is closed at four, because +// an open middleware surface recreates the thing being replaced and is far harder to narrow later +// than a closed one is to widen. +// // What it is given, written by the host from an ordinary declaration: // // $ROUTES every consumer, the name it asked for, and where the mesh says that machine is // +// Each contribution's values carry the name and port as before, and optionally: +// +// path the path prefix this rule is scoped to; absent means every path +// priority which rule wins where two match; higher first, and the order is total +// deny refuse the request outright — the shape an incident mitigation needs +// redirect answer with a permanent redirect to this name, keeping the path and query +// auth the *path of a secret* holding `user:hash` lines, never the credential itself +// +// A host may appear more than once, which is what path scoping means: one rule refusing a path +// while another serves everything else on the same name. +// +// **`auth` names a secret and never holds one.** A declaration carrying a credential is refused +// outright rather than served unprotected, and a secret that cannot be read makes the route refuse +// rather than open — a gate that cannot check is not a gate that opens. +// // It re-reads on change rather than being restarted, for the same reason the provisioner does: // a route arriving or leaving is an ordinary event and must not drop the connections of every // other workload. @@ -23,6 +44,7 @@ import ( "bytes" "context" "crypto/sha256" + "crypto/subtle" "crypto/tls" "crypto/x509" "encoding/hex" @@ -42,6 +64,7 @@ import ( "golang.org/x/crypto/acme" "golang.org/x/crypto/acme/autocert" + "golang.org/x/crypto/bcrypt" ) // Where public certificates come from when nothing says otherwise. @@ -74,7 +97,7 @@ func issuer() string { // to what it may serve. func onlyWhatTheMeshSaid(held *table) autocert.HostPolicy { return func(_ context.Context, host string) error { - if _, known := held.find(host); known { + if held.routed(host) { return nil } return fmt.Errorf("no route for %q in this mesh, so no certificate is asked for", host) @@ -95,48 +118,145 @@ type contribution struct { Values map[string]any `json:"values"` } +// policy is what a rule does with a request that matched it. +// +// **Decided by the mesh, not here** (novox/hq ADR 0108). A route grant used to hand back a name and +// say nothing about what the name admitted, so this proxy admitted everything. The set is closed at +// four — authentication, refusal, path scoping, redirect — because an open middleware surface +// recreates the thing being replaced and is far harder to narrow later than a closed one is to widen. +type policy struct { + // deny refuses the request outright, whatever it is. + deny bool + // redirectTo answers with a permanent redirect instead of proxying. The request's own path and + // query are carried across, which is what canonicalising one public name onto another means. + redirectTo string + // users is what a request must present, read at load time from the secret the declaration + // *named*. A declaration never carries the credential itself. + users map[string]string + // sealed is set when authentication was declared and the secret could not be read. The rule then + // refuses everything and says why. + // + // **Fail closed.** The alternative — serve the route unauthenticated because the gate is + // missing — turns an unreadable file into a silently public admin surface, which is the exact + // outcome ADR 0108 exists to prevent. A gate that cannot check is not a gate that opens. + sealed string +} + +// rule is one way a host may be routed. A host may have several, which is what path scoping means. +type rule struct { + path string // "" matches every path + priority int + policy policy + to *httputil.ReverseProxy + target string +} + // table is what the proxy is currently serving, replaced whole whenever the file changes. // // Replaced rather than merged: the file is the whole truth about who has a route, so merging // would keep serving a name whose module was unassigned — which is the stale-route fault // 08-connectivity lists as open, reintroduced one level down. +// +// Keyed by host to an *ordered* list rather than to one target, because two of the four policies +// need a single host routed more than one way: a refusal on a path the ordinary route also matches, +// and a certificate-challenge path on a host that otherwise serves a workload. type table struct { - mu sync.RWMutex - to map[string]*httputil.ReverseProxy - targets map[string]string + mu sync.RWMutex + to map[string][]rule } -func (t *table) set(routes map[string]string) { - made := map[string]*httputil.ReverseProxy{} - for name, target := range routes { - where, err := url.Parse(target) - if err != nil { - log.Printf("route %s points at %q, which is not a URL: %v", name, target, err) +func (t *table) set(routes map[string][]rule) { + made := map[string][]rule{} + for host, rules := range routes { + kept := make([]rule, 0, len(rules)) + for _, r := range rules { + // A rule that only refuses or only redirects has nowhere to send anything, and needs + // nowhere: it answers by itself. + if r.policy.deny || r.policy.redirectTo != "" { + kept = append(kept, r) + continue + } + where, err := url.Parse(r.target) + if err != nil { + log.Printf("route %s points at %q, which is not a URL: %v", host, r.target, err) + continue + } + r.to = httputil.NewSingleHostReverseProxy(where) + kept = append(kept, r) + } + if len(kept) == 0 { continue } - made[name] = httputil.NewSingleHostReverseProxy(where) + inOrder(kept) + made[host] = kept } t.mu.Lock() - t.to, t.targets = made, routes + t.to = made t.mu.Unlock() } -func (t *table) find(host string) (*httputil.ReverseProxy, bool) { - // The port is not part of the name. A request to app.example:8080 is for app.example. +// inOrder puts the rules for one host into the order they are matched in, and does so totally. +// +// **Equal priorities must resolve identically every time** (ADR 0108). Sorting only by priority +// leaves rules that share one in whatever order the map produced, so the same declaration would +// serve differently between restarts — a proxy that is not reproducible. Longest path first within a +// priority is also the intuitive reading: the more specific rule wins. The last two keys exist only +// to make the order total. +func inOrder(rules []rule) { + sort.SliceStable(rules, func(i, j int) bool { + a, b := rules[i], rules[j] + if a.priority != b.priority { + return a.priority > b.priority + } + if len(a.path) != len(b.path) { + return len(a.path) > len(b.path) + } + if a.path != b.path { + return a.path < b.path + } + return a.target < b.target + }) +} + +// find is the rule that answers this request, or nothing if the host is not routed here at all. +func (t *table) find(host, path string) (rule, bool) { + t.mu.RLock() + defer t.mu.RUnlock() + for _, r := range t.to[bareHost(host)] { + if r.path == "" || strings.HasPrefix(path, r.path) { + return r, true + } + } + return rule{}, false +} + +// routed says whether this proxy serves the name at all, whatever the path. +// +// Separate from find because certificate issuance is a question about the *name*: a host whose only +// rules are path-scoped is still a name this proxy answers to, and still needs a certificate. +func (t *table) routed(host string) bool { + t.mu.RLock() + defer t.mu.RUnlock() + return len(t.to[bareHost(host)]) > 0 +} + +// bareHost is the name without the port, lower-cased. +// +// The port is not part of the name: a request to app.example:8080 is for app.example. Lower-cased +// because a Host header is not case-sensitive, and a route that only answers the spelling in the +// manifest answers half the requests made to it. +func bareHost(host string) string { if h, _, err := net.SplitHostPort(host); err == nil { host = h } - t.mu.RLock() - defer t.mu.RUnlock() - p, ok := t.to[strings.ToLower(host)] - return p, ok + return strings.ToLower(host) } func (t *table) names() []string { t.mu.RLock() defer t.mu.RUnlock() - out := make([]string, 0, len(t.targets)) - for name := range t.targets { + out := make([]string, 0, len(t.to)) + for name := range t.to { out = append(out, name) } sort.Strings(out) @@ -285,13 +405,13 @@ func forThisAuthority(cache, directory string, root []byte) string { // newTable is an empty routing table. func newTable() *table { - return &table{to: map[string]*httputil.ReverseProxy{}, targets: map[string]string{}} + return &table{to: map[string][]rule{}} } // handler is the proxy itself, separated so it can be driven by a test without a listener. func handler(held *table) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - proxy, known := held.find(r.Host) + matched, known := held.find(r.Host, r.URL.Path) if !known { // **Named, not a bare 404.** A route that was withdrawn and a name that never existed // are different things, and a proxy that says only "not found" makes an operator go @@ -302,12 +422,89 @@ func handler(held *table) http.Handler { r.Host, strings.Join(held.names(), ", ")) return } - proxy.ServeHTTP(w, r) + + switch { + case matched.policy.sealed != "": + // Declared a gate, cannot check it. Refused, and says why — an operator reading this + // learns the secret is missing, rather than wondering why a protected name is 503. + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusServiceUnavailable) + fmt.Fprintf(w, "this route requires authentication and its credentials cannot be read: %s\n", + matched.policy.sealed) + return + + case matched.policy.deny: + http.Error(w, "this path is not served to you", http.StatusForbidden) + return + + case matched.policy.redirectTo != "": + http.Redirect(w, r, canonical(matched.policy.redirectTo, r.URL), http.StatusMovedPermanently) + return + + case len(matched.policy.users) > 0 && !allowed(matched.policy.users, r): + // The realm is the name asked for, so a browser's prompt says which route it is for. + w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q, charset=\"UTF-8\"", bareHost(r.Host))) + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + matched.to.ServeHTTP(w, r) }) } -// routesFrom reads what the mesh wrote and turns it into name → target. -func routesFrom(path string) (map[string]string, error) { +// canonical is where a redirect sends this request. +// +// The declaration names the destination *name*; the request keeps its own path and query. That is +// what canonicalising one public name onto another means — a link to a page under the old name has +// to arrive at the same page under the new one, or the redirect silently loses every deep link. +func canonical(to string, from *url.URL) string { + where, err := url.Parse(to) + if err != nil { + return to + } + if where.Path == "" || where.Path == "/" { + where.Path = from.Path + } + if where.RawQuery == "" { + where.RawQuery = from.RawQuery + } + return where.String() +} + +// allowed says whether the request presented credentials this route accepts. +// +// **Every path costs one bcrypt comparison**, including an unknown user, which is why the miss +// compares against a fixed hash rather than returning early. Returning early would make an unknown +// user measurably faster than a known one with a wrong password, and that difference is a way to +// enumerate the users of a route from outside it. +func allowed(users map[string]string, r *http.Request) bool { + // A hash of nothing anybody knows. Its only job is to cost what a real comparison costs. + const absent = "$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92ldGxad68LJZdL17lhWy" + user, password, ok := r.BasicAuth() + if !ok { + return false + } + want, known := users[user] + if !known { + want = absent + } + if err := bcrypt.CompareHashAndPassword([]byte(want), []byte(password)); err != nil { + return false + } + // `known` is checked after the comparison, not instead of it, so the timing is the same either + // way. subtle.ConstantTimeByteEq keeps the branch from being the thing that differs. + return subtle.ConstantTimeByteEq(boolByte(known), 1) == 1 +} + +func boolByte(b bool) byte { + if b { + return 1 + } + return 0 +} + +// routesFrom reads what the mesh wrote and turns it into host → the rules for that host. +func routesFrom(path string) (map[string][]rule, error) { raw, err := os.ReadFile(path) if err != nil { return nil, err @@ -317,31 +514,114 @@ func routesFrom(path string) (map[string]string, error) { return nil, err } - out := map[string]string{} + out := map[string][]rule{} for _, c := range said.Given { name, _ := c.Values["name"].(string) if name == "" { log.Printf("%s on %s asked for a route and named nothing; skipped", c.From, c.Node) continue } - port, ok := asPort(c.Values["port"]) - if !ok { - log.Printf("%s on %s asked for route %q and gave no usable port; skipped", - c.From, c.Node, name) - continue + host := strings.ToLower(name) + + made := rule{path: asPath(c.Values["path"])} + if p, ok := asPort(c.Values["priority"]); ok { + made.priority = p } - // Where the mesh says that machine is. Empty means it is this one — a workload beside the - // proxy is ordinary, and reaching it over loopback is both correct and the only thing - // that works when there is no private network. - at := c.At - if at == "" { - at = "127.0.0.1" + made.policy.deny, _ = c.Values["deny"].(bool) + made.policy.redirectTo, _ = c.Values["redirect"].(string) + + if named, carried := c.Values["auth"].(string); carried && strings.TrimSpace(named) != "" { + // **A declaration names a secret; it never holds one** (ADR 0108). Refused rather than + // tolerated, and the whole rule is dropped rather than served unprotected — the + // rejected option cannot come back by accident, which is the failure this check exists + // to make impossible. + if looksLikeACredential(named) { + log.Printf("%s on %s declared route %q with a credential in the declaration rather "+ + "than the name of a secret; the whole route is refused (novox/hq ADR 0108)", + c.From, c.Node, name) + continue + } + users, err := usersFrom(named) + if err != nil { + // Fail closed: the rule is kept so the name stays routed and answers, and it + // answers by refusing. Dropping it instead would make the name 404 and read as a + // withdrawn route rather than an unreadable secret. + made.policy.sealed = err.Error() + } + made.policy.users = users } - out[strings.ToLower(name)] = fmt.Sprintf("http://%s:%d", at, port) + + // Only a rule that actually proxies needs somewhere to send the request. + if !made.policy.deny && made.policy.redirectTo == "" { + port, ok := asPort(c.Values["port"]) + if !ok { + log.Printf("%s on %s asked for route %q and gave no usable port; skipped", + c.From, c.Node, name) + continue + } + // Where the mesh says that machine is. Empty means it is this one — a workload beside + // the proxy is ordinary, and reaching it over loopback is both correct and the only + // thing that works when there is no private network. + at := c.At + if at == "" { + at = "127.0.0.1" + } + made.target = fmt.Sprintf("http://%s:%d", at, port) + } + + out[host] = append(out[host], made) } return out, nil } +// asPath is the path prefix a rule is scoped to, or "" for every path. +func asPath(v any) string { + p, _ := v.(string) + p = strings.TrimSpace(p) + if p == "" { + return "" + } + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + return p +} + +// looksLikeACredential is the check that keeps a secret out of a declaration. +// +// It errs towards refusing: a value holding a `:` (the htpasswd separator) or opening with a bcrypt +// identifier is a credential, not a path, and no filesystem path the mesh writes needs either. A +// false refusal is a loud log and a route that does not serve; a false accept is a credential +// committed to a declaration, which is the thing being prevented. +func looksLikeACredential(v string) bool { + v = strings.TrimSpace(v) + return strings.Contains(v, ":") || strings.HasPrefix(v, "$2") +} + +// usersFrom reads the credentials the mesh mounted, in the one format every htpasswd already is. +func usersFrom(path string) (map[string]string, error) { + raw, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("cannot read the secret named for this route: %w", err) + } + users := map[string]string{} + for _, line := range strings.Split(string(raw), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + user, hash, ok := strings.Cut(line, ":") + if !ok || user == "" || hash == "" { + continue + } + users[user] = hash + } + if len(users) == 0 { + return nil, fmt.Errorf("the secret named for this route holds no usable credentials") + } + return users, nil +} + // asPort accepts what JSON makes of a number, which is a float even when it was written 8080. func asPort(v any) (int, bool) { switch n := v.(type) { diff --git a/examples/route-proxy/policy_test.go b/examples/route-proxy/policy_test.go new file mode 100644 index 0000000..7f98ec4 --- /dev/null +++ b/examples/route-proxy/policy_test.go @@ -0,0 +1,233 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "golang.org/x/crypto/bcrypt" +) + +// What a route carries about the requests arriving at it — novox/hq ADR 0108. +// +// Each test here is one of the four capabilities that record closed the set at, plus the negative +// case it promised would be refused. The negative case is the one that rots quietly: nothing fails +// if it stops working, so nothing tells you it has. + +// served starts a workload and gives back the host and port the mesh would have recorded for it. +func served(t *testing.T, body string) (string, int) { + t.Helper() + workload := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(workload.Close) + host, port, _ := strings.Cut(strings.TrimPrefix(workload.URL, "http://"), ":") + n, err := strconv.Atoi(port) + if err != nil { + t.Fatal(err) + } + return host, n +} + +// ask makes one request through the proxy for a given name and path, without following redirects. +func ask(t *testing.T, proxy, name, path string, auth [2]string) *http.Response { + t.Helper() + req, err := http.NewRequest(http.MethodGet, proxy+path, nil) + if err != nil { + t.Fatal(err) + } + req.Host = name + if auth[0] != "" { + req.SetBasicAuth(auth[0], auth[1]) + } + client := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }} + answer, err := client.Do(req) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = answer.Body.Close() }) + return answer +} + +func proxyFor(t *testing.T, routesJSON string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "routes.json") + if err := os.WriteFile(path, []byte(routesJSON), 0o644); err != nil { + t.Fatal(err) + } + routes, err := routesFrom(path) + if err != nil { + t.Fatal(err) + } + held := newTable() + held.set(routes) + server := httptest.NewServer(handler(held)) + t.Cleanup(server.Close) + return server.URL +} + +// A refusal on a path shadows the ordinary route for that path and leaves every other path alone. +// +// **This is why path scoping is a prerequisite and not a sibling capability.** The rule being +// reproduced matches a path on a host that is already routed to a workload, so a table mapping a +// host to one target cannot express it at all — no amount of authentication or source filtering +// would have helped. +func TestARefusedPathShadowsTheRouteAndLeavesTheRestServed(t *testing.T) { + at, port := served(t, "the workload") + proxy := proxyFor(t, `{"given":[ + {"from":"forge","node":"anchor","at":"`+at+`","values":{"name":"forge.example","port":`+strconv.Itoa(port)+`}}, + {"from":"forge","node":"anchor","values":{"name":"forge.example","path":"/api/internal","priority":100,"deny":true}} + ]}`) + + if got := ask(t, proxy, "forge.example", "/api/internal/hook", [2]string{}).StatusCode; got != http.StatusForbidden { + t.Fatalf("the refused path answered %d, so the block that was put in front of it during an "+ + "incident is not in front of it any more", got) + } + if got := ask(t, proxy, "forge.example", "/", [2]string{}).StatusCode; got != http.StatusOK { + t.Fatalf("refusing one path took the whole route with it: %d", got) + } +} + +// A redirect answers with the redirect, and the request keeps its own path and query. +// +// Losing the path would turn canonicalising one name onto another into "every deep link now lands +// on the front page", which is the kind of breakage that produces no error anywhere. +func TestARedirectKeepsThePathAndQuery(t *testing.T) { + proxy := proxyFor(t, `{"given":[ + {"from":"site","node":"anchor","values":{"name":"www.example","redirect":"https://example/"}} + ]}`) + + answer := ask(t, proxy, "www.example", "/deep/page?ref=1", [2]string{}) + if answer.StatusCode != http.StatusMovedPermanently { + t.Fatalf("a declared redirect answered %d", answer.StatusCode) + } + where := answer.Header.Get("Location") + if !strings.Contains(where, "/deep/page") || !strings.Contains(where, "ref=1") { + t.Fatalf("the redirect dropped the path or the query: %q", where) + } +} + +// Authentication refuses a request with no credentials, admits one with the right ones, and refuses +// the wrong ones — with the credentials read from the secret the declaration *named*. +func TestAuthenticationAdmitsOnlyWhatTheSecretSays(t *testing.T) { + at, port := served(t, "the console") + hash, err := bcrypt.GenerateFromPassword([]byte("correct horse"), bcrypt.MinCost) + if err != nil { + t.Fatal(err) + } + secret := filepath.Join(t.TempDir(), "console-auth") + if err := os.WriteFile(secret, []byte("# a comment\nadmin:"+string(hash)+"\n"), 0o600); err != nil { + t.Fatal(err) + } + + proxy := proxyFor(t, `{"given":[ + {"from":"console","node":"anchor","at":"`+at+`","values":{"name":"console.example","port":`+strconv.Itoa(port)+`,"auth":"`+secret+`"}} + ]}`) + + if got := ask(t, proxy, "console.example", "/", [2]string{}).StatusCode; got != http.StatusUnauthorized { + t.Fatalf("an admin surface with no login of its own answered %d without credentials", got) + } + if got := ask(t, proxy, "console.example", "/", [2]string{"admin", "wrong"}).StatusCode; got != http.StatusUnauthorized { + t.Fatalf("the wrong password answered %d", got) + } + if got := ask(t, proxy, "console.example", "/", [2]string{"admin", "correct horse"}).StatusCode; got != http.StatusOK { + t.Fatalf("the right password answered %d", got) + } +} + +// The negative case ADR 0108 promised would be refused: a credential in the declaration. +// +// **Refused whole, not tolerated and not served unprotected.** A hash carried in a declaration was +// the rejected option; nothing in the running system should quietly accept it later, because the +// precedent is far easier to set than to withdraw. If this test is deleted the option returns and +// nothing else notices. +func TestACredentialInTheDeclarationIsRefusedRatherThanServed(t *testing.T) { + inline := []string{ + `{"given":[{"from":"c","node":"n","at":"127.0.0.1","values":{"name":"c.example","port":8080,"auth":"admin:$2a$10$abcdefghijklmnopqrstuv"}}]}`, + `{"given":[{"from":"c","node":"n","at":"127.0.0.1","values":{"name":"c.example","port":8080,"auth":"$2a$10$abcdefghijklmnopqrstuv"}}]}`, + } + for _, body := range inline { + path := filepath.Join(t.TempDir(), "routes.json") + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + routes, err := routesFrom(path) + if err != nil { + t.Fatal(err) + } + if len(routes) != 0 { + t.Fatalf("a declaration carrying a credential was served anyway: %v", routes) + } + } +} + +// Authentication declared, secret unreadable: the route refuses. It does not serve unprotected. +// +// **Fail closed.** The alternative turns a missing file into a silently public admin surface, which +// is the outcome the whole record exists to prevent. It answers rather than 404s, so an operator +// sees "cannot read the credentials" instead of concluding the route was withdrawn. +func TestAnUnreadableSecretFailsClosed(t *testing.T) { + at, port := served(t, "the console") + missing := filepath.Join(t.TempDir(), "not-mounted") + + proxy := proxyFor(t, `{"given":[ + {"from":"console","node":"anchor","at":"`+at+`","values":{"name":"console.example","port":`+strconv.Itoa(port)+`,"auth":"`+missing+`"}} + ]}`) + + answer := ask(t, proxy, "console.example", "/", [2]string{}) + if answer.StatusCode == http.StatusOK { + t.Fatal("a route whose credentials could not be read served the workload unprotected") + } + if answer.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected the route to say it cannot check, got %d", answer.StatusCode) + } +} + +// Equal priorities resolve the same way every time, so the same declaration serves the same way +// after a restart. +// +// Sorting only by priority leaves rules that share one in whatever order the map produced. The +// proxy would still work, and would work differently between restarts — which is the hardest kind +// of fault to believe when it is reported. +func TestRulesThatShareAPriorityAreStillTotallyOrdered(t *testing.T) { + first := []rule{ + {path: "/a", priority: 10, target: "http://x:1"}, + {path: "/bb", priority: 10, target: "http://y:2"}, + {path: "", priority: 10, target: "http://z:3"}, + } + second := []rule{ + {path: "", priority: 10, target: "http://z:3"}, + {path: "/bb", priority: 10, target: "http://y:2"}, + {path: "/a", priority: 10, target: "http://x:1"}, + } + inOrder(first) + inOrder(second) + for i := range first { + if first[i].path != second[i].path || first[i].target != second[i].target { + t.Fatalf("two orderings of the same rules disagree at %d: %q vs %q", + i, first[i].path, second[i].path) + } + } + // And the more specific rule is matched first, which is the intuitive reading. + if first[0].path != "/bb" { + t.Fatalf("the longest path is not matched first: %q", first[0].path) + } +} + +// Priority decides before path length does, so a rule can be made to win regardless of specificity. +func TestPriorityOutranksPathLength(t *testing.T) { + rules := []rule{ + {path: "/very/long/path", priority: 1, target: "http://x:1"}, + {path: "", priority: 100, target: "http://y:2"}, + } + inOrder(rules) + if rules[0].priority != 100 { + t.Fatalf("a higher priority did not win: %+v", rules[0]) + } +} diff --git a/examples/route-proxy/routes_test.go b/examples/route-proxy/routes_test.go index 6740569..c4d6ef2 100644 --- a/examples/route-proxy/routes_test.go +++ b/examples/route-proxy/routes_test.go @@ -19,6 +19,23 @@ func write(t *testing.T, body string) string { return path } +// plain is the table an ordinary set of routes makes: one host, one target, no policy. +func plain(routes map[string]string) map[string][]rule { + out := map[string][]rule{} + for host, target := range routes { + out[host] = []rule{{target: target}} + } + return out +} + +// targetOf is where a host's first matching rule sends a request. +func targetOf(routes map[string][]rule, host string) string { + if rules := routes[host]; len(rules) > 0 { + return rules[0].target + } + return "" +} + // A route is a grant: the consumer supplies a target, and where that machine is comes from the // mesh rather than from a naming convention the proxy has to know. func TestARouteGoesToWhereTheMeshSaysTheConsumerIs(t *testing.T) { @@ -30,7 +47,7 @@ func TestARouteGoesToWhereTheMeshSaysTheConsumerIs(t *testing.T) { } // Lower-cased, because a Host header is not case-sensitive and a route that only answers the // spelling in the manifest answers half the requests made to it. - if routes["app.example"] != "http://laptop.internal:8080" { + if targetOf(routes, "app.example") != "http://laptop.internal:8080" { t.Fatalf("the route does not point at the consumer: %v", routes) } } @@ -44,7 +61,7 @@ func TestAConsumerOnTheProxysOwnMachineIsReachedOverLoopback(t *testing.T) { if err != nil { t.Fatal(err) } - if routes["app.example"] != "http://127.0.0.1:9000" { + if targetOf(routes, "app.example") != "http://127.0.0.1:9000" { t.Fatalf("a workload on this machine was not reachable: %v", routes) } } @@ -59,7 +76,7 @@ func TestAContributionMissingWhatARouteNeedsIsSkipped(t *testing.T) { if err != nil { t.Fatal(err) } - if len(routes) != 1 || routes["fine.example"] == "" { + if len(routes) != 1 || targetOf(routes, "fine.example") == "" { t.Fatalf("an unusable contribution was served: %v", routes) } } @@ -75,7 +92,7 @@ func TestTheProxyReachesTheWorkloadAndNamesWhatItServes(t *testing.T) { host, port, _ := strings.Cut(target, ":") held := newTable() - held.set(map[string]string{"app.example": "http://" + host + ":" + port}) + held.set(plain(map[string]string{"app.example": "http://" + host + ":" + port})) proxy := httptest.NewServer(handler(held)) defer proxy.Close() @@ -120,16 +137,16 @@ func TestTheProxyReachesTheWorkloadAndNamesWhatItServes(t *testing.T) { // nothing fails more visibly than a stale grant, which is exactly why it must not survive. func TestWithdrawingARouteStopsServingIt(t *testing.T) { held := newTable() - held.set(map[string]string{ + held.set(plain(map[string]string{ "going.example": "http://a.internal:80", "staying.example": "http://b.internal:80", - }) - held.set(map[string]string{"staying.example": "http://b.internal:80"}) + })) + held.set(plain(map[string]string{"staying.example": "http://b.internal:80"})) - if _, still := held.find("going.example"); still { + if _, still := held.find("going.example", "/"); still { t.Fatal("a route whose module was unassigned is still served") } - if _, kept := held.find("staying.example"); !kept { + if _, kept := held.find("staying.example", "/"); !kept { t.Fatal("withdrawing one route took another with it") } } @@ -137,8 +154,8 @@ func TestWithdrawingARouteStopsServingIt(t *testing.T) { // A Host header carries a port and the name does not. func TestARequestNamingAPortStillFindsItsRoute(t *testing.T) { held := newTable() - held.set(map[string]string{"app.example": "http://a.internal:8080"}) - if _, found := held.find("app.example:8080"); !found { + held.set(plain(map[string]string{"app.example": "http://a.internal:8080"})) + if _, found := held.find("app.example:8080", "/"); !found { t.Fatal("a request to app.example:8080 did not find the route for app.example") } } @@ -168,7 +185,7 @@ func TestTheIssuerIsStagingUnlessNamed(t *testing.T) { // rate limit — and the proxy would look healthy throughout. func TestNoCertificateIsAskedForOnAnUnroutedName(t *testing.T) { held := newTable() - held.set(map[string]string{"photos.example": "http://127.0.0.1:8080"}) + held.set(plain(map[string]string{"photos.example": "http://127.0.0.1:8080"})) policy := onlyWhatTheMeshSaid(held) if err := policy(context.Background(), "photos.example"); err != nil { @@ -184,7 +201,7 @@ func TestNoCertificateIsAskedForOnAnUnroutedName(t *testing.T) { // A route withdrawn stops being certifiable, without the proxy restarting. func TestWithdrawingARouteWithdrawsItsCertificate(t *testing.T) { held := newTable() - held.set(map[string]string{"photos.example": "http://127.0.0.1:8080"}) + held.set(plain(map[string]string{"photos.example": "http://127.0.0.1:8080"})) policy := onlyWhatTheMeshSaid(held) if err := policy(context.Background(), "photos.example"); err != nil { t.Fatal(err) From e11e1374bd37df6035d14e3c4a79cc83eff430bd Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 25 Sep 2026 14:20:06 +0200 Subject: [PATCH 05/14] route-proxy: a priority is an ordering, not a port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found reviewing my own change before merging it, and it was load-bearing rather than cosmetic. Priority was read with asPort, which caps at 65535. A rule declared above that silently became priority 0 and stopped shadowing the route it exists to shadow. The one real rule this has to reproduce is declared at 100000 — so path scoping and refusal would both have shipped looking complete, passing their tests, and doing nothing on the only case that motivated them. A priority is an ordering and has no range. asWhole takes any whole number the mesh wrote and rejects a non-integral one, which was not meant as a priority. Also: a host may now be routed on some paths and not others, which made the 404 dishonest — it said "no route for this name" while listing that very name as served, a contradiction an operator has to disbelieve the proxy to get past. An uncovered path now says so, and a name that is genuinely not served still lists what is. Two regression tests, both through the proxy rather than against the parser, because the parser was where the bug looked fine. --- examples/route-proxy/main.go | 31 +++++++++++++++++++++- examples/route-proxy/policy_test.go | 40 +++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/examples/route-proxy/main.go b/examples/route-proxy/main.go index 10d7217..9b59643 100644 --- a/examples/route-proxy/main.go +++ b/examples/route-proxy/main.go @@ -416,8 +416,17 @@ func handler(held *table) http.Handler { // **Named, not a bare 404.** A route that was withdrawn and a name that never existed // are different things, and a proxy that says only "not found" makes an operator go // and read the mesh to tell them apart. What it is serving is the answer to both. + // + // And since a host may now be routed only on some paths, those are a third thing: + // saying "no route for this name" while listing that very name as served is a + // contradiction an operator would have to disbelieve the proxy to get past. w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusNotFound) + if held.routed(r.Host) { + fmt.Fprintf(w, "%s is served here, but no route covers %q.\n", + bareHost(r.Host), r.URL.Path) + return + } fmt.Fprintf(w, "no route for %q in this mesh.\nserving: %s\n", r.Host, strings.Join(held.names(), ", ")) return @@ -524,7 +533,7 @@ func routesFrom(path string) (map[string][]rule, error) { host := strings.ToLower(name) made := rule{path: asPath(c.Values["path"])} - if p, ok := asPort(c.Values["priority"]); ok { + if p, ok := asWhole(c.Values["priority"]); ok { made.priority = p } made.policy.deny, _ = c.Values["deny"].(bool) @@ -574,6 +583,26 @@ func routesFrom(path string) (map[string][]rule, error) { return out, nil } +// asWhole is any whole number the mesh wrote, whatever its magnitude. +// +// **Not asPort.** Priority was read with the port reader first, which caps at 65535 — so a rule +// declared at a priority above that silently became priority 0 and stopped shadowing the route it +// exists to shadow. The one real rule this has to reproduce is declared at 100000, so the bug was +// exactly load-bearing. A priority is an ordering, not a port: it has no range. +func asWhole(v any) (int, bool) { + switch n := v.(type) { + case float64: + // JSON makes a float of every number, so a non-integral one was not meant as a priority. + if n != float64(int(n)) { + return 0, false + } + return int(n), true + case int: + return n, true + } + return 0, false +} + // asPath is the path prefix a rule is scoped to, or "" for every path. func asPath(v any) string { p, _ := v.(string) diff --git a/examples/route-proxy/policy_test.go b/examples/route-proxy/policy_test.go index 7f98ec4..db1dc0b 100644 --- a/examples/route-proxy/policy_test.go +++ b/examples/route-proxy/policy_test.go @@ -231,3 +231,43 @@ func TestPriorityOutranksPathLength(t *testing.T) { t.Fatalf("a higher priority did not win: %+v", rules[0]) } } + +// A priority above a port number survives, because a priority is an ordering and not a port. +// +// **Found by review, and it was load-bearing.** Priority was first read with the port reader, which +// caps at 65535 — so a rule declared above that silently became priority 0 and stopped shadowing the +// route it exists to shadow. The one real rule this has to reproduce is declared at 100000, so the +// capability would have shipped looking complete and doing nothing. +func TestAPriorityAboveAPortNumberSurvives(t *testing.T) { + at, port := served(t, "the workload") + proxy := proxyFor(t, `{"given":[ + {"from":"forge","node":"anchor","at":"`+at+`","values":{"name":"forge.example","port":`+strconv.Itoa(port)+`}}, + {"from":"forge","node":"anchor","values":{"name":"forge.example","path":"/api/internal","priority":100000,"deny":true}} + ]}`) + + if got := ask(t, proxy, "forge.example", "/api/internal/hook", [2]string{}).StatusCode; got != http.StatusForbidden { + t.Fatalf("a rule declared at priority 100000 answered %d instead of refusing", got) + } +} + +// A host routed only on some paths says so, rather than claiming the name is not served here. +// +// Saying "no route for this name" while listing that very name as served is a contradiction an +// operator has to disbelieve the proxy to get past — and path scoping makes it reachable, because a +// host can now have rules that none of this request's paths match. +func TestAHostRoutedOnlyOnSomePathsSaysSo(t *testing.T) { + proxy := proxyFor(t, `{"given":[ + {"from":"forge","node":"anchor","values":{"name":"forge.example","path":"/api/internal","deny":true}} + ]}`) + + answer := ask(t, proxy, "forge.example", "/elsewhere", [2]string{}) + if answer.StatusCode != http.StatusNotFound { + t.Fatalf("an uncovered path answered %d", answer.StatusCode) + } + body := make([]byte, 256) + n, _ := answer.Body.Read(body) + said := string(body[:n]) + if !strings.Contains(said, "is served here") || !strings.Contains(said, "/elsewhere") { + t.Fatalf("the refusal does not distinguish an uncovered path from an unserved name: %q", said) + } +} From f996a6707eab2e1959c29b52ece8b3c5cef06e48 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 25 Sep 2026 16:51:38 +0200 Subject: [PATCH 06/14] a route composes its internal-network alias too, not only its public name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every cutover done on novox tonight (drive, files, files-api, git, keycloak, umami) dropped the