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

linux: support options to idmap #874

Merged
merged 6 commits into from
Feb 17, 2022
Merged

Conversation

giuseppe
Copy link
Member

allow to specify what mapping must be used for idmapped mounts.

The mapping can be specified after the idmap option like:
idmap=uids=0-1-10;gids=0-100-10.

When uids and gids are specified, then a new user namespace is
created and used for the bind mount.

Closes: #873

Signed-off-by: Giuseppe Scrivano [email protected]

giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 14, 2022
[NO NEW TESTS NEEDED] the feature is still being worked on crun: containers/crun#874

Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 14, 2022
[NO NEW TESTS NEEDED] the feature is still being worked in crun:

containers/crun#874

also needs: containers/common#927

Signed-off-by: Giuseppe Scrivano <[email protected]>
@lukts30
Copy link

lukts30 commented Feb 14, 2022

The current implementation is a bit problematic since it would be difficult to be used together with --userns=auto.
From my testing with this PR:

# Files created in the container in /mnt/ as namespaced root will be owned by root on the host. 
sudo podman run --rm -it --uidmap 0:30000:7000 --gidmap 0:30000:7000 --user 0:0 --mount=type=bind,src=$(pwd)/mnt2/,dst=/mnt,idmap alpine /bin/sh
# Files created in the container in /mnt/ as namespaced root will be owned by root on the host. (equivalent to omitting uids=.../gids=...)
sudo podman run --rm -it --uidmap 0:30000:7000 --gidmap 0:30000:7000 --user 0:0 --mount=type=bind,src=$(pwd)/mnt2/,dst=/mnt,idmap="uids=0-30000-7000;gids=0-30000-7000" alpine /bin/sh


# Files created in the container in /mnt/ as namespaced root will be owned by uid 1000 on the host. 
sudo podman run --rm -it --uidmap 0:30000:7000 --gidmap 0:30000:7000 --user 0:0 --mount=type=bind,src=$(pwd)/mnt2/,dst=/mnt,idmap="uids=1000-30000-7000;gids=1000-30000-7000" alpine /bin/sh

This behavior is more from the perspective of the underlying filesystem (on_disk_uid-host_uid-amount) than from the perspective of the container.
It is problematic since knowledge of the real/host uid (30000) of the namespaced uid 0 is required and practically it would be incompatible with --userns=auto if podman would pass the values unprocessed to crun.


The behavior that I would prefer is the same as --uidmap (container_uid-host_uid-amount) replacing host_uid with on_disk_uid (container_uid-on_disk_uid-amount).

# Files created in the container in /mnt/ as namespaced root will be owned by uid 1000 on the host. 
sudo podman run --rm -it --uidmap 0:30000:7000 --gidmap 0:30000:7000 --user 0:0 --mount=type=bind,src=$(pwd)/mnt2/,dst=/mnt,idmap="uids=0-1000-7000;gids=0-1000-7000" alpine /bin/sh

container user namespace [0,6999] --> Filesystem [1000,7999]

or alternatively described as:
Filesystem [1000,7999] --> initial user namespace/host [30000,36999]
/proc/[pid]/uid_map: "1000 30000 7000"

With this behavior omitting uids=.../gids=... should be equivalent to:

sudo podman run --rm -it --uidmap 0:30000:7000 --gidmap 0:30000:7000 --user 0:0 --mount=type=bind,src=$(pwd)/mnt2/,dst=/mnt,idmap="uids=0-0-7000;gids=0-0-7000" alpine /bin/sh

@giuseppe giuseppe marked this pull request as draft February 15, 2022 08:05
@giuseppe
Copy link
Member Author

so you'd like that the specified uids= and gids= are relative to the container user namespace instead of using them directly.

This should be doable but adds some complexity, we need to deal with non-contiguous ranges, as well as handle an existing user namespace (e.g., podman run --userns ns:/proc/PID/ns/user) and read its configuration.

@giuseppe giuseppe marked this pull request as ready for review February 15, 2022 09:10
src/libcrun/linux.c Outdated Show resolved Hide resolved
@lukts30
Copy link

lukts30 commented Feb 15, 2022

so you'd like that the specified uids= and gids= are relative to the container user namespace instead of using them directly.

This should be doable but adds some complexity, we need to deal with non-contiguous ranges, as well as handle an existing user namespace

If a container user namespace is non-contiguous and unmapped ids are included in the idmap option then that should just return an error.

0 165536 7000
10000 200000 7000

(container_uid-on_disk_uid-amount)
idmap=0-1000-7000 ok
idmap=0-1000-7001 error

But a user namespace that is contiguous but its mapping is not contiguous should still just work.

0 165536 7000
7000 200000 7000

(container_uid-on_disk_uid-amount)
idmap=0-1000-14000 ok

I have tested the following with the unshare/newgidmap/nsenter commands (uses nested user namespaces).

  1. Create child process (C0). child:

    • setns(userns_container,CLONE_NEWUSER)
    • wait on parent
  2. parent:

    • capture /proc/c0_pid/uid_map
    • capture /proc/c0_pid/gid_map
  3. C0 Exit

  4. Create child process (C1) with CLONE_NEWUSER. parent:

    • write captured mapping to /proc/c1_pid/uid_map
    • write captured mapping to /proc/c1_pid/gid_map
    • (this creates a copy of userns_container as it is seen from the initial user namespace. making a copy since the user namespace created in step 5 should not be owned by userns_container. unsure if this is really needed)
  5. Create child process (C2) with CLONE_NEWUSER inside C1 Process. C1:

    • run a helper programm that reads /proc/self/uid_map and prints the required mapping for C2. [1]
    • if the user namespace is contiguous this is equivalent to:
      • "on_disk_uid container_uid amount" > /proc/c2_pid/gid_map
      • "on_disk_uid container_uid amount" > /proc/c2_pid/gid_map
  6. parent: sys_mount_setattr MOUNT_ATTR_IDMAP with the user namespace that was created in step 5.


Examples unshare/newgidmap/nsenter:

@lgtm-com
Copy link

lgtm-com bot commented Feb 16, 2022

This pull request introduces 3 alerts when merging 9a1ab46 into d1acf9d - view on LGTM.com

new alerts:

  • 3 for Wrong type of arguments to formatting function

@giuseppe
Copy link
Member Author

I've added a new flag that can help you with your use case.

If you prepend + to a uids or gids mapping, then the mapping is considered relative to the container user namespace (as long as the mappings are specified in the OCI configuration). It doesn't split ranges, so it is the caller responsibility to do so.

$ sudo touch /foo && sudo chown 1:1 /foo
$ $ sudo podman run --mount "type=bind,src=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/foo,dst=/foo,idmap=uids=+1--1-100;gids=+0-0-100" --userns=auto --rm fedora stat -c '%u:%g' /foo
0:1

Would this work for you?

@lgtm-com
Copy link

lgtm-com bot commented Feb 16, 2022

This pull request introduces 3 alerts when merging 7ff74bf into d1acf9d - view on LGTM.com

new alerts:

  • 3 for Wrong type of arguments to formatting function

@giuseppe giuseppe force-pushed the idmap-options branch 2 times, most recently from b7fe7e1 to 7b273a4 Compare February 16, 2022 10:19
giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 16, 2022
[NO NEW TESTS NEEDED] the feature is still being worked in crun:

containers/crun#874

also needs: containers/common#927

Signed-off-by: Giuseppe Scrivano <[email protected]>
crun.1 Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the idmap-options branch 3 times, most recently from 56ceec8 to 777d6f7 Compare February 16, 2022 15:59
crun.1 Outdated Show resolved Hide resolved
crun.1 Outdated Show resolved Hide resolved
src/libcrun/linux.c Outdated Show resolved Hide resolved
@lukts30
Copy link

lukts30 commented Feb 16, 2022

LGTM

allow to specify what mapping must be used for idmapped mounts.

The mapping can be specified after the `idmap` option like:
`idmap=uids=0-1-10;gids=0-100-10`.

When `uids` and `gids` are specified, then a new user namespace is
created and used for the bind mount.

Closes: containers#873

Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

finally centos:8 tests are green.

I think this is ready to merge

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

Just waiting for tests to pass.

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

Successfully merging this pull request may close these issues.

Allow configuration of the idmap option
3 participants