From edad0b158ea10a6647bb1c84629d93f5c3d8770e Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Mon, 18 Sep 2023 18:29:06 +1000 Subject: [PATCH] Replace Bash fix-permissions script with Go * Easier to test * Can test more things * Prevents symlink shenanigans --- .buildkite/docker-compose.yml | 17 ++ .buildkite/pipeline.yaml | 36 +++- .buildkite/steps/build-fixperms.sh | 5 + .buildkite/steps/packer.sh | 6 + docker-compose.unit-tests.yml | 9 - go.mod | 8 + go.sum | 4 + internal/fixperms/fdfs/fdfs.go | 64 +++++++ internal/fixperms/fdfs/fdfs_test.go | 61 +++++++ internal/fixperms/fixer/fixer.go | 85 +++++++++ internal/fixperms/fixer/fixer_test.go | 138 +++++++++++++++ internal/fixperms/fixperms.go | 63 +++++++ .../fixperms/fixtures/a/b/c/d/e | 0 internal/fixperms/fixtures/a/b/c/d/link | 1 + internal/fixperms/fixtures/a/b/c/link | 1 + .../fixperms}/fixtures/a/b/link | 0 .../fixperms}/fixtures/a/link | 0 .../fixperms}/fixtures/d/e/f | 0 .../fixperms}/fixtures/g/.gitkeep | 0 .../fixperms}/fixtures/link | 0 .../fix-buildkite-agent-builds-permissions | 100 ----------- ...ix-buildkite-agent-builds-permissions.bats | 162 ------------------ 22 files changed, 480 insertions(+), 280 deletions(-) create mode 100644 .buildkite/docker-compose.yml create mode 100755 .buildkite/steps/build-fixperms.sh delete mode 100644 docker-compose.unit-tests.yml create mode 100644 go.mod create mode 100644 go.sum create mode 100644 internal/fixperms/fdfs/fdfs.go create mode 100644 internal/fixperms/fdfs/fdfs_test.go create mode 100644 internal/fixperms/fixer/fixer.go create mode 100644 internal/fixperms/fixer/fixer_test.go create mode 100644 internal/fixperms/fixperms.go rename unit-tests/fixtures/a/b/c/.gitkeep => internal/fixperms/fixtures/a/b/c/d/e (100%) create mode 120000 internal/fixperms/fixtures/a/b/c/d/link create mode 120000 internal/fixperms/fixtures/a/b/c/link rename {unit-tests => internal/fixperms}/fixtures/a/b/link (100%) rename {unit-tests => internal/fixperms}/fixtures/a/link (100%) rename {unit-tests => internal/fixperms}/fixtures/d/e/f (100%) rename {unit-tests => internal/fixperms}/fixtures/g/.gitkeep (100%) rename {unit-tests => internal/fixperms}/fixtures/link (100%) delete mode 100755 packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions delete mode 100644 unit-tests/fix-buildkite-agent-builds-permissions.bats diff --git a/.buildkite/docker-compose.yml b/.buildkite/docker-compose.yml new file mode 100644 index 000000000..05baf3479 --- /dev/null +++ b/.buildkite/docker-compose.yml @@ -0,0 +1,17 @@ +version: '3' + +services: + fixperms-tests: + image: golang:latest + working_dir: /code + volumes: + - ..:/code:ro + command: go test -v ./... + + fixperms-build: + image: golang:latest + working_dir: /code + volumes: + - ..:/code + - /var/lib/buildkite-agent/git-mirrors:/var/lib/buildkite-agent/git-mirrors + command: .buildkite/steps/build-fixperms.sh diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml index e0ee9a893..27475aae8 100644 --- a/.buildkite/pipeline.yaml +++ b/.buildkite/pipeline.yaml @@ -7,14 +7,28 @@ steps: agents: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" - - id: "bats-tests" - name: ":bash: Unit tests" + - id: "fixperms-tests" + name: ":go: fixperms tests" agents: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" plugins: - docker-compose#v2.1.0: - run: unit-tests - config: docker-compose.unit-tests.yml + - docker-compose#v2.1.0: + run: fixperms-tests + config: .buildkite/docker-compose.yml + + - id: "fixperms-build" + name: ":go: fixperms build" + agents: + queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" + depends_on: + - "fixperms-tests" + artifact_paths: "build/fix-perms-*" + plugins: + - docker-compose#v2.1.0: + run: fixperms-build + config: .buildkite/docker-compose.yml + - artifacts#v1.9.0: + upload: "builds/fix-perms-*" - id: "deploy-service-role-stack" name: ":aws-iam: :cloudformation:" @@ -23,7 +37,8 @@ steps: command: .buildkite/steps/deploy-service-role-stack.sh depends_on: - "lint" - - "bats-tests" + - "fixperms-tests" + - "fixperms-build" - id: "packer-windows-amd64" name: ":packer: :windows:" @@ -34,7 +49,8 @@ steps: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" depends_on: - "lint" - - "bats-tests" + - "fixperms-tests" + - "fixperms-build" - id: "launch-windows-amd64" name: ":cloudformation: :windows: AMD64 Launch" @@ -77,7 +93,8 @@ steps: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" depends_on: - "lint" - - "bats-tests" + - "fixperms-tests" + - "fixperms-build" - id: "launch-linux-amd64" name: ":cloudformation: :linux: AMD64 Launch" @@ -119,7 +136,8 @@ steps: queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}" depends_on: - "lint" - - "bats-tests" + - "fixperms-tests" + - "fixperms-build" - id: "launch-linux-arm64" name: ":cloudformation: :linux: ARM64 Launch" diff --git a/.buildkite/steps/build-fixperms.sh b/.buildkite/steps/build-fixperms.sh new file mode 100755 index 000000000..a3b9cac34 --- /dev/null +++ b/.buildkite/steps/build-fixperms.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +set -euo pipefail +for arch in amd64 arm64; do + GOOS=linux GOARCH="${arch}" go build -v -o "build/fix-perms-linux-${arch}" ./internal/fixperms +done diff --git a/.buildkite/steps/packer.sh b/.buildkite/steps/packer.sh index 47333ad75..b04ffbb93 100755 --- a/.buildkite/steps/packer.sh +++ b/.buildkite/steps/packer.sh @@ -16,6 +16,12 @@ fi mkdir -p "build/" +if [[ "$os" == "linux" ]] ; then + buildkite-agent artifact download "build/fix-perms-linux-${arch}" ./build + mv "build/fix-perms-linux-${arch}" packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions + chmod 755 packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions +fi + # Build a hash of packer files and the agent versions packer_files_sha=$(find Makefile "packer/${os}" plugins/ -type f -print0 | xargs -0 sha1sum | awk '{print $1}' | sort | sha1sum | awk '{print $1}') stable_agent_sha=$(curl -Lfs "https://download.buildkite.com/agent/stable/latest/${agent_binary}.sha256") diff --git a/docker-compose.unit-tests.yml b/docker-compose.unit-tests.yml deleted file mode 100644 index a68c7a539..000000000 --- a/docker-compose.unit-tests.yml +++ /dev/null @@ -1,9 +0,0 @@ -version: '3' - -services: - unit-tests: - image: bats/bats - volumes: - - .:/code:ro - - ./unit-tests/fixtures:/var/lib/buildkite-agent/builds:ro - command: /code/unit-tests \ No newline at end of file diff --git a/go.mod b/go.mod new file mode 100644 index 000000000..6d7d7bd23 --- /dev/null +++ b/go.mod @@ -0,0 +1,8 @@ +module github.com/buildkite/elastic-ci-stack-for-aws/v6 + +go 1.20 + +require ( + github.com/google/go-cmp v0.5.9 + golang.org/x/sys v0.12.0 +) diff --git a/go.sum b/go.sum new file mode 100644 index 000000000..eb4ae603c --- /dev/null +++ b/go.sum @@ -0,0 +1,4 @@ +github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= +github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o= +golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/internal/fixperms/fdfs/fdfs.go b/internal/fixperms/fdfs/fdfs.go new file mode 100644 index 000000000..8d1641fa1 --- /dev/null +++ b/internal/fixperms/fdfs/fdfs.go @@ -0,0 +1,64 @@ +//go:build linux + +// Package fdfs is like os.DirFS, but with a file descriptor and openat(2), +// fchownat(2), etc, to ensure symlinks do not escape. +package fdfs + +import ( + "io/fs" + "os" + + "golang.org/x/sys/unix" +) + +const resolveFlags = unix.RESOLVE_BENEATH | unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_NO_MAGICLINKS | unix.RESOLVE_NO_XDEV + +// FS uses a file descriptor for a directory as the base of a fs.FS. +type FS uintptr + +// DirFS opens the directory dir, and returns an FS rooted at that directory. +// It uses open(2) with O_PATH+O_DIRECTORY+O_CLOEXEC. +func DirFS(dir string) (FS, error) { + bd, err := os.OpenFile(dir, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return 0, err + } + return FS(bd.Fd()), nil +} + +// Close closes the file descriptor. +func (s FS) Close() error { + return unix.Close(int(s)) +} + +// Open wraps openat2(2) with O_RDONLY+O_NOFOLLOW+O_CLOEXEC. +func (s FS) Open(path string) (fs.File, error) { + fd, err := unix.Openat2(int(s), path, &unix.OpenHow{ + Flags: unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_CLOEXEC, + Mode: 0, + Resolve: resolveFlags, + }) + if err != nil { + return nil, err + } + f := os.NewFile(uintptr(fd), path) + return f, nil +} + +// Lchown wraps fchownat(2) (with AT_SYMLINK_NOFOLLOW). +func (s FS) Lchown(path string, uid, gid int) error { + return unix.Fchownat(int(s), path, uid, gid, unix.AT_SYMLINK_NOFOLLOW) +} + +// Sub wraps openat2(2) (with O_PATH+O_DIRECTORY+O_NOFOLLOW+O_CLOEXEC), and returns an FS. +func (s FS) Sub(dir string) (FS, error) { + subFD, err := unix.Openat2(int(s), dir, &unix.OpenHow{ + Flags: unix.O_PATH | unix.O_DIRECTORY | unix.O_NOFOLLOW | unix.O_CLOEXEC, + Mode: 0, + Resolve: resolveFlags, + }) + if err != nil { + return 0, err + } + return FS(subFD), nil +} diff --git a/internal/fixperms/fdfs/fdfs_test.go b/internal/fixperms/fdfs/fdfs_test.go new file mode 100644 index 000000000..ccb61cc34 --- /dev/null +++ b/internal/fixperms/fdfs/fdfs_test.go @@ -0,0 +1,61 @@ +//go:build linux + +package fdfs + +import ( + "io/fs" + "os" + "path/filepath" + "testing" +) + +func TestTOCTOUShenanigans(t *testing.T) { + path := "/tmp/TestTOCTOUShenanigans/foo" + if err := os.MkdirAll(path, 0o777); err != nil { + t.Fatalf("os.MkdirAll(%s, %o) = %v", path, 0o777, err) + } + fp := filepath.Join(path, "data") + if err := os.WriteFile(fp, []byte("innocent"), 0o666); err != nil { + t.Fatalf("os.WriteFile(%s, nil, 0o666) = %v", fp, err) + } + + path2 := "/tmp/TestTOCTOUShenanigans/crimes" + if err := os.MkdirAll(path2, 0o777); err != nil { + t.Fatalf("os.MkdirAll(%s, %o) = %v", path2, 0o777, err) + } + fp2 := filepath.Join(path2, "data") + if err := os.WriteFile(fp2, []byte("guilty"), 0o666); err != nil { + t.Fatalf("os.WriteFile(%s, nil, 0o666) = %v", fp2, err) + } + + // Do it in two steps, to simulate a trusted directory and an untrusted + // subpath. + fsys, err := DirFS("/tmp/TestTOCTOUShenanigans") + if err != nil { + t.Fatalf("DirFS(/tmp/TestTOCTOUShenanigans) error = %v", err) + } + defer fsys.Close() + fooFS, err := fsys.Sub("foo") + if err != nil { + t.Fatalf("DirFS(/tmp/TestTOCTOUShenanigans).Sub(foo) error = %v", err) + } + defer fooFS.Close() + + // Replace foo with a symlink to crimes... + path3 := "/tmp/TestTOCTOUShenanigans/foo.bak" + if err := os.Rename(path, path3); err != nil { + t.Fatalf("os.Rename(%s, %s) = %v", path, path3, err) + } + if err := os.Symlink(path2, path); err != nil { + t.Fatalf("os.Symlink(%s, %s) = %v", path2, path, err) + } + + // What do we get? + df, err := fs.ReadFile(fooFS, "data") + if err != nil { + t.Fatalf("fs.ReadFile(DirFS(%s), data) error = %v", path, err) + } + if got, want := string(df), "innocent"; got != want { + t.Fatalf("fs.ReadFile(DirFS(%s), data) contents = %q, want %q", path, got, want) + } +} diff --git a/internal/fixperms/fixer/fixer.go b/internal/fixperms/fixer/fixer.go new file mode 100644 index 000000000..49a688ec2 --- /dev/null +++ b/internal/fixperms/fixer/fixer.go @@ -0,0 +1,85 @@ +//go:buid linux + +package fixer + +import ( + "errors" + "fmt" + "io/fs" + "os/user" + "path/filepath" + "strconv" + "strings" + + "github.com/buildkite/elastic-ci-stack-for-aws/v6/internal/fixperms/fdfs" +) + +// Main contains the higher-level operations of the permissions fixer. +func Main(argv []string, baseDir, uname string) (string, int) { + if len(argv) != 4 { + return exitf(1, "Usage: %s AGENT_DIR ORG_DIR PIPELINE_DIR", argv[0]) + } + for _, seg := range argv[1:] { + if seg != filepath.Clean(seg) { + return exitf(2, "Invalid argument %q", seg) + } + if seg == "." || seg == ".." || strings.ContainsRune(seg, '/') { + return exitf(2, "Invalid argument %q", seg) + } + } + subpath := filepath.Join(argv[1:]...) + + // Get a file descriptor for the base builds directory. + bd, err := fdfs.DirFS(baseDir) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return exit0() + } + return exitf(3, "Couldn't open %s: %v", baseDir, err) + } + defer bd.Close() + + // Get a file descriptor for the agentdir/orgdir/pipelinedir within the + // builds directory. + // openat2(2) flags ensures this is within the builds directory, and does + // not involve a symlink. + pd, err := bd.Sub(subpath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return exit0() + } + return exitf(3, "Couldn't open %s: %v", subpath, err) + } + defer pd.Close() + + // Get the uid and gid of buildkite-agent + agentUser, err := user.Lookup(uname) + if err != nil { + return exitf(4, "Couldn't look up buildkite-agent user: %v", err) + } + uid, err := strconv.Atoi(agentUser.Uid) + if err != nil { + return exitf(4, "buildkite-agent uid %q not an integer: %v", agentUser.Uid, err) + } + gid, err := strconv.Atoi(agentUser.Gid) + if err != nil { + return exitf(4, "buildkite-agent gid %q not an integer: %v", agentUser.Gid, err) + } + + // fs.WalkDir to find everything within the directory. + // fchownat(2) to change the owner of the item. + // We allow symlinks here, but operate on the symlinks themselves. + if err := fs.WalkDir(pd, ".", func(path string, d fs.DirEntry, err error) error { + return pd.Lchown(path, uid, gid) + }); err != nil { + return exitf(5, "Couldn't recursively chown %s: %v", subpath, err) + } + + return exit0() +} + +func exit0() (string, int) { return "", 0 } + +func exitf(code int, f string, v ...any) (string, int) { + return fmt.Sprintf(f, v...), code +} diff --git a/internal/fixperms/fixer/fixer_test.go b/internal/fixperms/fixer/fixer_test.go new file mode 100644 index 000000000..838c5e7c9 --- /dev/null +++ b/internal/fixperms/fixer/fixer_test.go @@ -0,0 +1,138 @@ +//go:build linux + +package fixer + +import ( + "fmt" + "io/fs" + "os" + "os/exec" + "syscall" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestFixer_SlashesErrors(t *testing.T) { + tests := [][]string{ + {"os.Args[0]", "/", "abc", "abc"}, + {"os.Args[0]", "abc/", "abc", "abc"}, + {"os.Args[0]", "/abc", "abc", "abc"}, + {"os.Args[0]", "abc/def", "abc", "abc"}, + {"os.Args[0]", "abc/def/ghi", "abc", "abc"}, + {"os.Args[0]", "/abc/", "abc", "abc"}, + {"os.Args[0]", "abc", "/", "abc"}, + {"os.Args[0]", "abc", "abc/", "abc"}, + {"os.Args[0]", "abc", "/abc", "abc"}, + {"os.Args[0]", "abc", "abc/def", "abc"}, + {"os.Args[0]", "abc", "abc/def/ghi", "abc"}, + {"os.Args[0]", "abc", "/abc/", "abc"}, + {"os.Args[0]", "abc", "abc", "/"}, + {"os.Args[0]", "abc", "abc", "abc/"}, + {"os.Args[0]", "abc", "abc", "/abc"}, + {"os.Args[0]", "abc", "abc", "abc/def"}, + {"os.Args[0]", "abc", "abc", "abc/def/ghi"}, + {"os.Args[0]", "abc", "abc", "/abc/"}, + } + for _, test := range tests { + _, code := Main(test, "/code/internal/fixperms/fixtures", "root") + if got, want := code, 2; got != want { + t.Errorf("Main(%v) code = %d, want %d", test, got, want) + } + } +} + +func TestFixer_DotsErrors(t *testing.T) { + tests := [][]string{ + {"os.Args[0]", ".", "abc", "abc"}, + {"os.Args[0]", "..", "abc", "abc"}, + {"os.Args[0]", "abc", ".", "abc"}, + {"os.Args[0]", "abc", "..", "abc"}, + {"os.Args[0]", "abc", "abc", "."}, + {"os.Args[0]", "abc", "abc", ".."}, + } + for _, test := range tests { + _, code := Main(test, "/code/internal/fixperms/fixtures", "root") + if got, want := code, 2; got != want { + t.Errorf("Main(%v) code = %d, want %d", test, got, want) + } + } +} + +func TestFixer_SymlinksErrors(t *testing.T) { + tests := [][]string{ + {"os.Args[0]", "link", "b", "c"}, + {"os.Args[0]", "a", "link", "c"}, + {"os.Args[0]", "a", "b", "link"}, + } + for _, test := range tests { + _, code := Main(test, "/code/internal/fixperms/fixtures", "root") + if got, want := code, 3; got != want { + t.Errorf("Main(%v) code = %d, want %d", test, got, want) + } + } +} + +func TestFixer_NonDirectoryErrors(t *testing.T) { + argv := []string{"os.Args[0]", "d", "e", "f"} + _, code := Main(argv, "/code/internal/fixperms/fixtures", "root") + if got, want := code, 3; got != want { + t.Errorf("Main(%v) code = %d, want %d", argv, got, want) + } +} + +func TestFixer_NonExistSkips(t *testing.T) { + argv := []string{"os.Args[0]", "g", "h", "i"} + _, code := Main(argv, "/code/internal/fixperms/fixtures", "root") + if got, want := code, 0; got != want { + t.Errorf("Main(%v) code = %d, want %d", argv, got, want) + } +} + +func TestFixer_Fixes(t *testing.T) { + if err := exec.Command("/usr/bin/cp", "-r", "/code/internal/fixperms/fixtures/a", "/tmp").Run(); err != nil { + t.Fatalf("cp -r fixtures/a /tmp: %v", err) + } + + argv := []string{"os.Args[0]", "a", "b", "c"} + _, code := Main(argv, "/tmp", "nobody") + if got, want := code, 0; got != want { + t.Errorf("Main(%v) code = %d, want %d", argv, got, want) + } + + var gotFiles []string + wantFiles := []string{ + ".", + "d", + "d/e", + "d/link", + "link", + } + + if err := fs.WalkDir(os.DirFS("/tmp/a/b/c"), ".", func(path string, d fs.DirEntry, err error) error { + gotFiles = append(gotFiles, path) + + fi, err := d.Info() + if err != nil { + return err + } + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("file info for %s not a *syscall.Stat_t: %T", path, fi.Sys()) + } + if st.Uid != 65534 { + t.Errorf("uid of %s = %d, want 65534", path, st.Uid) + } + if st.Gid != 65534 { + t.Errorf("gid of %s = %d, want 65534", path, st.Gid) + } + return nil + + }); err != nil { + t.Errorf("fs.WalkDir(/tmp/a/b/c, .) = %v", err) + } + + if diff := cmp.Diff(gotFiles, wantFiles); diff != "" { + t.Errorf("walked files diff (-got +want):\n%s", diff) + } +} diff --git a/internal/fixperms/fixperms.go b/internal/fixperms/fixperms.go new file mode 100644 index 000000000..495adc17e --- /dev/null +++ b/internal/fixperms/fixperms.go @@ -0,0 +1,63 @@ +//go:build linux + +// The fixperms tool changes the ownership of certain files to buildkite-agent. +package main + +import ( + "fmt" + "os" + + "github.com/buildkite/elastic-ci-stack-for-aws/v6/internal/fixperms/fixer" +) + +// Files that are created by Docker containers end up with strange user and +// group ids, usually 0 (root). Docker namespacing will one day save us, but it +// can only map a single docker user id to a given user id (not any docker user +// id to a single system user id). +// +// Until we can map any old user id back to buildkite-agent automatically with +// Docker, then we just need to fix the permissions manually before each build +// runs so git clean can work as expected. +// +// In order to fix ownership of files owned by root, we need to be root. Thus, +// buildkite-agent has rights to run this program with sudo (see sudoers.conf). +// That means we have to take extra care to not chown things we shouldn't. +// +// Q1: Why not `chown -Rh /var/lib/buildkite-agents/builds`? +// A1: That gets slower as more and more builds are run on this agent, hence the +// args that specify a particular pipeline dir. See #340. +// +// Q2: Why not a small script that checks the args for shenanigans, then runs +// `chown -Rh ...`? +// A2: Because of TOCTOU. There's a race between checking that there are no +// symlink shenanigans, and calling `chown`, which provides time for an +// attacker to put some shenanigans back in before `chown` is called. +// +// Q3: What about running `chown -Rh` in a chroot that also contains the dir? +// A3: You have to copy the tools you need to run into the chroot. A job could +// overwrite the tool with its own binary, which is then run as root. +// And if you think you can carefully lay out the chroot and set permissions +// to prevent that, you still need to stop the script receiving a symlink to +// the directory containing chown, changing its own perms. If you add a +// check for that first, then there's still TOCTOU. +// +// Q4: Containers! +// A4: *sigh* +// +// File paths are not a good security interface for files. But! We can use file +// descriptors. openat(2), fchownat(2), etc provide a way to resolve file paths +// relative to a given parent directory, and prevent symlink resolution at the +// same time. + +const ( + buildsDir = "/var/lib/buildkite-agent/builds" + username = "buildkite-agent" +) + +func main() { + msg, code := fixer.Main(os.Args, buildsDir, username) + if code != 0 { + fmt.Fprintln(os.Stderr, msg) + os.Exit(code) + } +} diff --git a/unit-tests/fixtures/a/b/c/.gitkeep b/internal/fixperms/fixtures/a/b/c/d/e similarity index 100% rename from unit-tests/fixtures/a/b/c/.gitkeep rename to internal/fixperms/fixtures/a/b/c/d/e diff --git a/internal/fixperms/fixtures/a/b/c/d/link b/internal/fixperms/fixtures/a/b/c/d/link new file mode 120000 index 000000000..9cbe6ea56 --- /dev/null +++ b/internal/fixperms/fixtures/a/b/c/d/link @@ -0,0 +1 @@ +e \ No newline at end of file diff --git a/internal/fixperms/fixtures/a/b/c/link b/internal/fixperms/fixtures/a/b/c/link new file mode 120000 index 000000000..c59d9b634 --- /dev/null +++ b/internal/fixperms/fixtures/a/b/c/link @@ -0,0 +1 @@ +d \ No newline at end of file diff --git a/unit-tests/fixtures/a/b/link b/internal/fixperms/fixtures/a/b/link similarity index 100% rename from unit-tests/fixtures/a/b/link rename to internal/fixperms/fixtures/a/b/link diff --git a/unit-tests/fixtures/a/link b/internal/fixperms/fixtures/a/link similarity index 100% rename from unit-tests/fixtures/a/link rename to internal/fixperms/fixtures/a/link diff --git a/unit-tests/fixtures/d/e/f b/internal/fixperms/fixtures/d/e/f similarity index 100% rename from unit-tests/fixtures/d/e/f rename to internal/fixperms/fixtures/d/e/f diff --git a/unit-tests/fixtures/g/.gitkeep b/internal/fixperms/fixtures/g/.gitkeep similarity index 100% rename from unit-tests/fixtures/g/.gitkeep rename to internal/fixperms/fixtures/g/.gitkeep diff --git a/unit-tests/fixtures/link b/internal/fixperms/fixtures/link similarity index 100% rename from unit-tests/fixtures/link rename to internal/fixperms/fixtures/link diff --git a/packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions b/packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions deleted file mode 100755 index ae6ab8885..000000000 --- a/packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions +++ /dev/null @@ -1,100 +0,0 @@ -#!/usr/bin/env bash - -# To run the unit tests for this file, run the following command in the root of -# the project: -# $ docker-compose -f docker-compose.unit-tests.yml run unit-tests - -# Files that are created by Docker containers end up with strange user and -# group ids, usually 0 (root). Docker namespacing will one day save us, but it -# can only map a single docker user id to a given user id (not any docker user -# id to a single system user id). -# -# Until we can map any old user id back to -# buildkite-agent automatically with Docker, then we just need to fix the -# permissions manually before each build runs so git clean can work as -# expected. - -set -eu -o pipefail - -# We need to scope the next bit to only the currently running agent dir and -# pipeline, but we also need to control security and make sure arbitrary folders -# can't be chmoded. -# -# We prepare the agent build directory basename in the environment hook and pass -# it as the first argument, org name as second argument, and the pipeline dir as -# the third. -# -# In here we need to check that they both don't contain slashes or contain a -# traversal component. - -AGENT_DIR="$1" -# => "my-agent-1" - -ORG_DIR="$2" -# => "my-org" - -PIPELINE_DIR="$3" -# => "my-pipeline" - -# Make sure it doesn't contain any slashes by substituting slashes with nothing -# and making sure it doesn't change -function exit_if_contains_slashes() { - if [[ "${1//\//}" != "${1}" ]]; then - exit 1 - fi -} - -function exit_if_contains_traversal() { - if [[ "${1}" == "." || "${1}" == ".." ]]; then - exit 2 - fi -} - -function exit_if_blank() { - if [[ -z "${1}" ]]; then - exit 3 - fi -} - -# Check them for slashes -exit_if_contains_slashes "${AGENT_DIR}" -exit_if_contains_slashes "${ORG_DIR}" -exit_if_contains_slashes "${PIPELINE_DIR}" - -# Check them for traversals -exit_if_contains_traversal "${AGENT_DIR}" -exit_if_contains_traversal "${ORG_DIR}" -exit_if_contains_traversal "${PIPELINE_DIR}" - -# Check them for blank values -exit_if_blank "${AGENT_DIR}" -exit_if_blank "${ORG_DIR}" -exit_if_blank "${PIPELINE_DIR}" - -# We know the builds path: -BUILDS_PATH="/var/lib/buildkite-agent/builds" - -# And now we can reconstruct the full agent builds path: -PIPELINE_PATH="${BUILDS_PATH}/${AGENT_DIR}/${ORG_DIR}/${PIPELINE_DIR}" -# => "/var/lib/buildkite-agent/builds/my-agent-1/my-org/my-pipeline" - -# If it doesn't exist, then we won't do anything. -if [[ ! -e "${PIPELINE_PATH}" ]]; then - exit 0 -fi - - -# Check for symlink shenanigans -if [[ "$(realpath "${PIPELINE_PATH}")" != "${PIPELINE_PATH}" ]]; then - exit 4 -fi - -# It should be a directory. -if [[ ! -d "${PIPELINE_PATH}" ]]; then - exit 5 -fi - -# If we make it here, we're safe to go! - -/bin/chown -R buildkite-agent:buildkite-agent "${PIPELINE_PATH}" - diff --git a/unit-tests/fix-buildkite-agent-builds-permissions.bats b/unit-tests/fix-buildkite-agent-builds-permissions.bats deleted file mode 100644 index e7bde924f..000000000 --- a/unit-tests/fix-buildkite-agent-builds-permissions.bats +++ /dev/null @@ -1,162 +0,0 @@ -#!/usr/bin/env bats - -FIX_PERMISSIONS_SCRIPT="/code/packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions" - -@test "Slashes in the agent arg cause an exit 1 (A)" { - run "$FIX_PERMISSIONS_SCRIPT" "/" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the agent arg cause an exit 1 (B)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc/" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the agent arg cause an exit 1 (C)" { - run "$FIX_PERMISSIONS_SCRIPT" "/abc" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the agent arg cause an exit 1 (D)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc/def" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the agent arg cause an exit 1 (E)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc/def/ghi" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the agent arg cause an exit 1 (F)" { - run "$FIX_PERMISSIONS_SCRIPT" "/abc/" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (A)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "/" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (B)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc/" "abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (C)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "/abc" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (D)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc/def" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (E)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc/def/ghi" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the org arg cause an exit 1 (F)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "/abc/" "abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (A)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "/" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (B)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "abc/" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (C)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "/abc" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (D)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "abc/def" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (E)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "abc/def/ghi" - [ "$status" -eq 1 ] -} - -@test "Slashes in the pipeline arg cause an exit 1 (F)" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "/abc/" - [ "$status" -eq 1 ] -} - -@test "Single dot traversal in the agent arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" "." "abc" "abc" - [ "$status" -eq 2 ] -} - -@test "Double dot traversal in the agent arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" ".." "abc" "abc" - [ "$status" -eq 2 ] -} - -@test "Single dot traversal in the org arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "." "abc" - [ "$status" -eq 2 ] -} - -@test "Double dot traversal in the org arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" ".." "abc" - [ "$status" -eq 2 ] -} - -@test "Single dot traversal in the pipeline arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "." - [ "$status" -eq 2 ] -} - -@test "Double dot traversal in the pipeline arg cause an exit 2" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" ".." - [ "$status" -eq 2 ] -} - -@test "Blank agent arg cause an exit 3" { - run "$FIX_PERMISSIONS_SCRIPT" "" "abc" "abc" - [ "$status" -eq 3 ] -} - -@test "Blank org arg cause an exit 3" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "" "abc" - [ "$status" -eq 3 ] -} - -@test "Blank pipeline arg cause an exit 3" { - run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc" "" - [ "$status" -eq 3 ] -} - -@test "Non-existing path is skipped" { - "$FIX_PERMISSIONS_SCRIPT" "g" "h" "i" -} - -@test "Symlinks in the args cause an exit 4 (A)" { - run "$FIX_PERMISSIONS_SCRIPT" "link" "b" "c" - [ "$status" -eq 4 ] -} - -@test "Symlinks in the args cause an exit 4 (B)" { - run "$FIX_PERMISSIONS_SCRIPT" "a" "link" "c" - [ "$status" -eq 4 ] -} - -@test "Symlinks in the args cause an exit 4 (C)" { - run "$FIX_PERMISSIONS_SCRIPT" "a" "b" "link" - [ "$status" -eq 4 ] -} - -@test "Path not a directory causes an exit 5" { - run "$FIX_PERMISSIONS_SCRIPT" "d" "e" "f" - [ "$status" -eq 5 ] -}