Skip to content

Commit 286a01f

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]> (cherry picked from commit 3eda46a) > Conflicts: > integration/addition_gids_test.go > pkg/cri/sbserver/container_create_linux.go Signed-off-by: Akihiro Suda <[email protected]>
1 parent 3018234 commit 286a01f

5 files changed

Lines changed: 168 additions & 54 deletions

File tree

‎integration/addition_gids_test.go‎

Lines changed: 91 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package integration
2121

2222
import (
23+
"fmt"
2324
"os"
2425
"path/filepath"
2526
"testing"
@@ -31,49 +32,98 @@ import (
3132
)
3233

3334
func TestAdditionalGids(t *testing.T) {
34-
testPodLogDir, err := os.MkdirTemp("/tmp", "additional-gids")
35-
require.NoError(t, err)
36-
defer os.RemoveAll(testPodLogDir)
35+
testImage := GetImage(BusyBox)
36+
EnsureImageExists(t, testImage)
37+
type testCase struct {
38+
description string
39+
opts []ContainerOpts
40+
expected string
41+
}
3742

38-
t.Log("Create a sandbox with log directory")
39-
sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "additional-gids",
40-
WithPodLogDirectory(testPodLogDir))
43+
testCases := []testCase{
44+
{
45+
description: "Equivalent of `docker run` (no option)",
46+
opts: nil,
47+
expected: "groups=0(root),10(wheel)",
48+
},
49+
{
50+
description: "Equivalent of `docker run --group-add 1 --group-add 1234`",
51+
opts: []ContainerOpts{WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/})},
52+
expected: "groups=0(root),1(daemon),10(wheel),1234",
53+
},
54+
{
55+
description: "Equivalent of `docker run --user 1234`",
56+
opts: []ContainerOpts{WithRunAsUser(1234)},
57+
expected: "groups=0(root)",
58+
},
59+
{
60+
description: "Equivalent of `docker run --user 1234:1234`",
61+
opts: []ContainerOpts{WithRunAsUser(1234), WithRunAsGroup(1234)},
62+
expected: "groups=1234",
63+
},
64+
{
65+
description: "Equivalent of `docker run --user 1234 --group-add 1234`",
66+
opts: []ContainerOpts{WithRunAsUser(1234), WithSupplementalGroups([]int64{1234})},
67+
expected: "groups=0(root),1234",
68+
},
69+
{
70+
description: "Equivalent of `docker run --user daemon` (Supported by CRI, although unsupported by kube-apiserver)",
71+
opts: []ContainerOpts{WithRunAsUsername("daemon")},
72+
expected: "groups=1(daemon)",
73+
},
74+
{
75+
description: "Equivalent of `docker run --user daemon --group-add 1234` (Supported by CRI, although unsupported by kube-apiserver)",
76+
opts: []ContainerOpts{WithRunAsUsername("daemon"), WithSupplementalGroups([]int64{1234})},
77+
expected: "groups=1(daemon),1234",
78+
},
79+
}
4180

42-
var (
43-
testImage = GetImage(BusyBox)
44-
containerName = "test-container"
45-
)
81+
for i, tc := range testCases {
82+
i, tc := i, tc
83+
tBasename := fmt.Sprintf("case-%d", i)
84+
t.Run(tBasename, func(t *testing.T) {
85+
t.Log(tc.description)
86+
t.Logf("Expected=%q", tc.expected)
4687

47-
EnsureImageExists(t, testImage)
88+
testPodLogDir := t.TempDir()
89+
90+
t.Log("Create a sandbox with log directory")
91+
sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", tBasename,
92+
WithPodLogDirectory(testPodLogDir))
93+
94+
t.Log("Create a container to print id")
95+
containerName := tBasename
96+
cnConfig := ContainerConfig(
97+
containerName,
98+
testImage,
99+
append(
100+
[]ContainerOpts{
101+
WithCommand("id"),
102+
WithLogPath(containerName),
103+
}, tc.opts...)...,
104+
)
105+
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
106+
require.NoError(t, err)
107+
108+
t.Log("Start the container")
109+
require.NoError(t, runtimeService.StartContainer(cn))
110+
111+
t.Log("Wait for container to finish running")
112+
require.NoError(t, Eventually(func() (bool, error) {
113+
s, err := runtimeService.ContainerStatus(cn)
114+
if err != nil {
115+
return false, err
116+
}
117+
if s.GetState() == runtime.ContainerState_CONTAINER_EXITED {
118+
return true, nil
119+
}
120+
return false, nil
121+
}, time.Second, 30*time.Second))
48122

49-
t.Log("Create a container to print id")
50-
cnConfig := ContainerConfig(
51-
containerName,
52-
testImage,
53-
WithCommand("id"),
54-
WithLogPath(containerName),
55-
WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/}),
56-
)
57-
cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
58-
require.NoError(t, err)
59-
60-
t.Log("Start the container")
61-
require.NoError(t, runtimeService.StartContainer(cn))
62-
63-
t.Log("Wait for container to finish running")
64-
require.NoError(t, Eventually(func() (bool, error) {
65-
s, err := runtimeService.ContainerStatus(cn)
66-
if err != nil {
67-
return false, err
68-
}
69-
if s.GetState() == runtime.ContainerState_CONTAINER_EXITED {
70-
return true, nil
71-
}
72-
return false, nil
73-
}, time.Second, 30*time.Second))
74-
75-
t.Log("Search additional groups in container log")
76-
content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName))
77-
assert.NoError(t, err)
78-
assert.Contains(t, string(content), "groups=1(daemon),10(wheel),1234")
123+
t.Log("Search additional groups in container log")
124+
content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName))
125+
assert.NoError(t, err)
126+
assert.Contains(t, string(content), tc.expected+"\n")
127+
})
128+
}
79129
}

