Skip to content

Commit 3eda46a

Browse files
AkihiroSudaAkihiroSuda
committed
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 <[email protected]>
1 parent ef2560d commit 3eda46a

6 files changed

Lines changed: 173 additions & 56 deletions

File tree

‎integration/addition_gids_test.go‎

Lines changed: 94 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
package integration
2020

2121
import (
22+
"fmt"
2223
"os"
2324
"path/filepath"
2425
"testing"
@@ -32,47 +33,98 @@ import (
3233
)
3334

3435
func TestAdditionalGids(t *testing.T) {
35-
testPodLogDir := t.TempDir()
36-
37-
t.Log("Create a sandbox with log directory")
38-
sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "additional-gids",
39-
WithPodLogDirectory(testPodLogDir))
40-
41-
var (
42-
testImage = images.Get(images.BusyBox)
43-
containerName = "test-container"
44-
)
45-
36+
testImage := images.Get(images.BusyBox)
4637
EnsureImageExists(t, testImage)
47-
48-
t.Log("Create a container to print id")
49-
cnConfig := ContainerConfig(
50-
containerName,
51-
testImage,
52-
WithCommand("id"),
53-
WithLogPath(containerName),
54-
WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/}),
55-
)
56-
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
57-
require.NoError(t, err)
58-
59-
t.Log("Start the container")
60-
require.NoError(t, runtimeService.StartContainer(cn))
61-
62-
t.Log("Wait for container to finish running")
63-
require.NoError(t, Eventually(func() (bool, error) {
64-
s, err := runtimeService.ContainerStatus(cn)
65-
if err != nil {
66-
return false, err
67-
}
68-
if s.GetState() == runtime.ContainerState_CONTAINER_EXITED {
69-
return true, nil
70-
}
71-
return false, nil
72-
}, time.Second, 30*time.Second))
73-
74-
t.Log("Search additional groups in container log")
75-
content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName))
76-
assert.NoError(t, err)
77-
assert.Contains(t, string(content), "groups=1(daemon),10(wheel),1234")
38+
type testCase struct {
39+
description string
40+
opts []ContainerOpts
41+
expected string
42+
}
43+
44+
testCases := []testCase{
45+
{
46+
description: "Equivalent of `docker run` (no option)",
47+
opts: nil,
48+
expected: "groups=0(root),10(wheel)",
49+
},
50+
{
51+
description: "Equivalent of `docker run --group-add 1 --group-add 1234`",
52+
opts: []ContainerOpts{WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/})},
53+
expected: "groups=0(root),1(daemon),10(wheel),1234",
54+
},
55+
{
56+
description: "Equivalent of `docker run --user 1234`",
57+
opts: []ContainerOpts{WithRunAsUser(1234)},
58+
expected: "groups=0(root)",
59+
},
60+
{
61+
description: "Equivalent of `docker run --user 1234:1234`",
62+
opts: []ContainerOpts{WithRunAsUser(1234), WithRunAsGroup(1234)},
63+
expected: "groups=1234",
64+
},
65+
{
66+
description: "Equivalent of `docker run --user 1234 --group-add 1234`",
67+
opts: []ContainerOpts{WithRunAsUser(1234), WithSupplementalGroups([]int64{1234})},
68+
expected: "groups=0(root),1234",
69+
},
70+
{
71+
description: "Equivalent of `docker run --user daemon` (Supported by CRI, although unsupported by kube-apiserver)",
72+
opts: []ContainerOpts{WithRunAsUsername("daemon")},
73+
expected: "groups=1(daemon)",
74+
},
75+
{
76+
description: "Equivalent of `docker run --user daemon --group-add 1234` (Supported by CRI, although unsupported by kube-apiserver)",
77+
opts: []ContainerOpts{WithRunAsUsername("daemon"), WithSupplementalGroups([]int64{1234})},
78+
expected: "groups=1(daemon),1234",
79+
},
80+
}
81+
82+
for i, tc := range testCases {
83+
i, tc := i, tc
84+
tBasename := fmt.Sprintf("case-%d", i)
85+
t.Run(tBasename, func(t *testing.T) {
86+
t.Log(tc.description)
87+
t.Logf("Expected=%q", tc.expected)
88+
89+
testPodLogDir := t.TempDir()
90+
91+
t.Log("Create a sandbox with log directory")
92+
sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", tBasename,
93+
WithPodLogDirectory(testPodLogDir))
94+
95+
t.Log("Create a container to print id")
96+
containerName := tBasename
97+
cnConfig := ContainerConfig(
98+
containerName,
99+
testImage,
100+
append(
101+
[]ContainerOpts{
102+
WithCommand("id"),
103+
WithLogPath(containerName),
104+
}, tc.opts...)...,
105+
)
106+
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
107+
require.NoError(t, err)
108+
109+
t.Log("Start the container")
110+
require.NoError(t, runtimeService.StartContainer(cn))
111+
112+
t.Log("Wait for container to finish running")
113+
require.NoError(t, Eventually(func() (bool, error) {
114+
s, err := runtimeService.ContainerStatus(cn)
115+
if err != nil {
116+
return false, err
117+
}
118+
if s.GetState() == runtime.ContainerState_CONTAINER_EXITED {
119+
return true, nil
120+
}
121+
return false, nil
122+
}, time.Second, 30*time.Second))
123+
124+
t.Log("Search additional groups in container log")
125+
content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName))
126+
assert.NoError(t, err)
127+
assert.Contains(t, string(content), tc.expected+"\n")
128+
})
129+
}
78130
}

