diff --git a/pkg/securitycontextconstraints/sccadmission/admission.go b/pkg/securitycontextconstraints/sccadmission/admission.go index 02ceaacd4..1a17b1802 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission.go +++ b/pkg/securitycontextconstraints/sccadmission/admission.go @@ -81,7 +81,7 @@ func NewConstraint() *constraint { // any change that claims the pod is no longer privileged will be removed. That should hold until // we get a true old/new set of objects in. func (c *constraint) Admit(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error { - if ignore, err := shouldIgnore(a); err != nil { + if ignore, err := shouldSkipSCCEvaluation(a); err != nil { return err } else if ignore { return nil @@ -131,7 +131,7 @@ func (c *constraint) Admit(ctx context.Context, a admission.Attributes, _ admiss } func (c *constraint) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error { - if ignore, err := shouldIgnore(a); err != nil { + if ignore, err := shouldSkipSCCEvaluation(a); err != nil { return err } else if ignore { return nil @@ -420,7 +420,14 @@ var ignoredAnnotations = sets.NewString( "k8s.ovn.org/pod-networks", ) -func shouldIgnore(a admission.Attributes) (bool, error) { +// shouldSkipSCCEvaluation skips evaluation for: +// - non-Pod resources, +// - specific subresources that don't affect Pod security context (e.g., exec, attach, log), +// - Windows Pods (SCC is not applied), +// - update operations that only change fields like SchedulingGates or non-critical metadata. +// If the request is malformed (e.g., object can't be cast to a Pod), it fails closed to avoid +// bypassing security enforcement unintentionally. +func shouldSkipSCCEvaluation(a admission.Attributes) (bool, error) { if a.GetResource().GroupResource() != coreapi.Resource("pods") { return true, nil } @@ -448,14 +455,17 @@ func shouldIgnore(a admission.Attributes) (bool, error) { return false, admission.NewForbidden(a, fmt.Errorf("object was marked as kind pod but was unable to be converted: %v", a.GetOldObject())) } - // never ignore any spec changes - if !kapihelper.Semantic.DeepEqual(pod.Spec, oldPod.Spec) { + // Create deep copies to avoid mutating the original objects + podWithoutSchedulingGates := pod.DeepCopy() + // Skip SchedulingGates when comparing specs + podWithoutSchedulingGates.Spec.SchedulingGates = oldPod.Spec.SchedulingGates + if !kapihelper.Semantic.DeepEqual(podWithoutSchedulingGates.Spec, oldPod.Spec) { return false, nil } // see if we are only doing meta changes that should be ignored during admission // for example, the OVN controller adds informative networking annotations that shouldn't cause the pod to go through admission again - if shouldIgnoreMetaChanges(pod, oldPod) { + if shouldIgnoreMetaChanges(podWithoutSchedulingGates, oldPod) { return true, nil } } @@ -463,9 +473,10 @@ func shouldIgnore(a admission.Attributes) (bool, error) { return false, nil } -func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool { +// newPodCopy is expected to be a copy of the original Pod that we can safely mutate +func shouldIgnoreMetaChanges(newPodCopy, oldPod *coreapi.Pod) bool { // check if we're adding or changing only annotations from the ignore list - for key, newVal := range newPod.ObjectMeta.Annotations { + for key, newVal := range newPodCopy.ObjectMeta.Annotations { if oldVal, ok := oldPod.ObjectMeta.Annotations[key]; ok && newVal == oldVal { continue } @@ -477,7 +488,7 @@ func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool { // check if we're removing only annotations from the ignore list for key := range oldPod.ObjectMeta.Annotations { - if _, ok := newPod.ObjectMeta.Annotations[key]; ok { + if _, ok := newPodCopy.ObjectMeta.Annotations[key]; ok { continue } @@ -486,12 +497,11 @@ func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool { } } - newPodCopy := newPod.DeepCopyObject() - newPodCopyMeta, err := meta.Accessor(newPodCopy) + newPodCopyWithoutSchedulingGatesCopyMeta, err := meta.Accessor(newPodCopy) if err != nil { return false } - newPodCopyMeta.SetAnnotations(oldPod.ObjectMeta.Annotations) + newPodCopyWithoutSchedulingGatesCopyMeta.SetAnnotations(oldPod.ObjectMeta.Annotations) // see if we are only updating the ownerRef. Garbage collection does this // and we should allow it in general, since you had the power to update and the power to delete. diff --git a/pkg/securitycontextconstraints/sccadmission/admission_test.go b/pkg/securitycontextconstraints/sccadmission/admission_test.go index 4d7b1fe2e..daa8253a3 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission_test.go +++ b/pkg/securitycontextconstraints/sccadmission/admission_test.go @@ -10,6 +10,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/diff" @@ -204,7 +205,7 @@ func TestAdmitCaps(t *testing.T) { } } -func TestShouldIgnore(t *testing.T) { +func TestShouldSkipSCCEvaluation(t *testing.T) { podCreationToAttributes := func(p *coreapi.Pod) admission.Attributes { return admission.NewAttributesRecord( p, nil, @@ -268,6 +269,15 @@ func TestShouldIgnore(t *testing.T) { shouldIgnore: true, admissionAttributes: withStatusUpdate(goodPod()), }, + { + description: "schedulingGates updates should be ignored", + shouldIgnore: true, + admissionAttributes: withUpdate(schedulingGatePod(), "", + func(p *coreapi.Pod) *coreapi.Pod { + p.Spec.SchedulingGates = []coreapi.PodSchedulingGate{} + return p + }), + }, { description: "don't ignore normal updates", shouldIgnore: false, @@ -305,7 +315,7 @@ func TestShouldIgnore(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { - ignored, err := shouldIgnore(test.admissionAttributes) + ignored, err := shouldSkipSCCEvaluation(test.admissionAttributes) if err != nil { t.Errorf("expected the test to not error but it errored with %v", err) } @@ -386,6 +396,95 @@ func TestShouldIgnoreMetaChanges(t *testing.T) { } } +func TestShouldSkipSCCEvaluationDoesNotMutateOriginalPod(t *testing.T) { + tests := []struct { + name string + setupPod func() *coreapi.Pod + setupAttrs func(pod *coreapi.Pod) admission.Attributes + }{ + { + name: "scheduling gates update should not mutate original pod", + setupPod: func() *coreapi.Pod { + return schedulingGatePod() + }, + setupAttrs: func(pod *coreapi.Pod) admission.Attributes { + oldPod := pod.DeepCopy() + updatedPod := pod.DeepCopy() + updatedPod.Spec.SchedulingGates = []coreapi.PodSchedulingGate{} + + return admission.NewAttributesRecord( + updatedPod, oldPod, + coreapi.Kind("Pod").WithVersion("version"), + pod.Namespace, pod.Name, + coreapi.Resource("pods").WithVersion("version"), + "", + admission.Update, + nil, + false, + &user.DefaultInfo{}, + ) + }, + }, + { + name: "metadata changes should not mutate original pod", + setupPod: func() *coreapi.Pod { + pod := goodPod() + pod.Annotations = map[string]string{ + "k8s.ovn.org/pod-networks": "original-value", + "other-annotation": "other-value", + } + return pod + }, + setupAttrs: func(pod *coreapi.Pod) admission.Attributes { + oldPod := pod.DeepCopy() + updatedPod := pod.DeepCopy() + updatedPod.Annotations["k8s.ovn.org/pod-networks"] = "updated-value" + + return admission.NewAttributesRecord( + updatedPod, oldPod, + coreapi.Kind("Pod").WithVersion("version"), + pod.Namespace, pod.Name, + coreapi.Resource("pods").WithVersion("version"), + "", + admission.Update, + nil, + false, + &user.DefaultInfo{}, + ) + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + originalPod := test.setupPod() + originalPodCopy := originalPod.DeepCopy() + attrs := test.setupAttrs(originalPod) + + // Call shouldSkipSCCEvaluation with the admission attributes + _, err := shouldSkipSCCEvaluation(attrs) + if err != nil { + t.Fatalf("shouldSkipSCCEvaluation returned unexpected error: %v", err) + } + + // Verify that the original pod hasn't been mutated + if !apiequality.Semantic.DeepEqual(originalPod, originalPodCopy) { + t.Errorf("Original pod was mutated during shouldSkipSCCEvaluation call") + t.Errorf("Original: %+v", originalPodCopy) + t.Errorf("After call: %+v", originalPod) + } + + // Also verify that the old pod in the attributes hasn't been mutated + oldPodFromAttrs := attrs.GetOldObject().(*coreapi.Pod) + if !apiequality.Semantic.DeepEqual(oldPodFromAttrs, originalPodCopy) { + t.Errorf("Old pod from admission attributes was mutated during shouldSkipSCCEvaluation call") + t.Errorf("Expected: %+v", originalPodCopy) + t.Errorf("Actual: %+v", oldPodFromAttrs) + } + }) + } +} + func testSCCAdmit(testCaseName string, sccs []*securityv1.SecurityContextConstraints, pod *coreapi.Pod, shouldPass bool, t *testing.T) { t.Helper() @@ -2008,6 +2107,14 @@ func goodPod() *coreapi.Pod { } } +// schedulingGatePod is empty pod with scheduling gate. schedulingGates modifications +// should be safely ignored. +func schedulingGatePod() *coreapi.Pod { + p := goodPod() + p.Spec.SchedulingGates = []coreapi.PodSchedulingGate{{Name: "testGate"}} + return p +} + // windowsPod returns windows pod without any SCCs which are specific to Linux. The admission of Windows pod // should be safely ignored. func windowsPod() *coreapi.Pod {