From bf3c8464baebdf9293145c0d028663c10500cc93 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Wed, 21 Feb 2024 11:18:44 -0800 Subject: [PATCH] Implement Kubelet AppArmor field handling --- pkg/kubelet/kuberuntime/helpers.go | 34 ++++++ pkg/kubelet/kuberuntime/helpers_test.go | 73 ++++++++++++ pkg/kubelet/kuberuntime/security_context.go | 6 +- pkg/security/apparmor/helpers.go | 71 +++++++++-- pkg/security/apparmor/helpers_test.go | 124 ++++++++++++++++++++ pkg/security/apparmor/validate.go | 13 +- pkg/security/apparmor/validate_test.go | 1 - 7 files changed, 304 insertions(+), 18 deletions(-) create mode 100644 pkg/security/apparmor/helpers_test.go diff --git a/pkg/kubelet/kuberuntime/helpers.go b/pkg/kubelet/kuberuntime/helpers.go index a87e46ed770..2d82a630f89 100644 --- a/pkg/kubelet/kuberuntime/helpers.go +++ b/pkg/kubelet/kuberuntime/helpers.go @@ -18,6 +18,7 @@ package kuberuntime import ( "context" + "errors" "fmt" "path/filepath" "strconv" @@ -28,6 +29,7 @@ import ( runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1" "k8s.io/klog/v2" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" + "k8s.io/kubernetes/pkg/security/apparmor" ) type podsByID []*kubecontainer.Pod @@ -285,3 +287,35 @@ func (m *kubeGenericRuntimeManager) getSeccompProfile(annotations map[string]str ProfileType: runtimeapi.SecurityProfile_Unconfined, }, nil } + +func getAppArmorProfile(pod *v1.Pod, container *v1.Container) (*runtimeapi.SecurityProfile, error) { + profile := apparmor.GetProfile(pod, container) + if profile == nil { + return nil, nil + } + + switch profile.Type { + case v1.AppArmorProfileTypeRuntimeDefault: + return &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_RuntimeDefault, + }, nil + + case v1.AppArmorProfileTypeUnconfined: + return &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_Unconfined, + }, nil + + case v1.AppArmorProfileTypeLocalhost: + if profile.LocalhostProfile == nil { + return nil, errors.New("missing localhost apparmor profile name") + } + return &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_Localhost, + LocalhostRef: *profile.LocalhostProfile, + }, nil + + default: + // Shouldn't happen. + return nil, fmt.Errorf("unknown apparmor profile type: %q", profile.Type) + } +} diff --git a/pkg/kubelet/kuberuntime/helpers_test.go b/pkg/kubelet/kuberuntime/helpers_test.go index b2032d0d99e..8eb7ded1a70 100644 --- a/pkg/kubelet/kuberuntime/helpers_test.go +++ b/pkg/kubelet/kuberuntime/helpers_test.go @@ -31,6 +31,7 @@ import ( runtimetesting "k8s.io/cri-api/pkg/apis/testing" "k8s.io/kubernetes/pkg/features" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" + "k8s.io/utils/ptr" ) type podStatusProviderFunc func(uid types.UID, name, namespace string) (*kubecontainer.PodStatus, error) @@ -363,3 +364,75 @@ func TestToKubeContainerState(t *testing.T) { }) } } + +func TestGetAppArmorProfile(t *testing.T) { + tests := []struct { + name string + podProfile *v1.AppArmorProfile + expectedProfile *runtimeapi.SecurityProfile + expectError bool + }{{ + name: "no appArmor", + expectedProfile: nil, + }, { + name: "runtime default", + podProfile: &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeRuntimeDefault}, + expectedProfile: &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_RuntimeDefault, + }, + }, { + name: "unconfined", + podProfile: &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeUnconfined}, + expectedProfile: &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_Unconfined, + }, + }, { + name: "localhost", + podProfile: &v1.AppArmorProfile{ + Type: v1.AppArmorProfileTypeLocalhost, + LocalhostProfile: ptr.To("test"), + }, + expectedProfile: &runtimeapi.SecurityProfile{ + ProfileType: runtimeapi.SecurityProfile_Localhost, + LocalhostRef: "test", + }, + }, { + name: "invalid localhost", + podProfile: &v1.AppArmorProfile{ + Type: v1.AppArmorProfileTypeLocalhost, + }, + expectError: true, + }, { + name: "invalid type", + podProfile: &v1.AppArmorProfile{ + Type: "foo", + }, + expectError: true, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + pod := v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + }, + Spec: v1.PodSpec{ + SecurityContext: &v1.PodSecurityContext{ + AppArmorProfile: test.podProfile, + }, + Containers: []v1.Container{{Name: "foo"}}, + }, + } + + actual, err := getAppArmorProfile(&pod, &pod.Spec.Containers[0]) + + if test.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, test.expectedProfile, actual) + }) + } +} diff --git a/pkg/kubelet/kuberuntime/security_context.go b/pkg/kubelet/kuberuntime/security_context.go index 7db21ed74f0..e7cde1e38a0 100644 --- a/pkg/kubelet/kuberuntime/security_context.go +++ b/pkg/kubelet/kuberuntime/security_context.go @@ -20,7 +20,6 @@ import ( v1 "k8s.io/api/core/v1" runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1" runtimeutil "k8s.io/kubernetes/pkg/kubelet/kuberuntime/util" - "k8s.io/kubernetes/pkg/security/apparmor" "k8s.io/kubernetes/pkg/securitycontext" ) @@ -42,7 +41,10 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po } // set ApparmorProfile. - synthesized.ApparmorProfile = apparmor.GetProfileNameFromPodAnnotations(pod.Annotations, container.Name) + synthesized.Apparmor, err = getAppArmorProfile(pod, container) + if err != nil { + return nil, err + } // set RunAsUser. if synthesized.RunAsUser == nil { diff --git a/pkg/security/apparmor/helpers.go b/pkg/security/apparmor/helpers.go index 55203a470db..148a70019ee 100644 --- a/pkg/security/apparmor/helpers.go +++ b/pkg/security/apparmor/helpers.go @@ -19,11 +19,29 @@ package apparmor import ( "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" + podutil "k8s.io/kubernetes/pkg/api/v1/pod" ) -// Checks whether app armor is required for pod to be run. +// Checks whether app armor is required for the pod to run. AppArmor is considered required if any +// non-unconfined profiles are specified. func isRequired(pod *v1.Pod) bool { + if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.AppArmorProfile != nil && + pod.Spec.SecurityContext.AppArmorProfile.Type != v1.AppArmorProfileTypeUnconfined { + return true + } + + inUse := !podutil.VisitContainers(&pod.Spec, podutil.AllContainers, func(c *v1.Container, _ podutil.ContainerType) bool { + if c.SecurityContext != nil && c.SecurityContext.AppArmorProfile != nil && + c.SecurityContext.AppArmorProfile.Type != v1.AppArmorProfileTypeUnconfined { + return false // is in use; short-circuit + } + return true + }) + if inUse { + return true + } + for key, value := range pod.Annotations { if strings.HasPrefix(key, v1.AppArmorBetaContainerAnnotationKeyPrefix) { return value != v1.AppArmorBetaProfileNameUnconfined @@ -33,12 +51,49 @@ func isRequired(pod *v1.Pod) bool { } // GetProfileName returns the name of the profile to use with the container. -func GetProfileName(pod *v1.Pod, containerName string) string { - return GetProfileNameFromPodAnnotations(pod.Annotations, containerName) +func GetProfile(pod *v1.Pod, container *v1.Container) *v1.AppArmorProfile { + if container.SecurityContext != nil && container.SecurityContext.AppArmorProfile != nil { + return container.SecurityContext.AppArmorProfile + } + + // Static pods may not have had annotations synced to fields, so fallback to annotations before + // the pod profile. + if profile := getProfileFromPodAnnotations(pod.Annotations, container.Name); profile != nil { + return profile + } + + if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.AppArmorProfile != nil { + return pod.Spec.SecurityContext.AppArmorProfile + } + + return nil } -// GetProfileNameFromPodAnnotations gets the name of the profile to use with container from -// pod annotations -func GetProfileNameFromPodAnnotations(annotations map[string]string, containerName string) string { - return annotations[v1.AppArmorBetaContainerAnnotationKeyPrefix+containerName] +// getProfileFromPodAnnotations gets the AppArmor profile to use with container from +// (deprecated) pod annotations. +func getProfileFromPodAnnotations(annotations map[string]string, containerName string) *v1.AppArmorProfile { + val, ok := annotations[v1.AppArmorBetaContainerAnnotationKeyPrefix+containerName] + if !ok { + return nil + } + + switch { + case val == v1.AppArmorBetaProfileRuntimeDefault: + return &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeRuntimeDefault} + + case val == v1.AppArmorBetaProfileNameUnconfined: + return &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeUnconfined} + + case strings.HasPrefix(val, v1.AppArmorBetaProfileNamePrefix): + // Note: an invalid empty localhost profile will be rejected by kubelet admission. + profileName := strings.TrimPrefix(val, v1.AppArmorBetaProfileNamePrefix) + return &v1.AppArmorProfile{ + Type: v1.AppArmorProfileTypeLocalhost, + LocalhostProfile: &profileName, + } + + default: + // Invalid annotation. + return nil + } } diff --git a/pkg/security/apparmor/helpers_test.go b/pkg/security/apparmor/helpers_test.go new file mode 100644 index 00000000000..df9e4ff9b17 --- /dev/null +++ b/pkg/security/apparmor/helpers_test.go @@ -0,0 +1,124 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package apparmor + +import ( + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +func TestGetProfile(t *testing.T) { + runtimeDefault := &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeRuntimeDefault} + unconfined := &v1.AppArmorProfile{Type: v1.AppArmorProfileTypeUnconfined} + localhost := &v1.AppArmorProfile{ + Type: v1.AppArmorProfileTypeLocalhost, + LocalhostProfile: ptr.To("test"), + } + + tests := []struct { + name string + annotationProfile string + containerProfile *v1.AppArmorProfile + podProfile *v1.AppArmorProfile + expectedProfile *v1.AppArmorProfile + }{{ + name: "no appArmor", + expectedProfile: nil, + }, { + name: "pod profile", + podProfile: runtimeDefault, + expectedProfile: runtimeDefault, + }, { + name: "container profile", + containerProfile: unconfined, + expectedProfile: unconfined, + }, { + name: "annotation profile", + annotationProfile: v1.AppArmorBetaProfileNamePrefix + "test", + expectedProfile: localhost, + }, { + name: "invalid annotation", + annotationProfile: "invalid", + expectedProfile: nil, + }, { + name: "invalid annotation with pod field", + annotationProfile: "invalid", + podProfile: runtimeDefault, + expectedProfile: runtimeDefault, + }, { + name: "container field before annotation", + annotationProfile: v1.AppArmorBetaProfileNameUnconfined, + containerProfile: runtimeDefault, + expectedProfile: runtimeDefault, + }, { + name: "container field before pod field", + containerProfile: runtimeDefault, + podProfile: unconfined, + expectedProfile: runtimeDefault, + }, { + name: "annotation before pod field", + annotationProfile: v1.AppArmorBetaProfileNameUnconfined, + podProfile: runtimeDefault, + expectedProfile: unconfined, + }, { + name: "all profiles", + annotationProfile: v1.AppArmorBetaProfileRuntimeDefault, + containerProfile: localhost, + podProfile: unconfined, + expectedProfile: localhost, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + container := v1.Container{ + Name: "foo", + } + if test.containerProfile != nil { + container.SecurityContext = &v1.SecurityContext{ + AppArmorProfile: test.containerProfile.DeepCopy(), + } + } + pod := v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Annotations: map[string]string{ + "unrelated": "baz", + v1.AppArmorBetaContainerAnnotationKeyPrefix + "other": v1.AppArmorBetaProfileRuntimeDefault, + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{container}, + }, + } + if test.annotationProfile != "" { + pod.Annotations[v1.AppArmorBetaContainerAnnotationKeyPrefix+container.Name] = test.annotationProfile + } + if test.podProfile != nil { + pod.Spec.SecurityContext = &v1.PodSecurityContext{ + AppArmorProfile: test.podProfile.DeepCopy(), + } + } + + actual := GetProfile(&pod, &container) + assert.Equal(t, test.expectedProfile, actual) + }) + } +} diff --git a/pkg/security/apparmor/validate.go b/pkg/security/apparmor/validate.go index 23b637e535e..ef0caaea0c0 100644 --- a/pkg/security/apparmor/validate.go +++ b/pkg/security/apparmor/validate.go @@ -25,7 +25,6 @@ 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" ) @@ -62,15 +61,15 @@ 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 = validation.ValidateAppArmorProfileFormat(profile) - if retErr != nil { - return false + profile := GetProfile(pod, container) + if profile == nil { + return true } + // 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)) == "" { + if profile.Type == v1.AppArmorProfileTypeLocalhost { + if profile.LocalhostProfile == nil || strings.TrimSpace(*profile.LocalhostProfile) == "" { retErr = fmt.Errorf("invalid empty AppArmor profile name: %q", profile) return false } diff --git a/pkg/security/apparmor/validate_test.go b/pkg/security/apparmor/validate_test.go index 1c59e6a59f2..1dee7ff9071 100644 --- a/pkg/security/apparmor/validate_test.go +++ b/pkg/security/apparmor/validate_test.go @@ -64,7 +64,6 @@ func TestValidateValidHost(t *testing.T) { {v1.AppArmorBetaProfileNamePrefix + "docker-default", true}, {v1.AppArmorBetaProfileNamePrefix + "foo-container", true}, {v1.AppArmorBetaProfileNamePrefix + "/usr/sbin/ntpd", true}, - {"docker-default", false}, {v1.AppArmorBetaProfileNamePrefix + "", false}, // Empty profile explicitly forbidden. {v1.AppArmorBetaProfileNamePrefix + " ", false}, }