From 3eda46af12b1deedab3d0802adb2e81cb3521950 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Sat, 24 Dec 2022 20:09:04 +0900 Subject: [PATCH] oci: fix additional GIDs Test suite: ```yaml --- apiVersion: v1 kind: Pod metadata: name: test-no-option annotations: description: "Equivalent of `docker run` (no option)" spec: restartPolicy: Never containers: - name: main image: ghcr.io/containerd/busybox:1.28 args: ['sh', '-euxc', '[ "$(id)" = "uid=0(root) gid=0(root) groups=0(root),10(wheel)" ]'] --- apiVersion: v1 kind: Pod metadata: name: test-group-add-1-group-add-1234 annotations: description: "Equivalent of `docker run --group-add 1 --group-add 1234`" spec: restartPolicy: Never containers: - name: main image: ghcr.io/containerd/busybox:1.28 args: ['sh', '-euxc', '[ "$(id)" = "uid=0(root) gid=0(root) groups=0(root),1(daemon),10(wheel),1234" ]'] securityContext: supplementalGroups: [1, 1234] --- apiVersion: v1 kind: Pod metadata: name: test-user-1234 annotations: description: "Equivalent of `docker run --user 1234`" spec: restartPolicy: Never containers: - name: main image: ghcr.io/containerd/busybox:1.28 args: ['sh', '-euxc', '[ "$(id)" = "uid=1234 gid=0(root) groups=0(root)" ]'] securityContext: runAsUser: 1234 --- apiVersion: v1 kind: Pod metadata: name: test-user-1234-1234 annotations: description: "Equivalent of `docker run --user 1234:1234`" spec: restartPolicy: Never containers: - name: main image: ghcr.io/containerd/busybox:1.28 args: ['sh', '-euxc', '[ "$(id)" = "uid=1234 gid=1234 groups=1234" ]'] securityContext: runAsUser: 1234 runAsGroup: 1234 --- apiVersion: v1 kind: Pod metadata: name: test-user-1234-group-add-1234 annotations: description: "Equivalent of `docker run --user 1234 --group-add 1234`" spec: restartPolicy: Never containers: - name: main image: ghcr.io/containerd/busybox:1.28 args: ['sh', '-euxc', '[ "$(id)" = "uid=1234 gid=0(root) groups=0(root),1234" ]'] securityContext: runAsUser: 1234 supplementalGroups: [1234] ``` Signed-off-by: Akihiro Suda --- integration/addition_gids_test.go | 128 +++++++++++++++------ integration/main_test.go | 39 +++++++ oci/spec_opts.go | 22 ++++ oci/spec_opts_linux_test.go | 26 +++-- pkg/cri/sbserver/container_create_linux.go | 3 +- pkg/cri/server/container_create_linux.go | 3 +- 6 files changed, 169 insertions(+), 52 deletions(-) diff --git a/integration/addition_gids_test.go b/integration/addition_gids_test.go index d57fe0984..b3ddf9875 100644 --- a/integration/addition_gids_test.go +++ b/integration/addition_gids_test.go @@ -19,6 +19,7 @@ package integration import ( + "fmt" "os" "path/filepath" "testing" @@ -32,47 +33,98 @@ import ( ) func TestAdditionalGids(t *testing.T) { - testPodLogDir := t.TempDir() - - t.Log("Create a sandbox with log directory") - sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "additional-gids", - WithPodLogDirectory(testPodLogDir)) - - var ( - testImage = images.Get(images.BusyBox) - containerName = "test-container" - ) - + testImage := images.Get(images.BusyBox) EnsureImageExists(t, testImage) + type testCase struct { + description string + opts []ContainerOpts + expected string + } - t.Log("Create a container to print id") - cnConfig := ContainerConfig( - containerName, - testImage, - WithCommand("id"), - WithLogPath(containerName), - WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/}), - ) - cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig) - require.NoError(t, err) + testCases := []testCase{ + { + description: "Equivalent of `docker run` (no option)", + opts: nil, + expected: "groups=0(root),10(wheel)", + }, + { + description: "Equivalent of `docker run --group-add 1 --group-add 1234`", + opts: []ContainerOpts{WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/})}, + expected: "groups=0(root),1(daemon),10(wheel),1234", + }, + { + description: "Equivalent of `docker run --user 1234`", + opts: []ContainerOpts{WithRunAsUser(1234)}, + expected: "groups=0(root)", + }, + { + description: "Equivalent of `docker run --user 1234:1234`", + opts: []ContainerOpts{WithRunAsUser(1234), WithRunAsGroup(1234)}, + expected: "groups=1234", + }, + { + description: "Equivalent of `docker run --user 1234 --group-add 1234`", + opts: []ContainerOpts{WithRunAsUser(1234), WithSupplementalGroups([]int64{1234})}, + expected: "groups=0(root),1234", + }, + { + description: "Equivalent of `docker run --user daemon` (Supported by CRI, although unsupported by kube-apiserver)", + opts: []ContainerOpts{WithRunAsUsername("daemon")}, + expected: "groups=1(daemon)", + }, + { + description: "Equivalent of `docker run --user daemon --group-add 1234` (Supported by CRI, although unsupported by kube-apiserver)", + opts: []ContainerOpts{WithRunAsUsername("daemon"), WithSupplementalGroups([]int64{1234})}, + expected: "groups=1(daemon),1234", + }, + } - t.Log("Start the container") - require.NoError(t, runtimeService.StartContainer(cn)) + for i, tc := range testCases { + i, tc := i, tc + tBasename := fmt.Sprintf("case-%d", i) + t.Run(tBasename, func(t *testing.T) { + t.Log(tc.description) + t.Logf("Expected=%q", tc.expected) - t.Log("Wait for container to finish running") - require.NoError(t, Eventually(func() (bool, error) { - s, err := runtimeService.ContainerStatus(cn) - if err != nil { - return false, err - } - if s.GetState() == runtime.ContainerState_CONTAINER_EXITED { - return true, nil - } - return false, nil - }, time.Second, 30*time.Second)) + testPodLogDir := t.TempDir() - t.Log("Search additional groups in container log") - content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName)) - assert.NoError(t, err) - assert.Contains(t, string(content), "groups=1(daemon),10(wheel),1234") + t.Log("Create a sandbox with log directory") + sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", tBasename, + WithPodLogDirectory(testPodLogDir)) + + t.Log("Create a container to print id") + containerName := tBasename + cnConfig := ContainerConfig( + containerName, + testImage, + append( + []ContainerOpts{ + WithCommand("id"), + WithLogPath(containerName), + }, tc.opts...)..., + ) + cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig) + require.NoError(t, err) + + t.Log("Start the container") + require.NoError(t, runtimeService.StartContainer(cn)) + + t.Log("Wait for container to finish running") + require.NoError(t, Eventually(func() (bool, error) { + s, err := runtimeService.ContainerStatus(cn) + if err != nil { + return false, err + } + if s.GetState() == runtime.ContainerState_CONTAINER_EXITED { + return true, nil + } + return false, nil + }, time.Second, 30*time.Second)) + + t.Log("Search additional groups in container log") + content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName)) + assert.NoError(t, err) + assert.Contains(t, string(content), tc.expected+"\n") + }) + } } diff --git a/integration/main_test.go b/integration/main_test.go index 32432ab9a..5e86ec934 100644 --- a/integration/main_test.go +++ b/integration/main_test.go @@ -384,6 +384,45 @@ func WithLogPath(path string) ContainerOpts { } } +// WithRunAsUser sets the uid. +func WithRunAsUser(uid int64) ContainerOpts { + return func(c *runtime.ContainerConfig) { + if c.Linux == nil { + c.Linux = &runtime.LinuxContainerConfig{} + } + if c.Linux.SecurityContext == nil { + c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{} + } + c.Linux.SecurityContext.RunAsUser = &runtime.Int64Value{Value: uid} + } +} + +// WithRunAsUsername sets the username. +func WithRunAsUsername(username string) ContainerOpts { + return func(c *runtime.ContainerConfig) { + if c.Linux == nil { + c.Linux = &runtime.LinuxContainerConfig{} + } + if c.Linux.SecurityContext == nil { + c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{} + } + c.Linux.SecurityContext.RunAsUsername = username + } +} + +// WithRunAsGroup sets the gid. +func WithRunAsGroup(gid int64) ContainerOpts { + return func(c *runtime.ContainerConfig) { + if c.Linux == nil { + c.Linux = &runtime.LinuxContainerConfig{} + } + if c.Linux.SecurityContext == nil { + c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{} + } + c.Linux.SecurityContext.RunAsGroup = &runtime.Int64Value{Value: gid} + } +} + // WithSupplementalGroups adds supplemental groups. func WithSupplementalGroups(gids []int64) ContainerOpts { return func(c *runtime.ContainerConfig) { diff --git a/oci/spec_opts.go b/oci/spec_opts.go index 18f653347..00942995b 100644 --- a/oci/spec_opts.go +++ b/oci/spec_opts.go @@ -121,6 +121,17 @@ func setCapabilities(s *Spec) { } } +// ensureAdditionalGids ensures that the primary GID is also included in the additional GID list. +func ensureAdditionalGids(s *Spec) { + setProcess(s) + for _, f := range s.Process.User.AdditionalGids { + if f == s.Process.User.GID { + return + } + } + s.Process.User.AdditionalGids = append([]uint32{s.Process.User.GID}, s.Process.User.AdditionalGids...) +} + // WithDefaultSpec returns a SpecOpts that will populate the spec with default // values. // @@ -586,7 +597,9 @@ func WithNamespacedCgroup() SpecOpts { // user, uid, user:group, uid:gid, uid:group, user:gid func WithUser(userstr string) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) error { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil // For LCOW it's a bit harder to confirm that the user actually exists on the host as a rootfs isn't // mounted on the host and shared into the guest, but rather the rootfs is constructed entirely in the @@ -679,7 +692,9 @@ func WithUser(userstr string) SpecOpts { // WithUIDGID allows the UID and GID for the Process to be set func WithUIDGID(uid, gid uint32) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil s.Process.User.UID = uid s.Process.User.GID = gid return nil @@ -692,7 +707,9 @@ func WithUIDGID(uid, gid uint32) SpecOpts { // additionally sets the gid to 0, and does not return an error. func WithUserID(uid uint32) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil setUser := func(root string) error { user, err := UserFromPath(root, func(u user.User) bool { return u.Uid == int(uid) @@ -738,7 +755,9 @@ func WithUserID(uid uint32) SpecOpts { // the container. func WithUsername(username string) SpecOpts { return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) { + defer ensureAdditionalGids(s) setProcess(s) + s.Process.User.AdditionalGids = nil if s.Linux != nil { setUser := func(root string) error { user, err := UserFromPath(root, func(u user.User) bool { @@ -789,7 +808,9 @@ func WithAdditionalGIDs(userstr string) SpecOpts { return nil } setProcess(s) + s.Process.User.AdditionalGids = nil setAdditionalGids := func(root string) error { + defer ensureAdditionalGids(s) var username string uid, err := strconv.Atoi(userstr) if err == nil { @@ -860,6 +881,7 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts { } setProcess(s) setAdditionalGids := func(root string) error { + defer ensureAdditionalGids(s) gpath, err := fs.RootPath(root, "/etc/group") if err != nil { return err diff --git a/oci/spec_opts_linux_test.go b/oci/spec_opts_linux_test.go index 9fac5d1f3..b340c72e6 100644 --- a/oci/spec_opts_linux_test.go +++ b/oci/spec_opts_linux_test.go @@ -180,23 +180,23 @@ sys:x:3:root,bin,adm }{ { user: "root", - expected: []uint32{1, 2, 3}, + expected: []uint32{0, 1, 2, 3}, }, { user: "1000", - expected: nil, + expected: []uint32{0}, }, { user: "bin", - expected: []uint32{2, 3}, + expected: []uint32{0, 2, 3}, }, { user: "bin:root", - expected: []uint32{}, + expected: []uint32{0}, }, { user: "daemon", - expected: []uint32{1}, + expected: []uint32{0, 1}, }, } for _, testCase := range testCases { @@ -461,8 +461,9 @@ daemon:x:2:root,bin,daemon err string }{ { - name: "no additional gids", - groups: []string{}, + name: "no additional gids", + groups: []string{}, + expected: []uint32{0}, }, { name: "no additional gids, append root gid", @@ -472,7 +473,7 @@ daemon:x:2:root,bin,daemon { name: "no additional gids, append bin and daemon gids", groups: []string{"bin", "daemon"}, - expected: []uint32{1, 2}, + expected: []uint32{0, 1, 2}, }, { name: "has root additional gids, append bin and daemon gids", @@ -483,12 +484,13 @@ daemon:x:2:root,bin,daemon { name: "append group id", groups: []string{"999"}, - expected: []uint32{999}, + expected: []uint32{0, 999}, }, { - name: "unknown group", - groups: []string{"unknown"}, - err: "unable to find group unknown", + name: "unknown group", + groups: []string{"unknown"}, + err: "unable to find group unknown", + expected: []uint32{0}, }, } diff --git a/pkg/cri/sbserver/container_create_linux.go b/pkg/cri/sbserver/container_create_linux.go index ba39c1219..76b4319c7 100644 --- a/pkg/cri/sbserver/container_create_linux.go +++ b/pkg/cri/sbserver/container_create_linux.go @@ -137,7 +137,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon // Because it is still useful to get additional gids for uid 0. userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10) } - specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr)) + specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr), + customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups())) asp := securityContext.GetApparmor() if asp == nil { diff --git a/pkg/cri/server/container_create_linux.go b/pkg/cri/server/container_create_linux.go index 05a2f05d2..17a26423c 100644 --- a/pkg/cri/server/container_create_linux.go +++ b/pkg/cri/server/container_create_linux.go @@ -376,7 +376,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon // Because it is still useful to get additional gids for uid 0. userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10) } - specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr)) + specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr), + customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups())) asp := securityContext.GetApparmor() if asp == nil {