chore: adding cel for psp-seccomp policy #540

JaydipGabani · 2024-06-03T21:34:25Z

What this PR does / why we need it:

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

Special notes for your reviewer:

Signed-off-by: Jaydip Gabani <[email protected]>

src/pod-security-policy/seccomp/src.cel

maxsmythe · 2024-06-08T02:19:52Z

src/pod-security-policy/seccomp/src.cel

+    })
+- name: allBadContainerProfiles
+  expression: |
+    variables.podAnnotationsProfiles + variables.containerAnnotationsProfiles + variables.podSecurityContextProfiles + variables.containerSecurityContextProfiles + variables.containerProfilesMissing


Kubernetes no longer reads security contexts from the annotation, we should give constraint authors the ability to turn off relying on the annotation in order to avoid a security hole where pod runners "pretend" to have the correct security context.

I.e. we should add a parameter to turn off reading from the annotation.

Here is the k8s code for retrieving the seccomp profile:

https://github.com/kubernetes/kubernetes/blob/c6b5191c37f939d2d61e76de222a96ae5f5d9558/pkg/kubelet/kuberuntime/helpers.go#L270-L291

Added v2 for seccomp with a variable to enable/disable reading from annotations.

If we are creating "v2", we should just remove annotations from v2 altogether, since K8s no longer supports seccomp annotations

…read from annotations Signed-off-by: Jaydip Gabani <[email protected]>

Signed-off-by: Jaydip Gabani <[email protected]>

src/pod-security-policy/seccomp/src.rego

src/pod-security-policy/seccompv2/constraint.tmpl

maxsmythe · 2024-06-19T00:33:35Z

src/pod-security-policy/seccomp/src.rego

