Audit RBAC, avoid global (*) permissions (#2866)

* Audit rbac, avoid global (*) permissions There are some permissions which are logically not needed, and some others where we can just reduce the verb set allowed. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Avoid unnecessary "persistentvolumes" create permission https://kubernetes.io/docs/concepts/security/rbac-good-practices/#persistent-volume-creation Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Less control over cluster webhooks Following https://kubernetes.io/docs/concepts/security/rbac-good-practices/#control-admission-webhooks We know the names of our validating/mutating webhooks upfront, so we can only allow update/delete on those. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Get rid of implicit escalate/bind verbs on operator roles https://kubernetes.io/docs/concepts/security/rbac-good-practices/#escalate-verb Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt · Aug 31, 2023 · 9b890db · 9b890db
1 parent b67c663
commit 9b890db
Show file tree

Hide file tree

Showing 12 changed files with 213 additions and 78 deletions.
diff --git a/cmd/cdi-controller/BUILD.bazel b/cmd/cdi-controller/BUILD.bazel
@@ -24,6 +24,7 @@ go_library(
         "//vendor/github.com/pkg/errors:go_default_library",
         "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
         "//vendor/go.uber.org/zap/zapcore:go_default_library",
+        "//vendor/k8s.io/api/batch/v1:go_default_library",
         "//vendor/k8s.io/api/networking/v1:go_default_library",
         "//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library",

diff --git a/cmd/cdi-controller/controller.go b/cmd/cdi-controller/controller.go
@@ -15,6 +15,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/zap/zapcore"
+	batchv1 "k8s.io/api/batch/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/fields"
@@ -387,6 +388,12 @@ func getNewManagerCache(cdiNamespace string) cache.NewCacheFunc {
 				&routev1.Route{}: {
 					Field: namespaceSelector,
 				},
+				&batchv1.CronJob{}: {
+					Field: namespaceSelector,
+				},
+				&batchv1.Job{}: {
+					Field: namespaceSelector,
+				},
 			},
 		},
 	)

diff --git a/pkg/operator/controller/prometheus.go b/pkg/operator/controller/prometheus.go
@@ -36,11 +36,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
-	"kubevirt.io/containerized-data-importer/pkg/monitoring"
-
 	"kubevirt.io/containerized-data-importer/pkg/common"
 	cc "kubevirt.io/containerized-data-importer/pkg/controller/common"
+	"kubevirt.io/containerized-data-importer/pkg/monitoring"
+	cdinamespaced "kubevirt.io/containerized-data-importer/pkg/operator/resources/namespaced"
 	"kubevirt.io/containerized-data-importer/pkg/util"
+
 	sdk "kubevirt.io/controller-lifecycle-operator-sdk/pkg/sdk"
 )
 
@@ -256,21 +257,7 @@ func newPrometheusRole(namespace string) *rbacv1.Role {
 				common.PrometheusLabelKey: common.PrometheusLabelValue,
 			},
 		},
-		Rules: []rbacv1.PolicyRule{
-			{
-				APIGroups: []string{
-					"",
-				},
-				Resources: []string{
-					"services",
-					"endpoints",
-					"pods",
-				},
-				Verbs: []string{
-					"get", "list", "watch",
-				},
-			},
-		},
+		Rules: cdinamespaced.GetPrometheusNamespacedRules(),
 	}
 }
 

diff --git a/pkg/operator/resources/cluster/apiserver.go b/pkg/operator/resources/cluster/apiserver.go
@@ -136,7 +136,6 @@ func getAPIServerClusterPolicyRules() []rbacv1.PolicyRule {
 				"datasources",
 			},
 			Verbs: []string{
-				"list",
 				"get",
 			},
 		},
@@ -159,7 +158,7 @@ func getAPIServerClusterPolicyRules() []rbacv1.PolicyRule {
 				"cdis/finalizers",
 			},
 			Verbs: []string{
-				"*",
+				"update",
 			},
 		},
 	}

