Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: adding cel for psp-seccomp policy #540

Merged
merged 31 commits into from
Oct 28, 2024

Conversation

JaydipGabani
Copy link
Contributor

@JaydipGabani JaydipGabani commented Jun 3, 2024

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:

@JaydipGabani JaydipGabani requested a review from a team as a code owner June 3, 2024 21:34
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
src/pod-security-policy/seccomp/src.cel Outdated Show resolved Hide resolved
src/pod-security-policy/seccomp/src.cel Outdated Show resolved Hide resolved
src/pod-security-policy/seccomp/src.cel Outdated Show resolved Hide resolved
})
- name: allBadContainerProfiles
expression: |
variables.podAnnotationsProfiles + variables.containerAnnotationsProfiles + variables.podSecurityContextProfiles + variables.containerSecurityContextProfiles + variables.containerProfilesMissing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani JaydipGabani requested a review from maxsmythe June 18, 2024 23:10
src/pod-security-policy/seccomp/src.rego Show resolved Hide resolved
src/pod-security-policy/seccomp/src.rego Show resolved Hide resolved
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JaydipGabani JaydipGabani requested a review from maxsmythe June 20, 2024 19:38
expression: |
(variables.inputAllowedProfiles.filter(profile,
profile != "Localhost").map(profile, profile == "Unconfined" ? "unconfined" : profile)) +
(variables.inputAllowedProfiles.exists(profile, profile == "RuntimeDefault") ? ["runtime/default", "docker/default"] : [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

@JaydipGabani JaydipGabani requested a review from maxsmythe June 26, 2024 20:49
@JaydipGabani JaydipGabani requested review from sozercan and ritazh June 27, 2024 22:34
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's different between non-v2 and v2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated description.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update the description too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated description.

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc change request, otherwise LGTM

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@JaydipGabani
Copy link
Contributor Author

@maxsmythe @ritazh PTAL

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, prefer field comparison to string manipulation

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2 looks good modulo docstring comment.

v1 appears to have a bug WRT localhost

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`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the name. allowedLocalHostFiles is correct.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, missed it being added in a lower line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! Good catch! I updated the code.

@JaydipGabani JaydipGabani requested a review from maxsmythe October 1, 2024 21:17
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final nit, then LGTM

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 : "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use the variable.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, missed it being added in a lower line

Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani
Copy link
Contributor Author

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

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the description.

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

one nit

@JaydipGabani JaydipGabani merged commit af24955 into open-policy-agent:master Oct 28, 2024
22 checks passed
@JaydipGabani JaydipGabani deleted the psp-seccomp branch October 28, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CEL code for PSP Policies in library
4 participants