diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index 51dcd1b4..4ad26abc 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -313,8 +313,9 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri registry = r.Address } spec := checkSpecOf(request) - // The judge cache the build agent declares as its own data, never in the workspace: only judge builds mount - // it, so no pull request's script can write what a judge is built with or kept as (novox/hq ADR 0358 §6). + // The judge cache the build agent declares as its own data, never in the workspace: only judge builds and + // mesh-lab's replays mount it, never a check's containers, so no pull request's script can write what a judge + // is built with or kept as, or what the replays run (novox/hq ADR 0358 §6, issue 709). spec.JudgeCache = judgeCache() // What the merge would move, built by the check for the walk to take (novox/hq ADR 0350 §1): into the // scratch namespace. **Never with the package registry's credential**: this is code nobody has approved diff --git a/internal/builder/check.go b/internal/builder/check.go index 898e1d37..fc368569 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -335,7 +335,16 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if err != nil { return CheckVerdict{}, err } - defer os.RemoveAll(replay.base) + if spec.JudgeCache != "" { + defer os.RemoveAll(replay.base) + } + defer func() { + if replay.volumeMade { + removing, done := context.WithTimeout(context.Background(), time.Minute) + defer done() + _, _ = run(removing, root, "docker", "volume", "rm", "--force", replay.volume) + } + }() // cloneAt clones a repository and checks out ref — or, when the repository has no branch ref and orElse is said, // orElse (novox/hq ADR 0358 §3); readAt keeps which, by directory. readAt := map[string]string{} @@ -454,9 +463,10 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if err := cloneAt(b.Repository, b.Ref, b.Else, dir); err != nil { return CheckVerdict{}, fmt.Errorf("beside it, %w", err) } - if dir == "mesh-lab" && replay.lab != "" { - // The copy the replays run from, made before anything of the change runs, outside every check container. - if err := copyCheckout(ctx, run, filepath.Join(root, dir), replay.lab); err != nil { + if dir == "mesh-lab" { + // The copy the replays run from, made before anything of the change runs, outside every check container + // (novox/hq issue 709) — and whether it holds replays, read now, so nothing written later decides it. + if err := replay.copyLab(ctx, run, spec, root, workspace, filepath.Join(root, dir)); err != nil { return CheckVerdict{}, fmt.Errorf("mesh-lab could not be copied for its replays: %w", err) } } @@ -630,16 +640,14 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry return append([]string{args[0], "--volume", judgeHome + ":" + judgeHome, "--volume", judgeGoCache + ":" + judgeGoCache}, args[1:]...) } - replayLab := filepath.Join(root, "mesh-lab") - if replay.lab != "" { - replayLab = replay.lab - } inReplays := func(env []string, command ...string) []string { - args := in(spec.Toolchain, filepath.Join(replayLab, "replays"), append(ReplayEnv(replay.home, replay.goCache, + args := in(spec.Toolchain, filepath.Join(replay.lab, "replays"), append(ReplayEnv(replay.home, replay.goCache, replay.modCache), env...), command...) // The socket goes to the replays alone, never to the change's own code. mounts := []string{"--volume", "/var/run/docker.sock:/var/run/docker.sock"} - if spec.JudgeCache != "" { + if replay.volume != "" { + mounts = append(mounts, "--volume", replay.volume+":"+replay.base) + } else { mounts = append(mounts, "--volume", replay.base+":"+replay.base, "--volume", replay.goCache+":"+replay.goCache, "--volume", replay.modCache+":"+replay.modCache) } @@ -698,7 +706,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry say("check", "the gate: %d module(s) of the graph touched — %s", len(modules), strings.Join(modules, ", ")) fmt.Fprintf(&out, "--- the gate: %s\n", strings.Join(modules, ", ")) v.Gate.Verdict, v.Gate.Summary = gateLayer(ctx, spec, tree, root, gate, verdictFile, env, inToolchain, - running, &out, bus, name, replayLab, inReplays, say) + running, &out, bus, name, replay.hasReplays, inReplays, say) } if timedOut() { v.Gate.Verdict, v.Gate.Summary = "error", fmt.Sprintf("the check ran past %s and was ended", CheckTimeout) @@ -976,7 +984,7 @@ const SomeManifestsEnv = "MESH_MODULE_CHECK_SOME=1" // change, and the replays of what the mesh runs. It answers the gate's verdict and summary. func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFile string, env []string, inToolchain func(string, []string, ...string) []string, running func([]string) error, out *tail, bus, - name, replayLab string, inReplays func([]string, ...string) []string, + name string, hasReplays bool, inReplays func([]string, ...string) []string, say func(step, format string, args ...any)) (string, string) { // 1. The manifests the change touches, as the judge reads them: a manifest it cannot read, or one with a // problem the change brings, fails here. **What was already so on the base branch is said and fails @@ -1069,8 +1077,8 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil // 3. **The replays of what the mesh runs** (to-be 45 §9, M9): mesh-lab's, from its main — reviewed code, // so given the container runtime the resolver replay raises containers with — against the bus of the // release the mesh runs and the change's own catalogue when the change is to the catalogue. - if lab := filepath.Join(replayLab, "replays"); ctx.Err() == nil { - if _, err := os.Stat(lab); err == nil { + if ctx.Err() == nil { + if hasReplays { catalogue := filepath.Join(root, "mesh-catalog") if name == "mesh-catalog" { catalogue = tree @@ -1253,38 +1261,39 @@ func judgeBuildDirs(cache, workspace, root, id string) (string, string, error) { return home, goCache, nil } -// replayPlace is where mesh-lab's replays run from and with (novox/hq issue 709): base, the directory made for this -// check and removed after it; lab, the copy of mesh-lab they run (empty: the checkout beside the check); home, their -// HOME and configuration; and the Go build and module caches. +// replayPlace is where mesh-lab's replays run from and with (novox/hq issue 709), never anything a pull request can +// write: base, the place made for this check; lab, the copy of mesh-lab they run; home, their HOME and configuration; +// and their Go build and module caches. With a judge cache, base is a directory under its homes/ (the paths the same +// on the host and in the build agent) and the caches are the judge cache's. Without one, base is where a container +// volume of this check's own is mounted (volume), holding the copy, the home and throwaway caches, removed after the +// check: the build agent declares no other host path outside the workspace. hasReplays says whether mesh-lab held +// replays when it was copied, before anything of the change ran. type replayPlace struct { base, lab, home, goCache, modCache string + volume string + volumeMade, hasReplays bool } -// replayDirs makes the replays' place (issue 709, ADR 0358 §6). With a judge cache: a directory under its homes/ for -// this check, holding a copy of mesh-lab made before anything of the change runs and a fresh home, and the judge -// cache's Go build and module caches — none of which a check's container mounts. The replays' build is as trusted as a -// judge's: it compiles mesh-lab's trunk, reviewed code, and nothing of the pull request. Without a judge cache: a home -// and caches under the check's root, which every check empties before it clones, and the replays run from the -// checkout beside the check, as before. +// ReplayMount is where a check's own replay volume is mounted in the replays' container when there is no judge cache. +var ReplayMount = "/mesh-replays" + +// replayDirs makes the replays' place (issue 709, ADR 0358 §6). The replays' build is as trusted as a judge's: it +// compiles mesh-lab's trunk, reviewed code, and nothing of the pull request, which hands them only its catalogue, as +// data. Never the workspace: the gate runs before the replays, a pull request's own code among its containers. func replayDirs(cache, workspace, root, id string) (replayPlace, error) { + if !safeName.MatchString(id) { + id = "check" + } if cache == "" { - p := replayPlace{base: filepath.Join(root, "replay-home"), goCache: filepath.Join(root, "judge-go-cache"), - modCache: filepath.Join(root, "replay-go-modules")} - p.home = p.base - for _, d := range []string{p.home, p.goCache, p.modCache} { - if err := os.MkdirAll(d, 0o700); err != nil { - return replayPlace{}, err - } - } - return p, nil + base := ReplayMount + return replayPlace{base: base, lab: filepath.Join(base, "mesh-lab"), home: filepath.Join(base, "home"), + goCache: filepath.Join(base, "go-cache"), modCache: filepath.Join(base, "go-modules"), + volume: "mesh-check-" + strings.ToLower(id) + "-replays"}, nil } if withinDir(workspace, cache) { return replayPlace{}, fmt.Errorf("the judge cache %s is inside the workspace %s, which a check's containers "+ "mount: no replay is run where a pull request could reach it", cache, workspace) } - if !safeName.MatchString(id) { - id = "check" - } base := filepath.Join(cache, "homes", id+"-replays") p := replayPlace{base: base, lab: filepath.Join(base, "mesh-lab"), home: filepath.Join(base, "home"), goCache: filepath.Join(cache, "go-cache"), modCache: filepath.Join(cache, "go-modules")} @@ -1299,6 +1308,36 @@ func replayDirs(cache, workspace, root, id string) (replayPlace, error) { return p, nil } +// copyLab copies mesh-lab's checkout, freshly cloned and before anything of the change runs, to where the replays +// run it. With a judge cache, over git's own transport into its directory. Without one, into a volume of this check's +// own, made now: a container reads the checkout from the workspace, mounted read-only, and copies it with a fresh home +// and empty caches, owned by the user the check's containers run as. +func (p *replayPlace) copyLab(ctx context.Context, run Runner, spec CheckSpec, root, workspace, from string) error { + _, err := os.Stat(filepath.Join(from, "replays")) + p.hasReplays = err == nil + if p.volume == "" { + return copyCheckout(ctx, run, from, p.lab) + } + if spec.Toolchain == "" { + return errors.New("the mesh holds no Go toolchain to copy mesh-lab with") + } + user := spec.User + if user == "" { + user = fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) + } + _, _ = run(ctx, root, "docker", "volume", "rm", "--force", p.volume) // one an earlier delivery left + if _, err := run(ctx, root, "docker", "volume", "create", "--label", BuildLabel+"="+spec.ID, p.volume); err != nil { + return err + } + p.volumeMade = true + script := fmt.Sprintf("mkdir -p %s %s %s %s && cp -a %s/. %s/ && chown -R %s %s", + p.lab, p.home, p.goCache, p.modCache, from, p.lab, user, p.base) + _, err = run(ctx, root, "docker", LabelledArgs("docker", []string{"run", "--rm", "--network", "none", + "--volume", p.volume + ":" + p.base, "--volume", workspace + ":" + workspace + ":ro", "--workdir", "/", + spec.Toolchain, "sh", "-c", script}, spec.ID)...) + return err +} + // ReplayEnv is the environment mesh-lab's replays run in (novox/hq issue 709): home as HOME and configuration, the Go // build and module caches given, no global or system git configuration, no Go environment file, and no version // stamped from git — as a judge is built (JudgeBuildEnv), with the modules the replays fetch. @@ -1306,7 +1345,7 @@ func ReplayEnv(home, goCache, modCache string) []string { return []string{"HOME=" + home, "XDG_CONFIG_HOME=" + filepath.Join(home, ".config"), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1", "GOENV=off", "GOCACHE=" + goCache, "GOMODCACHE=" + modCache, "GOPATH=" + filepath.Join(home, "go"), - "GOFLAGS=-buildvcs=false"} + "GOFLAGS=-buildvcs=false", "GOWORK=off"} } // copyCheckout copies a checkout at the commit it stands at into dir, over git's own transport — never by linking diff --git a/internal/builder/release_branch_test.go b/internal/builder/release_branch_test.go index d1ea2c27..1e560d36 100644 --- a/internal/builder/release_branch_test.go +++ b/internal/builder/release_branch_test.go @@ -129,6 +129,10 @@ func aRuntimeThatBuildsTheJudge(t *testing.T) string { cmd="$1"; shift case "$cmd" in ps|rm|exec) exit 0 ;; +volume) + if [ -n "$VOLUMES" ]; then echo "$*" >> "$VOLUMES"; fi + if [ "$1" = rm ] && [ -n "$REPLAY_MOUNT" ]; then rm -rf "$REPLAY_MOUNT"; fi + exit 0 ;; port) echo "` + bus.Addr().String() + `"; exit 0 ;; run) if [ -n "$RUNS" ]; then echo "$*" >> "$RUNS"; fi @@ -724,3 +728,99 @@ func TestTheReplaysRunWhereNoPullRequestReaches(t *testing.T) { t.Fatalf("the replays left %d entr(ies) under the judge cache's homes", len(left)) } } + +// aSelfJudgingControllerThatWritesIntoTheReplays is a controller pull request whose own judge, while the gate runs it, +// writes a test into the replays beside the check — the reviewer's case for issue 709: code of the pull request's, +// run before the replays, reaching what they run. +func aSelfJudgingControllerThatWritesIntoTheReplays(t *testing.T, marker string) (string, string) { + t.Helper() + files := aJudgeOf("the change's controller") + evil := "package replays\n\nimport (\n\t\"os\"\n\t\"testing\"\n)\n\nfunc TestEvil(t *testing.T) {\n\t_ = os.WriteFile(" + + fmt.Sprintf("%q", marker) + ", []byte(\"ran\"), 0o644)\n}\n" + files["cmd/mesh-controller/main.go"] = strings.Replace(files["cmd/mesh-controller/main.go"], "func main() {", + "func main() {\n\tif len(os.Args) > 2 && os.Args[1] == \"merge-gate\" {\n\t\t_ = os.WriteFile(os.Getenv(\"MESH_CHECK_BESIDE\")+"+ + "\"/mesh-lab/replays/evil_test.go\", []byte("+fmt.Sprintf("%q", evil)+"), 0o644)\n\t\t_ = os.WriteFile("+ + fmt.Sprintf("%q", marker+".wrote")+", nil, 0o644)\n\t}", 1) + return aPullRequestInto(t, files, "main", map[string]string{"cmd/x.txt": "x"}) +} + +// **Without a judge cache, the replays still never read what a pull request wrote** (issue 709, review 12303): the +// copy of mesh-lab, its home and throwaway caches are in a container volume of the check's own, made before anything +// of the change runs and removed after — never the workspace. With a judge cache, under it. In both: a self-judging +// controller pull request writing a test into the replays beside the check, a planted GOFLAGS hook and a poisoned +// cache entry in the workspace neither run nor are read. +func TestTheReplaysNeverReadWhatAPullRequestWroteWithOrWithoutAJudgeCache(t *testing.T) { + for _, withCache := range []bool{false, true} { + t.Run(fmt.Sprintf("judge cache %v", withCache), func(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + replays, volumes := filepath.Join(t.TempDir(), "replays"), filepath.Join(t.TempDir(), "volumes") + t.Setenv("REPLAYS", replays) + t.Setenv("VOLUMES", volumes) + tmp := t.TempDir() + t.Setenv("TMPDIR", tmp) + mount := filepath.Join(t.TempDir(), "mesh-replays") + t.Setenv("REPLAY_MOUNT", mount) + was := ReplayMount + ReplayMount = mount + t.Cleanup(func() { ReplayMount = was }) + registry := aFactsRegistry(t) + workspace, ran := aPlantedWorkspace(t) + if err := os.MkdirAll(filepath.Join(workspace, "go-cache"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(workspace, "go-cache", "README"), []byte("planted"), 0o644); err != nil { + t.Fatal(err) + } + evilRan := filepath.Join(t.TempDir(), "evil-ran") + controller, head := aSelfJudgingControllerThatWritesIntoTheReplays(t, evilRan) + lab, labMain := aLabWithReplays(t) + cache := "" + if withCache { + cache = t.TempDir() + } + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-709-self", Repository: controller, Ref: head, + Owner: "novox", Repo: "mesh-controller", Number: 330, Base: "main", Judge: "self", JudgeCache: cache, + Paths: []string{"cmd/x.txt"}, Modules: []string{"mesh-controller"}, Toolchain: "golang", + Beside: map[string]Beside{"mesh-lab": {Repository: lab, Ref: labMain}}}, workspace, registry, + GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || v.Gate.Verdict != "pass" || !strings.Contains(v.Report, "--- the replays") { + t.Fatalf("the replays did not run and pass: %+v\n%s", v.Gate, v.Report) + } + if _, err := os.Stat(evilRan + ".wrote"); err != nil { + t.Fatalf("the self-judging pull request did not write into the replays beside it, so this proves nothing: %v", err) + } + if _, err := os.Stat(evilRan); err == nil { + t.Fatal("the test the pull request wrote into the replays ran with the socket") + } + if body, _ := os.ReadFile(ran); strings.Contains(string(body), "replays") || strings.Contains(string(body), "mesh-lab") { + t.Fatalf("a hook planted in the workspace ran during the replays:\n%s", body) + } + body, _ := os.ReadFile(replays) + said := strings.TrimSpace(string(body)) + if said == "" || strings.Contains(said, workspace) || !strings.Contains(said, "GOENV=off ") { + t.Fatalf("the replays ran with something of the workspace, or not at all: %q", said) + } + base := mount + if withCache { + base = filepath.Join(cache, "homes", "check-709-self-replays") + } + if !strings.Contains(said, "DIR="+filepath.Join(base, "mesh-lab", "replays")) { + t.Fatalf("the replays did not run from the copy under %s: %s", base, said) + } + vols, _ := os.ReadFile(volumes) + if !withCache && (!strings.Contains(string(vols), "create --label "+BuildLabel+"=check-709-self") || + !strings.Contains(string(vols), "rm --force mesh-check-check-709-self-replays")) { + t.Fatalf("the check's own replay volume was not made and removed: %q", vols) + } + if _, err := os.Stat(base); err == nil { + t.Fatalf("the replays' place %s was left after the check", base) + } + if left, _ := os.ReadDir(tmp); len(left) != 0 { + t.Fatalf("a check left %d entr(ies) in the temporary directory", len(left)) + } + }) + } +}