@@ -99,7 +70,7 @@ get_allowed_profiles[allowed] {
 # The simply translated profiles
 get_allowed_profiles[allowed] {
    profile := input.parameters.allowedProfiles[_]
-    not startswith(lower(profile), "localhost")


Instead of keeping naming_translation we can:

if there is a "/" in the name, use the name directly (assume its already in annotation format)

If there is no "/" in the name, build a profile object (via a function call), then call canonicalize_seccomp_profile

This ensures we use the same translation logic everywhere.

Signed-off-by: Jaydip Gabani <[email protected]>

maxsmythe · 2024-06-25T02:48:09Z

src/pod-security-policy/seccompv2/src.cel

+  expression: |
+    (variables.inputAllowedProfiles.filter(profile,
+    profile != "Localhost").map(profile, profile == "Unconfined" ? "unconfined" : profile)) + 
+    (variables.inputAllowedProfiles.exists(profile, profile == "RuntimeDefault") ? ["runtime/default", "docker/default"] : [])


No need to do the mapping for the v2 policy, that may simplify the code a bit.

Do you mean we should assume we have only SC style profile names in the input.params.allowedProfile since we are not using annotations in v2?
if that is the case, then we would not need to canonicalize obj.securityContext.seccompProfile.type as well.

Signed-off-by: Jaydip Gabani <[email protected]>

…ary into psp-seccomp

Signed-off-by: Jaydip Gabani <[email protected]>

…ary into psp-seccomp

Signed-off-by: Jaydip Gabani <[email protected]>

sozercan · 2024-09-03T23:22:14Z

website/docs/validation/seccompv2.md

+# Seccomp V2
+
+## Description
+Controls the seccomp profile used by containers. Corresponds to the `seccomp.security.alpha.kubernetes.io/allowedProfileNames` annotation on a PodSecurityPolicy. For more information, see https://kubernetes.io/docs/concepts/policy/pod-security-policy/#seccompv2


what's different between non-v2 and v2?

looks like difference is the annotation. can we document it here?

Updated description.

sozercan · 2024-09-04T00:59:31Z

artifacthub/library/pod-security-policy/seccompv2/1.0.0/artifacthub-pkg.yml

+name: k8spspseccompv2
+displayName: Seccomp V2
+createdAt: "2024-06-13T18:28:19Z"
+description: Controls the seccomp profile used by containers. Corresponds to the `seccomp.security.alpha.kubernetes.io/allowedProfileNames` annotation on a PodSecurityPolicy. For more information, see https://kubernetes.io/docs/concepts/policy/pod-security-policy/#seccompv2


we should update the description too

Updated description.

Signed-off-by: Jaydip Gabani <[email protected]>

…ary into psp-seccomp

sozercan

doc change request, otherwise LGTM

sozercan

Thanks! LGTM

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani · 2024-09-12T16:19:30Z

@maxsmythe @ritazh PTAL

maxsmythe

Couple comments on v2, have not reviewed v1. Looking close!

maxsmythe · 2024-09-13T00:25:30Z

library/pod-security-policy/seccompv2/template.yaml

+              description: >-
+                An array of allowed profile values for seccomp on Pods/Containers.
+
+                Can use the annotation naming scheme: `runtime/default`, `docker/default`, `unconfined` and/or


We should get rid of mention/use of the annotation naming scheme for V2, since that is no longer a recognized paradigm for K8s

maxsmythe · 2024-09-13T00:31:36Z

library/pod-security-policy/seccompv2/template.yaml

+                profile := input.parameters.allowedProfiles[_]
+                profile == "Localhost"
+                file := object.get(input.parameters, "allowedLocalhostFiles", [])[_]
+                allowed := sprintf("Localhost/%s", [file])


rather than attempting string packing/unpacking, we can just mimic the structure of local host profiles, this will simplify comparison later on.

basically, allowed := {"type": allowed, "localHostProfile": file}

Similar for above.

Then get_profile() is also simplified, as is the comparison with localhost. We also are no longer dependent on our scratch string format being invalidated.

maxsmythe · 2024-09-13T00:36:52Z

library/pod-security-policy/seccompv2/template.yaml

+          - name: inputAllowedProfiles
+            expression: |
+              !has(variables.params.allowedProfiles) ? [] : variables.params.allowedProfiles
+          - name: derivedAllowedLocalhostFiles


like the Rego code, we should avoid using string transformations here, just structs.

In this case, we can keep the params.allowedLocalhostFiles strings unmodified.

maxsmythe · 2024-09-13T00:40:46Z

library/pod-security-policy/seccompv2/template.yaml

+          - name: podLocalHostProfile
+            expression: |
+              variables.hasPodSeccomp && has(variables.anyObject.spec.securityContext.seccompProfile.localhostProfile) ? variables.anyObject.spec.securityContext.seccompProfile.localhostProfile : ""
+          - name: podSecurityContextProfile


nit: podSecurityContextProfileType, that is what is actually being stored, and otherwise you are relying on users to read a plural (extra "s") to distinguish between two very different bits of data.

maxsmythe · 2024-09-13T00:42:39Z

library/pod-security-policy/seccompv2/template.yaml

+              variables.allContainerProfiles.filter(badContainerProfile, 
+                badContainerProfile.profile == "Localhost" &&
+                !variables.allowAllLocalhostFiles &&
+                !((badContainerProfile.profile + "/" + badContainerProfile.file) in variables.allowedProfiles)


as above, prefer field comparison to string manipulation

Signed-off-by: Jaydip Gabani <[email protected]>

maxsmythe

v2 looks good modulo docstring comment.

v1 appears to have a bug WRT localhost

maxsmythe · 2024-09-27T21:41:37Z

library/pod-security-policy/seccompv2/template.yaml

+                An array of allowed profile values for seccomp on Pods/Containers.
+
+                Can use the securityContext naming scheme: `RuntimeDefault`, `Unconfined`
+                and/or `Localhost`. For securityContext `Localhost`, use the parameter `allowedLocalhostProfiles`


Parameter is called allowedLocalHostFiles below. Which name is more correct?

Updated the name. allowedLocalHostFiles is correct.

maxsmythe · 2024-09-27T22:06:50Z

library/pod-security-policy/seccomp/template.yaml

+          - name: localhostWildcardAllowed
+            expression: |
+              variables.inputAllowedProfiles.exists(profile, profile == "localhost/*") || variables.derivedAllowedLocalhostFiles.exists(profile, profile == "localhost/*")
+          - name: allowedProfiles


What about users who input localhost/<some profile>? It looks like that's getting dropped?

I don't think so. I added the same in constraint - https://github.com/open-policy-agent/gatekeeper-library/pull/540/files#diff-58803b65572868312ed7364039674b1871f873f15cfd13aa29c8b9adb42e2901 - and added the examples to test Localhost profile. PTAL.

Oops, missed it being added in a lower line

No worries! Good catch! I updated the code.

Signed-off-by: Jaydip Gabani <[email protected]>

maxsmythe

One final nit, then LGTM

maxsmythe · 2024-10-22T01:59:04Z

library/pod-security-policy/seccomp/template.yaml

+              has(variables.hasPodSeccomp) && has(variables.anyObject.spec.securityContext.seccompProfile.type) ? 
+                (variables.anyObject.spec.securityContext.seccompProfile.type == "RuntimeDefault" ? (
+                  variables.allowedProfiles.exists(profile, profile == "runtime/default") ? "runtime/default" : variables.allowedProfiles.exists(profile, profile == "docker/default") ? "docker/default" : "runtime/default") : 
+                variables.anyObject.spec.securityContext.seccompProfile.type == "Unconfined" ? "unconfined" : variables.anyObject.spec.securityContext.seccompProfile.type == "Localhost" ? "localhost/" + variables.anyObject.spec.securityContext.seccompProfile.localhostProfile : "")


When accessing the localhost profile should we be using variables.podLocalHostProfile?

Updated to use the variable.

maxsmythe · 2024-10-22T02:01:57Z

library/pod-security-policy/seccomp/template.yaml

+          - name: localhostWildcardAllowed
+            expression: |
+              variables.inputAllowedProfiles.exists(profile, profile == "localhost/*") || variables.derivedAllowedLocalhostFiles.exists(profile, profile == "localhost/*")
+          - name: allowedProfiles


Oops, missed it being added in a lower line

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani · 2024-10-24T19:53:28Z

@ritazh @maxsmythe Fixed the code, should be good now.

maxsmythe

LGTM

ritazh · 2024-10-25T05:16:04Z

src/pod-security-policy/seccompv2/constraint.tmpl

+    metadata.gatekeeper.sh/version: 1.0.0
+    description: >-
+      Controls the seccomp profile used by containers. Corresponds to the
+      `securityContext.seccompProfile` field.


might be good to explicitly mention:

Suggested change

`securityContext.seccompProfile` field.

`securityContext.seccompProfile` field. Security contexts from the annotation is not considered as Kubernetes no longer reads security contexts from the annotation.

Updated the description.

ritazh

LGTM

one nit

Signed-off-by: Jaydip Gabani <[email protected]>

chore: adding cel for psp-seccomp policy

90223c5

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested a review from a team as a code owner June 3, 2024 21:34

JaydipGabani added 2 commits June 3, 2024 21:42

using anyObject variable

e74ac1d

Signed-off-by: Jaydip Gabani <[email protected]>

moving rego under engine

158fa77

Signed-off-by: Jaydip Gabani <[email protected]>

maxsmythe requested changes Jun 8, 2024

View reviewed changes

JaydipGabani added 3 commits June 18, 2024 20:25

tracking SC to annotations, adding v2 for seccomp with option to not …

2b39fbd

…read from annotations Signed-off-by: Jaydip Gabani <[email protected]>

fixing bugs

04a5b72

Signed-off-by: Jaydip Gabani <[email protected]>

fixing bugs

ccc7044

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested a review from maxsmythe June 18, 2024 23:10

Merge branch 'master' into psp-seccomp

ea66a2a

maxsmythe reviewed Jun 19, 2024

View reviewed changes

removing naming_translations and streamlining name mapping

d5d787c

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested a review from maxsmythe June 20, 2024 19:38

maxsmythe reviewed Jun 25, 2024

View reviewed changes

JaydipGabani requested a review from maxsmythe June 26, 2024 20:49

JaydipGabani added 4 commits June 27, 2024 09:03

Merge branch 'master' into psp-seccomp

0083b09

adding seccompv2 in kustomize

0a990b9

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'psp-seccomp' of github.com:JaydipGabani/gatekeeper-libr…

308fe29

…ary into psp-seccomp

fixing rego bugs

df5aaba

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested review from sozercan and ritazh June 27, 2024 22:34

JaydipGabani added 8 commits July 30, 2024 22:56

adding seccompv2

467cd42

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'master' into psp-seccomp

26570a0

fixing unused variables

dcbfa57

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'psp-seccomp' of github.com:JaydipGabani/gatekeeper-libr…

48f2cd3

…ary into psp-seccomp

adding new line

ec12a95

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'master' into psp-seccomp

4306ad7

Merge branch 'master' into psp-seccomp

dc6ee09

Merge branch 'master' into psp-seccomp

f06bad5

sozercan reviewed Sep 3, 2024

View reviewed changes

sozercan reviewed Sep 4, 2024

View reviewed changes

JaydipGabani added 2 commits September 4, 2024 01:37

updating seccompv2 description

3b5e429

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'psp-seccomp' of github.com:JaydipGabani/gatekeeper-libr…

123c6b5

…ary into psp-seccomp

JaydipGabani requested a review from sozercan September 4, 2024 01:38

sozercan reviewed Sep 4, 2024

View reviewed changes

sozercan approved these changes Sep 4, 2024

View reviewed changes

JaydipGabani added 2 commits September 5, 2024 01:40

removing unused example

1bfcd91

Signed-off-by: Jaydip Gabani <[email protected]>

Merge branch 'master' into psp-seccomp

76e4624

maxsmythe requested changes Sep 13, 2024

View reviewed changes

JaydipGabani added 3 commits September 20, 2024 11:53

Merge branch 'master' into psp-seccomp

0ea1c1c

updating seccompv2 to remove string building for localhost profiles

3ed88dc

Signed-off-by: Jaydip Gabani <[email protected]>

updating rego variable name

fbd2560

Signed-off-by: Jaydip Gabani <[email protected]>

maxsmythe requested changes Sep 27, 2024

View reviewed changes

JaydipGabani added 2 commits October 1, 2024 10:02

Merge branch 'master' into psp-seccomp

d11f81d

adding examples for localhost profile

42c932d

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested a review from maxsmythe October 1, 2024 21:17

maxsmythe requested changes Oct 22, 2024

View reviewed changes

fixing CEL code

0b65a32

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani requested a review from maxsmythe October 24, 2024 19:52

maxsmythe approved these changes Oct 25, 2024

View reviewed changes

ritazh reviewed Oct 25, 2024

View reviewed changes

ritazh approved these changes Oct 25, 2024

View reviewed changes

addressing nit, updating description for seccompv2

14f4ac1

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani merged commit af24955 into open-policy-agent:master Oct 28, 2024
22 checks passed

JaydipGabani deleted the psp-seccomp branch October 28, 2024 19:24

	`securityContext.seccompProfile` field.
	`securityContext.seccompProfile` field. Security contexts from the annotation is not considered as Kubernetes no longer reads security contexts from the annotation.

chore: adding cel for psp-seccomp policy #540

chore: adding cel for psp-seccomp policy #540

Conversation

JaydipGabani commented Jun 3, 2024 • edited Loading

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

sozercan left a comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

JaydipGabani commented Sep 12, 2024

maxsmythe 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

maxsmythe 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

maxsmythe 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

JaydipGabani commented Oct 24, 2024

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

JaydipGabani commented Jun 3, 2024 •

edited

Loading