‎integration/main_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,45 @@ func WithLogPath(path string) ContainerOpts {
384384
}
385385
}
386386

387+
// WithRunAsUser sets the uid.
388+
func WithRunAsUser(uid int64) ContainerOpts {
389+
return func(c *runtime.ContainerConfig) {
390+
if c.Linux == nil {
391+
c.Linux = &runtime.LinuxContainerConfig{}
392+
}
393+
if c.Linux.SecurityContext == nil {
394+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
395+
}
396+
c.Linux.SecurityContext.RunAsUser = &runtime.Int64Value{Value: uid}
397+
}
398+
}
399+
400+
// WithRunAsUsername sets the username.
401+
func WithRunAsUsername(username string) ContainerOpts {
402+
return func(c *runtime.ContainerConfig) {
403+
if c.Linux == nil {
404+
c.Linux = &runtime.LinuxContainerConfig{}
405+
}
406+
if c.Linux.SecurityContext == nil {
407+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
408+
}
409+
c.Linux.SecurityContext.RunAsUsername = username
410+
}
411+
}
412+
413+
// WithRunAsGroup sets the gid.
414+
func WithRunAsGroup(gid int64) ContainerOpts {
415+
return func(c *runtime.ContainerConfig) {
416+
if c.Linux == nil {
417+
c.Linux = &runtime.LinuxContainerConfig{}
418+
}
419+
if c.Linux.SecurityContext == nil {
420+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
421+
}
422+
c.Linux.SecurityContext.RunAsGroup = &runtime.Int64Value{Value: gid}
423+
}
424+
}
425+
387426
// WithSupplementalGroups adds supplemental groups.
388427
func WithSupplementalGroups(gids []int64) ContainerOpts {
389428
return func(c *runtime.ContainerConfig) {

‎oci/spec_opts.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,17 @@ func setCapabilities(s *Spec) {
121121
}
122122
}
123123

124+
// ensureAdditionalGids ensures that the primary GID is also included in the additional GID list.
125+
func ensureAdditionalGids(s *Spec) {
126+
setProcess(s)
127+
for _, f := range s.Process.User.AdditionalGids {
128+
if f == s.Process.User.GID {
129+
return
130+
}
131+
}
132+
s.Process.User.AdditionalGids = append([]uint32{s.Process.User.GID}, s.Process.User.AdditionalGids...)
133+
}
134+
124135
// WithDefaultSpec returns a SpecOpts that will populate the spec with default
125136
// values.
126137
//
@@ -586,7 +597,9 @@ func WithNamespacedCgroup() SpecOpts {
586597
// user, uid, user:group, uid:gid, uid:group, user:gid
587598
func WithUser(userstr string) SpecOpts {
588599
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) error {
600+
defer ensureAdditionalGids(s)
589601
setProcess(s)
602+
s.Process.User.AdditionalGids = nil
590603

591604
// For LCOW it's a bit harder to confirm that the user actually exists on the host as a rootfs isn't
592605
// 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 {
679692
// WithUIDGID allows the UID and GID for the Process to be set
680693
func WithUIDGID(uid, gid uint32) SpecOpts {
681694
return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
695+
defer ensureAdditionalGids(s)
682696
setProcess(s)
697+
s.Process.User.AdditionalGids = nil
683698
s.Process.User.UID = uid
684699
s.Process.User.GID = gid
685700
return nil
@@ -692,7 +707,9 @@ func WithUIDGID(uid, gid uint32) SpecOpts {
692707
// additionally sets the gid to 0, and does not return an error.
693708
func WithUserID(uid uint32) SpecOpts {
694709
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
710+
defer ensureAdditionalGids(s)
695711
setProcess(s)
712+
s.Process.User.AdditionalGids = nil
696713
setUser := func(root string) error {
697714
user, err := UserFromPath(root, func(u user.User) bool {
698715
return u.Uid == int(uid)
@@ -738,7 +755,9 @@ func WithUserID(uid uint32) SpecOpts {
738755
// the container.
739756
func WithUsername(username string) SpecOpts {
740757
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
758+
defer ensureAdditionalGids(s)
741759
setProcess(s)
760+
s.Process.User.AdditionalGids = nil
742761
if s.Linux != nil {
743762
setUser := func(root string) error {
744763
user, err := UserFromPath(root, func(u user.User) bool {
@@ -789,7 +808,9 @@ func WithAdditionalGIDs(userstr string) SpecOpts {
789808
return nil
790809
}
791810
setProcess(s)
811+
s.Process.User.AdditionalGids = nil
792812
setAdditionalGids := func(root string) error {
813+
defer ensureAdditionalGids(s)
793814
var username string
794815
uid, err := strconv.Atoi(userstr)
795816
if err == nil {
@@ -860,6 +881,7 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
860881
}
861882
setProcess(s)
862883
setAdditionalGids := func(root string) error {
884+
defer ensureAdditionalGids(s)
863885
gpath, err := fs.RootPath(root, "/etc/group")
864886
if err != nil {
865887
return err

‎oci/spec_opts_linux_test.go‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -180,23 +180,23 @@ sys:x:3:root,bin,adm
180180
}{
181181
{
182182
user: "root",
183-
expected: []uint32{1, 2, 3},
183+
expected: []uint32{0, 1, 2, 3},
184184
},
185185
{
186186
user: "1000",
187-
expected: nil,
187+
expected: []uint32{0},
188188
},
189189
{
190190
user: "bin",
191-
expected: []uint32{2, 3},
191+
expected: []uint32{0, 2, 3},
192192
},
193193
{
194194
user: "bin:root",
195-
expected: []uint32{},
195+
expected: []uint32{0},
196196
},
197197
{
198198
user: "daemon",
199-
expected: []uint32{1},
199+
expected: []uint32{0, 1},
200200
},
201201
}
202202
for _, testCase := range testCases {
@@ -461,8 +461,9 @@ daemon:x:2:root,bin,daemon
461461
err string
462462
}{
463463
{
464-
name: "no additional gids",
465-
groups: []string{},
464+
name: "no additional gids",
465+
groups: []string{},
466+
expected: []uint32{0},
466467
},
467468
{
468469
name: "no additional gids, append root gid",
@@ -472,7 +473,7 @@ daemon:x:2:root,bin,daemon
472473
{
473474
name: "no additional gids, append bin and daemon gids",
474475
groups: []string{"bin", "daemon"},
475-
expected: []uint32{1, 2},
476+
expected: []uint32{0, 1, 2},
476477
},
477478
{
478479
name: "has root additional gids, append bin and daemon gids",
@@ -483,12 +484,13 @@ daemon:x:2:root,bin,daemon
483484
{
484485
name: "append group id",
485486
groups: []string{"999"},
486-
expected: []uint32{999},
487+
expected: []uint32{0, 999},
487488
},
488489
{
489-
name: "unknown group",
490-
groups: []string{"unknown"},
491-
err: "unable to find group unknown",
490+
name: "unknown group",
491+
groups: []string{"unknown"},
492+
err: "unable to find group unknown",
493+
expected: []uint32{0},
492494
},
493495
}
494496

‎pkg/cri/sbserver/container_create_linux.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
137137
// Because it is still useful to get additional gids for uid 0.
138138
userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10)
139139
}
140-
specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr))
140+
specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr),
141+
customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups()))
141142

142143
asp := securityContext.GetApparmor()
143144
if asp == nil {

‎pkg/cri/server/container_create_linux.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
376376
// Because it is still useful to get additional gids for uid 0.
377377
userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10)
378378
}
379-
specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr))
379+
specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr),
380+
customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups()))
380381

381382
asp := securityContext.GetApparmor()
382383
if asp == nil {

0 commit comments

Comments
 (0)