feat: 177 remove unneeded dependencies due to security concerns #185

BGrasnick · 2023-08-29T16:15:15Z

~~Feat: remove deprecated mount of /sys/kernel/debug which is not needed anymore since add an option to use libbpf attacher kepler#752~~
Feat: make pod use HostPID to avoid any possible PID collision in the future
Feat: make as many mounted host volumes as possible ReadOnly: true
Feat: reduce priviliges granted through SecurityContextConstraints as minimal as possible
Test: add test for HostPID: true
Test: volume mounts are set up correctly
Test: add tests for SCC allows
Test: add tests for volumes
Chore: move utility functions to pkg/utils/k8s/k8s.go

handles one part of #177

…d anymore since sustainable-computing-io/kepler#752 Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

… future Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

… minimal as possible Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

no /sys/kernel/debug and as many VolumeMounts as possible ReadOnly Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

BGrasnick · 2023-08-29T16:20:10Z

I tested this with both kind locally as well as an OpenShift cluster running version 4.11.47.
All go unit tests ran successfully locally.

I was not so sure about the testing code because I only have 1 test case each but wanted to follow the previous approach started with the NodeSelection and Tolerations test. However, maybe this is a bit too much and could be made more minimal. I am open to your feedback and suggestions :)

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

sthaha · 2023-08-29T21:55:03Z

pkg/components/exporter/exporter_test.go

+		AllowHostPID:             SCC.AllowHostPID,
+		AllowHostPorts:           SCC.AllowHostPorts,
+	}
+}


nit: (feel free to ignore): I think these generic utility functions are better placed in pkg/utils/k8s so that these are useful in e2e as and in controllers as well.

sthaha · 2023-08-29T21:57:34Z

@BGrasnick , thanks a lot for the contribution 🤗. Looks good to me. Since this needs to be tested in openshift 4.13 (due to SCC chnage), we will have to do it manually. @vimalk78 does this align with what you were also trying to achieve?

@vprashar2929 could you please take a look as well?

vimalk78 · 2023-08-30T05:56:37Z

pkg/components/exporter/exporter.go

-							{Name: "lib-modules", MountPath: "/lib/modules"},
-							{Name: "tracing", MountPath: "/sys"},
-							{Name: "kernel-src", MountPath: "/usr/src/kernels"},
-							{Name: "kernel-debug", MountPath: "/sys/kernel/debug"},


we need sys/kernel/debug for bcc tracepoints to compile. can we remove this when bcc is removed?

If I understand correctly, it should be obsolete now that sustainable-computing-io/kepler#752 is merged and released

@BGrasnick Could you please clarify which release contains that PR?

git tag --contains 885c1bc37ba18e00725149cf06c3f53c86846b59

returns nothing 🤔

Also note that this version of operator deploy 0.5.4 version of kepler. See https://github.com/sustainable-computing-io/kepler-operator/blob/v1alpha1/pkg/components/exporter/exporter.go#L51

Would you like to update that as well?

I don't know what happened with the commit and its SHA but apparently the change is in the main branch and also releases 0.5.2, 0.5.3 and 0.5.4. It was first merged, then reverted and the revert was reverted again...

But in quay there is also a release-0.5.3-libbpf, release-0.5.4-libbpf and latest-libbpf build.

Regarding the version I don't quite understand because as far as I can tell, this PR also uses release-0.5.4 version of kepler.

Also I just tried locally with our 4.11 OpenShift cluster and from what I can see this PR works with both works with latest and latest-libbpf images of kepler. There were some missing metrics when using release-0.5.4 but I think this is due to sustainable-computing-io/kepler#877. So it should work with both bcc and libbpf without mounting sys/kernel/debug but I am no expert in that area and would love if you could confirm this.

If I understand correctly, right now the user can choose whether they want to use bcc or libbpf by using a specific image of kepler. If sys/kernel/debug is necessary for bcc maybe we can make this optional and enable choosing bcc vs libbpf image in the spec of kepler CR and then optionally mount sys/kernel/debug? Is the plan for the future to offer both or go for only one of them?

I am not sure about this either since it is @vimalk78 who has been looking into kepler implementation (including bcc)

@rootfs , could you please help us resolve this?
I guess the question is if we require {Name: "kernel-debug", MountPath: "/sys/kernel/debug"}, mount for 0.5.4 to work properly given it uses bcc?

@sthaha This is a good question. The k8s kepler exporter yaml doesn't have it now https://github.com/sustainable-computing-io/kepler/blob/main/manifests/config/exporter/exporter.yaml, but for some unknown reason it is required for openshift when first experimented https://github.com/sustainable-computing-io/kepler/blob/main/manifests/config/exporter/patch/patch-openshift.yaml

Would you please check if this is still the requirement for openshift? It would be great if kepler doesn't need this mount any more.

@BGrasnick I would love to include this patch in the release we are going to make today. Since we don't have a consensus on whether {Name: "kernel-debug", MountPath: "/sys/kernel/debug"} is required, would you mind restoring that back which gives buys us time to properly investigate this issue?

Yeah we could do that and separate it into another PR and then both together would fix the issue. If you don't mind, what is the release cadence so when could this /sys/kernel/debug fix be released in the future?

I created #198 as a follow-up PR for the /sys/kernel/debug mount. Let's continue the discussion there! @vimalk78 @sthaha @rootfs

vimalk78 · 2023-08-30T06:00:33Z

