Support restricted PSA for worker pods #2410

mhenriks · 2022-08-25T15:20:12Z

Signed-off-by: Michael Henriksen [email protected]

What this PR does / why we need it:

Attempt to get in compliance with restricted PSA. See: https://kubernetes.io/docs/concepts/security/pod-security-admission/

Remove root pods
Remove selinux params
SecurityContextConstraint update
Run func test in restricted SCC

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Support Restricted PSA for worker pods

mhenriks · 2022-08-25T15:20:19Z

/hold

awels · 2022-08-25T16:05:26Z

I suspect the nbdkit socket will be an issue here.

mhenriks · 2022-08-26T13:07:18Z

I suspect the nbdkit socket will be an issue here.

please elaborate

awels · 2022-08-26T13:13:12Z

Basically when I was working on #2331 I also tried removing root as part of that, and when I was running tests, they started failing because I was unable to access the nbdkit socket since I wasn't root. I didn't look into the exact reason much more though.

awels · 2022-08-26T13:15:17Z

But it looks like the tests are passing, so maybe not an issue.

akalenyu

Basically when I was working on #2331 I also tried removing root as part of that, and when I was running tests, they started failing because I was unable to access the nbdkit socket since I wasn't root. I didn't look into the exact reason much more though.

Really interested about why last time around we saw imports failing on block without root.
Maybe related to us switching to less nbdkit usages?

Attempt to get in compliance with restricted PSA. See: https://kubernetes.io/docs/concepts/security/pod-security-admission/

Reading the k8s docs IIUC we need to explicitly set more things to comply with "restricted":

