From 6d040812f001feaa39bab8ffa58d094c2e1d3e72 Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 5 Sep 2017 11:08:53 -0400 Subject: [PATCH 1/2] check block owner ref on finalizers subresource --- plugin/pkg/admission/gc/gc_admission.go | 5 +++-- plugin/pkg/admission/gc/gc_admission_test.go | 14 +++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/plugin/pkg/admission/gc/gc_admission.go b/plugin/pkg/admission/gc/gc_admission.go index 93834cb040a..5c4287925b6 100644 --- a/plugin/pkg/admission/gc/gc_admission.go +++ b/plugin/pkg/admission/gc/gc_admission.go @@ -122,7 +122,7 @@ func (a *gcPermissionsEnforcement) Admit(attributes admission.Attributes) (err e for _, record := range records { allowed, reason, err := a.authorizer.Authorize(record) if !allowed { - return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't delete: %v, %v", reason, err)) + return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: %v, %v", reason, err)) } } } @@ -178,12 +178,13 @@ func (a *gcPermissionsEnforcement) ownerRefToDeleteAttributeRecords(ref metav1.O for _, mapping := range mappings { ret = append(ret, authorizer.AttributesRecord{ User: attributes.GetUserInfo(), - Verb: "delete", + Verb: "update", // ownerReference can only refer to an object in the same namespace, so attributes.GetNamespace() equals to the owner's namespace Namespace: attributes.GetNamespace(), APIGroup: groupVersion.Group, APIVersion: groupVersion.Version, Resource: mapping.Resource, + Subresource: "finalizers", Name: ref.Name, ResourceRequest: true, Path: "", diff --git a/plugin/pkg/admission/gc/gc_admission_test.go b/plugin/pkg/admission/gc/gc_admission_test.go index 4d7a6aac6a4..ac750c29f36 100644 --- a/plugin/pkg/admission/gc/gc_admission_test.go +++ b/plugin/pkg/admission/gc/gc_admission_test.go @@ -39,6 +39,9 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { if a.GetVerb() == "delete" { return false, "", nil } + if a.GetVerb() == "update" && a.GetSubresource() == "/finalizers" { + return false, "", nil + } return true, "", nil } @@ -46,6 +49,9 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { if a.GetVerb() == "delete" && a.GetResource() == "pods" { return false, "", nil } + if a.GetVerb() == "update" && a.GetResource() == "pods" && a.GetSubresource() == "finalizers" { + return false, "", nil + } return true, "", nil } @@ -53,6 +59,9 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { if a.GetVerb() == "delete" && a.GetResource() == "replicationcontrollers" { return false, "", nil } + if a.GetVerb() == "update" && a.GetResource() == "replicationcontrollers" && a.GetSubresource() == "finalizers" { + return false, "", nil + } return true, "", nil } @@ -326,7 +335,10 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) { return err == nil } expectCantSetBlockOwnerDeletionError := func(err error) bool { - return strings.Contains(err.Error(), "cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't delete") + if err == nil { + return false + } + return strings.Contains(err.Error(), "cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on") } tests := []struct { name string From 2572ea50e05fa2ce2e9f09d02337a012cc6be7dc Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 5 Sep 2017 11:15:35 -0400 Subject: [PATCH 2/2] add permissions to workload controllers to block owners --- plugin/pkg/admission/gc/gc_admission_test.go | 2 +- .../rbac/bootstrappolicy/controller_policy.go | 7 +++ .../testdata/controller-roles.yaml | 44 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/plugin/pkg/admission/gc/gc_admission_test.go b/plugin/pkg/admission/gc/gc_admission_test.go index ac750c29f36..691be3da3b6 100644 --- a/plugin/pkg/admission/gc/gc_admission_test.go +++ b/plugin/pkg/admission/gc/gc_admission_test.go @@ -39,7 +39,7 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { if a.GetVerb() == "delete" { return false, "", nil } - if a.GetVerb() == "update" && a.GetSubresource() == "/finalizers" { + if a.GetVerb() == "update" && a.GetSubresource() == "finalizers" { return false, "", nil } return true, "", nil diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go index 178cf8ff453..b00ca2c7eb3 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go @@ -74,6 +74,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { rbac.NewRule("get", "list", "watch", "update").Groups(batchGroup).Resources("cronjobs").RuleOrDie(), rbac.NewRule("get", "list", "watch", "create", "update", "delete", "patch").Groups(batchGroup).Resources("jobs").RuleOrDie(), rbac.NewRule("update").Groups(batchGroup).Resources("cronjobs/status").RuleOrDie(), + rbac.NewRule("update").Groups(batchGroup).Resources("cronjobs/finalizers").RuleOrDie(), rbac.NewRule("list", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, @@ -83,6 +84,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { Rules: []rbac.PolicyRule{ rbac.NewRule("get", "list", "watch").Groups(extensionsGroup, appsGroup).Resources("daemonsets").RuleOrDie(), rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("daemonsets/status").RuleOrDie(), + rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("daemonsets/finalizers").RuleOrDie(), rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("nodes").RuleOrDie(), rbac.NewRule("list", "watch", "create", "delete", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(), rbac.NewRule("create").Groups(legacyGroup).Resources("pods/binding").RuleOrDie(), @@ -95,6 +97,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { Rules: []rbac.PolicyRule{ rbac.NewRule("get", "list", "watch", "update").Groups(extensionsGroup, appsGroup).Resources("deployments").RuleOrDie(), rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("deployments/status").RuleOrDie(), + rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("deployments/finalizers").RuleOrDie(), rbac.NewRule("get", "list", "watch", "create", "update", "patch", "delete").Groups(extensionsGroup).Resources("replicasets").RuleOrDie(), // TODO: remove "update" once // https://github.com/kubernetes/kubernetes/issues/36897 is resolved. @@ -168,6 +171,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { Rules: []rbac.PolicyRule{ rbac.NewRule("get", "list", "watch", "update").Groups(batchGroup).Resources("jobs").RuleOrDie(), rbac.NewRule("update").Groups(batchGroup).Resources("jobs/status").RuleOrDie(), + rbac.NewRule("update").Groups(batchGroup).Resources("jobs/finalizers").RuleOrDie(), rbac.NewRule("list", "watch", "create", "delete", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, @@ -225,6 +229,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { Rules: []rbac.PolicyRule{ rbac.NewRule("get", "list", "watch", "update").Groups(extensionsGroup).Resources("replicasets").RuleOrDie(), rbac.NewRule("update").Groups(extensionsGroup).Resources("replicasets/status").RuleOrDie(), + rbac.NewRule("update").Groups(extensionsGroup).Resources("replicasets/finalizers").RuleOrDie(), rbac.NewRule("list", "watch", "patch", "create", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, @@ -235,6 +240,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { // 1.0 controllers needed get, update, so without these old controllers break on new servers rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("replicationcontrollers").RuleOrDie(), rbac.NewRule("update").Groups(legacyGroup).Resources("replicationcontrollers/status").RuleOrDie(), + rbac.NewRule("update").Groups(legacyGroup).Resources("replicationcontrollers/finalizers").RuleOrDie(), rbac.NewRule("list", "watch", "patch", "create", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(), eventsRule(), }, @@ -278,6 +284,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) { rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("pods").RuleOrDie(), rbac.NewRule("get", "list", "watch").Groups(appsGroup).Resources("statefulsets").RuleOrDie(), rbac.NewRule("update").Groups(appsGroup).Resources("statefulsets/status").RuleOrDie(), + rbac.NewRule("update").Groups(appsGroup).Resources("statefulsets/finalizers").RuleOrDie(), rbac.NewRule("get", "create", "delete", "update", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(), rbac.NewRule("get", "create", "delete", "update", "patch", "list", "watch").Groups(appsGroup).Resources("controllerrevisions").RuleOrDie(), rbac.NewRule("get", "create").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(), diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml index f7ca7ecd269..b90d55e497f 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml @@ -124,6 +124,12 @@ items: - cronjobs/status verbs: - update + - apiGroups: + - batch + resources: + - cronjobs/finalizers + verbs: + - update - apiGroups: - "" resources: @@ -165,6 +171,13 @@ items: - daemonsets/status verbs: - update + - apiGroups: + - apps + - extensions + resources: + - daemonsets/finalizers + verbs: + - update - apiGroups: - "" resources: @@ -234,6 +247,13 @@ items: - deployments/status verbs: - update + - apiGroups: + - apps + - extensions + resources: + - deployments/finalizers + verbs: + - update - apiGroups: - extensions resources: @@ -495,6 +515,12 @@ items: - jobs/status verbs: - update + - apiGroups: + - batch + resources: + - jobs/finalizers + verbs: + - update - apiGroups: - "" resources: @@ -741,6 +767,12 @@ items: - replicasets/status verbs: - update + - apiGroups: + - extensions + resources: + - replicasets/finalizers + verbs: + - update - apiGroups: - "" resources: @@ -784,6 +816,12 @@ items: - replicationcontrollers/status verbs: - update + - apiGroups: + - "" + resources: + - replicationcontrollers/finalizers + verbs: + - update - apiGroups: - "" resources: @@ -958,6 +996,12 @@ items: - statefulsets/status verbs: - update + - apiGroups: + - apps + resources: + - statefulsets/finalizers + verbs: + - update - apiGroups: - "" resources: