From 712dadda599ef9fe5982c00e776127653a836a3d Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Wed, 25 Sep 2019 15:48:09 -0500 Subject: [PATCH] Fix #4524 - set permissions on rootless sandboxes This fix addresses #4524, where the recent change to use the umoci extracter for OCI layers can lead to sandboxes that cannot be mv'd across filesystems (leading to a build failure), or deleted/modified by the user. Return to the <=3.3 behaviour here, by modifying permissions on the temporary rootfs sandbox once it is extracted, and before the mv to its final location. --- e2e/internal/e2e/fileutil.go | 11 ++ e2e/regressions/build.go | 50 +++++++- .../pkg/build/sources/oci_unpack_linux.go | 119 +++++++++++++++++- 3 files changed, 177 insertions(+), 3 deletions(-) diff --git a/e2e/internal/e2e/fileutil.go b/e2e/internal/e2e/fileutil.go index ff4f5131dc..c7b59399c9 100644 --- a/e2e/internal/e2e/fileutil.go +++ b/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 +} diff --git a/e2e/regressions/build.go b/e2e/regressions/build.go index 01156c4625..4394727bb4 100644 --- a/e2e/regressions/build.go +++ b/e2e/regressions/build.go @@ -7,6 +7,7 @@ package regressions import ( "os" + "path" "path/filepath" "testing" @@ -56,6 +57,51 @@ 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 := ®ressionsTests{ @@ -63,7 +109,7 @@ func RunE2ETests(env e2e.TestEnv) func(*testing.T) { } 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 } } diff --git a/internal/pkg/build/sources/oci_unpack_linux.go b/internal/pkg/build/sources/oci_unpack_linux.go index decbe585ee..c6ed43f987 100644 --- a/internal/pkg/build/sources/oci_unpack_linux.go +++ b/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 }