From cf9c0c4b1048869356cc8259ede2950a4253e5b6 Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Tue, 13 Oct 2026 09:25:18 +0200 Subject: [PATCH] fix(agent): set the override owner on the temp file before the rename (#24) --- CLAUDE.md | 9 ++- internal/agent/updater/compose.go | 38 ++------- internal/agent/updater/compose_test.go | 76 ++---------------- internal/agent/updater/override.go | 88 +++++++++++++++++--- internal/agent/updater/override_test.go | 102 ++++++++++++++++++++++-- 5 files changed, 185 insertions(+), 128 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0e770cb..611bc02 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,9 +98,6 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ without the other has caused a fix commit already. - **`` C# convention from global CLAUDE.md does not apply here** — this is Go. Use idiomatic GoDoc (`// FuncName does X.`). -- **The override file gets the owner of the existing override, else of the stack's first - compose file.** The agent runs as root and renames a new file into place; it chowns it - afterwards. A failure is a `warning:` line in the job output, never a failed job. - **Compose loads only one override file.** Of `compose.override.yml`, `compose.override.yaml`, `docker-compose.override.yml`, `docker-compose.override.yaml` only the first that exists is read. Hence the refusal described above. @@ -108,6 +105,12 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ agent's own jobs always pass it explicitly. - **Override entries are never pruned.** A service deleted from the stack file keeps its `image:` line in `compose.override.yaml`. +- **The override file gets the owner of the existing override, else of the stack's first + compose file.** The exception is a root-owned existing override (written before this + fix): then the compose file's owner is used, so it stops being root-owned. The agent runs + as root, so `WriteOverride` chowns the open temp file before the rename (never the path + afterwards: a symlink could be swapped in). A failure is a `warning:` line in the job + output, never a failed job. - **There is no `STACKS_ROOT`.** Stack directories must be mounted read-write at the host path (`/path:/path`), and `SELF_ID` must resolve to the agent's own container. On Docker Desktop/OrbStack bind sources are translated, so Compose jobs fail there. diff --git a/internal/agent/updater/compose.go b/internal/agent/updater/compose.go index 30c337b..0e6ccb9 100644 --- a/internal/agent/updater/compose.go +++ b/internal/agent/updater/compose.go @@ -2,15 +2,11 @@ package updater import ( "context" - "errors" "fmt" "io" - "io/fs" "os" "os/exec" - "path/filepath" "strings" - "syscall" "time" "github.com/docker/docker/api/types/container" @@ -31,26 +27,6 @@ const ( // job's error message; the full output is in the job log. const errorOutputTail = 2 << 10 -// chown is os.Chown; tests replace it. -var chown = os.Chown - -// stackOwner returns the uid and gid the override file should have: those of -// the existing override if there is one, else those of the stack's compose file. -func stackOwner(overridePath, composeFile string) (uid, gid int, err error) { - fi, err := os.Stat(overridePath) - if errors.Is(err, fs.ErrNotExist) { - fi, err = os.Stat(composeFile) - } - if err != nil { - return 0, 0, err - } - st, ok := fi.Sys().(*syscall.Stat_t) - if !ok { - return 0, 0, errors.New("file owner is not available on this platform") - } - return int(st.Uid), int(st.Gid), nil -} - // tailWriter adapts a tailBuffer to io.Writer. type tailWriter struct{ b *tailBuffer } @@ -176,18 +152,14 @@ func (e *ComposeExecutor) Execute(ctx context.Context, job protocol.JobStart, ou } fmt.Fprintln(out, resolveNote(job.Ref, pinned)) - // The agent runs as root and WriteOverride renames a new file into place, - // so the override would end up root-owned. Hand it back to the stack's - // owner. A failure here is only a warning: the update itself is fine. - overridePath := filepath.Join(dir, OverrideName) - uid, gid, ownerErr := stackOwner(overridePath, files[0]) - if _, err := WriteOverride(dir, service, pinned); err != nil { + // Giving the override the stack's owner is best effort: a failure is only + // a warning, the update itself is fine. + _, ownerErr, err := WriteOverride(dir, service, pinned, files[0]) + if err != nil { return "", err } if ownerErr != nil { - fmt.Fprintf(out, "warning: could not read the owner of %s or %s to give it to %s: %v\n", overridePath, files[0], overridePath, ownerErr) - } else if err := chown(overridePath, uid, gid); err != nil { - fmt.Fprintf(out, "warning: could not give %s the owner %d:%d: %v\n", overridePath, uid, gid, err) + fmt.Fprintf(out, "warning: could not give the override the stack's owner: %v\n", ownerErr) } base := composeBase(project, workingDir, files, envs) diff --git a/internal/agent/updater/compose_test.go b/internal/agent/updater/compose_test.go index 0523d30..2fbdb85 100644 --- a/internal/agent/updater/compose_test.go +++ b/internal/agent/updater/compose_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "strings" - "syscall" "testing" "github.com/docker/docker/api/types/container" @@ -65,53 +64,12 @@ func (r *recorder) run(_ context.Context, dir string, out io.Writer, args ...str return nil } -// stubChown replaces chown for the test. Tests that use it must not run in parallel. -func stubChown(t *testing.T, f func(path string, uid, gid int) error) { - t.Helper() - orig := chown - chown = f - t.Cleanup(func() { chown = orig }) -} - -func TestStackOwner(t *testing.T) { - dir := t.TempDir() - override := filepath.Join(dir, OverrideName) - compose := filepath.Join(dir, "compose.yaml") - - t.Run("falls back to the compose file when there is no override", func(t *testing.T) { - require.NoError(t, os.WriteFile(compose, nil, 0o644)) - uid, gid, err := stackOwner(override, compose) - require.NoError(t, err) - assert.Equal(t, os.Getuid(), uid) - assert.Equal(t, os.Getgid(), gid) - }) - t.Run("prefers the existing override", func(t *testing.T) { - require.NoError(t, os.WriteFile(override, nil, 0o644)) - require.NoError(t, os.Remove(compose)) - uid, _, err := stackOwner(override, compose) - require.NoError(t, err, "the compose file is not consulted") - assert.Equal(t, os.Getuid(), uid) - }) - t.Run("fails when neither file exists", func(t *testing.T) { - require.NoError(t, os.Remove(override)) - _, _, err := stackOwner(override, compose) - assert.Error(t, err) - }) -} - -func TestChown_RealToOwnIDs(t *testing.T) { - f := filepath.Join(t.TempDir(), "f") - require.NoError(t, os.WriteFile(f, nil, 0o644)) - uid, gid, err := stackOwner(f, f) - require.NoError(t, err) - assert.NoError(t, chown(f, uid, gid)) -} - func TestComposeExecutor(t *testing.T) { const d = "sha256:d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1d1" root, _ := filepath.EvalSymlinks(t.TempDir()) stack := filepath.Join(root, "slusk") require.NoError(t, os.MkdirAll(stack, 0o755)) + composeFile := stackFile(t, stack) job := protocol.JobStart{Kind: protocol.KindCompose, ComposeProject: "slusk", ComposeService: "web", Ref: "gitea.shcizo.se/shcizo/slusk:1.4"} pinned := job.Ref + "@" + d @@ -150,41 +108,17 @@ func TestComposeExecutor(t *testing.T) { _, doc := readOverride(t, stack) assert.Equal(t, pinned, serviceImage(doc, "web")) assert.Contains(t, out.String(), "resolved "+pinned) - }) - t.Run("the override is chowned to the owner of the compose file", func(t *testing.T) { - t.Cleanup(cleanOverride) - composeFile := filepath.Join(stack, "compose.yaml") - require.NoError(t, os.WriteFile(composeFile, []byte("services: {}\n"), 0o644)) - t.Cleanup(func() { os.Remove(composeFile) }) - fi, err := os.Stat(composeFile) - require.NoError(t, err) - st := fi.Sys().(*syscall.Stat_t) - var gotPath string - var gotUID, gotGID int - stubChown(t, func(path string, uid, gid int) error { - gotPath, gotUID, gotGID = path, uid, gid - return nil - }) - var out strings.Builder - _, err = setup(fakeCompose{rw, []container.Summary{ct}}, &fakeDist{digest: d}, &recorder{}).Execute(context.Background(), job, &out) - require.NoError(t, err) - assert.Equal(t, overridePath, gotPath) - assert.Equal(t, int(st.Uid), gotUID) - assert.Equal(t, int(st.Gid), gotGID) assert.NotContains(t, out.String(), "warning") }) - t.Run("a chown failure is a warning and the job still succeeds", func(t *testing.T) { + t.Run("an unreadable owner is a warning and the job still succeeds", func(t *testing.T) { t.Cleanup(cleanOverride) - composeFile := filepath.Join(stack, "compose.yaml") - require.NoError(t, os.WriteFile(composeFile, []byte("services: {}\n"), 0o644)) - t.Cleanup(func() { os.Remove(composeFile) }) - stubChown(t, func(string, int, int) error { return errors.New("operation not permitted") }) + require.NoError(t, os.Rename(composeFile, composeFile+".bak")) + t.Cleanup(func() { os.Rename(composeFile+".bak", composeFile) }) var out strings.Builder got, err := setup(fakeCompose{rw, []container.Summary{ct}}, &fakeDist{digest: d}, &recorder{}).Execute(context.Background(), job, &out) require.NoError(t, err) assert.Equal(t, pinned, got) - assert.Contains(t, out.String(), "warning: could not give "+overridePath+" the owner ") - assert.Contains(t, out.String(), "operation not permitted") + assert.Contains(t, out.String(), "warning: could not give the override the stack's owner: read owner: ") _, doc := readOverride(t, stack) assert.Equal(t, pinned, serviceImage(doc, "web")) }) diff --git a/internal/agent/updater/override.go b/internal/agent/updater/override.go index e1ad899..7882d2f 100644 --- a/internal/agent/updater/override.go +++ b/internal/agent/updater/override.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/mount" @@ -34,15 +35,68 @@ var overrideNames = []string{ // keeping every other entry in the file, and returns the file's path. It // refuses when the file exists without the header, or when another // auto-loaded override file exists in dir. The write is atomic. -func WriteOverride(dir, service, ref string) (string, error) { +// +// composeFile is the stack's first compose file. The new file gets the owner +// of the existing override, or of composeFile when there is none or the +// existing one is owned by root (see pickOwner). The agent runs as root and the +// rename creates a new file, so without this the stack's owner would lose +// control of the file. ownerErr reports that the owner could not be read or +// set; the override is still written. err means it was not written. +func WriteOverride(dir, service, ref, composeFile string) (path string, ownerErr error, err error) { + overridePath := filepath.Join(dir, OverrideName) + return writeOverride(dir, service, ref, func() (fileOwner, error) { + return resolveOwner(overridePath, composeFile) + }) +} + +// fileOwner is the uid and gid of a file. +type fileOwner struct{ uid, gid int } + +// readOwner returns the owner of the file at path. +func readOwner(path string) (fileOwner, error) { + fi, err := os.Stat(path) + if err != nil { + return fileOwner{}, fmt.Errorf("read owner: %w", err) + } + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return fileOwner{}, fmt.Errorf("read owner of %s: not available on this platform", path) + } + return fileOwner{int(st.Uid), int(st.Gid)}, nil +} + +// pickOwner chooses the owner of a new override: the existing override's +// (nil when there is none), unless that is root, then the compose file's. +// Overrides written before the agent set owners are root-owned and would +// otherwise stay so. compose is only called when needed. +func pickOwner(override *fileOwner, compose func() (fileOwner, error)) (fileOwner, error) { + if override != nil && override.uid != 0 { + return *override, nil + } + return compose() +} + +// resolveOwner applies pickOwner to the files on disk. +func resolveOwner(overridePath, composeFile string) (fileOwner, error) { + var existing *fileOwner + if o, err := readOwner(overridePath); err == nil { + existing = &o + } else if !errors.Is(err, os.ErrNotExist) { + return fileOwner{}, err + } + return pickOwner(existing, func() (fileOwner, error) { return readOwner(composeFile) }) +} + +// writeOverride is WriteOverride with the owner lookup injected, for tests. +func writeOverride(dir, service, ref string, owner func() (fileOwner, error)) (string, error, error) { for _, name := range overrideNames { if name == OverrideName { continue } if _, err := os.Lstat(filepath.Join(dir, name)); err == nil { - return "", fmt.Errorf("refused: %s exists; the agent only manages %s and never edits operator files", name, OverrideName) + return "", nil, fmt.Errorf("refused: %s exists; the agent only manages %s and never edits operator files", name, OverrideName) } else if !errors.Is(err, os.ErrNotExist) { - return "", fmt.Errorf("check %s: %w", name, err) + return "", nil, fmt.Errorf("check %s: %w", name, err) } } path := filepath.Join(dir, OverrideName) @@ -51,12 +105,12 @@ func WriteOverride(dir, service, ref string) (string, error) { switch { case errors.Is(err, os.ErrNotExist): case err != nil: - return "", fmt.Errorf("read override: %w", err) + return "", nil, fmt.Errorf("read override: %w", err) case !bytes.HasPrefix(raw, []byte(overrideHeader)): - return "", fmt.Errorf("refused: %s exists and was not written by package-updater", path) + return "", nil, fmt.Errorf("refused: %s exists and was not written by package-updater", path) default: if err := yaml.Unmarshal(raw, &doc); err != nil { - return "", fmt.Errorf("parse override: %w", err) + return "", nil, fmt.Errorf("parse override: %w", err) } if doc == nil { doc = map[string]any{} @@ -75,30 +129,38 @@ func WriteOverride(dir, service, ref string) (string, error) { doc["services"] = services body, err := yaml.Marshal(doc) if err != nil { - return "", fmt.Errorf("encode override: %w", err) + return "", nil, fmt.Errorf("encode override: %w", err) } // Temp file in the same directory + rename: a crash never leaves a // half-written override that compose would then load. tmp, err := os.CreateTemp(dir, "."+OverrideName+".*") if err != nil { - return "", fmt.Errorf("write override: %w", err) + return "", nil, fmt.Errorf("write override: %w", err) } defer os.Remove(tmp.Name()) // no-op after a successful rename if _, err := tmp.WriteString(overrideHeader + string(body)); err != nil { tmp.Close() - return "", fmt.Errorf("write override: %w", err) + return "", nil, fmt.Errorf("write override: %w", err) } if err := tmp.Chmod(0o644); err != nil { tmp.Close() - return "", fmt.Errorf("write override: %w", err) + return "", nil, fmt.Errorf("write override: %w", err) + } + // Chown the open file, not the path after the rename: a path could have + // been swapped for a symlink by someone with write access to the stack. + var ownerErr error + if o, err := owner(); err != nil { + ownerErr = err + } else if err := tmp.Chown(o.uid, o.gid); err != nil { + ownerErr = fmt.Errorf("set owner %d:%d: %w", o.uid, o.gid, err) } if err := tmp.Close(); err != nil { - return "", fmt.Errorf("write override: %w", err) + return "", nil, fmt.Errorf("write override: %w", err) } if err := os.Rename(tmp.Name(), path); err != nil { - return "", fmt.Errorf("write override: %w", err) + return "", nil, fmt.Errorf("write override: %w", err) } - return path, nil + return path, ownerErr, nil } // SelfInspector is the subset of the Docker SDK MountRoots needs. diff --git a/internal/agent/updater/override_test.go b/internal/agent/updater/override_test.go index 36e0b7c..3c20612 100644 --- a/internal/agent/updater/override_test.go +++ b/internal/agent/updater/override_test.go @@ -29,11 +29,96 @@ func serviceImage(doc map[string]any, name string) any { return entry["image"] } +// stackFile creates dir/compose.yaml and returns its path. +func stackFile(t *testing.T, dir string) string { + t.Helper() + p := filepath.Join(dir, "compose.yaml") + require.NoError(t, os.WriteFile(p, []byte("services: {}\n"), 0o644)) + return p +} + +func TestPickOwner(t *testing.T) { + compose := fileOwner{1000, 1000} + fromCompose := func() (fileOwner, error) { return compose, nil } + failing := func() (fileOwner, error) { return fileOwner{}, errors.New("boom") } + tests := []struct { + name string + override *fileOwner + compose func() (fileOwner, error) + want fileOwner + wantErr bool + }{ + {"no override uses the compose file", nil, fromCompose, compose, false}, + {"override owned by a user wins", &fileOwner{1001, 1002}, fromCompose, fileOwner{1001, 1002}, false}, + {"root-owned override uses the compose file", &fileOwner{0, 0}, fromCompose, compose, false}, + {"compose file is not read when the override decides", &fileOwner{1001, 1002}, failing, fileOwner{1001, 1002}, false}, + {"unreadable compose file is an error", nil, failing, fileOwner{}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := pickOwner(tt.override, tt.compose) + assert.Equal(t, tt.wantErr, err != nil) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestResolveOwner(t *testing.T) { + me := fileOwner{os.Getuid(), os.Getgid()} + t.Run("no override uses the compose file", func(t *testing.T) { + dir := t.TempDir() + got, err := resolveOwner(filepath.Join(dir, OverrideName), stackFile(t, dir)) + require.NoError(t, err) + assert.Equal(t, me, got) + }) + t.Run("existing override is used without the compose file", func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, OverrideName), nil, 0o644)) + got, err := resolveOwner(filepath.Join(dir, OverrideName), filepath.Join(dir, "missing.yaml")) + require.NoError(t, err) + assert.Equal(t, me, got) + }) + t.Run("neither file is an error", func(t *testing.T) { + dir := t.TempDir() + _, err := resolveOwner(filepath.Join(dir, OverrideName), filepath.Join(dir, "missing.yaml")) + assert.Error(t, err) + }) +} + +func TestWriteOverride_Owner(t *testing.T) { + t.Run("success sets no ownerErr", func(t *testing.T) { + dir := t.TempDir() + _, ownerErr, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) + require.NoError(t, err) + assert.NoError(t, ownerErr) + }) + t.Run("unreadable owner is reported and the file is still written", func(t *testing.T) { + dir := t.TempDir() + _, ownerErr, err := WriteOverride(dir, "web", "a:1@sha256:aa", filepath.Join(dir, "missing.yaml")) + require.NoError(t, err) + assert.ErrorContains(t, ownerErr, "read owner") + _, doc := readOverride(t, dir) + assert.Equal(t, "a:1@sha256:aa", serviceImage(doc, "web")) + }) + t.Run("chown failure is reported and the file is still written", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root can chown to anyone") + } + dir := t.TempDir() + _, ownerErr, err := writeOverride(dir, "web", "a:1@sha256:aa", func() (fileOwner, error) { return fileOwner{0, 0}, nil }) + require.NoError(t, err) + assert.ErrorContains(t, ownerErr, "set owner 0:0") + _, doc := readOverride(t, dir) + assert.Equal(t, "a:1@sha256:aa", serviceImage(doc, "web")) + }) +} + func TestWriteOverride(t *testing.T) { t.Run("new file", func(t *testing.T) { dir := t.TempDir() - path, err := WriteOverride(dir, "web", "a:1@sha256:aa") + path, ownerErr, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) require.NoError(t, err) + assert.NoError(t, ownerErr) assert.Equal(t, filepath.Join(dir, OverrideName), path) raw, doc := readOverride(t, dir) assert.True(t, len(raw) > len(overrideHeader) && raw[:len(overrideHeader)] == overrideHeader) @@ -42,7 +127,7 @@ func TestWriteOverride(t *testing.T) { t.Run("keeps other services", func(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, OverrideName), []byte(overrideHeader+"services:\n db:\n image: db:1\n"), 0o644)) - _, err := WriteOverride(dir, "web", "a:1@sha256:aa") + _, _, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) require.NoError(t, err) _, doc := readOverride(t, dir) assert.Equal(t, "db:1", serviceImage(doc, "db")) @@ -51,7 +136,7 @@ func TestWriteOverride(t *testing.T) { t.Run("replaces own entry", func(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, OverrideName), []byte(overrideHeader+"services:\n web:\n image: old\n"), 0o644)) - _, err := WriteOverride(dir, "web", "a:2@sha256:bb") + _, _, err := WriteOverride(dir, "web", "a:2@sha256:bb", stackFile(t, dir)) require.NoError(t, err) _, doc := readOverride(t, dir) assert.Equal(t, "a:2@sha256:bb", serviceImage(doc, "web")) @@ -60,7 +145,7 @@ func TestWriteOverride(t *testing.T) { dir := t.TempDir() const body = "services:\n web:\n environment: [A=1]\n" require.NoError(t, os.WriteFile(filepath.Join(dir, OverrideName), []byte(body), 0o644)) - _, err := WriteOverride(dir, "web", "a:1@sha256:aa") + _, _, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) assert.ErrorContains(t, err, "refused") got, _ := os.ReadFile(filepath.Join(dir, OverrideName)) assert.Equal(t, body, string(got)) @@ -69,7 +154,7 @@ func TestWriteOverride(t *testing.T) { t.Run("refuses next to "+other, func(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, other), []byte("services: {}\n"), 0o644)) - _, err := WriteOverride(dir, "web", "a:1@sha256:aa") + _, _, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) assert.ErrorContains(t, err, other) _, statErr := os.Stat(filepath.Join(dir, OverrideName)) assert.True(t, errors.Is(statErr, os.ErrNotExist)) @@ -77,12 +162,13 @@ func TestWriteOverride(t *testing.T) { } t.Run("leaves no temp files", func(t *testing.T) { dir := t.TempDir() - _, err := WriteOverride(dir, "web", "a:1@sha256:aa") + _, _, err := WriteOverride(dir, "web", "a:1@sha256:aa", stackFile(t, dir)) require.NoError(t, err) entries, err := os.ReadDir(dir) require.NoError(t, err) - require.Len(t, entries, 1) - assert.Equal(t, OverrideName, entries[0].Name()) + require.Len(t, entries, 2) + assert.Equal(t, "compose.override.yaml", entries[0].Name()) + assert.Equal(t, "compose.yaml", entries[1].Name()) }) }