diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index ed7d0ec85c6..9d553d6ff0e 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -53,7 +53,6 @@ import ( "k8s.io/kubernetes/pkg/cluster/ports" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/fieldpath" - "k8s.io/kubernetes/pkg/security/apparmor" netutils "k8s.io/utils/net" ) @@ -4058,7 +4057,7 @@ func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *core.Po allErrs = append(allErrs, field.Invalid(fldPath.Key(k), containerName, "container not found")) } - if err := apparmor.ValidateProfileFormat(p); err != nil { + if err := ValidateAppArmorProfileFormat(p); err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Key(k), p, err.Error())) } } @@ -4066,6 +4065,16 @@ func ValidateAppArmorPodAnnotations(annotations map[string]string, spec *core.Po return allErrs } +func ValidateAppArmorProfileFormat(profile string) error { + if profile == "" || profile == v1.AppArmorBetaProfileRuntimeDefault || profile == v1.AppArmorBetaProfileNameUnconfined { + return nil + } + if !strings.HasPrefix(profile, v1.AppArmorBetaProfileNamePrefix) { + return fmt.Errorf("invalid AppArmor profile name: %q", profile) + } + return nil +} + func podSpecHasContainer(spec *core.PodSpec, containerName string) bool { var hasContainer bool podshelper.VisitContainersWithPath(spec, field.NewPath("spec"), func(c *core.Container, _ *field.Path) bool { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 695d38f1dc2..71528095ff9 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18,6 +18,7 @@ package validation import ( "bytes" + "fmt" "math" "reflect" "strings" @@ -25,6 +26,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" asserttestify "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" @@ -20134,3 +20136,26 @@ func TestValidateOS(t *testing.T) { }) } } + +func TestValidateAppArmorProfileFormat(t *testing.T) { + tests := []struct { + profile string + expectValid bool + }{ + {"", true}, + {v1.AppArmorBetaProfileRuntimeDefault, true}, + {v1.AppArmorBetaProfileNameUnconfined, true}, + {"baz", false}, // Missing local prefix. + {v1.AppArmorBetaProfileNamePrefix + "/usr/sbin/ntpd", true}, + {v1.AppArmorBetaProfileNamePrefix + "foo-bar", true}, + } + + for _, test := range tests { + err := ValidateAppArmorProfileFormat(test.profile) + if test.expectValid { + assert.NoError(t, err, "Profile %s should be valid", test.profile) + } else { + assert.Error(t, err, fmt.Sprintf("Profile %s should not be valid", test.profile)) + } + } +} diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index 73062f586a6..53e3bbaf821 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -33,7 +33,6 @@ import ( core "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/policy" - "k8s.io/kubernetes/pkg/security/apparmor" "k8s.io/kubernetes/pkg/security/podsecuritypolicy/seccomp" psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util" ) @@ -138,13 +137,13 @@ func ValidatePodSecurityPolicySpecificAnnotations(annotations map[string]string, allErrs := field.ErrorList{} if p := annotations[v1.AppArmorBetaDefaultProfileAnnotationKey]; p != "" { - if err := apparmor.ValidateProfileFormat(p); err != nil { + if err := apivalidation.ValidateAppArmorProfileFormat(p); err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Key(v1.AppArmorBetaDefaultProfileAnnotationKey), p, err.Error())) } } if allowed := annotations[v1.AppArmorBetaAllowedProfilesAnnotationKey]; allowed != "" { for _, p := range strings.Split(allowed, ",") { - if err := apparmor.ValidateProfileFormat(p); err != nil { + if err := apivalidation.ValidateAppArmorProfileFormat(p); err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Key(v1.AppArmorBetaAllowedProfilesAnnotationKey), allowed, err.Error())) } } diff --git a/pkg/security/apparmor/OWNERS b/pkg/security/apparmor/OWNERS new file mode 100644 index 00000000000..123e203f90e --- /dev/null +++ b/pkg/security/apparmor/OWNERS @@ -0,0 +1,10 @@ +# See the OWNERS docs at https://go.k8s.io/owners + +approvers: + - sig-node-approvers + - tallclair +reviewers: + - sig-node-reviewers + - tallclair +labels: + - sig/node diff --git a/pkg/security/apparmor/validate.go b/pkg/security/apparmor/validate.go index 34a0b1ee368..cbcd9c49f2c 100644 --- a/pkg/security/apparmor/validate.go +++ b/pkg/security/apparmor/validate.go @@ -28,6 +28,7 @@ import ( v1 "k8s.io/api/core/v1" utilfeature "k8s.io/apiserver/pkg/util/feature" podutil "k8s.io/kubernetes/pkg/api/v1/pod" + "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/features" utilpath "k8s.io/utils/path" ) @@ -74,10 +75,19 @@ func (v *validator) Validate(pod *v1.Pod) error { var retErr error podutil.VisitContainers(&pod.Spec, podutil.AllContainers, func(container *v1.Container, containerType podutil.ContainerType) bool { - retErr = ValidateProfileFormat(GetProfileName(pod, container.Name)) + profile := GetProfileName(pod, container.Name) + retErr = validation.ValidateAppArmorProfileFormat(profile) if retErr != nil { return false } + // TODO(#64841): This would ideally be part of validation.ValidateAppArmorProfileFormat, but + // that is called for API validation, and this is tightening validation. + if strings.HasPrefix(profile, v1.AppArmorBetaProfileNamePrefix) { + if strings.TrimSpace(strings.TrimPrefix(profile, v1.AppArmorBetaProfileNamePrefix)) == "" { + retErr = fmt.Errorf("invalid empty AppArmor profile name: %q", profile) + return false + } + } return true }) @@ -108,17 +118,6 @@ func validateHost() error { return nil } -// ValidateProfileFormat checks the format of the profile. -func ValidateProfileFormat(profile string) error { - if profile == "" || profile == v1.AppArmorBetaProfileRuntimeDefault || profile == v1.AppArmorBetaProfileNameUnconfined { - return nil - } - if !strings.HasPrefix(profile, v1.AppArmorBetaProfileNamePrefix) { - return fmt.Errorf("invalid AppArmor profile name: %q", profile) - } - return nil -} - func getAppArmorFS() (string, error) { mountsFile, err := os.Open("/proc/mounts") if err != nil { diff --git a/pkg/security/apparmor/validate_test.go b/pkg/security/apparmor/validate_test.go index 03b4a487f47..2c2d4e14ff9 100644 --- a/pkg/security/apparmor/validate_test.go +++ b/pkg/security/apparmor/validate_test.go @@ -46,29 +46,6 @@ func TestValidateHost(t *testing.T) { assert.NoError(t, validateHost()) } -func TestValidateProfileFormat(t *testing.T) { - tests := []struct { - profile string - expectValid bool - }{ - {"", true}, - {v1.AppArmorBetaProfileRuntimeDefault, true}, - {v1.AppArmorBetaProfileNameUnconfined, true}, - {"baz", false}, // Missing local prefix. - {v1.AppArmorBetaProfileNamePrefix + "/usr/sbin/ntpd", true}, - {v1.AppArmorBetaProfileNamePrefix + "foo-bar", true}, - } - - for _, test := range tests { - err := ValidateProfileFormat(test.profile) - if test.expectValid { - assert.NoError(t, err, "Profile %s should be valid", test.profile) - } else { - assert.Error(t, err, fmt.Sprintf("Profile %s should not be valid", test.profile)) - } - } -} - func TestValidateBadHost(t *testing.T) { hostErr := errors.New("expected host error") v := &validator{ @@ -109,6 +86,8 @@ func TestValidateValidHost(t *testing.T) { {v1.AppArmorBetaProfileNamePrefix + "foo-container", true}, {v1.AppArmorBetaProfileNamePrefix + "/usr/sbin/ntpd", true}, {"docker-default", false}, + {v1.AppArmorBetaProfileNamePrefix + "", false}, // Empty profile explicitly forbidden. + {v1.AppArmorBetaProfileNamePrefix + " ", false}, } for _, test := range tests {