fix: check user/group in container start

daemon/mgr/container.go

@@ -288,20 +288,6 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, err
 	}
 
-	// Get snapshot UpperDir
-	var upperDir string
-	mounts, err := mgr.Client.GetMounts(ctx, id)
-	if err != nil {
-		return nil, err
-	} else if len(mounts) != 1 {
-		return nil, fmt.Errorf("failed to get snapshot %s mounts: not equals one", id)
-	}
-	for _, opt := range mounts[0].Options {
-		if strings.HasPrefix(opt, "upperdir=") {
-			upperDir = strings.TrimPrefix(opt, "upperdir=")
-		}
-	}
-
 	// set lxcfs binds
 	if config.HostConfig.EnableLxcfs && lxcfs.IsLxcfsEnabled {
 		config.HostConfig.Binds = append(config.HostConfig.Binds, lxcfs.LxcfsParentDir+":/var/lib/lxc:shared")
@@ -379,16 +365,14 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty
 		return nil, err
 	}
 
-	// set snapshotter for container
-	// TODO(ziren): now we only support overlayfs
-	container.Snapshotter = &types.SnapshotterData{
-		Name: "overlayfs",
-		Data: map[string]string{},
-	}
-
-	if upperDir != "" {
-		container.Snapshotter.Data["UpperDir"] = upperDir
+	// Get snapshot UpperDir
+	mounts, err := mgr.Client.GetMounts(ctx, id)
+	if err != nil {
+		return nil, err
+	} else if len(mounts) != 1 {
+		return nil, fmt.Errorf("failed to get snapshot %s mounts: not equals one", id)
 	}
+	container.SetSnapshotterMeta(mounts)
 
 	// amendContainerSettings modify container config settings to wanted
 	amendContainerSettings(&config.ContainerConfig, config.HostConfig)

daemon/mgr/container_exec.go

@@ -84,7 +84,8 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac
 		execConfig.User = c.Config.User
 	}
 
-	uid, gid, err := user.Get(c.BaseFS, execConfig.User)
+	uid, gid, additionalGids, err := user.Get(c.GetSpecificBasePath(user.PasswdFile),
+		c.GetSpecificBasePath(user.GroupFile), execConfig.User, c.HostConfig.GroupAdd)
 	if err != nil {
 		return err
 	}
@@ -97,7 +98,7 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac
 		User: specs.User{
 			UID:            uid,
 			GID:            gid,
-			AdditionalGids: user.GetAdditionalGids(c.HostConfig.GroupAdd),
+			AdditionalGids: additionalGids,
 		},
 	}
 

daemon/mgr/container_types.go

@@ -5,6 +5,8 @@ import (
 	"io"
 	"net/http"
 	"os"
+	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -13,6 +15,7 @@ import (
 	"github.com/alibaba/pouch/pkg/meta"
 	"github.com/alibaba/pouch/pkg/utils"
 
+	"github.com/containerd/containerd/mount"
 	"github.com/opencontainers/image-spec/specs-go/v1"
 )
 
@@ -352,6 +355,46 @@ func (c *Container) UnsetMergedDir() {
 	c.Snapshotter.Data["MergedDir"] = ""
 }
 
+// SetSnapshotterMeta sets snapshotter for container
+func (c *Container) SetSnapshotterMeta(mounts []mount.Mount) {
+	// TODO(ziren): now we only support overlayfs
+	data := make(map[string]string, 0)
+	for _, opt := range mounts[0].Options {
+		if strings.HasPrefix(opt, "upperdir=") {
+			data["UpperDir"] = strings.TrimPrefix(opt, "upperdir=")
+		}
+		if strings.HasPrefix(opt, "lowerdir=") {
+			data["LowerDir"] = strings.TrimPrefix(opt, "lowerdir=")
+		}
+		if strings.HasPrefix(opt, "workdir=") {
+			data["WorkDir"] = strings.TrimPrefix(opt, "workdir=")
+		}
+	}
+
+	c.Snapshotter = &types.SnapshotterData{
+		Name: "overlayfs",
+		Data: data,
+	}
+}
+
+// GetSpecificBasePath accepts a given path, look for whether the path is exist
+// within container, if has, returns container base path like BaseFS, if not, return empty string
+func (c *Container) GetSpecificBasePath(path string) string {
+	// first try container BaseFS, it is a general view,
+	if utils.IsFileExist(filepath.Join(c.BaseFS, path)) {
+		return c.BaseFS
+	}
+
+	// then try lower and upper directory, since overlay filesystem support only.
+	for _, prefixPath := range c.Snapshotter.Data {
+		if utils.IsFileExist(filepath.Join(prefixPath, path)) {
+			return prefixPath
+		}
+	}
+
+	return ""
+}
+
 // ContainerRestartPolicy represents the policy is used to manage container.
 type ContainerRestartPolicy types.RestartPolicy
 

daemon/mgr/spec_process.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/docker/docker/daemon/caps"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/sirupsen/logrus"
 )
 
 // setupProcess setups spec process.
