Roll back problematic pod labeling feature

Some time ago I introduced a feature for the SPO to label denials for
pods. While this seemed like a good idea at the time, it introduces too
much privileges for te SPOd (update and patch all pods).

While there hasn't been any security incidents reported for the SPOd,
the drawbacks of giving such privileges are more critical than the
features the labeling would have enabled. So, let's get rid of this
feature and revisit the problem another time.

Signed-off-by: Juan Antonio Osorio <juan.osoriorobles@eu.equinix.com>

api/spod/v1alpha1/spod_types.go

@@ -69,10 +69,6 @@ type SPODSpec struct {
 	// tells the operator whether or not to enable AppArmor support for this
 	// SPOD instance.
 	EnableAppArmor bool `json:"enableAppArmor,omitempty"`
-	// tells the operator whether or not to apply labels to pods that present
-	// security policy-related denials. Note that this will be done cluster-wide.
-	// Note that this currently requires the log enricher to be enabled.
-	LabelPodDenials bool `json:"labelPodDenials,omitempty"`
 	// If specified, the SPOD's tolerations.
 	// +optional
 	Tolerations []corev1.Toleration `json:"tolerations,omitempty"`

bundle/manifests/security-profiles-operator.x-k8s.io_securityprofilesoperatordaemons.yaml

@@ -82,12 +82,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator

bundle/manifests/spod_rbac.authorization.k8s.io_v1_clusterrole.yaml

@@ -43,8 +43,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

cmd/security-profiles-operator/main.go

@@ -66,7 +66,6 @@ const (
 	jsonFlag           string = "json"
 	selinuxFlag        string = "with-selinux"
 	apparmorFlag       string = "with-apparmor"
-	labelDenialsFlag   string = "label-denials"
 	webhookFlag        string = "webhook"
 	defaultWebhookPort int    = 9443
 )
@@ -188,13 +187,6 @@ func main() {
 			Name:    "log-enricher",
 			Aliases: []string{"l"},
 			Usage:   "run the audit's log enricher",
-			Flags: []cli.Flag{
-				&cli.BoolFlag{
-					Name:  labelDenialsFlag,
-					Usage: "whether to label problematic pods or not",
-					Value: false,
-				},
-			},
 			Action: func(ctx *cli.Context) error {
 				return runLogEnricher(ctx, info)
 			},
@@ -468,7 +460,7 @@ func runLogEnricher(clictx *cli.Context, info *version.Info) error {
 	const component = "log-enricher"
 	printInfo(component, info)
 
-	e, err := enricher.New(ctrl.Log.WithName(component), clictx.Bool(labelDenialsFlag))
+	e, err := enricher.New(ctrl.Log.WithName(component))
 	if err != nil {
 		return fmt.Errorf("building log enricher: %w", err)
 	}

deploy/base-crds/crds/securityprofilesoperatordaemon.yaml

@@ -80,12 +80,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator

deploy/base/role.yaml

@@ -287,8 +287,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

deploy/helm/crds/crds.yaml

@@ -599,12 +599,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator

deploy/helm/templates/static-resources.yaml

@@ -326,8 +326,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

deploy/namespace-operator.yaml

@@ -599,12 +599,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator
@@ -1410,8 +1404,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

deploy/openshift-dev.yaml

@@ -599,12 +599,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator
@@ -1410,8 +1404,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

deploy/operator.yaml

@@ -599,12 +599,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator
@@ -1410,8 +1404,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

deploy/webhook-operator.yaml

@@ -774,12 +774,6 @@ spec:
                   as bpf-recorder to retrieve the container ID for a process ID. This
                   can be helpful for nested environments, for example when using "kind".
                 type: string
-              labelPodDenials:
-                description: tells the operator whether or not to apply labels to
-                  pods that present security policy-related denials. Note that this
-                  will be done cluster-wide. Note that this currently requires the
-                  log enricher to be enabled.
-                type: boolean
               selinuxOptions:
                 description: Defines options specific to the SELinux functionality
                   of the SecurityProfilesOperator
@@ -1547,8 +1541,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - security-profiles-operator.x-k8s.io

internal/pkg/daemon/enricher/container.go

@@ -21,12 +21,10 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/go-logr/logr"
 	"github.com/jellydator/ttlcache/v3"
 	"golang.org/x/net/context"
 	"golang.org/x/sync/errgroup"
 	v1 "k8s.io/api/core/v1"
-	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 
 	"sigs.k8s.io/security-profiles-operator/internal/pkg/config"
@@ -40,16 +38,14 @@ const (
 	backoffDuration  = 500 * time.Millisecond
 	backoffFactor    = 1.5
 	backoffSteps     = 10
-
-	problematicContainerLabelKey = "spo.x-k8s.io/had-denials"
 )
 
 var errContainerIDEmpty = errors.New("container ID is empty")
 
 // NOTE(jaosorior): Should this actually be namespace-scoped?
 //
 // Cluster scoped
-// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;patch;update
+// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch
 
 func (e *Enricher) getContainerInfo(
 	nodeName, targetContainerID string,
@@ -156,12 +152,6 @@ func (e *Enricher) populateCacheEntryForContainer(
 				RecordProfile: recordProfile,
 			}
 
-			if e.labelDenials && rawContainerID == targetContainerID {
-				e.logger.Info("labeling problematic container", "containerID", containerID,
-					"podNamespace", pod.Namespace, "podName", pod.Name)
-				e.labelPodDenials(context.TODO(), info.ContainerName, pod.DeepCopy(), e.logger)
-			}
-
 			// Update the cache
 			e.infoCache.Set(rawContainerID, info, ttlcache.DefaultTTL)
 		}
@@ -187,36 +177,3 @@ func (e *Enricher) handleContainerIDEmpty(podName, containerName string, contain
 		containerStatus.State,
 	)
 }
-
-func (e *Enricher) labelPodDenials(
-	ctx context.Context, containerName string, pod *v1.Pod, l logr.Logger,
-) {
-	// verify if we need to label or if the label is already there
-	if labels := pod.GetLabels(); labels != nil {
-		if _, ok := labels[problematicContainerLabelKey]; ok {
-			return
-		}
-	}
-
-	containerRetryBackoff := wait.Backoff{
-		Duration: backoffDuration,
-		Factor:   backoffFactor,
-		Steps:    backoffSteps,
-	}
-
-	if err := util.RetryEx(
-		&containerRetryBackoff,
-		func() (retryErr error) {
-			return e.impl.LabelPodDenials(ctx, e.clientset, pod)
-		},
-		func(inErr error) bool {
-			return !kerrors.IsNotFound(inErr)
-		},
-	); err != nil {
-		l.Error(
-			err, "unable to patch container to mark it as problematic",
-			"pod.Namespace", pod.GetNamespace(), "pod.Name", pod.GetName(),
-			"containerName", containerName,
-		)
-	}
-}

internal/pkg/daemon/enricher/enricher.go

@@ -64,11 +64,10 @@ type Enricher struct {
 	avcs             sync.Map
 	auditLineCache   *ttlcache.Cache[string, []*types.AuditLine]
 	clientset        kubernetes.Interface
-	labelDenials     bool
 }
 
 // New returns a new Enricher instance.
-func New(logger logr.Logger, labelDenials bool, impls ...impl) (*Enricher, error) {
+func New(logger logr.Logger, impls ...impl) (*Enricher, error) {
 	var effectiveimpl impl
 	if len(impls) == 0 {
 		effectiveimpl = &defaultImpl{}
@@ -106,18 +105,13 @@ func New(logger logr.Logger, labelDenials bool, impls ...impl) (*Enricher, error
 			// if/when the cache is full.
 			ttlcache.WithDisableTouchOnHit[string, []*types.AuditLine](),
 		),
-		labelDenials: labelDenials,
-		clientset:    clientset,
+		clientset: clientset,
 	}, nil
 }
 
 // Run the log-enricher to scrap audit logs and enrich them with
 // Kubernetes data (namespace, pod and container).
 func (e *Enricher) Run() error {
-	if e.labelDenials {
-		e.logger.Info("Labeling problematic containers is enabled")
-	}
-
 	e.logger.Info(fmt.Sprintf("Setting up caches with expiry of %v", defaultCacheTimeout))
 	go e.containerIDCache.Start()
 	go e.infoCache.Start()

internal/pkg/daemon/enricher/enricher_test.go

@@ -323,7 +323,7 @@ func TestRun(t *testing.T) {
 		mock := &enricherfakes.FakeImpl{}
 		tc.prepare(mock, lineChan)
 
-		sut, ebuildErr := New(logr.Discard(), false, mock)
+		sut, ebuildErr := New(logr.Discard(), mock)
 		require.NoError(t, ebuildErr)
 
 		var err error