From 9bed7d6398e2872624b59ae361604d86a4779cfe Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 05:25:29 +0200 Subject: [PATCH 1/8] broker: grant each credential the tool calls that name it as the caller, and no other (hq issue 365) Every grant to call a tool is granted again under the call kind, with the credential's own bus user as the last token, and every grant to answer one is granted for calls naming any caller: the caller of a tool call becomes a fact the bus enforces, as ADR 0259 section 3 made the asker of an ask. A kind of its own because every existing tool grant is a wildcard that would match any caller appended. The old tool grants stay for one release so nothing loses its way to a tool (hq issue 464); the controller's own grants move with that issue, since they are also the installer's first user list. --- internal/broker/callers.go | 117 ++++++++++++++++++++++++ internal/broker/callers_live_test.go | 100 +++++++++++++++++++++ internal/broker/callers_test.go | 119 +++++++++++++++++++++++++ internal/broker/invokes_test.go | 5 +- internal/broker/nats.go | 17 ++++ internal/broker/nats_test.go | 6 +- internal/broker/testdata/composed.conf | 6 +- 7 files changed, 362 insertions(+), 8 deletions(-) create mode 100644 internal/broker/callers.go create mode 100644 internal/broker/callers_live_test.go create mode 100644 internal/broker/callers_test.go diff --git a/internal/broker/callers.go b/internal/broker/callers.go new file mode 100644 index 00000000..24392b58 --- /dev/null +++ b/internal/broker/callers.go @@ -0,0 +1,117 @@ +package broker + +import ( + "regexp" + "strings" +) + +// A tool call names its caller (novox/hq issue 365, by ADR 0259 §3's precedent for asks). +// +// mesh.mod..call.[.]. +// mesh.seat..call.[.]. +// +// The last token is the bus user that published the call, and each user is granted these subjects with its own +// name there and no other — the caller is a fact the server enforces, and the tool runtime hands it to the +// module from the subject a call arrived on (mesh-tools node-tools internal/bus caller.go holds the same shape). +// +// **A kind of its own, `call`, not the `tool` subject with a token appended.** Every grant to call a tool is a +// wildcard over the `tool` kind — `.tool..*` for the instance on any machine, `mesh.mod.*.tool.>` for every +// tool — and either would match a `tool` subject with any caller appended, so a caller could name another. +// Under `call` nothing is granted but the caller-named subjects. No stream's filter covers it: a tool call is +// never persisted (design 25 §3). +// +// **Derived from the `tool` grants, in one place** (callerNamed): wherever a principal may call or answer a +// tool, it may call it in its own name, or answer it naming any caller — so no kind of principal is left +// unable to call in its own name, and a new grant to call is one in both shapes by construction. +// +// The `tool` grants stay beside these for one release, so every caller and every runtime moves without a gap: +// their retirement is hq issue 464. +const CallKind = "call" + +var userName = regexp.MustCompile(`^[A-Za-z0-9_-]+(\.[A-Za-z0-9_-]+)*$`) + +// CallerToken is a bus user's name as the last token of a call it publishes: each dot written `~`, which no part +// of a user's name may hold (safeSubject), so the token names that user and no other. "" for a name that is not +// a user's. +func CallerToken(user string) string { + if !userName.MatchString(user) { + return "" + } + return strings.ReplaceAll(user, ".", "~") +} + +// toolGrant splits a grant on the `tool` kind — `mesh.mod..tool.` or `mesh.seat..tool.`, any +// part possibly a wildcard — at the kind; ok is false for any other subject. +func toolGrant(subject string) (head, rest string, ok bool) { + parts := strings.SplitN(subject, ".", 5) + if len(parts) != 5 || parts[0] != "mesh" || (parts[1] != "mod" && parts[1] != "seat") || parts[3] != "tool" || + parts[2] == "" || parts[4] == "" { + return "", "", false + } + return parts[0] + "." + parts[1] + "." + parts[2], parts[4], true +} + +// CalledSubject is where a call to a tool subject goes naming its caller; "" when the subject is not a tool's +// or the user is not a bus user. +func CalledSubject(subject, user string) string { + head, rest, ok := toolGrant(subject) + token := CallerToken(user) + if !ok || token == "" || strings.ContainsAny(rest, "*>") { + return "" + } + return head + "." + CallKind + "." + rest + "." + token +} + +// CalledPattern is what a holder answering a tool subject also subscribes, to hear the calls that name their +// caller: the same address under the `call` kind, any caller last. "" for a subject that is not a tool's. +func CalledPattern(subject string) string { + head, rest, ok := toolGrant(subject) + if !ok || strings.ContainsAny(rest, "*>") { + return "" + } + return head + "." + CallKind + "." + rest + ".*" +} + +// calledPublish is a grant to call on the `tool` kind, as the same grant in the caller's own name: its last +// token the caller's, and nothing that reaches past it. A `>` is every tail a call carries — the tool alone or +// the tool and the machine — so it becomes both, each ending in the caller. +func calledPublish(grant, token string) []string { + head, rest, ok := toolGrant(grant) + if !ok || token == "" { + return nil + } + base := head + "." + CallKind + "." + switch { + case rest == ">": + return []string{base + "*." + token, base + "*.*." + token} + case strings.HasSuffix(rest, ".>"): + return []string{base + strings.TrimSuffix(rest, ">") + "*." + token} + } + return []string{base + rest + "." + token} +} + +// calledSubscribe is a grant to answer on the `tool` kind, as the same grant for the calls naming any caller. +func calledSubscribe(grant string) []string { + head, rest, ok := toolGrant(grant) + if !ok { + return nil + } + base := head + "." + CallKind + "." + if strings.HasSuffix(rest, ">") { + return []string{base + rest} + } + return []string{base + rest + ".*"} +} + +// callerNamed adds, beside a principal's grants on the `tool` kind, the same grants under `call`: to publish in +// its own name, to subscribe naming anybody. +func callerNamed(user string, pub, sub []string) ([]string, []string) { + token := CallerToken(user) + for _, g := range pub { + pub = append(pub, calledPublish(g, token)...) + } + for _, g := range sub { + sub = append(sub, calledSubscribe(g)...) + } + return unique(pub), unique(sub) +} diff --git a/internal/broker/callers_live_test.go b/internal/broker/callers_live_test.go new file mode 100644 index 00000000..cacb0424 --- /dev/null +++ b/internal/broker/callers_live_test.go @@ -0,0 +1,100 @@ +package broker + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/nats-io/nats-server/v2/server" + "github.com/nats-io/nats.go" + "golang.org/x/crypto/bcrypt" +) + +// **On a real server, as composed** (novox/hq issue 365): a machine's runtime calls in its own name, and the +// server refuses it a call naming another — the grant, not the runtime, is what makes the caller a fact. +func TestAServerComposedFromTheGrantsRefusesACallNamingAnother(t *testing.T) { + hash, err := bcrypt.GenerateFromPassword([]byte("pw"), bcrypt.MinCost) + if err != nil { + t.Fatal(err) + } + ledger := Seat{Name: "issue-tracker", Scope: "mesh", Serves: []string{"open"}} + accounts, err := ComposeAccounts([]Principal{ + {Kind: KindNodeTools, Node: "novox", Module: RuntimeModule, PasswordHash: string(hash), + Carries: []Declared{{Module: "mesh-issues", Holds: []Seat{ledger}}}}, + {Kind: KindNodeTools, Node: "shanks", Module: RuntimeModule, PasswordHash: string(hash)}, + }) + if err != nil { + t.Fatal(err) + } + conf := filepath.Join(t.TempDir(), "bus.conf") + if err := os.WriteFile(conf, []byte("listen: 127.0.0.1:-1\njetstream { store_dir: "+ + `"`+t.TempDir()+`"`+" }\n"+accounts), 0o600); err != nil { + t.Fatal(err) + } + opts, err := server.ProcessConfigFile(conf) + if err != nil { + t.Fatalf("the composed accounts do not parse: %v", err) + } + opts.NoLog, opts.NoSigs = true, true + s, err := server.NewServer(opts) + if err != nil { + t.Fatal(err) + } + go s.Start() + if !s.ReadyForConnections(10 * time.Second) { + t.Fatal("the bus did not come up") + } + defer s.Shutdown() + + holder, err := nats.Connect(s.ClientURL(), nats.UserInfo("novox.node-tools", "pw")) + if err != nil { + t.Fatal(err) + } + defer holder.Close() + heard := make(chan string, 4) + sub, err := holder.Subscribe("mesh.seat.issue-tracker.call.open.*", func(m *nats.Msg) { + heard <- m.Subject + _ = m.Respond([]byte(`{"result":{}}`)) + }) + if err != nil { + t.Fatal(err) + } + _ = holder.Flush() + if !sub.IsValid() { + t.Fatal("the holder may not hear the caller-named calls to its seat") + } + + refusals := make(chan error, 4) + caller, err := nats.Connect(s.ClientURL(), nats.UserInfo("shanks.node-tools", "pw"), + nats.CustomInboxPrefix("_INBOX.shanks.node-tools"), + nats.ErrorHandler(func(_ *nats.Conn, _ *nats.Subscription, err error) { refusals <- err })) + if err != nil { + t.Fatal(err) + } + defer caller.Close() + if _, err := caller.Request("mesh.seat.issue-tracker.call.open.shanks~node-tools", []byte(`{}`), 3*time.Second); err != nil { + t.Fatalf("a call in the caller's own name was not answered: %v", err) + } + if got := <-heard; got != "mesh.seat.issue-tracker.call.open.shanks~node-tools" { + t.Fatalf("heard %s", got) + } + if err := caller.Publish("mesh.seat.issue-tracker.call.open.novox~node-tools", []byte(`{}`)); err != nil { + t.Fatal(err) + } + _ = caller.Flush() + select { + case err := <-refusals: + if !errors.Is(err, nats.ErrPermissionViolation) { + t.Errorf("the server said %v, want a permissions violation", err) + } + case <-time.After(3 * time.Second): + t.Error("the server did not refuse a call naming another caller") + } + select { + case got := <-heard: + t.Errorf("a call naming another reached the holder: %s", got) + case <-time.After(200 * time.Millisecond): + } +} diff --git a/internal/broker/callers_test.go b/internal/broker/callers_test.go new file mode 100644 index 00000000..f11cac30 --- /dev/null +++ b/internal/broker/callers_test.go @@ -0,0 +1,119 @@ +package broker + +import ( + "strings" + "testing" +) + +// **A tool call names its caller, and the bus lets each user name itself alone** (novox/hq issue 365, by ADR +// 0259 §3's precedent for asks): wherever a principal may call a tool, it may call it on the caller-named +// subject — kind `call`, its own bus user last, dots written `~` — and on no subject naming anybody else. +func TestEachCredentialMayCallOnlyInItsOwnName(t *testing.T) { + ledger := Seat{Name: "node-desk", Scope: "node", Serves: []string{"who"}} + tracker := Seat{Name: "issue-tracker", Scope: "mesh", Serves: []string{"open"}} + for _, c := range []struct { + p Principal + may []string + mayNot []string + subject []string // what it subscribes, to answer calls naming any caller + }{ + {p: Principal{Kind: KindPerson, Module: "jochen", Invokes: []string{"*"}}, + may: []string{"mesh.mod.ledger.call.who.person~jochen", "mesh.mod.ledger.call.who.anchor.person~jochen", + "mesh.seat.issue-tracker.call.open.person~jochen", "mesh.seat.node-desk.call.who.anchor.person~jochen"}, + mayNot: []string{"mesh.mod.ledger.call.who.shanks~node-tools", "mesh.mod.ledger.call.who.anchor.controller", + "mesh.seat.issue-tracker.call.open.person~somebody", "mesh.mod.ledger.call.who.person"}}, + {p: Principal{Kind: KindNodeTools, Node: "shanks", Module: RuntimeModule, + Carries: []Declared{{Module: "ledger", Holds: []Seat{ledger, tracker}}}}, + may: []string{"mesh.mod.ledger.call.who.shanks~node-tools", "mesh.seat.issue-tracker.call.open.shanks~node-tools"}, + mayNot: []string{"mesh.mod.ledger.call.who.novox~node-tools", "mesh.seat.issue-tracker.call.open.controller"}, + subject: []string{"mesh.mod.ledger.call.who.novox~node-tools", "mesh.seat.node-desk.call.who.shanks.person~jochen", "mesh.seat.issue-tracker.call.open.controller"}}, + {p: Principal{Kind: KindModule, Node: "two", Module: "shop", Invokes: []string{"ledger.who", "seat:issue-tracker.open"}}, + may: []string{"mesh.mod.ledger.call.who.two~shop", "mesh.mod.ledger.call.who.anchor.two~shop", + "mesh.seat.issue-tracker.call.open.two~shop"}, + mayNot: []string{"mesh.mod.ledger.call.other.two~shop", "mesh.mod.ledger.call.who.one~shop", + "mesh.seat.issue-tracker.call.open.one~telegram"}}, + {p: Principal{Kind: KindNode, Node: "one", Checks: []string{"ledger.health"}}, + may: []string{"mesh.mod.ledger.call.health.one.node~one"}, + mayNot: []string{"mesh.mod.ledger.call.health.two.node~one", "mesh.mod.ledger.call.health.one.node~two"}}, + {p: Principal{Kind: KindModule, Node: "one", Module: "ledger", Holds: []Seat{ledger, tracker}}, + subject: []string{"mesh.mod.ledger.call.who.person~jochen", "mesh.mod.ledger.call.who.one.controller", + "mesh.seat.node-desk.call.who.one.two~shop", "mesh.seat.issue-tracker.call.open.two~shop"}}, + } { + perms, err := PermissionsFor(c.p) + if err != nil { + t.Fatalf("%s: %v", c.p.Username(), err) + } + for _, s := range c.may { + if !MayPublish(perms, s) { + t.Errorf("%s may not call %s, in its own name", c.p.Username(), s) + } + } + for _, s := range c.mayNot { + if MayPublish(perms, s) { + t.Errorf("%s may call %s, naming somebody else", c.p.Username(), s) + } + } + for _, s := range c.subject { + if !MaySubscribe(perms, s) { + t.Errorf("%s does not hear %s, a call to what it serves", c.p.Username(), s) + } + } + } +} + +// The subjects that name no caller stay granted beside the caller-named ones for one release, so a caller +// moves over without a gap (their retirement: hq issue 464). +func TestTheSubjectsThatNameNoCallerStayGrantedForOneRelease(t *testing.T) { + perms, err := PermissionsFor(Principal{Kind: KindModule, Node: "two", Module: "shop", Invokes: []string{"ledger.who"}}) + if err != nil { + t.Fatal(err) + } + for _, s := range []string{"mesh.mod.ledger.tool.who", "mesh.mod.ledger.tool.who.anchor"} { + if !MayPublish(perms, s) { + t.Errorf("the old subject %s is no longer granted", s) + } + } +} + +// The desk's hidden prompt is the controller's alone on the caller-named subjects too (the review of +// 2026-10-09, M4): a grant of every tool does not reach it there either. +func TestTheDesksPromptIsTheControllersAloneUnderCallToo(t *testing.T) { + for _, p := range []Principal{{Kind: KindPerson, Module: "jochen", Invokes: []string{"*"}}, + {Kind: KindNodeTools, Node: "shanks", Module: RuntimeModule}} { + perms, err := PermissionsFor(p) + if err != nil { + t.Fatal(err) + } + token := CallerToken(p.Username()) + for _, s := range []string{"mesh.seat.node-launcher.call.secret.shanks." + token, + "mesh.mod.shell.call.node-launcher.secret.shanks." + token, "mesh.mod.shell.call.node-launcher.secret." + token} { + if MayPublish(perms, s) { + t.Errorf("%s may publish %s, the desk's hidden prompt", p.Username(), s) + } + } + } +} + +// The controller's grants are the installer's first user list too, so they move with hq issue 464: this release +// it calls and serves on the subjects that name no caller alone, and nobody may call in its name. +func TestTheControllerKeepsTheSubjectsThatNameNoCallerThisRelease(t *testing.T) { + perms, err := PermissionsFor(Principal{Kind: KindController}) + if err != nil { + t.Fatal(err) + } + for _, s := range append(perms.Publish, perms.Subscribe...) { + if strings.Contains(s, "."+CallKind+".") { + t.Errorf("the controller is granted %s, which the installer's first user list does not carry", s) + } + } + for _, p := range []Principal{{Kind: KindPerson, Module: "jochen", Invokes: []string{"*"}}, + {Kind: KindNodeTools, Node: "shanks", Module: RuntimeModule}} { + perms, err := PermissionsFor(p) + if err != nil { + t.Fatal(err) + } + if MayPublish(perms, "mesh.mod.ledger.call.who.controller") || MayPublish(perms, "mesh.seat.mesh-controller.call.status.controller") { + t.Errorf("%s may call in the controller's name", p.Username()) + } + } +} diff --git a/internal/broker/invokes_test.go b/internal/broker/invokes_test.go index 3d1e86ca..c22c12e8 100644 --- a/internal/broker/invokes_test.go +++ b/internal/broker/invokes_test.go @@ -42,8 +42,9 @@ func TestInvokingGrantsNothingButTheCall(t *testing.T) { if strings.Contains(p, ".event.") { t.Errorf("a module that only invokes may publish %q, an event it never declared", p) } - // A role's tools are tools (ADR 0132); a role's work queue and events are not. - if strings.HasPrefix(p, "mesh.seat.") && !strings.Contains(p, ".tool.") { + // A role's tools are tools (ADR 0132), named by their caller or not (novox/hq issue 365); a role's work + // queue and events are not. + if strings.HasPrefix(p, "mesh.seat.") && !strings.Contains(p, ".tool.") && !strings.Contains(p, ".call.") { t.Errorf("a module that only invokes may publish %q, a seat it neither holds nor uses", p) } } diff --git a/internal/broker/nats.go b/internal/broker/nats.go index d46caf7f..659a9cc8 100644 --- a/internal/broker/nats.go +++ b/internal/broker/nats.go @@ -212,6 +212,10 @@ func ControllerOnly() []string { for _, base := range []string{"mesh.seat." + v.Seat + ".tool." + v.Verb, "mesh.mod.*.tool." + v.Seat + "." + v.Verb} { out = append(out, base, base+".*") } + // And where a call names its caller (novox/hq issue 365): any machine, any caller. + for _, base := range []string{"mesh.seat." + v.Seat + "." + CallKind + "." + v.Verb, "mesh.mod.*." + CallKind + "." + v.Seat + "." + v.Verb} { + out = append(out, base+".>") + } } return out } @@ -875,6 +879,19 @@ func PermissionsFor(p Principal) (Permissions, error) { pub = append(pub, "$JS.ACK."+consumerStream(p)+"."+consumerDurable(p)+".>") } + // **And every tool grant again, naming its caller** (novox/hq issue 365): a call in this principal's own + // name, and an answer to a call naming anybody. The `tool` grants above stay for one release beside these, + // so callers and runtimes move without a gap; their retirement is hq issue 464. + // + // **Not the controller's, this release.** Its grants are also the installer's first user list + // (TestTheInstallersFirstUserListIsWhatTheControllerWouldCompose), judged against the node-engine the mesh + // runs, so a change to them waits on a node-engine delivery. It calls and serves on the subjects that name + // no caller meanwhile — a caller-named call to its seat reaches nobody and is asked again on those at once — + // and moves with issue 464. + if p.Kind != KindController { + pub, sub = callerNamed(p.Username(), pub, sub) + } + sort.Strings(pub) sort.Strings(sub) // One writer per piece of state (novox/hq to-be 45 §1): a grant that would make a second is diff --git a/internal/broker/nats_test.go b/internal/broker/nats_test.go index 23d3f8d3..bfd52cfb 100644 --- a/internal/broker/nats_test.go +++ b/internal/broker/nats_test.go @@ -240,9 +240,9 @@ func TestAPersonReachesNothingButTools(t *testing.T) { perms, _ := PermissionsFor(Principal{Kind: KindPerson, Module: "jo", Invokes: []string{"*"}, PasswordHash: "x"}) for _, p := range perms.Publish { - // A tool call, or asking what answers (novox/hq ADR 0197) — a question every service - // answers about itself, which claims nothing and controls nothing. - if !strings.Contains(p, ".tool.") && !strings.HasPrefix(p, "$SRV.") { + // A tool call — named by its caller or not (novox/hq issue 365) — or asking what answers (novox/hq + // ADR 0197), a question every service answers about itself, which claims nothing and controls nothing. + if !strings.Contains(p, ".tool.") && !strings.Contains(p, ".call.") && !strings.HasPrefix(p, "$SRV.") { t.Errorf("a person may publish %q, which is not a tool call", p) } } diff --git a/internal/broker/testdata/composed.conf b/internal/broker/testdata/composed.conf index 0919b2b8..ea22d3c5 100644 --- a/internal/broker/testdata/composed.conf +++ b/internal/broker/testdata/composed.conf @@ -43,17 +43,17 @@ accounts { } } { user: "one.telegram", password: "$2a$11$tttttttttttttttttttttt", permissions: { publish: { allow: ["$JS.ACK.EVENTS.one_telegram.>", "$JS.ACK.SEAT_TELEGRAM_SENDER.SEAT_TELEGRAM_SENDER_worker.>", "$JS.API.CONSUMER.INFO.EVENTS.one_telegram", "$JS.API.CONSUMER.INFO.SEAT_TELEGRAM_SENDER.SEAT_TELEGRAM_SENDER_worker", "$JS.API.CONSUMER.MSG.NEXT.EVENTS.one_telegram", "$JS.API.CONSUMER.MSG.NEXT.SEAT_TELEGRAM_SENDER.SEAT_TELEGRAM_SENDER_worker", "$JS.API.DIRECT.GET.ASSIGNMENTS.mesh.assignment.one.telegram", "mesh.seat.telegram-sender.event.delivered", "mesh.seat.telegram-sender.event.failed"] } - subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.telegram", "$SRV.INFO.telegram.>", "$SRV.PING", "$SRV.PING.telegram", "$SRV.PING.telegram.>", "$SRV.STATS", "$SRV.STATS.telegram", "$SRV.STATS.telegram.>", "_INBOX.one.telegram.>", "mesh.assignment.one.telegram", "mesh.mod.telegram.tool.>", "mesh.seat.telegram-sender.accept.send"] } + subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.telegram", "$SRV.INFO.telegram.>", "$SRV.PING", "$SRV.PING.telegram", "$SRV.PING.telegram.>", "$SRV.STATS", "$SRV.STATS.telegram", "$SRV.STATS.telegram.>", "_INBOX.one.telegram.>", "mesh.assignment.one.telegram", "mesh.mod.telegram.call.>", "mesh.mod.telegram.tool.>", "mesh.seat.telegram-sender.accept.send"] } allow_responses: { max: 1, ttl: "1m" } } } { user: "two.audit", password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaa", permissions: { publish: { allow: ["$JS.ACK.EVENTS.two_audit.>", "$JS.API.CONSUMER.INFO.EVENTS.two_audit", "$JS.API.CONSUMER.MSG.NEXT.EVENTS.two_audit", "$JS.API.DIRECT.GET.ASSIGNMENTS.mesh.assignment.two.audit"] } - subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.audit", "$SRV.INFO.audit.>", "$SRV.PING", "$SRV.PING.audit", "$SRV.PING.audit.>", "$SRV.STATS", "$SRV.STATS.audit", "$SRV.STATS.audit.>", "_INBOX.two.audit.>", "mesh.assignment.two.audit", "mesh.mod.audit.tool.>", "mesh.mod.shop.event.order.placed"] } + subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.audit", "$SRV.INFO.audit.>", "$SRV.PING", "$SRV.PING.audit", "$SRV.PING.audit.>", "$SRV.STATS", "$SRV.STATS.audit", "$SRV.STATS.audit.>", "_INBOX.two.audit.>", "mesh.assignment.two.audit", "mesh.mod.audit.call.>", "mesh.mod.audit.tool.>", "mesh.mod.shop.event.order.placed"] } allow_responses: { max: 1, ttl: "1m" } } } { user: "two.shop", password: "$2a$11$ssssssssssssssssssssss", permissions: { publish: { allow: ["$JS.ACK.EVENTS.two_shop.>", "$JS.API.CONSUMER.INFO.EVENTS.two_shop", "$JS.API.CONSUMER.MSG.NEXT.EVENTS.two_shop", "$JS.API.DIRECT.GET.ASSIGNMENTS.mesh.assignment.two.shop", "mesh.mod.shop.event.order.placed", "mesh.seat.telegram-sender.accept.send"] } - subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.shop", "$SRV.INFO.shop.>", "$SRV.PING", "$SRV.PING.shop", "$SRV.PING.shop.>", "$SRV.STATS", "$SRV.STATS.shop", "$SRV.STATS.shop.>", "_INBOX.two.shop.>", "mesh.assignment.two.shop", "mesh.mod.shop.tool.>"] } + subscribe: { allow: ["$SRV.INFO", "$SRV.INFO.shop", "$SRV.INFO.shop.>", "$SRV.PING", "$SRV.PING.shop", "$SRV.PING.shop.>", "$SRV.STATS", "$SRV.STATS.shop", "$SRV.STATS.shop.>", "_INBOX.two.shop.>", "mesh.assignment.two.shop", "mesh.mod.shop.call.>", "mesh.mod.shop.tool.>"] } allow_responses: { max: 1, ttl: "1m" } } } ] From ebca7816bf0817bdba189479da21ae065f49a66c Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 05:47:40 +0200 Subject: [PATCH 2/8] inventory: a person's one tool is granted naming that person as the caller too (hq issue 365) --- internal/inventory/people_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/inventory/people_test.go b/internal/inventory/people_test.go index 7df4b15c..5919607e 100644 --- a/internal/inventory/people_test.go +++ b/internal/inventory/people_test.go @@ -44,15 +44,17 @@ func TestAPersonMayCallToolsAndNothingElse(t *testing.T) { } // The one tool, both ways it is addressed (novox/hq ADR 0159): to whichever instance // answers, and to the instance on one machine. Nothing else. - // And asking what answers (novox/hq ADR 0197), which claims nothing and calls nothing. + // And asking what answers (novox/hq ADR 0197), which claims nothing and calls nothing. Each way again + // naming ada as the caller and nobody else (novox/hq issue 365). var tools []string for _, s := range perms.Publish { if !strings.HasPrefix(s, "$SRV.") { tools = append(tools, s) } } - if len(tools) != 2 || tools[0] != "mesh.mod.mesh-catalog.tool.catalog_tools" || - tools[1] != "mesh.mod.mesh-catalog.tool.catalog_tools.*" { + want := []string{"mesh.mod.mesh-catalog.call.catalog_tools.*.person~ada", "mesh.mod.mesh-catalog.call.catalog_tools.person~ada", + "mesh.mod.mesh-catalog.tool.catalog_tools", "mesh.mod.mesh-catalog.tool.catalog_tools.*"} + if strings.Join(tools, " ") != strings.Join(want, " ") { t.Errorf("ada may publish %v, which should be the one tool, both ways addressed, and nothing else", perms.Publish) } for _, s := range perms.Publish { From 094d3d5bc6c704bcaa451c44df90113544f1921b Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 10:51:41 +0200 Subject: [PATCH 3/8] Wait out a registry held still, for up to five minutes, instead of failing the build (issue 457) The store's nightly collection stops the registry for about a minute and a half; builds in that window failed on connection refused and failed their whole plans. A refusal is now waited for with a growing pause, said in the build's log, and still fails the build past the bound. --- internal/builder/registry.go | 12 +- internal/builder/registry_wait.go | 131 +++++++++++++++++++++ internal/builder/registry_wait_test.go | 154 +++++++++++++++++++++++++ 3 files changed, 294 insertions(+), 3 deletions(-) create mode 100644 internal/builder/registry_wait.go create mode 100644 internal/builder/registry_wait_test.go diff --git a/internal/builder/registry.go b/internal/builder/registry.go index 219b4fe5..4f7ecd11 100644 --- a/internal/builder/registry.go +++ b/internal/builder/registry.go @@ -52,7 +52,11 @@ func (r Registry) PublishImage(ctx context.Context, localTag, repository string) if _, err := r.Run(ctx, "", "docker", "tag", localTag, remote); err != nil { return "", err } - if _, err := r.Run(ctx, "", "docker", "push", remote); err != nil { + // The push waits for a registry held still, as every request to it does (novox/hq issue 457). + if err := waitForRegistry(ctx, r.Address, "docker push "+remote, func() error { + _, err := r.Run(ctx, "", "docker", "push", remote) + return err + }); err != nil { return "", err } out, err := r.Run(ctx, "", "docker", "inspect", "--format", "{{index .RepoDigests 0}}", remote) @@ -169,11 +173,13 @@ func (r Registry) has(ctx context.Context, url string, accept ...string) (bool, return response.StatusCode == http.StatusOK, nil } +// client is the client for the registry and for upstream, whose requests to the registry wait out a +// registry held still (novox/hq issue 457). func (r Registry) client() *http.Client { if r.HTTP != nil { - return r.HTTP + return waiting(r.HTTP, r.Address) } - return http.DefaultClient + return waiting(http.DefaultClient, r.Address) } // separator is whether the upload location already carries a query. diff --git a/internal/builder/registry_wait.go b/internal/builder/registry_wait.go new file mode 100644 index 00000000..010622a0 --- /dev/null +++ b/internal/builder/registry_wait.go @@ -0,0 +1,131 @@ +package builder + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "syscall" + "time" +) + +// A build waits out a registry that refuses connections, for a bounded time (novox/hq issue 457). +// +// **Why waiting, and why here.** The store's nightly collection holds the registry still for its run — +// about a minute and a half, measured on 2026-10-11 — and a build that reached the registry in that +// window failed on "connection refused", and its whole delivery plan with it: a plan failed for a +// pause the mesh itself scheduled. The other design weighed was the collection telling the controller +// it holds the registry, and the controller holding build asks while it runs. Waiting here is smaller +// and covers more: it is local to the one place that talks to the registry, needs no new message +// between modules, and also carries a build over any other short outage — a registry restarted by its +// own update, say. A refusal is the one error waited for: nothing was sent, so trying again cannot +// do anything twice, and it is what a registry that is stopped answers. +// +// **Bounded, and loud past the bound.** registryWait is longer than the collection holds the registry +// (five minutes against about one and a half), so the pause the mesh schedules is always waited out, +// and a registry that is really down still fails the build — saying how long it was refused — rather +// than holding a build machine for ever. Every wait is said in the build's log, with why, and so is +// the registry answering again. + +var ( + // registryWait is how long a build waits for a registry that refuses, per call that found it so. + registryWait = 5 * time.Minute + // registryFirstPause is the first pause between tries; each pause doubles, up to registryMostPause. + registryFirstPause = time.Second +) + +// registryMostPause is the longest pause between two tries: short enough that a build goes on within +// seconds of the registry answering again. +const registryMostPause = 10 * time.Second + +// refused is whether an error is a connection refused: from a dial here, or as a command such as docker +// said it in its output. +func refused(err error) bool { + return err != nil && (errors.Is(err, syscall.ECONNREFUSED) || strings.Contains(err.Error(), "connection refused")) +} + +// waitForRegistry runs try, and while it fails because the registry at address refuses connections, +// tries again with a growing pause until registryWait has passed. what names the call, for the log. +func waitForRegistry(ctx context.Context, address, what string, try func() error) error { + err := try() + if !refused(err) { + return err + } + started := time.Now() + pause := registryFirstPause + tell("registry", "%s: refused; the build waits for the registry at %s, for up to %s — it is held still while "+ + "the store's nightly collection runs, about a minute and a half (novox/hq issue 457)", + what, address, registryWait) + for { + left := registryWait - time.Since(started) + if left <= 0 { + tell("registry", "%s: the registry at %s still refuses after %s; the build fails", what, address, + time.Since(started).Round(time.Second)) + return fmt.Errorf("the registry at %s refused every connection for %s, longer than its nightly "+ + "collection holds it still, so it is down, not paused: %w", + address, time.Since(started).Round(time.Millisecond), err) + } + wait := min(pause, left) + select { + case <-ctx.Done(): + return fmt.Errorf("stopped while waiting for the registry at %s: %w (last: %v)", address, ctx.Err(), err) + case <-time.After(wait): + } + pause = min(pause*2, registryMostPause) + if err = try(); !refused(err) { + tell("registry", "%s: the registry at %s answers again after %s; the build goes on", what, address, + time.Since(started).Round(time.Millisecond)) + return err + } + } +} + +// waitingTransport waits for the registry on every request to it, and on none to anywhere else: the +// same client copies from upstream registries, whose refusals are theirs to answer. +type waitingTransport struct { + base http.RoundTripper + address string +} + +func (t waitingTransport) RoundTrip(request *http.Request) (*http.Response, error) { + if request.URL.Host != t.address { + return t.base.RoundTrip(request) + } + var response *http.Response + tries := 0 + err := waitForRegistry(request.Context(), t.address, request.Method+" "+request.URL.Path, func() error { + attempt := request + if tries > 0 && request.Body != nil && request.Body != http.NoBody { + // A body is sent again only when it can be read again; one that cannot is not retried. + if request.GetBody == nil { + return fmt.Errorf("%s %s cannot be sent again: its body cannot be read twice", request.Method, request.URL) + } + body, err := request.GetBody() + if err != nil { + return err + } + attempt = request.Clone(request.Context()) + attempt.Body = body + } + tries++ + var err error + response, err = t.base.RoundTrip(attempt) + return err + }) + return response, err +} + +// waiting is a client like c whose requests to the registry wait for it. +func waiting(c *http.Client, address string) *http.Client { + if _, already := c.Transport.(waitingTransport); already { + return c + } + copied := *c + base := c.Transport + if base == nil { + base = http.DefaultTransport + } + copied.Transport = waitingTransport{base: base, address: address} + return &copied +} diff --git a/internal/builder/registry_wait_test.go b/internal/builder/registry_wait_test.go new file mode 100644 index 00000000..8bb9be85 --- /dev/null +++ b/internal/builder/registry_wait_test.go @@ -0,0 +1,154 @@ +package builder + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net" + "net/http" + "strings" + "sync" + "testing" + "time" +) + +// A registry held still for a while (novox/hq issue 457): the store's nightly collection stops it for +// about a minute and a half, and a build in that window was failed, and its whole plan with it, for a +// pause the mesh itself scheduled. A build waits it out — for a bound longer than the collection +// holds it — says so in its log, and still fails, loudly, past the bound. + +// heldStill is a registry address that refuses every connection until it starts answering after +// pause, or never when pause is negative. +func heldStill(t *testing.T, f *fakeRegistry, pause time.Duration) string { + t.Helper() + handler := f.serve(t).Config.Handler + reserved, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + address := reserved.Addr().String() + reserved.Close() // refused from here on: nothing listens + if pause < 0 { + return address + } + server := &http.Server{Handler: handler} + go func() { + time.Sleep(pause) + l, err := net.Listen("tcp", address) + if err != nil { + t.Errorf("cannot answer at %s again: %v", address, err) + return + } + _ = server.Serve(l) + }() + t.Cleanup(func() { _ = server.Close() }) + return address +} + +// saying collects what a build says, as the build machine's per-build Said does. +func saying(t *testing.T) func() []string { + t.Helper() + var mu sync.Mutex + var lines []string + was := Said + Said = func(step, message string) { + mu.Lock() + defer mu.Unlock() + lines = append(lines, step+": "+message) + } + t.Cleanup(func() { Said = was }) + return func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), lines...) + } +} + +// waitingFor shortens the bound and the first pause, so a test waits for milliseconds. +func waitingFor(t *testing.T, bound time.Duration) { + t.Helper() + wasBound, wasFirst := registryWait, registryFirstPause + registryWait, registryFirstPause = bound, 20*time.Millisecond + t.Cleanup(func() { registryWait, registryFirstPause = wasBound, wasFirst }) +} + +func TestABuildWaitsForARegistryHeldStillAndGoesOn(t *testing.T) { + waitingFor(t, 5*time.Second) + said := saying(t) + f := &fakeRegistry{} + r := Registry{Address: heldStill(t, f, 400*time.Millisecond)} + body := []byte("a theme") + sum := sha256.Sum256(body) + digest := "sha256:" + hex.EncodeToString(sum[:]) + + if _, err := r.PublishArchive(context.Background(), "shell/config", body, digest); err != nil { + t.Fatalf("a registry refusing for 400ms failed the build: %v", err) + } + if string(f.blobs[digest]) != "a theme" { + t.Fatalf("the registry holds %q", f.blobs[digest]) + } + log := strings.Join(said(), "\n") + if !strings.Contains(log, "waits for the registry at "+r.Address) || !strings.Contains(log, "nightly collection") { + t.Errorf("the build's log does not say it waited for the registry, and why:\n%s", log) + } + if !strings.Contains(log, "answers again") { + t.Errorf("the build's log does not say the registry came back:\n%s", log) + } +} + +func TestABuildFailsLoudlyOnARegistryRefusingPastTheBound(t *testing.T) { + waitingFor(t, 300*time.Millisecond) + said := saying(t) + f := &fakeRegistry{} + r := Registry{Address: heldStill(t, f, -1)} + body := []byte("a theme") + sum := sha256.Sum256(body) + digest := "sha256:" + hex.EncodeToString(sum[:]) + + started := time.Now() + _, err := r.PublishArchive(context.Background(), "shell/config", body, digest) + if err == nil { + t.Fatal("a registry that never answered published the archive") + } + if !strings.Contains(err.Error(), "refused every connection for") || !strings.Contains(err.Error(), "connection refused") { + t.Errorf("the failure does not say it waited and was refused throughout: %v", err) + } + if waited := time.Since(started); waited < 300*time.Millisecond { + t.Errorf("failed after %s, before the bound", waited) + } + if log := strings.Join(said(), "\n"); !strings.Contains(log, "waits for the registry") { + t.Errorf("the build's log does not say it waited:\n%s", log) + } +} + +func TestAnImagePushWaitsForARegistryHeldStill(t *testing.T) { + waitingFor(t, 5*time.Second) + said := saying(t) + pushes := 0 + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + switch args[0] { + case "push": + pushes++ + if pushes < 3 { + return "", errorString("docker push: dial tcp 127.0.0.1:5000: connect: connection refused") + } + case "inspect": + return "127.0.0.1:5000/m/server@sha256:abc\n", nil + } + return "", nil + } + r := Registry{Address: "127.0.0.1:5000", Run: run} + if _, err := r.PublishImage(context.Background(), "local", "m/server"); err != nil { + t.Fatalf("a push refused twice failed the build: %v", err) + } + if pushes != 3 { + t.Errorf("pushed %d times, want 3", pushes) + } + if log := strings.Join(said(), "\n"); !strings.Contains(log, "waits for the registry") { + t.Errorf("the build's log does not say it waited:\n%s", log) + } +} + +type errorString string + +func (e errorString) Error() string { return string(e) } From 7758301444c99eb1163377ae53fccd13f75d393d Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 10:51:41 +0200 Subject: [PATCH 4/8] Ask every failed build of the tier on a retry, not only the first (issue 457) An outcome arriving after its plan failed is kept only in the build records, so a retry read from the plan alone asked one failed build and the records then failed the plan again on the next. Settle the tier from the records first. --- cmd/mesh-controller/plan_retry.go | 22 ++++++++ cmd/mesh-controller/plan_retry_tier_test.go | 57 +++++++++++++++++++++ cmd/mesh-controller/release_plan.go | 51 ++++++++++-------- 3 files changed, 110 insertions(+), 20 deletions(-) create mode 100644 cmd/mesh-controller/plan_retry_tier_test.go diff --git a/cmd/mesh-controller/plan_retry.go b/cmd/mesh-controller/plan_retry.go index a8377bc1..10ed9c45 100644 --- a/cmd/mesh-controller/plan_retry.go +++ b/cmd/mesh-controller/plan_retry.go @@ -288,6 +288,15 @@ func retryPlan(ctx context.Context, open *stores, id string) (string, error) { "those walks answer them; a newer merge, or `rebuild `, builds again", p.ID) } } + // Settled from the build records before anything is judged (novox/hq issue 457): a build of the tier + // that failed after the plan did is as failed as the one that failed it. + if p.Tier < len(p.Tiers) { + recorded, byID, err := recordsOfAsked(ctx, inv, &p, p.Tiers[p.Tier]) + if err != nil { + return "", err + } + toRetry(&p, recorded, byID) + } if err := retryRefusal(p, plans); err != nil { return "", err } @@ -525,3 +534,16 @@ func retryTierWhole(ctx context.Context, open *stores, p *inventory.Plan, again func sendAgain(s *inventory.PlanModule) { s.First, s.FirstAt, s.Gate, s.GatedBy, s.Previous, s.Why = nil, nil, nil, "", "", "" } + +// toRetry is the modules a retry asks again: every one of the tier that failed, the plan's state +// settled from the build records first (novox/hq issue 457). A plan fails on the first failure in its +// tier, and an outcome arriving after that finds no open plan to answer — it is kept only in the build +// records. Read from the plan alone, a retry asked only the build that failed first, and the records +// then failed the plan again on the next: each failed build of a tier took a retry of its own. What +// still runs is left asked, and its outcome is the plan's once the retry sets it building. +func toRetry(p *inventory.Plan, recorded map[string][]inventory.Build, byID map[string]inventory.Build) []string { + if p.Tier < len(p.Tiers) { + settleFromRecords(p, p.Tiers[p.Tier], recorded, byID) + } + return failedIn(*p) +} diff --git a/cmd/mesh-controller/plan_retry_tier_test.go b/cmd/mesh-controller/plan_retry_tier_test.go new file mode 100644 index 00000000..7efd44a8 --- /dev/null +++ b/cmd/mesh-controller/plan_retry_tier_test.go @@ -0,0 +1,57 @@ +package main + +import ( + "reflect" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/inventory" +) + +// A retry asks every failed build of the tier, not one (novox/hq issue 457). Seen 2026-10-11: gitea +// and plex both failed in tier 0 while the registry was held still; gitea's failure failed the plan, +// and plex's, arriving after, found no open plan and was kept only in the build records. The first +// retry asked gitea alone, the records then failed the plan again on plex, and a second retry asked +// plex. +func TestARetryAsksEveryFailedBuildOfTheTier(t *testing.T) { + asked := time.Date(2026, 10, 11, 1, 29, 0, 0, time.UTC) + failedAt := asked.Add(2 * time.Minute) + p := inventory.Plan{ID: "plan-457", State: inventory.PlanFailed, Tier: 0, + Tiers: [][]string{{"gitea", "plex"}}, Note: "gitea failed to build in tier 0", + Modules: map[string]*inventory.PlanModule{ + "gitea": {State: "failed", AskedAt: &asked, Build: "build-gitea", Why: "cannot reach the registry"}, + "plex": {State: "asked", AskedAt: &asked, Build: "build-plex"}, + }} + byID := map[string]inventory.Build{ + "build-plex": {ID: "build-plex", Module: "plex", At: failedAt, Failed: "cannot reach the registry"}, + } + + if got := toRetry(&p, nil, byID); !reflect.DeepEqual(got, []string{"gitea", "plex"}) { + t.Fatalf("a retry of a tier where gitea and plex failed asks %v", got) + } +} + +// A build of the tier still running when the plan failed is not asked again: its outcome is the plan's +// once the retry sets it building, and one that is recorded built is taken as built. +func TestARetryLeavesABuildThatRunsOrWorked(t *testing.T) { + asked := time.Date(2026, 10, 11, 1, 29, 0, 0, time.UTC) + builtAt := asked.Add(3 * time.Minute) + p := inventory.Plan{ID: "plan-457", State: inventory.PlanFailed, Tier: 0, + Tiers: [][]string{{"a", "b", "c"}}, + Modules: map[string]*inventory.PlanModule{ + "a": {State: "failed", AskedAt: &asked, Build: "build-a", Why: "broken"}, + "b": {State: "asked", AskedAt: &asked, Build: "build-b"}, + "c": {State: "asked", AskedAt: &asked, Build: "build-c"}, + }} + byID := map[string]inventory.Build{"build-c": {ID: "build-c", Module: "c", At: builtAt, Commit: "c0ffee"}} + + if got := toRetry(&p, nil, byID); !reflect.DeepEqual(got, []string{"a"}) { + t.Fatalf("asks %v, want a alone", got) + } + if s := p.Modules["c"]; s.State != "built" || s.Commit != "c0ffee" { + t.Errorf("c, recorded built, is %+v", s) + } + if s := p.Modules["b"]; s.State != "asked" { + t.Errorf("b, still building, is %+v", s) + } +} diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 76fbd95b..7adc089c 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -649,26 +649,9 @@ func advanceOnce(ctx context.Context, open *stores, p *inventory.Plan, // the controller in its first tier: the build that produced the new one is recorded, and the // plan never hears it. The record is the fact; a build recorded after the ask is that tier's // outcome, whoever was listening. - recorded := map[string][]inventory.Build{} - byID := map[string]inventory.Build{} - for _, m := range tier { - if s := p.Modules[m]; s != nil && s.State == "asked" { - builds, err := inv.Builds(ctx, m, 5) - if err != nil { - return false, err - } - recorded[m] = builds - // Its own ask's record, by id — found even when the outcome named no module (ADR 0219). - if s.Build != "" { - b, found, err := inv.BuildByID(ctx, s.Build) - if err != nil { - return false, err - } - if found { - byID[s.Build] = b - } - } - } + recorded, byID, err := recordsOfAsked(ctx, inv, p, tier) + if err != nil { + return false, err } if settleFromRecords(p, tier, recorded, byID) { return true, nil @@ -1776,6 +1759,34 @@ func splitList(s string) []string { return out } +// recordsOfAsked is what settleFromRecords reads: for every module of the tier still `asked`, its last +// builds and the record of its own ask, by id. +func recordsOfAsked(ctx context.Context, inv *inventory.Inventory, p *inventory.Plan, tier []string) ( + map[string][]inventory.Build, map[string]inventory.Build, error) { + recorded := map[string][]inventory.Build{} + byID := map[string]inventory.Build{} + for _, m := range tier { + if s := p.Modules[m]; s != nil && s.State == "asked" { + builds, err := inv.Builds(ctx, m, 5) + if err != nil { + return nil, nil, err + } + recorded[m] = builds + // Its own ask's record, by id — found even when the outcome named no module (ADR 0219). + if s.Build != "" { + b, found, err := inv.BuildByID(ctx, s.Build) + if err != nil { + return nil, nil, err + } + if found { + byID[s.Build] = b + } + } + } + } + return recorded, byID, nil +} + // settleFromRecords marks every module of the tier still `asked` built — or failed — from a build // recorded after it was asked, and says whether it changed anything (novox/hq 04-ISSUES/214). // Newest first, as Builds answers: the first record after the ask is the outcome of that ask. From 83ce19b9c0141624445728d43a2cd969227a56e6 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:05:30 +0200 Subject: [PATCH 5/8] Say a registry came back only when the call worked, and retry from toRetry's set (issue 457 review) The streaming blob PUT is left unwaited, with why, since its body cannot be read twice and the POST before it already waited. --- cmd/mesh-controller/plan_retry.go | 11 ++++++----- internal/builder/mirror.go | 5 +++++ internal/builder/registry_wait.go | 10 ++++++++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/cmd/mesh-controller/plan_retry.go b/cmd/mesh-controller/plan_retry.go index 10ed9c45..df28926e 100644 --- a/cmd/mesh-controller/plan_retry.go +++ b/cmd/mesh-controller/plan_retry.go @@ -289,21 +289,23 @@ func retryPlan(ctx context.Context, open *stores, id string) (string, error) { } } // Settled from the build records before anything is judged (novox/hq issue 457): a build of the tier - // that failed after the plan did is as failed as the one that failed it. + // that failed after the plan did is as failed as the one that failed it. What toRetry says is the + // failed set every step below works from. + var failed []string if p.Tier < len(p.Tiers) { recorded, byID, err := recordsOfAsked(ctx, inv, &p, p.Tiers[p.Tier]) if err != nil { return "", err } - toRetry(&p, recorded, byID) + failed = toRetry(&p, recorded, byID) } if err := retryRefusal(p, plans); err != nil { return "", err } - if again := unjudgedAtGate(p); len(failedIn(p)) == 0 && len(again) > 0 { + if again := unjudgedAtGate(p); len(failed) == 0 && len(again) > 0 { return retryTierWhole(ctx, open, &p, again) } - if len(failedIn(p)) == 0 { + if len(failed) == 0 { return retryRollouts(ctx, open, &p) } entries, err := inv.Catalogued(ctx) @@ -314,7 +316,6 @@ func retryPlan(ctx context.Context, open *stores, id string) (string, error) { for _, e := range entries { byName[e.Manifest.Module] = e } - failed := failedIn(p) var asked []string for _, m := range failed { askModule(ctx, &p, m, byName) diff --git a/internal/builder/mirror.go b/internal/builder/mirror.go index 8840e3d5..4a2da87e 100644 --- a/internal/builder/mirror.go +++ b/internal/builder/mirror.go @@ -429,6 +429,11 @@ func (r Registry) copyBlob(ctx context.Context, src *source, where upstream, dig if response.ContentLength > 0 { put.ContentLength = response.ContentLength } + // **Not waited for if refused** (novox/hq issue 457): the body streams from upstream and cannot be + // read twice, so a registry held still between the POST above and this PUT fails the copy with + // "its body cannot be read twice" rather than waiting. Accepted: the POST a moment before already + // waited the registry out, so the window is the length of one upstream fetch, and the build fails + // loudly, to be asked again, rather than buffering every base blob in memory. done, err := r.client().Do(put) if err != nil { return fmt.Errorf("cannot upload blob %s: %w", digest, err) diff --git a/internal/builder/registry_wait.go b/internal/builder/registry_wait.go index 010622a0..f2b10bae 100644 --- a/internal/builder/registry_wait.go +++ b/internal/builder/registry_wait.go @@ -74,8 +74,14 @@ func waitForRegistry(ctx context.Context, address, what string, try func() error } pause = min(pause*2, registryMostPause) if err = try(); !refused(err) { - tell("registry", "%s: the registry at %s answers again after %s; the build goes on", what, address, - time.Since(started).Round(time.Millisecond)) + // Said as it is: the registry answering is only the build going on when the call worked. + if err == nil { + tell("registry", "%s: the registry at %s answers again after %s; the build goes on", what, address, + time.Since(started).Round(time.Millisecond)) + } else { + tell("registry", "%s: the registry at %s no longer refuses after %s, and answered with: %v", what, + address, time.Since(started).Round(time.Millisecond), err) + } return err } } From c494ed03a7c1c60a17109aa9a09c5b6c997b704f Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:20:10 +0200 Subject: [PATCH 6/8] Keep the forge credential out of what a check's container sees, and redact what a check publishes (issue 462) The toolchain container mounts the workspace as HOME, so the credential kept there, and a clone's recorded userinfo, were readable by any pull request; its output is kept on the bus for days. The credential now lives in a private directory outside the workspace only while cloning, clones record their URL without userinfo, and every line said to the build's log and the verdict passes a redactor copied from the journal tool (sharing it: issue 471). --- internal/builder/builder.go | 60 ++++++- internal/builder/builder_test.go | 34 ++-- internal/builder/check.go | 45 +++-- internal/builder/check_credential_test.go | 200 ++++++++++++++++++++++ internal/builder/redact.go | 191 +++++++++++++++++++++ 5 files changed, 497 insertions(+), 33 deletions(-) create mode 100644 internal/builder/check_credential_test.go create mode 100644 internal/builder/redact.go diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 2c38e005..3f8a3ba2 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "os" @@ -159,13 +160,11 @@ func build(ctx context.Context, run Runner, publish Publisher, } // The credential is a file git reads, never an argument: a URL carrying a password in argv // would be readable by anything that can list processes for as long as a clone runs. - credentials := "" - if forge.URL != "" { - credentials = filepath.Join(workspace, "git-credentials") - if err := os.WriteFile(credentials, []byte(forge.URL+"\n"), 0o600); err != nil { - return Result{}, err - } + credentials, forget, err := storeCredential(workspace, forge) + if err != nil { + return Result{}, err } + defer forget() tree := filepath.Join(workspace, "source") if err := os.RemoveAll(tree); err != nil { return Result{}, err @@ -467,6 +466,55 @@ func contextURL(from catalogue.ArtifactContext, seats map[string]string) (string return strings.TrimRight(base, "/") + "/" + strings.TrimSuffix(strings.Trim(from.Repository, "/"), ".git") + ".git", nil } +// storeCredential writes the forge credential where git's credential store reads it, and the function that +// removes it again; "" and nothing to remove when the builder holds none. +// +// **Never inside the workspace** (novox/hq issue 462): a merge check's toolchain container mounts the +// workspace as its HOME, so a credential kept there — even one a build left behind — is readable by any pull +// request's merge-check.sh, and what it prints is kept on the bus. So it lives in a directory of its own +// outside the workspace, made private, and a credential an older builder left in the workspace is removed. +func storeCredential(workspace string, forge GitCredential) (string, func(), error) { + if err := os.Remove(filepath.Join(workspace, "git-credentials")); err != nil && !errors.Is(err, os.ErrNotExist) { + return "", nil, fmt.Errorf("a credential left in the workspace cannot be removed: %w", err) + } + if forge.URL == "" { + return "", func() {}, nil + } + dir, err := os.MkdirTemp("", "mesh-forge-credential-") + if err != nil { + return "", nil, err + } + forget := func() { os.RemoveAll(dir) } + if withinDir(workspace, dir) { + forget() + return "", nil, fmt.Errorf("the temporary directory %s is inside the workspace %s, which a check's "+ + "container mounts: the forge credential is not written where it could read it", dir, workspace) + } + path := filepath.Join(dir, "git-credentials") + if err := os.WriteFile(path, []byte(forge.URL+"\n"), 0o600); err != nil { + forget() + return "", nil, err + } + return path, forget, nil +} + +// withinDir is whether path is dir or under it, both made absolute and resolved. +func withinDir(dir, path string) bool { + d, err1 := filepath.Abs(dir) + p, err2 := filepath.Abs(path) + if err1 != nil || err2 != nil { + return true + } + if r, err := filepath.EvalSymlinks(d); err == nil { + d = r + } + if r, err := filepath.EvalSymlinks(p); err == nil { + p = r + } + rel, err := filepath.Rel(d, p) + return err != nil || rel == "." || filepath.IsLocal(rel) +} + // cloneWith is a git invocation that may offer a stored credential. // // The first `-c credential.helper=` clears every helper the environment might carry, so exactly diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index da362f2c..afe57572 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -438,30 +438,34 @@ func TestABuildOffersTheForgesCredentialThroughGitsOwnStore(t *testing.T) { if err != nil { t.Fatal(err) } - stored := filepath.Join(workspace, "git-credentials") clone := r.ran[0] - if !strings.Contains(clone, "credential.helper=store --file="+stored) { - t.Fatalf("the clone does not name the credential store: %s", clone) - } + stored := storeNamedIn(t, clone) for _, line := range r.ran { if strings.Contains(line, "sw0rdfi5h") { t.Fatalf("the secret is in a command line, readable by anything that can list processes: %s", line) } } - raw, err := os.ReadFile(stored) - if err != nil { - t.Fatal(err) + // Outside the workspace a check's container mounts, and gone once the build is (novox/hq issue 462); what + // it held while git read it is checked with the check's clones (TestACheckContainerSeesNoForgeCredential). + if withinDir(workspace, stored) { + t.Fatalf("the credential store %s is inside the workspace %s", stored, workspace) } - if strings.TrimSpace(string(raw)) != "http://mesh_novox_builder:sw0rdfi5h@forge.invalid:20000" { - t.Fatalf("the store does not hold the credential as given: %q", raw) + if _, err := os.Stat(stored); !os.IsNotExist(err) { + t.Fatalf("the credential store outlives the build: %v", err) } - info, err := os.Stat(stored) - if err != nil { - t.Fatal(err) + if _, err := os.Stat(filepath.Join(workspace, "git-credentials")); !os.IsNotExist(err) { + t.Fatal("a credential file is left in the workspace") } - if info.Mode().Perm() != 0o600 { - t.Fatalf("the credential file is readable beyond its owner: %v", info.Mode()) +} + +// storeNamedIn is the credential store a git command line offers. +func storeNamedIn(t *testing.T, line string) string { + t.Helper() + _, after, ok := strings.Cut(line, "credential.helper=store --file=") + if !ok { + t.Fatalf("the clone does not name the credential store: %s", line) } + return strings.Fields(after)[0] } // Without a credential, a clone is exactly the invocation it always was, and no credential file @@ -506,7 +510,7 @@ func TestAContextCloneCarriesTheSameCredentialStore(t *testing.T) { if err != nil { t.Fatal(err) } - stored := filepath.Join(workspace, "git-credentials") + stored := storeNamedIn(t, r.ran[0]) var contextClone string for _, line := range r.ran { if strings.Contains(line, "clone") && strings.Contains(line, "source.git") { diff --git a/internal/builder/check.go b/internal/builder/check.go index 30e58f7e..fb154a6b 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -229,7 +229,13 @@ func ScriptToolchain(script []byte) string { // Check runs one merge check. An error is that it could not run; the verdict is then "error". func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry string, forge GitCredential, log Log) (CheckVerdict, error) { - say := logging(log) + // Everything a check says goes to the build's log, which the bus keeps: said redacted (novox/hq issue 462). + redact := redactorFor(forge) + say := logging(func(step, message string) { + if log != nil { + log(step, redact.redact(message)) + } + }) began := time.Now() ctx, stop := context.WithTimeout(ctx, CheckTimeout) defer stop() @@ -242,20 +248,27 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry return CheckVerdict{}, err } defer os.RemoveAll(root) - credentials := "" - if forge.URL != "" { - credentials = filepath.Join(workspace, "git-credentials") - if err := os.WriteFile(credentials, []byte(forge.URL+"\n"), 0o600); err != nil { - return CheckVerdict{}, err - } + // The forge credential lives outside the workspace the check's containers mount, and only while the + // check clones (novox/hq issue 462). + credentials, forget, err := storeCredential(workspace, forge) + if err != nil { + return CheckVerdict{}, err } + defer forget() clone := func(repository, ref, dir string) error { if _, err := run(ctx, root, "git", cloneWith(credentials, "clone", "--quiet", repository, dir)...); err != nil { - return fmt.Errorf("cannot clone %s: %w", repository, err) + return fmt.Errorf("cannot clone %s: %s", redact.redact(repository), redact.redact(err.Error())) + } + // Git records the URL cloned from as given, in the clone's .git/config the container reads: one + // carrying userinfo is recorded without it (novox/hq issue 462). + if bare, ok := withoutUserinfo(repository); ok { + if _, err := run(ctx, filepath.Join(root, dir), "git", "remote", "set-url", "origin", bare); err != nil { + return fmt.Errorf("the clone of %s keeps its credential: %w", bare, err) + } } if ref != "" { if _, err := run(ctx, filepath.Join(root, dir), "git", "checkout", "--quiet", ref); err != nil { - return fmt.Errorf("%s has no %s: %w", repository, ref, err) + return fmt.Errorf("%s has no %s: %w", redact.redact(repository), ref, err) } } return nil @@ -323,6 +336,9 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry } } + // Every clone is made: the credential is gone before anything of the check runs (novox/hq issue 462). + forget() + // The facts, and the versions they say the mesh runs. if registry == "" { return CheckVerdict{}, errors.New("no artifact store to read the facts snapshot from") @@ -479,8 +495,11 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry } } + // What the check printed travels in the verdict to the forge and the controller: redacted as the log is. + v.Gate.Summary, v.Repo.Summary, v.Repo.Failed = redact.redact(v.Gate.Summary), redact.redact(v.Repo.Summary), + redact.redact(v.Repo.Failed) v.Verdict, v.Summary = v.Gate.Verdict, v.Gate.Summary - v.Report, v.Took = withWhatFailed(out.String(), v.Repo), time.Since(began) + v.Report, v.Took = redact.redact(withWhatFailed(out.String(), v.Repo)), time.Since(began) say("check", "gate %s — %s; repository %s — %s (%s)", strings.ToUpper(v.Gate.Verdict), v.Gate.Summary, strings.ToUpper(v.Repo.Verdict), v.Repo.Summary, v.Took.Round(time.Second)) return v, nil @@ -606,7 +625,9 @@ func (l *toTheLog) line(line string) { line = line[:logLineBytes] + fmt.Sprintf(" … (%d bytes more)", len(line)-logLineBytes) } l.said++ - l.say("output", "%s", line) + // Redacted by its shape before the bus keeps it (novox/hq issue 462); Check's own say adds the secrets + // the builder knows. + l.say("output", "%s", redactor{}.redact(line)) } // close says the last line, and, when lines were left out, how many and what failed. @@ -624,7 +645,7 @@ func (l *toTheLog) close(failed string) { } l.say("output", "--- what failed, picked from the whole of its output") for _, line := range strings.Split(failed, "\n") { - l.say("output", "%s", line) + l.say("output", "%s", redactor{}.redact(line)) } } diff --git a/internal/builder/check_credential_test.go b/internal/builder/check_credential_test.go new file mode 100644 index 00000000..f61a6474 --- /dev/null +++ b/internal/builder/check_credential_test.go @@ -0,0 +1,200 @@ +package builder + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "io/fs" + "net/http" + "net/http/httptest" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/facts" +) + +// **A check's container never sees the forge credential** (novox/hq issue 462): the toolchain container +// mounts the workspace as HOME, so a credential kept there — or a clone's .git/config carrying one — is +// readable by any pull request's merge-check.sh, and printed, kept on the bus for days. + +const ( + forgeSecret = "sw0rdfi5h-forge" + forgeURL = "http://mesh_novox_builder:" + forgeSecret + "@forge.invalid:20000" + besideSecret = "b3side-t0ken" +) + +// aFactsRegistry is an artifact store holding the facts snapshot, and nothing else. +func aFactsRegistry(t *testing.T) string { + t.Helper() + body, err := json.Marshal(facts.Facts{Format: facts.Format, Taken: time.Now().UTC(), + Versions: facts.Versions{Bus: "2.11.17", Store: "17.11"}, Machines: []facts.Machine{{Name: "abcdef", Length: 6}}}) + if err != nil { + t.Fatal(err) + } + sum := sha256.Sum256(body) + digest := "sha256:" + hex.EncodeToString(sum[:]) + manifest, _ := json.Marshal(map[string]any{"schemaVersion": 2, "layers": []map[string]any{ + {"mediaType": facts.MediaType, "digest": digest, "size": len(body)}}}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/manifests/"): + w.Write(manifest) + case strings.HasSuffix(r.URL.Path, "/blobs/"+digest): + w.Write(body) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return strings.TrimPrefix(srv.URL, "http://") +} + +// bareURL is a URL with its userinfo left out. +func bareURL(t *testing.T, raw string) string { + u, err := url.Parse(raw) + if err != nil { + t.Fatal(err) + } + u.User = nil + return u.String() +} + +func TestACheckContainerSeesNoForgeCredential(t *testing.T) { + repo, head := aCheckedRepository(t, map[string]string{CheckScript: "echo checked\n"}) + besideRepo, besideHead := aCheckedRepository(t, map[string]string{"README": "beside"}) + // A clone source that carries userinfo, as a forge's clone URL may: git records it as given in the + // clone's .git/config, which the container reads. + besideURL := "file://beside-user:" + besideSecret + "@" + besideRepo + workspace := t.TempDir() + + var stores []string + reached := false + var leaks []string + run := func(ctx context.Context, dir, name string, args ...string) (string, error) { + switch name { + case "git": + for _, a := range args { + if f, ok := strings.CutPrefix(a, "credential.helper=store --file="); ok { + stores = append(stores, f) + raw, err := os.ReadFile(f) + if err != nil || strings.TrimSpace(string(raw)) != forgeURL { + t.Errorf("git is offered a store that does not hold the credential as given: %q, %v", raw, err) + } + if info, err := os.Stat(f); err == nil && info.Mode().Perm() != 0o600 { + t.Errorf("the credential store is readable beyond its owner: %v", info.Mode()) + } + } + } + if len(args) >= 2 && args[len(args)-3] == "--quiet" && hasString(args, "clone") { + source := args[len(args)-2] + if u, err := url.Parse(source); err == nil && u.User != nil { + // Git cannot reach a file:// URL with userinfo; clone it without, then record it as git + // would have: as given. + clone := append(append([]string{}, args[:len(args)-2]...), bareURL(t, source), args[len(args)-1]) + if out, err := Command(ctx, dir, "git", clone...); err != nil { + return out, err + } + return Command(ctx, filepath.Join(dir, args[len(args)-1]), "git", "remote", "set-url", "origin", source) + } + } + return Command(ctx, dir, name, args...) + case "docker": + if !reached { + reached = true + // The first container: everything the workspace holds is what the toolchain container sees. + filepath.WalkDir(workspace, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil + } + raw, _ := os.ReadFile(path) + s := string(raw) + if strings.Contains(s, forgeSecret) || strings.Contains(s, besideSecret) || + d.Name() == "git-credentials" || strings.Contains(s, "credential.helper") { + leaks = append(leaks, path) + } + return nil + }) + for _, f := range stores { + if _, err := os.Stat(f); !errors.Is(err, os.ErrNotExist) { + leaks = append(leaks, f+" (still there when the first container runs)") + } + if rel, err := filepath.Rel(workspace, f); err == nil && !strings.HasPrefix(rel, "..") { + leaks = append(leaks, f+" (inside the workspace the container mounts)") + } + } + } + if len(args) > 0 && args[0] == "ps" { + return "", nil + } + return "", errors.New("no container runtime in this test") + } + return "", errors.New("unexpected command " + name) + } + // A credential an older builder left in the workspace is removed too. + if err := os.WriteFile(filepath.Join(workspace, "git-credentials"), []byte(forgeURL+"\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := Check(t.Context(), run, CheckSpec{ID: "check-462", Repository: repo, Ref: head, Owner: "novox", + Repo: "mesh-controller", Number: 1, Toolchain: "golang", Beside: map[string]Beside{ + "mesh-catalog": {Repository: besideURL, Ref: besideHead}}}, workspace, aFactsRegistry(t), + GitCredential{URL: forgeURL}, nil) + if err == nil { + t.Fatal("the check ran past its first container in a test with none") + } + if !reached { + t.Fatalf("the check never reached its first container: %v", err) + } + if len(stores) == 0 { + t.Fatal("no clone was offered the forge credential") + } + if len(leaks) > 0 { + t.Fatalf("the check's container sees the credential:\n%s", strings.Join(leaks, "\n")) + } +} + +// Every line a repository's own check prints is published to the build's log redacted. +func TestACheckLinePublishedToTheLogIsRedacted(t *testing.T) { + var said []string + say := func(step, format string, args ...any) { + if step == "output" && len(args) > 0 { + said = append(said, args[0].(string)) + } + } + var out tail + layer := ownCheck(t.Context(), CheckSpec{Toolchain: "golang"}, []ScriptPart{{Toolchain: "go", Script: CheckScript}}, + t.TempDir(), &out, func() bool { return false }, func(string, string) *exec.Cmd { + return exec.CommandContext(t.Context(), "sh", "-c", "echo cloning http://mesh_builder:t0ps3cret-forge@forge.invalid/novox/x.git; "+ + "echo token ghp_abcdefghijklmnopqrstuvwxyz0123456789") + }, say) + if layer == nil || layer.Verdict != "pass" { + t.Fatalf("the check answered %+v\n%s", layer, out.String()) + } + joined := strings.Join(said, "\n") + if strings.Contains(joined, "t0ps3cret-forge") || strings.Contains(joined, "ghp_abcdef") { + t.Fatalf("a secret the check printed is published to the build's log:\n%s", joined) + } + if !strings.Contains(joined, "http://mesh_builder:[redacted: a password in a URI]@forge.invalid/novox/x.git") { + t.Fatalf("the line is not said with what was there named:\n%s", joined) + } +} + +func TestTheRedactorHidesTheForgeCredentialAndShapes(t *testing.T) { + r := redactorFor(GitCredential{URL: forgeURL}) + for in, want := range map[string]string{ + "the secret alone: " + forgeSecret: "the secret alone: [redacted: the forge credential]", + "go test ./... ok": "go test ./... ok", + "--password hunter22 and done": "--password [redacted: the word after --password] and done", + "commit 3b6b54a0c1d2e3f4a5b6c7d8e9f0": "commit 3b6b54a0c1d2e3f4a5b6c7d8e9f0", + } { + if got := r.redact(in); got != want { + t.Errorf("%q redacted as %q, want %q", in, got, want) + } + } +} diff --git a/internal/builder/redact.go b/internal/builder/redact.go new file mode 100644 index 00000000..84205614 --- /dev/null +++ b/internal/builder/redact.go @@ -0,0 +1,191 @@ +package builder + +// **Every line of a check's output is redacted before it is kept** (novox/hq issue 462). +// +// A repository's own check prints into the build's log, which the bus keeps for days and anyone who may read +// its events reads, and into the verdict, which the forge shows on the pull request. Software prints what it +// was given — a URL carrying a password, a token in a flag — and a pull request may print on purpose. +// So a line is said only after every secret the builder knows (the forge credential's password) and every +// value whose shape says it is one is replaced by a mark naming what was there, as the journal verb does. +// +// Copied from the journal tool's redactor (mesh-catalog, modules/systemd/cmd/systemd-tools/secrets.go, +// itself a copy of the docker module's), narrowed to a line's shapes, with the token shapes a check's output +// may carry added. A third copy: sharing them through mesh-sdk is novox/hq issue 471. + +import ( + "net/url" + "regexp" + "strings" +) + +// secretName is a variable name that says its value is a secret. +var secretName = regexp.MustCompile(`(?i)(pass(word|wd|phrase)?|secret|token|api_?key|private_?key|access_?key|credential|auth)`) + +// notAValue is a name that says its value is where a secret is, not the secret: a file or a path. +var notAValue = regexp.MustCompile(`(?i)(_FILE|FILE|_PATH|_DIR)$`) + +// uriPassword is a URI carrying a password in its userinfo: scheme://user:password@. +var uriPassword = regexp.MustCompile(`[A-Za-z][A-Za-z0-9+.-]*://[^\s/:@'"]*:([^\s/@'"]+)@`) + +// tokenShaped are tokens recognised by their own prefix, whatever surrounds them: a forge's or a host's +// access token, a JSON web token, a NATS seed. +var tokenShaped = []struct { + name string + re *regexp.Regexp +}{ + {"an access token", regexp.MustCompile(`\b(gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,}|glpat-[A-Za-z0-9_-]{20,}|xox[abpr]-[A-Za-z0-9-]{10,}|sk-ant-[A-Za-z0-9_-]{20,})`)}, + {"a JSON web token", regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{8,}\.eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]+`)}, + {"a NATS seed", regexp.MustCompile(`\bS[ACNOU][A-Z2-7]{56}\b`)}, +} + +// masked is a password a program already hid: ***, xxx, , [REDACTED]. +var masked = regexp.MustCompile(`^(\*+|x+|X+|<[^>]*>|\[[^\]]*\]|%2A+)$`) + +// ordinary is a value under a secret's name that is not one: a path, an address, a number, a switch. +var ordinary = regexp.MustCompile(`^(/.*|[A-Za-z][A-Za-z0-9+.-]*://.*|[0-9.]+[a-z]?|(?i:true|false|yes|no|on|off|none|null))$`) + +// leastSecret is the shortest value compared as a secret: a shorter one matches ordinary words. +const leastSecret = 6 + +// passwordFlags take a secret as their next word, or after `=`, whatever the program. +var passwordFlags = map[string]bool{ + "-P": true, "--password": true, "--pass": true, "--passwd": true, "--secret": true, "--secret-key": true, + "--token": true, "--api-key": true, "--apikey": true, "--auth": true, +} + +// knownSecret is one value the builder holds, by the name it is said under. +type knownSecret struct { + Name string + Value string +} + +// redactor hides the secrets it knows and those a line's shapes say are secrets. +type redactor struct{ known []knownSecret } + +// redactorFor knows the forge credential's password, and its user's name with it, in every form git or a +// program may print them. +func redactorFor(forge GitCredential) redactor { + var r redactor + if forge.URL == "" { + return r + } + for _, m := range uriPassword.FindAllStringSubmatch(forge.URL, -1) { + r.add("the forge credential", m[1]) + if dec, err := url.PathUnescape(m[1]); err == nil && dec != m[1] { + r.add("the forge credential", dec) + } + } + return r +} + +func (r *redactor) add(name, value string) { + if len(value) < leastSecret || masked.MatchString(value) { + return + } + for _, k := range r.known { + if k.Value == value { + return + } + } + r.known = append(r.known, knownSecret{name, value}) +} + +// redact is a text with every known secret, every value its shape says is one, and every password inside a +// URI replaced by a mark naming what was there. Line by line: a shape is judged within its line. +func (r redactor) redact(text string) string { + if !strings.ContainsAny(text, "\n") { + return r.line(text) + } + lines := strings.Split(text, "\n") + for i, l := range lines { + lines[i] = r.line(l) + } + return strings.Join(lines, "\n") +} + +func (r redactor) line(line string) string { + replace := func(s knownSecret) { + for _, f := range forms(s.Value) { + line = strings.ReplaceAll(line, f, "[redacted: "+s.Name+"]") + } + } + for _, s := range r.known { + replace(s) + } + for _, s := range shaped(line) { + replace(s) + } + line = uriPassword.ReplaceAllStringFunc(line, func(m string) string { + sub := uriPassword.FindStringSubmatch(m) + if masked.MatchString(sub[1]) || strings.HasPrefix(sub[1], "[redacted") { + return m + } + return strings.TrimSuffix(m, sub[1]+"@") + "[redacted: a password in a URI]@" + }) + for _, t := range tokenShaped { + line = t.re.ReplaceAllString(line, "[redacted: "+t.name+"]") + } + return line +} + +// forms are the ways a value may appear printed: as given, and URL-encoded. +func forms(value string) []string { + out := []string{value} + for _, f := range []string{url.QueryEscape(value), url.PathEscape(value)} { + if f != value && !hasString(out, f) { + out = append(out, f) + } + } + return out +} + +func hasString(list []string, s string) bool { + for _, x := range list { + if x == s { + return true + } + } + return false +} + +// shaped are the values a line carries by their shape: the word after a password flag, or the value of one +// given with `=`, and a NAME=value whose name says secret. +func shaped(line string) []knownSecret { + var out []knownSecret + add := func(name, value string) { + value = strings.Trim(value, `"',;`) + if len(value) < leastSecret || masked.MatchString(value) || ordinary.MatchString(value) || + strings.HasPrefix(value, "[redacted") { + return + } + out = append(out, knownSecret{name, value}) + } + words := strings.Fields(line) + for i, w := range words { + if flag, value, ok := strings.Cut(w, "="); ok && strings.HasPrefix(flag, "-") { + if passwordFlags[flag] { + add("the value of "+flag, value) + } + continue + } + if name, value, ok := strings.Cut(w, "="); ok && name != "" && secretName.MatchString(name) && + !notAValue.MatchString(name) && !strings.ContainsAny(name, "/:") { + add("the value of "+name, value) + continue + } + if i+1 < len(words) && passwordFlags[w] { + add("the word after "+w, words[i+1]) + } + } + return out +} + +// withoutUserinfo is a URL with its userinfo left out, and whether it carried any. +func withoutUserinfo(raw string) (string, bool) { + u, err := url.Parse(raw) + if err != nil || u.User == nil { + return raw, false + } + u.User = nil + return u.String(), true +} From 5c4fa43f8babb6d191dd82b1ceb0f6b88d46a8d9 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:24:58 +0200 Subject: [PATCH 7/8] Leave no package-registry credential or clone userinfo in the workspace a check mounts (issue 462) A build wrote .npmrc into its source tree and never removed it, and its clone recorded the URL's userinfo; a later check's container mounts the workspace as HOME. The .npmrc and the tree now go when the build ends, clones record their URL without userinfo, and a check first removes any .npmrc an older builder left. --- internal/builder/builder.go | 53 ++++++++++++++++++++++ internal/builder/check.go | 11 +++-- internal/builder/check_credential_test.go | 55 ++++++++++++++++++++++- internal/builder/packages_test.go | 24 +++++++--- 4 files changed, 129 insertions(+), 14 deletions(-) diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 3f8a3ba2..e142ef29 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path" @@ -169,6 +170,9 @@ func build(ctx context.Context, run Runner, publish Publisher, if err := os.RemoveAll(tree); err != nil { return Result{}, err } + // Removed when the build ends, whatever happens: the tree holds the .npmrc a build may be handed, in the + // workspace a later check's container mounts as its HOME (novox/hq issue 462). + defer os.RemoveAll(tree) // A fresh clone every time rather than a fetch into a tree that is already there. A build // that reuses a working tree can succeed because of something a previous build left behind, // and that is a build nobody can reproduce. @@ -176,6 +180,9 @@ func build(ctx context.Context, run Runner, publish Publisher, say("clone", "FAILED: %v", err) return Result{}, fmt.Errorf("cannot clone %s: %w", repository, err) } + if err := recordedWithoutUserinfo(ctx, run, tree, repository); err != nil { + return Result{}, err + } say("clone", "done") if ref != "" { if _, err := run(ctx, tree, "git", "checkout", "--quiet", ref); err != nil { @@ -247,6 +254,8 @@ func build(ctx context.Context, run Runner, publish Publisher, if err := os.WriteFile(npmrcPath, []byte(content), 0o600); err != nil { return Result{}, fmt.Errorf("cannot write the package-registry credential for the build: %w", err) } + // Only for as long as the build: a later check's container mounts this workspace (novox/hq issue 462). + defer os.Remove(npmrcPath) say("packages", "resolving %s from the mesh's package registry", npmrc.Scope) src.notPinned("it resolves packages from the mesh's registry at build time") } @@ -440,6 +449,9 @@ func contextFrom(ctx context.Context, run Runner, workspace, artifact, credentia if _, err := run(ctx, workspace, "git", cloneWith(credentials, "clone", "--quiet", url, dir)...); err != nil { return "", fmt.Errorf("cannot clone %s: %w", url, err) } + if err := recordedWithoutUserinfo(ctx, run, dir, url); err != nil { + return "", err + } if from.Ref != "" { if _, err := run(ctx, dir, "git", "checkout", "--quiet", from.Ref); err != nil { return "", fmt.Errorf("%s has no %s: %w", from.Repository, from.Ref, err) @@ -498,6 +510,47 @@ func storeCredential(workspace string, forge GitCredential) (string, func(), err return path, forget, nil } +// recordedWithoutUserinfo makes a clone record the URL it came from without userinfo: git keeps it as given in +// the clone's .git/config, inside the workspace a check's container mounts (novox/hq issue 462). +func recordedWithoutUserinfo(ctx context.Context, run Runner, clone, repository string) error { + bare, carried := withoutUserinfo(repository) + if !carried { + return nil + } + if _, err := run(ctx, clone, "git", "remote", "set-url", "origin", bare); err != nil { + return fmt.Errorf("the clone of %s keeps its credential: %w", bare, err) + } + return nil +} + +// forgetLeftCredentials removes what an earlier build or an older builder left in the workspace that holds +// a credential: the forge's git-credentials, and every .npmrc a build wrote into a tree it cloned. Run +// before a check, whose container mounts the workspace as its HOME (novox/hq issue 462). The Go caches are +// not walked: no build writes a credential there, and they hold more files than everything else. +func forgetLeftCredentials(workspace string) error { + if err := os.Remove(filepath.Join(workspace, "git-credentials")); err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + return filepath.WalkDir(workspace, func(p string, d fs.DirEntry, err error) error { + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil + } + return err + } + if d.IsDir() && p != workspace && (d.Name() == "go-cache" || d.Name() == "go-modules") && + filepath.Dir(p) == workspace { + return filepath.SkipDir + } + if !d.IsDir() && d.Name() == ".npmrc" { + if err := os.Remove(p); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("a package-registry credential left at %s cannot be removed: %w", p, err) + } + } + return nil + }) +} + // withinDir is whether path is dir or under it, both made absolute and resolved. func withinDir(dir, path string) bool { d, err1 := filepath.Abs(dir) diff --git a/internal/builder/check.go b/internal/builder/check.go index fb154a6b..50b3a476 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -255,16 +255,15 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry return CheckVerdict{}, err } defer forget() + if err := forgetLeftCredentials(workspace); err != nil { + return CheckVerdict{}, err + } clone := func(repository, ref, dir string) error { if _, err := run(ctx, root, "git", cloneWith(credentials, "clone", "--quiet", repository, dir)...); err != nil { return fmt.Errorf("cannot clone %s: %s", redact.redact(repository), redact.redact(err.Error())) } - // Git records the URL cloned from as given, in the clone's .git/config the container reads: one - // carrying userinfo is recorded without it (novox/hq issue 462). - if bare, ok := withoutUserinfo(repository); ok { - if _, err := run(ctx, filepath.Join(root, dir), "git", "remote", "set-url", "origin", bare); err != nil { - return fmt.Errorf("the clone of %s keeps its credential: %w", bare, err) - } + if err := recordedWithoutUserinfo(ctx, run, filepath.Join(root, dir), repository); err != nil { + return err } if ref != "" { if _, err := run(ctx, filepath.Join(root, dir), "git", "checkout", "--quiet", ref); err != nil { diff --git a/internal/builder/check_credential_test.go b/internal/builder/check_credential_test.go index f61a6474..7fc52aa9 100644 --- a/internal/builder/check_credential_test.go +++ b/internal/builder/check_credential_test.go @@ -28,6 +28,7 @@ const ( forgeSecret = "sw0rdfi5h-forge" forgeURL = "http://mesh_novox_builder:" + forgeSecret + "@forge.invalid:20000" besideSecret = "b3side-t0ken" + npmSecret = "npm-s3cret-t0ken" ) // aFactsRegistry is an artifact store holding the facts snapshot, and nothing else. @@ -116,7 +117,8 @@ func TestACheckContainerSeesNoForgeCredential(t *testing.T) { raw, _ := os.ReadFile(path) s := string(raw) if strings.Contains(s, forgeSecret) || strings.Contains(s, besideSecret) || - d.Name() == "git-credentials" || strings.Contains(s, "credential.helper") { + strings.Contains(s, npmSecret) || d.Name() == "git-credentials" || d.Name() == ".npmrc" || + strings.Contains(s, "credential.helper") { leaks = append(leaks, path) } return nil @@ -137,10 +139,20 @@ func TestACheckContainerSeesNoForgeCredential(t *testing.T) { } return "", errors.New("unexpected command " + name) } - // A credential an older builder left in the workspace is removed too. + // A credential an older builder left in the workspace is removed too: the forge's, and the .npmrc a + // build wrote into the tree it cloned. if err := os.WriteFile(filepath.Join(workspace, "git-credentials"), []byte(forgeURL+"\n"), 0o600); err != nil { t.Fatal(err) } + for _, dir := range []string{"source/x", "context-server"} { + if err := os.MkdirAll(filepath.Join(workspace, dir), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(workspace, dir, ".npmrc"), + []byte("//forge.invalid/api/packages/novox/npm/:_authToken="+npmSecret+"\n"), 0o600); err != nil { + t.Fatal(err) + } + } _, err := Check(t.Context(), run, CheckSpec{ID: "check-462", Repository: repo, Ref: head, Owner: "novox", Repo: "mesh-controller", Number: 1, Toolchain: "golang", Beside: map[string]Beside{ "mesh-catalog": {Repository: besideURL, Ref: besideHead}}}, workspace, aFactsRegistry(t), @@ -198,3 +210,42 @@ func TestTheRedactorHidesTheForgeCredentialAndShapes(t *testing.T) { } } } + +// A build's clone of a URL carrying userinfo records it without, so no build leaves a credential in +// workspace/source/.git/config while it runs either (novox/hq issue 462); the tree itself is gone when the +// build ends. +func TestABuildsCloneRecordsNoUserinfo(t *testing.T) { + repo, _ := aCheckedRepository(t, map[string]string{ManifestName: `{"module":"plain","version":"1"}`}) + source := "file://build-user:" + besideSecret + "@" + repo + workspace := t.TempDir() + tree := filepath.Join(workspace, "source") + var configs []string + run := func(ctx context.Context, dir, name string, args ...string) (string, error) { + if name == "git" && hasString(args, "clone") && args[len(args)-2] == source { + clone := append(append([]string{}, args[:len(args)-2]...), bareURL(t, source), args[len(args)-1]) + if out, err := Command(ctx, dir, "git", clone...); err != nil { + return out, err + } + return Command(ctx, args[len(args)-1], "git", "remote", "set-url", "origin", source) + } + if name == "git" && len(args) > 0 && args[0] == "rev-parse" { + raw, _ := os.ReadFile(filepath.Join(tree, ".git", "config")) + configs = append(configs, string(raw)) + } + return Command(ctx, dir, name, args...) + } + if _, err := Build(t.Context(), run, &recorded{}, source, "", "", workspace, nil, Npmrc{}, GitCredential{}, nil); err != nil { + t.Fatal(err) + } + if len(configs) == 0 { + t.Fatal("the build never read its clone") + } + for _, c := range configs { + if strings.Contains(c, besideSecret) || strings.Contains(c, "build-user") { + t.Fatalf("the build's clone records the credential it was cloned with:\n%s", c) + } + } + if _, err := os.Stat(tree); !os.IsNotExist(err) { + t.Fatalf("the build's tree outlives the build: %v", err) + } +} diff --git a/internal/builder/packages_test.go b/internal/builder/packages_test.go index a87f19fd..7cccbf16 100644 --- a/internal/builder/packages_test.go +++ b/internal/builder/packages_test.go @@ -70,7 +70,16 @@ func TestNpmrcDisabledUntilThereIsARegistry(t *testing.T) { func TestAnImageBuildGetsTheCredentialInTheContextAndHostNetwork(t *testing.T) { r, workspace := aRepository(t, withBoth, map[string]string{"Dockerfile": "FROM scratch\nCOPY .npmrc ./", "files/x": "y"}) n := Npmrc{Scope: "@novox", Registry: "https://forge.invalid/api/packages/novox/npm/", Token: "t"} - if _, err := Build(context.Background(), r.run, r, + npmrc := filepath.Join(workspace, "source", ".npmrc") + inContext := false + run := func(ctx context.Context, dir, name string, args ...string) (string, error) { + if name == "docker" && len(args) > 0 && args[0] == "build" { + _, err := os.Stat(npmrc) + inContext = err == nil + } + return r.run(ctx, dir, name, args...) + } + if _, err := Build(context.Background(), run, r, "https://forge.invalid/meshboard.git", "", "", workspace, nil, n, GitCredential{}, nil); err != nil { t.Fatalf("the build failed: %v", err) } @@ -90,11 +99,14 @@ func TestAnImageBuildGetsTheCredentialInTheContextAndHostNetwork(t *testing.T) { if !strings.Contains(build, "--network host") { t.Fatalf("the build was not given the host network to reach the registry: %s", build) } - // The .npmrc is written into the build context (the source tree), where a Dockerfile COPYs it. - tree := filepath.Join(workspace, "source") - npmrc := filepath.Join(tree, ".npmrc") - if _, err := os.Stat(npmrc); err != nil { - t.Fatalf("the credential was not written into the build context: %v", err) + // The .npmrc is written into the build context (the source tree), where a Dockerfile COPYs it, and + // removed with the tree when the build ends: a later check's container mounts the workspace (novox/hq + // issue 462). + if !inContext { + t.Fatal("the credential was not in the build context when the image was built") + } + if _, err := os.Stat(npmrc); !os.IsNotExist(err) { + t.Fatalf("the credential outlives the build in the workspace: %v", err) } } From 483c387b724954853f78b0433a227677cc9312a2 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 18:46:31 +0200 Subject: [PATCH 8/8] Judge the runtime's daemon.json against the docker module's own keys, not a frozen copy The test pinned daemon.json to live-restore alone, so every controller check failed once the docker module gained log rotation as a preference (mesh-catalog #205, #217). It still refuses dns (ADR 0196) and any key the module does not declare, and checks the log defaults where the module declares them (novox/hq issue 498). --- internal/catalogue/resolver_manifests_test.go | 50 ++++++++++++++++++- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/internal/catalogue/resolver_manifests_test.go b/internal/catalogue/resolver_manifests_test.go index 8ea3e32b..7d149596 100644 --- a/internal/catalogue/resolver_manifests_test.go +++ b/internal/catalogue/resolver_manifests_test.go @@ -194,8 +194,31 @@ func TestTheResolverAndWhatAsksItComposeOnOneMachine(t *testing.T) { if err := json.Unmarshal([]byte(runtime["content"].(string)), &keys); err != nil { t.Fatalf("the runtime's keys are not JSON: %v", err) } - if len(keys) != 1 || keys["live-restore"] != true { - t.Errorf("the runtime is given %v; live-restore and nothing else", keys) + // What the runtime is given is what its own module declares, and nothing a layer or another module + // adds. This used to say "live-restore and nothing else", to hold the line ADR 0196 drew when `dns` + // left the file; since the docker module gained log rotation as a preference (mesh-catalog #205 and + // #217, novox/hq issue 452) that frozen copy failed every check (novox/hq issue 498). So the keys are + // judged against the module's own daemon.json instead: live-restore is given, `dns` never is, and no + // key the module does not declare appears. + if keys["live-restore"] != true { + t.Errorf("the runtime is given %v; a restart must keep every container running (live-restore)", keys) + } + if _, ok := keys["dns"]; ok { + t.Errorf("the runtime is given a resolver of its own, %v; a container copies its machine's (ADR 0196)", keys["dns"]) + } + declared := daemonKeysDeclared(t) + for key := range keys { + if !declared[key] { + t.Errorf("the runtime is given %q, which the docker module does not declare: %v", key, keys) + } + } + // The log rotation preference, at its defaults, wherever the module declares it. The copy captured in + // testdata/beside predates it, so outside a merge check it is not there to judge. + if declared["log-driver"] { + opts, _ := keys["log-opts"].(map[string]any) + if keys["log-driver"] != "json-file" || opts["max-size"] != "100m" || opts["max-file"] != "10" { + t.Errorf("the runtime is given %v; its containers' logs are cut at 100m and ten are kept (novox/hq issue 452)", keys) + } } // The runtime is reloaded when that file changes, and never restarted: a restart stops every // container on the machine (ADR 0102), and a reload is what turns live-restore on. @@ -275,3 +298,26 @@ func TestTheResolverOnAMachineOffTheNetworkIsRefused(t *testing.T) { t.Fatalf("a machine off the network was refused for another reason: %v", err) } } + +// daemonKeysDeclared is the top-level keys of the docker module's own daemon.json, read from its +// manifest: what the runtime may be given (novox/hq issue 498). A placeholder sits inside a JSON +// string there, so the content parses as it is. +func daemonKeysDeclared(t *testing.T) map[string]bool { + t.Helper() + for _, r := range catalogueManifest(t, "docker").Resources { + if r["id"] != "daemon" { + continue + } + var keys map[string]any + if err := json.Unmarshal([]byte(r["content"].(string)), &keys); err != nil { + t.Fatalf("the docker module's daemon.json is not JSON: %v", err) + } + declared := map[string]bool{} + for key := range keys { + declared[key] = true + } + return declared + } + t.Fatal("the docker module declares no daemon.json") + return nil +}