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

feat: Update apparmor: add CEL, support securityContext #533

Merged
merged 8 commits into from
Sep 4, 2024

Conversation

maxsmythe
Copy link
Contributor

@maxsmythe maxsmythe commented May 25, 2024

For context... here is how the applicable apparmor profile is derived:

https://github.com/kubernetes/kubernetes/blob/master/pkg/security/apparmor/helpers.go#L55-L76

@maxsmythe
Copy link
Contributor Author

Looks like there may be a bug in the Rego side... I'll take a look in a bit

@maxsmythe
Copy link
Contributor Author

Rego fixed ... pod-level security context retrieval did not take into account that it was a child of spec

@JaydipGabani
Copy link
Contributor

@maxsmythe do we want to update the policy version to 1.1.0 since we are updating rego and adding cel as well?

@maxsmythe
Copy link
Contributor Author

@JaydipGabani SGTM. Is there anything special we need to do for that? Or just update the annotation?

@JaydipGabani
Copy link
Contributor

@JaydipGabani SGTM. Is there anything special we need to do for that? Or just update the annotation?

@maxsmythe Updating annotations and running make generate-all should work. It will generate files for artifact-hub as well.

@maxsmythe
Copy link
Contributor Author

bumped minor version

@JaydipGabani
Copy link
Contributor

Fixes #541

Copy link
Contributor

@JaydipGabani JaydipGabani left a comment

Choose a reason for hiding this comment

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

LGTM on CEL

@maxsmythe
Copy link
Contributor Author

I'm guessing the k8s tests are failing because securityContext.appArmorProfile only exists as of k8s 1.30?

@JaydipGabani
Copy link
Contributor

@maxsmythe we are running tests with kind: 0.17.0 and that seems to have k8s 1.25.3. So I agree, I think that would be the reason as well.

variables.podAppArmor.type == "Localhost" ? "localhost/" + variables.podAppArmor.localhostProfile : ""
- name: appArmorByContainer
expression: |
variables.allContainers.map(container, [container.name,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is container.name unique across containers/ephemeralContainers/initContainers ? We should probably track these separately per-field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, container.name is unique across all containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

I think I still want to change this, as gator will not have the K8s API server enforcing that rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maxsmythe I am not sure how this change makes any difference as the cel code it-self doesnt check for unique names across all types of containers. can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, I think I got it.

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

@sozercan sozercan merged commit 598df74 into open-policy-agent:master Sep 4, 2024
22 checks passed
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.

4 participants