From a44b356274bf2a5f672734698519f2bb93d10efb Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 30 Dec 2022 13:55:28 -0300 Subject: [PATCH 1/3] cri: Fix assert vs require in tests Currently we require that c.containerSpec() does not return an error if test.err is not set. However, if the require fails (i.e. it indeed returned an error) the rest of the code is executed anyways. The rest of the code assumes it did not return an error (so code assumes spec is not nil). This fails miserably if it indeed returned an error, as spec is nil and go crashes while running the unit tests. Let's require it is not an error, so code does not continue to execute if that fails and go doesn't crash. In the test.err case is not harmful the bug of using assert, but let's switch it to require too as that is what we really want. Signed-off-by: Rodrigo Campos --- pkg/cri/server/container_create_linux_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cri/server/container_create_linux_test.go b/pkg/cri/server/container_create_linux_test.go index 8a963bf1a..6fd1e0e8d 100644 --- a/pkg/cri/server/container_create_linux_test.go +++ b/pkg/cri/server/container_create_linux_test.go @@ -895,11 +895,11 @@ func TestUserNamespace(t *testing.T) { spec, err := c.containerSpec(testID, testSandboxID, testPid, "", testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime) if test.err { - assert.Error(t, err) + require.Error(t, err) assert.Nil(t, spec) return } - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, spec.Linux.UIDMappings, test.expUIDMapping) assert.Equal(t, spec.Linux.GIDMappings, test.expGIDMapping) From 4eed20fc31c9166634dfbbbc40704fa85ff3f403 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 30 Dec 2022 14:19:50 -0300 Subject: [PATCH 2/3] cri: Verify userns container config is consisten with sandbox The sandbox and container both have the userns config. Lets make sure they are the same, therefore consistent. Signed-off-by: Rodrigo Campos --- pkg/cri/server/container_create_linux.go | 6 ++ pkg/cri/server/container_create_linux_test.go | 67 +++++++++++++++++++ pkg/cri/server/helpers_linux.go | 58 ++++++++++++++++ 3 files changed, 131 insertions(+) diff --git a/pkg/cri/server/container_create_linux.go b/pkg/cri/server/container_create_linux.go index 5181c5f5e..05a2f05d2 100644 --- a/pkg/cri/server/container_create_linux.go +++ b/pkg/cri/server/container_create_linux.go @@ -316,6 +316,12 @@ func (c *criService) containerSpec( return nil, fmt.Errorf("user namespace configuration: %w", err) } + // Check sandbox userns config is consistent with container config. + sandboxUsernsOpts := sandboxConfig.GetLinux().GetSecurityContext().GetNamespaceOptions().GetUsernsOptions() + if !sameUsernsConfig(sandboxUsernsOpts, nsOpts.GetUsernsOptions()) { + return nil, fmt.Errorf("user namespace config for sandbox is different from container. Sandbox userns config: %v - Container userns config: %v", sandboxUsernsOpts, nsOpts.GetUsernsOptions()) + } + specOpts = append(specOpts, customopts.WithOOMScoreAdj(config, c.config.RestrictOOMScoreAdj), customopts.WithPodNamespaces(securityContext, sandboxPid, targetPid, uids, gids), diff --git a/pkg/cri/server/container_create_linux_test.go b/pkg/cri/server/container_create_linux_test.go index 6fd1e0e8d..b6b9cff2b 100644 --- a/pkg/cri/server/container_create_linux_test.go +++ b/pkg/cri/server/container_create_linux_test.go @@ -814,6 +814,11 @@ func TestUserNamespace(t *testing.T) { ContainerId: 1000, Length: 10, } + otherIDMap := runtime.IDMapping{ + HostId: 2000, + ContainerId: 2000, + Length: 10, + } expIDMap := runtimespec.LinuxIDMapping{ HostID: 1000, ContainerID: 1000, @@ -824,6 +829,7 @@ func TestUserNamespace(t *testing.T) { c := newTestCRIService() for desc, test := range map[string]struct { userNS *runtime.UserNamespace + sandboxUserNS *runtime.UserNamespace expNS *runtimespec.LinuxNamespace expNotNS *runtimespec.LinuxNamespace // Does NOT contain this namespace expUIDMapping []runtimespec.LinuxIDMapping @@ -871,6 +877,58 @@ func TestUserNamespace(t *testing.T) { expUIDMapping: []runtimespec.LinuxIDMapping{expIDMap}, expGIDMapping: []runtimespec.LinuxIDMapping{expIDMap}, }, + "pod namespace mode with inconsistent sandbox config (different GIDs)": { + userNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + sandboxUserNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&otherIDMap}, + }, + err: true, + }, + "pod namespace mode with inconsistent sandbox config (different UIDs)": { + userNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + sandboxUserNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&otherIDMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + err: true, + }, + "pod namespace mode with inconsistent sandbox config (different len)": { + userNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + sandboxUserNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap, &idMap}, + Gids: []*runtime.IDMapping{&idMap, &idMap}, + }, + err: true, + }, + "pod namespace mode with inconsistent sandbox config (different mode)": { + userNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_POD, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + sandboxUserNS: &runtime.UserNamespace{ + Mode: runtime.NamespaceMode_NODE, + Uids: []*runtime.IDMapping{&idMap}, + Gids: []*runtime.IDMapping{&idMap}, + }, + err: true, + }, "pod namespace mode with several mappings": { userNS: &runtime.UserNamespace{ Mode: runtime.NamespaceMode_POD, @@ -892,6 +950,15 @@ func TestUserNamespace(t *testing.T) { test := test t.Run(desc, func(t *testing.T) { containerConfig.Linux.SecurityContext.NamespaceOptions = &runtime.NamespaceOption{UsernsOptions: test.userNS} + // By default, set sandbox and container config to the same (this is + // required by containerSpec). However, if the test wants to test for what + // happens when they don't match, the test.sandboxUserNS should be set and + // we just use that. + sandboxUserns := test.userNS + if test.sandboxUserNS != nil { + sandboxUserns = test.sandboxUserNS + } + sandboxConfig.Linux.SecurityContext.NamespaceOptions = &runtime.NamespaceOption{UsernsOptions: sandboxUserns} spec, err := c.containerSpec(testID, testSandboxID, testPid, "", testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime) if test.err { diff --git a/pkg/cri/server/helpers_linux.go b/pkg/cri/server/helpers_linux.go index d0cb13006..63fb7d116 100644 --- a/pkg/cri/server/helpers_linux.go +++ b/pkg/cri/server/helpers_linux.go @@ -349,6 +349,64 @@ func parseUsernsIDs(userns *runtime.UserNamespace) (uids, gids []specs.LinuxIDMa return uids, gids, nil } +// sameUsernsConfig checks if the userns configs are the same. If the mappings +// on each config are the same but in different order, it returns false. +// XXX: If the runtime.UserNamespace struct changes, we should update this +// function accordingly. +func sameUsernsConfig(a, b *runtime.UserNamespace) bool { + // If both are nil, they are the same. + if a == nil && b == nil { + return true + } + // If only one is nil, they are different. + if a == nil || b == nil { + return false + } + // At this point, a is not nil nor b. + + if a.GetMode() != b.GetMode() { + return false + } + + aUids, aGids, err := parseUsernsIDs(a) + if err != nil { + return false + } + bUids, bGids, err := parseUsernsIDs(b) + if err != nil { + return false + } + + if !sameMapping(aUids, bUids) { + return false + } + if !sameMapping(aGids, bGids) { + return false + } + return true +} + +// sameMapping checks if the mappings are the same. If the mappings are the same +// but in different order, it returns false. +func sameMapping(a, b []specs.LinuxIDMapping) bool { + if len(a) != len(b) { + return false + } + + for x := range a { + if a[x].ContainerID != b[x].ContainerID { + return false + } + if a[x].HostID != b[x].HostID { + return false + } + if a[x].Size != b[x].Size { + return false + } + } + return true +} + func snapshotterRemapOpts(nsOpts *runtime.NamespaceOption) ([]snapshots.Opt, error) { snapshotOpt := []snapshots.Opt{} usernsOpts := nsOpts.GetUsernsOptions() From 72ef986222b22f0f87eb897efc581c904f85fb85 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Fri, 30 Dec 2022 14:48:23 -0300 Subject: [PATCH 3/3] cri: Simplify parseUsernsIDs() Signed-off-by: Rodrigo Campos --- pkg/cri/server/helpers_linux.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/cri/server/helpers_linux.go b/pkg/cri/server/helpers_linux.go index 63fb7d116..d490ce881 100644 --- a/pkg/cri/server/helpers_linux.go +++ b/pkg/cri/server/helpers_linux.go @@ -319,15 +319,12 @@ func parseUsernsIDs(userns *runtime.UserNamespace) (uids, gids []specs.LinuxIDMa return nil, nil, nil } - uidRuntimeMap := userns.GetUids() - gidRuntimeMap := userns.GetGids() - - uids, err := parseUsernsIDMap(uidRuntimeMap) + uids, err := parseUsernsIDMap(userns.GetUids()) if err != nil { return nil, nil, fmt.Errorf("UID mapping: %w", err) } - gids, err = parseUsernsIDMap(gidRuntimeMap) + gids, err = parseUsernsIDMap(userns.GetGids()) if err != nil { return nil, nil, fmt.Errorf("GID mapping: %w", err) }