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 8a963bf1a..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,14 +950,23 @@ 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 { - 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) diff --git a/pkg/cri/server/helpers_linux.go b/pkg/cri/server/helpers_linux.go index d0cb13006..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) } @@ -349,6 +346,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()