Repository: kubernetes-sigs/security-profiles-operator
Pull request #1157 (2022-09-09 09:59:03)

Introduce vulnerability scans for operator image
#### What type of PR is this?

/kind cleanup

#### What this PR does / why we need it:

We were doing scanning for the build dependencies image, as well as the
UBI-based one. But I missed the SPO itself. This fixes that.

#### Which issue(s) this PR fixes:

None

#### Does this PR have test?

This is a test in CI

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?

```release-note
NONE
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/security-profiles-operator/pull/1157#" title="Author self-approved">JAORMX</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fsecurity-profiles-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/security-profiles-operator/blob/main/OWNERS)~~ [JAORMX]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/security-profiles-operator/pull/1157#" title="Author self-approved">JAORMX</a>*, *<a href="https://github.com/kubernetes-sigs/security-profiles-operator/pull/1157#pullrequestreview-1102072893" title="Approved">saschagrunert</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fsecurity-profiles-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/security-profiles-operator/blob/main/OWNERS)~~ [JAORMX,saschagrunert]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7b95250a1..d678d6a8c 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -58,7 +58,7 @@ jobs:
       - run: make verify-bpf
       - run: make verify-btf
 
-  image:
+  build-image:
     runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v3
@@ -105,6 +105,49 @@ jobs:
           severity: 'CRITICAL,HIGH'
           timeout: 10m
 
+  operator-image:
+    runs-on: ubuntu-22.04
+    steps:
+      - uses: actions/checkout@v3
+      - name: Set up Docker Buildx
+        id: buildx
+        uses: docker/setup-buildx-action@dc7b9719a96d48369863986a06765841d7ea23f6
+      - name: Login to Quay
+        uses: docker/login-action@49ed152c8eca782a232dede0303416e8f356c37b
+        with:
+          registry: quay.io/security-profiles-operator
+          username: security-profiles-operator+github
+          password: ${{ secrets.QUAY_TOKEN }}
+        if: github.ref == 'refs/heads/main'
+      - name: Image metadata
+        id: meta
+        uses: docker/metadata-action@v4
+        with:
+          images: |
+            quay.io/security-profiles-operator/spo
+          tags: |
+            type=ref,event=branch
+            type=ref,event=pr
+            type=ref,event=tag
+            type=sha,format=long
+      - name: Build
+        uses: docker/build-push-action@c84f38281176d4c9cdb1626ffafcd6b3911b5d94
+        with:
+          context: .
+          file: Dockerfile
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
+          load: true
+      - name: Run container image vulnerability scanner
+        uses: aquasecurity/trivy-action@d63413b0a4a4482237085319f7f4a1ce99a8f2ac
+        with:
+          image-ref: ${{ fromJSON(steps.meta.outputs.json).tags[0] }}
+          format: 'table'
+          exit-code: '1'
+          ignore-unfixed: true
+          vuln-type: 'os,library'
+          severity: 'CRITICAL,HIGH'
+
   ubi-image:
     runs-on: ubuntu-22.04
     steps:
diff --git a/cmd/security-profiles-operator/main.go b/cmd/security-profiles-operator/main.go
index d43d816aa..d6d27e2e7 100644
--- a/cmd/security-profiles-operator/main.go
+++ b/cmd/security-profiles-operator/main.go
@@ -514,7 +514,7 @@ func runWebhook(ctx *cli.Context, info *version.Info) error {
 	setupLog.Info("registering webhooks")
 	hookserver := mgr.GetWebhookServer()
 	binding.RegisterWebhook(hookserver, mgr.GetClient())
-	recording.RegisterWebhook(hookserver, mgr.GetClient())
+	recording.RegisterWebhook(hookserver, mgr.GetEventRecorderFor("recording-webhook"), mgr.GetClient())
 
 	sigHandler := ctrl.SetupSignalHandler()
 	setupLog.Info("starting webhook")
diff --git a/installation-usage.md b/installation-usage.md
index 34aad82e5..854314cf3 100644
--- a/installation-usage.md
+++ b/installation-usage.md
@@ -453,6 +453,12 @@ test-recording-redis   Installed   15s
 Recording a SELinux profile would work the same, except you'd use `kind: SelinuxProfile`
 in the `ProfileRecording` object.
 
+Please note that log based recording does not have any effect if the recorded container
+is privileged, that is, the container's security context sets `privileged: true`. This
+is because privileged containers are not subject to SELinux or seccomp policies at all
+and the log based recording makes use of a special seccomp or SELinux profile respectively
+to record the syscalls or SELinux events.
+
 #### eBPF based recording
 
 The operator also supports an [eBPF](https://ebpf.io) based recorder. This
diff --git a/internal/pkg/webhooks/recording/recording.go b/internal/pkg/webhooks/recording/recording.go
index 679eba46a..6f8b8cef0 100644
--- a/internal/pkg/webhooks/recording/recording.go
+++ b/internal/pkg/webhooks/recording/recording.go
@@ -28,6 +28,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -44,16 +45,18 @@ const finalizer = "active-seccomp-profile-recording-lock"
 
 type podSeccompRecorder struct {
 	impl
-	log logr.Logger
+	log    logr.Logger
+	record *utils.SafeRecorder
 }
 
-func RegisterWebhook(server *webhook.Server, c client.Client) {
+func RegisterWebhook(server *webhook.Server, rec record.EventRecorder, c client.Client) {
 	server.Register(
 		"/mutate-v1-pod-recording",
 		&webhook.Admission{
 			Handler: &podSeccompRecorder{
-				impl: &defaultImpl{client: c},
-				log:  logf.Log.WithName("recording"),
+				impl:   &defaultImpl{client: c},
+				log:    logf.Log.WithName("recording"),
+				record: utils.NewSafeRecorder(rec),
 			},
 		},
 	)
@@ -194,6 +197,8 @@ func (p *podSeccompRecorder) updatePod(
 			return false, err
 		}
 
+		p.warnEventIfContainerPrivileged(profileRecording, ctr, pod)
+
 		p.updateSecurityContext(ctr, profileRecording)
 		existingValue, ok := pod.GetAnnotations()[key]
 		if !ok {
@@ -236,9 +241,9 @@ func (p *podSeccompRecorder) updateSecurityContext(
 
 	switch pr.Spec.Kind {
 	case profilerecordingv1alpha1.ProfileRecordingKindSeccompProfile:
-		p.updateSeccompSecurityContext(ctr)
+		p.updateSeccompSecurityContext(ctr, pr)
 	case profilerecordingv1alpha1.ProfileRecordingKindSelinuxProfile:
-		p.updateSelinuxSecurityContext(ctr)
+		p.updateSelinuxSecurityContext(ctr, pr)
 	}
 
 	p.log.Info(fmt.Sprintf(
@@ -249,6 +254,7 @@ func (p *podSeccompRecorder) updateSecurityContext(
 
 func (p *podSeccompRecorder) updateSeccompSecurityContext(
 	ctr *corev1.Container,
+	pr *profilerecordingv1alpha1.ProfileRecording,
 ) {
 	if ctr.SecurityContext == nil {
 		ctr.SecurityContext = &corev1.SecurityContext{}
@@ -256,6 +262,11 @@ func (p *podSeccompRecorder) updateSeccompSecurityContext(
 
 	if ctr.SecurityContext.SeccompProfile == nil {
 		ctr.SecurityContext.SeccompProfile = &corev1.SeccompProfile{}
+	} else {
+		p.record.Eventf(pr,
+			corev1.EventTypeWarning,
+			"SecurityContextAlreadySet",
+			"Container %s had SecurityContext already set, the profile recorder overwrote it", ctr.Name)
 	}
 
 	ctr.SecurityContext.SeccompProfile.Type = corev1.SeccompProfileTypeLocalhost
@@ -269,6 +280,7 @@ func (p *podSeccompRecorder) updateSeccompSecurityContext(
 
 func (p *podSeccompRecorder) updateSelinuxSecurityContext(
 	ctr *corev1.Container,
+	pr *profilerecordingv1alpha1.ProfileRecording,
 ) {
 	if ctr.SecurityContext == nil {
 		ctr.SecurityContext = &corev1.SecurityContext{}
@@ -276,6 +288,11 @@ func (p *podSeccompRecorder) updateSelinuxSecurityContext(
 
 	if ctr.SecurityContext.SELinuxOptions == nil {
 		ctr.SecurityContext.SELinuxOptions = &corev1.SELinuxOptions{}
+	} else {
+		p.record.Eventf(pr,
+			corev1.EventTypeWarning,
+			"SecurityContextAlreadySet",
+			"Container %s had SecurityContext already set, the profile recorder overwrote it", ctr.Name)
 	}
 
 	ctr.SecurityContext.SELinuxOptions.Type = config.SelinuxPermissiveProfile
@@ -391,3 +408,22 @@ func (p *podSeccompRecorder) InjectDecoder(decoder *admission.Decoder) error {
 	p.impl.SetDecoder(decoder)
 	return nil
 }
+
+func (p *podSeccompRecorder) warnEventIfContainerPrivileged(
+	profileRecording *profilerecordingv1alpha1.ProfileRecording,
+	ctr *corev1.Container,
+	pod *corev1.Pod,
+) {
+	if profileRecording.Spec.Recorder != profilerecordingv1alpha1.ProfileRecorderLogs {
+		return
+	}
+
+	if ctr.SecurityContext == nil || ctr.SecurityContext.Privileged == nil || !*ctr.SecurityContext.Privileged {
+		return
+	}
+
+	p.record.Eventf(profileRecording,
+		corev1.EventTypeWarning,
+		"PrivilegedContainer",
+		"Container %s in pod %s is privileged, cannot use log-based profile recording", ctr.Name, pod.Name)
+}
diff --git a/internal/pkg/webhooks/utils/safe_events.go b/internal/pkg/webhooks/utils/safe_events.go
new file mode 100644
index 000000000..bde3d564f
--- /dev/null
+++ b/internal/pkg/webhooks/utils/safe_events.go
@@ -0,0 +1,61 @@
+/*
+Copyright 2022 The Kubernetes 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 utils
+
+import (
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/tools/record"
+)
+
+type SafeRecorder struct {
+	recorder record.EventRecorder
+}
+
+func NewSafeRecorder(recorder record.EventRecorder) *SafeRecorder {
+	return &SafeRecorder{recorder: recorder}
+}
+
+func (sr *SafeRecorder) Event(object runtime.Object, eventtype, reason, message string) {
+	if sr.recorder == nil {
+		return
+	}
+
+	sr.recorder.Event(object, eventtype, reason, message)
+}
+
+// Eventf is just like Event, but with Sprintf for the message field.
+func (sr *SafeRecorder) Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) {
+	if sr.recorder == nil {
+		return
+	}
+
+	sr.recorder.Eventf(object, eventtype, reason, messageFmt, args...)
+}
+
+// AnnotatedEventf is just like eventf, but with annotations attached.
+func (sr *SafeRecorder) AnnotatedEventf(
+	object runtime.Object,
+	annotations map[string]string,
+	eventtype, reason, messageFmt string,
+	args ...interface{},
+) {
+	if sr.recorder == nil {
+		return
+	}
+
+	sr.recorder.AnnotatedEventf(object, annotations, eventtype, reason, messageFmt, args...)
+}


Test patch:
diff --git a/internal/pkg/webhooks/recording/recording_test.go b/internal/pkg/webhooks/recording/recording_test.go
index 19dcb4f2c..c75835c50 100644
--- a/internal/pkg/webhooks/recording/recording_test.go
+++ b/internal/pkg/webhooks/recording/recording_test.go
@@ -36,6 +36,7 @@ import (
 
 	"sigs.k8s.io/security-profiles-operator/api/profilerecording/v1alpha1"
 	"sigs.k8s.io/security-profiles-operator/internal/pkg/webhooks/recording/recordingfakes"
+	"sigs.k8s.io/security-profiles-operator/internal/pkg/webhooks/utils"
 )
 
 var (
@@ -499,7 +500,7 @@ func TestHandle(t *testing.T) {
 		mock := &recordingfakes.FakeImpl{}
 		tc.prepare(mock)
 
-		recorder := podSeccompRecorder{impl: mock, log: logr.Discard()}
+		recorder := podSeccompRecorder{impl: mock, log: logr.Discard(), record: utils.NewSafeRecorder(nil)}
 		resp := recorder.Handle(context.Background(), tc.request)
 		tc.assert(resp)
 	}