-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Forbid empty AppArmor localhost profile #108143
Conversation
@tallclair: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Added a pkg/security/apparmor/OWNERS file (owned by sig-node), so AppArmor changes don't require top-level approval. |
fwiw, ValidateProfileFormat is called from ValidateAppArmorPodAnnotations and ValidatePodSecurityPolicySpecificAnnotations in API validation, so at least some parts of this package are REST API-facing what can we do to avoid accidentally making changes to those functions in an uncontrolled way that doesn't consider the ratcheting required by API validation changes? |
I moved the validation method to the API validation package. /assign @liggitt |
/test pull-kubernetes-unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit, otherwise LGTM
/test pull-kubernetes-unit |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, saschagrunert, tallclair The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
#97966 removed the requirement that AppArmor profiles be pre-loaded, but in doing so opened up
localhost/
as a valid profile. Whenlocalhost/
gets passed to the CRI runtime, it strips out thelocalhost/
prefix before passing the empty string to runc, which then treats it asunconfined
. The end result is thatlocalhost/
is equivalent tounconfined
, and could be used to evade admission policies that explicitly forbid the use of unconfined.Also adds a
pkg/security/apparmor/OWNERS
file (owned by sig-node), so AppArmor changes don't require top-level approval.Special notes for your reviewer:
Note that this validation would ideally be done as part of API validation, but because of #64841 the Kubelet just performs the check on pod admission (to the node).
Does this PR introduce a user-facing change?
#97966 was only pushed out in an alpha release, so no user-facing changes are introduced here.
/sig node
/assign @saschagrunert
/cc @liggitt