allowPrivilegeEscalation to false (done by Comply with restricted security context #2331)
runAsNonRoot (on pod level is easier)
seccompProfile (pod level)

What am I missing?

SecurityContextConstraint update

If we end up changing our version of the SCC we should probably make the reconciliation of it more strict so it gets upgraded properly. Currently, it'll only reconcile if

containerized-data-importer/pkg/operator/controller/scc.go

Lines 105 to 109 in 13a9b12

    
           if !sdk.ContainsStringValue(scc.Users, userName) { 
        
           	scc.Users = append(scc.Users, userName) 
        
           	return c.Update(context.TODO(), scc) 
        
           }

OFC this is a potential issue regardless of this PR so we can tackle elsewhere

akalenyu · 2022-08-28T08:24:58Z

pkg/controller/clone-controller.go

@@ -733,6 +723,7 @@ func MakeCloneSourcePodSpec(sourceVolumeMode corev1.PersistentVolumeMode, image,
 			{
 				Name:      DataVolName,
 				MountPath: common.ClonerMountPath,
+				ReadOnly:  true,


Nice, is there a real impact for this? IIUC the other ReadOnly is just about enforcing this one

akalenyu · 2022-08-28T08:34:04Z

pkg/controller/clone-controller.go

@@ -592,15 +592,6 @@ func MakeCloneSourcePodSpec(sourceVolumeMode corev1.PersistentVolumeMode, image,
 			},
 		},
 		Spec: corev1.PodSpec{
-			SecurityContext: &corev1.PodSecurityContext{


Nice! this will fix an issue someone was seeing on OpenShift 4.12 with kubevirt tekton tasks https://github.com/kubevirt/tekton-tasks-operator/blob/main/data/tekton-pipelines/okd/windows10-customize.yaml#L126
I can try to find the chats if needed

mhenriks · 2022-08-31T01:41:47Z

/hold

mhenriks · 2022-09-06T20:37:18Z

/hold cancel

mhenriks · 2022-09-09T00:24:22Z

/retest-required

mhenriks · 2022-09-09T13:49:26Z

/test pull-cdi-linter

mhenriks · 2022-09-09T13:51:33Z

/retest

akalenyu

Awesome
We probably won't hit the same issues as in the PR description here, right? kubevirt/kubevirt#8436

akalenyu · 2022-09-11T13:32:39Z

pkg/importer/http-datasource.go

@@ -46,8 +46,8 @@ import (

 const (
 	tempFile         = "tmpimage"
-	nbdkitPid        = "/var/run/nbdkit.pid"
-	nbdkitSocket     = "/var/run/nbdkit.sock"
+	nbdkitPid        = "/tmp/nbdkit.pid"


are we concerned about some OS variants cleaning up /tmp after a certain period of time?

I've never heard of any tmp cleanup happening in the absence of a reboot.

https://serverfault.com/a/377349
For example (RHEL)
But I don't this applies/matters much to our ephemeral workloads

akalenyu · 2022-09-11T13:33:10Z

pkg/importer/registry-datasource.go

@@ -165,7 +165,7 @@ func getImageFileName(dir string) (string, error) {
 }

 func createCertificateDir(registryCertDir string) (string, error) {
-	allCerts := "/var/local/all_certs"
+	allCerts := "/tmp/all_certs"


Same here, I am missing why /var/local wasn't good enough?

non root cannot write to /var/local

akalenyu · 2022-09-11T13:35:50Z

pkg/operator/controller/scc.go

@@ -37,6 +37,34 @@ import (

 const sccName = "containerized-data-importer"

+func setSCC(scc *secv1.SecurityContextConstraints) {


Not really important but I recently discovered https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil#CreateOrUpdate which could've worked in ensureSCCExists()

Cool, maybe next time

akalenyu · 2022-09-11T13:38:52Z

pkg/util/util.go

-	}
-	options := fmt.Sprintf("-%s%s", tarOptions, "xvC")
-	untar := exec.Command("/usr/bin/tar", "--no-same-owner", options, destDir, strings.Join(args, ""))
+	untar := exec.Command("/usr/bin/tar", "--no-same-owner", "-xvC", destDir)


WDYT about printing the whole cmd before executing it with verbosity lvl 3?
always nice when debugging, we do the same for qemu-img

I saw you took up the suggestion in the clone source pod but not here. Intended?

akalenyu · 2022-09-11T14:20:19Z

cmd/cdi-cloner/clone-source.go

+		args = append(args, "--files-from", "/dev/null")
+	}
+
+	cmd := exec.Command("/usr/bin/tar", args...)


Same here, if you're up for printing the args of course

mhenriks · 2022-09-11T22:12:53Z

Awesome We probably won't hit the same issues as in the PR description here, right? kubevirt/kubevirt#8436

What issues, specifically? They are attacking the issue in a totally different way for now

Signed-off-by: Michael Henriksen <[email protected]>

refactor scc management Signed-off-by: Michael Henriksen <[email protected]>

akalenyu · 2022-09-13T08:47:57Z

tests/framework/framework.go

+	// pod-security.kubernetes.io/<MODE>: <LEVEL>
+	labels["pod-security.kubernetes.io/enforce"] = "restricted"
+	if utils.IsOpenshift(f.K8sClient) {
+		labels["security.openshift.io/scc.podSecurityLabelSync"] = "false"


Nice OpenShift awareness!

akalenyu · 2022-09-13T08:48:14Z

Awesome We probably won't hit the same issues as in the PR description here, right? kubevirt/kubevirt#8436

What issues, specifically? They are attacking the issue in a totally different way for now

Looks good just let me know if you want to tackle the few small replies, otherwise I think we are good

jean-edouard

Just one minor comment in-line

jean-edouard · 2022-09-13T13:16:56Z

cluster-sync/clean.sh

    fi
 fi

+set +e
+_kubectl patch cdi ${CR_NAME} --type=json -p '[{ "op": "remove", "path": "/metadata/finalizers" }]' | echo "this is fine"


DId you mean || echo "this is fine"? Piping the output of the kubectl command to echo does nothing...
Either way however, the error code of the kubectl command will be ignored, so set +e/set -e should not be necessary here.
But the code works the way it is, so no big deal.

Signed-off-by: Michael Henriksen <[email protected]>

mhenriks · 2022-09-14T13:46:19Z

/test pull-containerized-data-importer-e2e-destructive

akalenyu · 2022-09-14T14:03:26Z

/lgtm

mhenriks · 2022-09-14T20:14:54Z

/approve

kubevirt-bot · 2022-09-14T20:14:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Michael Henriksen <[email protected]>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 25, 2022

kubevirt-bot requested review from aglitke and maya-r August 25, 2022 15:20

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S labels Aug 25, 2022

mhenriks force-pushed the restricted-psa branch from 9d6fe45 to 0298d15 Compare August 25, 2022 15:20

mhenriks changed the title ~~Support restricted PSA~~ Support restricted PSA for worker pods Aug 25, 2022

kubevirt-bot added size/M and removed size/S labels Aug 25, 2022

akalenyu reviewed Aug 28, 2022

View reviewed changes

mhenriks force-pushed the restricted-psa branch from 13a9b12 to 50bf8b5 Compare September 6, 2022 20:36

kubevirt-bot added size/L and removed size/M labels Sep 6, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2022

mhenriks force-pushed the restricted-psa branch 2 times, most recently from 74f1da8 to 1f1c0fc Compare September 6, 2022 23:56

akalenyu reviewed Sep 11, 2022

View reviewed changes

remove root worker pods

35b069e

Signed-off-by: Michael Henriksen <[email protected]>

mhenriks added 5 commits September 12, 2022 09:34

remove selinux requirement for worker pods

b63cf43

Signed-off-by: Michael Henriksen <[email protected]>

run tests in restricted namespace and required changes

af99d02

Signed-off-by: Michael Henriksen <[email protected]>

handle empty tar

150f40a

Signed-off-by: Michael Henriksen <[email protected]>

add PSA label when running functional tests in OpenShift

363ec7d

Signed-off-by: Michael Henriksen <[email protected]>

cannot use restricted PSA with istio (for now)

0338798

refactor scc management Signed-off-by: Michael Henriksen <[email protected]>

mhenriks force-pushed the restricted-psa branch from f53115b to 0338798 Compare September 13, 2022 02:00

akalenyu reviewed Sep 13, 2022

View reviewed changes

jean-edouard approved these changes Sep 13, 2022

View reviewed changes

kubevirt-bot assigned jean-edouard Sep 13, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2022

xpivarc mentioned this pull request Sep 13, 2022

Integrate with Pod security kubevirt/kubevirt#8436

Merged

fix clean script

fb8a29d

Signed-off-by: Michael Henriksen <[email protected]>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2022

kubevirt-bot assigned akalenyu Sep 14, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2022

kubevirt-bot merged commit 46c6aa9 into kubevirt:main Sep 14, 2022

0xFelix mentioned this pull request Sep 29, 2022

cdi-source-update-poller cannot be started because of failing PSA #2437

Closed

farcaller mentioned this pull request Sep 30, 2022

virtctl image-upload xxx cannot open /dev/cdi-block-volume: Permission denied #2433

Closed

mhenriks mentioned this pull request Jan 9, 2023

mount block pvc readonly #2536

Merged

mhenriks added a commit to mhenriks/containerized-data-importer that referenced this pull request Apr 13, 2023

backport tar handling from kubevirt#2410

c8f96f9

Signed-off-by: Michael Henriksen <[email protected]>

kubevirt-bot pushed a commit that referenced this pull request Apr 17, 2023

backport tar handling from #2410 (#2693)

ab5540d

Signed-off-by: Michael Henriksen <[email protected]>

mhenriks added a commit to mhenriks/containerized-data-importer that referenced this pull request Apr 26, 2023

backport tar handling from kubevirt#2410

320e191

Signed-off-by: Michael Henriksen <[email protected]>

kubevirt-bot pushed a commit that referenced this pull request Apr 26, 2023

backport tar handling from #2410 (#2702)

2f8915d

Signed-off-by: Michael Henriksen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support restricted PSA for worker pods #2410

Support restricted PSA for worker pods #2410

mhenriks commented Aug 25, 2022 •

edited

Loading

mhenriks commented Aug 25, 2022

awels commented Aug 25, 2022

mhenriks commented Aug 26, 2022

awels commented Aug 26, 2022

awels commented Aug 26, 2022

akalenyu left a comment •

edited

Loading

akalenyu Aug 28, 2022

akalenyu Aug 28, 2022

mhenriks commented Aug 31, 2022

mhenriks commented Sep 6, 2022

mhenriks commented Sep 9, 2022

mhenriks commented Sep 9, 2022

mhenriks commented Sep 9, 2022

akalenyu left a comment

akalenyu Sep 11, 2022

mhenriks Sep 11, 2022

akalenyu Sep 13, 2022

akalenyu Sep 11, 2022

mhenriks Sep 11, 2022

akalenyu Sep 13, 2022

akalenyu Sep 11, 2022

mhenriks Sep 11, 2022

akalenyu Sep 11, 2022

akalenyu Sep 13, 2022

akalenyu Sep 11, 2022

mhenriks commented Sep 11, 2022

akalenyu Sep 13, 2022

akalenyu commented Sep 13, 2022

jean-edouard left a comment

jean-edouard Sep 13, 2022

mhenriks commented Sep 14, 2022

akalenyu commented Sep 14, 2022

mhenriks commented Sep 14, 2022

kubevirt-bot commented Sep 14, 2022

	if !sdk.ContainsStringValue(scc.Users, userName) {
	scc.Users = append(scc.Users, userName)

	return c.Update(context.TODO(), scc)
	}

		@@ -37,6 +37,34 @@ import (

		const sccName = "containerized-data-importer"

		func setSCC(scc *secv1.SecurityContextConstraints) {

Support restricted PSA for worker pods #2410

Support restricted PSA for worker pods #2410

Conversation

mhenriks commented Aug 25, 2022 • edited Loading

mhenriks commented Aug 25, 2022

awels commented Aug 25, 2022

mhenriks commented Aug 26, 2022

awels commented Aug 26, 2022

awels commented Aug 26, 2022

akalenyu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Aug 31, 2022

mhenriks commented Sep 6, 2022

mhenriks commented Sep 9, 2022

mhenriks commented Sep 9, 2022

mhenriks commented Sep 9, 2022

akalenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Sep 11, 2022

Choose a reason for hiding this comment

akalenyu commented Sep 13, 2022

jean-edouard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Sep 14, 2022

akalenyu commented Sep 14, 2022

mhenriks commented Sep 14, 2022

kubevirt-bot commented Sep 14, 2022

mhenriks commented Aug 25, 2022 •

edited

Loading

akalenyu left a comment •

edited

Loading