Hold the trust-before-push order by a test, and refuse early what would stop it (issue 92)

The fix is an order, and nothing failed if the trust call moved after the
push or went: steps 7 and 8 are now one function a test drives against a
runtime that refuses an untrusted push. Preflight now refuses, before
anything is raised, a runtime file that is no JSON object, a docker.service
that is not loaded and active, and a dockerd started with
--insecure-registry, which would refuse the reload. The comment names the
order the docker module's takeover rests on.

internal/bootstrap/bootstrap.go

@@ -36,6 +36,7 @@ import (
 	"time"
 
 	"github.com/novox/mesh-host/internal/firewall"
+	"github.com/novox/mesh-host/internal/system"
 	"github.com/novox/mesh-host/internal/tunnel"
 )
 
@@ -658,33 +659,10 @@ func Run(ctx context.Context, o Options, d Deps, say func(string)) (Result, erro
 		return result, failed(StepOperator, err)
 	}
 
-	// ---- 7. registry ----------------------------------------------------------------------
-	say("registry — somewhere for this mesh to keep its own images")
-	// Trusted by the runtime before anything is pushed to it, by every name the mesh will use for
-	// it here: the module that writes this trust is applied long after the push (novox/hq issue 92).
-	trusted, err := TrustTheRegistry(ctx, o, sys, d.Run, say)
-	result.RegistryTrusted = trusted
+	// ---- 7. registry, 8. publish -----------------------------------------------------------
+	published, err := RegistryAndPublish(ctx, o, d, sys, temporary, controlPlaneImage, &result, say)
 	if err != nil {
-		return result, failed(StepRegistry, err)
-	}
-	registry, err := InstallRegistry(ctx, o, d, temporary, say)
-	result.Registry, result.RegistryRunning = registry.Address, registry.Container
-	result.RegistryKnown, result.RegistryReplied = registry.Known, registry.Answered
-	if err != nil {
-		return result, failed(StepRegistry, err)
-	}
-
-	// ---- 8. publish -----------------------------------------------------------------------
-	say("publish — the control plane's image gets its first manifest digest")
-	// **The image this mesh built, not the one the installer carried.** The carried one is the
-	// builder; publishing it here would put a builder in the registry under the control plane's
-	// name and install it as the control plane — which is exactly what happened the first time
-	// this ran, and presented as a control plane that started, printed a builder's usage, and
-	// exited cleanly over and over.
-	published, err := PublishControlPlane(ctx, o, d, controlPlaneImage, say)
-	result.PublishedAs, result.PublishedAlready = published.Reference, published.Already
-	if err != nil {
-		return result, failed(StepPublish, err)
+		return result, err
 	}
 
 	// ---- 9. control plane -----------------------------------------------------------------
@@ -814,3 +792,44 @@ func Run(ctx context.Context, o Options, d Deps, say func(string)) (Result, erro
 
 	return result, nil
 }
+
+// RegistryAndPublish is steps 7 and 8: the runtime trusts the mesh's registry, the registry is
+// installed and answers, and the control plane's image is pushed into it.
+//
+// **One function so the order is held by a test** (novox/hq issue 92). The runtime refuses plain
+// HTTP to a registry it has not been told to trust, and the module that tells it is applied long
+// after the push — so the trust is written and confirmed here, before anything is tagged or pushed.
+// A refusal names the step it happened in.
+func RegistryAndPublish(ctx context.Context, o Options, d Deps, sys system.System, temporary controlPlane,
+	controlPlaneImage string, result *Result, say func(string)) (Published, error) {
+
+	// ---- 7. registry ----------------------------------------------------------------------
+	say("registry — somewhere for this mesh to keep its own images")
+	// Trusted by the runtime before anything is pushed to it, by every name the mesh will use for
+	// it here: the module that writes this trust is applied long after the push (novox/hq issue 92).
+	trusted, err := TrustTheRegistry(ctx, o, sys, d.Run, say)
+	result.RegistryTrusted = trusted
+	if err != nil {
+		return Published{}, failed(StepRegistry, err)
+	}
+	registry, err := InstallRegistry(ctx, o, d, temporary, say)
+	result.Registry, result.RegistryRunning = registry.Address, registry.Container
+	result.RegistryKnown, result.RegistryReplied = registry.Known, registry.Answered
+	if err != nil {
+		return Published{}, failed(StepRegistry, err)
+	}
+
+	// ---- 8. publish -----------------------------------------------------------------------
+	say("publish — the control plane's image gets its first manifest digest")
+	// **The image this mesh built, not the one the installer carried.** The carried one is the
+	// builder; publishing it here would put a builder in the registry under the control plane's
+	// name and install it as the control plane — which is exactly what happened the first time
+	// this ran, and presented as a control plane that started, printed a builder's usage, and
+	// exited cleanly over and over.
+	published, err := PublishControlPlane(ctx, o, d, controlPlaneImage, say)
+	result.PublishedAs, result.PublishedAlready = published.Reference, published.Already
+	if err != nil {
+		return published, failed(StepPublish, err)
+	}
+	return published, nil
+}

