Merge pull request #603 from dqminh/mrunalp-add_groups_lookup

Rebased: Additional groups lookup

configs/config.go

@@ -119,7 +119,7 @@ type Config struct {
 
 	// AdditionalGroups specifies the gids that should be added to supplementary groups
 	// in addition to those that the user belongs to.
-	AdditionalGroups []int `json:"additional_groups"`
+	AdditionalGroups []string `json:"additional_groups"`
 
 	// UidMappings is an array of User ID mappings for User Namespaces
 	UidMappings []IDMap `json:"uid_mappings"`

init_linux.go

@@ -177,10 +177,17 @@ func setupUser(config *initConfig) error {
 	if err != nil {
 		return err
 	}
-	suppGroups := append(execUser.Sgids, config.Config.AdditionalGroups...)
+
+	addGroups, err := user.GetAdditionalGroupsPath(config.Config.AdditionalGroups, groupPath)
+	if err != nil {
+		return err
+	}
+
+	suppGroups := append(execUser.Sgids, addGroups...)
 	if err := syscall.Setgroups(suppGroups); err != nil {
 		return err
 	}
+
 	if err := system.Setgid(execUser.Gid); err != nil {
 		return err
 	}

integration/exec_test.go

@@ -386,6 +386,53 @@ func TestProcessCaps(t *testing.T) {
 	}
 }
 
+func TestAdditionalGroups(t *testing.T) {
+	if testing.Short() {
+		return
+	}
+	root, err := newTestRoot()
+	ok(t, err)
+	defer os.RemoveAll(root)
+
+	rootfs, err := newRootfs()
+	ok(t, err)
+	defer remove(rootfs)
+
+	config := newTemplateConfig(rootfs)
+	config.AdditionalGroups = []string{"plugdev", "audio"}
+
+	factory, err := libcontainer.New(root, libcontainer.Cgroupfs)
+	ok(t, err)
+
+	container, err := factory.Create("test", config)
+	ok(t, err)
+	defer container.Destroy()
+
+	var stdout bytes.Buffer
+	pconfig := libcontainer.Process{
+		Args:   []string{"sh", "-c", "id", "-Gn"},
+		Env:    standardEnvironment,
+		Stdin:  nil,
+		Stdout: &stdout,
+	}
+	err = container.Start(&pconfig)
+	ok(t, err)
+
+	// Wait for process
+	waitProcess(&pconfig, t)
+
+	outputGroups := string(stdout.Bytes())
+
+	// Check that the groups output has the groups that we specified
+	if !strings.Contains(outputGroups, "audio") {
+		t.Fatalf("Listed groups do not contain the audio group as expected: %v", outputGroups)
+	}
+
+	if !strings.Contains(outputGroups, "plugdev") {
+		t.Fatalf("Listed groups do not contain the plugdev group as expected: %v", outputGroups)
+	}
+}
+
 func TestFreeze(t *testing.T) {
 	testFreeze(t, false)
 }

nsinit/config.go

@@ -46,6 +46,7 @@ var createFlags = []cli.Flag{
 	cli.StringSliceFlag{Name: "bind", Value: &cli.StringSlice{}, Usage: "add bind mounts to the container"},
 	cli.StringSliceFlag{Name: "sysctl", Value: &cli.StringSlice{}, Usage: "set system properties in the container"},
 	cli.StringSliceFlag{Name: "tmpfs", Value: &cli.StringSlice{}, Usage: "add tmpfs mounts to the container"},
+	cli.StringSliceFlag{Name: "groups", Value: &cli.StringSlice{}, Usage: "add additional groups"},
 }
 
 var configCommand = cli.Command{
@@ -113,6 +114,7 @@ func modify(config *configs.Config, context *cli.Context) {
 			node.Gid = uint32(userns_uid)
 		}
 	}
+
 	config.SystemProperties = make(map[string]string)
 	for _, sysProp := range context.StringSlice("sysctl") {
 		parts := strings.SplitN(sysProp, "=", 2)
@@ -121,6 +123,11 @@ func modify(config *configs.Config, context *cli.Context) {
 		}
 		config.SystemProperties[parts[0]] = parts[1]
 	}
+
+	for _, group := range context.StringSlice("groups") {
+		config.AdditionalGroups = append(config.AdditionalGroups, group)
+	}
+
 	for _, rawBind := range context.StringSlice("bind") {
 		mount := &configs.Mount{
 			Device: "bind",

user/user.go

@@ -348,3 +348,60 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 
 	return user, nil
 }
+
+// GetAdditionalGroupsPath looks up a list of groups by name or group id
+// against the group file. If a group name cannot be found, an error will be
+// returned. If a group id cannot be found, it will be returned as-is.
+func GetAdditionalGroupsPath(additionalGroups []string, groupPath string) ([]int, error) {
+	groupReader, err := os.Open(groupPath)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to open group file: %v", err)
+	}
+	defer groupReader.Close()
+
+	groups, err := ParseGroupFilter(groupReader, func(g Group) bool {
+		for _, ag := range additionalGroups {
+			if g.Name == ag || strconv.Itoa(g.Gid) == ag {
+				return true
+			}
+		}
+		return false
+	})
+	if err != nil {
+		return nil, fmt.Errorf("Unable to find additional groups %v: %v", additionalGroups, err)
+	}
+
+	gidMap := make(map[int]struct{})
+	for _, ag := range additionalGroups {
+		var found bool
+		for _, g := range groups {
+			// if we found a matched group either by name or gid, take the
+			// first matched as correct
+			if g.Name == ag || strconv.Itoa(g.Gid) == ag {
+				if _, ok := gidMap[g.Gid]; !ok {
+					gidMap[g.Gid] = struct{}{}
+					found = true
+					break
+				}
+			}
+		}
+		// we asked for a group but didn't find it. let's check to see
+		// if we wanted a numeric group
+		if !found {
+			gid, err := strconv.Atoi(ag)
+			if err != nil {
+				return nil, fmt.Errorf("Unable to find group %s", ag)
+			}
+			// Ensure gid is inside gid range.
+			if gid < minId || gid > maxId {
+				return nil, ErrRange
+			}
+			gidMap[gid] = struct{}{}
+		}
+	}
+	gids := []int{}
+	for gid := range gidMap {
+		gids = append(gids, gid)
+	}
+	return gids, nil
+}

user/user_test.go

@@ -1,8 +1,12 @@
 package user
 
 import (
+	"fmt"
 	"io"
+	"io/ioutil"
 	"reflect"
+	"sort"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -350,3 +354,90 @@ this is just some garbage data
 		}
 	}
 }
