-
Couldn't load subscription status.
- Fork 47
cdi: inject mount UID/GID mappings if user NS is in use. #288
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
base: main
Are you sure you want to change the base?
Conversation
87acc6c to
19dae60
Compare
19dae60 to
0c5ec3a
Compare
pkg/cdi/oci.go
Outdated
| type extraOCIMountOption func(*spec.Mount) | ||
|
|
||
| // withMountIDMappings adds UID and GID mappings for the given mount. | ||
| func withMountIDMappings(uid, gid []spec.LinuxIDMapping) extraOCIMountOption { |
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.
Does it make sense to have uid and gid settable by separate options.
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.
I am fine with anything. Separate options, combined options, even with simply doing it like this
func (m *Mount) toOCI(uids, gids []spec.LinuxIDMapping) spec.Mount {
om := spec.Mount{
Source: m.HostPath,
Destination: m.ContainerPath,
Options: m.Options,
Type: m.Type,
UIDMappings: uids,
GIDMappings: gids,
}
return om| Destination: "/dest/path/a", | ||
| UIDMappings: []oci.LinuxIDMapping{ | ||
| { | ||
| ContainerID: 0, |
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.
When is ContainerID non-zero?
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.
Do you mean in practice ? In principle, one could set up arbitrary mappings with multiple ranges which is possible, but in practice I think you'll only see a single range and starting (in the container) with 0, because that's what this is primarily used for. But from the OCI Spec point of view, nothing dictates any range or sub-range to start (in the container) at 0.
|
Is something similar required for devices since the uid and gid in the spec are the IDs in the container (https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#devices)? |
At least nothing identical we can do for devices, since they don't have UID/GID mappings, only mounts have them. Also AFAICT, device access control is a bit simpler (when compared with access control for a full bind-mounted subdirectory), just device node permissions and cgroup access rules. |
069d16f to
7832481
Compare
When injecting mounts to a container with user namespaces in use, inject the mounts with UID and GID mappings taken from the OCI Spec. Signed-off-by: Krisztian Litkey <[email protected]>
7832481 to
7116a1d
Compare
|
Device permissions are handled inside the containerd/cri-o based on runAsUser in non-root containers. |
|
Hi! userns KEP author here. I'm not very familiar with the CDI injection process to provide more input. do you have any links I should read? My current opinion without much education on CDI injection is that we should allow drivers to set some mappings, but we should complete with sane defaults (as this PR does) if userns are used and those fields are not set. Wouldn't that solve the "However, that would imply that only dynamically generated CDI Specs would allow injecting non-static mappings" issue you pointed in the description? The k8s end user currently can't select mappings for mounts, as the kubelet will set them to what makes sense in that context. However, if CDI might inject devices or things that are not just bind-mounts (like a device node), setting the UID/GID for it will be critical to work with user-namespaces (uidmappings only work for bind-mounts). I wonder if the CDI drivers have access to the mapping used for the userns, as you basically need to set the UID/GID of the inode to the hostUID of the userns mapping. Sorry I took some time to reply, I was on PTO and just coming back. I'm happy to help in any way that I can with this :) |
When injecting mounts to a container with user namespaces in use, inject the mounts with UID and GID mappings taken from the OCI Spec.
Fixes #287.
@elezar @haircommander Although this simplistic fix works for the reported case, it might be a bit too simplistic (in the long run).
In particular, I wonder if we should eventually provide a way for the one requesting CDI injection (typically a DRA driver) to also provide UID-/GID-mappings for injected mounts, and only let CDI itself pick and inject UID-/GID-mappings on its own, if the necessary preconditions apply but mappings are not provided.
IOW, should we provide a way for a DRA driver itself to pick and inject the mappings ? If we are of the opinion that we should, then I think we have a bit of a problem with APIs and how things are currently wired up with CDI/CRI. The straightforward way to allow a driver to pick mappings would be to simply extend the CDI notion of a mount with UID- and GID-mappings. However, that would imply that only dynamically generated CDI Specs would allow injecting non-static mappings, since there is no way to pass on any additionald/dynamic parameters along a CDI device injection request over CRI (or in the CDI/runtime API).