make sure that GC can delete privileged pods

Author: deads2k

pkg/build/admission/strategyrestrictions/admission.go

@@ -50,7 +50,7 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error {
 	// if this is an update, 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.
 	// The worst that happens is that you delete something, but you aren't controlling the privileged object itself
-	if attr.GetOldObject() != nil && oadmission.IsOnlyMutatingOwnerRefs(attr.GetObject(), attr.GetOldObject()) {
+	if attr.GetOldObject() != nil && oadmission.IsOnlyMutatingGCFields(attr.GetObject(), attr.GetOldObject()) {
 		return nil
 	}
 

pkg/cmd/server/admission/helpers.go

@@ -6,7 +6,9 @@ import (
 	kapi "k8s.io/kubernetes/pkg/api"
 )
 
-func IsOnlyMutatingOwnerRefs(obj, old runtime.Object) bool {
+// IsOnlyMutatingGCFields checks finalizers and ownerrefs which GC manipulates
+// and indicates that only those fields are changing
+func IsOnlyMutatingGCFields(obj, old runtime.Object) bool {
 	// make a copy of the newObj so that we can stomp for comparison
 	copied, err := kapi.Scheme.Copy(obj)
 	if err != nil {
@@ -22,6 +24,7 @@ func IsOnlyMutatingOwnerRefs(obj, old runtime.Object) bool {
 		return false
 	}
 	copiedMeta.SetOwnerReferences(oldMeta.GetOwnerReferences())
+	copiedMeta.SetFinalizers(oldMeta.GetFinalizers())
 
 	return kapi.Semantic.DeepEqual(copied, old)
 }

pkg/cmd/server/admission/helpers_test.go

@@ -19,7 +19,7 @@ func newPod() *kapi.Pod {
 
 }
 
-func TestIsOnlyMutatingOwnerRefs(t *testing.T) {
+func TestIsOnlyMutatingGCFields(t *testing.T) {
 	tests := []struct {
 		name     string
 		obj      func() runtime.Object
@@ -72,6 +72,19 @@ func TestIsOnlyMutatingOwnerRefs(t *testing.T) {
 			},
 			expected: true,
 		},
+		{
+			name: "ownerRef and finalizer",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.OwnerReferences = append(obj.OwnerReferences, metav1.OwnerReference{Name: "foo"})
+				obj.Finalizers = []string{"final"}
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: true,
+		},
 		{
 			name: "and annotations",
 			obj: func() runtime.Object {
@@ -101,7 +114,7 @@ func TestIsOnlyMutatingOwnerRefs(t *testing.T) {
 	}
 
 	for _, tc := range tests {
-		actual := IsOnlyMutatingOwnerRefs(tc.obj(), tc.old())
+		actual := IsOnlyMutatingGCFields(tc.obj(), tc.old())
 		if tc.expected != actual {
 			t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, actual)
 		}

pkg/security/admission/admission.go

@@ -77,7 +77,7 @@ func (c *constraint) Admit(a admission.Attributes) error {
 	// if this is an update, 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.
 	// The worst that happens is that you delete something, but you aren't controlling the privileged object itself
-	if a.GetOldObject() != nil && oadmission.IsOnlyMutatingOwnerRefs(a.GetObject(), a.GetOldObject()) {
+	if a.GetOldObject() != nil && oadmission.IsOnlyMutatingGCFields(a.GetObject(), a.GetOldObject()) {
 		return nil
 	}
 

test/cmd/policy.sh

@@ -50,6 +50,11 @@ os::cmd::expect_success_and_not_text 'oc get rolebinding/cluster-admin --no-head
 os::cmd::expect_success 'oc policy remove-group system:unauthenticated'
 os::cmd::expect_success 'oc policy remove-user system:no-user'
 
+# check to make sure that our SCC policies don't prevent GC from deleting pods
+os::cmd::expect_success 'oc create -f ${OS_ROOT}/test/testdata/privileged-pod.yaml'
+os::cmd::expect_success 'oc delete pod/test-build-pod-issue --cascade=false'
+os::cmd::try_until_failure 'oc get pods pod/test-build-pod-issue'
+
 
 os::cmd::expect_success_and_text 'oc policy add-role-to-user admin namespaced-user' 'role "admin" added: "namespaced-user"'
 # Ensure the user has create permissions on builds, but that build strategy permissions are granted through the authenticated users group

test/testdata/privileged-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  annotations:
+    openshift.io/scc: privileged
+  name: test-build-pod-issue
+spec:
+  containers:
+  - image: openshift/hello-openshift
+    imagePullPolicy: IfNotPresent
+    name: hello
+    securityContext:
+      privileged: true
+  restartPolicy: Never