Skip to content

Commit

Permalink
oci: fix additional GIDs
Browse files Browse the repository at this point in the history 
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 <akihiro.suda.cz@hco.ntt.co.jp>
(cherry picked from commit 3eda46a)
> Conflicts:
>	integration/addition_gids_test.go
>	pkg/cri/sbserver/container_create_linux.go
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
  • Loading branch information
@AkihiroSuda
AkihiroSuda committed Feb 10, 2023
1 parent 3018234 commit 286a01f
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 54 deletions.
132 changes: 91 additions & 41 deletions integration/addition_gids_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package integration

import (
"fmt"
"os"
"path/filepath"
"testing"
Expand All @@ -31,49 +32,98 @@ import (
)

func TestAdditionalGids(t *testing.T) {
testPodLogDir, err := os.MkdirTemp("/tmp", "additional-gids")
require.NoError(t, err)
defer os.RemoveAll(testPodLogDir)
testImage := GetImage(BusyBox)
EnsureImageExists(t, testImage)
type testCase struct {
description string
opts []ContainerOpts
expected string
}

t.Log("Create a sandbox with log directory")
sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "additional-gids",
WithPodLogDirectory(testPodLogDir))
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",
},
}

var (
testImage = GetImage(BusyBox)
containerName = "test-container"
)
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)

EnsureImageExists(t, testImage)
testPodLogDir := t.TempDir()

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("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)

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), "groups=1(daemon),10(wheel),1234")
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")
})
}
}
39 changes: 39 additions & 0 deletions integration/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,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 { //nolint:unused
return func(c *runtime.ContainerConfig) {
Expand Down
22 changes: 22 additions & 0 deletions oci/spec_opts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,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.
//
Expand Down Expand Up @@ -522,7 +533,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
Expand Down Expand Up @@ -615,7 +628,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
Expand All @@ -628,7 +643,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)
Expand Down Expand Up @@ -674,7 +691,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 {
Expand Down Expand Up @@ -725,7 +744,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 {
Expand Down Expand Up @@ -796,6 +817,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
Expand Down
26 changes: 14 additions & 12 deletions oci/spec_opts_linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,23 +179,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 {
Expand Down Expand Up @@ -460,8 +460,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",
Expand All @@ -471,7 +472,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",
Expand All @@ -482,12 +483,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},
},
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/cri/server/container_create_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,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 {
Expand Down

0 comments on commit 286a01f

Please sign in to comment.