@@ -73,25 +72,12 @@ func createEnvironment(c *Container) []string {
 }
 
 func setupUser(ctx context.Context, c *Container, s *specs.Spec) (err error) {
-	// container rootfs is created by containerd, pouch just creates a snapshot
-	// id and keeps it in memory. If container is in start process, we can not
-	// find if user if exist in container image, so we do some simple check.
-	var uid, gid uint32
-
-	if c.Config.User != "" {
-		if _, err := os.Stat(c.BaseFS); err != nil {
-			logrus.Infof("snapshot %s is not exist, maybe in start process.", c.BaseFS)
-			uid, gid = user.GetIntegerID(c.Config.User)
-		} else {
-			uid, gid, err = user.Get(c.BaseFS, c.Config.User)
-			if err != nil {
-				return err
-			}
-		}
+	uid, gid, additionalGids, err := user.Get(c.GetSpecificBasePath(user.PasswdFile),
+		c.GetSpecificBasePath(user.GroupFile), c.Config.User, c.HostConfig.GroupAdd)
+	if err != nil {
+		return err
 	}
 
-	additionalGids := user.GetAdditionalGids(c.HostConfig.GroupAdd)
-
 	s.Process.User = specs.User{
 		UID:            uid,
 		GID:            gid,

pkg/user/user.go

@@ -42,15 +42,27 @@ type gidParser struct {
 	otherGroup  []string
 }
 
-// Get accepts user string like uid:gid or username:groupname, and transfers them to format valid uid:gid.
+// Get accepts user and group slice, return valid uid, gid and additional gids.
+// Through Get is a interface returns all user informations runtime-spec need,
+// GetUser, GetIntegerID, GetAdditionalGids still can be used independently.
+func Get(passwdPath, groupPath, user string, groups []string) (uint32, uint32, []uint32, error) {
+	uid, gid, err := GetUser(passwdPath, groupPath, user)
+	if err != nil {
+		return 0, 0, nil, err
+	}
+
+	return uid, gid, GetAdditionalGids(groups), nil
+}
+
+// GetUser accepts user string like <uid|username>:<gid|groupname>, and transfers them to format valid uid:gid.
 // user format example:
 // user
 // uid
 // uid:gid
 // user:group
 // uid:group
 // user:gid
-func Get(path string, user string) (uint32, uint32, error) {
+func GetUser(passwdPath, groupPath, user string) (uint32, uint32, error) {
 	if user == "" {
 		// if user is null, return 0 value as root user
 		return 0, 0, nil
@@ -65,7 +77,7 @@ func Get(path string, user string) (uint32, uint32, error) {
 	ParseString(user, &uidStr, &gidStr)
 
 	// get uid from /etc/passwd
-	uid, err = ParseID(filepath.Join(path, PasswdFile), uidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
+	uid, err = ParseID(filepath.Join(passwdPath, PasswdFile), uidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
 		var up uidParser
 		ParseString(line, &up.user, &up.placeholder, &up.uid)
 		if (idErr == nil && idInt == up.uid) || str == up.user {
@@ -74,41 +86,49 @@ func Get(path string, user string) (uint32, uint32, error) {
 		return 0, false
 	})
 	if err != nil {
-		return 0, 0, err
+		// if uidStr is a integer, treat it as valid uid
+		integer, e := strconv.Atoi(uidStr)
+		if e != nil {
+			return 0, 0, err
+		}
+		uid = uint32(integer)
 	}
 
 	// if gidStr is null, then get gid from /etc/passwd
 	if len(gidStr) == 0 {
-		gid, err = ParseID(filepath.Join(path, PasswdFile), uidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
+		gid, err = ParseID(filepath.Join(passwdPath, PasswdFile), uidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
 			var up uidParser
 			ParseString(line, &up.user, &up.placeholder, &up.uid, &up.gid)
 			if (idErr == nil && idInt == up.uid) || str == up.user {
 				return uint32(up.gid), true
 			}
 			return 0, false
 		})
-		if err != nil {
-			return 0, 0, err
-		}
 	} else {
-		gid, err = ParseID(filepath.Join(path, GroupFile), gidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
+		gid, err = ParseID(filepath.Join(groupPath, GroupFile), gidStr, func(line, str string, idInt int, idErr error) (uint32, bool) {
 			var gp gidParser
 			ParseString(line, &gp.group, &gp.placeholder, &gp.gid)
 			if (idErr == nil && idInt == gp.gid) || str == gp.group {
 				return uint32(gp.gid), true
 			}
 			return 0, false
 		})
-		if err != nil {
+	}
+	if err != nil {
+		// if gidStr is a integer, treat it as valid gid
+		integer, e := strconv.Atoi(gidStr)
+		if e != nil {
 			return 0, 0, err
 		}
+		gid = uint32(integer)
 	}
 
 	return uid, gid, nil
 }
 
 // GetIntegerID only parser user format uid:gid, cause container rootfs is not created
 // by contianerd now, can not change user to id, only support user id >= 1000
+// TODO(huamin.thm): removed later
 func GetIntegerID(user string) (uint32, uint32) {
 	if user == "" {
 		// return default user root
@@ -165,7 +185,7 @@ func ParseID(file, str string, parserFilter filterFunc) (uint32, error) {
 		}
 	}
 
-	return 0, fmt.Errorf("failed to find id or name %s in %s", str, file)
+	return 0, fmt.Errorf("failed to find %s in %s", str, file)
 }
 
 // isUnknownUser determines if id can be accepted as a unknown user. this kind of user id should >= 1000

pkg/utils/utils.go

@@ -460,3 +460,12 @@ func ConvertStrToKV(input string) (string, string, error) {
 	}
 	return results[0], results[1], nil
 }
+
+// IsFileExist checks if file is exits on host.
+func IsFileExist(file string) bool {
+	if _, err := os.Stat(file); err == nil {
+		return true
+	}
+
+	return false
+}

pkg/utils/utils_test.go

@@ -667,3 +667,58 @@ func TestConvertStrToKV(t *testing.T) {
 		})
 	}
 }
+
+func TestIsFileExist(t *testing.T) {
+	assert := assert.New(t)
+	tempDir, err := ioutil.TempDir("/tmp", "")
+	assert.NoError(err)
+	defer os.RemoveAll(tempDir)
+	existPath := make([]string, 0)
+	for _, v := range []string{
+		"a", "b", "c", "d", "e",
+	} {
+		path := filepath.Join(tempDir, v)
+		existPath = append(existPath, path)
+		os.Create(path)
+	}
+
+	for _, t := range []struct {
+		path  string
+		exist bool
+	}{
+		{
+			path:  existPath[0],
+			exist: true,
+		},
+		{
+			path:  existPath[1],
+			exist: true,
+		},
+		{
+			path:  existPath[2],
+			exist: true,
+		},
+		{
+			path:  existPath[3],
+			exist: true,
+		},
+		{
+			path:  existPath[4],
+			exist: true,
+		},
+		{
+			path:  filepath.Join(tempDir, "foo"),
+			exist: false,
+		},
+		{
+			path:  filepath.Join(tempDir, "bar"),
+			exist: false,
+		},
+		{
+			path:  filepath.Join(tempDir, "foo/bar"),
+			exist: false,
+		},
+	} {
+		assert.Equal(IsFileExist(t.path), t.exist)
+	}
+}

test/cli_run_user_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/alibaba/pouch/test/command"
@@ -32,23 +33,31 @@ func (suite *PouchRunUserSuite) TearDownTest(c *check.C) {
 
 // TestRunWithUser is to verify run container with user.
 func (suite *PouchRunUserSuite) TestRunWithUser(c *check.C) {
-	user := "1001"
-	name := "run-user"
+	namePrefix := "run-user"
 
-	{
+	for idx, user := range []string{
+		"1001", "1001:1001", "root", "root:root", "1001:root", "root:1001",
+	} {
+		name := fmt.Sprintf("%s-%d", namePrefix, idx)
 		res := command.PouchRun("run", "-d", "--name", name,
 			"--user", user, busyboxImage, "top")
-		defer DelContainerForceMultyTime(c, name)
 		res.Assert(c, icmd.Success)
+		DelContainerForceMultyTime(c, name)
 	}
+}
 
-	{
-		res := command.PouchRun("exec", name, "id", "-u")
-		res.Assert(c, icmd.Success)
-		if !strings.Contains(res.Stdout(), user) {
-			c.Fatalf("failed to run a container with user: %s",
-				res.Stdout())
-		}
+// TestRunWithUserFail is to verify run container with wrong user will fails.
+func (suite *PouchRunUserSuite) TestRunWithUserFail(c *check.C) {
+	namePrefix := "run-user-fail"
+
+	for idx, user := range []string{
+		"wrong-user", "wrong-user:wrong-group", "1001:wrong-group", "wrong-user:1001",
+	} {
+		name := fmt.Sprintf("%s-%d", namePrefix, idx)
+		res := command.PouchRun("run", "-d", "--name", name,
+			"--user", user, busyboxImage, "top")
+		c.Assert(res.Stderr(), check.NotNil)
+		DelContainerForceMultyTime(c, name)
 	}
 }