-
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
apis: drop check for volumes with user namespaces #118691
apis: drop check for volumes with user namespaces #118691
Conversation
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. |
4a24f97
to
2de3a4c
Compare
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.
@giuseppe LGTM, thanks! Left one simple comment about updating a comment in the tests, but is super minor :)
The second phase of user namespaces support was related to supporting only stateless pods. Since the changes were accepted for the KEP, now the scope is extended to support stateful pods as well. Remove the check that blocks creating PODs with volumes when using user namespaces. Signed-off-by: Giuseppe Scrivano <[email protected]>
2de3a4c
to
af9687f
Compare
now it is called UserNamespacesSupport since all kind of volumes are supported. Signed-off-by: Giuseppe Scrivano <[email protected]>
af9687f
to
531d38e
Compare
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.
LGTM, thanks!
@thockin friendly ping? |
Things not assigned to me fall into lots of cracks. :) |
I had to go back to the KEP, and I have a question on the idmapped approach. I think one of the primary questions we need to ask is "what happens in the case of a container escape?" Userns means that (probably) such an escape results in code execution as the host UID, so even if running as 0 in container, it's non-zero in host. Yay. What about volumes? IIUC (please confirm) the host-side UID/GID on files will be in the 0-64K range, which is never used for userns, right? So if I escape my container, I can't read those files any more, or anyone else's files (unless they allow "other" read). Is that correct? Is there anyothing we could do to limit even those "other" reads in case of an escape? |
Correct :)
Not with userns. We need other security layers to cover that, probably apparmor can help. I want to explore landlock too (it can have several advantages, and I want to see if we can enable some things transparently in OCI runtimes). But those are completely orthogonal things. |
/assign thockin |
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.
Thanks!
/lgtm
/approve
LGTM label has been added. Git tree hash: 572c8f026b4c038e91c8de7d9fb49d211733d4a1
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rata, thockin 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 |
The second phase of user namespaces support was related to supporting only stateless pods. Since the changes were accepted for the KEP, now the scope is extended to support stateful pods as well. Remove the check that blocks creating PODs with volumes when using user namespaces.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
KEP: kubernetes/enhancements#127