internal/bootstrap/preflight.go

@@ -110,6 +110,13 @@ func Preflight(ctx context.Context, o Options, d Deps, say func(string)) ([]byte
 	if err := RefuseAMachineInUse(ctx, o, d.Run, say); err != nil {
 		return nil, err
 	}
+	// The registry's trust is written at step 7, before the push (novox/hq issue 92). What can be
+	// known about whether it will take, without writing anything, is asked now.
+	if o.pivots() {
+		if err := RuntimeWillTakeTheTrust(ctx, o, d.Run, say); err != nil {
+			return nil, err
+		}
+	}
 
 	// 4. Can this machine reach what the bundle's images come from?
 	//

internal/bootstrap/trust.go

@@ -31,10 +31,23 @@ import (
 // could hold it, because the retire step re-applies the bundle and takes away a carried record the
 // bundle does not name. The `docker` module, applied later, declares the artifact store's reach —
 // the mesh name on the node's port — finds it already in the file it has no record of, and takes it
-// as its own (ADR 0102: a declared member found in an unrecorded file is the mesh's). So nothing
-// changes on the machine at the handover, no reload; from then on the module owns that member. The
-// address the operator gave, when it is not the mesh name, stays as part of the machine's own
-// configuration, as the hand-written trust it replaces would have.
+// as its own (ADR 0102: a declared member found in an unrecorded file is the mesh's). That member
+// does not change at the handover. The module's first apply usually still changes the file and
+// reloads the runtime once, for the keys it writes beside it (`live-restore`, `log-driver`,
+// `log-opts`); a reload stops nothing. The address the operator gave, when it is not the mesh name,
+// stays as part of the machine's own configuration, as the hand-written trust it replaces would have.
+//
+// **This rests on an assumption about order.** The module takes the mesh name's member over only
+// because its FIRST apply on this node already declares it: genesis assigns no `docker` before the
+// network step (16), and once the node is on the network the controller composes the reach as
+// `<node>.<suffix>:<port>`.
+// Were `docker` applied earlier — while the reach is empty or `127.0.0.1:<port>`, as it is before the
+// network — its record would exist without the mesh name, and the member would stay the machine's
+// for good: undeclaring the module would never take it out. Installing the node's seat holders
+// earlier in genesis must revisit this.
+//
+// **The suffix** is read from this process's environment, and the controller reads its own: a mesh
+// with a custom `MESH_INTERNAL_SUFFIX` sets it for both.
 
 // runtimeConfig is the container runtime's configuration, which the trust is written into. A
 // variable so a test writes into a file of its own.
@@ -230,3 +243,75 @@ func inAny(ip net.IP, cidrs []string) bool {
 	}
 	return false
 }
+
+// procRoot is where a process's command line is read from. A variable so a test gives its own.
+var procRoot = "/proc"
+
+// RuntimeWillTakeTheTrust is preflight's half of the registry's trust: everything that can be known
+// about it without writing anything, asked at step 1 so a machine that cannot take the trust is
+// refused before the foundation is raised, rather than at step 7 after it (novox/hq issue 92).
+//
+//   - The runtime's file is absent or a JSON object: the mesh writes into an object, and refuses to
+//     write over anything else (ADR 0102).
+//   - The runtime's service is known to systemd and active: the trust is taken by reloading it.
+//   - The runtime was not started with `--insecure-registry`: dockerd refuses a directive given both
+//     as a flag and in its file, so it would refuse the reload and keep distrusting the registry.
+func RuntimeWillTakeTheTrust(ctx context.Context, o Options, run Runner, say func(string)) error {
+	names := RegistryNames(o)
+	if len(names) == 0 {
+		return nil
+	}
+	if raw, err := os.ReadFile(runtimeConfig); err == nil {
+		var object map[string]json.RawMessage
+		if len(strings.TrimSpace(string(raw))) > 0 {
+			if err := json.Unmarshal(raw, &object); err != nil || object == nil {
+				return fmt.Errorf("%s is not a JSON object, and the mesh writes the trust of its registry "+
+					"(%s) into it beside what is there, never over it (novox/hq ADR 0102). Make it an object "+
+					"or remove it; nothing has been changed", runtimeConfig, strings.Join(names, ", "))
+			}
+		}
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("the runtime's configuration %s cannot be read: %w", runtimeConfig, err)
+	}
+
+	asking, cancel := context.WithTimeout(ctx, o.Timeout)
+	out, err := run(asking, "systemctl", "show", runtimeUnit, "--property=LoadState,ActiveState,MainPID")
+	cancel()
+	if err != nil {
+		return fmt.Errorf("systemd could not be asked about %s: %w\nThe registry's trust is taken by "+
+			"reloading it; nothing has been changed", runtimeUnit, err)
+	}
+	props := map[string]string{}
+	for _, line := range strings.Split(out, "\n") {
+		if k, v, ok := strings.Cut(strings.TrimSpace(line), "="); ok {
+			props[k] = v
+		}
+	}
+	if props["LoadState"] != "loaded" || props["ActiveState"] != "active" {
+		return fmt.Errorf("%s is %s and %s, and the registry's trust (%s) is taken by reloading it. "+
+			"Genesis needs the runtime run by systemd as %s; nothing has been changed",
+			runtimeUnit, orUnknown(props["LoadState"]), orUnknown(props["ActiveState"]),
+			strings.Join(names, ", "), runtimeUnit)
+	}
+	if pid := props["MainPID"]; pid != "" && pid != "0" {
+		if cmdline, err := os.ReadFile(procRoot + "/" + pid + "/cmdline"); err == nil {
+			for _, arg := range strings.Split(string(cmdline), "\x00") {
+				if strings.HasPrefix(arg, "--insecure-registry") {
+					return fmt.Errorf("the runtime was started with %s, and dockerd refuses insecure registries "+
+						"given both as a flag and in %s — so it would refuse the reload that makes it trust "+
+						"%s. Move the flag's registries into the file and restart the runtime once; nothing "+
+						"has been changed", arg, runtimeConfig, strings.Join(names, ", "))
+				}
+			}
+		}
+	}
+	say("  registry trust    " + runtimeConfig + " can be written into and " + runtimeUnit + " reloaded")
+	return nil
+}
+
+func orUnknown(s string) string {
+	if s == "" {
+		return "unknown"
+	}
+	return s
+}

internal/bootstrap/trust_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
@@ -218,3 +219,129 @@ func TestARangeTheRuntimeTrustsCoversAnAddressInIt(t *testing.T) {
 		t.Errorf("missing %v", missing)
 	}
 }
+
+// **The order, which is the fault issue 92 is about.** Steps 7 and 8 against a runtime that behaves
+// as dockerd does: a push to a registry it has not been told to trust is refused with the error the
+// first genesis met. The trust must be written and confirmed before anything is tagged or pushed;
+// moving the trust after the push, or dropping it, fails this test.
+func TestNothingIsPushedToTheRegistryBeforeTheRuntimeTrustsIt(t *testing.T) {
+	o, runtime, sys := trustSetup(t, `{"data-root":"/srv/docker"}`)
+	o.Catalogue = catalogueWith(t, RegistryModule, upstreamRegistryManifest)
+	o.Registry = "192.0.2.10:5100"
+
+	pushed := false
+	mesh := aMeshThatAgrees(nil)
+	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		if name == "systemctl" || (name == "docker" && args[0] == "info") {
+			return runtime.run(ctx, name, args...)
+		}
+		runtime.commands = append(runtime.commands, name+" "+strings.Join(args, " "))
+		if name == "docker" {
+			switch args[0] {
+			case "tag":
+				return "", nil
+			case "push":
+				trusted := false
+				for _, n := range runtime.trusted {
+					trusted = trusted || strings.HasPrefix(args[1], n+"/")
+				}
+				if !trusted {
+					return "", fmt.Errorf(`Get "https://%s/v2/": http: server gave HTTP response to HTTPS client`, o.Registry)
+				}
+				pushed = true
+				return "", nil
+			case "image":
+				return `["` + o.Registry + `/mesh-controller@sha256:` + strings.Repeat("a", 64) + `"]`, nil
+			}
+		}
+		return mesh(name, args)
+	}
+	d := Deps{Run: run, Fetch: func(_ context.Context, url string) (int, string, error) {
+		if strings.HasSuffix(url, "/v2/") || pushed {
+			return http.StatusOK, `{"name":"mesh-controller","tags":["genesis"]}`, nil
+		}
+		return http.StatusNotFound, "", nil
+	}}
+	var result Result
+	published, err := RegistryAndPublish(context.Background(), o, d, sys,
+		controlPlane{container: "temp-mesh-controller", run: run, timeout: time.Second}, held, &result, func(string) {})
+	if err != nil {
+		t.Fatalf("genesis did not get past the push: %v\ncommands: %v", err, runtime.commands)
+	}
+	if !strings.HasPrefix(published.Reference, o.Registry+"/mesh-controller@sha256:") {
+		t.Errorf("published as %q", published.Reference)
+	}
+
+	firstPush, reloaded, confirmed := -1, -1, -1
+	for i, c := range runtime.commands {
+		switch {
+		case firstPush < 0 && (strings.HasPrefix(c, "docker tag") || strings.HasPrefix(c, "docker push")):
+			firstPush = i
+		case strings.HasPrefix(c, "systemctl") && strings.Contains(c, "reload"):
+			reloaded = i
+		case strings.HasPrefix(c, "docker info"):
+			confirmed = i
+		}
+	}
+	if firstPush < 0 || reloaded < 0 || confirmed < 0 || !(reloaded < confirmed && confirmed < firstPush) {
+		t.Errorf("the trust was not written (%d) and confirmed (%d) before the first tag or push (%d): %v",
+			reloaded, confirmed, firstPush, runtime.commands)
+	}
+	if fmt.Sprint(result.RegistryTrusted) != "[192.0.2.10:5100 anchor.internal:5100]" {
+		t.Errorf("the result says it trusted %v", result.RegistryTrusted)
+	}
+}
+
+// Preflight asks, without writing, what would stop the trust from taking at step 7.
+func TestPreflightRefusesARuntimeThatCouldNotTakeTheTrust(t *testing.T) {
+	proc := t.TempDir()
+	was := procRoot
+	procRoot = proc
+	t.Cleanup(func() { procRoot = was })
+	if err := os.MkdirAll(filepath.Join(proc, "42"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	unit := func(load, active string) Runner {
+		return func(_ context.Context, name string, args ...string) (string, error) {
+			return "LoadState=" + load + "\nActiveState=" + active + "\nMainPID=42\n", nil
+		}
+	}
+	cmdline := func(args ...string) {
+		if err := os.WriteFile(filepath.Join(proc, "42", "cmdline"), []byte(strings.Join(args, "\x00")+"\x00"), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	cmdline("/usr/bin/dockerd", "-H", "fd://")
+
+	for _, c := range []struct {
+		name, file string
+		run        Runner
+		flag       bool
+		refused    string
+	}{
+		{"a runtime that can take it", `{"data-root":"/srv/docker"}`, unit("loaded", "active"), false, ""},
+		{"no file yet", "", unit("loaded", "active"), false, ""},
+		{"a file that is not an object", `["not", "an", "object"]`, unit("loaded", "active"), false, "not a JSON object"},
+		{"a runtime systemd does not run", `{}`, unit("not-found", "inactive"), false, "not-found and inactive"},
+		{"a runtime with the flag", `{}`, unit("loaded", "active"), true, "--insecure-registry=10.0.0.1:5000"},
+	} {
+		t.Run(c.name, func(t *testing.T) {
+			o, _, _ := trustSetup(t, c.file)
+			if c.flag {
+				cmdline("/usr/bin/dockerd", "--insecure-registry=10.0.0.1:5000")
+				defer cmdline("/usr/bin/dockerd", "-H", "fd://")
+			}
+			before, _ := os.ReadFile(runtimeConfig)
+			err := RuntimeWillTakeTheTrust(context.Background(), o, c.run, func(string) {})
+			switch {
+			case c.refused == "" && err != nil:
+				t.Errorf("refused: %v", err)
+			case c.refused != "" && (err == nil || !strings.Contains(err.Error(), c.refused)):
+				t.Errorf("wanted a refusal naming %q, got %v", c.refused, err)
+			}
+			if after, _ := os.ReadFile(runtimeConfig); string(after) != string(before) {
+				t.Error("preflight wrote the runtime's file")
+			}
+		})
+	}
+}