Merge pull request #4522 from dctrud/issue4517

Set workable permissions on OCI -> sandbox rootless builds

e2e/internal/e2e/fileutil.go

@@ -88,3 +88,14 @@ func FileExists(t *testing.T, path string) bool {
 	// todo: we should check if it is a file
 	return PathExists(t, path)
 }
+
+// PathPerms return true if the path (file or directory) has specified permissions, false otherwise.
+func PathPerms(t *testing.T, path string, perms os.FileMode) bool {
+	s, err := os.Stat(path)
+
+	if err != nil {
+		t.Fatalf("While stating file: %v", err)
+	}
+
+	return s.Mode().Perm() == perms
+}

e2e/regressions/build.go

@@ -7,6 +7,7 @@ package regressions
 
 import (
 	"os"
+	"path"
 	"path/filepath"
 	"testing"
 
@@ -56,14 +57,59 @@ func (c *regressionsTests) issue4203(t *testing.T) {
 	)
 }
 
+// This test will build a sandbox, as a non-root user from a dockerhub image
+// that contains a single folder and file with `000` permissions.
+// To verify we force files to be accessible / moveable / removable by the user
+// we check for `700` and `400` permissions on the folder and file respectively.
+func (c *regressionsTests) issue4524(t *testing.T) {
+	sandbox := filepath.Join(c.env.TestDir, "issue_4524")
+
+	c.env.RunSingularity(
+		t,
+		e2e.WithPrivileges(false),
+		e2e.WithCommand("build"),
+		e2e.WithArgs("--sandbox", sandbox, "docker://sylabsio/issue4524"),
+		e2e.PostRun(func(t *testing.T) {
+
+			// If we failed to build the sandbox completely, leave what we have for
+			// investigation.
+			if t.Failed() {
+				t.Logf("Test %s failed, not removing directory %s", t.Name(), sandbox)
+				return
+			}
+
+			if !e2e.PathPerms(t, path.Join(sandbox, "directory"), 0700) {
+				t.Error("Expected 0700 permissions on 000 test directory in rootless sandbox")
+			}
+			if !e2e.PathPerms(t, path.Join(sandbox, "file"), 0600) {
+				t.Error("Expected 0600 permissions on 000 test file in rootless sandbox")
+			}
+
+			// If the permissions aren't as we expect them to be, leave what we have for
+			// investigation.
+			if t.Failed() {
+				t.Logf("Test %s failed, not removing directory %s", t.Name(), sandbox)
+				return
+			}
+
+			err := os.RemoveAll(sandbox)
+			if err != nil {
+				t.Logf("Cannot remove sandbox directory: %#v", err)
+			}
+
+		}),
+		e2e.ExpectExit(0),
+	)
+}
+
 // RunE2ETests is the main func to trigger the test suite
 func RunE2ETests(env e2e.TestEnv) func(*testing.T) {
 	c := &regressionsTests{
 		env: env,
 	}
 
 	return func(t *testing.T) {
-		// https://github.com/sylabs/singularity/issues/4203
-		t.Run("Issue4203", c.issue4203)
+		t.Run("Issue 4203", c.issue4203) // https://github.com/sylabs/singularity/issues/4203
+		t.Run("Issue 4524", c.issue4524) // https://github.com/sylabs/singularity/issues/4524
 	}
 }

internal/pkg/build/sources/oci_unpack_linux.go

@@ -14,12 +14,15 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
+	"sort"
 
 	"github.com/containers/image/types"
 	"github.com/openSUSE/umoci"
 	umocilayer "github.com/openSUSE/umoci/oci/layer"
 	"github.com/openSUSE/umoci/pkg/idtools"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/sylabs/singularity/internal/pkg/sylog"
 	sytypes "github.com/sylabs/singularity/pkg/build/types"
 )
 
