Security mitigation: remove secret get from RBAC (#160)

* Security mitigation: remove secret get from RBAC

* Security migtigation: update the description for the custom image and extraFile secrets in the CRD

* Security compliance: remove create and update from RBAC for PV and PVC

Author: jianrongzhang89

.rhdh/bundle/manifests/rhdh-operator.csv.yaml

@@ -25,7 +25,7 @@ metadata:
       It comes with pre-built plug-ins, configuration settings, and deployment mechanisms,
       which can help streamline the process of setting up a self-managed internal
       developer portal for adopters who are just starting out.
-    operatorframework.io/suggested-namespace: openshift-operators
+    operatorframework.io/suggested-namespace: openshift-rhdh-operator
     operators.openshift.io/valid-subscription: '["OpenShift Container Platform", "OpenShift
       Platform Plus"]'
     operators.operatorframework.io/builder: operator-sdk-v1.33.0

Makefile

@@ -359,7 +359,7 @@ undeploy-olm: ## Un-deploy the operator with OLM
 DEFAULT_OLM_NAMESPACE ?= openshift-marketplace
 .PHONY: catalog-update
 catalog-update: ## Update catalog source in the default namespace for catalogsource
-	kubectl delete catalogsource backstage-operator -n $(DEFAULT_OLM_NAMESPACE)
+	-kubectl delete catalogsource backstage-operator -n $(DEFAULT_OLM_NAMESPACE)
 	sed "s/{{CATALOG_IMG}}/$(subst /,\/,$(CATALOG_IMG))/g" config/samples/catalog-source-template.yaml | kubectl apply -n $(DEFAULT_OLM_NAMESPACE) -f -
 
 .PHONY: deploy-openshift

api/v1alpha1/backstage_types.go

@@ -97,7 +97,8 @@ type Application struct {
 	//+kubebuilder:default=1
 	Replicas *int32 `json:"replicas,omitempty"`
 
-	// Image to use in all containers (including Init Containers)
+	// Custom image to use in all containers (including Init Containers).
+	// It is your responsibility to make sure the image is from trusted sources and has been validated for security compliance
 	// +optional
 	Image *string `json:"image,omitempty"`
 
@@ -138,8 +139,7 @@ type ExtraFiles struct {
 	ConfigMaps []ObjectKeyRef `json:"configMaps,omitempty"`
 
 	// List of references to Secrets objects mounted as extra files under the MountPath specified.
-	// For each item in this array, if a key is not specified, it means that all keys in the Secret will be mounted as files.
-	// Otherwise, only the specified key will be mounted as a file.
+	// For each item in this array, a key must be specified that will be mounted as a file.
 	// +optional
 	Secrets []ObjectKeyRef `json:"secrets,omitempty"`
 }

bundle/manifests/backstage-operator.clusterserviceversion.yaml

@@ -20,8 +20,9 @@ metadata:
           "spec": null
         }
       ]
-    capabilities: Basic Install
-    createdAt: "2024-01-19T01:12:25Z"
+    capabilities: Seamless Upgrades
+    createdAt: "2024-01-29T20:18:14Z"
+    operatorframework.io/suggested-namespace: backstage-system
     operators.operatorframework.io/builder: operator-sdk-v1.33.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
   name: backstage-operator.v0.0.1
@@ -48,9 +49,6 @@ spec:
           - ""
           resources:
           - configmaps
-          - persistentvolumeclaims
-          - persistentvolumes
-          - secrets
           - services
           verbs:
           - create
@@ -59,6 +57,22 @@ spec:
           - list
           - update
           - watch
+        - apiGroups:
+          - ""
+          resources:
+          - persistentvolumeclaims
+          - persistentvolumes
+          verbs:
+          - get
+          - list
+          - watch
+        - apiGroups:
+          - ""
+          resources:
+          - secrets
+          verbs:
+          - create
+          - delete
         - apiGroups:
           - apps
           resources:

config/rbac/backstage_viewer_role.yaml renamed to bundle/manifests/backstage-secret-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml

@@ -1,27 +1,21 @@
-# permissions for end users to view backstages.
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
+  creationTimestamp: null
   labels:
-    app.kubernetes.io/name: clusterrole
-    app.kubernetes.io/instance: backstage-viewer-role
     app.kubernetes.io/component: rbac
     app.kubernetes.io/created-by: backstage-operator
-    app.kubernetes.io/part-of: backstage-operator
+    app.kubernetes.io/instance: backstage-secret-viewer-role
     app.kubernetes.io/managed-by: kustomize