+
+func TestGetAdditionalGroupsPath(t *testing.T) {
+	const groupContent = `
+root:x:0:root
+adm:x:43:
+grp:x:1234:root,adm
+adm:x:4343:root,adm-duplicate
+this is just some garbage data
+`
+	tests := []struct {
+		groups   []string
+		expected []int
+		hasError bool
+	}{
+		{
+			// empty group
+			groups:   []string{},
+			expected: []int{},
+		},
+		{
+			// single group
+			groups:   []string{"adm"},
+			expected: []int{43},
+		},
+		{
+			// multiple groups
+			groups:   []string{"adm", "grp"},
+			expected: []int{43, 1234},
+		},
+		{
+			// invalid group
+			groups:   []string{"adm", "grp", "not-exist"},
+			expected: nil,
+			hasError: true,
+		},
+		{
+			// group with numeric id
+			groups:   []string{"43"},
+			expected: []int{43},
+		},
+		{
+			// group with unknown numeric id
+			groups:   []string{"adm", "10001"},
+			expected: []int{43, 10001},
+		},
+		{
+			// groups specified twice with numeric and name
+			groups:   []string{"adm", "43"},
+			expected: []int{43},
+		},
+		{
+			// groups with too small id
+			groups:   []string{"-1"},
+			expected: nil,
+			hasError: true,
+		},
+		{
+			// groups with too large id
+			groups:   []string{strconv.Itoa(1 << 31)},
+			expected: nil,
+			hasError: true,
+		},
+	}
+
+	for _, test := range tests {
+		tmpFile, err := ioutil.TempFile("", "get-additional-groups-path")
+		if err != nil {
+			t.Error(err)
+		}
+		fmt.Fprint(tmpFile, groupContent)
+		tmpFile.Close()
+
+		gids, err := GetAdditionalGroupsPath(test.groups, tmpFile.Name())
+		if test.hasError && err == nil {
+			t.Errorf("Parse(%#v) expects error but has none", test)
+			continue
+		}
+		if !test.hasError && err != nil {
+			t.Errorf("Parse(%#v) has error %v", test, err)
+			continue
+		}
+		sort.Sort(sort.IntSlice(gids))
+		if !reflect.DeepEqual(gids, test.expected) {
+			t.Errorf("Gids(%v), expect %v from groups %v", gids, test.expected, test.groups)
+		}
+	}
+}