check the executor subject permission for delete (#160)

Signed-off-by: zhujian <jiazhu@redhat.com>

Signed-off-by: zhujian <jiazhu@redhat.com>
This commit is contained in:
Jian Zhu
2022-10-10 09:21:06 +08:00
committed by GitHub
parent 3947bd293b
commit 6820f68d81
7 changed files with 350 additions and 96 deletions

View File

@@ -636,3 +636,74 @@ func TestApplyOwnerReferences(t *testing.T) {
})
}
}
func TestOwnedByTheWork(t *testing.T) {
testGVR := schema.GroupVersionResource{Version: "v1", Resource: "secrets"}
namespace := "testns"
name := "test"
cases := []struct {
name string
deleteOption *workapiv1.DeleteOption
expected bool
}{
{
name: "foreground by default",
expected: true,
},
{
name: "orphan the resource",
deleteOption: &workapiv1.DeleteOption{PropagationPolicy: workapiv1.DeletePropagationPolicyTypeOrphan},
expected: false,
},
{
name: "no orphan rule with selectively orphan",
deleteOption: &workapiv1.DeleteOption{PropagationPolicy: workapiv1.DeletePropagationPolicyTypeSelectivelyOrphan},
expected: true,
},
{
name: "orphan the resource with selectively orphan",
deleteOption: &workapiv1.DeleteOption{
PropagationPolicy: workapiv1.DeletePropagationPolicyTypeSelectivelyOrphan,
SelectivelyOrphan: &workapiv1.SelectivelyOrphan{
OrphaningRules: []workapiv1.OrphaningRule{
{
Group: "",
Resource: "secrets",
Namespace: namespace,
Name: name,
},
},
},
},
expected: false,
},
{
name: "resourcec is not matched in orphan rule with selectively orphan",
deleteOption: &workapiv1.DeleteOption{
PropagationPolicy: workapiv1.DeletePropagationPolicyTypeSelectivelyOrphan,
SelectivelyOrphan: &workapiv1.SelectivelyOrphan{
OrphaningRules: []workapiv1.OrphaningRule{
{
Group: "",
Resource: "secrets",
Namespace: "testns1",
Name: name,
},
},
},
},
expected: true,
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
own := OwnedByTheWork(testGVR, namespace, name, c.deleteOption)
if own != c.expected {
t.Errorf("Expect owned by the work is %v, but got %v", c.expected, own)
}
})
}
}

View File

@@ -408,3 +408,47 @@ func ApplyOwnerReferences(ctx context.Context, dynamicClient dynamic.Interface,
_, err = dynamicClient.Resource(gvr).Namespace(accessor.GetNamespace()).Patch(ctx, accessor.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{})
return err
}
// OwnedByTheWork checks whether the manifest resource will be owned by the manifest work based on the deleteOption
func OwnedByTheWork(gvr schema.GroupVersionResource,
namespace, name string,
deleteOption *workapiv1.DeleteOption) bool {
// Be default, it is forgound deletion, the manifestwork will own the manifest
if deleteOption == nil {
return true
}
switch deleteOption.PropagationPolicy {
case workapiv1.DeletePropagationPolicyTypeForeground:
return true
case workapiv1.DeletePropagationPolicyTypeOrphan:
return false
}
// If there is none specified selectivelyOrphan, none of the manifests should be orphaned
if deleteOption.SelectivelyOrphan == nil {
return true
}
for _, o := range deleteOption.SelectivelyOrphan.OrphaningRules {
if o.Group != gvr.Group {
continue
}
if o.Resource != gvr.Resource {
continue
}
if o.Name != name {
continue
}
if o.Namespace != namespace {
continue
}
return false
}
return true
}

View File

