campaigns: widen permissions on mounted paths (#366)

This fixes #365 by ensuring that files and workspaces mounted into
campaign containers are world readable, writable, and executable as
appropriate.

Author: LawnGnome

CHANGELOG.md

@@ -17,6 +17,8 @@ All notable changes to `src-cli` are documented in this file.
 
 ### Fixed
 
+- Campaign steps run in a container that does not run as root could fail on systems that do not map the running user ID to the container, most notably desktop Linux. This has been fixed: temporary files and workspaces mounted into the container now have sufficient permissions to allow the container user to execute the step. [#366](https://github.com/sourcegraph/src-cli/pull/366)
+
 ## 3.21.5
 
 ### Added

internal/campaigns/archive_fetcher.go

@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 	"github.com/sourcegraph/src-cli/internal/api"
 	"github.com/sourcegraph/src-cli/internal/campaigns/graphql"
@@ -67,6 +68,11 @@ func unzipToTempDir(ctx context.Context, zipFile, tempDir, tempFilePrefix string
 	if err != nil {
 		return "", err
 	}
+
+	if err := os.Chmod(volumeDir, 0777); err != nil {
+		return "", err
+	}
+
 	return volumeDir, unzip(zipFile, volumeDir)
 }
 
@@ -86,6 +92,10 @@ func fetchRepositoryArchive(ctx context.Context, client api.Client, repo *graphq
 		return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String())
 	}
 
+	// Unlike the mkdirAll() calls elsewhere in this file, this is only giving
+	// us a temporary place on the filesystem to keep the archive. Since it's
+	// never mounted into the containers being run, we can keep these
+	// directories 0700 without issue.
 	if err := os.MkdirAll(filepath.Dir(dest), 0700); err != nil {
 		return err
 	}
@@ -130,13 +140,13 @@ func unzip(zipFile, dest string) error {
 		}
 
 		if f.FileInfo().IsDir() {
-			if err := os.MkdirAll(fpath, os.ModePerm); err != nil {
+			if err := mkdirAll(dest, f.Name, 0777); err != nil {
 				return err
 			}
 			continue
 		}
 
-		if err := os.MkdirAll(filepath.Dir(fpath), os.ModePerm); err != nil {
+		if err := mkdirAll(dest, filepath.Dir(f.Name), 0777); err != nil {
 			return err
 		}
 
@@ -145,6 +155,16 @@ func unzip(zipFile, dest string) error {
 			return err
 		}
 
+		// Since the container might not run as the same user, we need to ensure
+		// that the file is globally writable. If the execute bit is normally
+		// set on the zipped up file, let's ensure we propagate that to the
+		// group and other permission bits too.
+		if f.Mode()&0111 != 0 {
+			outFile.Chmod(0777)
+		} else {
+			outFile.Chmod(0666)
+		}
+
 		rc, err := f.Open()
 		if err != nil {
 			outFile.Close()
@@ -166,3 +186,61 @@ func unzip(zipFile, dest string) error {
 
 	return nil
 }
+
+// Technically, this is a misnomer, since it might be a socket or block special,
+// but errPathExistsAsNonDir is just ugly for an internal type.
+type errPathExistsAsFile string
+
+var _ error = errPathExistsAsFile("")
+
+func (e errPathExistsAsFile) Error() string {
+	return fmt.Sprintf("path already exists, but not as a directory: %s", string(e))
+}
+
+// mkdirAll is essentially os.MkdirAll(filepath.Join(base, path), perm), but
+// applies the given permission regardless of the user's umask.
+func mkdirAll(base, path string, perm os.FileMode) error {
+	abs := filepath.Join(base, path)
+
+	// Create the directory if it doesn't exist.
+	st, err := os.Stat(abs)
+	if err != nil {
+		// It's expected that we'll get an error if the directory doesn't exist,
+		// so let's check that it's of the type we expect.
+		if !os.IsNotExist(err) {
+			return err
+		}
+
+		// Now we're clear to create the directory.
+		if err := os.MkdirAll(abs, perm); err != nil {
+			return err
+		}
+	} else if !st.IsDir() {
+		// The file/socket/whatever exists, but it's not a directory. That's
+		// definitely going to be an issue.
+		return errPathExistsAsFile(abs)
+	}
+
+	// If os.MkdirAll() was invoked earlier, then the permissions it set were
+	// subject to the umask. Let's walk the directories we may or may not have
+	// created and ensure their permissions look how we want.
+	return ensureAll(base, path, perm)
+}
+
+// ensureAll ensures that all directories under path have the expected
+// permissions.
+func ensureAll(base, path string, perm os.FileMode) error {
+	var errs *multierror.Error
+
+	// In plain English: for each directory in the path parameter, we should
+	// chmod that path to the permissions that are expected.
+	acc := []string{base}
+	for _, element := range strings.Split(path, string(os.PathSeparator)) {
+		acc = append(acc, element)
+		if err := os.Chmod(filepath.Join(acc...), perm); err != nil {
+			errs = multierror.Append(errs, err)
+		}
+	}
+
+	return errs.ErrorOrNil()
+}

internal/campaigns/archive_fetcher_nonwin_test.go

@@ -0,0 +1,53 @@
+// +build !windows
+
+package campaigns
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/pkg/errors"
+)
+
+func TestMkdirAllStatError(t *testing.T) {
+	// This test can't be trivially reproduced on Windows, so we just won't run
+	// it there.
+
+	// Create a shared workspace.
+	base := mustCreateWorkspace(t)
+	defer os.RemoveAll(base)
+
+	// We'll create a directory and a file within it, remove the execute bit on
+	// the directory, and then stat() the file to cause a failure.
+	if err := os.MkdirAll(filepath.Join(base, "locked"), 0700); err != nil {
+		t.Fatal(err)
+	}
+
+	f, err := os.Create(filepath.Join(base, "locked", "file"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	f.Close()
+
+	if err := os.Chmod(filepath.Join(base, "locked"), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	err = mkdirAll(base, "locked/file", 0750)
+	if err == nil {
+		t.Errorf("unexpected nil error")
+	} else if _, ok := err.(errPathExistsAsFile); ok {
+		t.Errorf("unexpected error of type %T: %v", err, err)
+	}
+
+}
+
+func mustHavePerm(t *testing.T, path string, want os.FileMode) error {
+	t.Helper()
+
+	if have := mustGetPerm(t, path); have != want {
+		return errors.Errorf("unexpected permissions: have=%o want=%o", have, want)
+	}
+	return nil
+}

internal/campaigns/archive_fetcher_test.go

@@ -0,0 +1,141 @@
+package campaigns
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestMkdirAll(t *testing.T) {
+	// TestEnsureAll does most of the heavy lifting here; we're just testing the
+	// MkdirAll scenarios here around whether the directory exists.
+
+	// Create a shared workspace.
+	base := mustCreateWorkspace(t)
+	defer os.RemoveAll(base)
+
+	t.Run("directory exists", func(t *testing.T) {
+		if err := os.MkdirAll(filepath.Join(base, "exist"), 0755); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := mkdirAll(base, "exist", 0750); err != nil {
+			t.Errorf("unexpected non-nil error: %v", err)
+		}
+
+		if err := mustHavePerm(t, filepath.Join(base, "exist"), 0750); err != nil {
+			t.Error(err)
+		}
+
+		if !isDir(t, filepath.Join(base, "exist")) {
+			t.Error("not a directory")
+		}
+	})
+
+	t.Run("directory does not exist", func(t *testing.T) {
+		if err := mkdirAll(base, "new", 0750); err != nil {
+			t.Errorf("unexpected non-nil error: %v", err)
+		}
+
+		if err := mustHavePerm(t, filepath.Join(base, "new"), 0750); err != nil {
+			t.Error(err)
+		}
+
+		if !isDir(t, filepath.Join(base, "new")) {
+			t.Error("not a directory")
+		}
+	})
+
+	t.Run("directory exists, but is not a directory", func(t *testing.T) {
+		f, err := os.Create(filepath.Join(base, "file"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		f.Close()
+
+		err = mkdirAll(base, "file", 0750)
+		if _, ok := err.(errPathExistsAsFile); !ok {
+			t.Errorf("unexpected error of type %T: %v", err, err)
+		}
+	})
+}
+
+func TestEnsureAll(t *testing.T) {
+	// Create a workspace.
+	base := mustCreateWorkspace(t)
+	defer os.RemoveAll(base)
+
+	// Create three nested directories with 0700 permissions. We'll use Chmod
+	// explicitly to avoid any umask issues.
+	if err := os.MkdirAll(filepath.Join(base, "a", "b", "c"), 0700); err != nil {
+		t.Fatal(err)
+	}
+	dirs := []string{
+		filepath.Join(base, "a"),
+		filepath.Join(base, "a", "b"),
+		filepath.Join(base, "a", "b", "c"),
+	}
+	for _, dir := range dirs {
+		if err := os.Chmod(dir, 0700); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Now we'll set them to 0750 and see what happens.
+	if err := ensureAll(base, "a/b/c", 0750); err != nil {
+		t.Errorf("unexpected non-nil error: %v", err)
+	}
+	for _, dir := range dirs {
+		if err := mustHavePerm(t, dir, 0750); err != nil {
+			t.Error(err)
+		}
+	}
+	if err := mustHavePerm(t, base, 0700); err != nil {
+		t.Error(err)
+	}
+
+	// Finally, let's ensure we get an error when we try to ensure a directory
+	// that doesn't exist.
+	if err := ensureAll(base, "d", 0750); err == nil {
+		t.Errorf("unexpected nil error")
+	}
+}
+
+func mustCreateWorkspace(t *testing.T) string {
+	base, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We'll explicitly set the base workspace to 0700 so we have a known
+	// environment for testing.
+	if err := os.Chmod(base, 0700); err != nil {
+		t.Fatal(err)
+	}
+
+	return base
+}
+
+func mustGetPerm(t *testing.T, file string) os.FileMode {
+	t.Helper()
+
+	st, err := os.Stat(file)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We really only need the lower bits here.
+	return st.Mode() & 0777
+}
+
+func isDir(t *testing.T, path string) bool {
+	t.Helper()
+
+	st, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return st.IsDir()
+}

internal/campaigns/archive_fetcher_windows_test.go

@@ -0,0 +1,32 @@
+package campaigns
+
+import (
+	"os"
+	"testing"
+
+	"github.com/pkg/errors"
+)
+
+func mustHavePerm(t *testing.T, path string, want os.FileMode) error {
+	t.Helper()
+
+	have := mustGetPerm(t, path)
+
+	// Go maps Windows file attributes onto Unix permissions in a fairly trivial
+	// way: readonly files will be 0444, normal files will be 0666, and
+	// directories will have 0111 or-ed onto that value. The end. Source:
+	// https://sourcegraph.com/github.com/golang/go@fd841f65368906923e287afab91857043036459d/-/blob/src/os/types_windows.go#L112-134
+	if want&0222 != 0 {
+		want = 0666
+	} else {
+		want = 0444
+	}
+	if isDir(t, path) {
+		want = want | 0111
+	}
+
+	if have != want {
+		return errors.Errorf("unexpected permissions: have=%o want=%o", have, want)
+	}
+	return nil
+}

internal/campaigns/run_steps.go

@@ -112,6 +112,19 @@ func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repositor
 			return nil, errors.Wrap(err, "closing temporary file")
 		}
 
+		// This file needs to be readable within the container regardless of the
+		// user the container is running as, so we'll set the appropriate group
+		// and other bits to make it so.
+		//
+		// A fun note: although os.File exposes a Chmod() method, we can't
+		// unconditionally use it because Windows cannot change the attributes
+		// of an open file. Rather than going to the trouble of having
+		// conditionally compiled files here, instead we'll just wait until the
+		// file is closed to twiddle the permission bits. Which is now!
+		if err := os.Chmod(runScriptFile.Name(), 0644); err != nil {
+			return nil, errors.Wrap(err, "setting permissions on the temporary file")
+		}
+
 		// Parse and render the step.Files.
 		files, err := renderStepFiles(step.Files, &stepContext)
 		if err != nil {