From 93ba6915b9bec21b556fcbc88644dec91c732232 Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Mon, 12 Oct 2026 19:13:25 +0200 Subject: [PATCH] fix(agent): minimal env and explicit project dir for compose, resolve every config file's symlinks (#7) --- .dockerignore | 1 - cmd/agent/main.go | 7 +++-- internal/agent/updater/compose.go | 24 ++++++++++++++++- internal/agent/updater/compose_test.go | 35 ++++++++++++++++++++++++- internal/agent/updater/override.go | 33 ++++++++++++----------- internal/agent/updater/override_test.go | 5 ++++ internal/agent/updater/registryauth.go | 3 +-- internal/agent/updater/resolve.go | 3 +-- 8 files changed, 86 insertions(+), 25 deletions(-) diff --git a/.dockerignore b/.dockerignore index fb92392..c63ba6b 100644 --- a/.dockerignore +++ b/.dockerignore @@ -7,4 +7,3 @@ README.md .env .env.* gitea-action/ -docker-compose.example.yml diff --git a/cmd/agent/main.go b/cmd/agent/main.go index 03138ec..8d4ee12 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -75,8 +75,11 @@ func run() error { func logMounts(ctx context.Context, cli updater.SelfInspector, selfID string, log *slog.Logger) { roots, err := updater.MountRoots(ctx, cli, selfID) if err != nil || len(roots) == 0 { - log.Warn("no_stack_mounts", "error", err, - "hint", "Compose updates will fail; mount each stacks directory at the same path, read-write") + attrs := []any{"hint", "Compose updates will fail; mount each stacks directory at the same path, read-write"} + if err != nil { + attrs = append(attrs, "error", err) + } + log.Warn("no_stack_mounts", attrs...) return } log.Info("stack_mounts", "roots", roots) diff --git a/internal/agent/updater/compose.go b/internal/agent/updater/compose.go index b3251b5..a6a8ef9 100644 --- a/internal/agent/updater/compose.go +++ b/internal/agent/updater/compose.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "os" "os/exec" "strings" "time" @@ -43,10 +44,29 @@ func ExecDocker(ctx context.Context, dir string, out io.Writer, args ...string) cmd.Dir = dir cmd.Stdout = out cmd.Stderr = out + cmd.Env = dockerEnv() cmd.WaitDelay = 10 * time.Second return cmd.Run() } +// dockerEnvVars are the only variables from the agent's environment that +// reach docker compose. Compose prefers process variables over the stack's +// .env file when it interpolates, so passing the agent's own (HOSTNAME, +// AGENT_TOKEN, ...) would change stack configuration and expose the token. +var dockerEnvVars = []string{"PATH", "HOME", "DOCKER_HOST", "DOCKER_CONFIG", "DOCKER_TLS_VERIFY", "DOCKER_CERT_PATH"} + +// dockerEnv returns the allowed variables that are set in the agent's +// environment, as KEY=value pairs. +func dockerEnv() []string { + var env []string + for _, k := range dockerEnvVars { + if v := os.Getenv(k); v != "" { + env = append(env, k+"="+v) + } + } + return env +} + // ComposeDocker is the subset of the Docker SDK ComposeExecutor needs. type ComposeDocker interface { SelfInspector @@ -115,7 +135,9 @@ func (e *ComposeExecutor) Execute(ctx context.Context, job protocol.JobStart, ou return "", err } - base := []string{"compose", "-p", project} + // --project-directory is explicit because, with -f, compose would take it + // from the first file, which differs for stacks started with the flag. + base := []string{"compose", "-p", project, "--project-directory", workingDir} for _, f := range files { base = append(base, "-f", f) } diff --git a/internal/agent/updater/compose_test.go b/internal/agent/updater/compose_test.go index fae49ad..96d3c6a 100644 --- a/internal/agent/updater/compose_test.go +++ b/internal/agent/updater/compose_test.go @@ -66,7 +66,7 @@ func TestComposeExecutor(t *testing.T) { ro := fakeSelf{mounts: []container.MountPoint{{Type: mount.TypeBind, Source: root, Destination: root, RW: false}}} wantFiles := []string{"-f", filepath.Join(stack, "compose.yaml"), "-f", filepath.Join(stack, OverrideName)} cmd := func(rest ...string) []string { - return append(append([]string{"compose", "-p", "slusk"}, wantFiles...), rest...) + return append(append([]string{"compose", "-p", "slusk", "--project-directory", stack}, wantFiles...), rest...) } overridePath := filepath.Join(stack, OverrideName) cleanOverride := func() { os.Remove(overridePath) } @@ -122,6 +122,18 @@ func TestComposeExecutor(t *testing.T) { assert.ErrorContains(t, err, "refused") assert.Empty(t, rec.cmds) }) + t.Run("working dir outside the roots", func(t *testing.T) { + outside := filepath.Join(t.TempDir(), "elsewhere") + bad := container.Summary{Labels: map[string]string{ + labelWorkingDir: outside, + labelConfigFiles: filepath.Join(stack, "compose.yaml"), + }} + rec := &recorder{} + _, err := setup(fakeCompose{rw, []container.Summary{bad}}, &fakeDist{digest: d}, rec).Execute(context.Background(), job, io.Discard) + assert.ErrorContains(t, err, "not mounted") + assert.Empty(t, rec.cmds) + assert.NoFileExists(t, overridePath) + }) t.Run("read-only mount", func(t *testing.T) { rec := &recorder{} _, err := setup(fakeCompose{ro, []container.Summary{ct}}, &fakeDist{digest: d}, rec).Execute(context.Background(), job, io.Discard) @@ -130,3 +142,24 @@ func TestComposeExecutor(t *testing.T) { assert.NoFileExists(t, overridePath) }) } + +func TestDockerEnv(t *testing.T) { + t.Setenv("AGENT_TOKEN", "secret") + t.Setenv("HOSTNAME", "abc") + t.Setenv("MASTER_URL", "wss://x") + t.Setenv("PATH", "/usr/bin") + t.Setenv("DOCKER_CONFIG", "/docker-config") + t.Setenv("DOCKER_HOST", "") + os.Unsetenv("DOCKER_CERT_PATH") + + env := dockerEnv() + assert.Contains(t, env, "PATH=/usr/bin") + assert.Contains(t, env, "DOCKER_CONFIG=/docker-config") + assert.NotContains(t, env, "DOCKER_HOST=", "empty variables are not passed") + for _, e := range env { + assert.NotContains(t, e, "AGENT_TOKEN") + assert.NotContains(t, e, "HOSTNAME") + assert.NotContains(t, e, "MASTER_URL") + assert.False(t, strings.HasPrefix(e, "DOCKER_CERT_PATH="), "unset variables are not passed") + } +} diff --git a/internal/agent/updater/override.go b/internal/agent/updater/override.go index 7d67ccd..01d9490 100644 --- a/internal/agent/updater/override.go +++ b/internal/agent/updater/override.go @@ -133,7 +133,7 @@ func MountRoots(ctx context.Context, cli SelfInspector, selfID string) ([]string // composeFiles returns the -f arguments for a job: the stack's config files // from the container label with any copy of the override removed, then the -// override last so it wins. Every file, and the real location of the stack +// override last so it wins. Every file, and the real location of its // directory after resolving symlinks, must be inside one of roots. func composeFiles(roots []string, configFilesLabel string) (files []string, dir string, err error) { if len(roots) == 0 { @@ -149,19 +149,7 @@ func composeFiles(roots []string, configFilesLabel string) (files []string, dir } dir = filepath.Dir(files[0]) override := filepath.Join(dir, OverrideName) - out := files[:0] - for _, f := range files { - if !filepath.IsAbs(f) || !insideAny(roots, f) { - return nil, "", notMounted(filepath.Dir(f)) - } - if filepath.Clean(f) != override { - out = append(out, f) - } - } - real, err := filepath.EvalSymlinks(dir) - if err != nil { - return nil, "", fmt.Errorf("resolve %s: %w", dir, err) - } + // Only directories count once resolved: this drops a same-path mount of // a single file such as /var/run/docker.sock. var realRoots []string @@ -172,8 +160,21 @@ func composeFiles(roots []string, configFilesLabel string) (files []string, dir } } } - if !insideAny(realRoots, real) { - return nil, "", notMounted(dir) + out := files[:0] + for _, f := range files { + if !filepath.IsAbs(f) || !insideAny(roots, f) { + return nil, "", notMounted(filepath.Dir(f)) + } + real, err := filepath.EvalSymlinks(filepath.Dir(f)) + if err != nil { + return nil, "", fmt.Errorf("resolve %s: %w", filepath.Dir(f), err) + } + if !insideAny(realRoots, real) { + return nil, "", notMounted(filepath.Dir(f)) + } + if filepath.Clean(f) != override { + out = append(out, f) + } } return append(out, override), dir, nil } diff --git a/internal/agent/updater/override_test.go b/internal/agent/updater/override_test.go index 31a2724..36e0b7c 100644 --- a/internal/agent/updater/override_test.go +++ b/internal/agent/updater/override_test.go @@ -185,6 +185,11 @@ func TestComposeFiles(t *testing.T) { _, _, err := composeFiles([]string{root}, filepath.Join(root, "evil", "compose.yaml")) assert.EqualError(t, err, "stack directory "+filepath.Join(root, "evil")+" is not mounted into the agent at the same path (read-write)") }) + t.Run("second file under a symlink escaping the root", func(t *testing.T) { + label := filepath.Join(a, "compose.yaml") + "," + filepath.Join(root, "evil", "extra.yaml") + _, _, err := composeFiles([]string{root}, label) + assert.ErrorContains(t, err, "not mounted") + }) t.Run("prefix trick", func(t *testing.T) { evil := root + "-evil" require.NoError(t, os.MkdirAll(evil, 0o755)) diff --git a/internal/agent/updater/registryauth.go b/internal/agent/updater/registryauth.go index a19e8b4..3c100dc 100644 --- a/internal/agent/updater/registryauth.go +++ b/internal/agent/updater/registryauth.go @@ -3,7 +3,6 @@ package updater import ( "encoding/base64" "encoding/json" - "errors" "fmt" "log/slog" "os" @@ -64,7 +63,7 @@ func LoadRegistryAuth(dockerConfigDir string, log *slog.Logger) (*RegistryAuth, } user, pass, ok := strings.Cut(string(dec), ":") if !ok { - return nil, errors.New(path + ": auth for " + key + " is not user:password") + return nil, fmt.Errorf("%s: auth for %s is not user:password", path, key) } ac.Username, ac.Password = user, pass default: diff --git a/internal/agent/updater/resolve.go b/internal/agent/updater/resolve.go index 9923cbf..a3ddab2 100644 --- a/internal/agent/updater/resolve.go +++ b/internal/agent/updater/resolve.go @@ -2,7 +2,6 @@ package updater import ( "context" - "errors" "fmt" "github.com/docker/docker/api/types/registry" @@ -43,7 +42,7 @@ func (r *Resolver) Resolve(ctx context.Context, ref string) (string, error) { return "", fmt.Errorf("resolve %s: %w", ref, err) } if info.Descriptor.Digest == "" { - return "", errors.New("resolve " + ref + ": registry returned no digest") + return "", fmt.Errorf("resolve %s: registry returned no digest", ref) } return ref + "@" + info.Descriptor.Digest.String(), nil }