Add PodSecurityPolicy to the Helm manifests #259

youssefazrak · 2021-08-12T09:41:18Z

Installing Connaisseur on a Kubernetes cluster that has the
PodSecurityPolicy feature enabled is not working.

This PR fixes this issue by adding the option on Helm chart to take into
acccount the PSP feature.

Fixes: #258

Signed-off-by: Youssef Azrak yazrak.tech@gmail.com

youssefazrak · 2021-08-12T11:05:03Z

@xopham would be great to get a review on this one.
Also, we were thinking of adding a PSP manifest to be deployed directly to the cluster. Sometimes the existing PSPs are not tailored for a specific workload. If you are ok with this, I will add the PSP manifest too.

xopham · 2021-08-12T17:30:10Z

@youssefazrak thanks for the PR. We'll look into this and review.

codecov-commenter · 2021-08-12T17:37:30Z

Codecov Report

Merging #259 (e971b45) into master (3398329) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #259   +/-   ##
=======================================
  Coverage   96.66%   96.66%           
=======================================
  Files          22       22           
  Lines        1078     1078           
=======================================
  Hits         1042     1042           
  Misses         36       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3398329...e971b45. Read the comment docs.

Starkteetje · 2021-08-13T07:39:54Z

helm/values.yaml

+# PodSecurityPolicy for Kubernetes cluster using this feature
+psp:
+  enabled: false
+  name:  # name of the psp 


Not deep into PSPs, but two caveats:

PSPs have been deprecated. I think we don't mind making backwards compatible changes, but we need to make sure that they won't break the future, so we'd probably want to add a k8s version check to {{- if .Values.psp.enabled }} and leave it out for >=1.25 (and maybe warn for >=1.21 [@phbelitz does allow helm that?])

Afaik, one can specify multiple PSPs, so the name field here should be an array, which needs changes to - {{ .Values.psp.name }}.

Also, would we want to check for empty names? With a single empty name, installation breaks if enabled=true, which I'd guess is what we want. But if it becomes an array, do we ignore empty lines (if valid ones exist) or still fail due to invalid k8s manifest? Would guess we choose the latter, but I guess @phbelitz is more into that

I think I would disagree on the first point. In case someone sets a PSP in v1.25 and above, Connaisseur should not silently drop it, but actually fail. As for the thoughts on issuing a warning, I would keep that to Kubernetes itself and not inherit Kubernetes deprecation.

On the second point, I agree that it should be a list and handled as such in case a list is possible (which I think is correct). I would think that an empty list needs to be handled such that no policy is written in that case or an error is thrown. In case the list contains empty elements, I would suggest to fail if it fails.

I will introduce the array part, thanks.

xopham

@youssefazrak a very nice PR. I added some comments that would need to be resolved before merging, but see no major obstacles.

If I understand your earlier comment correctly, you would like to provide a PSP tailored to Connaisseur specifically, right? If that is the case, I would consider it a nice idea. We would just need to make sure that the policy is handled such that Connaisseur won't break by default for Kubernetes v1.25 and above. However, it could probably simply be the default value for the PSP.name which would make it easily applicable.

Many thanks for contributing 🙏

xopham · 2021-08-13T15:15:25Z

helm/templates/role.yaml

+{{- if .Values.psp.enabled }} 
+- apiGroups: ['policy']
+  resources: ['podsecuritypolicies']
+  verbs:     ['use']
+  resourceNames:
+  - {{ .Values.psp.name }}
+{{- end}}


Suggested change

{{- if .Values.psp.enabled }}

- apiGroups: ['policy']

resources: ['podsecuritypolicies']

verbs: ['use']

resourceNames:

- {{ .Values.psp.name }}

{{- end}}

{{- if .Values.psp }}

{{- if .Values.psp.enabled }}

- apiGroups: ['policy']

resources: ['podsecuritypolicies']

verbs: ['use']

resourceNames:

- {{ .Values.psp.name }}

{{- end}}

{{- end}}

when implemented like this, not providing any psp entry in helm/values.yaml does not cause an error which is somewhat more fail safe.

xopham · 2021-08-13T15:16:09Z

helm/templates/webhook/role.yaml

