fix: RBAC config when OwnerReferencesPermissionEnforcement admission …

…controller is enabled (#517) Resolves: #514 Signed-off-by: Daniel Pacak <[email protected]>
aquasecurity · Apr 21, 2021 · d1cbe3a · d1cbe3a
1 parent 977996e
commit d1cbe3a
Show file tree

Hide file tree

Showing 14 changed files with 206 additions and 61 deletions.
diff --git a/deploy/helm/templates/rbac.yaml b/deploy/helm/templates/rbac.yaml
@@ -57,6 +57,12 @@ rules:
       - get
       - create
       - update
+  - apiGroups:
+      - ""
+    resources:
+      - secrets
+    verbs:
+      - delete
   - apiGroups:
       - ""
     resources:

diff --git a/deploy/static/03-starboard-operator.clusterrole.yaml b/deploy/static/03-starboard-operator.clusterrole.yaml
@@ -35,6 +35,12 @@ rules:
       - get
       - create
       - update
+  - apiGroups:
+      - ""
+    resources:
+      - secrets
+    verbs:
+      - delete
   - apiGroups:
       - ""
     resources:

diff --git a/itest/matcher/matcher.go b/itest/matcher/matcher.go
@@ -76,7 +76,7 @@ func (m *vulnerabilityReportMatcher) Match(actual interface{}) (bool, error) {
 				Name:               m.owner.GetName(),
 				UID:                m.owner.GetUID(),
 				Controller:         pointer.BoolPtr(true),
-				BlockOwnerDeletion: pointer.BoolPtr(true),
+				BlockOwnerDeletion: pointer.BoolPtr(false),
 			}),
 		}),
 		"Report": MatchFields(IgnoreExtras, Fields{
@@ -150,7 +150,7 @@ func (m *configAuditReportMatcher) Match(actual interface{}) (bool, error) {
 				Name:               m.owner.GetName(),
 				UID:                m.owner.GetUID(),
 				Controller:         pointer.BoolPtr(true),
-				BlockOwnerDeletion: pointer.BoolPtr(true),
+				BlockOwnerDeletion: pointer.BoolPtr(false),
 			}),
 		}),
 		"Report": MatchFields(IgnoreExtras, Fields{

diff --git a/itest/matcher/matcher_test.go b/itest/matcher/matcher_test.go
@@ -49,7 +49,7 @@ func TestVulnerabilityReportMatcher(t *testing.T) {
 						Name:               "nginx-pod",
 						UID:                "56d53a84-c81b-4620-81a1-e226c35d3983",
 						Controller:         pointer.BoolPtr(true),
-						BlockOwnerDeletion: pointer.BoolPtr(true),
+						BlockOwnerDeletion: pointer.BoolPtr(false),
 					},
 				},
 			},
@@ -101,7 +101,7 @@ func TestConfigAuditReportMatcher(t *testing.T) {
 						Name:               "nginx-6d4cf56db6",
 						UID:                "494b2727-5d52-4057-9a9b-8b508c753fea",
 						Controller:         pointer.BoolPtr(true),
-						BlockOwnerDeletion: pointer.BoolPtr(true),
+						BlockOwnerDeletion: pointer.BoolPtr(false),
 					},
 				},
 			},

