Merge pull request #108143 from tallclair/apparmor

Forbid empty AppArmor localhost profile

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 {

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))
+		}
+	}
+}

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()))
 			}
 		}

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

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 {

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 {