diff --git a/pkg/operator/resources/cluster/controller.go b/pkg/operator/resources/cluster/controller.go
@@ -58,7 +58,6 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
 				"",
 			},
 			Resources: []string{
-				"persistentvolumes",
 				"persistentvolumeclaims",
 			},
 			Verbs: []string{
@@ -72,6 +71,20 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
 				"patch",
 			},
 		},
+		{
+			APIGroups: []string{
+				"",
+			},
+			Resources: []string{
+				"persistentvolumes",
+			},
+			Verbs: []string{
+				"get",
+				"list",
+				"watch",
+				"update",
+			},
+		},
 		{
 			APIGroups: []string{
 				"",
@@ -155,10 +168,28 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
 				"snapshot.storage.k8s.io",
 			},
 			Resources: []string{
-				"*",
+				"volumesnapshots",
+				"volumesnapshotclasses",
+				"volumesnapshotcontents",
 			},
 			Verbs: []string{
-				"*",
+				"get",
+				"list",
+				"watch",
+				"create",
+				"delete",
+			},
+		},
+		{
+			APIGroups: []string{
+				"snapshot.storage.k8s.io",
+			},
+			Resources: []string{
+				"volumesnapshots",
+			},
+			Verbs: []string{
+				"update",
+				"deletecollection",
 			},
 		},
 		{
@@ -211,30 +242,6 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
 				"create",
 			},
 		},
