Merge pull request #44268 from thaJeztah/idtools_cleanup3

pkg/idtools: remove CanAccess(), and move to daemon

daemon/daemon_unix.go

@@ -15,6 +15,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/containerd/cgroups"
@@ -1247,7 +1248,7 @@ func setupDaemonRoot(config *config.Config, rootDir string, remappedRoot idtools
 			if dirPath == "/" {
 				break
 			}
-			if !idtools.CanAccess(dirPath, remappedRoot) {
+			if !canAccess(dirPath, remappedRoot) {
 				return fmt.Errorf("a subdirectory in your graphroot path (%s) restricts access to the remapped root uid/gid; please fix by allowing 'o+x' permissions on existing directories", config.Root)
 			}
 		}
@@ -1259,6 +1260,34 @@ func setupDaemonRoot(config *config.Config, rootDir string, remappedRoot idtools
 	return nil
 }
 
+// canAccess takes a valid (existing) directory and a uid, gid pair and determines
+// if that uid, gid pair has access (execute bit) to the directory.
+//
+// Note: this is a very rudimentary check, and may not produce accurate results,
+// so should not be used for anything other than the current use, see:
+// https://github.com/moby/moby/issues/43724
+func canAccess(path string, pair idtools.Identity) bool {
+	statInfo, err := os.Stat(path)
+	if err != nil {
+		return false
+	}
+	perms := statInfo.Mode().Perm()
+	if perms&0o001 == 0o001 {
+		// world access
+		return true
+	}
+	ssi := statInfo.Sys().(*syscall.Stat_t)
+	if ssi.Uid == uint32(pair.UID) && (perms&0o100 == 0o100) {
+		// owner access.
+		return true
+	}
+	if ssi.Gid == uint32(pair.GID) && (perms&0o010 == 0o010) {
+		// group access.
+		return true
+	}
+	return false
+}
+
 func setupDaemonRootPropagation(cfg *config.Config) error {
 	rootParentMount, mountOptions, err := getSourceMount(cfg.Root)
 	if err != nil {

pkg/idtools/idtools_unix.go

@@ -14,7 +14,6 @@ import (
 	"sync"
 	"syscall"
 
-	"github.com/docker/docker/pkg/system"
 	"github.com/opencontainers/runc/libcontainer/user"
 )
 
@@ -29,7 +28,7 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting
 		return err
 	}
 
-	stat, err := system.Stat(path)
+	stat, err := os.Stat(path)
 	if err == nil {
 		if !stat.IsDir() {
 			return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
@@ -38,8 +37,8 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting
 			return nil
 		}
 
-		// short-circuit--we were called with an existing directory and chown was requested
-		return setPermissions(path, mode, owner.UID, owner.GID, stat)
+		// short-circuit -- we were called with an existing directory and chown was requested
+		return setPermissions(path, mode, owner, stat)
 	}
 
 	// make an array containing the original path asked for, plus (for mkAll == true)
@@ -60,51 +59,26 @@ func mkdirAs(path string, mode os.FileMode, owner Identity, mkAll, chownExisting
 			if dirPath == "/" {
 				break
 			}
-			if _, err := os.Stat(dirPath); err != nil && os.IsNotExist(err) {
+			if _, err = os.Stat(dirPath); err != nil && os.IsNotExist(err) {
 				paths = append(paths, dirPath)
 			}
 		}
-		if err := os.MkdirAll(path, mode); err != nil {
-			return err
-		}
-	} else {
-		if err := os.Mkdir(path, mode); err != nil && !os.IsExist(err) {
+		if err = os.MkdirAll(path, mode); err != nil {
 			return err
 		}
+	} else if err = os.Mkdir(path, mode); err != nil {
+		return err
 	}
 	// even if it existed, we will chown the requested path + any subpaths that
 	// didn't exist when we called MkdirAll
 	for _, pathComponent := range paths {
-		if err := setPermissions(pathComponent, mode, owner.UID, owner.GID, nil); err != nil {
+		if err = setPermissions(pathComponent, mode, owner, nil); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-// CanAccess takes a valid (existing) directory and a uid, gid pair and determines
-// if that uid, gid pair has access (execute bit) to the directory
-func CanAccess(path string, pair Identity) bool {
-	statInfo, err := system.Stat(path)
-	if err != nil {
-		return false
-	}
-	perms := os.FileMode(statInfo.Mode()).Perm()
-	if perms&0o001 == 0o001 {
-		// world access
-		return true
-	}
-	if statInfo.UID() == uint32(pair.UID) && (perms&0o100 == 0o100) {
-		// owner access.
-		return true
-	}
-	if statInfo.GID() == uint32(pair.GID) && (perms&0o010 == 0o010) {
-		// group access.
-		return true
-	}
-	return false
-}
-
 // LookupUser uses traditional local system files lookup (from libcontainer/user) on a username,
 // followed by a call to `getent` for supporting host configured non-files passwd and group dbs
 func LookupUser(name string) (user.User, error) {
@@ -229,23 +203,24 @@ func getExitCode(err error) (int, error) {
 // Normally a Chown is a no-op if uid/gid match, but in some cases this can still cause an error, e.g. if the
 // dir is on an NFS share, so don't call chown unless we absolutely must.
 // Likewise for setting permissions.
-func setPermissions(p string, mode os.FileMode, uid, gid int, stat *system.StatT) error {
+func setPermissions(p string, mode os.FileMode, owner Identity, stat os.FileInfo) error {
 	if stat == nil {
 		var err error
-		stat, err = system.Stat(p)
+		stat, err = os.Stat(p)
 		if err != nil {
 			return err
 		}
 	}
-	if os.FileMode(stat.Mode()).Perm() != mode.Perm() {
+	if stat.Mode().Perm() != mode.Perm() {
 		if err := os.Chmod(p, mode.Perm()); err != nil {
 			return err
 		}
 	}
-	if stat.UID() == uint32(uid) && stat.GID() == uint32(gid) {
+	ssi := stat.Sys().(*syscall.Stat_t)
+	if ssi.Uid == uint32(owner.UID) && ssi.Gid == uint32(owner.GID) {
 		return nil
 	}
-	return os.Chown(p, uid, gid)
+	return os.Chown(p, owner.UID, owner.GID)
 }
 
 // LoadIdentityMapping takes a requested username and

pkg/idtools/idtools_unix_test.go

@@ -6,8 +6,10 @@ package idtools // import "github.com/docker/docker/pkg/idtools"
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"os/user"
 	"path/filepath"
+	"syscall"
 	"testing"
 
 	"golang.org/x/sys/unix"
@@ -425,7 +427,12 @@ func TestNewIDMappings(t *testing.T) {
 
 	err = MkdirAllAndChown(dirName, 0o700, Identity{UID: rootUID, GID: rootGID})
 	assert.Check(t, err, "Couldn't change ownership of file path. Got error")
-	assert.Check(t, CanAccess(dirName, idMapping.RootPair()), "Unable to access %s directory with user UID:%d and GID:%d", dirName, rootUID, rootGID)
+	cmd := exec.Command("ls", "-la", dirName)
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Credential: &syscall.Credential{Uid: uint32(rootUID), Gid: uint32(rootGID)},
+	}
+	out, err := cmd.CombinedOutput()
+	assert.Check(t, err, "Unable to access %s directory with user UID:%d and GID:%d:\n%s", dirName, rootUID, rootGID, string(out))
 }
 
 func TestLookupUserAndGroup(t *testing.T) {