-  name: backstage-viewer-role
+    app.kubernetes.io/name: clusterrole
+    app.kubernetes.io/part-of: backstage-operator
+  name: backstage-secret-viewer-role
 rules:
 - apiGroups:
-  - janus-idp.io
+  - ""
   resources:
-  - backstages
+  - secrets
   verbs:
   - get
   - list
   - watch
-- apiGroups:
-  - janus-idp.io
-  resources:
-  - backstages/status
-  verbs:
-  - get

bundle/manifests/janus-idp.io_backstages.yaml

@@ -179,9 +179,8 @@ spec:
                       secrets:
                         description: List of references to Secrets objects mounted
                           as extra files under the MountPath specified. For each item
-                          in this array, if a key is not specified, it means that
-                          all keys in the Secret will be mounted as files. Otherwise,
-                          only the specified key will be mounted as a file.
+                          in this array, a key must be specified that will be mounted
+                          as a file.
                         items:
                           properties:
                             key:
@@ -197,7 +196,10 @@ spec:
                         type: array
                     type: object
                   image:
-                    description: Image to use in all containers (including Init Containers)
+                    description: Custom image to use in all containers (including
+                      Init Containers). It is your responsibility to make sure the
+                      image is from trusted sources and has been validated for security
+                      compliance
                     type: string
                   imagePullSecrets:
                     description: Image Pull Secrets to use in all containers (including

config/crd/bases/janus-idp.io_backstages.yaml

@@ -180,9 +180,8 @@ spec:
                       secrets:
                         description: List of references to Secrets objects mounted
                           as extra files under the MountPath specified. For each item
-                          in this array, if a key is not specified, it means that
-                          all keys in the Secret will be mounted as files. Otherwise,
-                          only the specified key will be mounted as a file.
+                          in this array, a key must be specified that will be mounted
+                          as a file.
                         items:
                           properties:
                             key:
@@ -198,7 +197,10 @@ spec:
                         type: array
                     type: object
                   image:
-                    description: Image to use in all containers (including Init Containers)
+                    description: Custom image to use in all containers (including
+                      Init Containers). It is your responsibility to make sure the
+                      image is from trusted sources and has been validated for security
+                      compliance
                     type: string
                   imagePullSecrets:
                     description: Image Pull Secrets to use in all containers (including

config/manifests/bases/backstage-operator.clusterserviceversion.yaml

@@ -3,7 +3,8 @@ kind: ClusterServiceVersion
 metadata:
   annotations:
     alm-examples: '[]'
-    capabilities: Basic Install
+    capabilities: Seamless Upgrades
+    operatorframework.io/suggested-namespace: backstage-system
   name: backstage-operator.v0.0.0
   namespace: placeholder
 spec:

config/rbac/role.yaml

@@ -9,9 +9,6 @@ rules:
   - ""
   resources:
   - configmaps
-  - persistentvolumeclaims
-  - persistentvolumes
-  - secrets
   - services
   verbs:
   - create
@@ -20,6 +17,22 @@ rules:
   - list
   - update
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - persistentvolumeclaims
+  - persistentvolumes
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - ""
+  resources:
+  - secrets
+  verbs:
+  - create
+  - delete
 - apiGroups:
   - apps
   resources:

controllers/backstage_controller.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/client-go/kubernetes"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -44,6 +45,9 @@ const (
 // BackstageReconciler reconciles a Backstage object
 type BackstageReconciler struct {
 	client.Client
+
+	Clientset *kubernetes.Clientset
+
 	Scheme *runtime.Scheme
 	// If true, Backstage Controller always sync the state of runtime objects created
 	// otherwise, runtime objects can be re-configured independently
@@ -64,10 +68,12 @@ type BackstageReconciler struct {
 //+kubebuilder:rbac:groups=janus-idp.io,resources=backstages,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=janus-idp.io,resources=backstages/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=janus-idp.io,resources=backstages/finalizers,verbs=update
-//+kubebuilder:rbac:groups="",resources=configmaps;secrets;persistentvolumes;persistentvolumeclaims;services,verbs=get;watch;create;update;list;delete
-//+kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;watch;create;update;list;delete
-//+kubebuilder:rbac:groups="apps",resources=statefulsets,verbs=get;watch;create;update;list;delete
-//+kubebuilder:rbac:groups="route.openshift.io",resources=routes;routes/custom-host,verbs=get;watch;create;update;list;delete
+//+kubebuilder:rbac:groups="",resources=configmaps;services,verbs=get;list;watch;create;update;delete
+//+kubebuilder:rbac:groups="",resources=persistentvolumes;persistentvolumeclaims,verbs=get;list;watch
+//+kubebuilder:rbac:groups="",resources=secrets,verbs=create;delete
+//+kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;list;watch;create;update;delete
+//+kubebuilder:rbac:groups="apps",resources=statefulsets,verbs=get;list;watch;create;update;delete
+//+kubebuilder:rbac:groups="route.openshift.io",resources=routes;routes/custom-host,verbs=get;list;watch;create;update;delete
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
@@ -293,6 +299,13 @@ func (r *BackstageReconciler) labels(meta *v1.ObjectMeta, backstage bs.Backstage
 // SetupWithManager sets up the controller with the Manager.
 func (r *BackstageReconciler) SetupWithManager(mgr ctrl.Manager, log logr.Logger) error {
 
+	clientset, err := kubernetes.NewForConfig(mgr.GetConfig())
+	if err != nil {
+		log.Error(err, "unable to create clientset")
+		return err
+	}
+	r.Clientset = clientset
+
 	if len(r.PsqlImage) == 0 {
 		r.PsqlImage = "quay.io/fedora/postgresql-15:latest"
 		log.Info("Enviroment variable is not set, default is used", bs.EnvPostGresImage, r.PsqlImage)

controllers/backstage_controller_test.go

@@ -62,6 +62,7 @@ var _ = Describe("Backstage controller", func() {
 
 		backstageReconciler = &BackstageReconciler{
 			Client:         k8sClient,
+			Clientset:      k8sClientset,
 			Scheme:         k8sClient.Scheme(),
 			Namespace:      ns,
 			OwnsRuntime:    true,
@@ -852,9 +853,19 @@ plugins: []
 	Context("Extra Files", func() {
 		for _, kind := range []string{"ConfigMap", "Secret"} {
 			kind := kind
-			When(fmt.Sprintf("referencing non-existing %s as extra-file", kind), func() {
+			name := "a-" + strings.ToLower(kind)
+			title := ""
+			errExpected := ""
+			switch kind {
+			case "ConfigMap":
+				title = fmt.Sprintf("referencing non-existing %s as extra-file without key", kind)
+				errExpected = fmt.Sprintf("configmaps \"%s\" not found", name)
+			case "Secret":
+				title = fmt.Sprintf("referencing %s as extra-file without key", kind)
+				errExpected = fmt.Sprintf("key is required to mount extra file with secret %s", name)
+			}
+			When(title, func() {
 				var backstage *bsv1alpha1.Backstage
-				name := "a-non-existing-" + strings.ToLower(kind)
 
 				BeforeEach(func() {
 					var (
@@ -891,9 +902,8 @@ plugins: []
 						NamespacedName: types.NamespacedName{Name: backstageName, Namespace: ns},
 					})
 					Expect(err).To(HaveOccurred())
-					errStr := fmt.Sprintf("failed to add volume mounts to Backstage deployment, reason: %ss \"%s\" not found", strings.ToLower(kind), name)
-					Expect(err.Error()).Should(ContainSubstring(errStr))
-					verifyBackstageInstanceError(ctx, errStr)
+					Expect(err.Error()).Should(ContainSubstring(errExpected))
+					verifyBackstageInstanceError(ctx, errExpected)
 
 					By("Not creating a Backstage Deployment")
 					Consistently(func() error {
@@ -903,13 +913,11 @@ plugins: []
 				})
 			})
 		}
-
 		for _, mountPath := range []string{"", "/some/path/for/extra/config"} {
 			mountPath := mountPath
 			When("referencing ConfigMaps and Secrets for extra files - mountPath="+mountPath, func() {
 				const (
 					extraConfig1CmNameAll        = "my-extra-config-1-cm-all"
-					extraConfig2SecretNameAll    = "my-extra-config-2-secret-all"
 					extraConfig1CmNameSingle     = "my-extra-config-1-cm-single"
 					extraConfig2SecretNameSingle = "my-extra-config-2-secret-single"
 				)
@@ -928,17 +936,6 @@ plugins: []
 					err := k8sClient.Create(ctx, extraConfig1CmAll)
 					Expect(err).To(Not(HaveOccurred()))
 
-					extraConfig2SecretAll := buildSecret(extraConfig2SecretNameAll, map[string][]byte{
-						"my-extra-config-21.yaml": []byte(`
-# my-extra-config-21.yaml
-`),
-						"my-extra-config-22.yaml": []byte(`
-# my-extra-config-22.yaml
-`),
-					})
-					err = k8sClient.Create(ctx, extraConfig2SecretAll)
-					Expect(err).To(Not(HaveOccurred()))
-
 					extraConfig1CmSingle := buildConfigMap(extraConfig1CmNameSingle, map[string]string{
 						"my-extra-file-11-single.yaml": `
 # my-extra-file-11-single.yaml
@@ -970,7 +967,6 @@ plugins: []
 									{Name: extraConfig1CmNameSingle, Key: "my-extra-file-12-single.yaml"},
 								},
 								Secrets: []bsv1alpha1.ObjectKeyRef{
-									{Name: extraConfig2SecretNameAll},
 									{Name: extraConfig2SecretNameSingle, Key: "my-extra-file-22-single.yaml"},
 								},
 							},
@@ -1012,7 +1008,7 @@ plugins: []
 					})
 
 					By("Checking the Volumes in the Backstage Deployment", func() {
-						Expect(found.Spec.Template.Spec.Volumes).To(HaveLen(8))
+						Expect(found.Spec.Template.Spec.Volumes).To(HaveLen(7))
 
 						backendAuthAppConfigVol, ok := findVolume(found.Spec.Template.Spec.Volumes, backendAuthConfigName)
 						Expect(ok).To(BeTrue(), "No volume found with name: %s", backendAuthConfigName)
@@ -1026,12 +1022,6 @@ plugins: []
 						Expect(extraConfig1CmVol.VolumeSource.ConfigMap.DefaultMode).To(HaveValue(Equal(int32(420))))
 						Expect(extraConfig1CmVol.VolumeSource.ConfigMap.LocalObjectReference.Name).To(Equal(extraConfig1CmNameAll))
 
-						extraConfig2SecretVol, ok := findVolume(found.Spec.Template.Spec.Volumes, extraConfig2SecretNameAll)
-						Expect(ok).To(BeTrue(), "No volume found with name: %s", extraConfig2SecretNameAll)
-						Expect(extraConfig2SecretVol.VolumeSource.ConfigMap).To(BeNil())
-						Expect(extraConfig2SecretVol.VolumeSource.Secret.DefaultMode).To(HaveValue(Equal(int32(420))))
-						Expect(extraConfig2SecretVol.VolumeSource.Secret.SecretName).To(Equal(extraConfig2SecretNameAll))
-
 						extraConfig1SingleCmVol, ok := findVolume(found.Spec.Template.Spec.Volumes, extraConfig1CmNameSingle)
 						Expect(ok).To(BeTrue(), "No volume found with name: %s", extraConfig1CmNameSingle)
 						Expect(extraConfig1SingleCmVol.VolumeSource.Secret).To(BeNil())
@@ -1051,13 +1041,12 @@ plugins: []
 
 						// Extra config mounted in the main container
 						Expect(findVolumeMounts(initCont.VolumeMounts, extraConfig1CmNameAll)).Should(HaveLen(0))
-						Expect(findVolumeMounts(initCont.VolumeMounts, extraConfig2SecretNameAll)).Should(HaveLen(0))
 					})
 
 					mainCont := found.Spec.Template.Spec.Containers[0]
 
 					By("Checking the main container Volume Mounts in the Backstage Deployment", func() {
-						Expect(mainCont.VolumeMounts).To(HaveLen(8))
+						Expect(mainCont.VolumeMounts).To(HaveLen(6))
 
 						expectedMountPath := mountPath
 						if expectedMountPath == "" {
@@ -1083,20 +1072,6 @@ plugins: []
 									Equal("my-extra-config-12.yaml")))
 						}
 
-						extraConfig2SecretMounts := findVolumeMounts(mainCont.VolumeMounts, extraConfig2SecretNameAll)
-						Expect(extraConfig2SecretMounts).To(HaveLen(2), "No volume mounts found with name: %s", extraConfig2SecretNameAll)
-						Expect(extraConfig2SecretMounts[0].MountPath).ToNot(Equal(extraConfig2SecretMounts[1].MountPath))
-						for i := 0; i <= 1; i++ {
-							Expect(extraConfig2SecretMounts[i].MountPath).To(
-								SatisfyAny(
-									Equal(expectedMountPath+"/my-extra-config-21.yaml"),
-									Equal(expectedMountPath+"/my-extra-config-22.yaml")))
-							Expect(extraConfig2SecretMounts[i].SubPath).To(
-								SatisfyAny(
-									Equal("my-extra-config-21.yaml"),
-									Equal("my-extra-config-22.yaml")))
-						}
-
 						extraConfig1CmSingleMounts := findVolumeMounts(mainCont.VolumeMounts, extraConfig1CmNameSingle)
 						Expect(extraConfig1CmSingleMounts).To(HaveLen(1), "No volume mounts found with name: %s", extraConfig1CmNameSingle)
 						Expect(extraConfig1CmSingleMounts[0].MountPath).To(Equal(expectedMountPath + "/my-extra-file-12-single.yaml"))