From 80b7ff1ffa2a0d23475824ef16a7dae834231307 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Wed, 20 Nov 2024 00:18:18 +0100 Subject: [PATCH 01/18] add the rbac test suite description --- test/e2e/authorisation.go | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 test/e2e/authorisation.go diff --git a/test/e2e/authorisation.go b/test/e2e/authorisation.go new file mode 100644 index 0000000000..ec19c24776 --- /dev/null +++ b/test/e2e/authorisation.go @@ -0,0 +1,125 @@ +package e2e + +import ( + g "github.com/onsi/ginkgo/v2" +) + +var _ = g.Describe("Authorisation [RBAC] [Zalando]", func() { + + g.Context("For all groups", func() { + g.When("the verb is impersonate", func() { + g.It("should deny access for users", func() {}) + g.It("should deny access for service accounts", func() {}) + }) + g.When("the verb is escalate", func() { + g.It("should deny access for cluster roles", func() {}) + g.It("should deny access for roles in all namespaces", func() {}) + }) + }) + + g.Context("For ReadOnly group", func() { + g.When("the resource is a Secret", func() { + g.It("should deny read access in all namespaces", func() {}) + }) + g.When("the resource is not a Secret resource", func() { + g.It("should allow read access in all namespaces", func() {}) + g.It("should deny write access in all namespaces", func() {}) + }) + g.When("the resource is a global resource", func() { + g.It("should allow read access", func() {}) + g.It("should deny write access", func() {}) + }) + }) + + g.Context("For PowerUser, Manual and Emergency groups", func() { + + g.It("should deny read access to Secrets in kube-system and visibility namespaces", func() {}) + g.It("should deny write access to Nodes", func() {}) + g.It("should deny write access to DaemonSets", func() {}) + g.It("should deny deleting CRDs", func() {}) + g.It("should deny deleting kube-system or visibility namespaces", func() {}) + + g.When("the resource is a namespaced resource", func() { + g.It("should deny write access in kube-system and visibility namespaces", func() {}) + g.It("should allow write access in namespaces other than kube-system and visibility", func() {}) + }) + g.When("the resource is a global resource", func() { + g.It("should deny access to Nodes", func() {}) + g.It("should allow access to resources other than Nodes", func() {}) + }) + }) + + g.Context("For CollaboratorPowerUser, CollaboratorManual and CollaboratorEmergency groups", func() { + g.When("the resource is a Secret", func() { + g.It("should allow read access to visibility namespace", func() {}) + g.It("should deny read access to kube-system namespace", func() {}) + }) + + g.It("should deny write access to Nodes", func() {}) + g.It("should allow write access to DaemonSets", func() {}) + g.It("should allow deletion of CRDs", func() {}) + g.It("should deny deletion of kube-system or visibility namespaces", func() {}) + + g.When("the resource is a namespaced resource", func() { + g.It("should deny write access in kube-system namespace", func() {}) + g.It("should allow write access in namespaces other than kube-system", func() {}) + }) + + g.When("the resource is a global resource", func() { + g.It("should deny access to Nodes", func() {}) + g.It("should allow access to resources other than Nodes", func() {}) + }) + }) + + g.Context("For system users", func() { + g.When("the user is kubelet", func() { + g.It("should allow to get Pods", func() {}) + }) + + g.When("the service account is daemonset-controller", func() { + g.It("should allow to update DaemonSet status subresource", func() {}) + g.It("should allow to update DaemonSet finalizers", func() {}) + g.It("should allow to create Pods", func() {}) + }) + + g.When("the service account is default", func() { + g.It("should deny to list StatefulSets when in default namespace", func() {}) + g.It("should deny to list StatefulSets when in non-default namespace", func() {}) + }) + + g.When("the service account is persistent-volume-binder", func() { + g.It("should allow to update PersistentVolumeClaims", func() {}) + g.It("should allow to create PersistentVolumes", func() {}) + + }) + + g.When("the service account is aws-cloud-provider", func() { + g.It("should allow to patch Nodes", func() {}) + }) + + g.When("the service account is api-monitoring-controller", func() { + g.It("should allow to update the skipper-default-filters ConfigMap in kube-system namespace", func() {}) + g.It("should deny to update ConfigMaps other than skipper-default-filters", func() {}) + }) + + g.When("the user is k8sapi_credentials-provider", func() { + g.It("should allow to get Secrets in kube-system namespace", func() {}) + }) + + g.When("the user is stups_cdp-controller", func() { + g.It("should deny access to Secrets in kube-system namespace", func() {}) + }) + + }) + + g.Context("For administrators", func() { + g.It("should allow read access to resources other than Secrets in kube-system namespace", func() {}) + g.It("should allow write access to resources other than Secrets in kube-system namespace", func() {}) + g.It("should allow read access to Secrets in kube-system namespace", func() {}) + g.It("should allow read access to Secrets in namespaces other than kube-system", func() {}) + g.It("should allow write access to namespaces other than kube-system", func() {}) + g.It("should allow to proxy", func() {}) + g.It("should allow write access to DaemonSets", func() {}) + }) + +}) From 7ed7c87b94b49406cc2ae978397b43a7cd60f5c3 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Wed, 20 Nov 2024 00:18:32 +0100 Subject: [PATCH 02/18] add rbac e2e utils --- .../{authorisation.go => authorization.go} | 0 test/e2e/authorization_utils.go | 38 +++++++++++++++++++ 2 files changed, 38 insertions(+) rename test/e2e/{authorisation.go => authorization.go} (100%) create mode 100644 test/e2e/authorization_utils.go diff --git a/test/e2e/authorisation.go b/test/e2e/authorization.go similarity index 100% rename from test/e2e/authorisation.go rename to test/e2e/authorization.go diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go new file mode 100644 index 0000000000..96dbf76b7a --- /dev/null +++ b/test/e2e/authorization_utils.go @@ -0,0 +1,38 @@ +package e2e + +import ( + "context" + "fmt" + + authv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" +) + +func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar authv1.SubjectAccessReview) (*authv1.SubjectAccessReview, error) { + return cs.AuthorizationV1().SubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) +} + +func createLocalClient() (kubernetes.Interface, error) { + kubeconfigPath := "/workdir/test/e2e/kubeconfig" + + // use the current context in kubeconfig + config, err := clientcmd.BuildConfigFromFlags("", kubeconfigPath) + if err != nil { + return nil, err + } + + client, err := kubernetes.NewForConfig(config) + if err != nil { + return nil, fmt.Errorf("unable to create a client: %v", err) + } + + return client, nil +} + +type testCase struct { + name string + sar authv1.SubjectAccessReview + expectedStatus authv1.SubjectAccessReviewStatus +} From e1040a732c3f466dd3a08dfbe5b65523eaf0030c Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Wed, 20 Nov 2024 12:34:41 +0100 Subject: [PATCH 03/18] setup the testing interface between test cases and utils backend --- test/e2e/authorization.go | 79 +++++++++++++++++++++++++++++--- test/e2e/authorization_utils.go | 80 ++++++++++++++++++++++++++------- 2 files changed, 138 insertions(+), 21 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index ec19c24776..2ff243ac13 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -1,19 +1,88 @@ package e2e import ( + "context" + g "github.com/onsi/ginkgo/v2" + gomega "github.com/onsi/gomega" + "k8s.io/client-go/kubernetes" + "k8s.io/kubernetes/test/e2e/framework" +) + +var ( + allGroups = [][]string{ + []string{"FooBar"}, + []string{"ReadOnly"}, + []string{"PowerUser"}, + []string{"Emergency"}, + []string{"Manual"}, + []string{"system:serviceaccounts:kube-system"}, + []string{"CollaboratorEmergency"}, + []string{"CollaboratorManual"}, + []string{"Collaborator24x7"}, + []string{"CollaboratorPowerUser"}, + []string{"Administrator"}, + } ) -var _ = g.Describe("Authorisation [RBAC] [Zalando]", func() { +var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { + var cs kubernetes.Interface + + f := framework.NewDefaultFramework("authorization") + // Initialise the clientset before each test + g.BeforeEach(func() { + cs = f.ClientSet + }) + + // Test cases for all groups of users g.Context("For all groups", func() { + var tc testCase + g.BeforeEach(func() { + tc.data.groups = allGroups + tc.data.users = []string{"test-user"} + }) g.When("the verb is impersonate", func() { - g.It("should deny access for users", func() {}) - g.It("should deny access for service accounts", func() {}) + g.BeforeEach(func() { + tc.data.verbs = []string{"impersonate"} + }) + + g.It("should deny access for users and groups", func() { + // This is safe to do since the BeforeEach block + // will clear these values for other specs. + // https://onsi.github.io/ginkgo/#organizing-specs-with-container-nodes + tc.data.resources = []string{"users", "groups"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + + }) + g.It("should deny access for service accounts", func() { + tc.data.resources = []string{"serviceaccounts"} + tc.data.namespaces = []string{"", "teapot", "kube-system"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) }) g.When("the verb is escalate", func() { - g.It("should deny access for cluster roles", func() {}) - g.It("should deny access for roles in all namespaces", func() {}) + g.BeforeEach(func() { + tc.data.verbs = []string{"escalate"} + }) + + g.It("should deny access for cluster roles", func() { + tc.data.resources = []string{"rbac.authorization.k8s.io/clusterrole"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + g.It("should deny access for roles in all namespaces", func() { + tc.data.resources = []string{"rbac.authorization.k8s.io/role"} + tc.data.namespaces = []string{"", "teapot", "kube-system"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) }) }) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 96dbf76b7a..68ca269795 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -2,37 +2,85 @@ package e2e import ( "context" - "fmt" authv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" - "k8s.io/client-go/tools/clientcmd" ) -func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar authv1.SubjectAccessReview) (*authv1.SubjectAccessReview, error) { - return cs.AuthorizationV1().SubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) +// testCase is a struct that represents a single testcase. +type testCase struct { + data testcaseData + output testcaseOutput +} + +// testcaseData is a struct that makes it user-friendly to write testcases +// more logically. This will be used to generate individual SubjectAccessReview +// objects in order to test the authorization rules. +type testcaseData struct { + namespaces []string + names []string + verbs []string + apiGroups []string + resources []string + subresources []string + nonResourceVerbs []string + nonResourcePaths []string + users []string + + // this is double slice since we need to check individually for + // each group of users. A single slice would mean that the same user + // is part of all the groups. + groups [][]string } -func createLocalClient() (kubernetes.Interface, error) { - kubeconfigPath := "/workdir/test/e2e/kubeconfig" +// testcaseOutput is a struct that represents the expected result of a testcase. +// This is based on the SubjectAccessReviewStatus type. It provides simplicity in testcase +// writing since one testcase can have multiple SubjectAccessReview objects +// and we need to determine the "final" result and expose that as the testcase output. +type testcaseOutput struct { + // the final result based on results of individual SubjectAccessReview objects + allowed, denied bool + // the set of reasons from individual SubjectAccessReview objects + reason []string +} - // use the current context in kubeconfig - config, err := clientcmd.BuildConfigFromFlags("", kubeconfigPath) +func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { + // Generate the list of SubjectAccessReview objects based on the testcase data + sars, err := generateSubjectAccessReviews(t.data) if err != nil { - return nil, err + return err } - client, err := kubernetes.NewForConfig(config) + // Create the SubjectAccessReview objects in the cluster + err = createSubjectAccessReviews(ctx, cs, sars) if err != nil { - return nil, fmt.Errorf("unable to create a client: %v", err) + return err } - return client, nil + // TODO: Implement the logic to determine the final result based on the results of individual SubjectAccessReview objects + return nil } -type testCase struct { - name string - sar authv1.SubjectAccessReview - expectedStatus authv1.SubjectAccessReviewStatus +// accessReviewGenerator generates a list of SubjectAccessReview objects based on the +// testcase data provided. +func generateSubjectAccessReviews(data testcaseData) ([]authv1.SubjectAccessReview, error) { + // TODO: Implement this function + return nil, nil +} + +// createSubjectAccessReviews creates provided SubjectAccessReview objects in the cluster +func createSubjectAccessReviews(ctx context.Context, cs kubernetes.Interface, sars []authv1.SubjectAccessReview) error { + for _, sar := range sars { + _, err := createSubjectAccessReview(ctx, cs, sar) + if err != nil { + return err + } + } + return nil +} + +// createSubjectAccessReview creates a SubjectAccessReview object in the cluster +func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar authv1.SubjectAccessReview) (*authv1.SubjectAccessReview, error) { + return cs.AuthorizationV1().SubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) } From d3c1578063d0da9006b8dadeab71fd91368e4567 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Wed, 20 Nov 2024 13:09:52 +0100 Subject: [PATCH 04/18] implement logic to evaluate output of a testcase --- test/e2e/authorization_utils.go | 51 +++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 68ca269795..eb35218240 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -53,12 +53,15 @@ func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { } // Create the SubjectAccessReview objects in the cluster - err = createSubjectAccessReviews(ctx, cs, sars) + createdSars, err := createSubjectAccessReviews(ctx, cs, sars) if err != nil { return err } - // TODO: Implement the logic to determine the final result based on the results of individual SubjectAccessReview objects + // Evaluate the output based on the created SubjectAccessReview objects + // and set the final result in the testcase output + t.evaluateOutput(createdSars) + return nil } @@ -70,17 +73,53 @@ func generateSubjectAccessReviews(data testcaseData) ([]authv1.SubjectAccessRevi } // createSubjectAccessReviews creates provided SubjectAccessReview objects in the cluster -func createSubjectAccessReviews(ctx context.Context, cs kubernetes.Interface, sars []authv1.SubjectAccessReview) error { +func createSubjectAccessReviews(ctx context.Context, cs kubernetes.Interface, sars []authv1.SubjectAccessReview) ([]authv1.SubjectAccessReview, error) { + createdSars := make([]authv1.SubjectAccessReview, 0) + for _, sar := range sars { - _, err := createSubjectAccessReview(ctx, cs, sar) + createdSar, err := createSubjectAccessReview(ctx, cs, sar) if err != nil { - return err + return createdSars, err } + createdSars = append(createdSars, *createdSar) } - return nil + return createdSars, nil } // createSubjectAccessReview creates a SubjectAccessReview object in the cluster func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar authv1.SubjectAccessReview) (*authv1.SubjectAccessReview, error) { return cs.AuthorizationV1().SubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) } + +func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview) { + tcOutput := testcaseOutput{} + + // Iterate over all the SubjectAccessReviews created and determine the final result + // We don't break the loop if we have denied access from one SubjectAccessReview, + // we continue to check all of them and collect all reasons. + for _, sar := range createdSars { + // We skip the SubjectAccessReviews that are allowed + if sar.Status.Allowed { + continue + } + // If any of the SubjectAccessReviews have denied access, the final result is denied + if sar.Status.Denied { + tcOutput.denied = true + tcOutput.reason = append(tcOutput.reason, sar.Status.Reason) + continue + } + // Undecided access is also considered as denied + if !sar.Status.Allowed && !sar.Status.Denied { + tcOutput.denied = true + tcOutput.reason = append(tcOutput.reason, sar.Status.Reason) + continue + } + } + + // If none of the SubjectAccessReviews have denied access, the final result is allowed + if !tcOutput.denied { + tcOutput.allowed = true + } + + t.output = tcOutput +} From 6bfbe461510a855ebc243ecdc6b2ebf989d52b23 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 01:13:50 +0100 Subject: [PATCH 05/18] add logic to expand into resourceAttributes and nonResourceAttributes from testCaseData --- test/e2e/authorization_utils.go | 166 ++++++++++++++++++++++++++++++-- 1 file changed, 159 insertions(+), 7 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index eb35218240..f3c3ccaebf 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -47,10 +47,7 @@ type testcaseOutput struct { func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { // Generate the list of SubjectAccessReview objects based on the testcase data - sars, err := generateSubjectAccessReviews(t.data) - if err != nil { - return err - } + sars := t.generateSubjectAccessReviews() // Create the SubjectAccessReview objects in the cluster createdSars, err := createSubjectAccessReviews(ctx, cs, sars) @@ -67,9 +64,163 @@ func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { // accessReviewGenerator generates a list of SubjectAccessReview objects based on the // testcase data provided. -func generateSubjectAccessReviews(data testcaseData) ([]authv1.SubjectAccessReview, error) { - // TODO: Implement this function - return nil, nil +func (t *testCase) generateSubjectAccessReviews() []authv1.SubjectAccessReview { + // Initialize the list of SubjectAccessReview objects + sars := make([]authv1.SubjectAccessReview, 0) + + // expand the testcase data to generate a list of ResourceAttributes + resourceAttributes := t.expandResourceAttributes() + + // expand the testcase data to generate a list of NonResourceAttributes + // nonResourceAttributes := t.expandNonResourceAttributes() + + // expand the testcase data to generate a list of SubjectAccessReview objects + // based on the ResourceAttributes and NonResourceAttributes + for _, ra := range resourceAttributes { + for _, user := range t.data.users { + for _, group := range t.data.groups { + sar := authv1.SubjectAccessReview{ + Spec: authv1.SubjectAccessReviewSpec{ + ResourceAttributes: &ra, + User: user, + Groups: group, + }, + } + sars = append(sars, sar) + } + } + } + return sars +} + +// expandResourceAttributes expands the testcase data to generate a list of ResourceAttributes +func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { + // This will hold the expanded ResourceAttributes + ras := make([]authv1.ResourceAttributes, 0) + + // TODO: Convert this logic in a function similar to the way it is implemented + // today to avoid code duplication + nsExpansions := make([]authv1.ResourceAttributes, 0) + // expand on namespaces + if len(t.data.namespaces) > 0 { + for _, ns := range t.data.namespaces { + ra := authv1.ResourceAttributes{ + Namespace: ns, + } + nsExpansions = append(nsExpansions, ra) + } + // we update the expanded list with namespace expansions + ras = nsExpansions + } + + // expand on verbs + verbExpansions := make([]authv1.ResourceAttributes, 0) + if len(t.data.verbs) > 0 { + for _, verb := range t.data.verbs { + for _, ra := range ras { + // copy the ResourceAttributes object to avoid modifying the original object + // and make it safe to user in the next iterations + copy := ra + copy.Verb = verb + verbExpansions = append(verbExpansions, copy) + } + } + // we update the expanded list with verb expansions + ras = verbExpansions + } + + // expand on apiGroups + apiGroupExpansions := make([]authv1.ResourceAttributes, 0) + if len(t.data.apiGroups) > 0 { + for _, apiGroup := range t.data.apiGroups { + for _, ra := range ras { + copy := ra + copy.Group = apiGroup + apiGroupExpansions = append(apiGroupExpansions, copy) + } + } + // we update the expanded list with apiGroup expansions + ras = apiGroupExpansions + } + + // expand on resources + resourceExpansions := make([]authv1.ResourceAttributes, 0) + if len(t.data.resources) > 0 { + for _, resource := range t.data.resources { + for _, ra := range ras { + copy := ra + copy.Resource = resource + resourceExpansions = append(resourceExpansions, copy) + } + } + // we update the expanded list with resource expansions + ras = resourceExpansions + } + + // expand on subresources + subresourceExpansions := make([]authv1.ResourceAttributes, 0) + if len(t.data.subresources) > 0 { + for _, subresource := range t.data.subresources { + for _, ra := range ras { + copy := ra + copy.Subresource = subresource + subresourceExpansions = append(subresourceExpansions, copy) + } + } + // we update the expanded list with subresource expansions + ras = subresourceExpansions + } + + // expand on names + nameExpansions := make([]authv1.ResourceAttributes, 0) + if len(t.data.names) > 0 { + for _, name := range t.data.names { + for _, ra := range ras { + copy := ra + copy.Name = name + nameExpansions = append(nameExpansions, copy) + } + } + // we update the expanded list with name expansions + ras = nameExpansions + } + + return ras +} + +// expandNonResourceAttributes expands the testcase data to generate a list of NonResourceAttributes +func (t *testCase) expandNonResourceAttributes() []authv1.NonResourceAttributes { + // This will hold the expanded NonResourceAttributes + nras := make([]authv1.NonResourceAttributes, 0) + + // expand on paths + pathExpansions := make([]authv1.NonResourceAttributes, 0) + if len(t.data.nonResourcePaths) > 0 { + for _, path := range t.data.nonResourcePaths { + nra := authv1.NonResourceAttributes{ + Path: path, + } + pathExpansions = append(pathExpansions, nra) + } + // we update the expanded list with path expansions + nras = pathExpansions + } + + // expand on verbs + verbExpansions := make([]authv1.NonResourceAttributes, 0) + if len(t.data.nonResourceVerbs) > 0 { + for _, verb := range t.data.nonResourceVerbs { + for _, nra := range nras { + copy := nra + copy.Verb = verb + verbExpansions = append(verbExpansions, copy) + } + } + // we update the expanded list with verb expansions + nras = verbExpansions + } + + return nras } // createSubjectAccessReviews creates provided SubjectAccessReview objects in the cluster @@ -91,6 +242,7 @@ func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar return cs.AuthorizationV1().SubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) } +// evaluateOutput evaluates the output based on the created SubjectAccessReview objects func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview) { tcOutput := testcaseOutput{} From 183cff9178d7d00194212bfe4fbc6cf4e6cedb81 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 01:52:56 +0100 Subject: [PATCH 06/18] enable role-sync-controller --- cluster/config-defaults.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cluster/config-defaults.yaml b/cluster/config-defaults.yaml index b9440555a5..a64856484c 100644 --- a/cluster/config-defaults.yaml +++ b/cluster/config-defaults.yaml @@ -1194,4 +1194,8 @@ teapot_admission_controller_scheduling_controls_enabled: "false" teapot_admission_controller_scheduling_controls_default_architecture: "amd64" # role-sync-controller configs +{{ if eq .Cluster.Environment "e2e" }} +role_sync_controller_enabled: "true" +{{ else }} role_sync_controller_enabled: "false" +{{ end }} From 21d9068b5bb67966cec2063526a94a828ce6b3bb Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 03:07:34 +0100 Subject: [PATCH 07/18] add test cases for ReadOnly group --- test/e2e/authorization.go | 71 ++++++++++++++++++++++++++++++--- test/e2e/authorization_utils.go | 1 + 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index 2ff243ac13..ab1eb5fcd1 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -87,16 +87,77 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.Context("For ReadOnly group", func() { + var tc testCase + g.BeforeEach(func() { + tc.data.groups = [][]string{{"ReadOnly"}} + tc.data.users = []string{"test-user"} + }) g.When("the resource is a Secret", func() { - g.It("should deny read access in all namespaces", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{"secrets"} + }) + g.It("should deny access in all namespaces", func() { + tc.data.verbs = []string{"get", "list", "watch", "create", "update", "delete", "patch"} + tc.data.namespaces = []string{"", "teapot", "kube-system"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) }) g.When("the resource is not a Secret resource", func() { - g.It("should allow read access in all namespaces", func() {}) - g.It("should deny write access in all namespaces", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{ + "pods", + "apps/deployments", + "apps/daemonsets", + "apps/statefulsets", + "apps/deployments/scale", + "apps/statefulsets/scale", + "services", + "persistentvolumes", + "persistentvolumeclaims", + "configmaps", + } + tc.data.namespaces = []string{"", "teapot", "kube-system"} + }) + g.It("should allow read access in all namespaces", func() { + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.allowed).To(gomega.BeTrue(), + "Reason: %v", output.reason) + }) + g.It("should deny write access in all namespaces", func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) }) g.When("the resource is a global resource", func() { - g.It("should allow read access", func() {}) - g.It("should deny write access", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{ + "namespaces", + "nodes", + "rbac.authorization.k8s.io/clusterroles", + "storage.k8s.io/storageclasses", + "policy/podsecuritypolicies", + "apiextensions.k8s.io/customresourcedefinitions", + } + g.It("should allow read access", func() { + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.allowed).To(gomega.BeTrue(), + "Reason: %v", output.reason) + }) + g.It("should deny write access", func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + }) }) }) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index f3c3ccaebf..c4e2cce0a8 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -149,6 +149,7 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { for _, resource := range t.data.resources { for _, ra := range ras { copy := ra + // TODO: handle group/resource/subresource combination copy.Resource = resource resourceExpansions = append(resourceExpansions, copy) } From b845d0367e64580680f25372f67a3e15b1651478 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 10:50:43 +0100 Subject: [PATCH 08/18] resolve TODO about handling subresources --- test/e2e/authorization_utils.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index c4e2cce0a8..51c200fa25 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -2,6 +2,7 @@ package e2e import ( "context" + "strings" authv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -149,8 +150,21 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { for _, resource := range t.data.resources { for _, ra := range ras { copy := ra - // TODO: handle group/resource/subresource combination - copy.Resource = resource + // split the resource string to get the group, resource and subresource + parts := strings.Split(resource, "/") + if len(parts) > 1 { + switch len(parts) { + case 2: + copy.Group = parts[0] + copy.Resource = parts[1] + case 3: + copy.Group = parts[0] + copy.Resource = parts[1] + copy.Subresource = parts[2] + } + } else { + copy.Resource = parts[0] + } resourceExpansions = append(resourceExpansions, copy) } } From a53f942119b26976c1efd28dfc116a7e184b931c Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 11:07:42 +0100 Subject: [PATCH 09/18] fix bug in resource attribute expansion --- test/e2e/authorization_utils.go | 105 ++++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 27 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 51c200fa25..0541d6fcaa 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -118,12 +118,22 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { verbExpansions := make([]authv1.ResourceAttributes, 0) if len(t.data.verbs) > 0 { for _, verb := range t.data.verbs { - for _, ra := range ras { - // copy the ResourceAttributes object to avoid modifying the original object - // and make it safe to user in the next iterations - copy := ra - copy.Verb = verb - verbExpansions = append(verbExpansions, copy) + // If an expansion already take place, we need to copy and + // change the existing objects + if len(ras) > 0 { + for _, ra := range ras { + // copy the ResourceAttributes object to avoid modifying the original object + // and make it safe to user in the next iterations + copy := ra + copy.Verb = verb + verbExpansions = append(verbExpansions, copy) + } + } else { + // If no expansion has taken place, we need to create a new object + ra := authv1.ResourceAttributes{ + Verb: verb, + } + verbExpansions = append(verbExpansions, ra) } } // we update the expanded list with verb expansions @@ -134,10 +144,17 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { apiGroupExpansions := make([]authv1.ResourceAttributes, 0) if len(t.data.apiGroups) > 0 { for _, apiGroup := range t.data.apiGroups { - for _, ra := range ras { - copy := ra - copy.Group = apiGroup - apiGroupExpansions = append(apiGroupExpansions, copy) + if len(ras) > 0 { + for _, ra := range ras { + copy := ra + copy.Group = apiGroup + apiGroupExpansions = append(apiGroupExpansions, copy) + } + } else { + ra := authv1.ResourceAttributes{ + Group: apiGroup, + } + apiGroupExpansions = append(apiGroupExpansions, ra) } } // we update the expanded list with apiGroup expansions @@ -148,24 +165,44 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { resourceExpansions := make([]authv1.ResourceAttributes, 0) if len(t.data.resources) > 0 { for _, resource := range t.data.resources { - for _, ra := range ras { - copy := ra + if len(ras) > 0 { + for _, ra := range ras { + copy := ra + // split the resource string to get the group, resource and subresource + parts := strings.Split(resource, "/") + if len(parts) > 1 { + switch len(parts) { + case 2: + copy.Group = parts[0] + copy.Resource = parts[1] + case 3: + copy.Group = parts[0] + copy.Resource = parts[1] + copy.Subresource = parts[2] + } + } else { + copy.Resource = parts[0] + } + resourceExpansions = append(resourceExpansions, copy) + } + } else { + ra := authv1.ResourceAttributes{} // split the resource string to get the group, resource and subresource parts := strings.Split(resource, "/") if len(parts) > 1 { switch len(parts) { case 2: - copy.Group = parts[0] - copy.Resource = parts[1] + ra.Group = parts[0] + ra.Resource = parts[1] case 3: - copy.Group = parts[0] - copy.Resource = parts[1] - copy.Subresource = parts[2] + ra.Group = parts[0] + ra.Resource = parts[1] + ra.Subresource = parts[2] } } else { - copy.Resource = parts[0] + ra.Resource = parts[0] } - resourceExpansions = append(resourceExpansions, copy) + resourceExpansions = append(resourceExpansions, ra) } } // we update the expanded list with resource expansions @@ -176,10 +213,17 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { subresourceExpansions := make([]authv1.ResourceAttributes, 0) if len(t.data.subresources) > 0 { for _, subresource := range t.data.subresources { - for _, ra := range ras { - copy := ra - copy.Subresource = subresource - subresourceExpansions = append(subresourceExpansions, copy) + if len(ras) > 0 { + for _, ra := range ras { + copy := ra + copy.Subresource = subresource + subresourceExpansions = append(subresourceExpansions, copy) + } + } else { + ra := authv1.ResourceAttributes{ + Subresource: subresource, + } + subresourceExpansions = append(subresourceExpansions, ra) } } // we update the expanded list with subresource expansions @@ -190,10 +234,17 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { nameExpansions := make([]authv1.ResourceAttributes, 0) if len(t.data.names) > 0 { for _, name := range t.data.names { - for _, ra := range ras { - copy := ra - copy.Name = name - nameExpansions = append(nameExpansions, copy) + if len(ras) > 0 { + for _, ra := range ras { + copy := ra + copy.Name = name + nameExpansions = append(nameExpansions, copy) + } + } else { + ra := authv1.ResourceAttributes{ + Name: name, + } + nameExpansions = append(nameExpansions, ra) } } // we update the expanded list with name expansions From 0566c2a5a7a7b12aa9dbc4c9a4331b35e865ccb4 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 11:45:12 +0100 Subject: [PATCH 10/18] add test cases for PowerUser, Manual and Emergency groups --- test/e2e/authorization.go | 130 ++++++++++++++++++++++++++------ test/e2e/authorization_utils.go | 5 ++ 2 files changed, 114 insertions(+), 21 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index ab1eb5fcd1..ee5326931f 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -11,17 +11,17 @@ import ( var ( allGroups = [][]string{ - []string{"FooBar"}, - []string{"ReadOnly"}, - []string{"PowerUser"}, - []string{"Emergency"}, - []string{"Manual"}, - []string{"system:serviceaccounts:kube-system"}, - []string{"CollaboratorEmergency"}, - []string{"CollaboratorManual"}, - []string{"Collaborator24x7"}, - []string{"CollaboratorPowerUser"}, - []string{"Administrator"}, + {"FooBar"}, + {"ReadOnly"}, + {"PowerUser"}, + {"Emergency"}, + {"Manual"}, + {"system:serviceaccounts:kube-system"}, + {"CollaboratorEmergency"}, + {"CollaboratorManual"}, + {"Collaborator24x7"}, + {"CollaboratorPowerUser"}, + {"Administrator"}, } ) @@ -141,7 +141,6 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { "nodes", "rbac.authorization.k8s.io/clusterroles", "storage.k8s.io/storageclasses", - "policy/podsecuritypolicies", "apiextensions.k8s.io/customresourcedefinitions", } g.It("should allow read access", func() { @@ -162,20 +161,109 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.Context("For PowerUser, Manual and Emergency groups", func() { + var tc testCase + g.BeforeEach(func() { + tc.data.groups = [][]string{ + {"PowerUser"}, + {"Manual"}, + {"Emergency"}, + } + tc.data.users = []string{"test-user"} + }) - g.It("should deny read access to Secrets in kube-system and visibility namespaces", func() {}) - g.It("should deny write access to Nodes", func() {}) - g.It("should deny write access to DaemonSets", func() {}) - g.It("should deny deleting CRDs", func() {}) - g.It("should deny deleting kube-system or visibility namespaces", func() {}) + g.It("should deny read access to Secrets in kube-system and visibility namespaces", func() { + tc.data.resources = []string{"secrets"} + tc.data.namespaces = []string{"kube-system", "visibility"} + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + + g.It("should deny write access to Nodes", func() { + tc.data.resources = []string{"nodes"} + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + + g.It("should deny write access to DaemonSets", func() { + tc.data.resources = []string{"apps/daemonsets"} + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + + // TODO: Double check if the original test case is correct + g.It("should allow deleting CRDs", func() { + tc.data.resources = []string{"apiextensions.k8s.io/customresourcedefinitions"} + tc.data.verbs = []string{"delete"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.allowed).To(gomega.BeTrue()) + }) + + g.It("should deny deleting kube-system or visibility namespaces", func() { + tc.data.resources = []string{"namespaces"} + tc.data.namespaces = []string{"kube-system", "visibility"} + tc.data.verbs = []string{"delete"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) g.When("the resource is a namespaced resource", func() { - g.It("should deny write access in kube-system and visibility namespaces", func() {}) - g.It("should allow write access in namespaces other than kube-system and visibility", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{ + "pods", + "apps/deployments", + "apps/statefulsets", + "apps/deployments/scale", + "apps/statefulsets/scale", + "services", + "persistentvolumes", + "persistentvolumeclaims", + "configmaps", + } + tc.data.verbs = []string{"create", "update", "delete", "patch"} + }) + g.It("should deny write access in kube-system and visibility namespaces", func() { + tc.data.namespaces = []string{"kube-system", "visibility"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + g.It("should allow write access in namespaces other than kube-system and visibility", func() { + tc.data.namespaces = []string{"", "teapot"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.allowed).To(gomega.BeTrue(), + "Reason: %v", output.reason) + }) }) g.When("the resource is a global resource", func() { - g.It("should deny access to Nodes", func() {}) - g.It("should allow access to resources other than Nodes", func() {}) + g.BeforeEach(func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + }) + g.It("should deny write access to Nodes", func() { + tc.data.resources = []string{"nodes"} + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.denied).To(gomega.BeTrue()) + }) + g.It("should allow write access to resources other than Nodes", func() { + tc.data.resources = []string{ + "namespaces", + "storage.k8s.io/storageclasses", + "apiextensions.k8s.io/customresourcedefinitions", + } + tc.run(context.TODO(), cs) + output := tc.output + gomega.Expect(output.allowed).To(gomega.BeTrue(), + "Reason: %v", output.reason) + }) }) }) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 0541d6fcaa..0bb86f2aec 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -311,6 +311,11 @@ func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar // evaluateOutput evaluates the output based on the created SubjectAccessReview objects func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview) { tcOutput := testcaseOutput{} + // TODO: Test should only pass if all SubjectAccessReviews have expected + // value. Need to rethink this composition logic. + // For example if we have 3 SubjectAccessReviews and the expecataion is 'deny', + // then ALL 3 of them should have a 'denied: true' in response. In this implementation + // even if 1 of them was denied, the test would pass even if the other 2 were allowed. // Iterate over all the SubjectAccessReviews created and determine the final result // We don't break the loop if we have denied access from one SubjectAccessReview, From cdc0ee75606e8bb50ce5b0a7a65d448399c10b37 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 19:51:35 +0100 Subject: [PATCH 11/18] Refactor how the testCase output is evaluated We now match each SubjectAccessReview object with the expected result. We then populate a list of all those that didn't meet expectation. If a test fails, we use the String() function on the testCaseOutput to print all these violating SARs. If there are not such SARs, we consider the test passed. --- test/e2e/authorization.go | 96 +++++++++++++-------------------- test/e2e/authorization_utils.go | 92 +++++++++++++++++++------------ 2 files changed, 94 insertions(+), 94 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index ee5326931f..8ab4d2af33 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -52,17 +52,14 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { // will clear these values for other specs. // https://onsi.github.io/ginkgo/#organizing-specs-with-container-nodes tc.data.resources = []string{"users", "groups"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) - + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny access for service accounts", func() { tc.data.resources = []string{"serviceaccounts"} tc.data.namespaces = []string{"", "teapot", "kube-system"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) g.When("the verb is escalate", func() { @@ -72,16 +69,14 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { g.It("should deny access for cluster roles", func() { tc.data.resources = []string{"rbac.authorization.k8s.io/clusterrole"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny access for roles in all namespaces", func() { tc.data.resources = []string{"rbac.authorization.k8s.io/role"} tc.data.namespaces = []string{"", "teapot", "kube-system"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) }) @@ -99,9 +94,8 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { g.It("should deny access in all namespaces", func() { tc.data.verbs = []string{"get", "list", "watch", "create", "update", "delete", "patch"} tc.data.namespaces = []string{"", "teapot", "kube-system"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) g.When("the resource is not a Secret resource", func() { @@ -122,16 +116,13 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.It("should allow read access in all namespaces", func() { tc.data.verbs = []string{"get", "list", "watch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.allowed).To(gomega.BeTrue(), - "Reason: %v", output.reason) + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny write access in all namespaces", func() { tc.data.verbs = []string{"create", "update", "delete", "patch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) g.When("the resource is a global resource", func() { @@ -145,16 +136,13 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { } g.It("should allow read access", func() { tc.data.verbs = []string{"get", "list", "watch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.allowed).To(gomega.BeTrue(), - "Reason: %v", output.reason) + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny write access", func() { tc.data.verbs = []string{"create", "update", "delete", "patch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) }) @@ -175,43 +163,37 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { tc.data.resources = []string{"secrets"} tc.data.namespaces = []string{"kube-system", "visibility"} tc.data.verbs = []string{"get", "list", "watch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny write access to Nodes", func() { tc.data.resources = []string{"nodes"} tc.data.verbs = []string{"create", "update", "delete", "patch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny write access to DaemonSets", func() { tc.data.resources = []string{"apps/daemonsets"} tc.data.verbs = []string{"create", "update", "delete", "patch"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) - // TODO: Double check if the original test case is correct g.It("should allow deleting CRDs", func() { tc.data.resources = []string{"apiextensions.k8s.io/customresourcedefinitions"} tc.data.verbs = []string{"delete"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.allowed).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should deny deleting kube-system or visibility namespaces", func() { tc.data.resources = []string{"namespaces"} tc.data.namespaces = []string{"kube-system", "visibility"} tc.data.verbs = []string{"delete"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.When("the resource is a namespaced resource", func() { @@ -231,16 +213,13 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.It("should deny write access in kube-system and visibility namespaces", func() { tc.data.namespaces = []string{"kube-system", "visibility"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should allow write access in namespaces other than kube-system and visibility", func() { tc.data.namespaces = []string{"", "teapot"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.allowed).To(gomega.BeTrue(), - "Reason: %v", output.reason) + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) g.When("the resource is a global resource", func() { @@ -249,9 +228,8 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.It("should deny write access to Nodes", func() { tc.data.resources = []string{"nodes"} - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.denied).To(gomega.BeTrue()) + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) g.It("should allow write access to resources other than Nodes", func() { tc.data.resources = []string{ @@ -259,10 +237,8 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { "storage.k8s.io/storageclasses", "apiextensions.k8s.io/customresourcedefinitions", } - tc.run(context.TODO(), cs) - output := tc.output - gomega.Expect(output.allowed).To(gomega.BeTrue(), - "Reason: %v", output.reason) + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) }) }) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 0bb86f2aec..b7611f5126 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -2,6 +2,7 @@ package e2e import ( "context" + "strconv" "strings" authv1 "k8s.io/api/authorization/v1" @@ -41,12 +42,24 @@ type testcaseData struct { // and we need to determine the "final" result and expose that as the testcase output. type testcaseOutput struct { // the final result based on results of individual SubjectAccessReview objects - allowed, denied bool - // the set of reasons from individual SubjectAccessReview objects - reason []string + passed bool + // the set of SARs that failed expectation in the test. This is an empty slice if the test passed. + // It contains pretty printed SAR objects for debugging, when a test fails. It will + // contain all the SAR objects that didn't match expectation. + failingSARs []string } -func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { +// String returns a pretty printed string of the testcase output. This is used +// to help debug the RBAC test cases. +func (o *testcaseOutput) String() string { + outputStr := "" + for _, sar := range o.failingSARs { + outputStr += sar + } + return outputStr +} + +func (t *testCase) run(ctx context.Context, cs kubernetes.Interface, allowExpected bool) error { // Generate the list of SubjectAccessReview objects based on the testcase data sars := t.generateSubjectAccessReviews() @@ -58,7 +71,7 @@ func (t *testCase) run(ctx context.Context, cs kubernetes.Interface) error { // Evaluate the output based on the created SubjectAccessReview objects // and set the final result in the testcase output - t.evaluateOutput(createdSars) + t.evaluateOutput(createdSars, allowExpected) return nil } @@ -309,40 +322,51 @@ func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar } // evaluateOutput evaluates the output based on the created SubjectAccessReview objects -func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview) { - tcOutput := testcaseOutput{} - // TODO: Test should only pass if all SubjectAccessReviews have expected - // value. Need to rethink this composition logic. - // For example if we have 3 SubjectAccessReviews and the expecataion is 'deny', - // then ALL 3 of them should have a 'denied: true' in response. In this implementation - // even if 1 of them was denied, the test would pass even if the other 2 were allowed. - - // Iterate over all the SubjectAccessReviews created and determine the final result - // We don't break the loop if we have denied access from one SubjectAccessReview, - // we continue to check all of them and collect all reasons. +// allowExpected is a boolean that determines if the expected result is 'allow' or 'deny'. +func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview, allowExpected bool) { + + //TODO: check if it's safe to override the output object of the testcase like this + + // Iterate over all the SubjectAccessReviews created and check for expecated result. + // We don't break the loop if a result doesn't match expectation since we want to + // capture all the failing SubjectAccessReviews for debugging. for _, sar := range createdSars { - // We skip the SubjectAccessReviews that are allowed - if sar.Status.Allowed { - continue - } - // If any of the SubjectAccessReviews have denied access, the final result is denied - if sar.Status.Denied { - tcOutput.denied = true - tcOutput.reason = append(tcOutput.reason, sar.Status.Reason) - continue + // if the expected result is 'allow' and the SAR is denied, we add it to the failingSARs + if allowExpected && !sar.Status.Allowed { + t.output.failingSARs = append(t.output.failingSARs, prettyPrintSAR(sar)) } - // Undecided access is also considered as denied - if !sar.Status.Allowed && !sar.Status.Denied { - tcOutput.denied = true - tcOutput.reason = append(tcOutput.reason, sar.Status.Reason) - continue + // if the expected result is 'deny' and the SAR is allowed, we add it to the failingSARs + if !allowExpected && sar.Status.Allowed { + t.output.failingSARs = append(t.output.failingSARs, prettyPrintSAR(sar)) } } - // If none of the SubjectAccessReviews have denied access, the final result is allowed - if !tcOutput.denied { - tcOutput.allowed = true + // if the failingSARs is empty, it means the test passed + if len(t.output.failingSARs) == 0 { + t.output.passed = true } +} - t.output = tcOutput +// prettyPrintSAR pretty prints the SubjectAccessReview object. This is used +// to help debug the RBAC test cases. +func prettyPrintSAR(sar authv1.SubjectAccessReview) string { + str := "SubjectAccessReviewSpec:" + str += "\n Namespace: " + sar.Spec.ResourceAttributes.Namespace + str += "\n Verb: " + sar.Spec.ResourceAttributes.Verb + str += "\n APIGroup: " + sar.Spec.ResourceAttributes.Group + str += "\n Resource: " + sar.Spec.ResourceAttributes.Resource + str += "\n Subresource: " + sar.Spec.ResourceAttributes.Subresource + str += "\n Name: " + sar.Spec.ResourceAttributes.Name + if sar.Spec.NonResourceAttributes != nil { + str += "\n NonResourcePath: " + sar.Spec.NonResourceAttributes.Path + str += "\n NonResourceVerb: " + sar.Spec.NonResourceAttributes.Verb + } + str += "\n User: " + sar.Spec.User + str += "\n Groups: " + strings.Join(sar.Spec.Groups, ",") + str += "\nSubjectAccessReviewStatus:" + str += "\n Allowed: " + strconv.FormatBool(sar.Status.Allowed) + str += "\n Denied: " + strconv.FormatBool(sar.Status.Denied) + str += "\n Reason: " + sar.Status.Reason + str += "\n" + return str } From 5bae116c761292e953ed9aface19035a18732a21 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 21:51:15 +0100 Subject: [PATCH 12/18] add all expansions in SAR generation --- test/e2e/authorization_utils.go | 142 ++++++++++++++++++++++++++------ 1 file changed, 115 insertions(+), 27 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index b7611f5126..442f4626af 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -82,38 +82,34 @@ func (t *testCase) generateSubjectAccessReviews() []authv1.SubjectAccessReview { // Initialize the list of SubjectAccessReview objects sars := make([]authv1.SubjectAccessReview, 0) - // expand the testcase data to generate a list of ResourceAttributes + // expand the testcaseData to generate a list of ResourceAttributes resourceAttributes := t.expandResourceAttributes() - // expand the testcase data to generate a list of NonResourceAttributes - // nonResourceAttributes := t.expandNonResourceAttributes() + // expand the testcaseData to generate a list of NonResourceAttributes + nonResourceAttributes := t.expandNonResourceAttributes() - // expand the testcase data to generate a list of SubjectAccessReview objects - // based on the ResourceAttributes and NonResourceAttributes - for _, ra := range resourceAttributes { - for _, user := range t.data.users { - for _, group := range t.data.groups { - sar := authv1.SubjectAccessReview{ - Spec: authv1.SubjectAccessReviewSpec{ - ResourceAttributes: &ra, - User: user, - Groups: group, - }, - } - sars = append(sars, sar) - } + // expand users and groups + userGroupExpansions := t.expandUsersAndGroups() + + // expand the ResourceAttributes, NonResourceAttributes and UserGroupExpansions + // to generate a list of SubjectAccessReviewSpec objects + sarSpecs := t.expandSubjectAccessReviewSpecs(resourceAttributes, nonResourceAttributes, userGroupExpansions) + + // create the SubjectAccessReview objects based on the SubjectAccessReviewSpec objects + for _, sarSpec := range sarSpecs { + sar := authv1.SubjectAccessReview{ + Spec: sarSpec, } + sars = append(sars, sar) } return sars } -// expandResourceAttributes expands the testcase data to generate a list of ResourceAttributes +// expandResourceAttributes expands the testcaseData to generate a list of ResourceAttributes func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { // This will hold the expanded ResourceAttributes ras := make([]authv1.ResourceAttributes, 0) - // TODO: Convert this logic in a function similar to the way it is implemented - // today to avoid code duplication nsExpansions := make([]authv1.ResourceAttributes, 0) // expand on namespaces if len(t.data.namespaces) > 0 { @@ -267,7 +263,7 @@ func (t *testCase) expandResourceAttributes() []authv1.ResourceAttributes { return ras } -// expandNonResourceAttributes expands the testcase data to generate a list of NonResourceAttributes +// expandNonResourceAttributes expands the testcaseData to generate a list of NonResourceAttributes func (t *testCase) expandNonResourceAttributes() []authv1.NonResourceAttributes { // This will hold the expanded NonResourceAttributes nras := make([]authv1.NonResourceAttributes, 0) @@ -289,10 +285,17 @@ func (t *testCase) expandNonResourceAttributes() []authv1.NonResourceAttributes verbExpansions := make([]authv1.NonResourceAttributes, 0) if len(t.data.nonResourceVerbs) > 0 { for _, verb := range t.data.nonResourceVerbs { - for _, nra := range nras { - copy := nra - copy.Verb = verb - verbExpansions = append(verbExpansions, copy) + if len(nras) > 0 { + for _, nra := range nras { + copy := nra + copy.Verb = verb + verbExpansions = append(verbExpansions, copy) + } + } else { + nra := authv1.NonResourceAttributes{ + Verb: verb, + } + verbExpansions = append(verbExpansions, nra) } } // we update the expanded list with verb expansions @@ -302,6 +305,93 @@ func (t *testCase) expandNonResourceAttributes() []authv1.NonResourceAttributes return nras } +// expandUsersAndGroups expands the users and groups in the testcaseData to generate a list of SubjectAccessReviewSpecs +func (t *testCase) expandUsersAndGroups() []authv1.SubjectAccessReviewSpec { + // This will hold the expanded SubjectAccessReviewSpec + sars := make([]authv1.SubjectAccessReviewSpec, 0) + + // expand on users + userExpansions := make([]authv1.SubjectAccessReviewSpec, 0) + if len(t.data.users) > 0 { + for _, user := range t.data.users { + sar := authv1.SubjectAccessReviewSpec{ + User: user, + } + userExpansions = append(userExpansions, sar) + } + // we update the expanded list with user expansions + sars = userExpansions + } + + // expand on groups + groupExpansions := make([]authv1.SubjectAccessReviewSpec, 0) + if len(t.data.groups) > 0 { + for _, group := range t.data.groups { + // If an expansion already took place, we need to copy the + // existing objects and change the groups + if len(sars) > 0 { + for _, sar := range sars { + copy := sar + copy.Groups = group + groupExpansions = append(groupExpansions, copy) + } + } else { + // If no expansion has taken place, we need to create a new object + sar := authv1.SubjectAccessReviewSpec{ + Groups: group, + } + groupExpansions = append(groupExpansions, sar) + } + } + // we update the expanded list with group expansions + sars = groupExpansions + } + + return sars +} + +// expandSubjectAccessReviewSpecs takes the expanded ResourceAttributes, NonResourceAttributes and +// UserGroupExpansions and generates a list of SubjectAccessReviewSpec objects +func (t *testCase) expandSubjectAccessReviewSpecs(ras []authv1.ResourceAttributes, nras []authv1.NonResourceAttributes, userGroupExpansions []authv1.SubjectAccessReviewSpec) []authv1.SubjectAccessReviewSpec { + sars := make([]authv1.SubjectAccessReviewSpec, 0) + + // expand on ResourceAttributes if they are defined + rasExpansions := make([]authv1.SubjectAccessReviewSpec, 0) + if len(ras) > 0 { + for _, ra := range ras { + for _, ug := range userGroupExpansions { + sar := authv1.SubjectAccessReviewSpec{ + ResourceAttributes: &ra, + User: ug.User, + Groups: ug.Groups, + } + rasExpansions = append(rasExpansions, sar) + } + } + // we update the expanded list with ResourceAttributes expansions + sars = rasExpansions + } + + // expand on NonResourceAttributes if they are defined + nrasExpansions := make([]authv1.SubjectAccessReviewSpec, 0) + if len(nras) > 0 { + for _, nra := range nras { + for _, ug := range userGroupExpansions { + sar := authv1.SubjectAccessReviewSpec{ + NonResourceAttributes: &nra, + User: ug.User, + Groups: ug.Groups, + } + nrasExpansions = append(nrasExpansions, sar) + } + } + // we update the expanded list with NonResourceAttributes expansions + sars = nrasExpansions + } + + return sars +} + // createSubjectAccessReviews creates provided SubjectAccessReview objects in the cluster func createSubjectAccessReviews(ctx context.Context, cs kubernetes.Interface, sars []authv1.SubjectAccessReview) ([]authv1.SubjectAccessReview, error) { createdSars := make([]authv1.SubjectAccessReview, 0) @@ -325,8 +415,6 @@ func createSubjectAccessReview(ctx context.Context, cs kubernetes.Interface, sar // allowExpected is a boolean that determines if the expected result is 'allow' or 'deny'. func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview, allowExpected bool) { - //TODO: check if it's safe to override the output object of the testcase like this - // Iterate over all the SubjectAccessReviews created and check for expecated result. // We don't break the loop if a result doesn't match expectation since we want to // capture all the failing SubjectAccessReviews for debugging. From 766d21d7e65889974a98c6c5120ebeae1c5eaba8 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Thu, 21 Nov 2024 22:55:42 +0100 Subject: [PATCH 13/18] implement all the testcase specs --- test/e2e/authorization.go | 322 ++++++++++++++++++++++++++++++++++---- 1 file changed, 290 insertions(+), 32 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index 8ab4d2af33..cadb151492 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -244,76 +244,334 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.Context("For CollaboratorPowerUser, CollaboratorManual and CollaboratorEmergency groups", func() { + var tc testCase + g.BeforeEach(func() { + tc.data.groups = [][]string{ + {"CollaboratorPowerUser", "PowerUser"}, + {"CollaboratorManual", "Manual"}, + {"CollaboratorEmergency", "Emergency"}, + } + tc.data.users = []string{"test-user"} + }) + g.When("the resource is a Secret", func() { - g.It("should allow read access to visibility namespace", func() {}) - g.It("should deny read access to kube-system namespace", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{"secrets"} + tc.data.verbs = []string{"get", "list", "watch"} + }) + + g.It("should allow read access to visibility namespace", func() { + tc.data.namespaces = []string{"visibility"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should deny read access to kube-system namespace", func() { + tc.data.namespaces = []string{"kube-system"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) - g.It("should deny write access to Nodes", func() {}) - g.It("should allow write access to DaemonSets", func() {}) - g.It("should allow deletion of CRDs", func() {}) - g.It("should deny deletion of kube-system or visibility namespaces", func() {}) + g.It("should deny write access to Nodes", func() { + tc.data.resources = []string{"nodes"} + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow write access to DaemonSets", func() { + tc.data.resources = []string{"apps/daemonsets"} + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow deletion of CRDs", func() { + tc.data.resources = []string{"apiextensions.k8s.io/customresourcedefinitions"} + tc.data.verbs = []string{"delete"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should deny deletion of kube-system or visibility namespaces", func() { + tc.data.resources = []string{"namespaces"} + tc.data.namespaces = []string{"kube-system", "visibility"} + tc.data.verbs = []string{"delete"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) g.When("the resource is a namespaced resource", func() { - g.It("should deny write access in kube-system namespace", func() {}) - g.It("should allow write access in namespaces other than kube-system", func() {}) + g.BeforeEach(func() { + tc.data.resources = []string{ + "pods", + "apps/deployments", + "apps/statefulsets", + "services", + "persistentvolumes", + "persistentvolumeclaims", + "configmaps", + } + tc.data.verbs = []string{"create", "update", "delete", "patch"} + }) + g.It("should deny write access in kube-system namespace", func() { + tc.data.namespaces = []string{"kube-system"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow write access in namespaces other than kube-system", func() { + tc.data.namespaces = []string{"", "teapot"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the resource is a global resource", func() { - g.It("should deny access to Nodes", func() {}) - g.It("should allow access to resources other than Nodes", func() {}) + g.BeforeEach(func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + }) + g.It("should deny access to Nodes", func() { + tc.data.resources = []string{"nodes"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow access to resources other than Nodes", func() { + tc.data.resources = []string{ + "namespaces", + "storage.k8s.io/storageclasses", + "apiextensions.k8s.io/customresourcedefinitions", + } + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) }) g.Context("For system users", func() { + var tc testCase + g.When("the user is kubelet", func() { - g.It("should allow to get Pods", func() {}) + g.BeforeEach(func() { + tc.data.groups = [][]string{{"system:masters"}} + tc.data.users = []string{"kubelet"} + }) + g.It("should allow to get Pods", func() { + tc.data.resources = []string{"pods"} + tc.data.verbs = []string{"get"} + tc.data.namespaces = []string{"teapot"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the service account is daemonset-controller", func() { - g.It("should allow to update DaemonSet status subresource", func() {}) - g.It("should allow to update DaemonSet finalizers", func() {}) - g.It("should allow to create Pods", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:kube-system:daemon-set-controller"} + tc.data.groups = [][]string{{"system:serviceaccounts:kube-system"}} + }) + g.It("should allow to update DaemonSet status subresource", func() { + tc.data.resources = []string{"apps/daemonsets/status"} + tc.data.verbs = []string{"update"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow to update DaemonSet finalizers", func() { + tc.data.resources = []string{"apps/daemonsets/finalizers"} + tc.data.verbs = []string{"update"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + // TODO: Need to verify in the original tests if this is a permission on + // the controller-manager or the daemonset-controller. + // g.It("should allow to create Pods", func() {}) + }) + + g.When("the default service account is in default namespace", func() { + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:default:default"} + }) + g.It("should deny to list StatefulSets", func() { + tc.data.resources = []string{"apps/statefulsets"} + tc.data.verbs = []string{"list"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) - g.When("the service account is default", func() { - g.It("should deny to list StatefulSets when in default namespace", func() {}) - g.It("should deny to list StatefulSets when in non-default namespace", func() {}) + g.When("the default service account is in non-default namespace", func() { + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:non-default:default"} + }) + g.It("should deny to list StatefulSets", func() { + tc.data.resources = []string{"apps/statefulsets"} + tc.data.verbs = []string{"list"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the service account is persistent-volume-binder", func() { - g.It("should allow to update PersistentVolumeClaims", func() {}) - g.It("should allow to create PersistentVolumes", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:kube-system:persistent-volume-binder"} + tc.data.groups = [][]string{{"system:serviceaccounts:kube-system"}} + tc.data.namespaces = []string{"kube-system"} + }) + g.It("should allow to update PersistentVolumeClaims", func() { + tc.data.resources = []string{"persistentvolumeclaims"} + tc.data.verbs = []string{"update"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow to create PersistentVolumes", func() { + tc.data.resources = []string{"persistentvolumes"} + tc.data.verbs = []string{"create"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the service account is aws-cloud-provider", func() { - g.It("should allow to patch Nodes", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:kube-system:aws-cloud-provider"} + tc.data.groups = [][]string{{"system:serviceaccounts:kube-system"}} + }) + g.It("should allow to patch Nodes", func() { + tc.data.resources = []string{"nodes"} + tc.data.verbs = []string{"patch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the service account is api-monitoring-controller", func() { - g.It("should allow to update the skipper-default-filters ConfigMap in kube-system namespace", func() {}) - g.It("should deny to update ConfigMaps other than skipper-default-filters", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"system:serviceaccount:api-infrastructure:api-monitoring-controller"} + }) + g.When("the namespace is kube-system", func() { + g.BeforeEach(func() { + tc.data.namespaces = []string{"kube-system"} + }) + g.It("should allow to update 'skipper-default-filters' ConfigMap", func() { + tc.data.resources = []string{"configmaps"} + tc.data.verbs = []string{"update"} + tc.data.names = []string{"skipper-default-filters"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should deny to update any other ConfigMap", func() { + tc.data.resources = []string{"configmaps"} + tc.data.verbs = []string{"update"} + // Technically, this should result in access undecided because we allow + // access to 'skipper-default-filters' ConfigMap only and we haven't + // specified a resource name in the test case. + // We consider access undecided cases also as denied. + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) }) g.When("the user is k8sapi_credentials-provider", func() { - g.It("should allow to get Secrets in kube-system namespace", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"zalando-iam:zalando:service:k8sapi_credentials-provider"} + }) + g.It("should allow to get Secrets in kube-system namespace", func() { + tc.data.resources = []string{"secrets"} + tc.data.namespaces = []string{"kube-system"} + tc.data.verbs = []string{"get"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) }) g.When("the user is stups_cdp-controller", func() { - g.It("should deny access to Secrets in kube-system namespace", func() {}) + g.BeforeEach(func() { + tc.data.users = []string{"zalando-iam:zalando:service:stups_cdp-controller"} + }) + g.When("the namespace is kube-system", func() { + g.BeforeEach(func() { + tc.data.namespaces = []string{"kube-system"} + }) + g.It("should deny to get Secrets", func() { + tc.data.resources = []string{"secrets"} + tc.data.verbs = []string{"get"} + tc.run(context.TODO(), cs, false) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) }) }) g.Context("For administrators", func() { - g.It("should allow read access to resources other than Secrets in kube-system namespace", func() {}) - g.It("should allow write access to resources other than Secrets in kube-system namespace", func() {}) - g.It("should allow read access to Secrets in kube-system namespace", func() {}) - g.It("should allow read access to Secrets in namespaces other than kube-system", func() {}) - g.It("should allow write access to namespaces other than kube-system", func() {}) - g.It("should allow to proxy", func() {}) - g.It("should allow write access to DaemonSets", func() {}) - }) + var tc testCase + g.BeforeEach(func() { + tc.data.groups = [][]string{{"system:masters"}} + tc.data.users = []string{"nmalik"} + }) + + g.When("namespace is kube-system", func() { + g.BeforeEach(func() { + tc.data.namespaces = []string{"kube-system"} + }) + g.When("the resource is a Secret", func() { + g.BeforeEach(func() { + tc.data.resources = []string{"secrets"} + }) + g.It("should allow read access", func() { + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) + + g.When("the resource is not a Secret", func() { + g.BeforeEach(func() { + tc.data.resources = []string{"pods"} + }) + g.It("should allow read access", func() { + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + g.It("should allow write access", func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) + }) + + g.When("namespace is not kube-system", func() { + g.BeforeEach(func() { + tc.data.namespaces = []string{"teapot"} + }) + + g.It("should allow to proxy", func() { + tc.data.verbs = []string{"proxy"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + + g.When("the resource is a Secret", func() { + g.BeforeEach(func() { + tc.data.resources = []string{"secrets"} + }) + g.It("should allow read access", func() { + tc.data.verbs = []string{"get", "list", "watch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) + g.When("the resource is not a Secret", func() { + g.BeforeEach(func() { + tc.data.resources = []string{"pods, apps/daemonsets"} + }) + g.It("should allow write access", func() { + tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.run(context.TODO(), cs, true) + gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) + }) + }) + }) + }) }) From 6d59fb5031daf66a45ac33d727ad01aa81cea841 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Fri, 22 Nov 2024 02:45:30 +0100 Subject: [PATCH 14/18] improve pretty printing by filtering out empty values --- test/e2e/authorization_utils.go | 51 +++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 442f4626af..7fd20ec38e 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -438,23 +438,50 @@ func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview, allo // prettyPrintSAR pretty prints the SubjectAccessReview object. This is used // to help debug the RBAC test cases. func prettyPrintSAR(sar authv1.SubjectAccessReview) string { - str := "SubjectAccessReviewSpec:" - str += "\n Namespace: " + sar.Spec.ResourceAttributes.Namespace - str += "\n Verb: " + sar.Spec.ResourceAttributes.Verb - str += "\n APIGroup: " + sar.Spec.ResourceAttributes.Group - str += "\n Resource: " + sar.Spec.ResourceAttributes.Resource - str += "\n Subresource: " + sar.Spec.ResourceAttributes.Subresource - str += "\n Name: " + sar.Spec.ResourceAttributes.Name + + str := "\nSubjectAccessReviewSpec:" + // we print the field values conditionally since some fields might be empty + // this helps in making the output more readable + if ns := sar.Spec.ResourceAttributes.Namespace; ns != "" { + str += "\n Namespace: " + ns + } + if verb := sar.Spec.ResourceAttributes.Verb; verb != "" { + str += "\n Verb: " + verb + } + if group := sar.Spec.ResourceAttributes.Group; group != "" { + str += "\n APIGroup: " + group + } + if resource := sar.Spec.ResourceAttributes.Resource; resource != "" { + str += "\n Resource: " + resource + } + if subresource := sar.Spec.ResourceAttributes.Subresource; subresource != "" { + str += "\n Subresource: " + subresource + } + if name := sar.Spec.ResourceAttributes.Name; name != "" { + str += "\n Name: " + name + } if sar.Spec.NonResourceAttributes != nil { - str += "\n NonResourcePath: " + sar.Spec.NonResourceAttributes.Path - str += "\n NonResourceVerb: " + sar.Spec.NonResourceAttributes.Verb + if verb := sar.Spec.NonResourceAttributes.Verb; verb != "" { + str += "\n NonResourceVerb: " + verb + } + if path := sar.Spec.NonResourceAttributes.Path; path != "" { + str += "\n NonResourcePath: " + path + } + } + if user := sar.Spec.User; user != "" { + str += "\n User: " + user + } + if groups := sar.Spec.Groups; len(groups) > 0 { + str += "\n Groups: " + strings.Join(groups, ",") } - str += "\n User: " + sar.Spec.User - str += "\n Groups: " + strings.Join(sar.Spec.Groups, ",") str += "\nSubjectAccessReviewStatus:" + // these fields are always present in the SubjectAccessReviewStatus str += "\n Allowed: " + strconv.FormatBool(sar.Status.Allowed) str += "\n Denied: " + strconv.FormatBool(sar.Status.Denied) - str += "\n Reason: " + sar.Status.Reason + + if reason := sar.Status.Reason; reason != "" { + str += "\n Reason: " + reason + } str += "\n" return str } From 66e2269e0f9982b81260a04fab8b32f8a4e6e356 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Fri, 22 Nov 2024 02:59:38 +0100 Subject: [PATCH 15/18] cleanup pretty printing, add a comment --- test/e2e/authorization.go | 2 ++ test/e2e/authorization_utils.go | 53 ++++++++++++--------------------- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index cadb151492..278c0d5800 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -247,6 +247,8 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { var tc testCase g.BeforeEach(func() { tc.data.groups = [][]string{ + // Collaborator groups can escalate privileges to their respective groups + // so, we need to include the respective group in the list as well. {"CollaboratorPowerUser", "PowerUser"}, {"CollaboratorManual", "Manual"}, {"CollaboratorEmergency", "Emergency"}, diff --git a/test/e2e/authorization_utils.go b/test/e2e/authorization_utils.go index 7fd20ec38e..0db109640e 100644 --- a/test/e2e/authorization_utils.go +++ b/test/e2e/authorization_utils.go @@ -439,49 +439,34 @@ func (t *testCase) evaluateOutput(createdSars []authv1.SubjectAccessReview, allo // to help debug the RBAC test cases. func prettyPrintSAR(sar authv1.SubjectAccessReview) string { + // helper function to print the field values conditionally + ifNotNil := func(k, v string) string { + if v != "" { + return "\n " + k + ": " + v + } + return "" + } + str := "\nSubjectAccessReviewSpec:" // we print the field values conditionally since some fields might be empty // this helps in making the output more readable - if ns := sar.Spec.ResourceAttributes.Namespace; ns != "" { - str += "\n Namespace: " + ns - } - if verb := sar.Spec.ResourceAttributes.Verb; verb != "" { - str += "\n Verb: " + verb - } - if group := sar.Spec.ResourceAttributes.Group; group != "" { - str += "\n APIGroup: " + group - } - if resource := sar.Spec.ResourceAttributes.Resource; resource != "" { - str += "\n Resource: " + resource - } - if subresource := sar.Spec.ResourceAttributes.Subresource; subresource != "" { - str += "\n Subresource: " + subresource - } - if name := sar.Spec.ResourceAttributes.Name; name != "" { - str += "\n Name: " + name - } + str += ifNotNil("Namespace", sar.Spec.ResourceAttributes.Namespace) + str += ifNotNil("Verb", sar.Spec.ResourceAttributes.Verb) + str += ifNotNil("Group", sar.Spec.ResourceAttributes.Group) + str += ifNotNil("Resource", sar.Spec.ResourceAttributes.Resource) + str += ifNotNil("Subresource", sar.Spec.ResourceAttributes.Subresource) + str += ifNotNil("Name", sar.Spec.ResourceAttributes.Name) if sar.Spec.NonResourceAttributes != nil { - if verb := sar.Spec.NonResourceAttributes.Verb; verb != "" { - str += "\n NonResourceVerb: " + verb - } - if path := sar.Spec.NonResourceAttributes.Path; path != "" { - str += "\n NonResourcePath: " + path - } - } - if user := sar.Spec.User; user != "" { - str += "\n User: " + user - } - if groups := sar.Spec.Groups; len(groups) > 0 { - str += "\n Groups: " + strings.Join(groups, ",") + str += ifNotNil("Path", sar.Spec.NonResourceAttributes.Path) + str += ifNotNil("Verb", sar.Spec.NonResourceAttributes.Verb) } + str += ifNotNil("User", sar.Spec.User) + str += ifNotNil("Groups", strings.Join(sar.Spec.Groups, ",")) str += "\nSubjectAccessReviewStatus:" // these fields are always present in the SubjectAccessReviewStatus str += "\n Allowed: " + strconv.FormatBool(sar.Status.Allowed) str += "\n Denied: " + strconv.FormatBool(sar.Status.Denied) - - if reason := sar.Status.Reason; reason != "" { - str += "\n Reason: " + reason - } + str += ifNotNil("Reason", sar.Status.Reason) str += "\n" return str } From 06b4c4f6e62e4f9ef8d1ffb7545db6b6ec809a0b Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Fri, 22 Nov 2024 08:08:05 +0100 Subject: [PATCH 16/18] fix test cases --- test/e2e/authorization.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index 278c0d5800..d54283154b 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -283,6 +283,7 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { g.It("should allow write access to DaemonSets", func() { tc.data.resources = []string{"apps/daemonsets"} tc.data.verbs = []string{"create", "update", "delete", "patch"} + tc.data.namespaces = []string{"visibility"} tc.run(context.TODO(), cs, true) gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) }) @@ -294,7 +295,7 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { }) g.It("should deny deletion of kube-system or visibility namespaces", func() { tc.data.resources = []string{"namespaces"} - tc.data.namespaces = []string{"kube-system", "visibility"} + tc.data.names = []string{"kube-system", "visibility"} tc.data.verbs = []string{"delete"} tc.run(context.TODO(), cs, false) gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) From 2b71b456d1351147250c6be907ad2b7576b78221 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Fri, 22 Nov 2024 12:52:03 +0100 Subject: [PATCH 17/18] remove write e2e tests that are to be covered by admission-controller e2e --- test/e2e/authorization.go | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/test/e2e/authorization.go b/test/e2e/authorization.go index d54283154b..f3d33c9ffe 100644 --- a/test/e2e/authorization.go +++ b/test/e2e/authorization.go @@ -211,16 +211,10 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { } tc.data.verbs = []string{"create", "update", "delete", "patch"} }) - g.It("should deny write access in kube-system and visibility namespaces", func() { - tc.data.namespaces = []string{"kube-system", "visibility"} - tc.run(context.TODO(), cs, false) - gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) - }) - g.It("should allow write access in namespaces other than kube-system and visibility", func() { - tc.data.namespaces = []string{"", "teapot"} - tc.run(context.TODO(), cs, true) - gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) - }) + // These should be covered by the admission-controller tests. + // They're written here for completeness. + g.It("should deny write access in kube-system and visibility namespaces", func() {}) + g.It("should allow write access in namespaces other than kube-system and visibility", func() {}) }) g.When("the resource is a global resource", func() { g.BeforeEach(func() { @@ -314,11 +308,9 @@ var _ = g.Describe("Authorization [RBAC] [Zalando]", func() { } tc.data.verbs = []string{"create", "update", "delete", "patch"} }) - g.It("should deny write access in kube-system namespace", func() { - tc.data.namespaces = []string{"kube-system"} - tc.run(context.TODO(), cs, false) - gomega.Expect(tc.output.passed).To(gomega.BeTrue(), tc.output.String()) - }) + // This should be covered by the admission-controller tests. + // It's written here for completeness. + g.It("should deny write access in kube-system namespace", func() {}) g.It("should allow write access in namespaces other than kube-system", func() { tc.data.namespaces = []string{"", "teapot"} tc.run(context.TODO(), cs, true) From 444b75a912a2a08f63d7ce1a4dc37eef491d1ef1 Mon Sep 17 00:00:00 2001 From: Noor Malik Date: Fri, 22 Nov 2024 15:00:26 +0100 Subject: [PATCH 18/18] enable role-sync-controller by default only on EKS --- cluster/config-defaults.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cluster/config-defaults.yaml b/cluster/config-defaults.yaml index a64856484c..bda219be8e 100644 --- a/cluster/config-defaults.yaml +++ b/cluster/config-defaults.yaml @@ -1194,7 +1194,8 @@ teapot_admission_controller_scheduling_controls_enabled: "false" teapot_admission_controller_scheduling_controls_default_architecture: "amd64" # role-sync-controller configs -{{ if eq .Cluster.Environment "e2e" }} +# Enabled by default only on Zalando EKS clusters +{{ if eq .Cluster.Provider "zalando-eks" }} role_sync_controller_enabled: "true" {{ else }} role_sync_controller_enabled: "false"