+{{- if .Values.psp.enabled }}
+- apiGroups: ['policy']
+  resources: ['podsecuritypolicies']
+  verbs:     ['use']
+  resourceNames:
+  - {{ .Values.psp.name }}
+{{- end}}


Suggested change

{{- if .Values.psp.enabled }}

- apiGroups: ['policy']

resources: ['podsecuritypolicies']

verbs: ['use']

resourceNames:

- {{ .Values.psp.name }}

{{- end}}

{{- if .Values.psp }}

{{- if .Values.psp.enabled }}

- apiGroups: ['policy']

resources: ['podsecuritypolicies']

verbs: ['use']

resourceNames:

- {{ .Values.psp.name }}

{{- end}}

{{- end}}

same as in comment above

xopham · 2021-08-13T15:23:46Z

helm/values.yaml

@@ -108,6 +108,11 @@ namespacedValidation:
  enabled: false
  mode: ignore  # 'ignore' or 'validate'

+# PodSecurityPolicy for Kubernetes cluster using this feature
+psp:


I think the name should be explicit and self-explanatory as podSecurityPolicy.
Furthermore, I would consider PodSecurityPolicy a part of the deployment section at the beginning of helm/values.yaml, as it only really affects the actual deployment of Connaisseur itself and does not hold any value for its features. I would propose to append to the end of deployment as deployment.podSecurityPolicy.
Note that the templates above will require adjustment.

Fixed and changed the above templates.

xopham · 2021-08-13T15:36:13Z

helm/values.yaml

+# PodSecurityPolicy for Kubernetes cluster using this feature
+psp:
+  enabled: false
+  name:  # name of the psp 


I think I would disagree on the first point. In case someone sets a PSP in v1.25 and above, Connaisseur should not silently drop it, but actually fail. As for the thoughts on issuing a warning, I would keep that to Kubernetes itself and not inherit Kubernetes deprecation.

On the second point, I agree that it should be a list and handled as such in case a list is possible (which I think is correct). I would think that an empty list needs to be handled such that no policy is written in that case or an error is thrown. In case the list contains empty elements, I would suggest to fail if it fails.

xopham

@youssefazrak really like where this PR is going 👍

Awesome that you added a default policy 🙂
I made some comments how we can condense it even further and improve the usability.

A few side notes for the next steps after the discussions are resolved:

rebase is need to run the checks on the PR
we are using conventional commits so in the end the commit name should be something like feat: add pod security policy

Cheers 🙂

xopham · 2021-08-27T08:03:46Z

helm/values.yaml

+    name: # name of the psp
+    customManifest: false # deploy your own podSecurityPolicy manifest


Suggested change

name: # name of the psp

customManifest: false # deploy your own podSecurityPolicy manifest

name: ["connaisseur-psp"] # list of PSPs to use, "connaisseur-psp" is the project-provided default

if you make name a list and define your implemented PSP as default, you shouldn't need the customManifest flag

xopham · 2021-08-27T08:05:06Z

helm/values.yaml

@@ -14,6 +14,11 @@ deployment:
  nodeSelector: {}
  tolerations: []
  affinity: {}
+  # podSecurityPolicy for Kubernetes cluster using this feature


Suggested change

# podSecurityPolicy for Kubernetes cluster using this feature

Think you made explicit what it is, you probably won't need this comment. You could mention the deprecation version if you like though

xopham · 2021-08-27T08:07:36Z

helm/templates/podsecuritypolicy.yaml

+apiVersion: policy/v1beta1
+kind: PodSecurityPolicy
+metadata:
+  name: {{ .Values.deployment.podSecurityPolicy.name }}


Suggested change

name: {{ .Values.deployment.podSecurityPolicy.name }}

name: {{ .Chart.Name }}-psp

Will give the project-provided PSP a standard name that can be referenced in the name list in helm/values.yaml

xopham · 2021-08-27T08:18:32Z

helm/templates/podsecuritypolicy.yaml

@@ -0,0 +1,26 @@
+{{- if and (.Values.deployment.podSecurityPolicy) (.Values.deployment.podSecurityPolicy.enabled) (.Values.deployment.podSecurityPolicy.customManifest) }}


I believe this fails in case there is no .Values.deployment.podSecurityPolicy. If I remember correctly, Helm does not do lazy evaluation.
You might have to do something like:

{{- if .Values.deployment.podSecurityPolicy }} {{- if .Values.deployment.podSecurityPolicy.enabled }} {{- if has {{ .Chart.Name }}-psp .Values.deployment.podSecurityPolicy.name }}

...and finish with {{- end }} three times below. In the last line it can be checked whether the project-provided default PSP is in the list of names (see comment below) which means the the customManifest flag is not required anymore

@xopham I'm still having issues with the {{ .Chart.Name }}-psp part. Tried a bit of everything to get it working inside the curly brackets but still not able to get it done. For the moment I'm using a simple "connaisseur-psp" string.

xopham · 2021-08-27T08:21:29Z

helm/templates/webhook/role.yaml

+  resources: ['podsecuritypolicies']
+  verbs:     ['use']
+  resourceNames:
+  - {{ .Values.deployment.podSecurityPolicy.name }}


this will probably have to be rendered as a list, right?

@xopham indeed, I pushed the change for the list.

youssefazrak · 2021-09-01T17:11:00Z

@xopham thanks again for the review!
Just pushed the fixes minor the comment about the {{ }} inside the {{ if statement }}.
Also rebased and followed the commit description convention.
Cheers!

xopham

Lgtm! Played around with it a bit, but did not do a full test. You probably know more about PSPs and tested that it works in your setup.

Made few minor last comments. After that, I think we can merge.
Will you rebase and sign it or should I?

Looking really neat. Thanks 🙏

xopham · 2021-09-03T11:54:45Z

helm/templates/podsecuritypolicy.yaml

+  requiredDropCapabilities:
+  - ALL
+  runAsUser:
+    rule: 'RunAsAny'


Suggested change

rule: 'RunAsAny'

rule: 'MustRunAsNonRoot'

I think we can be more restrictive here. Haven't tested it yet though

xopham · 2021-09-03T11:55:49Z

helm/templates/podsecuritypolicy.yaml

+  runAsUser:
+    rule: 'RunAsAny'
+  fsGroup:
+    rule: 'RunAsAny'


Suggested change

rule: 'RunAsAny'

rule: 'MustRunAs'

ranges:

- min: 1

max: 65535

w/o having tested yet, I presume we can be more restrictive

xopham · 2021-09-03T11:57:14Z

helm/templates/podsecuritypolicy.yaml

+  seLinux:
+    rule: 'RunAsAny'
+  supplementalGroups:
+    rule: 'RunAsAny'


Suggested change

rule: 'RunAsAny'

rule: 'MustRunAs'

ranges:

- min: 1

max: 65535

again w/o having tested, I believe we can be more restrictive here

youssefazrak · 2021-09-05T09:50:41Z

@xopham cheers!
IIRC restricting to MustRunAsNonRoot caused an issue for the job, hence my less restrictive approach. I will play with it a bit and try to optimize as much as possible.
Regarding the rebasing, I will take care of it no worries. From my understanding, we are rebasing against the develop branch right?
Thanks again xopham, much appreciated!

xopham

@youssefazrak correct. We rebase against develop.
Concerning the rules: if it doesn't work, feel free to drop it. Are you using Openshift or OKD? I think Openshift set its own user and group and therefore doesn't like specifying those. We had another PR recently that tackled this topic: https://github.com/sse-secure-systems/connaisseur/pull/288/files

youssefazrak · 2021-09-07T09:49:13Z

@xopham well, working fine now. Seems like this fixed the issue regarding the webhook job requirements for root privileges.
I pushed the fixes and rebased against the develop branch. Let me know if anything else is needed. Thanks!

Installing Connaisseur on a Kubernetes cluster that has the PodSecurityPolicy feature enabled is not working. This PR fixes this issue by adding the option on Helm chart to take into acccount the PSP feature. Fixes: sse-secure-systems#258 Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>

xopham

lgtm!
Really nice work and collaboration. Thanks for contributing 🙏

addressed

youssefazrak · 2021-09-10T10:17:11Z

@xopham pleasure mate :)

youssefazrak force-pushed the add-psp-helm branch from 7abe20b to e971b45 Compare August 12, 2021 10:06

Starkteetje changed the base branch from master to develop August 13, 2021 07:15

Starkteetje previously requested changes Aug 13, 2021

