Check idmap mounts support before creating a container

[rata]

What happened?

Hi!

I wanted to raise awareness on the fact that is not safe to use idmap mounts in high-level container runtimes without checking the OCI runtime features support.

The thing is, old runtimes (withot idmap mounts support) can't fail on unknown fields because the spec requires to ignore them. therefore, old OCI runtimes ignore the idmap mounts configs (the new uidMapping and gidMapping on mounts), which is an issue to use it safely (more on this later). This is sad, but that is how runtimes behave now.

For crun probably this is not an issue here, as you release them in lockstep, right? So CRI-O XX goes with crun YY, and that is always the case. Am I right? In that case it all works, as you can make sure crun YY supports idmap mounts and all is done.

The issue, IIUC, comes with runc support. If CRI-O is used with an old runc that ignore idmap mounts and just start the container, volumes will be mixed up. The UID/GID will be the hostUID/GID of the pod, that is basically garbage, and completely unusable as the hostUID/GID is not guaranteed to be stable over time.

In containerd I'm currently doing the following, to handle this case and give a proper error to users:

• If userns are in use ==> run "runc features" and check for idmap mounts support.
• If idmap mounts supports is there, go ahead and create the container. Otherwise, throw a clear error.
• The container creation might work or not (depends if the host supports idmap mounts for all the fs), but it will never be silently ignored. In the worst case, runc throws an error.

You can check the code I wrote to do this in containerd here.

Exposing info on idmap mounts support is something I did in the runtime-spec due to this issue (not being able to use idmap mounts safely from a high-level container runtime). You can see the PR here: opencontainers/runtime-spec#1219

I've also added support for that in runc and crun:

• features: Support mountExtensions containers/crun#1279
• features: Expose idmap support opencontainers/runc#3993

Although these changes are not yet included on a release.

Let me know if something is not clear :)

What did you expect to happen?

---

How can we reproduce it (as minimally and precisely as possible)?

---

Anything else we need to know?

No response

CRI-O and Kubernetes version

$ crio --version
# paste output here

$ kubectl version --output=json
# paste output here

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

Additional environment details (AWS, VirtualBox, physical, etc.)

[haircommander]

Thanks for making us aware @rata ! Do you have any bandwidth/desire to add a $runtime features check? For whoever does it: we have a set of packages in internal/config/ that do static checks like this. We could add a new one: internal/config/ociruntime where we go through all of the configured runtimes and initialize their support of various features (wasi could be another good one cc @sohankunkerkar )

For crun probably this is not an issue here, as you release them in lockstep, right?

nope! we release independently :)

[AdityaVardhanSingh]

Hey, can I work on this?

[rata]

@haircommander awesome!
@AdityaVardhanSingh sure, you can do it! Let me know if you can't, I can try to find some time to work on this :)

[MeenuyD]

Hello, @rata I would like to work on this issue can you guide me on how to get started to resolve?

[rata]

@MeenuyD it seems @AdityaVardhanSingh will work on this already

[MeenuyD]

ok

[hasan4791]

/assign @AdityaVardhanSingh

[AdityaVardhanSingh]

Hey, @rata I think I got the issue but what are we expecting here to be done?

[rata]

@AdityaVardhanSingh What I would expect is something similar to what I mentioned we are doing in containerd:

1. If idmap mounts are in use (i.e. if the field uidMappings or gidMappings for any mount is not nil), then run the command "runc features" or "crun features". I'd NOT add this extra burden of running the "features" command for all containers, but ONLY for containers that are using idmap mounts.
2. Check in the command output that the Linux MountExtension "idmap" is enabled
3. If it is enabled, then start the container. If it is not enabled, return an error message saying the OCI runtime is old and doesn't support idmap mounts.

Something like this is currently done in this PR I linked in the issue (see the pkg/process/init.go, the whole file if you need more context): https://github.com/containerd/containerd/pull/8287/files#diff-17085223b7307c8c0ed7f2cdcfb904efa5a184a71b186a83961d7feef9e3556fR197-R227.

To test this with runc and crun, you will have to compile them from git, in their latest commit. This functionality was merged in the PR I mentioned in the original bug report here and are not yet part of a release.

[haircommander]

I actually would suggest an alternative approach. Instead of re-checking whether runc/crun support the feature each time a container with idmap is created, I would use a similar mechanism to the internal/config/node/ package. When the config is generated in pkg/config/config.go we have a list of the runtime handlers. Once that's finalized, you could loop through the handlers and call $runtime features on them, and save whether they support idmap as part of the handler object. Then, in container creation, you could just query that field

[rata]

@haircommander that seems way better! :)

[AdityaVardhanSingh]

@rata can we connect?? I have somethings to clarify.

[rata]

@AdityaVardhanSingh what do you mean? You can post here. @haircommander will probably have better suggestions, he is a maintainer and I've never checkout the cri-o repo :-)

[haircommander]

You can reach out to me on k8s slack if you'd like to chat elsewhere @AdityaVardhanSingh :)

[AdityaVardhanSingh]

That would be really helpful.

[rata]

@AdityaVardhanSingh any progress on this? If this is not ready by next week, we might need to fix this ourselves to make a release, sorry. There will be plenty of other issues to help anyways :)

[AdityaVardhanSingh]

@rata I am working on this but many things came up which was new to me so just trying to do it in the best possible way. Just wanted to give my best, if you can give me a couple of days more if that is ok?

[rata]

We have blog posts scheduled for next week that depend on this, so I'm sorry, but I don't think we can :(

btw, crun 1.9 is out now, it ships with the PR I linked in the description, to expose idmap mounts support.

[rata]

This is fixed in main now, backported to 1.28 and will be included in the next path release.

Thanks all for your help!

@AdityaVardhanSingh I hope you find another good first issue to collaborate. Thanks a lot anyways! :)