@@ -3,7 +3,6 @@ package auth
import (
"context"
"fmt"
"strings"
"time"
authorizationv1 "k8s.io/api/authorization/v1"
@@ -19,23 +18,13 @@ import (
workapiv1 "open-cluster-management.io/api/work/v1"
)
// ExecuteAction is the action of executing the manifest work
type ExecuteAction string
const (
// ApplyAction represents applying(create/update) resource to the managed cluster
ApplyAction ExecuteAction = "Apply"
// DeleteAction represents deleting resource from the managed cluster
DeleteAction ExecuteAction = "Delete"
)
// ExecutorValidator validates whether the executor has permission to perform the requests
// to the local managed cluster
type ExecutorValidator interface {
// Validate whether the work executor subject has permission to perform action on the specific manifest,
// if there is no permission will return a kubernetes forbidden error.
Validate(ctx context.Context, executor *workapiv1.ManifestWorkExecutor, gvr schema.GroupVersionResource,
namespace, name string, obj *unstructured.Unstructured, action ExecuteAction) error
namespace, name string, ownedByTheWork bool, obj *unstructured.Unstructured) error
}
type NotAllowedError struct {
@@ -77,7 +66,8 @@ func defaultNewImpersonateClient(config *rest.Config, username string) (dynamic.
}
func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.ManifestWorkExecutor,
gvr schema.GroupVersionResource, namespace, name string, obj *unstructured.Unstructured, action ExecuteAction) error {
gvr schema.GroupVersionResource, namespace, name string,
ownedByTheWork bool, obj *unstructured.Unstructured) error {
if executor == nil {
return nil
}
@@ -91,14 +81,13 @@ func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.Manifes
return fmt.Errorf("the executor service account is nil")
}
var verbs []string
switch action {
case ApplyAction:
verbs = []string{"create", "update", "patch", "get"}
case DeleteAction:
verbs = []string{"delete"}
default:
return fmt.Errorf("execute action %s is invalid", action)
verbs := []string{"create", "update", "patch", "get"}
if ownedByTheWork {
// if the resource to be applied is owned by the manifestwork, will check the delete permission in
// the applying phase in advance. it means the resource will be applied if the executor has the
// delete permission when applying. and even if the delete permission of the executor is revoked
// after applying success, the resource will also be deleted when deleting the manifestwork
verbs = append(verbs, "delete")
}
resource := authorizationv1.ResourceAttributes{
@@ -117,20 +106,18 @@ func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.Manifes
if !allowed {
return &NotAllowedError{
Err: fmt.Errorf("not allowed to %s the resource %s %s, %s %s",
strings.ToLower(string(action)), resource.Group, resource.Resource, resource.Namespace, resource.Name),
Err: fmt.Errorf("not allowed to apply the resource %s %s, %s %s",
resource.Group, resource.Resource, resource.Namespace, resource.Name),
RequeueTime: 60 * time.Second,
}
}
switch {
case action != ApplyAction:
if gvr.Group != "rbac.authorization.k8s.io" {
return nil
case gvr.Group != "rbac.authorization.k8s.io":
return nil
case gvr.Resource == "roles", gvr.Resource == "rolebindings",
gvr.Resource == "clusterroles", gvr.Resource == "clusterrolebindings":
// subjectaccessreview can not permission escalation, use an impersonation request to check again
}
if gvr.Resource == "roles" || gvr.Resource == "rolebindings" ||
gvr.Resource == "clusterroles" || gvr.Resource == "clusterrolebindings" {
// subjectaccessreview can not check permission escalation, use an impersonation request to check again
return v.checkEscalation(ctx, sa, gvr, namespace, name, obj)
}

View File

@@ -26,7 +26,6 @@ func TestValidate(t *testing.T) {
executor *workapiv1.ManifestWorkExecutor
namespace string
name string
action ExecuteAction
expect error
}{
"executor nil": {
@@ -49,19 +48,6 @@ func TestValidate(t *testing.T) {
},
expect: fmt.Errorf("the executor service account is nil"),
},
"action invalid": {
executor: &workapiv1.ManifestWorkExecutor{
Subject: workapiv1.ManifestWorkExecutorSubject{
Type: workapiv1.ExecutorSubjectTypeServiceAccount,
ServiceAccount: &workapiv1.ManifestWorkSubjectServiceAccount{
Namespace: "test-ns",
Name: "test-name",
},
},
},
action: "test-action",
expect: fmt.Errorf("execute action test-action is invalid"),
},
"forbideen": {
executor: &workapiv1.ManifestWorkExecutor{
Subject: workapiv1.ManifestWorkExecutorSubject{
@@ -74,7 +60,6 @@ func TestValidate(t *testing.T) {
},
namespace: "test-deny",
name: "test",
action: ApplyAction,
expect: fmt.Errorf("not allowed to apply the resource secrets, test-deny test, will try again in 1m0s"),
},
"allow": {
@@ -89,7 +74,6 @@ func TestValidate(t *testing.T) {
},
namespace: "test-allow",
name: "test",
action: ApplyAction,
expect: nil,
},
}
@@ -121,7 +105,7 @@ func TestValidate(t *testing.T) {
validator := NewExecutorValidator(nil, kubeClient)
for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
err := validator.Validate(context.TODO(), test.executor, gvr, test.namespace, test.name, nil, test.action)
err := validator.Validate(context.TODO(), test.executor, gvr, test.namespace, test.name, true, nil)
if test.expect == nil {
if err != nil {
t.Errorf("expect nil but got %s", err)
@@ -212,7 +196,7 @@ func TestValidateEscalation(t *testing.T) {
for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
err := validator.Validate(context.TODO(), test.executor, gvr, test.namespace, test.name, test.obj, ApplyAction)
err := validator.Validate(context.TODO(), test.executor, gvr, test.namespace, test.name, true, test.obj)
if test.expect == nil {
if err != nil {
t.Errorf("expect nil but got %s", err)

View File

@@ -274,15 +274,18 @@ func (m *ManifestWorkController) applyOneManifest(
return result
}
// check if the resource to be applied should be owned by the manifest work
ownedByTheWork := helper.OwnedByTheWork(gvr, resMeta.Namespace, resMeta.Name, workSpec.DeleteOption)
// check the Executor subject permission before applying
err = m.validator.Validate(ctx, workSpec.Executor, gvr, resMeta.Namespace, resMeta.Name, required, auth.ApplyAction)
err = m.validator.Validate(ctx, workSpec.Executor, gvr, resMeta.Namespace, resMeta.Name, ownedByTheWork, required)
if err != nil {
result.Error = err
return result
}
// compute required ownerrefs based on delete option
requiredOwner := manageOwnerRef(gvr, resMeta.Namespace, resMeta.Name, workSpec.DeleteOption, owner)
requiredOwner := manageOwnerRef(ownedByTheWork, owner)
// find update strategy option.
option := helper.FindManifestConiguration(resMeta, workSpec.ManifestConfigs)
@@ -303,57 +306,18 @@ func (m *ManifestWorkController) applyOneManifest(
return result
}
// manageOwnerRef return a ownerref based on the resource and the deleteOption indicating whether the owneref
// should be removed or added. If the resource is orphaned, the owner's UID is updated for removal.
// manageOwnerRef return a ownerref based on the resource and the ownedByTheWork indicating whether the owneref
// should be removed or added. If the resource is not owned by the work, the owner's UID is updated for removal.
func manageOwnerRef(
gvr schema.GroupVersionResource,
namespace, name string,
deleteOption *workapiv1.DeleteOption,
ownedByTheWork bool,
myOwner metav1.OwnerReference) metav1.OwnerReference {
// Be default, it is forgound deletion.
if deleteOption == nil {
if ownedByTheWork {
return myOwner
}
removalKey := fmt.Sprintf("%s-", myOwner.UID)
ownerCopy := myOwner.DeepCopy()
switch deleteOption.PropagationPolicy {
case workapiv1.DeletePropagationPolicyTypeForeground:
return myOwner
case workapiv1.DeletePropagationPolicyTypeOrphan:
ownerCopy.UID = types.UID(removalKey)
return *ownerCopy
}
// If there is none specified selectivelyOrphan, none of the manifests should be orphaned
if deleteOption.SelectivelyOrphan == nil {
return myOwner
}
for _, o := range deleteOption.SelectivelyOrphan.OrphaningRules {
if o.Group != gvr.Group {
continue
}
if o.Resource != gvr.Resource {
continue
}
if o.Name != name {
continue
}
if o.Namespace != namespace {
continue
}
ownerCopy.UID = types.UID(removalKey)
return *ownerCopy
}
return myOwner
ownerCopy.UID = types.UID(removalKey)
return *ownerCopy
}
// generateUpdateStatusFunc returns a function which aggregates manifest conditions and generates work conditions.

View File

@@ -21,6 +21,7 @@ import (
fakeworkclient "open-cluster-management.io/api/client/work/clientset/versioned/fake"
workinformers "open-cluster-management.io/api/client/work/informers/externalversions"
workapiv1 "open-cluster-management.io/api/work/v1"
"open-cluster-management.io/work/pkg/helper"
"open-cluster-management.io/work/pkg/spoke/apply"
"open-cluster-management.io/work/pkg/spoke/auth"
"open-cluster-management.io/work/pkg/spoke/controllers"
@@ -816,7 +817,7 @@ func TestManageOwner(t *testing.T) {
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
owner := manageOwnerRef(testGVR, namespace, name, c.deleteOption, c.owner)
owner := manageOwnerRef(helper.OwnedByTheWork(testGVR, namespace, name, c.deleteOption), c.owner)
if !equality.Semantic.DeepEqual(owner, c.expectOwner) {
t.Errorf("Expect owner is %v, but got %v", c.expectOwner, owner)

View File

@@ -210,6 +210,209 @@ var _ = ginkgo.Describe("ManifestWork Executor Subject", func() {
})
})
ginkgo.Context("Apply the resource with executor deleting validating", func() {
executorName := "test-executor"
ginkgo.BeforeEach(func() {
manifests = []workapiv1.Manifest{
util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm1", map[string]string{"a": "b"}, []string{})),
util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm2", map[string]string{"c": "d"}, []string{})),
}
executor = &workapiv1.ManifestWorkExecutor{
Subject: workapiv1.ManifestWorkExecutorSubject{
Type: workapiv1.ExecutorSubjectTypeServiceAccount,
ServiceAccount: &workapiv1.ManifestWorkSubjectServiceAccount{
Namespace: o.SpokeClusterName,
Name: executorName,
},
},
}
})
ginkgo.It("Executor does not have delete permission and delete option is foreground", func() {
roleName := "role1"
_, err = spokeKubeClient.RbacV1().Roles(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.Role{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Rules: []rbacv1.PolicyRule{
{
Verbs: []string{"create", "update", "patch", "get", "list"},
APIGroups: []string{""},
Resources: []string{"configmaps"},
ResourceNames: []string{"cm1", "cm2"},
},
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
_, err = spokeKubeClient.RbacV1().RoleBindings(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Subjects: []rbacv1.Subject{
{
Kind: "ServiceAccount",
Namespace: o.SpokeClusterName,
Name: executorName,
},
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: roleName,
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
work, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Create(
context.Background(), work, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied),
metav1.ConditionFalse, []metav1.ConditionStatus{metav1.ConditionFalse, metav1.ConditionFalse},
eventuallyTimeout, eventuallyInterval)
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable),
metav1.ConditionFalse, []metav1.ConditionStatus{metav1.ConditionFalse, metav1.ConditionFalse},
eventuallyTimeout, eventuallyInterval)
// ensure configmaps not exist
util.AssertNonexistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
})
ginkgo.It("Executor does not have delete permission and delete option is orphan", func() {
roleName := "role1"
_, err = spokeKubeClient.RbacV1().Roles(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.Role{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Rules: []rbacv1.PolicyRule{
{
Verbs: []string{"create", "update", "patch", "get", "list"},
APIGroups: []string{""},
Resources: []string{"configmaps"},
ResourceNames: []string{"cm1", "cm2"},
},
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
_, err = spokeKubeClient.RbacV1().RoleBindings(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Subjects: []rbacv1.Subject{
{
Kind: "ServiceAccount",
Namespace: o.SpokeClusterName,
Name: executorName,
},
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: roleName,
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
work.Spec.DeleteOption = &workapiv1.DeleteOption{
PropagationPolicy: workapiv1.DeletePropagationPolicyTypeOrphan,
}
work, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Create(
context.Background(), work, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied),
metav1.ConditionTrue, []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue},
eventuallyTimeout, eventuallyInterval)
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable),
metav1.ConditionTrue, []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionTrue},
eventuallyTimeout, eventuallyInterval)
// ensure configmaps all exist
util.AssertExistenceOfConfigMaps(manifests, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
})
ginkgo.It("Executor does not have delete permission and delete option is selectively orphan", func() {
roleName := "role1"
_, err = spokeKubeClient.RbacV1().Roles(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.Role{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Rules: []rbacv1.PolicyRule{
{
Verbs: []string{"create", "update", "patch", "get", "list"},
APIGroups: []string{""},
Resources: []string{"configmaps"},
ResourceNames: []string{"cm1", "cm2"},
},
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
_, err = spokeKubeClient.RbacV1().RoleBindings(o.SpokeClusterName).Create(
context.TODO(), &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
Namespace: o.SpokeClusterName,
Name: roleName,
},
Subjects: []rbacv1.Subject{
{
Kind: "ServiceAccount",
Namespace: o.SpokeClusterName,
Name: executorName,
},
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: roleName,
},
}, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
work.Spec.DeleteOption = &workapiv1.DeleteOption{
PropagationPolicy: workapiv1.DeletePropagationPolicyTypeSelectivelyOrphan,
SelectivelyOrphan: &workapiv1.SelectivelyOrphan{
OrphaningRules: []workapiv1.OrphaningRule{
{
Resource: "configmaps",
Namespace: o.SpokeClusterName,
Name: "cm1",
},
},
},
}
work, err = hubWorkClient.WorkV1().ManifestWorks(o.SpokeClusterName).Create(
context.Background(), work, metav1.CreateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkApplied),
metav1.ConditionFalse, []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionFalse},
eventuallyTimeout, eventuallyInterval)
util.AssertWorkCondition(work.Namespace, work.Name, hubWorkClient, string(workapiv1.WorkAvailable),
metav1.ConditionFalse, []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionFalse},
eventuallyTimeout, eventuallyInterval)
// ensure configmap cm1 exist and cm2 not exist
util.AssertExistenceOfConfigMaps(
[]workapiv1.Manifest{
util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm1", map[string]string{"a": "b"}, []string{})),
}, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
util.AssertNonexistenceOfConfigMaps(
[]workapiv1.Manifest{
util.ToManifest(util.NewConfigmap(o.SpokeClusterName, "cm2", map[string]string{"a": "b"}, []string{})),
}, spokeKubeClient, eventuallyTimeout, eventuallyInterval)
})
})
ginkgo.Context("Apply the resource with executor escalation validating", func() {
executorName := "test-executor"
ginkgo.BeforeEach(func() {