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

(u|g)idMappings should not exist when joining an existing user ns #4122

Closed
lifubang opened this issue Nov 19, 2023 · 9 comments · Fixed by #4124
Closed

(u|g)idMappings should not exist when joining an existing user ns #4122

lifubang opened this issue Nov 19, 2023 · 9 comments · Fixed by #4124
Labels
Milestone

Comments

@lifubang
Copy link
Member

Description

When reviewing #3985, we found an error when joining an existing user namespace.
Ref: #3985 (comment)

Steps to reproduce the issue

  1. start a container test with user mapping, for example:
    .linux.namespaces += [{"type": "user"}]
    .linux.uidMappings = [{"hostID": 100000, "containerID": 0, "size": 65536}]
    .linux.gidMappings = [{"hostID": 100000, "containerID": 0, "size": 65536}]

  2. get the container init process's pid
    runc ps test
    for example the pid is 14821

  3. start an new container test1 with pid 14821's user namespace, for example:
    .linux.namespaces += [{"type": "user", "path": "/proc/14821/ns/user"}]

Describe the results you received and expected

Received:
ERRO[0000] runc run failed: User namespaces enabled, but no uid mappings found.

Expected:
The container should be started successfully.

What version of runc are you using?

all

Host OS information

NAME="Ubuntu"
VERSION="20.04.6 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.6 LTS"
VERSION_ID="20.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=focal
UBUNTU_CODENAME=focal

Host kernel information

Linux codespaces-21ad96 6.2.0-1016-azure #16~22.04.1-Ubuntu SMP Tue Oct 10 17:11:51 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

@lifubang lifubang added this to the 1.1.11 milestone Nov 19, 2023
@lifubang lifubang added the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Nov 19, 2023
@lifubang lifubang changed the title ERRO[0000] runc run failed: User namespaces enabled, but no uid mappings found. (u|g)idMappings should not exist when joining an existing user ns Nov 19, 2023
@cyphar
Copy link
Member

cyphar commented Nov 20, 2023

There's another problem -- we don't check that uidMappings/gidMappings are nil if you've used a path. You should only be able to set one of them. The time namespace validator has a similar issue.

I'll make a PR for this.

EDIT: Ah, this isn't an issue in the validator. The problem is that HostUID doesn't work if you have a userns path. That is extremely unfortunate... There is an obvious solution (join the userns and look at /proc/self/[ug]id_map but it's incredibly ugly -- and it doesn't help that Go doesn't even let you do this with with os.CreateProcess (you can only create namespaces, not join them).

@cyphar
Copy link
Member

cyphar commented Nov 21, 2023

I have a patch for this, but in writing tests I found two separate issues:

  1. Starting a container using the mount namespace of another container doesn't work for a few reasons. Some of them can be worked around (obviously you need to remove all mounts entries, as well as stuff like linux.maskedPaths and linux.readonlyPaths) but the core issue is that we unconditionally pivot-root into the rootfs and you cannot have an empty root section. So, if the target mount namespace has a different root, you will not be able to use it as the namespace to join for a new container.
  2. It seems that userns and timens are incompatible. I'm not sure why our rootless integration tests work, but if you create a proper userns+timens, nsexec errors out when trying to write to /proc/self/timens_offsets with -EACCES (specifically the open fails, not the write so there's some permission issue with userns).

I'm still debugging 2. For 1, I don't really know how much there is to do -- maybe it's fair to say that we just don't support that use-case? @kolyshkin?

@lifubang
Copy link
Member Author

I think we should backport the PR to release-1.1.

@lifubang lifubang reopened this Dec 13, 2023
@lifubang
Copy link
Member Author

@cyphar In the commit 09822c3, you fix the validation issues about user namespace and time namespace, but I think time namespace has not been supported in release-1.1. What should we do when we are doing cherry-pick this commit to release-1.1? Also backport #3876 or remove the code about time namespace when doing cherry-pick?

@cyphar
Copy link
Member

cyphar commented Dec 13, 2023

@lifubang We would only need to backport the userns changes. That being said, I'm not sure whether we plan to have a 1.1.11 release. If we do, we should backport the fix, so I tagged it as such.

@kolyshkin
Copy link
Contributor

I'm not sure whether we plan to have a 1.1.11 release.

Feels like we have to make 1.1.11.

@cyphar
Copy link
Member

cyphar commented Dec 13, 2023

I'll do the backport then. Can we make it the last 1.1.z release? 😸

@cyphar
Copy link
Member

cyphar commented Dec 14, 2023

We also need #4134 for the backport, because the original patch broke containerd and crio.

@kolyshkin kolyshkin removed the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Dec 14, 2023
@lifubang
Copy link
Member Author

#4144 has been merged, close this and prepare to release 1.1.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants