From cf8164bccfc1cc8fa82061cf29813d1570bb6948 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 7 Jul 2022 16:49:39 +0200 Subject: [PATCH] apis: add validation for HostUsers This commit just adds a validation according to KEP-127. We check that only the supported volumes for phase 1 of the KEP are accepted. Signed-off-by: Rodrigo Campos --- pkg/api/pod/util.go | 18 +++ pkg/api/pod/util_test.go | 97 ++++++++++++ pkg/apis/core/validation/validation.go | 47 ++++++ pkg/apis/core/validation/validation_test.go | 167 ++++++++++++++++++++ 4 files changed, 329 insertions(+) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index a0af5b15de9..819bd63d174 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -539,6 +539,15 @@ func dropDisabledFields( }) } + // If the feature is disabled and not in use, drop the hostUsers field. + if !utilfeature.DefaultFeatureGate.Enabled(features.UserNamespacesStatelessPodsSupport) && !hostUsersInUse(oldPodSpec) { + // Drop the field in podSpec only if SecurityContext is not nil. + // If it is nil, there is no need to set hostUsers=nil (it will be nil too). + if podSpec.SecurityContext != nil { + podSpec.SecurityContext.HostUsers = nil + } + } + dropDisabledProcMountField(podSpec, oldPodSpec) dropDisabledCSIVolumeSourceAlphaFields(podSpec, oldPodSpec) @@ -672,6 +681,15 @@ func nodeTaintsPolicyInUse(podSpec *api.PodSpec) bool { return false } +// hostUsersInUse returns true if the pod spec has spec.hostUsers field set. +func hostUsersInUse(podSpec *api.PodSpec) bool { + if podSpec != nil && podSpec.SecurityContext != nil && podSpec.SecurityContext.HostUsers != nil { + return true + } + + return false +} + // procMountInUse returns true if the pod spec is non-nil and has a SecurityContext's ProcMount field set to a non-default value func procMountInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index c536cc006ff..4796010a9d2 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1949,3 +1949,100 @@ func TestDropDisabledMatchLabelKeysField(t *testing.T) { }) } } + +func TestDropHostUsers(t *testing.T) { + falseVar := false + trueVar := true + + podWithoutHostUsers := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{}}, + } + } + podWithHostUsersFalse := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{ + HostUsers: &falseVar, + }, + }, + } + } + podWithHostUsersTrue := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{ + HostUsers: &trueVar, + }, + }, + } + } + + podInfo := []struct { + description string + hasHostUsers bool + pod func() *api.Pod + }{ + { + description: "with hostUsers=true", + hasHostUsers: true, + pod: podWithHostUsersTrue, + }, + { + description: "with hostUsers=false", + hasHostUsers: true, + pod: podWithHostUsersFalse, + }, + { + description: "with hostUsers=nil", + pod: func() *api.Pod { return nil }, + }, + } + + for _, enabled := range []bool{true, false} { + for _, oldPodInfo := range podInfo { + for _, newPodInfo := range podInfo { + oldPodHasHostUsers, oldPod := oldPodInfo.hasHostUsers, oldPodInfo.pod() + newPodHasHostUsers, newPod := newPodInfo.hasHostUsers, newPodInfo.pod() + if newPod == nil { + continue + } + + t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.UserNamespacesStatelessPodsSupport, enabled)() + + DropDisabledPodFields(newPod, oldPod) + + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) { + t.Errorf("old pod changed: %v", cmp.Diff(oldPod, oldPodInfo.pod())) + } + + switch { + case enabled || oldPodHasHostUsers: + // new pod should not be changed if the feature is enabled, or if the old pod had hostUsers + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + case newPodHasHostUsers: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod was not changed") + } + // new pod should not have hostUsers + if exp := podWithoutHostUsers(); !reflect.DeepEqual(newPod, exp) { + t.Errorf("new pod had hostUsers: %v", cmp.Diff(newPod, exp)) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodInfo.pod()) { + t.Errorf("new pod changed: %v", cmp.Diff(newPod, newPodInfo.pod())) + } + } + }) + } + } + } + +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index f6a7e417fd9..a8ea7551061 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3075,6 +3075,52 @@ func validateContainerCommon(ctr *core.Container, volumes map[string]core.Volume allErrs = append(allErrs, validatePullPolicy(ctr.ImagePullPolicy, path.Child("imagePullPolicy"))...) allErrs = append(allErrs, ValidateResourceRequirements(&ctr.Resources, path.Child("resources"), opts)...) allErrs = append(allErrs, ValidateSecurityContext(ctr.SecurityContext, path.Child("securityContext"))...) + return allErrs +} + +func validateHostUsers(spec *core.PodSpec, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + // Only make the following checks if hostUsers is false (otherwise, the container uses the + // same userns as the host, and so there isn't anything to check). + if spec.SecurityContext == nil || spec.SecurityContext.HostUsers == nil || *spec.SecurityContext.HostUsers == true { + return allErrs + } + + // For now only these volumes are supported: + // - configmap + // - secret + // - downwardAPI + // - emptyDir + // - projected + // So reject anything else. + for i, vol := range spec.Volumes { + switch { + case vol.EmptyDir != nil: + case vol.Secret != nil: + case vol.DownwardAPI != nil: + case vol.ConfigMap != nil: + case vol.Projected != nil: + default: + allErrs = append(allErrs, field.Forbidden(fldPath.Child("volumes").Index(i), "volume type not supported when `pod.Spec.HostUsers` is false")) + } + } + + // We decided to restrict the usage of userns with other host namespaces: + // https://github.com/kubernetes/kubernetes/pull/111090#discussion_r935994282 + // The tl;dr is: you can easily run into permission issues that seem unexpected, we don't + // know of any good use case and we can always enable them later. + + // Note we already validated above spec.SecurityContext is not nil. + if spec.SecurityContext.HostNetwork { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("hostNetwork"), "when `pod.Spec.HostUsers` is false")) + } + if spec.SecurityContext.HostPID { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("HostPID"), "when `pod.Spec.HostUsers` is false")) + } + if spec.SecurityContext.HostIPC { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("HostIPC"), "when `pod.Spec.HostUsers` is false")) + } return allErrs } @@ -3545,6 +3591,7 @@ func ValidatePodSpec(spec *core.PodSpec, podMeta *metav1.ObjectMeta, fldPath *fi allErrs = append(allErrs, validateReadinessGates(spec.ReadinessGates, fldPath.Child("readinessGates"))...) allErrs = append(allErrs, validateTopologySpreadConstraints(spec.TopologySpreadConstraints, fldPath.Child("topologySpreadConstraints"))...) allErrs = append(allErrs, validateWindowsHostProcessPod(spec, fldPath, opts)...) + allErrs = append(allErrs, validateHostUsers(spec, fldPath)...) if len(spec.ServiceAccountName) > 0 { for _, msg := range ValidateServiceAccountName(spec.ServiceAccountName, false) { allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceAccountName"), spec.ServiceAccountName, msg)) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index ce7008bdbc8..0fd71f1264e 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18277,6 +18277,7 @@ func TestValidateOSFields(t *testing.T) { "SecurityContext.HostIPC", "SecurityContext.HostNetwork", "SecurityContext.HostPID", + "SecurityContext.HostUsers", "SecurityContext.RunAsGroup", "SecurityContext.RunAsUser", "SecurityContext.SELinuxOptions", @@ -20572,6 +20573,172 @@ func TestValidateNonSpecialIP(t *testing.T) { } } +func TestValidateHostUsers(t *testing.T) { + falseVar := false + trueVar := true + + cases := []struct { + name string + success bool + spec *core.PodSpec + }{ + { + name: "empty", + success: true, + spec: &core.PodSpec{}, + }, + { + name: "hostUsers unset", + success: true, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{}, + }, + }, + { + name: "hostUsers=false", + success: true, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + }, + }, + }, + { + name: "hostUsers=true", + success: true, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &trueVar, + }, + }, + }, + { + name: "hostUsers=false & volumes", + success: true, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + }, + Volumes: []core.Volume{ + { + Name: "configmap", + VolumeSource: core.VolumeSource{ + ConfigMap: &core.ConfigMapVolumeSource{ + LocalObjectReference: core.LocalObjectReference{Name: "configmap"}, + }, + }, + }, + { + Name: "secret", + VolumeSource: core.VolumeSource{ + Secret: &core.SecretVolumeSource{ + SecretName: "secret", + }, + }, + }, + { + Name: "downward-api", + VolumeSource: core.VolumeSource{ + DownwardAPI: &core.DownwardAPIVolumeSource{}, + }, + }, + { + Name: "proj", + VolumeSource: core.VolumeSource{ + Projected: &core.ProjectedVolumeSource{}, + }, + }, + { + Name: "empty-dir", + VolumeSource: core.VolumeSource{ + EmptyDir: &core.EmptyDirVolumeSource{}, + }, + }, + }, + }, + }, + { + name: "hostUsers=false - unsupported volume", + success: false, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + }, + Volumes: []core.Volume{ + { + Name: "host-path", + VolumeSource: core.VolumeSource{ + HostPath: &core.HostPathVolumeSource{}, + }, + }, + }, + }, + }, + { + // It should ignore unsupported volumes with hostUsers=true. + name: "hostUsers=true - unsupported volume", + success: true, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &trueVar, + }, + Volumes: []core.Volume{ + { + Name: "host-path", + VolumeSource: core.VolumeSource{ + HostPath: &core.HostPathVolumeSource{}, + }, + }, + }, + }, + }, + { + name: "hostUsers=false & HostNetwork", + success: false, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + HostNetwork: true, + }, + }, + }, + { + name: "hostUsers=false & HostPID", + success: false, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + HostPID: true, + }, + }, + }, + { + name: "hostUsers=false & HostIPC", + success: false, + spec: &core.PodSpec{ + SecurityContext: &core.PodSecurityContext{ + HostUsers: &falseVar, + HostIPC: true, + }, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fPath := field.NewPath("spec") + + allErrs := validateHostUsers(tc.spec, fPath) + if !tc.success && len(allErrs) == 0 { + t.Errorf("Unexpected success") + } + if tc.success && len(allErrs) != 0 { + t.Errorf("Unexpected error(s): %v", allErrs) + } + }) + } +} + func TestValidateWindowsHostProcessPod(t *testing.T) { const containerName = "container" falseVar := false