‎integration/main_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,45 @@ func WithLogPath(path string) ContainerOpts {
311311
}
312312
}
313313

314+
// WithRunAsUser sets the uid.
315+
func WithRunAsUser(uid int64) ContainerOpts {
316+
return func(c *runtime.ContainerConfig) {
317+
if c.Linux == nil {
318+
c.Linux = &runtime.LinuxContainerConfig{}
319+
}
320+
if c.Linux.SecurityContext == nil {
321+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
322+
}
323+
c.Linux.SecurityContext.RunAsUser = &runtime.Int64Value{Value: uid}
324+
}
325+
}
326+
327+
// WithRunAsUsername sets the username.
328+
func WithRunAsUsername(username string) ContainerOpts {
329+
return func(c *runtime.ContainerConfig) {
330+
if c.Linux == nil {
331+
c.Linux = &runtime.LinuxContainerConfig{}
332+
}
333+
if c.Linux.SecurityContext == nil {
334+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
335+
}
336+
c.Linux.SecurityContext.RunAsUsername = username
337+
}
338+
}
339+
340+
// WithRunAsGroup sets the gid.
341+
func WithRunAsGroup(gid int64) ContainerOpts {
342+
return func(c *runtime.ContainerConfig) {
343+
if c.Linux == nil {
344+
c.Linux = &runtime.LinuxContainerConfig{}
345+
}
346+
if c.Linux.SecurityContext == nil {
347+
c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
348+
}
349+
c.Linux.SecurityContext.RunAsGroup = &runtime.Int64Value{Value: gid}
350+
}
351+
}
352+
314353
// WithSupplementalGroups adds supplemental groups.
315354
func WithSupplementalGroups(gids []int64) ContainerOpts { //nolint:unused
316355
return func(c *runtime.ContainerConfig) {

‎oci/spec_opts.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,17 @@ func setCapabilities(s *Spec) {
113113
}
114114
}
115115

116+
// ensureAdditionalGids ensures that the primary GID is also included in the additional GID list.
117+
func ensureAdditionalGids(s *Spec) {
118+
setProcess(s)
119+
for _, f := range s.Process.User.AdditionalGids {
120+
if f == s.Process.User.GID {
121+
return
122+
}
123+
}
124+
s.Process.User.AdditionalGids = append([]uint32{s.Process.User.GID}, s.Process.User.AdditionalGids...)
125+
}
126+
116127
// WithDefaultSpec returns a SpecOpts that will populate the spec with default
117128
// values.
118129
//
@@ -522,7 +533,9 @@ func WithNamespacedCgroup() SpecOpts {
522533
// user, uid, user:group, uid:gid, uid:group, user:gid
523534
func WithUser(userstr string) SpecOpts {
524535
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) error {
536+
defer ensureAdditionalGids(s)
525537
setProcess(s)
538+
s.Process.User.AdditionalGids = nil
526539

527540
// For LCOW it's a bit harder to confirm that the user actually exists on the host as a rootfs isn't
528541
// mounted on the host and shared into the guest, but rather the rootfs is constructed entirely in the
@@ -615,7 +628,9 @@ func WithUser(userstr string) SpecOpts {
615628
// WithUIDGID allows the UID and GID for the Process to be set
616629
func WithUIDGID(uid, gid uint32) SpecOpts {
617630
return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
631+
defer ensureAdditionalGids(s)
618632
setProcess(s)
633+
s.Process.User.AdditionalGids = nil
619634
s.Process.User.UID = uid
620635
s.Process.User.GID = gid
621636
return nil
@@ -628,7 +643,9 @@ func WithUIDGID(uid, gid uint32) SpecOpts {
628643
// additionally sets the gid to 0, and does not return an error.
629644
func WithUserID(uid uint32) SpecOpts {
630645
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
646+
defer ensureAdditionalGids(s)
631647
setProcess(s)
648+
s.Process.User.AdditionalGids = nil
632649
setUser := func(root string) error {
633650
user, err := UserFromPath(root, func(u user.User) bool {
634651
return u.Uid == int(uid)
@@ -674,7 +691,9 @@ func WithUserID(uid uint32) SpecOpts {
674691
// the container.
675692
func WithUsername(username string) SpecOpts {
676693
return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
694+
defer ensureAdditionalGids(s)
677695
setProcess(s)
696+
s.Process.User.AdditionalGids = nil
678697
if s.Linux != nil {
679698
setUser := func(root string) error {
680699
user, err := UserFromPath(root, func(u user.User) bool {
@@ -725,7 +744,9 @@ func WithAdditionalGIDs(userstr string) SpecOpts {
725744
return nil
726745
}
727746
setProcess(s)
747+
s.Process.User.AdditionalGids = nil
728748
setAdditionalGids := func(root string) error {
749+
defer ensureAdditionalGids(s)
729750
var username string
730751
uid, err := strconv.Atoi(userstr)
731752
if err == nil {
@@ -796,6 +817,7 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
796817
}
797818
setProcess(s)
798819
setAdditionalGids := func(root string) error {
820+
defer ensureAdditionalGids(s)
799821
gpath, err := fs.RootPath(root, "/etc/group")
800822
if err != nil {
801823
return err

‎oci/spec_opts_linux_test.go‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -179,23 +179,23 @@ sys:x:3:root,bin,adm
179179
}{
180180
{
181181
user: "root",
182-
expected: []uint32{1, 2, 3},
182+
expected: []uint32{0, 1, 2, 3},
183183
},
184184
{
185185
user: "1000",
186-
expected: nil,
186+
expected: []uint32{0},
187187
},
188188
{
189189
user: "bin",
190-
expected: []uint32{2, 3},
190+
expected: []uint32{0, 2, 3},
191191
},
192192
{
193193
user: "bin:root",
194-
expected: []uint32{},
194+
expected: []uint32{0},
195195
},
196196
{
197197
user: "daemon",
198-
expected: []uint32{1},
198+
expected: []uint32{0, 1},
199199
},
200200
}
201201
for _, testCase := range testCases {
@@ -460,8 +460,9 @@ daemon:x:2:root,bin,daemon
460460
err string
461461
}{
462462
{
463-
name: "no additional gids",
464-
groups: []string{},
463+
name: "no additional gids",
464+
groups: []string{},
465+
expected: []uint32{0},
465466
},
466467
{
467468
name: "no additional gids, append root gid",
@@ -471,7 +472,7 @@ daemon:x:2:root,bin,daemon
471472
{
472473
name: "no additional gids, append bin and daemon gids",
473474
groups: []string{"bin", "daemon"},
474-
expected: []uint32{1, 2},
475+
expected: []uint32{0, 1, 2},
475476
},
476477
{
477478
name: "has root additional gids, append bin and daemon gids",
@@ -482,12 +483,13 @@ daemon:x:2:root,bin,daemon
482483
{
483484
name: "append group id",
484485
groups: []string{"999"},
485-
expected: []uint32{999},
486+
expected: []uint32{0, 999},
486487
},
487488
{
488-
name: "unknown group",
489-
groups: []string{"unknown"},
490-
err: "unable to find group unknown",
489+
name: "unknown group",
490+
groups: []string{"unknown"},
491+
err: "unable to find group unknown",
492+
expected: []uint32{0},
491493
},
492494
}
493495

‎pkg/cri/server/container_create_linux.go‎

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

353354
asp := securityContext.GetApparmor()
354355
if asp == nil {

0 commit comments

Comments
 (0)