diff --git a/itest/starboard/starboard_cli_test.go b/itest/starboard/starboard_cli_test.go
@@ -1044,7 +1044,7 @@ var _ = Describe("Starboard CLI", func() {
 							Name:               node.Name,
 							UID:                node.UID,
 							Controller:         pointer.BoolPtr(true),
-							BlockOwnerDeletion: pointer.BoolPtr(true),
+							BlockOwnerDeletion: pointer.BoolPtr(false),
 						}),
 					}),
 					"Report": MatchFields(IgnoreExtras, Fields{

diff --git a/pkg/configauditreport/builder.go b/pkg/configauditreport/builder.go
@@ -9,6 +9,7 @@ import (
 	"github.com/aquasecurity/starboard/pkg/starboard"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
@@ -27,11 +28,11 @@ func NewBuilder(scheme *runtime.Scheme) Builder {
 }
 
 type builder struct {
-	scheme     *runtime.Scheme
-	controller metav1.Object
-	hash       string
-	configHash string
-	result     v1alpha1.ConfigAuditResult
+	scheme           *runtime.Scheme
+	controller       metav1.Object
+	podSpecHash      string
+	pluginConfigHash string
+	result           v1alpha1.ConfigAuditResult
 }
 
 func (b *builder) Controller(controller metav1.Object) Builder {
@@ -40,12 +41,12 @@ func (b *builder) Controller(controller metav1.Object) Builder {
 }
 
 func (b *builder) PodSpecHash(hash string) Builder {
-	b.hash = hash
+	b.podSpecHash = hash
 	return b
 }
 
 func (b *builder) PluginConfigHash(hash string) Builder {
-	b.configHash = hash
+	b.pluginConfigHash = hash
 	return b
 }
 
@@ -66,7 +67,7 @@ func (b *builder) reportName() (string, error) {
 func (b *builder) Get() (v1alpha1.ConfigAuditReport, error) {
 	kind, err := kube.KindForObject(b.controller, b.scheme)
 	if err != nil {
-		return v1alpha1.ConfigAuditReport{}, err
+		return v1alpha1.ConfigAuditReport{}, fmt.Errorf("getting kind for object: %w", err)
 	}
 
 	labels := map[string]string{
@@ -75,12 +76,12 @@ func (b *builder) Get() (v1alpha1.ConfigAuditReport, error) {
 		starboard.LabelResourceNamespace: b.controller.GetNamespace(),
 	}
 
-	if b.hash != "" {
-		labels[starboard.LabelPodSpecHash] = b.hash
+	if b.podSpecHash != "" {
+		labels[starboard.LabelPodSpecHash] = b.podSpecHash
 	}
 
-	if b.configHash != "" {
-		labels[starboard.LabelPluginConfigHash] = b.configHash
+	if b.pluginConfigHash != "" {
+		labels[starboard.LabelPluginConfigHash] = b.pluginConfigHash
 	}
 
 	reportName, err := b.reportName()
@@ -98,7 +99,16 @@ func (b *builder) Get() (v1alpha1.ConfigAuditReport, error) {
 	}
 	err = controllerutil.SetControllerReference(b.controller, &report, b.scheme)
 	if err != nil {
-		return v1alpha1.ConfigAuditReport{}, err
+		return v1alpha1.ConfigAuditReport{}, fmt.Errorf("setting controller reference: %w", err)
 	}
+	// The OwnerReferencesPermissionsEnforcement admission controller protects the
+	// access to metadata.ownerReferences[x].blockOwnerDeletion of an object, so
+	// that only users with "update" permission to the finalizers subresource of the
+	// referenced owner can change it.
+	// We set metadata.ownerReferences[x].blockOwnerDeletion to false so that
+	// additional RBAC permissions are not required when the OwnerReferencesPermissionsEnforcement
+	// is enabled.
+	// See https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement
+	report.OwnerReferences[0].BlockOwnerDeletion = pointer.BoolPtr(false)
 	return report, nil
 }
diff --git a/pkg/configauditreport/builder_test.go b/pkg/configauditreport/builder_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1"
 	"github.com/aquasecurity/starboard/pkg/configauditreport"
+	"github.com/aquasecurity/starboard/pkg/starboard"
 	"github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -37,15 +38,15 @@ func TestBuilder(t *testing.T) {
 					Kind:               "ReplicaSet",
 					Name:               "some-owner",
 					Controller:         pointer.BoolPtr(true),
-					BlockOwnerDeletion: pointer.BoolPtr(true),
+					BlockOwnerDeletion: pointer.BoolPtr(false),
 				},
 			},
 			Labels: map[string]string{
-				"starboard.resource.kind":      "ReplicaSet",
-				"starboard.resource.name":      "some-owner",
-				"starboard.resource.namespace": "qa",
-				"pod-spec-hash":                "xyz",
-				"plugin-config-hash":           "nop",
+				starboard.LabelResourceKind:      "ReplicaSet",
+				starboard.LabelResourceName:      "some-owner",
+				starboard.LabelResourceNamespace: "qa",
+				starboard.LabelPodSpecHash:       "xyz",
+				starboard.LabelPluginConfigHash:  "nop",
 			},
 		},
 		Report: v1alpha1.ConfigAuditResult{},

diff --git a/pkg/kubebench/builder.go b/pkg/kubebench/builder.go
@@ -0,0 +1,82 @@
+package kubebench
+
+import (
+	"fmt"
+
+	"github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1"
+	"github.com/aquasecurity/starboard/pkg/kube"
+	"github.com/aquasecurity/starboard/pkg/starboard"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+type Builder interface {
+	Controller(controller metav1.Object) Builder
+	Data(data v1alpha1.CISKubeBenchOutput) Builder
+	Get() (v1alpha1.CISKubeBenchReport, error)
+}
+
+func NewBuilder(scheme *runtime.Scheme) Builder {
+	return &builder{
+		scheme: scheme,
+	}
+}
+
+type builder struct {
+	scheme     *runtime.Scheme
+	controller metav1.Object
+	data       v1alpha1.CISKubeBenchOutput
+}
+
+func (b *builder) Controller(controller metav1.Object) Builder {
+	b.controller = controller
+	return b
+}
+
+func (b *builder) Data(data v1alpha1.CISKubeBenchOutput) Builder {
+	b.data = data
+	return b
+}
+
+func (b *builder) reportName() string {
+	return b.controller.GetName()
+}
+
+func (b *builder) Get() (v1alpha1.CISKubeBenchReport, error) {
+	kind, err := kube.KindForObject(b.controller, b.scheme)
+	if err != nil {
+		return v1alpha1.CISKubeBenchReport{}, fmt.Errorf("getting kind for object: %w", err)
+	}
+
+	labels := map[string]string{
+		starboard.LabelResourceKind: kind,
+		starboard.LabelResourceName: b.controller.GetName(),
+	}
+
+	reportName := b.reportName()
+
+	report := v1alpha1.CISKubeBenchReport{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      reportName,
+			Namespace: b.controller.GetNamespace(),
+			Labels:    labels,
+		},
+		Report: b.data,
+	}
+	err = controllerutil.SetControllerReference(b.controller, &report, b.scheme)
+	if err != nil {
+		return v1alpha1.CISKubeBenchReport{}, fmt.Errorf("setting controller reference: %w", err)
+	}
+	// The OwnerReferencesPermissionsEnforcement admission controller protects the
+	// access to metadata.ownerReferences[x].blockOwnerDeletion of an object, so
+	// that only users with "update" permission to the finalizers subresource of the
+	// referenced owner can change it.
+	// We set metadata.ownerReferences[x].blockOwnerDeletion to false so that
+	// additional RBAC permissions are not required when the OwnerReferencesPermissionsEnforcement
+	// is enabled.
+	// See https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement
+	report.OwnerReferences[0].BlockOwnerDeletion = pointer.BoolPtr(false)
+	return report, nil
+}
diff --git a/pkg/kubebench/builder_test.go b/pkg/kubebench/builder_test.go
@@ -0,0 +1,47 @@
+package kubebench_test
+
+import (
+	"testing"
+
+	"github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1"
+	"github.com/aquasecurity/starboard/pkg/kubebench"
+	"github.com/aquasecurity/starboard/pkg/starboard"
+	"github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/utils/pointer"
+)
+
+func TestBuilder(t *testing.T) {
+	g := gomega.NewGomegaWithT(t)
+
+	report, err := kubebench.NewBuilder(scheme.Scheme).
+		Controller(&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "control-plane",
+			},
+		}).
+		Data(v1alpha1.CISKubeBenchOutput{}).Get()
+
+	g.Expect(err).ToNot(gomega.HaveOccurred())
+	g.Expect(report).To(gomega.Equal(v1alpha1.CISKubeBenchReport{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "control-plane",
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         "v1",
+					Kind:               "Node",
+					Name:               "control-plane",
+					Controller:         pointer.BoolPtr(true),
+					BlockOwnerDeletion: pointer.BoolPtr(false),
+				},
+			},
+			Labels: map[string]string{
+				starboard.LabelResourceKind: "Node",
+				starboard.LabelResourceName: "control-plane",
+			},
+		},
+		Report: v1alpha1.CISKubeBenchOutput{},
+	}))
+}
diff --git a/pkg/kubebench/plugin_test.go b/pkg/kubebench/plugin_test.go
@@ -0,0 +1 @@
+package kubebench_test
diff --git a/pkg/kubebench/scanner.go b/pkg/kubebench/scanner.go
@@ -21,7 +21,6 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog"
 	"k8s.io/utils/pointer"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 type Scanner struct {
@@ -91,20 +90,12 @@ func (s *Scanner) Scan(ctx context.Context, node corev1.Node) (v1alpha1.CISKubeB
 		return v1alpha1.CISKubeBenchReport{}, err
 	}
 
-	report := v1alpha1.CISKubeBenchReport{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: node.Name,
-			Labels: map[string]string{
-				starboard.LabelResourceKind: string(kube.KindNode),
-				starboard.LabelResourceName: node.Name,
-			},
-		},
-		Report: output,
-	}
-
-	err = controllerutil.SetControllerReference(&node, &report, s.scheme)
+	report, err := NewBuilder(s.scheme).
+		Controller(&node).
+		Data(output).
+		Get()
 	if err != nil {
-		return v1alpha1.CISKubeBenchReport{}, err
+		return v1alpha1.CISKubeBenchReport{}, fmt.Errorf("building report: %w", err)
 	}
 
 	return report, nil

diff --git a/pkg/operator/controller/ciskubebenchreport.go b/pkg/operator/controller/ciskubebenchreport.go
@@ -22,7 +22,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
@@ -271,21 +270,12 @@ func (r *CISKubeBenchReportReconciler) processCompleteScanJob(ctx context.Contex
 		_ = logsStream.Close()
 	}()
 
-	// TODO We have a similar code in CLI
-	report := v1alpha1.CISKubeBenchReport{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: node.Name,
-			Labels: map[string]string{
-				starboard.LabelResourceKind: string(kube.KindNode),
-				starboard.LabelResourceName: node.Name,
-			},
-		},
-		Report: output,
-	}
-
-	err = controllerutil.SetControllerReference(node, &report, r.Client.Scheme())
+	report, err := kubebench.NewBuilder(r.Client.Scheme()).
+		Controller(node).
+		Data(output).
+		Get()
 	if err != nil {
-		return fmt.Errorf("setting controller reference: %w", err)
+		return fmt.Errorf("building report: %w", err)
 	}
 
 	log.V(1).Info("Writing CIS Kubernetes Benchmark report", "reportName", report.Name)