fix(agent): give compose.override.yaml the owner of the stack (#24) (#26)

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.