-		{
-			APIGroups: []string{
-				"batch",
-			},
-			Resources: []string{
-				"cronjobs",
-			},
-			Verbs: []string{
-				"list",
-				"watch",
-			},
-		},
-		{
-			APIGroups: []string{
-				"batch",
-			},
-			Resources: []string{
-				"jobs",
-			},
-			Verbs: []string{
-				"list",
-				"watch",
-			},
-		},
 		{
 			APIGroups: []string{
 				"kubevirt.io",

diff --git a/pkg/operator/resources/namespaced/BUILD.bazel b/pkg/operator/resources/namespaced/BUILD.bazel
@@ -7,6 +7,7 @@ go_library(
         "controller.go",
         "cronjob.go",
         "factory.go",
+        "prometheus.go",
         "uploadproxy.go",
     ],
     importpath = "kubevirt.io/containerized-data-importer/pkg/operator/resources/namespaced",

diff --git a/pkg/operator/resources/namespaced/apiserver.go b/pkg/operator/resources/namespaced/apiserver.go
@@ -59,8 +59,8 @@ func createAPIServerRoleBinding() *rbacv1.RoleBinding {
 	return utils.ResourceBuilder.CreateRoleBinding(apiServerRessouceName, apiServerRessouceName, apiServerRessouceName, "")
 }
 
-func createAPIServerRole() *rbacv1.Role {
-	rules := []rbacv1.PolicyRule{
+func getAPIServerNamespacedRules() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
 		{
 			APIGroups: []string{
 				"",
@@ -70,11 +70,17 @@ func createAPIServerRole() *rbacv1.Role {
 				"configmaps",
 			},
 			Verbs: []string{
-				"*",
+				"get",
+				"list",
+				"watch",
+				"create",
 			},
 		},
 	}
-	return utils.ResourceBuilder.CreateRole(apiServerRessouceName, rules)
+}
+
+func createAPIServerRole() *rbacv1.Role {
+	return utils.ResourceBuilder.CreateRole(apiServerRessouceName, getAPIServerNamespacedRules())
 }
 
 func createAPIServerService() *corev1.Service {

diff --git a/pkg/operator/resources/namespaced/controller.go b/pkg/operator/resources/namespaced/controller.go
@@ -60,8 +60,8 @@ func createControllerRoleBinding() *rbacv1.RoleBinding {
 	return utils.ResourceBuilder.CreateRoleBinding(controllerResourceName, controllerResourceName, common.ControllerServiceAccountName, "")
 }
 
-func createControllerRole() *rbacv1.Role {
-	rules := []rbacv1.PolicyRule{
+func getControllerNamespacedRules() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
 		{
 			APIGroups: []string{
 				"",
@@ -70,7 +70,12 @@ func createControllerRole() *rbacv1.Role {
 				"configmaps",
 			},
 			Verbs: []string{
-				"*",
+				"get",
+				"list",
+				"watch",
+				"create",
+				"update",
+				"delete",
 			},
 		},
 		{
@@ -113,6 +118,8 @@ func createControllerRole() *rbacv1.Role {
 			Verbs: []string{
 				"create",
 				"delete",
+				"list",
+				"watch",
 			},
 		},
 		{
@@ -123,7 +130,9 @@ func createControllerRole() *rbacv1.Role {
 				"leases",
 			},
 			Verbs: []string{
-				"*",
+				"get",
+				"create",
+				"update",
 			},
 		},
 		{
@@ -153,7 +162,10 @@ func createControllerRole() *rbacv1.Role {
 			},
 		},
 	}
-	return utils.ResourceBuilder.CreateRole(controllerResourceName, rules)
+}
+
+func createControllerRole() *rbacv1.Role {
+	return utils.ResourceBuilder.CreateRole(controllerResourceName, getControllerNamespacedRules())
 }
 
 func createControllerServiceAccount() *corev1.ServiceAccount {

diff --git a/pkg/operator/resources/namespaced/factory.go b/pkg/operator/resources/namespaced/factory.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -97,3 +98,12 @@ func assignNamspaceIfMissing(resource client.Object, namespace string) {
 		obj.SetNamespace(namespace)
 	}
 }
+
+// GetRolePolicyRules returns all namespaced PolicyRules
+func GetRolePolicyRules() []rbacv1.PolicyRule {
+	result := getAPIServerNamespacedRules()
+	result = append(result, getControllerNamespacedRules()...)
+	result = append(result, getUploadProxyNamespacedRules()...)
+	result = append(result, GetPrometheusNamespacedRules()...)
+	return result
+}
diff --git a/pkg/operator/resources/namespaced/prometheus.go b/pkg/operator/resources/namespaced/prometheus.go
@@ -0,0 +1,40 @@
+/*
+Copyright 2018 The CDI Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package namespaced
+
+import (
+	rbacv1 "k8s.io/api/rbac/v1"
+)
+
+// GetPrometheusNamespacedRules returns the policy rules needed for CDI alerting setup
+func GetPrometheusNamespacedRules() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
+		{
+			APIGroups: []string{
+				"",
+			},
+			Resources: []string{
+				"services",
+				"endpoints",
+				"pods",
+			},
+			Verbs: []string{
+				"get", "list", "watch",
+			},
+		},
+	}
+}
diff --git a/pkg/operator/resources/namespaced/uploadproxy.go b/pkg/operator/resources/namespaced/uploadproxy.go
@@ -67,8 +67,8 @@ func createUploadProxyRoleBinding() *rbacv1.RoleBinding {
 	return utils.ResourceBuilder.CreateRoleBinding(uploadProxyResourceName, uploadProxyResourceName, uploadProxyResourceName, "")
 }
 
-func createUploadProxyRole() *rbacv1.Role {
-	rules := []rbacv1.PolicyRule{
+func getUploadProxyNamespacedRules() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
 		{
 			APIGroups: []string{
 				"",
@@ -81,7 +81,10 @@ func createUploadProxyRole() *rbacv1.Role {
 			},
 		},
 	}
-	return utils.ResourceBuilder.CreateRole(uploadProxyResourceName, rules)
+}
+
+func createUploadProxyRole() *rbacv1.Role {
+	return utils.ResourceBuilder.CreateRole(uploadProxyResourceName, getUploadProxyNamespacedRules())
 }
 
 func createUploadProxyDeployment(image, verbosity, pullPolicy string, imagePullSecrets []corev1.LocalObjectReference, priorityClassName string, infraNodePlacement *sdkapi.NodePlacement) *appsv1.Deployment {