diff --git a/cmd/mesh-controller/plan_retry.go b/cmd/mesh-controller/plan_retry.go index a8377bc1..df28926e 100644 --- a/cmd/mesh-controller/plan_retry.go +++ b/cmd/mesh-controller/plan_retry.go @@ -288,13 +288,24 @@ 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. 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 + } + 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) @@ -305,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) @@ -525,3 +535,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. 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" } } } ] diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 2c38e005..e142ef29 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -7,8 +7,10 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path" @@ -159,17 +161,18 @@ 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 } + // 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. @@ -177,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 { @@ -248,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") } @@ -441,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) @@ -467,6 +478,96 @@ 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 +} + +// 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) + 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..50b3a476 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,26 @@ 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() + 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: %w", repository, err) + return fmt.Errorf("cannot clone %s: %s", redact.redact(repository), redact.redact(err.Error())) + } + 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 { - 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 +335,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 +494,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 +624,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 +644,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..7fc52aa9 --- /dev/null +++ b/internal/builder/check_credential_test.go @@ -0,0 +1,251 @@ +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" + npmSecret = "npm-s3cret-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) || + strings.Contains(s, npmSecret) || d.Name() == "git-credentials" || d.Name() == ".npmrc" || + 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: 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), + 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) + } + } +} + +// 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/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/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) } } 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 +} 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..f2b10bae --- /dev/null +++ b/internal/builder/registry_wait.go @@ -0,0 +1,137 @@ +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) { + // 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 + } + } +} + +// 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) } 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 +} 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 {