pkg/components/exporter/exporter_test.go

+		AllowHostPID:             SCC.AllowHostPID,
+		AllowHostPorts:           SCC.AllowHostPorts,
+	}
+}


sthaha · 2023-08-30T08:09:08Z

@BGrasnick, could you also please rebase this patch on to the latest v1alpha1 which has the fix for "broken" e2e tests.

…e-to-security-concerns

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

BGrasnick · 2023-08-30T08:39:51Z

Rebased and put the util functions from the tests into pkg/utils/k8s :)
If there is anything else we could improve, let me know!

sthaha · 2023-08-31T00:57:57Z

pkg/components/exporter/exporter.go

@@ -121,6 +121,7 @@ func NewDaemonSet(detail components.Detail, k *v1alpha1.Kepler) *appsv1.DaemonSe
 					Labels:    podSelector,
 				},
 				Spec: corev1.PodSpec{
+					HostPID:            true,


Is this required?

Hey @sthaha, I added this because kepler mounts /proc and therefore can access the PID namespace of the host. I am not sure about the internals of kepler yet but apparently it does not interfere with the host PID namespace yet. However, if we don't set HostPID: true, the kepler pod will have its own PID namespace. In this kepler PID namespace maybe some process for example has PID 0 or 1 and another process on the host could have the same PID. If in the future kepler would somehow interact with this PID there could be collisions and unwanted behavior like interacting with systemd without knowing. To prevent this, HostPID: true should be set so there are no PID collisions between host and kepler pod.

@rootfs may be kepler manifests should have this as well

… it is obsolete Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

sthaha

Thanks for the contribution @BGrasnick

BGrasnick added 7 commits August 29, 2023 17:35

Feat: remove deprecated mount of /sys/kernel/debug which is not neede…

f7a6751

…d anymore since sustainable-computing-io/kepler#752 Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Feat: make pod use HostPID to avoid any possible PID collision in the…

fc9ff77

… future Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Feat: make as many mounted host volumes as possible ReadOnly: true

fec315a

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Feat: reduce priviliges granted through SecurityContextConstraints as…

7a8c6bb

… minimal as possible Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Test: add test for HostPID: true

031317b

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Test: volume mounts are set up correctly

81e2b57

no /sys/kernel/debug and as many VolumeMounts as possible ReadOnly Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

Test: add tests for SCC allows

0a675f5

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

BGrasnick changed the title ~~feat/177 remove unneeded dependencies due to security concerns~~ feat: 177 remove unneeded dependencies due to security concerns Aug 29, 2023

fixup! Test: add tests for SCC allows

45ad03c

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

BGrasnick force-pushed the feat/177-remove-unneeded-dependencies-due-to-security-concerns branch from b858848 to 45ad03c Compare August 29, 2023 16:24

sthaha reviewed Aug 29, 2023

View reviewed changes

vimalk78 reviewed Aug 30, 2023

View reviewed changes

BGrasnick added 2 commits August 30, 2023 10:31

Merge branch 'v1alpha1' into feat/177-remove-unneeded-dependencies-du…

3bb7ddc

…e-to-security-concerns

Chore: move utility functions to pkg/utils/k8s/k8s.go and fix new tests

dcad6bb

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

sthaha reviewed Aug 31, 2023

View reviewed changes

Feat: add back in /sys/kernel/debug mount as it is not certain yet if…

92840d6

… it is obsolete Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

BGrasnick force-pushed the feat/177-remove-unneeded-dependencies-due-to-security-concerns branch from 618fe62 to 92840d6 Compare August 31, 2023 10:15

Test: add tests for volumes

c22e2ec

Signed-off-by: Bastien Grasnick <bastien.grasnick@deutschebahn.com>

sthaha approved these changes Aug 31, 2023

View reviewed changes

sthaha merged commit 368f975 into sustainable-computing-io:v1alpha1 Sep 1, 2023

BGrasnick mentioned this pull request Sep 12, 2023

Feat: 771 remove unnecessary security concerns sustainable-computing-io/kepler#926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 177 remove unneeded dependencies due to security concerns #185

feat: 177 remove unneeded dependencies due to security concerns #185

BGrasnick commented Aug 29, 2023 •

edited

Loading

BGrasnick commented Aug 29, 2023

sthaha Aug 29, 2023

vimalk78 Aug 30, 2023

sthaha commented Aug 29, 2023

vimalk78 Aug 30, 2023

BGrasnick Aug 30, 2023

sthaha Aug 30, 2023

sthaha Aug 30, 2023

BGrasnick Aug 30, 2023

sthaha Aug 30, 2023

rootfs Aug 30, 2023

sthaha Aug 31, 2023

BGrasnick Aug 31, 2023

BGrasnick Sep 1, 2023

vimalk78 Aug 30, 2023

sthaha commented Aug 30, 2023

BGrasnick commented Aug 30, 2023

sthaha Aug 31, 2023

BGrasnick Aug 31, 2023

sthaha Aug 31, 2023

sthaha left a comment

feat: 177 remove unneeded dependencies due to security concerns #185

feat: 177 remove unneeded dependencies due to security concerns #185

Conversation

BGrasnick commented Aug 29, 2023 • edited Loading

BGrasnick commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha commented Aug 29, 2023

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

sthaha commented Aug 30, 2023

BGrasnick commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

BGrasnick commented Aug 29, 2023 •

edited

Loading