chore: adding psp-users CEL policy #537

JaydipGabani · 2024-05-30T00:28:28Z

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:

Rego policy nehavior is -

filed in below list is reference to securityContext fields - [ runAsUser, runAsGroup, fsGroup, supplementalGroup ]

Input:

field:
      rule: MustRunAs
      ranges:
        - min: 100
          max: 200

Behavior: if field is missing from object then throw missing violation, else throw violation is field is not in required range

Input:

runAsUser:
      rule: MustRunAsNonRoot

Behavior: if runAsUser and runAsNonRoot both are missing from object then throw missing violation, else throw violation is runAsUser == 0

Input:

field:
      rule: mayRunAs
      ranges:
        - min: 100
          max: 200

Behavior: No missing field violation, but is field is present and violating the range then throw the violation

Input:

field:
  rule: RunAsAny

Behavior: No violations

maxsmythe · 2024-07-03T22:48:31Z

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

+              (variables.containers + variables.initContainers + variables.ephemeralContainers).filter(container,
+                container.image in variables.exemptImageExplicit ||
+                variables.exemptImagePrefixes.exists(exemption, string(container.image).startsWith(exemption)))
+          - name: badContainers


these are not bad containers yet, just containers subject to validation... candidateContainers?

maxsmythe · 2024-07-03T23:00:08Z

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

+          - name: missingRequiredRunAsNonRootContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && (variables.params.runAsUser.rule == "MustRunAsNonRoot") && ((has(container.securityContext) && !(has(container.securityContext.runAsUser) || has(container.securityContext.runAsNonRoot))) ?


We need to check that runAsNonRoot is both defined and equal to true (this is implied in the Rego check, since not will invert both "missing" and "false" to true.

Also, has(container.securityContext.runAsUser) || has(container.securityContext.runAsNonRoot) should be has(container.securityContext.runAsUser) && has(container.securityContext.runAsNonRoot) , since both conditions need to be satisfied.

maxsmythe · 2024-07-03T23:10:08Z

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

+          - name: invalidRunAsUserContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                !(container.name in variables.processedRunAsUserContainers) &&


we should not assume that container names are globally unique, since tools like gator may not perform that kind of validation.

maxsmythe · 2024-07-03T23:14:54Z

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

+                    (
+                      variables.params.runAsUser.rule == "MustRunAs" ?
+                      (
+                        has(container.securityContext) && has(container.securityContext.runAsUser) ? !variables.params.runAsUser.ranges.all(range, container.securityContext.runAsUser >= range.min && container.securityContext.runAsUser <= range.max) :


I think this should be exists() instead of all(), since the constraint is satisfied as long as the user ID lies within at least one of the specified ranges.

maxsmythe · 2024-07-03T23:16:45Z

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

+                      (variables.params.runAsGroup.rule == "MustRunAs" || variables.params.runAsGroup.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.runAsGroup) ? 
+                          !variables.params.runAsGroup.ranges.all(range, container.securityContext.runAsGroup >= range.min && container.securityContext.runAsGroup <= range.max) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:22:08Z

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

+          - name: invalidRunAsFsGroupContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                !(variables.missingRequiredFsGroupContainers.exists(c, c.name == container.name)) &&


as above, let's not rely on names being globally unique

Processing each container type separately.

maxsmythe · 2024-07-03T23:22:47Z

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

+                      (variables.params.fsGroup.rule == "MustRunAs" || variables.params.fsGroup.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.fsGroup) ? 
+                          !variables.params.fsGroup.ranges.all(range, container.securityContext.fsGroup >= range.min && container.securityContext.fsGroup <= range.max) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:22:53Z

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

+                      (
+                        has(container.securityContext) && has(container.securityContext.fsGroup) ? 
+                          !variables.params.fsGroup.ranges.all(range, container.securityContext.fsGroup >= range.min && container.securityContext.fsGroup <= range.max) : 
+                          variables.podRunAsFsGroup == null || !variables.params.fsGroup.ranges.all(range, variables.podRunAsFsGroup >= range.min && variables.podRunAsFsGroup <= range.max)


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:23:56Z

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

+                      (variables.params.supplementalGroups.rule == "MustRunAs" || variables.params.supplementalGroups.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.supplementalGroups) ? 
+                          !variables.params.supplementalGroups.ranges.all(range, container.securityContext.supplementalGroups.all(gp, gp>= range.min && gp <= range.max)) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:24:03Z

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

+                      (
+                        has(container.securityContext) && has(container.securityContext.supplementalGroups) ? 
+                          !variables.params.supplementalGroups.ranges.all(range, container.securityContext.supplementalGroups.all(gp, gp>= range.min && gp <= range.max)) : 
+                          variables.podRunAsSupplementalGroups == null || !variables.params.supplementalGroups.ranges.all(range, variables.podRunAsSupplementalGroups.all(gp, gp >= range.min && gp <= range.max))


Same comment about all() vs. exists()

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

JaydipGabani · 2024-09-12T16:18:56Z

@maxsmythe @ritazh @sozercan PTAL

maxsmythe · 2024-09-13T00:54:20Z

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

+            expression: |
+              !has(variables.params.exemptImages) ? [] : 
+                variables.params.exemptImages.filter(image, !image.endsWith("*"))
+          - name: exemptImages


I think we need map(container, container.image) ?

this still needs fixing

maxsmythe · 2024-09-13T01:01:29Z

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

+            expression: |
+              variables.containers.filter(container, 
+                !(container.image in variables.exemptImages) &&
+                has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && (variables.params.runAsUser.rule == "MustRunAsNonRoot") ?


I don't think you need a trinary operator (?) here

maxsmythe · 2024-09-13T01:05:27Z

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

+                (
+                  (!has(container.securityContext) || (
+                    (!has(container.securityContext.runAsNonRoot) || !container.securityContext.runAsNonRoot) && (!has(container.securityContext.runAsUser) || container.securityContext.runAsUser == 0) 
+                  )) || variables.missingRunAsNonRootGlobal


This reads like it will be true if either there is no global definition for running as non root OR if there is no local definition for running as non root.

This is incorrect. If local is defined but global is not, then there is no violation, as local definition supersedes.

Updated the code to correct this.

maxsmythe · 2024-09-13T01:13:04Z

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

+                (
+                  variables.params.runAsUser.rule == "RunAsAny" ? false :
+                  (
+                    variables.params.runAsUser.rule == "MustRunAsNonRoot" ?


I think the code for this will be much simpler if we:

have a variable for each rule type (MustRunAs, MayRunAs, etc.) that returns the containers violating the rule.

if the rule type is not used, this variable will return an empty list.

This will allow you to:

remove all code duplication (can go back to concatenating all containers again, since you are no longer required to filter based off container name)

Reduce the nesting/branching for each variable.

Because this will significantly change/shorten the code, I'll stop reviewing here.

Thanks for the suggestion. Made an update to CEL code, it should now be much more compact. PTAL.

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

maxsmythe · 2024-09-28T00:01:00Z

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

+              variables.anyObject.kind == "Pod" && has(variables.anyObject.spec.securityContext) && has(variables.anyObject.spec.securityContext.supplementalGroups) ? variables.anyObject.spec.securityContext.supplementalGroups : null
+          - name: podRunAsGroup
+            expression: |
+              variables.anyObject.kind == "Pod" && has(variables.anyObject.spec.securityContext) && has(variables.anyObject.spec.securityContext.runAsGroup) ? variables.anyObject.spec.securityContext.runAsGroup : null


Can we just put the test for kind == Pod into a matchCondition?

I don't think this constraint makes sense for anything other than Pods... we can add the same test to the Rego if necessary.

wdym by "put test for kind == Pod into a matchCondition?

I removed the condition kind == Pod from CEL and rego.

matchCondition is a field in VAP:

https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/#matching-requests-matchconditions

Updated CEL to add matchCondition.

maxsmythe · 2024-09-28T00:07:54Z

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

+              !has(variables.anyObject.securityContext) || ((!has(variables.anyObject.securityContext.runAsNonRoot) || !variables.anyObject.securityContext.runAsNonRoot) && (!has(variables.anyObject.securityContext.runAsUser) || variables.anyObject.securityContext.runAsUser == 0))
+          - name: violatingMustOrMayRunAsUser
+            expression: |
+              has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && variables.params.runAsUser.rule == "MustRunAs" ? variables.nonExemptContainers.filter(container, (!has(container.securityContext) || !has(container.securityContext.runAsUser)) && variables.podRunAsUser == null).map(container, "Container " + container.name + " is attempting to run without a required securityContext/runAsUser") + variables.nonExemptContainers.filter(container, (has(container.securityContext) && has(container.securityContext.runAsUser) ? !variables.params.runAsUser.ranges.exists(range, container.securityContext.runAsUser >= range.min && container.securityContext.runAsUser <= range.max) : variables.podRunAsUser != null && !variables.params.runAsUser.ranges.exists(range, variables.podRunAsUser >= range.min && variables.podRunAsUser <= range.max))).map(container, "Container " + container.name + " is attempting to run as disallowed user. Allowed runAsUser: {ranges: [" + variables.params.runAsUser.ranges.map(range, "{max: " + string(range.max) + ", min: " + string(range.min) + "}").join(", ") + ", rule: " + variables.params.runAsUser.rule + "}") : []


Can we use some string/block formatting to make the various declarations more legible? It's hard to parse the assertions without formatting. This is a general comment for all of these larger code blocks.

TBH I can't review without formatting improvements... the statements bleed together.

Appologies for putting the large code block in one line. Formatted all code large code blocks. Should be easier to parse and read.

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

JaydipGabani · 2024-10-30T00:15:05Z

@maxsmythe PTAL.

JaydipGabani requested a review from a team as a code owner May 30, 2024 00:28

JaydipGabani requested review from sozercan, ritazh and maxsmythe May 30, 2024 00:28

maxsmythe requested changes Jul 3, 2024

View reviewed changes

JaydipGabani added 6 commits August 7, 2024 17:49

adding psp-users CEL policy

975a68a

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

using anyObject

475b94d

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

fixing ci errors

6730ee5

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

fixing exists vs all

2e6de13

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

fixing missingRequiredRunAsNonRoot

3446780

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

processing each container types separately

dbba915

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

JaydipGabani force-pushed the psp-user branch from deee58e to dbba915 Compare August 7, 2024 17:50

JaydipGabani added 2 commits August 19, 2024 11:27

Merge branch 'master' into psp-user

ddc475f

Merge branch 'master' into psp-user

f97c95c

JaydipGabani requested a review from maxsmythe August 21, 2024 00:11

JaydipGabani added 4 commits August 28, 2024 17:42

Merge branch 'master' into psp-user

b89e07b

fixing ci

8366a06

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

fixing error msgs

ff6ad88

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

Merge branch 'master' into psp-user

80dda64

maxsmythe requested changes Sep 13, 2024

View reviewed changes

JaydipGabani added 2 commits September 19, 2024 20:06

simplifying CEL code

093d80a

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

Merge branch 'master' into psp-user

54ca69f

JaydipGabani requested a review from maxsmythe September 19, 2024 20:07

maxsmythe requested changes Sep 28, 2024

View reviewed changes

JaydipGabani added 3 commits October 1, 2024 10:04

Merge branch 'master' into psp-user

d86f00d

formatting CEL code for better readability

7d0f8dc

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

adding exempt image test

df063ff

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

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

adding matchcondition to match pod

e860d00

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

Merge branch 'master' into psp-user

73afca4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adding psp-users CEL policy #537

chore: adding psp-users CEL policy #537

JaydipGabani commented May 30, 2024 •

edited

Loading

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

JaydipGabani Sep 4, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

maxsmythe Jul 3, 2024

JaydipGabani commented Sep 12, 2024

maxsmythe Sep 13, 2024

maxsmythe Sep 27, 2024

JaydipGabani Oct 1, 2024

maxsmythe Sep 13, 2024

maxsmythe Sep 13, 2024

JaydipGabani Sep 19, 2024

maxsmythe Sep 13, 2024

JaydipGabani Sep 19, 2024

maxsmythe Sep 28, 2024

JaydipGabani Oct 1, 2024

maxsmythe Oct 29, 2024

JaydipGabani Oct 30, 2024

maxsmythe Sep 28, 2024

JaydipGabani Oct 1, 2024

maxsmythe Oct 29, 2024

JaydipGabani commented Oct 30, 2024

chore: adding psp-users CEL policy #537

Are you sure you want to change the base?

chore: adding psp-users CEL policy #537

Conversation

JaydipGabani commented May 30, 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

JaydipGabani commented Sep 12, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani commented Oct 30, 2024

JaydipGabani commented May 30, 2024 •

edited

Loading