From f780889d4cef5f0cc53b83a07da139c9e24a1dd8 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 15 Feb 2022 13:19:02 -0800 Subject: [PATCH 1/3] Forbid empty AppArmor localhost profile --- pkg/security/apparmor/validate.go | 11 ++++++++++- pkg/security/apparmor/validate_test.go | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/security/apparmor/validate.go b/pkg/security/apparmor/validate.go index 34a0b1ee368..b800a4c0fe5 100644 --- a/pkg/security/apparmor/validate.go +++ b/pkg/security/apparmor/validate.go @@ -74,10 +74,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 = ValidateProfileFormat(profile) if retErr != nil { return false } + // TODO(#64841): This would ideally be part of ValidateProfileFormat, 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 }) diff --git a/pkg/security/apparmor/validate_test.go b/pkg/security/apparmor/validate_test.go index 03b4a487f47..818afd1a8d6 100644 --- a/pkg/security/apparmor/validate_test.go +++ b/pkg/security/apparmor/validate_test.go @@ -109,6 +109,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 { From 455f7c278cb3085b78f282c84eb50e49225d7030 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 15 Feb 2022 14:51:16 -0800 Subject: [PATCH 2/3] Add AppArmor OWNERS file --- pkg/security/apparmor/OWNERS | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 pkg/security/apparmor/OWNERS 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 From 5f2b12e0d4dc9fb358ed3b1dd74a63a2df49e950 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 15 Feb 2022 16:17:37 -0800 Subject: [PATCH 3/3] Move AppArmor profile validation to the API validation pkg --- pkg/apis/core/validation/validation.go | 13 +++++++++-- pkg/apis/core/validation/validation_test.go | 25 +++++++++++++++++++++ pkg/apis/policy/validation/validation.go | 5 ++--- pkg/security/apparmor/validate.go | 18 ++++----------- pkg/security/apparmor/validate_test.go | 23 ------------------- 5 files changed, 42 insertions(+), 42 deletions(-) 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/validate.go b/pkg/security/apparmor/validate.go index b800a4c0fe5..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" ) @@ -75,12 +76,12 @@ 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 { profile := GetProfileName(pod, container.Name) - retErr = ValidateProfileFormat(profile) + retErr = validation.ValidateAppArmorProfileFormat(profile) if retErr != nil { return false } - // TODO(#64841): This would ideally be part of ValidateProfileFormat, but that is called for - // API validation, and this is tightening validation. + // 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) @@ -117,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 818afd1a8d6..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{