@@ -68,5 +71,119 @@ func unpackRootfs(b *sytypes.Bundle, tmpfsRef types.ImageReference, sysCtx *type
 	os.RemoveAll(b.Rootfs())
 
 	// Unpack root filesystem
-	return umocilayer.UnpackRootfs(context.Background(), engineExt, b.Rootfs(), manifest, &mapOptions)
+	err = umocilayer.UnpackRootfs(context.Background(), engineExt, b.Rootfs(), manifest, &mapOptions)
+	if err != nil {
+		return fmt.Errorf("error unpacking rootfs: %s", err)
+	}
+
+	// If this is a rootless extraction we need to mangle permissions to fix #4524. This
+	// returns to the <=3.3 permissions on the rootfs, with the exception that umoci
+	// correctly applies permission changes across layers when extracting.
+	if mapOptions.Rootless {
+		sylog.Debugf("Modifying rootless permissions on temporary rootfs")
+		return fixPermsRootless(b.Rootfs())
+	}
+
+	return nil
+}
+
+// fixPermsRootless forces permissions on the rootfs so that it can be easily
+// moved and deleted by a non-root user owner.
+func fixPermsRootless(rootfs string) (err error) {
+	errors := 0
+	err = permWalk(rootfs, func(path string, f os.FileInfo, err error) error {
+		if err != nil {
+			sylog.Errorf("Unable to access sandbox path %s: %s", path, err)
+			errors++
+			return nil
+		}
+		// Directories must have the owner 'rx' bits to allow traversal and reading on move, and the 'w' bit
+		// so their content can be deleted by the user when the rootfs/sandbox is deleted
+		switch mode := f.Mode(); {
+		case mode.IsDir():
+			if err := os.Chmod(path, f.Mode().Perm()|0700); err != nil {
+				sylog.Errorf("Error setting rootless permission for %s: %s", path, err)
+				errors++
+			}
+		case mode.IsRegular():
+			// Regular files must have the owner 'r' bit so that everything can be read in order to
+			// copy or move the rootfs/sandbox around. Also, the `w` bit as the build does write into
+			// some files (e.g. resolv.conf) in the container rootfs.
+			if err := os.Chmod(path, f.Mode().Perm()|0600); err != nil {
+				sylog.Errorf("Error setting rootless permission for %s: %s", path, err)
+				errors++
+			}
+		}
+		return nil
+	})
+
+	if errors > 0 {
+		err = fmt.Errorf("%d errors were encountered when setting permissions", errors)
+	}
+
+	return err
+}
+
+// permWalk is similar to os.Walk - but:
+//   1. The skipDir checks are removed (we never want to skip anything here)
+//   2. Our walk will call walkFn on a directory *before* attempting to look
+//      inside that directory.
+func permWalk(root string, walkFn filepath.WalkFunc) error {
+	info, err := os.Lstat(root)
+	if err != nil {
+		return fmt.Errorf("could not access rootfs %s: %s", root, err)
+	}
+	return walk(root, info, walkFn)
+}
+
+func walk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error {
+	if !info.IsDir() {
+		return walkFn(path, info, nil)
+	}
+
+	// Unlike filepath.walk we call walkFn *before* trying to list the content of
+	// the directory, so that walkFn has a chance to assign perms that allow us into
+	// the directory, if we can't get in there already.
+	if err := walkFn(path, info, nil); err != nil {
+		return err
+	}
+
+	names, err := readDirNames(path)
+	if err != nil {
+		return err
+	}
+
+	for _, name := range names {
+		filename := filepath.Join(path, name)
+		fileInfo, err := os.Lstat(filename)
+		if err != nil {
+			if err := walkFn(filename, fileInfo, err); err != nil {
+				return err
+			}
+		} else {
+			err = walk(filename, fileInfo, walkFn)
+			if err != nil {
+				if !fileInfo.IsDir() {
+					return err
+				}
+			}
+		}
+	}
+	return nil
+}
+
+// readDirNames reads the directory named by dirname and returns
+// a sorted list of directory entries.
+func readDirNames(dirname string) ([]string, error) {
+	f, err := os.Open(dirname)
+	if err != nil {
+		return nil, err
+	}
+	names, err := f.Readdirnames(-1)
+	f.Close()
+	if err != nil {
+		return nil, err
+	}
+	sort.Strings(names)
+	return names, nil
 }