View reviewed changes

xopham requested changes Aug 13, 2021

View reviewed changes

xopham requested changes Aug 27, 2021

View reviewed changes

youssefazrak force-pushed the add-psp-helm branch 3 times, most recently from c5857c5 to 756a351 Compare September 1, 2021 17:09

xopham mentioned this pull request Sep 2, 2021

v2.2.0 release #301

Merged

13 tasks

phbelitz added this to the v2.2.0 milestone Sep 2, 2021

xopham force-pushed the add-psp-helm branch from e535e2b to 756a351 Compare September 3, 2021 12:24

xopham requested changes Sep 3, 2021

View reviewed changes

xopham force-pushed the add-psp-helm branch from 756a351 to 98d1c94 Compare September 3, 2021 14:30

xopham reviewed Sep 6, 2021

View reviewed changes

youssefazrak force-pushed the add-psp-helm branch from 98d1c94 to c15f283 Compare September 7, 2021 09:39

xopham force-pushed the add-psp-helm branch from c15f283 to 6f7cd85 Compare September 10, 2021 09:41

xopham approved these changes Sep 10, 2021

View reviewed changes

xopham merged commit 6f7cd85 into sse-secure-systems:develop Sep 10, 2021

youssefazrak deleted the add-psp-helm branch September 10, 2021 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PodSecurityPolicy to the Helm manifests #259

Add PodSecurityPolicy to the Helm manifests #259

youssefazrak commented Aug 12, 2021

youssefazrak commented Aug 12, 2021

xopham commented Aug 12, 2021

codecov-commenter commented Aug 12, 2021

Starkteetje Aug 13, 2021

xopham Aug 13, 2021

youssefazrak Aug 26, 2021

xopham left a comment

xopham Aug 13, 2021

youssefazrak Aug 26, 2021

xopham Aug 13, 2021

youssefazrak Aug 26, 2021

xopham Aug 13, 2021

youssefazrak Aug 26, 2021

xopham Aug 13, 2021

xopham left a comment

xopham Aug 27, 2021

xopham Aug 27, 2021

youssefazrak Sep 1, 2021

xopham Aug 27, 2021

xopham Aug 27, 2021

youssefazrak Sep 1, 2021

xopham Aug 27, 2021

youssefazrak Sep 1, 2021 •

edited

youssefazrak commented Sep 1, 2021

xopham left a comment •

edited

xopham Sep 3, 2021

xopham Sep 3, 2021

xopham Sep 3, 2021

youssefazrak commented Sep 5, 2021

xopham left a comment

youssefazrak commented Sep 7, 2021

xopham left a comment

youssefazrak commented Sep 10, 2021

		name: # name of the psp
		customManifest: false # deploy your own podSecurityPolicy manifest

	name: # name of the psp
	customManifest: false # deploy your own podSecurityPolicy manifest
	name: ["connaisseur-psp"] # list of PSPs to use, "connaisseur-psp" is the project-provided default

	name: {{ .Values.deployment.podSecurityPolicy.name }}
	name: {{ .Chart.Name }}-psp

		@@ -0,0 +1,26 @@
		{{- if and (.Values.deployment.podSecurityPolicy) (.Values.deployment.podSecurityPolicy.enabled) (.Values.deployment.podSecurityPolicy.customManifest) }}

-    rule: 'RunAsAny'
+    rule: 'MustRunAs'
+    ranges:
+      - min: 1
+        max: 65535

Add PodSecurityPolicy to the Helm manifests #259

Add PodSecurityPolicy to the Helm manifests #259

Conversation

youssefazrak commented Aug 12, 2021

youssefazrak commented Aug 12, 2021

xopham commented Aug 12, 2021

codecov-commenter commented Aug 12, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xopham 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

xopham 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

youssefazrak Sep 1, 2021 • edited

Choose a reason for hiding this comment

youssefazrak commented Sep 1, 2021

xopham left a comment • edited

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

youssefazrak commented Sep 5, 2021

xopham left a comment

Choose a reason for hiding this comment

youssefazrak commented Sep 7, 2021

xopham left a comment

Choose a reason for hiding this comment

youssefazrak commented Sep 10, 2021

youssefazrak Sep 1, 2021 •

edited

xopham left a comment •

edited