From e1c4e85b52d92f0b85774140130d87ef0d87de69 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 29 Oct 2021 18:05:57 -0700 Subject: [PATCH 1/4] [PodSecurity] Add ValidatePod unit test --- .../admission/admission.go | 5 +- .../admission/admission_test.go | 370 +++++++++++++++++- .../pod-security-admission/api/helpers.go | 3 + .../pod-security-admission/test/fixtures.go | 8 +- .../test/fixtures_test.go | 2 +- .../k8s.io/pod-security-admission/test/run.go | 2 +- 6 files changed, 373 insertions(+), 17 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index 805761d76f4..ce8064064ec 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -149,7 +149,7 @@ func extractPodSpecFromTemplate(template *corev1.PodTemplateSpec) (*metav1.Objec return &template.ObjectMeta, &template.Spec, nil } -// CompleteConfiguration() sets up default or derived configuration. +// CompleteConfiguration sets up default or derived configuration. func (a *Admission) CompleteConfiguration() error { if a.Configuration != nil { if p, err := admissionapi.ToPolicy(a.Configuration.Defaults); err != nil { @@ -168,7 +168,7 @@ func (a *Admission) CompleteConfiguration() error { return nil } -// ValidateConfiguration() ensures all required fields are set with valid values. +// ValidateConfiguration ensures all required fields are set with valid values. func (a *Admission) ValidateConfiguration() error { if a.Configuration == nil { return fmt.Errorf("configuration required") @@ -675,6 +675,7 @@ func internalErrorResponse(msg string) *admissionv1.AdmissionResponse { // Relevant mutable pod fields as of 1.21 are image and seccomp annotations: // * https://github.com/kubernetes/kubernetes/blob/release-1.21/pkg/apis/core/validation/validation.go#L3947-L3949 func isSignificantPodUpdate(pod, oldPod *corev1.Pod) bool { + // TODO: invert this logic to only allow specific update types. if pod.Annotations[corev1.SeccompPodAnnotationKey] != oldPod.Annotations[corev1.SeccompPodAnnotationKey] { return true } diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index c3c9b0b12cb..81033c50fcb 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -18,6 +18,7 @@ package admission import ( "context" + "fmt" "math/rand" "reflect" "strings" @@ -25,20 +26,24 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/uuid" admissionapi "k8s.io/pod-security-admission/admission/api" + "k8s.io/pod-security-admission/admission/api/load" "k8s.io/pod-security-admission/api" "k8s.io/pod-security-admission/metrics" "k8s.io/pod-security-admission/policy" + "k8s.io/pod-security-admission/test" "k8s.io/utils/pointer" ) @@ -194,12 +199,14 @@ func (t *testEvaluator) EvaluatePod(lv api.LevelVersion, meta *metav1.ObjectMeta } } -type testNamespaceGetter struct { - ns *corev1.Namespace -} +type testNamespaceGetter map[string]*corev1.Namespace -func (t *testNamespaceGetter) GetNamespace(ctx context.Context, name string) (*corev1.Namespace, error) { - return t.ns, nil +func (t testNamespaceGetter) GetNamespace(ctx context.Context, name string) (*corev1.Namespace, error) { + if ns, ok := t[name]; ok { + return ns.DeepCopy(), nil + } else { + return nil, apierrors.NewNotFound(corev1.Resource("namespaces"), name) + } } type testPodLister struct { @@ -734,11 +741,10 @@ func TestValidatePodController(t *testing.T) { evaluator, err := policy.NewEvaluator(policy.DefaultChecks()) assert.NoError(t, err) nsGetter := &testNamespaceGetter{ - ns: &corev1.Namespace{ + testNamespace: &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: testName, - Namespace: testNamespace, - Labels: nsLabels}}, + Name: testName, + Labels: nsLabels}}, } PodSpecExtractor := &DefaultPodSpecExtractor{} recorder := &FakeRecorder{} @@ -782,6 +788,329 @@ func TestValidatePodController(t *testing.T) { } } +func TestValidatePod(t *testing.T) { + const ( + exemptNs = "exempt-ns" + implicitNs = "implicit-ns" + privilegedNs = "privileged-ns" + baselineNs = "baseline-ns" + baselineWarnNs = "baseline-warn-ns" + baselineAuditNs = "baseline-audit-ns" + restrictedNs = "restricted-ns" + invalidNs = "invalid-ns" + + exemptUser = "exempt-user" + exemptRuntimeClass = "exempt-runtimeclass" + ) + + objMetadata := metav1.ObjectMeta{Name: "test-pod", Labels: map[string]string{"foo": "bar"}} + + restrictedPod, err := test.GetMinimalValidPod(api.LevelRestricted, api.MajorMinorVersion(1, 23)) + require.NoError(t, err) + restrictedPod.ObjectMeta = *objMetadata.DeepCopy() + + baselinePod, err := test.GetMinimalValidPod(api.LevelBaseline, api.MajorMinorVersion(1, 23)) + require.NoError(t, err) + baselinePod.ObjectMeta = *objMetadata.DeepCopy() + + privilegedPod := *baselinePod.DeepCopy() + privilegedPod.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{ + Privileged: pointer.Bool(true), + } + + exemptRCPod := *privilegedPod.DeepCopy() + exemptRCPod.Spec.RuntimeClassName = pointer.String(exemptRuntimeClass) + + tolerantPod := *privilegedPod.DeepCopy() + tolerantPod.Spec.Tolerations = []corev1.Toleration{{ + Operator: corev1.TolerationOpExists, + }} + + differentPrivilegedPod := *privilegedPod.DeepCopy() + differentPrivilegedPod.Spec.Containers[0].Image = "https://example.com/a-different-image" + + differentRestrictedPod := *restrictedPod.DeepCopy() + differentRestrictedPod.Spec.Containers[0].Image = "https://example.com/a-different-image" + + deployment := appsv1.Deployment{ + ObjectMeta: *objMetadata.DeepCopy(), + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: *objMetadata.DeepCopy(), + Spec: *restrictedPod.Spec.DeepCopy(), + }, + }, + } + + makeNs := func(enforceLevel, warnLevel, auditLevel api.Level) *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + } + if enforceLevel != "" { + ns.Labels[api.EnforceLevelLabel] = string(enforceLevel) + } + if warnLevel != "" { + ns.Labels[api.WarnLevelLabel] = string(warnLevel) + } + if auditLevel != "" { + ns.Labels[api.AuditLevelLabel] = string(auditLevel) + } + return ns + } + nsGetter := testNamespaceGetter{ + exemptNs: makeNs(api.LevelRestricted, api.LevelRestricted, api.LevelRestricted), + implicitNs: makeNs("", "", ""), + privilegedNs: makeNs(api.LevelPrivileged, api.LevelPrivileged, api.LevelPrivileged), + baselineNs: makeNs(api.LevelBaseline, "", ""), + baselineWarnNs: makeNs("", api.LevelBaseline, ""), + baselineAuditNs: makeNs("", "", api.LevelBaseline), + restrictedNs: makeNs(api.LevelRestricted, api.LevelRestricted, api.LevelRestricted), + invalidNs: makeNs("not-a-valid-level", "", ""), + } + + config, err := load.LoadFromData(nil) // Start with the default config. + require.NoError(t, err, "loading default config") + config.Exemptions.Namespaces = []string{exemptNs} + config.Exemptions.RuntimeClasses = []string{exemptRuntimeClass} + config.Exemptions.Usernames = []string{exemptUser} + + evaluator, err := policy.NewEvaluator(policy.DefaultChecks()) + assert.NoError(t, err) + + testCases := []struct { + desc string + + namespace string + username string + runtimeClass string + + operation admissionv1.Operation + obj runtime.Object + objErr error // Error to return instead of obj by attrs.GetObject() + oldObj runtime.Object + oldObjErr error // Error to return instead of oldObj by attrs.GetOldObject() + subresource string + + expectAllowed bool + expectReason metav1.StatusReason + expectWarning bool + expectedAuditAnnotationKeys []string + }{ + { + desc: "ignored subresource", + namespace: restrictedNs, + obj: privilegedPod.DeepCopy(), + subresource: "status", + expectAllowed: true, + }, + { + desc: "exempt namespace", + namespace: exemptNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"exempt"}, + }, + { + desc: "exempt user", + namespace: restrictedNs, + username: exemptUser, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"exempt"}, + }, + { + desc: "exempt runtimeClass", + namespace: restrictedNs, + obj: exemptRCPod.DeepCopy(), + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"exempt"}, + }, + { + desc: "namespace not found", + namespace: "missing-ns", + obj: restrictedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonInternalError, + }, + { + desc: "short-circuit privileged:latest (implicit)", + namespace: implicitNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + }, + { + desc: "short-circuit privileged:latest (explicit)", + namespace: privilegedNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + }, + { + desc: "failed decode", + namespace: baselineNs, + objErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + }, + { + desc: "invalid object", + namespace: baselineNs, + obj: deployment.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + }, + { + desc: "failed decode old object", + namespace: baselineNs, + operation: admissionv1.Update, + obj: restrictedPod.DeepCopy(), + oldObjErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + }, + { + desc: "invalid old object", + namespace: baselineNs, + operation: admissionv1.Update, + obj: restrictedPod.DeepCopy(), + oldObj: deployment.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + }, + { + desc: "insignificant pod update", + namespace: restrictedNs, + operation: admissionv1.Update, + obj: tolerantPod.DeepCopy(), + oldObj: privilegedPod.DeepCopy(), + expectAllowed: true, + }, + { + desc: "significant pod update - denied", + namespace: baselineNs, + operation: admissionv1.Update, + obj: differentPrivilegedPod.DeepCopy(), + oldObj: privilegedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectedAuditAnnotationKeys: []string{"enforce-policy"}, + }, + { + desc: "significant pod update - allowed", + namespace: restrictedNs, + operation: admissionv1.Update, + obj: differentRestrictedPod.DeepCopy(), + oldObj: restrictedPod, + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"enforce-policy"}, + }, + { + desc: "invalid namespace labels", + namespace: invalidNs, + obj: baselinePod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectedAuditAnnotationKeys: []string{"enforce-policy", "error"}, + }, + { + desc: "enforce deny", + namespace: baselineNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectedAuditAnnotationKeys: []string{"enforce-policy"}, + }, + { + desc: "enforce allow", + namespace: baselineNs, + obj: baselinePod.DeepCopy(), + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"enforce-policy"}, + }, + { + desc: "warn deny", + namespace: baselineWarnNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + expectWarning: true, + expectedAuditAnnotationKeys: []string{"enforce-policy"}, + }, + { + desc: "audit deny", + namespace: baselineAuditNs, + obj: privilegedPod.DeepCopy(), + expectAllowed: true, + expectedAuditAnnotationKeys: []string{"enforce-policy", "audit-violations"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + if tc.obj != nil { + tc.obj.(metav1.ObjectMetaAccessor).GetObjectMeta().SetNamespace(tc.namespace) + } + if tc.oldObj != nil { + tc.oldObj.(metav1.ObjectMetaAccessor).GetObjectMeta().SetNamespace(tc.namespace) + } + attrs := &testAttributes{ + AttributesRecord: api.AttributesRecord{ + Name: "test-pod", + Namespace: tc.namespace, + Kind: schema.GroupVersionKind{Version: "v1", Kind: "Pod"}, + Resource: schema.GroupVersionResource{Version: "v1", Resource: "pods"}, + Subresource: tc.subresource, + Operation: admissionv1.Create, + Object: tc.obj, + OldObject: tc.oldObj, + Username: "test-user", + }, + objectErr: tc.objErr, + oldObjectErr: tc.oldObjErr, + } + if tc.operation != "" { + attrs.Operation = tc.operation + } + if tc.username != "" { + attrs.Username = tc.username + } + + a := &Admission{ + PodLister: &testPodLister{}, + Evaluator: evaluator, + Configuration: config, + Metrics: &FakeRecorder{}, + NamespaceGetter: nsGetter, + } + require.NoError(t, a.CompleteConfiguration(), "CompleteConfiguration()") + require.NoError(t, a.ValidateConfiguration(), "ValidateConfiguration()") + + response := a.Validate(context.TODO(), attrs) + + if tc.expectAllowed { + assert.True(t, response.Allowed, "Allowed") + assert.Nil(t, response.Result) + } else { + assert.False(t, response.Allowed) + if assert.NotNil(t, response.Result, "Result") { + assert.Equal(t, tc.expectReason, response.Result.Reason, "Reason") + } + } + + if tc.expectWarning { + assert.NotEmpty(t, response.Warnings, "Warnings") + } else { + assert.Empty(t, response.Warnings, "Warnings") + } + + assert.Len(t, response.AuditAnnotations, len(tc.expectedAuditAnnotationKeys), "AuditAnnotations") + for _, key := range tc.expectedAuditAnnotationKeys { + assert.Contains(t, response.AuditAnnotations, key, "AuditAnnotations") + } + }) + } +} + type FakeRecorder struct { evaluations []EvaluationRecord } @@ -862,3 +1191,26 @@ func TestPrioritizePods(t *testing.T) { assert.Fail(t, "Pod count is not the same after prioritization") } } + +type testAttributes struct { + api.AttributesRecord + + objectErr error + oldObjectErr error +} + +func (a *testAttributes) GetObject() (runtime.Object, error) { + if a.objectErr != nil { + return nil, a.objectErr + } else { + return a.AttributesRecord.GetObject() + } +} + +func (a *testAttributes) GetOldObject() (runtime.Object, error) { + if a.oldObjectErr != nil { + return nil, a.oldObjectErr + } else { + return a.AttributesRecord.GetOldObject() + } +} diff --git a/staging/src/k8s.io/pod-security-admission/api/helpers.go b/staging/src/k8s.io/pod-security-admission/api/helpers.go index 5e309fddc2c..d46670de6c5 100644 --- a/staging/src/k8s.io/pod-security-admission/api/helpers.go +++ b/staging/src/k8s.io/pod-security-admission/api/helpers.go @@ -161,6 +161,9 @@ func PolicyToEvaluate(labels map[string]string, defaults Policy) (Policy, field. p = defaults ) + if len(labels) == 0 { + return p, nil + } if level, ok := labels[EnforceLevelLabel]; ok { p.Enforce.Level, err = ParseLevel(level) errs = appendErr(errs, err, EnforceLevelLabel, level) diff --git a/staging/src/k8s.io/pod-security-admission/test/fixtures.go b/staging/src/k8s.io/pod-security-admission/test/fixtures.go index 33d801c1c9d..cd11f0d380a 100644 --- a/staging/src/k8s.io/pod-security-admission/test/fixtures.go +++ b/staging/src/k8s.io/pod-security-admission/test/fixtures.go @@ -51,7 +51,7 @@ func init() { }) minimalValidPods[api.LevelRestricted][api.MajorMinorVersion(1, 0)] = restricted_1_0 - // 1.8+: runAsNonRoot=true + // 1.8+: allowPrivilegeEscalation=false restricted_1_8 := tweak(restricted_1_0, func(p *corev1.Pod) { p.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{AllowPrivilegeEscalation: pointer.BoolPtr(false)} p.Spec.InitContainers[0].SecurityContext = &corev1.SecurityContext{AllowPrivilegeEscalation: pointer.BoolPtr(false)} @@ -75,8 +75,8 @@ func init() { minimalValidPods[api.LevelRestricted][api.MajorMinorVersion(1, 22)] = restricted_1_22 } -// getValidPod returns a minimal valid pod for the specified level and version. -func getMinimalValidPod(level api.Level, version api.Version) (*corev1.Pod, error) { +// GetMinimalValidPod returns a minimal valid pod for the specified level and version. +func GetMinimalValidPod(level api.Level, version api.Version) (*corev1.Pod, error) { originalVersion := version for { pod, exists := minimalValidPods[level][version] @@ -169,7 +169,7 @@ func getFixtures(key fixtureKey) (fixtureData, error) { return fixtureData{}, err } - validPodForLevel, err := getMinimalValidPod(key.level, key.version) + validPodForLevel, err := GetMinimalValidPod(key.level, key.version) if err != nil { return fixtureData{}, err } diff --git a/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go b/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go index 86ded471653..b6a17b89b3d 100644 --- a/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go +++ b/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go @@ -61,7 +61,7 @@ func TestFixtures(t *testing.T) { failDir := filepath.Join("testdata", string(level), fmt.Sprintf("v1.%d", version), "fail") // render the minimal valid pod fixture - validPod, err := getMinimalValidPod(level, api.MajorMinorVersion(1, version)) + validPod, err := GetMinimalValidPod(level, api.MajorMinorVersion(1, version)) if err != nil { t.Fatal(err) } diff --git a/staging/src/k8s.io/pod-security-admission/test/run.go b/staging/src/k8s.io/pod-security-admission/test/run.go index 311203d60f1..030fe808c5b 100644 --- a/staging/src/k8s.io/pod-security-admission/test/run.go +++ b/staging/src/k8s.io/pod-security-admission/test/run.go @@ -296,7 +296,7 @@ func Run(t *testing.T, opts Options) { } } - minimalValidPod, err := getMinimalValidPod(level, version) + minimalValidPod, err := GetMinimalValidPod(level, version) if err != nil { t.Fatal(err) } From 98c86b350c6599c8247a7875aa899306ebd979d6 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 29 Oct 2021 21:52:40 -0700 Subject: [PATCH 2/4] [PodSecurity] Errors validating PodControllers are non blocking --- .../admission/admission.go | 18 +- .../admission/admission_test.go | 447 +++++++----------- 2 files changed, 176 insertions(+), 289 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index ce8064064ec..3beb36660dd 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -399,7 +399,11 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu if err != nil { klog.ErrorS(err, "failed to fetch pod namespace", "namespace", attrs.GetNamespace()) a.Metrics.RecordError(true, attrs) - return internalErrorResponse(fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace())) + response := allowedResponse() + response.AuditAnnotations = map[string]string{ + "error": fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace()), + } + return response } nsPolicy, nsPolicyErrs := a.PolicyToEvaluate(namespace.Labels) if len(nsPolicyErrs) == 0 && nsPolicy.Warn.Level == api.LevelPrivileged && nsPolicy.Audit.Level == api.LevelPrivileged { @@ -410,13 +414,21 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu if err != nil { klog.ErrorS(err, "failed to decode object") a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to decode object") + response := allowedResponse() + response.AuditAnnotations = map[string]string{ + "error": "failed to decode object", + } + return response } podMetadata, podSpec, err := a.PodSpecExtractor.ExtractPodSpec(obj) if err != nil { klog.ErrorS(err, "failed to extract pod spec") a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to extract pod template") + response := allowedResponse() + response.AuditAnnotations = map[string]string{ + "error": "failed to extract pod template", + } + return response } if podMetadata == nil && podSpec == nil { // if a controller with an optional pod spec does not contain a pod spec, skip validation diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index 81033c50fcb..6cca43869fc 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -589,206 +589,7 @@ func TestValidateNamespace(t *testing.T) { } } -func TestValidatePodController(t *testing.T) { - testName, testNamespace := "testname", "default" - objMetadata := metav1.ObjectMeta{Name: testName, Namespace: testNamespace, Labels: map[string]string{"foo": "bar"}} - // One of the pod-template objects - goodDeploy := appsv1.Deployment{ - ObjectMeta: objMetadata, - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: objMetadata, - Spec: corev1.PodSpec{ - RuntimeClassName: pointer.String("containerd"), - }, - }, - }, - } - badDeploy := appsv1.Deployment{ - ObjectMeta: objMetadata, - Spec: appsv1.DeploymentSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, - Template: corev1.PodTemplateSpec{ - ObjectMeta: objMetadata, - Spec: corev1.PodSpec{ - SecurityContext: &corev1.PodSecurityContext{ - // out of allowed sysctls to return auditAnnotation or warning - Sysctls: []corev1.Sysctl{{Name: "unknown", Value: "unknown"}}, - }, - RuntimeClassName: pointer.String("containerd"), - }, - }, - }, - } - - // Ensure that under the baseline policy, - // the pod-template object of all tests returns correct information or is exempted - nsLabels := map[string]string{ - api.EnforceLevelLabel: string(api.LevelBaseline), - api.WarnLevelLabel: string(api.LevelBaseline), - api.AuditLevelLabel: string(api.LevelBaseline), - } - nsLevelVersion := api.LevelVersion{api.LevelBaseline, api.LatestVersion()} - - testCases := []struct { - desc string - exemptNamespaces []string - exemptRuntimeClasses []string - exemptUsers []string - // request subresource - subresource string - // for create - newObject runtime.Object - // for update - oldObject runtime.Object - gvk schema.GroupVersionKind - gvr schema.GroupVersionResource - - expectWarnings []string - expectAuditAnnotations map[string]string - }{ - { - desc: "subresource(status) updates don't produce warnings", - subresource: "status", - newObject: &badDeploy, - oldObject: &goodDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - }, - { - desc: "namespace in exemptNamespaces will be exempted", - newObject: &badDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - exemptNamespaces: []string{testNamespace}, - expectAuditAnnotations: map[string]string{"exempt": "namespace"}, - }, - { - desc: "runtimeClass in exemptRuntimeClasses will be exempted", - newObject: &badDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - exemptRuntimeClasses: []string{"containerd"}, - expectAuditAnnotations: map[string]string{"exempt": "runtimeClass"}, - }, - { - desc: "user in exemptUsers will be exempted", - newObject: &badDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - exemptUsers: []string{"testuser"}, - expectAuditAnnotations: map[string]string{"exempt": "user"}, - }, - { - desc: "podMetadata == nil && podSpec == nil will skip verification", - newObject: &corev1.ReplicationController{ObjectMeta: metav1.ObjectMeta{Name: "foo-rc"}}, - gvk: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ReplicationController"}, - gvr: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "replicationcontrollers"}, - }, - { - desc: "good deploy creates and produce nothing", - newObject: &goodDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - expectAuditAnnotations: map[string]string{}, - }, - { - desc: "bad deploy creates produce correct user-visible warnings and correct auditAnnotations", - newObject: &badDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - expectAuditAnnotations: map[string]string{"audit-violations": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - expectWarnings: []string{"would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - }, - { - desc: "bad spec updates don't block on enforce failures and returns correct information", - newObject: &badDeploy, - oldObject: &goodDeploy, - gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - gvr: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, - expectAuditAnnotations: map[string]string{"audit-violations": "would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - expectWarnings: []string{"would violate PodSecurity \"baseline:latest\": forbidden sysctls (unknown)"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - var operation = admissionv1.Create - if tc.oldObject != nil { - operation = admissionv1.Update - } - - attrs := &api.AttributesRecord{ - testName, - testNamespace, - tc.gvk, - tc.gvr, - tc.subresource, - operation, - tc.newObject, - tc.oldObject, - "testuser", - } - - defaultPolicy := api.Policy{ - Enforce: api.LevelVersion{Level: api.LevelPrivileged, Version: api.LatestVersion()}, - Audit: api.LevelVersion{Level: api.LevelPrivileged, Version: api.LatestVersion()}, - Warn: api.LevelVersion{Level: api.LevelPrivileged, Version: api.LatestVersion()}, - } - - podLister := &testPodLister{} - evaluator, err := policy.NewEvaluator(policy.DefaultChecks()) - assert.NoError(t, err) - nsGetter := &testNamespaceGetter{ - testNamespace: &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: testName, - Labels: nsLabels}}, - } - PodSpecExtractor := &DefaultPodSpecExtractor{} - recorder := &FakeRecorder{} - a := &Admission{ - PodLister: podLister, - Evaluator: evaluator, - PodSpecExtractor: PodSpecExtractor, - Configuration: &admissionapi.PodSecurityConfiguration{ - Exemptions: admissionapi.PodSecurityExemptions{ - Namespaces: tc.exemptNamespaces, - RuntimeClasses: tc.exemptRuntimeClasses, - Usernames: tc.exemptUsers, - }, - }, - Metrics: recorder, - defaultPolicy: defaultPolicy, - NamespaceGetter: nsGetter, - } - - result := a.ValidatePodController(context.TODO(), attrs) - // podContorller will not return an error due to correct evaluation - resultError := "" - if result.Result != nil { - resultError = result.Result.Message - } - - assert.Equal(t, true, result.Allowed) - assert.Empty(t, resultError) - assert.Equal(t, tc.expectAuditAnnotations, result.AuditAnnotations, "unexpected AuditAnnotations") - assert.Equal(t, tc.expectWarnings, result.Warnings, "unexpected Warnings") - - expectedEvaluations := []EvaluationRecord{} - if _, ok := tc.expectAuditAnnotations["audit-violations"]; ok { - expectedEvaluations = append(expectedEvaluations, EvaluationRecord{testName, metrics.DecisionDeny, nsLevelVersion, metrics.ModeAudit}) - } - if len(tc.expectWarnings) > 0 { - expectedEvaluations = append(expectedEvaluations, EvaluationRecord{testName, metrics.DecisionDeny, nsLevelVersion, metrics.ModeWarn}) - } - recorder.ExpectEvaluations(t, expectedEvaluations) - }) - } -} - -func TestValidatePod(t *testing.T) { +func TestValidatePodAndController(t *testing.T) { const ( exemptNs = "exempt-ns" implicitNs = "implicit-ns" @@ -832,14 +633,9 @@ func TestValidatePod(t *testing.T) { differentRestrictedPod := *restrictedPod.DeepCopy() differentRestrictedPod.Spec.Containers[0].Image = "https://example.com/a-different-image" - deployment := appsv1.Deployment{ + emptyDeployment := appsv1.Deployment{ ObjectMeta: *objMetadata.DeepCopy(), - Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - ObjectMeta: *objMetadata.DeepCopy(), - Spec: *restrictedPod.Spec.DeepCopy(), - }, - }, + Spec: appsv1.DeploymentSpec{}, } makeNs := func(enforceLevel, warnLevel, auditLevel api.Level) *corev1.Namespace { @@ -863,10 +659,10 @@ func TestValidatePod(t *testing.T) { exemptNs: makeNs(api.LevelRestricted, api.LevelRestricted, api.LevelRestricted), implicitNs: makeNs("", "", ""), privilegedNs: makeNs(api.LevelPrivileged, api.LevelPrivileged, api.LevelPrivileged), - baselineNs: makeNs(api.LevelBaseline, "", ""), + baselineNs: makeNs(api.LevelBaseline, api.LevelBaseline, api.LevelBaseline), baselineWarnNs: makeNs("", api.LevelBaseline, ""), baselineAuditNs: makeNs("", "", api.LevelBaseline), - restrictedNs: makeNs(api.LevelRestricted, api.LevelRestricted, api.LevelRestricted), + restrictedNs: makeNs(api.LevelRestricted, "", api.LevelRestricted), invalidNs: makeNs("not-a-valid-level", "", ""), } @@ -879,36 +675,56 @@ func TestValidatePod(t *testing.T) { evaluator, err := policy.NewEvaluator(policy.DefaultChecks()) assert.NoError(t, err) - testCases := []struct { + a := &Admission{ + PodLister: &testPodLister{}, + Evaluator: evaluator, + Configuration: config, + Metrics: &FakeRecorder{}, + NamespaceGetter: nsGetter, + } + require.NoError(t, a.CompleteConfiguration(), "CompleteConfiguration()") + require.NoError(t, a.ValidateConfiguration(), "ValidateConfiguration()") + + type testCase struct { desc string namespace string username string runtimeClass string - operation admissionv1.Operation + operation admissionv1.Operation + pod *corev1.Pod + oldPod *corev1.Pod + + resource schema.GroupVersionResource + kind schema.GroupVersionKind obj runtime.Object - objErr error // Error to return instead of obj by attrs.GetObject() oldObj runtime.Object + objErr error // Error to return instead of obj by attrs.GetObject() oldObjErr error // Error to return instead of oldObj by attrs.GetOldObject() subresource string + skipPod bool // Whether to skip the ValidatePod test case. + skipDeployment bool // Whteher to skip the ValidatePodController test case. + expectAllowed bool expectReason metav1.StatusReason expectWarning bool + expectEnforce bool // Whether to expect an enforcing evaluation (metric+annotation) expectedAuditAnnotationKeys []string - }{ + } + podCases := []testCase{ { desc: "ignored subresource", namespace: restrictedNs, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), subresource: "status", expectAllowed: true, }, { desc: "exempt namespace", namespace: exemptNs, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), expectAllowed: true, expectedAuditAnnotationKeys: []string{"exempt"}, }, @@ -916,34 +732,34 @@ func TestValidatePod(t *testing.T) { desc: "exempt user", namespace: restrictedNs, username: exemptUser, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), expectAllowed: true, expectedAuditAnnotationKeys: []string{"exempt"}, }, { desc: "exempt runtimeClass", namespace: restrictedNs, - obj: exemptRCPod.DeepCopy(), + pod: exemptRCPod.DeepCopy(), expectAllowed: true, expectedAuditAnnotationKeys: []string{"exempt"}, }, { desc: "namespace not found", namespace: "missing-ns", - obj: restrictedPod.DeepCopy(), + pod: restrictedPod.DeepCopy(), expectAllowed: false, expectReason: metav1.StatusReasonInternalError, }, { desc: "short-circuit privileged:latest (implicit)", namespace: implicitNs, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), expectAllowed: true, }, { desc: "short-circuit privileged:latest (explicit)", namespace: privilegedNs, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), expectAllowed: true, }, { @@ -956,93 +772,162 @@ func TestValidatePod(t *testing.T) { { desc: "invalid object", namespace: baselineNs, - obj: deployment.DeepCopy(), + operation: admissionv1.Update, + obj: &corev1.Namespace{}, expectAllowed: false, expectReason: metav1.StatusReasonBadRequest, }, { - desc: "failed decode old object", - namespace: baselineNs, - operation: admissionv1.Update, - obj: restrictedPod.DeepCopy(), - oldObjErr: fmt.Errorf("expected (failed decode)"), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, + desc: "failed decode old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObjErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "invalid old object", - namespace: baselineNs, - operation: admissionv1.Update, - obj: restrictedPod.DeepCopy(), - oldObj: deployment.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, + desc: "invalid old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObj: &corev1.Namespace{}, + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "insignificant pod update", - namespace: restrictedNs, - operation: admissionv1.Update, - obj: tolerantPod.DeepCopy(), - oldObj: privilegedPod.DeepCopy(), - expectAllowed: true, + desc: "insignificant update", + namespace: restrictedNs, + operation: admissionv1.Update, + pod: tolerantPod.DeepCopy(), + oldPod: privilegedPod.DeepCopy(), + expectAllowed: true, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "significant pod update - denied", - namespace: baselineNs, - operation: admissionv1.Update, - obj: differentPrivilegedPod.DeepCopy(), - oldObj: privilegedPod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonForbidden, - expectedAuditAnnotationKeys: []string{"enforce-policy"}, - }, - { - desc: "significant pod update - allowed", + desc: "significant update denied", namespace: restrictedNs, operation: admissionv1.Update, - obj: differentRestrictedPod.DeepCopy(), - oldObj: restrictedPod, - expectAllowed: true, - expectedAuditAnnotationKeys: []string{"enforce-policy"}, + pod: differentPrivilegedPod.DeepCopy(), + oldPod: privilegedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectEnforce: true, + expectedAuditAnnotationKeys: []string{"audit-violations"}, + }, + { + desc: "significant update allowed", + namespace: restrictedNs, + operation: admissionv1.Update, + pod: differentRestrictedPod.DeepCopy(), + oldPod: restrictedPod, + expectAllowed: true, + expectEnforce: true, }, { desc: "invalid namespace labels", namespace: invalidNs, - obj: baselinePod.DeepCopy(), + pod: baselinePod.DeepCopy(), expectAllowed: false, expectReason: metav1.StatusReasonForbidden, - expectedAuditAnnotationKeys: []string{"enforce-policy", "error"}, + expectEnforce: true, + expectedAuditAnnotationKeys: []string{"error"}, }, { desc: "enforce deny", - namespace: baselineNs, - obj: privilegedPod.DeepCopy(), + namespace: restrictedNs, + pod: privilegedPod.DeepCopy(), expectAllowed: false, expectReason: metav1.StatusReasonForbidden, - expectedAuditAnnotationKeys: []string{"enforce-policy"}, + expectEnforce: true, + expectedAuditAnnotationKeys: []string{"audit-violations"}, }, { - desc: "enforce allow", - namespace: baselineNs, - obj: baselinePod.DeepCopy(), - expectAllowed: true, - expectedAuditAnnotationKeys: []string{"enforce-policy"}, + desc: "enforce allow", + namespace: baselineNs, + pod: baselinePod.DeepCopy(), + expectAllowed: true, + expectEnforce: true, }, { - desc: "warn deny", - namespace: baselineWarnNs, - obj: privilegedPod.DeepCopy(), - expectAllowed: true, - expectWarning: true, - expectedAuditAnnotationKeys: []string{"enforce-policy"}, + desc: "warn deny", + namespace: baselineWarnNs, + pod: privilegedPod.DeepCopy(), + expectAllowed: true, + expectWarning: true, + expectEnforce: true, }, { desc: "audit deny", namespace: baselineAuditNs, - obj: privilegedPod.DeepCopy(), + pod: privilegedPod.DeepCopy(), expectAllowed: true, - expectedAuditAnnotationKeys: []string{"enforce-policy", "audit-violations"}, + expectEnforce: true, + expectedAuditAnnotationKeys: []string{"audit-violations"}, }, + { + desc: "no pod template", + namespace: restrictedNs, + obj: emptyDeployment.DeepCopy(), + expectAllowed: true, + expectWarning: false, // No pod template skips validation. + skipPod: true, + }, + } + + podToDeployment := func(pod *corev1.Pod) *appsv1.Deployment { + if pod == nil { + return nil + } + return &appsv1.Deployment{ + ObjectMeta: pod.ObjectMeta, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: pod.ObjectMeta, + Spec: pod.Spec, + }, + }, + } + } + + // Convert "pod cases" into pod test cases & deployment test cases. + testCases := []testCase{} + for _, tc := range podCases { + podTest := tc + podTest.desc = "pod:" + tc.desc + podTest.resource = schema.GroupVersionResource{Version: "v1", Resource: "pods"} + podTest.kind = schema.GroupVersionKind{Version: "v1", Kind: "Pod"} + if tc.expectEnforce { + podTest.expectedAuditAnnotationKeys = append(podTest.expectedAuditAnnotationKeys, "enforce-policy") + } + + deploymentTest := tc + deploymentTest.desc = "deployment:" + tc.desc + deploymentTest.resource = schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + deploymentTest.kind = schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} + deploymentTest.expectAllowed = true // Deployments policies are non-enforcing. + deploymentTest.expectReason = "" + if tc.expectReason != "" && tc.expectReason != metav1.StatusReasonForbidden { + // Error case, expect an error annotation. + deploymentTest.expectedAuditAnnotationKeys = append(deploymentTest.expectedAuditAnnotationKeys, "error") + } + + if tc.pod != nil { + podTest.obj = tc.pod + deploymentTest.obj = podToDeployment(tc.pod) + } + if tc.oldPod != nil { + podTest.oldObj = tc.oldPod + deploymentTest.oldObj = podToDeployment(tc.oldPod) + } + if !tc.skipPod { + testCases = append(testCases, podTest) + } + if !tc.skipDeployment { + testCases = append(testCases, deploymentTest) + } } for _, tc := range testCases { @@ -1057,8 +942,8 @@ func TestValidatePod(t *testing.T) { AttributesRecord: api.AttributesRecord{ Name: "test-pod", Namespace: tc.namespace, - Kind: schema.GroupVersionKind{Version: "v1", Kind: "Pod"}, - Resource: schema.GroupVersionResource{Version: "v1", Resource: "pods"}, + Kind: tc.kind, + Resource: tc.resource, Subresource: tc.subresource, Operation: admissionv1.Create, Object: tc.obj, @@ -1075,16 +960,6 @@ func TestValidatePod(t *testing.T) { attrs.Username = tc.username } - a := &Admission{ - PodLister: &testPodLister{}, - Evaluator: evaluator, - Configuration: config, - Metrics: &FakeRecorder{}, - NamespaceGetter: nsGetter, - } - require.NoError(t, a.CompleteConfiguration(), "CompleteConfiguration()") - require.NoError(t, a.ValidateConfiguration(), "ValidateConfiguration()") - response := a.Validate(context.TODO(), attrs) if tc.expectAllowed { From c3398729e09a3362b8c33978126b2e0713662241 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 29 Oct 2021 22:09:17 -0700 Subject: [PATCH 3/4] [PodSecurity] Include error audit annotation on all non-forbidden errors --- .../admission/admission.go | 101 +++--------------- .../admission/admission_test.go | 87 +++++++-------- .../admission/response.go | 93 ++++++++++++++++ 3 files changed, 152 insertions(+), 129 deletions(-) create mode 100644 staging/src/k8s.io/pod-security-admission/admission/response.go diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index 3beb36660dd..e96c1a0bbd7 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -19,7 +19,6 @@ package admission import ( "context" "fmt" - "net/http" "reflect" "sort" "time" @@ -233,13 +232,13 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) } obj, err := attrs.GetObject() if err != nil { - klog.ErrorS(err, "failed to get object") - return internalErrorResponse("failed to get object") + klog.ErrorS(err, "failed to decode object") + return errorResponse(err, &apierrors.NewBadRequest("failed to decode object").ErrStatus) } namespace, ok := obj.(*corev1.Namespace) if !ok { klog.InfoS("failed to assert namespace type", "type", reflect.TypeOf(obj)) - return badRequestResponse("failed to decode namespace") + return errorResponse(nil, &apierrors.NewBadRequest("failed to decode namespace").ErrStatus) } newPolicy, newErrs := a.PolicyToEvaluate(namespace.Labels) @@ -257,12 +256,12 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) oldObj, err := attrs.GetOldObject() if err != nil { klog.ErrorS(err, "failed to decode old object") - return badRequestResponse("failed to decode old object") + return errorResponse(err, &apierrors.NewBadRequest("failed to decode old object").ErrStatus) } oldNamespace, ok := oldObj.(*corev1.Namespace) if !ok { klog.InfoS("failed to assert old namespace type", "type", reflect.TypeOf(oldObj)) - return badRequestResponse("failed to decode old namespace") + return errorResponse(nil, &apierrors.NewBadRequest("failed to decode old namespace").ErrStatus) } oldPolicy, oldErrs := a.PolicyToEvaluate(oldNamespace.Labels) @@ -335,7 +334,7 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi if err != nil { klog.ErrorS(err, "failed to fetch pod namespace", "namespace", attrs.GetNamespace()) a.Metrics.RecordError(true, attrs) - return internalErrorResponse(fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace())) + return errorResponse(err, &apierrors.NewInternalError(fmt.Errorf("failed to lookup namespace %s", attrs.GetNamespace())).ErrStatus) } nsPolicy, nsPolicyErrs := a.PolicyToEvaluate(namespace.Labels) if len(nsPolicyErrs) == 0 && nsPolicy.Enforce.Level == api.LevelPrivileged && nsPolicy.Warn.Level == api.LevelPrivileged && nsPolicy.Audit.Level == api.LevelPrivileged { @@ -347,26 +346,26 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi if err != nil { klog.ErrorS(err, "failed to decode object") a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to decode object") + return errorResponse(err, &apierrors.NewBadRequest("failed to decode object").ErrStatus) } pod, ok := obj.(*corev1.Pod) if !ok { klog.InfoS("failed to assert pod type", "type", reflect.TypeOf(obj)) a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to decode pod") + return errorResponse(nil, &apierrors.NewBadRequest("failed to decode pod").ErrStatus) } if attrs.GetOperation() == admissionv1.Update { oldObj, err := attrs.GetOldObject() if err != nil { klog.ErrorS(err, "failed to decode old object") a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to decode old object") + return errorResponse(err, &apierrors.NewBadRequest("failed to decode old object").ErrStatus) } oldPod, ok := oldObj.(*corev1.Pod) if !ok { klog.InfoS("failed to assert old pod type", "type", reflect.TypeOf(oldObj)) a.Metrics.RecordError(true, attrs) - return badRequestResponse("failed to decode old pod") + return errorResponse(nil, &apierrors.NewBadRequest("failed to decode old pod").ErrStatus) } if !isSignificantPodUpdate(pod, oldPod) { // Nothing we care about changed, so always allow the update. @@ -401,7 +400,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu a.Metrics.RecordError(true, attrs) response := allowedResponse() response.AuditAnnotations = map[string]string{ - "error": fmt.Sprintf("failed to lookup namespace %s", attrs.GetNamespace()), + "error": fmt.Sprintf("failed to lookup namespace %s: %v", attrs.GetNamespace(), err), } return response } @@ -416,7 +415,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu a.Metrics.RecordError(true, attrs) response := allowedResponse() response.AuditAnnotations = map[string]string{ - "error": "failed to decode object", + "error": fmt.Sprintf("failed to decode object: %v", err), } return response } @@ -426,7 +425,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu a.Metrics.RecordError(true, attrs) response := allowedResponse() response.AuditAnnotations = map[string]string{ - "error": "failed to extract pod template", + "error": fmt.Sprintf("failed to extract pod template: %v", err), } return response } @@ -464,8 +463,8 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli result := policy.AggregateCheckResults(a.Evaluator.EvaluatePod(nsPolicy.Enforce, podMetadata, podSpec)) if !result.Allowed { - response = forbiddenResponse(fmt.Sprintf( - "pod violates PodSecurity %q: %s", + response = forbiddenResponse(attrs, fmt.Errorf( + "violates PodSecurity %q: %s", nsPolicy.Enforce.String(), result.ForbiddenDetail(), )) @@ -613,76 +612,6 @@ func (a *Admission) PolicyToEvaluate(labels map[string]string) (api.Policy, fiel return api.PolicyToEvaluate(labels, a.defaultPolicy) } -var ( - _sharedAllowedResponse = allowedResponse() - _sharedAllowedByUserExemptionResponse = allowedByExemptResponse("user") - _sharedAllowedByNamespaceExemptionResponse = allowedByExemptResponse("namespace") - _sharedAllowedByRuntimeClassExemptionResponse = allowedByExemptResponse("runtimeClass") -) - -func sharedAllowedResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedResponse -} - -func sharedAllowedByUserExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByUserExemptionResponse -} - -func sharedAllowedByNamespaceExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByNamespaceExemptionResponse -} - -func sharedAllowedByRuntimeClassExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByRuntimeClassExemptionResponse -} - -// allowedResponse is the response used when the admission decision is allow. -func allowedResponse() *admissionv1.AdmissionResponse { - return &admissionv1.AdmissionResponse{Allowed: true} -} - -func allowedByExemptResponse(exemptionReason string) *admissionv1.AdmissionResponse { - return &admissionv1.AdmissionResponse{ - Allowed: true, - AuditAnnotations: map[string]string{api.ExemptionReasonAnnotationKey: exemptionReason}, - } -} - -func failureResponse(msg string, reason metav1.StatusReason, code int32) *admissionv1.AdmissionResponse { - return &admissionv1.AdmissionResponse{ - Allowed: false, - Result: &metav1.Status{ - Status: metav1.StatusFailure, - Reason: reason, - Message: msg, - Code: code, - }, - } -} - -// forbiddenResponse is the response used when the admission decision is deny for policy violations. -func forbiddenResponse(msg string) *admissionv1.AdmissionResponse { - return failureResponse(msg, metav1.StatusReasonForbidden, http.StatusForbidden) -} - -// invalidResponse is the response used for namespace requests when namespace labels are invalid. -func invalidResponse(attrs api.Attributes, fieldErrors field.ErrorList) *admissionv1.AdmissionResponse { - return &admissionv1.AdmissionResponse{ - Allowed: false, - Result: &apierrors.NewInvalid(attrs.GetKind().GroupKind(), attrs.GetName(), fieldErrors).ErrStatus, - } -} - -// badRequestResponse is the response used when a request cannot be processed. -func badRequestResponse(msg string) *admissionv1.AdmissionResponse { - return failureResponse(msg, metav1.StatusReasonBadRequest, http.StatusBadRequest) -} - -// internalErrorResponse is the response used for unexpected errors -func internalErrorResponse(msg string) *admissionv1.AdmissionResponse { - return failureResponse(msg, metav1.StatusReasonInternalError, http.StatusInternalServerError) -} - // isSignificantPodUpdate determines whether a pod update should trigger a policy evaluation. // Relevant mutable pod fields as of 1.21 are image and seccomp annotations: // * https://github.com/kubernetes/kubernetes/blob/release-1.21/pkg/apis/core/validation/validation.go#L3947-L3949 diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index 6cca43869fc..f140347c29c 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -688,14 +688,14 @@ func TestValidatePodAndController(t *testing.T) { type testCase struct { desc string - namespace string - username string - runtimeClass string + namespace string + username string - operation admissionv1.Operation - pod *corev1.Pod - oldPod *corev1.Pod + // pod and oldPod are used to populate obj and oldObj respectively, according to the test type (pod or deployment). + pod *corev1.Pod + oldPod *corev1.Pod + operation admissionv1.Operation resource schema.GroupVersionResource kind schema.GroupVersionKind obj runtime.Object @@ -744,11 +744,12 @@ func TestValidatePodAndController(t *testing.T) { expectedAuditAnnotationKeys: []string{"exempt"}, }, { - desc: "namespace not found", - namespace: "missing-ns", - pod: restrictedPod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonInternalError, + desc: "namespace not found", + namespace: "missing-ns", + pod: restrictedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonInternalError, + expectedAuditAnnotationKeys: []string{"error"}, }, { desc: "short-circuit privileged:latest (implicit)", @@ -763,39 +764,43 @@ func TestValidatePodAndController(t *testing.T) { expectAllowed: true, }, { - desc: "failed decode", - namespace: baselineNs, - objErr: fmt.Errorf("expected (failed decode)"), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, + desc: "failed decode", + namespace: baselineNs, + objErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectedAuditAnnotationKeys: []string{"error"}, }, { - desc: "invalid object", - namespace: baselineNs, - operation: admissionv1.Update, - obj: &corev1.Namespace{}, - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, + desc: "invalid object", + namespace: baselineNs, + operation: admissionv1.Update, + obj: &corev1.Namespace{}, + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectedAuditAnnotationKeys: []string{"error"}, }, { - desc: "failed decode old object", - namespace: baselineNs, - operation: admissionv1.Update, - pod: restrictedPod.DeepCopy(), - oldObjErr: fmt.Errorf("expected (failed decode)"), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - skipDeployment: true, // Updates aren't special cased for controller resources. + desc: "failed decode old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObjErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectedAuditAnnotationKeys: []string{"error"}, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "invalid old object", - namespace: baselineNs, - operation: admissionv1.Update, - pod: restrictedPod.DeepCopy(), - oldObj: &corev1.Namespace{}, - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - skipDeployment: true, // Updates aren't special cased for controller resources. + desc: "invalid old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObj: &corev1.Namespace{}, + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectedAuditAnnotationKeys: []string{"error"}, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { desc: "insignificant update", @@ -907,12 +912,8 @@ func TestValidatePodAndController(t *testing.T) { deploymentTest.desc = "deployment:" + tc.desc deploymentTest.resource = schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} deploymentTest.kind = schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} - deploymentTest.expectAllowed = true // Deployments policies are non-enforcing. + deploymentTest.expectAllowed = true // PodController validation is always non-enforcing. deploymentTest.expectReason = "" - if tc.expectReason != "" && tc.expectReason != metav1.StatusReasonForbidden { - // Error case, expect an error annotation. - deploymentTest.expectedAuditAnnotationKeys = append(deploymentTest.expectedAuditAnnotationKeys, "error") - } if tc.pod != nil { podTest.obj = tc.pod diff --git a/staging/src/k8s.io/pod-security-admission/admission/response.go b/staging/src/k8s.io/pod-security-admission/admission/response.go new file mode 100644 index 00000000000..b121f5dd2a9 --- /dev/null +++ b/staging/src/k8s.io/pod-security-admission/admission/response.go @@ -0,0 +1,93 @@ +/* +Copyright 2021 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 admission + +import ( + "fmt" + + admissionv1 "k8s.io/api/admission/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/pod-security-admission/api" +) + +var ( + _sharedAllowedResponse = allowedResponse() + _sharedAllowedByUserExemptionResponse = allowedByExemptResponse("user") + _sharedAllowedByNamespaceExemptionResponse = allowedByExemptResponse("namespace") + _sharedAllowedByRuntimeClassExemptionResponse = allowedByExemptResponse("runtimeClass") +) + +func sharedAllowedResponse() *admissionv1.AdmissionResponse { + return _sharedAllowedResponse +} + +func sharedAllowedByUserExemptionResponse() *admissionv1.AdmissionResponse { + return _sharedAllowedByUserExemptionResponse +} + +func sharedAllowedByNamespaceExemptionResponse() *admissionv1.AdmissionResponse { + return _sharedAllowedByNamespaceExemptionResponse +} + +func sharedAllowedByRuntimeClassExemptionResponse() *admissionv1.AdmissionResponse { + return _sharedAllowedByRuntimeClassExemptionResponse +} + +// allowedResponse is the response used when the admission decision is allow. +func allowedResponse() *admissionv1.AdmissionResponse { + return &admissionv1.AdmissionResponse{Allowed: true} +} + +func allowedByExemptResponse(exemptionReason string) *admissionv1.AdmissionResponse { + return &admissionv1.AdmissionResponse{ + Allowed: true, + AuditAnnotations: map[string]string{api.ExemptionReasonAnnotationKey: exemptionReason}, + } +} + +// forbiddenResponse is the response used when the admission decision is deny for policy violations. +func forbiddenResponse(attrs api.Attributes, err error) *admissionv1.AdmissionResponse { + return &admissionv1.AdmissionResponse{ + Allowed: false, + Result: &apierrors.NewForbidden(attrs.GetResource().GroupResource(), attrs.GetName(), err).ErrStatus, + } +} + +// invalidResponse is the response used for namespace requests when namespace labels are invalid. +func invalidResponse(attrs api.Attributes, fieldErrors field.ErrorList) *admissionv1.AdmissionResponse { + return &admissionv1.AdmissionResponse{ + Allowed: false, + Result: &apierrors.NewInvalid(attrs.GetKind().GroupKind(), attrs.GetName(), fieldErrors).ErrStatus, + } +} + +// errorResponse is the response used to capture generic errors. +func errorResponse(err error, status *metav1.Status) *admissionv1.AdmissionResponse { + var errDetail string + if err != nil { + errDetail = fmt.Sprintf("%s: %v", status.Message, err) + } else { + errDetail = status.Message + } + return &admissionv1.AdmissionResponse{ + Allowed: false, + Result: status, + AuditAnnotations: map[string]string{"error": errDetail}, + } +} From 81661d5a3485c8187234953ffda3610e8223b610 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 2 Nov 2021 13:52:19 -0700 Subject: [PATCH 4/4] [PodSecurity] Add metrics test coverage --- .../admission/admission.go | 36 +-- .../admission/admission_test.go | 292 ++++++++++-------- .../admission/main_test.go | 23 +- .../admission/response.go | 37 +-- 4 files changed, 217 insertions(+), 171 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index e96c1a0bbd7..8f7e9751379 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -228,7 +228,7 @@ func (a *Admission) Validate(ctx context.Context, attrs api.Attributes) *admissi func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) *admissionv1.AdmissionResponse { // short-circuit on subresources if attrs.GetSubresource() != "" { - return sharedAllowedResponse() + return sharedAllowedResponse } obj, err := attrs.GetObject() if err != nil { @@ -249,7 +249,7 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) if len(newErrs) > 0 { return invalidResponse(attrs, newErrs) } - return sharedAllowedResponse() + return sharedAllowedResponse case admissionv1.Update: // if update, check if policy labels changed @@ -276,24 +276,24 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) // * if the new enforce is the same version and level was relaxed // * for exempt namespaces if newPolicy.Enforce == oldPolicy.Enforce { - return sharedAllowedResponse() + return sharedAllowedResponse } if newPolicy.Enforce.Level == api.LevelPrivileged { - return sharedAllowedResponse() + return sharedAllowedResponse } if newPolicy.Enforce.Version == oldPolicy.Enforce.Version && api.CompareLevels(newPolicy.Enforce.Level, oldPolicy.Enforce.Level) < 1 { - return sharedAllowedResponse() + return sharedAllowedResponse } if a.exemptNamespace(attrs.GetNamespace()) { - return sharedAllowedByNamespaceExemptionResponse() + return sharedAllowedByNamespaceExemptionResponse } response := allowedResponse() response.Warnings = a.EvaluatePodsInNamespace(ctx, namespace.Name, newPolicy.Enforce) return response default: - return sharedAllowedResponse() + return sharedAllowedResponse } } @@ -316,17 +316,17 @@ var ignoredPodSubresources = map[string]bool{ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admissionv1.AdmissionResponse { // short-circuit on ignored subresources if ignoredPodSubresources[attrs.GetSubresource()] { - return sharedAllowedResponse() + return sharedAllowedResponse } // short-circuit on exempt namespaces and users if a.exemptNamespace(attrs.GetNamespace()) { a.Metrics.RecordExemption(attrs) - return sharedAllowedByNamespaceExemptionResponse() + return sharedAllowedByNamespaceExemptionResponse } if a.exemptUser(attrs.GetUserName()) { a.Metrics.RecordExemption(attrs) - return sharedAllowedByUserExemptionResponse() + return sharedAllowedByUserExemptionResponse } // short-circuit on privileged enforce+audit+warn namespaces @@ -339,7 +339,7 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi nsPolicy, nsPolicyErrs := a.PolicyToEvaluate(namespace.Labels) if len(nsPolicyErrs) == 0 && nsPolicy.Enforce.Level == api.LevelPrivileged && nsPolicy.Warn.Level == api.LevelPrivileged && nsPolicy.Audit.Level == api.LevelPrivileged { a.Metrics.RecordEvaluation(metrics.DecisionAllow, nsPolicy.Enforce, metrics.ModeEnforce, attrs) - return sharedAllowedResponse() + return sharedAllowedPrivilegedResponse } obj, err := attrs.GetObject() @@ -369,7 +369,7 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi } if !isSignificantPodUpdate(pod, oldPod) { // Nothing we care about changed, so always allow the update. - return sharedAllowedResponse() + return sharedAllowedResponse } } return a.EvaluatePod(ctx, nsPolicy, nsPolicyErrs.ToAggregate(), &pod.ObjectMeta, &pod.Spec, attrs, true) @@ -380,17 +380,17 @@ func (a *Admission) ValidatePod(ctx context.Context, attrs api.Attributes) *admi func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attributes) *admissionv1.AdmissionResponse { // short-circuit on subresources if attrs.GetSubresource() != "" { - return sharedAllowedResponse() + return sharedAllowedResponse } // short-circuit on exempt namespaces and users if a.exemptNamespace(attrs.GetNamespace()) { a.Metrics.RecordExemption(attrs) - return sharedAllowedByNamespaceExemptionResponse() + return sharedAllowedByNamespaceExemptionResponse } if a.exemptUser(attrs.GetUserName()) { a.Metrics.RecordExemption(attrs) - return sharedAllowedByUserExemptionResponse() + return sharedAllowedByUserExemptionResponse } // short-circuit on privileged audit+warn namespaces @@ -406,7 +406,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu } nsPolicy, nsPolicyErrs := a.PolicyToEvaluate(namespace.Labels) if len(nsPolicyErrs) == 0 && nsPolicy.Warn.Level == api.LevelPrivileged && nsPolicy.Audit.Level == api.LevelPrivileged { - return sharedAllowedResponse() + return sharedAllowedResponse } obj, err := attrs.GetObject() @@ -431,7 +431,7 @@ func (a *Admission) ValidatePodController(ctx context.Context, attrs api.Attribu } if podMetadata == nil && podSpec == nil { // if a controller with an optional pod spec does not contain a pod spec, skip validation - return sharedAllowedResponse() + return sharedAllowedResponse } return a.EvaluatePod(ctx, nsPolicy, nsPolicyErrs.ToAggregate(), podMetadata, podSpec, attrs, false) } @@ -443,7 +443,7 @@ func (a *Admission) EvaluatePod(ctx context.Context, nsPolicy api.Policy, nsPoli // short-circuit on exempt runtimeclass if a.exemptRuntimeClass(podSpec.RuntimeClassName) { a.Metrics.RecordExemption(attrs) - return sharedAllowedByRuntimeClassExemptionResponse() + return sharedAllowedByRuntimeClassExemptionResponse } auditAnnotations := map[string]string{} diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index f140347c29c..11459ca1a0e 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -602,9 +602,11 @@ func TestValidatePodAndController(t *testing.T) { exemptUser = "exempt-user" exemptRuntimeClass = "exempt-runtimeclass" + + podName = "test-pod" ) - objMetadata := metav1.ObjectMeta{Name: "test-pod", Labels: map[string]string{"foo": "bar"}} + objMetadata := metav1.ObjectMeta{Name: podName, Labels: map[string]string{"foo": "bar"}} restrictedPod, err := test.GetMinimalValidPod(api.LevelRestricted, api.MajorMinorVersion(1, 23)) require.NoError(t, err) @@ -662,7 +664,7 @@ func TestValidatePodAndController(t *testing.T) { baselineNs: makeNs(api.LevelBaseline, api.LevelBaseline, api.LevelBaseline), baselineWarnNs: makeNs("", api.LevelBaseline, ""), baselineAuditNs: makeNs("", "", api.LevelBaseline), - restrictedNs: makeNs(api.LevelRestricted, "", api.LevelRestricted), + restrictedNs: makeNs(api.LevelRestricted, api.LevelRestricted, api.LevelRestricted), invalidNs: makeNs("not-a-valid-level", "", ""), } @@ -675,16 +677,6 @@ func TestValidatePodAndController(t *testing.T) { evaluator, err := policy.NewEvaluator(policy.DefaultChecks()) assert.NoError(t, err) - a := &Admission{ - PodLister: &testPodLister{}, - Evaluator: evaluator, - Configuration: config, - Metrics: &FakeRecorder{}, - NamespaceGetter: nsGetter, - } - require.NoError(t, a.CompleteConfiguration(), "CompleteConfiguration()") - require.NoError(t, a.ValidateConfiguration(), "ValidateConfiguration()") - type testCase struct { desc string @@ -707,11 +699,14 @@ func TestValidatePodAndController(t *testing.T) { skipPod bool // Whether to skip the ValidatePod test case. skipDeployment bool // Whteher to skip the ValidatePodController test case. - expectAllowed bool - expectReason metav1.StatusReason - expectWarning bool - expectEnforce bool // Whether to expect an enforcing evaluation (metric+annotation) - expectedAuditAnnotationKeys []string + expectAllowed bool + expectReason metav1.StatusReason + expectExempt bool + expectError bool + + expectEnforce api.Level + expectWarning api.Level + expectAudit api.Level } podCases := []testCase{ { @@ -722,85 +717,87 @@ func TestValidatePodAndController(t *testing.T) { expectAllowed: true, }, { - desc: "exempt namespace", - namespace: exemptNs, - pod: privilegedPod.DeepCopy(), - expectAllowed: true, - expectedAuditAnnotationKeys: []string{"exempt"}, + desc: "exempt namespace", + namespace: exemptNs, + pod: privilegedPod.DeepCopy(), + expectAllowed: true, + expectExempt: true, }, { - desc: "exempt user", - namespace: restrictedNs, - username: exemptUser, - pod: privilegedPod.DeepCopy(), - expectAllowed: true, - expectedAuditAnnotationKeys: []string{"exempt"}, + desc: "exempt user", + namespace: restrictedNs, + username: exemptUser, + pod: privilegedPod.DeepCopy(), + expectAllowed: true, + expectExempt: true, }, { - desc: "exempt runtimeClass", - namespace: restrictedNs, - pod: exemptRCPod.DeepCopy(), - expectAllowed: true, - expectedAuditAnnotationKeys: []string{"exempt"}, + desc: "exempt runtimeClass", + namespace: restrictedNs, + pod: exemptRCPod.DeepCopy(), + expectAllowed: true, + expectExempt: true, }, { - desc: "namespace not found", - namespace: "missing-ns", - pod: restrictedPod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonInternalError, - expectedAuditAnnotationKeys: []string{"error"}, + desc: "namespace not found", + namespace: "missing-ns", + pod: restrictedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonInternalError, + expectError: true, }, { desc: "short-circuit privileged:latest (implicit)", namespace: implicitNs, pod: privilegedPod.DeepCopy(), expectAllowed: true, + expectEnforce: api.LevelPrivileged, }, { desc: "short-circuit privileged:latest (explicit)", namespace: privilegedNs, pod: privilegedPod.DeepCopy(), expectAllowed: true, + expectEnforce: api.LevelPrivileged, }, { - desc: "failed decode", - namespace: baselineNs, - objErr: fmt.Errorf("expected (failed decode)"), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - expectedAuditAnnotationKeys: []string{"error"}, + desc: "failed decode", + namespace: baselineNs, + objErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectError: true, }, { - desc: "invalid object", - namespace: baselineNs, - operation: admissionv1.Update, - obj: &corev1.Namespace{}, - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - expectedAuditAnnotationKeys: []string{"error"}, + desc: "invalid object", + namespace: baselineNs, + operation: admissionv1.Update, + obj: &corev1.Namespace{}, + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectError: true, }, { - desc: "failed decode old object", - namespace: baselineNs, - operation: admissionv1.Update, - pod: restrictedPod.DeepCopy(), - oldObjErr: fmt.Errorf("expected (failed decode)"), - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - expectedAuditAnnotationKeys: []string{"error"}, - skipDeployment: true, // Updates aren't special cased for controller resources. + desc: "failed decode old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObjErr: fmt.Errorf("expected (failed decode)"), + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectError: true, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "invalid old object", - namespace: baselineNs, - operation: admissionv1.Update, - pod: restrictedPod.DeepCopy(), - oldObj: &corev1.Namespace{}, - expectAllowed: false, - expectReason: metav1.StatusReasonBadRequest, - expectedAuditAnnotationKeys: []string{"error"}, - skipDeployment: true, // Updates aren't special cased for controller resources. + desc: "invalid old object", + namespace: baselineNs, + operation: admissionv1.Update, + pod: restrictedPod.DeepCopy(), + oldObj: &corev1.Namespace{}, + expectAllowed: false, + expectReason: metav1.StatusReasonBadRequest, + expectError: true, + skipDeployment: true, // Updates aren't special cased for controller resources. }, { desc: "insignificant update", @@ -812,15 +809,16 @@ func TestValidatePodAndController(t *testing.T) { skipDeployment: true, // Updates aren't special cased for controller resources. }, { - desc: "significant update denied", - namespace: restrictedNs, - operation: admissionv1.Update, - pod: differentPrivilegedPod.DeepCopy(), - oldPod: privilegedPod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonForbidden, - expectEnforce: true, - expectedAuditAnnotationKeys: []string{"audit-violations"}, + desc: "significant update denied", + namespace: restrictedNs, + operation: admissionv1.Update, + pod: differentPrivilegedPod.DeepCopy(), + oldPod: privilegedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectEnforce: api.LevelRestricted, + expectWarning: api.LevelRestricted, + expectAudit: api.LevelRestricted, }, { desc: "significant update allowed", @@ -829,55 +827,56 @@ func TestValidatePodAndController(t *testing.T) { pod: differentRestrictedPod.DeepCopy(), oldPod: restrictedPod, expectAllowed: true, - expectEnforce: true, + expectEnforce: api.LevelRestricted, }, { - desc: "invalid namespace labels", - namespace: invalidNs, - pod: baselinePod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonForbidden, - expectEnforce: true, - expectedAuditAnnotationKeys: []string{"error"}, + desc: "invalid namespace labels", + namespace: invalidNs, + pod: baselinePod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectEnforce: api.LevelRestricted, + expectError: true, }, { - desc: "enforce deny", - namespace: restrictedNs, - pod: privilegedPod.DeepCopy(), - expectAllowed: false, - expectReason: metav1.StatusReasonForbidden, - expectEnforce: true, - expectedAuditAnnotationKeys: []string{"audit-violations"}, + desc: "enforce deny", + namespace: restrictedNs, + pod: privilegedPod.DeepCopy(), + expectAllowed: false, + expectReason: metav1.StatusReasonForbidden, + expectEnforce: api.LevelRestricted, + expectWarning: api.LevelRestricted, + expectAudit: api.LevelRestricted, }, { desc: "enforce allow", namespace: baselineNs, pod: baselinePod.DeepCopy(), expectAllowed: true, - expectEnforce: true, + expectEnforce: api.LevelBaseline, }, { desc: "warn deny", namespace: baselineWarnNs, pod: privilegedPod.DeepCopy(), expectAllowed: true, - expectWarning: true, - expectEnforce: true, + expectEnforce: api.LevelPrivileged, + expectWarning: api.LevelBaseline, }, { - desc: "audit deny", - namespace: baselineAuditNs, - pod: privilegedPod.DeepCopy(), - expectAllowed: true, - expectEnforce: true, - expectedAuditAnnotationKeys: []string{"audit-violations"}, + desc: "audit deny", + namespace: baselineAuditNs, + pod: privilegedPod.DeepCopy(), + expectAllowed: true, + expectEnforce: api.LevelPrivileged, + expectAudit: api.LevelBaseline, }, { desc: "no pod template", namespace: restrictedNs, obj: emptyDeployment.DeepCopy(), expectAllowed: true, - expectWarning: false, // No pod template skips validation. + expectWarning: "", // No pod template skips validation. skipPod: true, }, } @@ -904,15 +903,17 @@ func TestValidatePodAndController(t *testing.T) { podTest.desc = "pod:" + tc.desc podTest.resource = schema.GroupVersionResource{Version: "v1", Resource: "pods"} podTest.kind = schema.GroupVersionKind{Version: "v1", Kind: "Pod"} - if tc.expectEnforce { - podTest.expectedAuditAnnotationKeys = append(podTest.expectedAuditAnnotationKeys, "enforce-policy") + if !tc.expectAllowed { + podTest.expectWarning = "" // Warnings should only be returned when the request is allowed. } deploymentTest := tc deploymentTest.desc = "deployment:" + tc.desc deploymentTest.resource = schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} deploymentTest.kind = schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} - deploymentTest.expectAllowed = true // PodController validation is always non-enforcing. + // PodController validation is always non-enforcing. + deploymentTest.expectAllowed = true + deploymentTest.expectEnforce = "" deploymentTest.expectReason = "" if tc.pod != nil { @@ -961,8 +962,21 @@ func TestValidatePodAndController(t *testing.T) { attrs.Username = tc.username } + recorder := &FakeRecorder{} + a := &Admission{ + PodLister: &testPodLister{}, + Evaluator: evaluator, + Configuration: config, + Metrics: recorder, + NamespaceGetter: nsGetter, + } + require.NoError(t, a.CompleteConfiguration(), "CompleteConfiguration()") + require.NoError(t, a.ValidateConfiguration(), "ValidateConfiguration()") + response := a.Validate(context.TODO(), attrs) + var expectedEvaluations []MetricsRecord + var expectedAuditAnnotationKeys []string if tc.expectAllowed { assert.True(t, response.Allowed, "Allowed") assert.Nil(t, response.Result) @@ -973,42 +987,72 @@ func TestValidatePodAndController(t *testing.T) { } } - if tc.expectWarning { + if tc.expectWarning != "" { assert.NotEmpty(t, response.Warnings, "Warnings") } else { assert.Empty(t, response.Warnings, "Warnings") } - assert.Len(t, response.AuditAnnotations, len(tc.expectedAuditAnnotationKeys), "AuditAnnotations") - for _, key := range tc.expectedAuditAnnotationKeys { + if tc.expectEnforce != "" { + expectedAuditAnnotationKeys = append(expectedAuditAnnotationKeys, "enforce-policy") + record := MetricsRecord{podName, metrics.DecisionAllow, tc.expectEnforce, metrics.ModeEnforce} + if !tc.expectAllowed { + record.EvalDecision = metrics.DecisionDeny + } + expectedEvaluations = append(expectedEvaluations, record) + } + if tc.expectWarning != "" { + expectedEvaluations = append(expectedEvaluations, MetricsRecord{podName, metrics.DecisionDeny, tc.expectWarning, metrics.ModeWarn}) + } + if tc.expectAudit != "" { + expectedEvaluations = append(expectedEvaluations, MetricsRecord{podName, metrics.DecisionDeny, tc.expectAudit, metrics.ModeAudit}) + expectedAuditAnnotationKeys = append(expectedAuditAnnotationKeys, "audit-violations") + } + if tc.expectError { + expectedAuditAnnotationKeys = append(expectedAuditAnnotationKeys, "error") + assert.ElementsMatch(t, []MetricsRecord{{ObjectName: podName}}, recorder.errors, "expected RecordError() calls") + } else { + assert.Empty(t, recorder.errors, "expected RecordError() calls") + } + if tc.expectExempt { + expectedAuditAnnotationKeys = append(expectedAuditAnnotationKeys, "exempt") + assert.ElementsMatch(t, []MetricsRecord{{ObjectName: podName}}, recorder.exemptions, "expected RecordExemption() calls") + } else { + assert.Empty(t, recorder.exemptions, "expected RecordExemption() calls") + } + + assert.Len(t, response.AuditAnnotations, len(expectedAuditAnnotationKeys), "AuditAnnotations") + for _, key := range expectedAuditAnnotationKeys { assert.Contains(t, response.AuditAnnotations, key, "AuditAnnotations") } + + assert.ElementsMatch(t, expectedEvaluations, recorder.evaluations, "expected RecordEvaluation() calls") }) } } type FakeRecorder struct { - evaluations []EvaluationRecord + evaluations []MetricsRecord + exemptions []MetricsRecord + errors []MetricsRecord } -type EvaluationRecord struct { - ObjectName string - Decision metrics.Decision - Policy api.LevelVersion - Mode metrics.Mode +type MetricsRecord struct { + ObjectName string + EvalDecision metrics.Decision + EvalPolicy api.Level + EvalMode metrics.Mode } func (r *FakeRecorder) RecordEvaluation(decision metrics.Decision, policy api.LevelVersion, evalMode metrics.Mode, attrs api.Attributes) { - r.evaluations = append(r.evaluations, EvaluationRecord{attrs.GetName(), decision, policy, evalMode}) + r.evaluations = append(r.evaluations, MetricsRecord{attrs.GetName(), decision, policy.Level, evalMode}) } -func (r *FakeRecorder) RecordExemption(api.Attributes) {} -func (r *FakeRecorder) RecordError(bool, api.Attributes) {} - -// ExpectEvaluation asserts that the evaluation was recorded, and clears the record. -func (r *FakeRecorder) ExpectEvaluations(t *testing.T, expected []EvaluationRecord) { - t.Helper() - assert.ElementsMatch(t, expected, r.evaluations) +func (r *FakeRecorder) RecordExemption(attrs api.Attributes) { + r.exemptions = append(r.exemptions, MetricsRecord{ObjectName: attrs.GetName()}) +} +func (r *FakeRecorder) RecordError(_ bool, attrs api.Attributes) { + r.errors = append(r.errors, MetricsRecord{ObjectName: attrs.GetName()}) } func TestPrioritizePods(t *testing.T) { diff --git a/staging/src/k8s.io/pod-security-admission/admission/main_test.go b/staging/src/k8s.io/pod-security-admission/admission/main_test.go index 4d0e5851bf2..7448ccf92aa 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/main_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/main_test.go @@ -21,15 +21,30 @@ import ( "os" "reflect" "testing" + + admissionv1 "k8s.io/api/admission/v1" ) func TestMain(m *testing.M) { - sharedCopy := sharedAllowedResponse().DeepCopy() + sharedResponses := map[string]*admissionv1.AdmissionResponse{ + "sharedAllowedResponse": sharedAllowedResponse, + "sharedAllowedPrivilegedResponse": sharedAllowedPrivilegedResponse, + "sharedAllowedByUserExemptionResponse": sharedAllowedByUserExemptionResponse, + "sharedAllowedByNamespaceExemptionResponse": sharedAllowedByNamespaceExemptionResponse, + "sharedAllowedByRuntimeClassExemptionResponse": sharedAllowedByRuntimeClassExemptionResponse, + } + sharedResponseCopies := map[string]*admissionv1.AdmissionResponse{} + for name, response := range sharedResponses { + sharedResponseCopies[name] = response.DeepCopy() + } + rc := m.Run() - if !reflect.DeepEqual(sharedCopy, sharedAllowedResponse()) { - fmt.Println("sharedAllowedReponse mutated") - rc = 1 + for name := range sharedResponses { + if !reflect.DeepEqual(sharedResponseCopies[name], sharedResponses[name]) { + fmt.Fprintf(os.Stderr, "%s mutated\n", name) + rc = 1 + } } os.Exit(rc) diff --git a/staging/src/k8s.io/pod-security-admission/admission/response.go b/staging/src/k8s.io/pod-security-admission/admission/response.go index b121f5dd2a9..abbf1a7a9f0 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/response.go +++ b/staging/src/k8s.io/pod-security-admission/admission/response.go @@ -27,26 +27,20 @@ import ( ) var ( - _sharedAllowedResponse = allowedResponse() - _sharedAllowedByUserExemptionResponse = allowedByExemptResponse("user") - _sharedAllowedByNamespaceExemptionResponse = allowedByExemptResponse("namespace") - _sharedAllowedByRuntimeClassExemptionResponse = allowedByExemptResponse("runtimeClass") + sharedAllowedResponse = allowedResponse() + sharedAllowedPrivilegedResponse = allowedResponse() + sharedAllowedByUserExemptionResponse = allowedResponse() + sharedAllowedByNamespaceExemptionResponse = allowedResponse() + sharedAllowedByRuntimeClassExemptionResponse = allowedResponse() ) -func sharedAllowedResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedResponse -} - -func sharedAllowedByUserExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByUserExemptionResponse -} - -func sharedAllowedByNamespaceExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByNamespaceExemptionResponse -} - -func sharedAllowedByRuntimeClassExemptionResponse() *admissionv1.AdmissionResponse { - return _sharedAllowedByRuntimeClassExemptionResponse +func init() { + sharedAllowedPrivilegedResponse.AuditAnnotations = map[string]string{ + api.EnforcedPolicyAnnotationKey: api.LevelVersion{Level: api.LevelPrivileged, Version: api.LatestVersion()}.String(), + } + sharedAllowedByUserExemptionResponse.AuditAnnotations = map[string]string{api.ExemptionReasonAnnotationKey: "user"} + sharedAllowedByNamespaceExemptionResponse.AuditAnnotations = map[string]string{api.ExemptionReasonAnnotationKey: "namespace"} + sharedAllowedByRuntimeClassExemptionResponse.AuditAnnotations = map[string]string{api.ExemptionReasonAnnotationKey: "runtimeClass"} } // allowedResponse is the response used when the admission decision is allow. @@ -54,13 +48,6 @@ func allowedResponse() *admissionv1.AdmissionResponse { return &admissionv1.AdmissionResponse{Allowed: true} } -func allowedByExemptResponse(exemptionReason string) *admissionv1.AdmissionResponse { - return &admissionv1.AdmissionResponse{ - Allowed: true, - AuditAnnotations: map[string]string{api.ExemptionReasonAnnotationKey: exemptionReason}, - } -} - // forbiddenResponse is the response used when the admission decision is deny for policy violations. func forbiddenResponse(attrs api.Attributes, err error) *admissionv1.AdmissionResponse { return &admissionv1.AdmissionResponse{