From 429752a69d4a224e0aa65e09a4f9c396ee3eaa70 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 30 Apr 2015 19:02:31 -0400 Subject: [PATCH 1/4] Lookup additional groups in the container. Signed-off-by: Mrunal Patel --- configs/config.go | 2 +- init_linux.go | 9 ++++++++- user/user.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/configs/config.go b/configs/config.go index 7275b642..04ea91ff 100644 --- a/configs/config.go +++ b/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"` diff --git a/init_linux.go b/init_linux.go index 3eabe3cd..521de51c 100644 --- a/init_linux.go +++ b/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 } diff --git a/user/user.go b/user/user.go index d7439f12..baff970c 100644 --- a/user/user.go +++ b/user/user.go @@ -348,3 +348,52 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( return user, nil } + +// GetAdditionalGroupsPath is a wrapper for GetAdditionalGroups. It reads data from the +// given file path and uses that data as the arguments to GetAdditionalGroups. +func GetAdditionalGroupsPath(additionalGroups []string, groupPath string) ([]int, error) { + var groupIds []int + + for _, ag := range additionalGroups { + groupReader, err := os.Open(groupPath) + if err != nil { + return nil, fmt.Errorf("Failed to open group file: %v", err) + } + defer groupReader.Close() + + groupId, err := GetAdditionalGroup(ag, groupReader) + if err != nil { + return nil, err + } + groupIds = append(groupIds, groupId) + } + + return groupIds, nil +} + +// GetAdditionalGroup looks up the specified group in the passed groupReader. +func GetAdditionalGroup(additionalGroup string, groupReader io.Reader) (int, error) { + groups, err := ParseGroupFilter(groupReader, func(g Group) bool { + return g.Name == additionalGroup || strconv.Itoa(g.Gid) == additionalGroup + }) + if err != nil { + return -1, fmt.Errorf("Unable to find additional groups %v: %v", additionalGroup, err) + } + if groups != nil && len(groups) > 0 { + // if we found any group entries that matched our filter, let's take the first one as "correct" + return groups[0].Gid, nil + } else { + // we asked for a group but didn't find id... let's check to see if we wanted a numeric group + addGroup, err := strconv.Atoi(additionalGroup) + if err != nil { + // not numeric - we have to bail + return -1, fmt.Errorf("Unable to find group %v", additionalGroup) + } + + // Ensure gid is inside gid range. + if addGroup < minId || addGroup > maxId { + return -1, ErrRange + } + return addGroup, nil + } +} From f28dff5539855bac2adbc5699f57f84349605b5f Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 30 Apr 2015 19:03:13 -0400 Subject: [PATCH 2/4] Add flags for specifying additional groups. Signed-off-by: Mrunal Patel --- nsinit/config.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/nsinit/config.go b/nsinit/config.go index 7fb28a58..58f71418 100644 --- a/nsinit/config.go +++ b/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", From 50603caabe7eecf9c6b0bd133f67e982f7a04a21 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Thu, 30 Apr 2015 19:27:52 -0400 Subject: [PATCH 3/4] Adds tests for Additional Groups. Signed-off-by: Mrunal Patel --- integration/exec_test.go | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/integration/exec_test.go b/integration/exec_test.go index 3b8a83b7..c218dbd6 100644 --- a/integration/exec_test.go +++ b/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) } From d4ece29c0bd38d680af6a2544a5ce705c3daeb24 Mon Sep 17 00:00:00 2001 From: "Daniel, Dao Quang Minh" Date: Mon, 25 May 2015 19:02:34 +0000 Subject: [PATCH 4/4] refactor GetAdditionalGroupsPath This parses group file only once to process a list of groups instead of parsing once for each group. Also added an unit test for GetAdditionalGroupsPath Signed-off-by: Daniel, Dao Quang Minh --- user/user.go | 82 +++++++++++++++++++++++------------------- user/user_test.go | 91 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 37 deletions(-) diff --git a/user/user.go b/user/user.go index baff970c..13226dbf 100644 --- a/user/user.go +++ b/user/user.go @@ -349,51 +349,59 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( return user, nil } -// GetAdditionalGroupsPath is a wrapper for GetAdditionalGroups. It reads data from the -// given file path and uses that data as the arguments to GetAdditionalGroups. +// 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) { - var groupIds []int - - for _, ag := range additionalGroups { - groupReader, err := os.Open(groupPath) - if err != nil { - return nil, fmt.Errorf("Failed to open group file: %v", err) - } - defer groupReader.Close() - - groupId, err := GetAdditionalGroup(ag, groupReader) - if err != nil { - return nil, err - } - groupIds = append(groupIds, groupId) + groupReader, err := os.Open(groupPath) + if err != nil { + return nil, fmt.Errorf("Failed to open group file: %v", err) } + defer groupReader.Close() - return groupIds, nil -} - -// GetAdditionalGroup looks up the specified group in the passed groupReader. -func GetAdditionalGroup(additionalGroup string, groupReader io.Reader) (int, error) { groups, err := ParseGroupFilter(groupReader, func(g Group) bool { - return g.Name == additionalGroup || strconv.Itoa(g.Gid) == additionalGroup + for _, ag := range additionalGroups { + if g.Name == ag || strconv.Itoa(g.Gid) == ag { + return true + } + } + return false }) if err != nil { - return -1, fmt.Errorf("Unable to find additional groups %v: %v", additionalGroup, err) + return nil, fmt.Errorf("Unable to find additional groups %v: %v", additionalGroups, err) } - if groups != nil && len(groups) > 0 { - // if we found any group entries that matched our filter, let's take the first one as "correct" - return groups[0].Gid, nil - } else { - // we asked for a group but didn't find id... let's check to see if we wanted a numeric group - addGroup, err := strconv.Atoi(additionalGroup) - if err != nil { - // not numeric - we have to bail - return -1, fmt.Errorf("Unable to find group %v", additionalGroup) - } - // Ensure gid is inside gid range. - if addGroup < minId || addGroup > maxId { - return -1, ErrRange + 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{}{} } - return addGroup, nil } + gids := []int{} + for gid := range gidMap { + gids = append(gids, gid) + } + return gids, nil } diff --git a/user/user_test.go b/user/user_test.go index 4fe008fb..ffb0760e 100644 --- a/user/user_test.go +++ b/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) + } + } +}