merge fix M4: a run's command binds its executable from a source of its own
The cmd unit and the instance and sidecar templates all bound <ExeRoot>/<iid>/app, and both POST /cmd and a sidecar's start rewrote it through ensureHostExeAt. The cmd lock only excluded other command starts. Around a swap, a command picking build A and a sidecar picking build B could each repoint the other's source before systemd bound it, and the unit would start the wrong validated build. - carlos-instance-cmd@.service binds /run/carlos-control/exe/%I/cmd-app and cmd-live at /opt/carlos/app and /opt/carlos/live, the same places as before, so the command's view inside the namespace is unchanged. - instancectl names them (CmdExeApp, CmdExeLive). POST /cmd lays them out through ensureCmdExeAt (ensureExe with the two names given), so it never writes the host's app or live. - A cmd role stop removes them under the host's cmd lock once the unit is inactive or failed, and the sweep removes them for each cmd unit it stops. - The template is now the seed outside seven named lines. The isolation test names the new bind, and the hardening test expects the cmd template's own sources. Test: 40 concurrent command and sidecar starts of one instance on different staged builds each find their own build at the source their unit binds, and a command's stop removes its sources and leaves the host's app alone. With the two names pointed back at app and live, the same test fails: a command start finds the sidecar's build.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9 files changed,
+215
−19
internal/host/units/carlos-instance-cmd@.service+8 −4internal/host/units_hardening_test.go+10 −7internal/host/units_isolation_test.go+5 −3internal/instancectl/cmd.go+18 −1internal/instancectl/cmd_stagedexe_linux_test.go+116 −1internal/instancectl/hardening.go+12 −2internal/instancectl/names.go+9 −0internal/instancectl/server.go+8 −0internal/instancectl/stagedexe.go+29 −1
diff --git a/internal/host/units/carlos-instance-cmd@.service b/internal/host/units/carlos-instance-cmd@.service| index c3237c770..0c02d9aa8 100644 |
| --- a/internal/host/units/carlos-instance-cmd@.service |
| +++ b/internal/host/units/carlos-instance-cmd@.service |
| @@ -1,5 +1,5 @@ |
| # One `carlos run` command against an isolated exec-backed instance (run |
| -# spec §5.4). It is the seed template outside six lines, and a test holds |
| +# spec §5.4). It is the seed template outside seven lines, and a test holds |
| # it there (TestCmdTemplateEqualsSeedOutsideTheNamedLines): the same uid, |
| # namespace, bind mounts, slice and working directory as the instance, so |
| # the command has exactly the instance's reach. The agent starts it |
| @@ -51,9 +51,13 @@ WorkingDirectory=/data/instances/%I |
| # Its own build, and no other app's (#214 item 2). The namespace block |
| # empties /opt/carlos; these put back /opt/carlos/live/<host>, a link to |
| # ../app, and /opt/carlos/app, which PID 1 binds by chasing the source |
| -# link to this host's live build at every start. The control service |
| -# writes both sources before it starts the unit. |
| -BindReadOnlyPaths=/run/carlos-control/exe/%I/app:/opt/carlos/app /run/carlos-control/exe/%I/live:/opt/carlos/live |
| +# link to the build the run was admitted against at every start. The |
| +# control service writes both sources before it starts the unit. They are |
| +# the command's own, cmd-app and cmd-live, not the app and live the |
| +# instance and sidecar templates bind (merge ruling M4): a sidecar starting |
| +# on another build around a swap can never repoint the source this unit |
| +# is about to bind, nor the reverse. |
| +BindReadOnlyPaths=/run/carlos-control/exe/%I/cmd-app:/opt/carlos/app /run/carlos-control/exe/%I/cmd-live:/opt/carlos/live |
| # --- the namespace (titogo@.service, 2026-07-31 design, on systemd 257) --- |
| ProtectSystem=strict |
diff --git a/internal/host/units_hardening_test.go b/internal/host/units_hardening_test.go| index 152640394..776b93b7b 100644 |
| --- a/internal/host/units_hardening_test.go |
| +++ b/internal/host/units_hardening_test.go |
| @@ -103,16 +103,19 @@ func TestInstanceDenyCoversTheDocEngineFence(t *testing.T) { |
| // Each template binds back exactly its own build (#214 item 2): the control |
| // service writes the two sources before every start (instancectl ensureExe). |
| +// The cmd template binds sources of its own, cmd-app and cmd-live, which no |
| +// other template's start writes (merge ruling M4). |
| func TestInstanceTemplatesBindOnlyTheirOwnBuild(t *testing.T) { |
| - for name, iid := range map[string]string{ |
| - "carlos-instance@.service": "%I", |
| - "carlos-instance-seed@.service": "%I", |
| - "carlos-instance-sidecar@.service": "%I", |
| - "carlos-instance-cmd@.service": "%I", |
| - "carlos-instance-probe.service": "_probe", |
| + for name, src := range map[string][2]string{ |
| + "carlos-instance@.service": {"%I", ""}, |
| + "carlos-instance-seed@.service": {"%I", ""}, |
| + "carlos-instance-sidecar@.service": {"%I", ""}, |
| + "carlos-instance-cmd@.service": {"%I", "cmd-"}, |
| + "carlos-instance-probe.service": {"_probe", ""}, |
| } { |
| u := readUnit(t, name) |
| - want := "BindReadOnlyPaths=/run/carlos-control/exe/" + iid + "/app:/opt/carlos/app /run/carlos-control/exe/" + iid + "/live:/opt/carlos/live" |
| + iid, pre := src[0], src[1] |
| + want := "BindReadOnlyPaths=/run/carlos-control/exe/" + iid + "/" + pre + "app:/opt/carlos/app /run/carlos-control/exe/" + iid + "/" + pre + "live:/opt/carlos/live" |
| if !hasDirective(u, want) { |
| t.Errorf("%s lacks %q", name, want) |
| } |
diff --git a/internal/host/units_isolation_test.go b/internal/host/units_isolation_test.go| index 43487a44f..21745e8a3 100644 |
| --- a/internal/host/units_isolation_test.go |
| +++ b/internal/host/units_isolation_test.go |
| @@ -413,9 +413,10 @@ func TestSameUIDAuditIsCoveredByTheTemplates(t *testing.T) { |
| } |
| } |
| -// The cmd template is the seed template outside six named lines (run spec |
| -// §5.4, codex 4): every sandbox line is the seed's, so a command has |
| -// exactly the reach a seed has. Comments and blank lines are free; every |
| +// The cmd template is the seed template outside seven named lines (run |
| +// spec §5.4, codex 4; the executable mount's own sources, merge ruling |
| +// M4): every sandbox line is the seed's, so a command has exactly the |
| +// reach a seed has. Comments and blank lines are free; every |
| // directive is compared, in order. |
| func TestCmdTemplateEqualsSeedOutsideTheNamedLines(t *testing.T) { |
| directives := func(name string) []string { |
| @@ -436,6 +437,7 @@ func TestCmdTemplateEqualsSeedOutsideTheNamedLines(t *testing.T) { |
| "ExecStopPost=/opt/carlos/carlos instance-exit seed %I": "ExecStopPost=/opt/carlos/carlos instance-exit cmd %I", |
| "StandardOutput=file:/run/carlos/logs/%I.seed.sock": "StandardOutput=file:/run/carlos/logs/%I.cmd.sock", |
| "StandardError=inherit": "StandardError=file:/run/carlos/logs/%I.cmd-err.sock", |
| + "BindReadOnlyPaths=/run/carlos-control/exe/%I/app:/opt/carlos/app /run/carlos-control/exe/%I/live:/opt/carlos/live": "BindReadOnlyPaths=/run/carlos-control/exe/%I/cmd-app:/opt/carlos/app /run/carlos-control/exe/%I/cmd-live:/opt/carlos/live", |
| } |
| seed, cmd := directives("carlos-instance-seed@.service"), directives("carlos-instance-cmd@.service") |
| want := make([]string, len(seed)) |
diff --git a/internal/instancectl/cmd.go b/internal/instancectl/cmd.go| index 9b915ddef..ffd7de516 100644 |
| --- a/internal/instancectl/cmd.go |
| +++ b/internal/instancectl/cmd.go |
| @@ -69,7 +69,10 @@ func (s *Server) handleCmd(w http.ResponseWriter, r *http.Request) { |
| if s.refuseStagedExe(w, req, unit) { |
| return |
| } |
| - if s.refuseStart(w, req.host, unit, func() error { return s.ensureHostExeAt(req.host, req.exe) }) { |
| + // The command's executable is laid out at its own source (merge ruling |
| + // M4), never at the host's app, which an instance or sidecar start may |
| + // be rewriting for a different build at this moment. |
| + if s.refuseStart(w, req.host, unit, func() error { return s.ensureCmdExeAt(req.host, req.exe) }) { |
| return |
| } |
| // A command runs beside an awake instance, whose own start set the |
| @@ -158,3 +161,17 @@ func cmdStatusOf(m map[string]string, want string) CmdStatus { |
| } |
| return st |
| } |
| + |
| +// cleanCmdExe removes host's cmd executable sources after its cmd unit has |
| +// stopped. It holds the host's cmd lock, so a start cannot be laying them |
| +// out meanwhile, and leaves them for a unit that is not inactive or failed |
| +// (a start that has since begun). |
| +func (s *Server) cleanCmdExe(ctx context.Context, host string) { |
| + lock, _ := s.cmdLocks.LoadOrStore(host, new(sync.Mutex)) |
| + lock.(*sync.Mutex).Lock() |
| + defer lock.(*sync.Mutex).Unlock() |
| + switch s.showValue(ctx, "ActiveState", UnitName(RoleCmd, host)) { |
| + case "inactive", "failed": |
| + s.removeCmdExe(provision.InstanceID(host)) |
| + } |
| +} |
diff --git a/internal/instancectl/cmd_stagedexe_linux_test.go b/internal/instancectl/cmd_stagedexe_linux_test.go| index e4dd3d7f1..ff8ccc935 100644 |
| --- a/internal/instancectl/cmd_stagedexe_linux_test.go |
| +++ b/internal/instancectl/cmd_stagedexe_linux_test.go |
| @@ -5,9 +5,11 @@ package instancectl |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| "os" |
| "path/filepath" |
| "strings" |
| + "sync" |
| "testing" |
| "github.com/carlosframework/platform/internal/cgroup" |
| @@ -40,7 +42,8 @@ func TestCmdStartMountsTheServingStagedExe(t *testing.T) { |
| }) |
| c := NewClient(sock) |
| ctx := context.Background() |
| - app := filepath.Join(exeRoot, provision.InstanceID(genHost), "app") |
| + // The command's own source (merge ruling M4), not the host's app. |
| + app := filepath.Join(exeRoot, provision.InstanceID(genHost), CmdExeApp) |
| arm("inv-1") |
| if inv, err := c.CmdExe(ctx, genHost, cgroup.TierDefault.MemoryMax, good); err != nil || inv != "inv-1" { |
| @@ -97,3 +100,115 @@ func TestCmdStartMountsTheServingStagedExe(t *testing.T) { |
| t.Fatal("a command without a build rewrote the populated slice's limit") |
| } |
| } |
| + |
| +// bindRecorder is a systemctl that reads, at each start, the source the |
| +// starting unit binds as its /opt/carlos/app, which is what systemd would |
| +// bind at that moment: cmd-app for a command, app for a sidecar. |
| +type bindRecorder struct { |
| + mu sync.Mutex |
| + dir string // <ExeRoot>/<instance id> |
| + inv int |
| + cmd []string // what each command start would bind |
| + sidecar []string // what each sidecar start would bind |
| +} |
| + |
| +func (r *bindRecorder) run(_ context.Context, args ...string) (string, error) { |
| + line := strings.Join(args, " ") |
| + switch { |
| + case args[0] == "start": |
| + unit := args[len(args)-1] |
| + src, into := "app", &r.sidecar |
| + if strings.HasPrefix(unit, UnitPrefixCmd) { |
| + src, into = CmdExeApp, &r.cmd |
| + } |
| + got, err := os.Readlink(filepath.Join(r.dir, src)) |
| + if err != nil { |
| + got = err.Error() |
| + } |
| + r.mu.Lock() |
| + *into = append(*into, got) |
| + r.inv++ |
| + r.mu.Unlock() |
| + case strings.HasPrefix(line, "show -p InvocationID --value"): |
| + r.mu.Lock() |
| + defer r.mu.Unlock() |
| + return fmt.Sprintf("inv-%d\n", r.inv), nil |
| + case strings.HasPrefix(line, "show -p ActiveState --value"): |
| + return "inactive\n", nil |
| + } |
| + return "", nil |
| +} |
| + |
| +// A command and a sidecar of one instance start at the same time on |
| +// different builds, as around a swap: each start binds its own build, |
| +// because the command's source is its own (merge ruling M4). With one |
| +// shared source, either start could repoint the other's before systemd |
| +// binds it. A stopped command's source is removed. |
| +func TestConcurrentCmdAndSidecarStartsBindTheirOwnBuilds(t *testing.T) { |
| + const host = "shop.example.com" |
| + opt, exeRoot := t.TempDir(), t.TempDir() |
| + stage := func(v string) StagedExe { |
| + path := release.VersionPath(opt, release.OpsAccount, v, "blog") |
| + return StagedExe{Path: path, Version: v, Digest: writeStaged(t, path, "build "+v)} |
| + } |
| + cmdBuild, sidecarBuild := stage("v2"), stage("v3") |
| + rec := &bindRecorder{dir: filepath.Join(exeRoot, provision.InstanceID(host))} |
| + _, sock := serveOnSocket(t, &fakeCtl{answers: map[string]string{}}, fastCmd, func(s *Server) { |
| + s.Systemctl = rec.run |
| + s.ExeRoot, s.OptDir = exeRoot, opt |
| + s.HostApp = func(string) (string, string, error) { return release.OpsAccount, "blog", nil } |
| + }) |
| + c := NewClient(sock) |
| + ctx := context.Background() |
| + const rounds = 40 |
| + var wg sync.WaitGroup |
| + errs := make(chan error, 2*rounds) |
| + wg.Add(2) |
| + go func() { |
| + defer wg.Done() |
| + for range rounds { |
| + if _, err := c.CmdExe(ctx, host, cgroup.TierDefault.MemoryMax, cmdBuild); err != nil { |
| + errs <- fmt.Errorf("CmdExe: %w", err) |
| + } |
| + } |
| + }() |
| + go func() { |
| + defer wg.Done() |
| + for range rounds { |
| + if _, err := c.StartSidecarExe(ctx, host, cgroup.TierDefault.MemoryMax, sidecarBuild); err != nil { |
| + errs <- fmt.Errorf("StartSidecarExe: %w", err) |
| + } |
| + } |
| + }() |
| + wg.Wait() |
| + close(errs) |
| + for err := range errs { |
| + t.Fatal(err) |
| + } |
| + rec.mu.Lock() |
| + defer rec.mu.Unlock() |
| + if len(rec.cmd) != rounds || len(rec.sidecar) != rounds { |
| + t.Fatalf("starts: %d commands, %d sidecars; want %d each", len(rec.cmd), len(rec.sidecar), rounds) |
| + } |
| + for i, got := range rec.cmd { |
| + if got != cmdBuild.Path { |
| + t.Fatalf("command start %d would bind %q, want its own build %q", i, got, cmdBuild.Path) |
| + } |
| + } |
| + for i, got := range rec.sidecar { |
| + if got != sidecarBuild.Path { |
| + t.Fatalf("sidecar start %d would bind %q, want its own build %q", i, got, sidecarBuild.Path) |
| + } |
| + } |
| + if err := c.Stop(ctx, host, RoleCmd); err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, name := range []string{CmdExeApp, CmdExeLive} { |
| + if _, err := os.Lstat(filepath.Join(rec.dir, name)); !os.IsNotExist(err) { |
| + t.Errorf("%s survived the command's stop (%v)", name, err) |
| + } |
| + } |
| + if got, err := os.Readlink(filepath.Join(rec.dir, "app")); err != nil || got != sidecarBuild.Path { |
| + t.Errorf("the command's stop touched the host's app: %q (%v)", got, err) |
| + } |
| +} |
diff --git a/internal/instancectl/hardening.go b/internal/instancectl/hardening.go| index e141cdada..b18035ae8 100644 |
| --- a/internal/instancectl/hardening.go |
| +++ b/internal/instancectl/hardening.go |
| @@ -52,6 +52,16 @@ const ( |
| // anything else found in live/ is removed all the same, because live/ is |
| // bound into a tenant. |
| func ensureExe(root, iid, target, name string) error { |
| + return ensureExeAs(root, iid, target, name, "app", "live") |
| +} |
| + |
| +// ensureExeAs is ensureExe with the two sources named: app and live under |
| +// root/<iid>/. The cmd template binds its own pair (CmdExeApp, CmdExeLive; |
| +// merge ruling M4), so a command's start and an instance's or sidecar's |
| +// start never write a source the other is about to bind. Inside the |
| +// namespace both pairs are bound at the same places, so the link in live |
| +// is ../app either way. |
| +func ensureExeAs(root, iid, target, name, app, liveName string) error { |
| if root == "" { |
| return errors.New("no exe root") |
| } |
| @@ -60,7 +70,7 @@ func ensureExe(root, iid, target, name string) error { |
| return fmt.Errorf("exe: %q/%q is not a name", iid, name) |
| } |
| dir := filepath.Join(root, iid) |
| - live := filepath.Join(dir, "live") |
| + live := filepath.Join(dir, liveName) |
| for _, d := range []string{root, dir, live} { |
| if err := os.Mkdir(d, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { |
| return err |
| @@ -73,7 +83,7 @@ func ensureExe(root, iid, target, name string) error { |
| return fmt.Errorf("exe: %s is not a directory", d) |
| } |
| } |
| - if err := replaceLink(filepath.Join(dir, "app"), target); err != nil { |
| + if err := replaceLink(filepath.Join(dir, app), target); err != nil { |
| return err |
| } |
| if err := replaceLink(filepath.Join(live, name), "../app"); err != nil { |
diff --git a/internal/instancectl/names.go b/internal/instancectl/names.go| index 5a8645eaa..f73ab14c9 100644 |
| --- a/internal/instancectl/names.go |
| +++ b/internal/instancectl/names.go |
| @@ -77,6 +77,15 @@ const ( |
| UnitPrefixCmd = "carlos-instance-cmd@" |
| ) |
| +// CmdExeApp and CmdExeLive are the cmd template's executable mount |
| +// sources under <ExeRoot>/<instance id>, apart from the app and live the |
| +// other original templates bind (merge ruling M4). They are bound at the |
| +// same places in the namespace, /opt/carlos/app and /opt/carlos/live. |
| +const ( |
| + CmdExeApp = "cmd-app" |
| + CmdExeLive = "cmd-live" |
| +) |
| + |
| func prefixFor(role Role) string { |
| switch role { |
| case RoleSeed: |
diff --git a/internal/instancectl/server.go b/internal/instancectl/server.go| index 7f9adddb0..cb1fcbdb4 100644 |
| --- a/internal/instancectl/server.go |
| +++ b/internal/instancectl/server.go |
| @@ -610,6 +610,9 @@ func (s *Server) handleStop(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| } |
| + if req.role == RoleCmd { |
| + s.cleanCmdExe(r.Context(), req.host) |
| + } |
| w.WriteHeader(http.StatusNoContent) |
| } |
| @@ -700,6 +703,11 @@ func (s *Server) handleSweep(w http.ResponseWriter, r *http.Request) { |
| } |
| stopped = append(stopped, unit) |
| s.removeGenFiles(unit) |
| + if role, esc, _ := ParseUnitName(unit); role == RoleCmd { |
| + if iid, ok := Unescape(esc); ok { |
| + s.removeCmdExe(iid) |
| + } |
| + } |
| } |
| s.sweepGenLeftovers(ctx, failed) |
| if len(failed) > 0 { |
diff --git a/internal/instancectl/stagedexe.go b/internal/instancectl/stagedexe.go| index 39537c3e8..9c4ff06d3 100644 |
| --- a/internal/instancectl/stagedexe.go |
| +++ b/internal/instancectl/stagedexe.go |
| @@ -111,10 +111,38 @@ func (s *Server) ensureGenExeAt(host string, n uint64, x StagedExe) error { |
| // ensureHostExeAt lays out the host's own executable mount (the one the |
| // original templates bind: <ExeRoot>/<id>/app). It points at a validated |
| // staged build when x names one, which only a legacy sidecar's start does, |
| -// and at the live link otherwise (ensureHostExe). |
| +// and at the live link otherwise (ensureHostExe). A run's command has a |
| +// mount of its own (ensureCmdExeAt). |
| func (s *Server) ensureHostExeAt(host string, x StagedExe) error { |
| if x.Path == "" { |
| return s.ensureHostExe(host) |
| } |
| return ensureExe(s.ExeRoot, provision.InstanceID(host), x.Path, host) |
| } |
| + |
| +// ensureCmdExeAt lays out a run's executable mount, apart from the host's |
| +// (merge ruling M4): <ExeRoot>/<id>/cmd-app and cmd-live/<host>, which only |
| +// the cmd template binds. A command and a sidecar starting on different |
| +// builds around a swap therefore never overwrite the source the other is |
| +// about to bind. It points at a validated staged build when x names one, |
| +// and at the live link otherwise. |
| +func (s *Server) ensureCmdExeAt(host string, x StagedExe) error { |
| + target := x.Path |
| + if target == "" { |
| + target = filepath.Join(LiveDir, host) |
| + } |
| + return ensureExeAs(s.ExeRoot, provision.InstanceID(host), target, host, CmdExeApp, CmdExeLive) |
| +} |
| + |
| +// removeCmdExe removes a run's executable mount sources, once its unit is |
| +// stopped: they name a build the instance may no longer serve. Best-effort, |
| +// as removeGenFiles is: a leftover is two links in /run, rewritten by the |
| +// next run's start. |
| +func (s *Server) removeCmdExe(iid string) { |
| + if s.ExeRoot == "" || iid == "" || strings.ContainsAny(iid, "/\x00") || iid == "." || iid == ".." { |
| + return |
| + } |
| + dir := filepath.Join(s.ExeRoot, iid) |
| + _ = os.Remove(filepath.Join(dir, CmdExeApp)) |
| + _ = os.RemoveAll(filepath.Join(dir, CmdExeLive)) |
| +} |