Mapping improvements to add additional groups

[zeehio]

This topic has been discussed at length at #18333, with
@giuseppe, @Luap99 and with the feedback from @rhatdan. The requirements were defined there and
this aims to be the implementation.

Status

• Implementation
• Unit testing
• Documentation

This is my first go contribution and my first podman contribution. I would appreciate help in terms on where to define unit tests and an example on how to define them, as well as pointers to documentation sources so I can improve docs.

I know further work is needed, but I'd rather have some early feedback if possible (and help).

Closes #18333

Motivation

These commits aim to make --uidmap and --gidmap easier to use, especially in rootless podman setups.

So to map our additional GID 1001, that has been subordinated to us, into GID 100000 in the container, instead of doing:

podman unshare cat /proc/self/gid_map
# Find that GID 1001 was subordinated to intermediate GID 1
# Map 1001->1 to 100000
podman run \
  --rm \
  --uidmap "0:0:65535" \
  --gidmap "0:0:1" \
  --gidmap "100000:1:1" \
  --gidmap "1:2:65534" \
  alpine id

We can just:

podman run \
  --rm \
  --gidmap "+100000:@1001:1" \
  alpine

A real use case can be found at: https://rocker-project.org/use/rootless.html#ask-the-system-administrator-to-subordinate-the-group. I hope to eventually simplify the explanation in that page once this is merged and released.

Long explanation follows

(I will focus here on the --gidmap option, although the same applies for --uidmap.)

In rootless podman, the user namespace mapping happens in two steps, through an intermediate mapping.

See https://docs.podman.io/en/latest/markdown/podman-run.1.html#uidmap-container-uid-from-uid-amount
for further detail, here is a summary:

First the user GID is mapped to 0 (root), and all subordinate GIDs (defined at /etc/subgid, and
usually >100000) are mapped starting at 1.

If we want to change it further, we can use the --gidmap option, to map that intermediate mapping
to the final mapping that will be seen by the container.

As an example, let's say we have as main GID the group 1000, and we also belong to the additional GID 2000,
that we want to make accessible inside the container.

We first ask the sysadmin to subordinate the group to us, by adding "$user:2000:1" to /etc/subgid.

Then we need to use --gidmap to specify that we want to map GID 2000 into some GID inside the container.

And here is the first trouble:

Since the --gidmap option operates on the intermediate mapping, we first need to figure out where has
podman placed our GID 2000 in that intermediate mapping using:

podman unshare cat /proc/self/gid_map

Then, we may see that GID 2000 was mapped intermediate GID 5. So our --gidmap option should include:

--gidmap 20000:5:1

This intermediate mapping may change in the future if further groups are subordinated to us (or we stop
having its subordination), so we are forced to verify the mapping with
podman unshare cat /proc/self/gid_map every time, and parse it if we want to script it.

The first usability improvement we agreed on #18333 is to be able to use:

--gidmap 20000:@2000:1

so podman does this lookup in the parent user namespace for us.

But this is only part of the problem. We must specify a full gidmap and not only what we want:

--gidmap 0:0:5 --gidmap 5:6:15000 --gidmap 20000:5:1

This is becoming complicated. We had to break the gidmap at 5, because the intermediate 5 had to
be mapped to another value (20000), and then we had to keep mapping all other subordinate ids... up to
close to the maximum number of subordinate ids that we have (or some reasonable value). This is hard
to explain to someone who does not understand how the mappings work internally.

The second usability improvement is to be able to use:

--gidmap "+20000:@2000:1"

where the plus sign (+) states that we want to start with an identity mapping, and break it where
necessary so this mapping gets included.

One final improvement related to this is the following:

By default, when podman gets a --gidmap argument but not a --uidmap argument, it copies the mapping.
With the new syntax this copying does not make sense. Having a GID subordinated to us does not imply
that the same UID will be subordinated as well. This means, that when we wanted to use:

--gidmap 0:0:5 --gidmap 5:6:15000 --gidmap 20000:5:1

We also had to include:

--gidmap 0:0:5 --gidmap 5:6:15000 --gidmap 20000:5:1 --uidmap 0:0:65000

making everything even harder to understand without proper context.

In this series of patches, when a "break and insert" gidmap is given (using the described + syntax)
without a --uidmap, we assume that we want the "identity mapping" as --uidmap (0:0:65000).

To preserve backwards compatibility, this different default mapping is only used when the + syntax
is used, so users who rely on the previous behaviour don't suffer any changes.

Does this PR introduce a user-facing change?

Extended the syntax of `--uidmap` and `--gidmap` so it accepts flags and becomes more expressive, especially in rootless containers. The 
syntax has been extended to be able to lookup the parent user namespace, and to extend default
mappings. Now mapping an additional subordinated group (e.g. GID 2000) can be done with
just `--gidmap +g102000:@2000:1`. See
https://github.com/containers/podman/blob/main/docs/tutorials/rootless_tutorial.md#giving-access-to-additional-groups 
for details  (#18333, #18713, @zeehio)

[rhatdan]

@giuseppe PTAL

[zeehio]

My unit tests are not working due to easy to fix issues and I don't know how to run the tests locally.

If anyone would be so kind to help me with running the unit tests I guess I can manage. Otherwise I am investing a lot of time on learning those basics.

I can however build podman and check that the code works.

[Luap99]

Unit tests for a single package can be run with go test ./pkg/util, if you want to run a specific test only use go test -run TestBreakInsert ./pkg/util/

[zeehio]

Thanks @Luap99 for the tip on how to run some tests. It helped me to get them running.

I have forced pushed and rebased some times, I hope those force pushes are not noisy.

While reviewing my code I came across with an unrelated trivial bug, which I fixed in this same branch. See 46a7bdc. I believe regardless of the outcome of this pull request that commit should be merged.

[giuseppe]

thanks for working on it!

One question, what is the expected output when you run something like podman run --rm --uidmap=0:2000:100000 --uidmap +1:100:1 alpine cat /proc/self/uid_map as root?

With the current version I get:

# podman run --rm --uidmap=0:2000:100000  --uidmap +1:100:1 alpine cat /proc/self/uid_map
         0       2000     100000
    100000          0        100
    100100        101       1899
    101999     102000 2147483647
2147585646 2147585647 2147381648

Could you please squash the commits? It is fine if you want to split different functionalities in different commits, but commits that fix issues introduced by previous patches in the same PR should just be squashed together.

[zeehio]

thanks for working on it!

It's my itch to scratch, so thanks to you for reviewing!

One question, what is the expected output when you run something like podman run --rm --uidmap=0:2000:100000 --uidmap +1:100:1 alpine cat /proc/self/uid_map as root?

Thanks for the question. I may need a bit more work to extend testing and provide well-defined behaviour in all cases. Without considering what is the current behaviour I would expect the following behaviour to happen:

• --uidmap=0:2000:100000 The host id range [2000-101999] is mapped to the container range [0-99999].
• --uidmap +1:100:1 Change the previous range, so the host id 100 becomes container id 1.
• Do not map any other uidmap.

The uidmap that would fulfill this specification is something like:

container host size
0   2000 1
1  100 1
2  2002 99998

With the current version I get:

# podman run --rm --uidmap=0:2000:100000  --uidmap +1:100:1 alpine cat /proc/self/uid_map
         0       2000     100000
    100000          0        100
    100100        101       1899
    101999     102000 2147483647
2147585646 2147585647 2147381648

Clearly my implementation is not working as it should. I will need more time to fix it and to add more tests, to ensure both root and rootless scenarios are covered as expected. I may need a bit of time.

---

To better define the specifications for this improvement, I have a question to you: With that same command you provided, what gidmap would you expect?

I ask because the default behaviour when only one of the two mappings are given (like you did, only providing the uidmap) is to copy the uidmap to the gidmap (or viceversa). I don't think that makes much sense, because user ids and group ids are different things. I would suggest to act as follows "if the user uses the + syntax then podman treats user and group mappings independently", which I believe makes more sense and still preserves backwards compatibility.

With those specifications, something like:

# podman run --rm --uidmap=0:2000:100000  --uidmap +1:100:1 alpine cat /proc/self/gid_map

Should return the same as:

# podman run --rm alpine cat /proc/self/gid_map
         0          0 4294967295

Would you agree to that behaviour, or would you prefer that I copy the resulting uidmap over the gidmap?

---

Could you please squash the commits? It is fine if you want to split different functionalities in different commits, but commits that fix issues introduced by previous patches in the same PR should just be squashed together.

Done. Note that there is one trivial unrelated bugfix that I have left aside on an independent commit. Anyway I still have to work on this issue. I will change the status to work in progress to reflect that.

[giuseppe]

To better define the specifications for this improvement, I have a question to you: With that same command you provided, what gidmap would you expect?

I ask because the default behaviour when only one of the two mappings are given (like you did, only providing the uidmap) is to copy the uidmap to the gidmap (or viceversa). I don't think that makes much sense, because user ids and group ids are different things. I would suggest to act as follows "if the user uses the + syntax then podman treats user and group mappings independently", which I believe makes more sense and still preserves backwards compatibility.

I'd still expect gidmap to be copied from uidmap if it is not overridden. Otherwise we will introduce a different behaviour for no apparent reason (the two behaviors have both pro and cons).

Do you disagree?

[zeehio]

To better define the specifications for this improvement, I have a question to you: With that same command you provided, what gidmap would you expect?
I ask because the default behaviour when only one of the two mappings are given (like you did, only providing the uidmap) is to copy the uidmap to the gidmap (or viceversa). I don't think that makes much sense, because user ids and group ids are different things. I would suggest to act as follows "if the user uses the + syntax then podman treats user and group mappings independently", which I believe makes more sense and still preserves backwards compatibility.

I'd still expect gidmap to be copied from uidmap if it is not overridden. Otherwise we will introduce a different behaviour for no apparent reason (the two behaviors have both pro and cons).

Do you disagree?

To be honest I haven't found use cases where copying the mapping is an advantage. My group IDs are not equal to my user IDs so I don't see a reason for mapping both when I type one. On the contrary:

My typical use case is "just leave things default but map this additional group". For that, the specification --gidmap "+100000:@2000:1" is explicit and quite straightforward (map group 2000 as group 100000).

I find it confusing to create errors in the uid mapping just by changing the gidmap. I just wanted to map my subordinated group 2000, and apparently podman wants to map user 2000 as well... I fail to understand why that happens.

I understand backwards compatibility should be preserved, but since I am introducing an extension of the [ug]idmap syntax I think this is an opportunity to change this behaviour (with + syntax). Do you want to "extend the uidmap"? Then use "--uidmap +..." And if you want to extend groups then go for it and use --gidmap as well.

If I keep the current default behaviour, then how would you specify "map group 2000 as group 100000"? I guess it should be something like:

--gidmap "+100000:@2000:1" --uidmap 0:0:60000?

But just try to explain why is it that way to a beginner... Why they would need to provide a uidmap when they want to map a group? Where does the 60000 come from? Why the "0:0"? In my opinion it's a bad default.

Thinking about this uidmap syntax I would even consider to go beyond this extension and (1) accept "@groupName" instead of "@groupId", and (2) assume ":1" if no map size is given. Then the option could become --gidmap "+100000:@shareddata", even more explicit.

It's your project and I am open to implement whatever you agree to, but if it was up to me I would try to avoid crossing uidmaps with gidmaps as much as backwards compatibility allows me to.

[giuseppe]

My typical use case is "just leave things default but map this additional group". For that, the specification --gidmap "+100000:@2000:1" is explicit and quite straightforward (map group 2000 as group 100000).

I find it confusing to create errors in the uid mapping just by changing the gidmap. I just wanted to map my subordinated group 2000, and apparently podman wants to map user 2000 as well... I fail to understand why that happens.

I understand backwards compatibility should be preserved, but since I am introducing an extension of the [ug]idmap syntax I think this is an opportunity to change this behaviour (with + syntax). Do you want to "extend the uidmap"? Then use "--uidmap +..." And if you want to extend groups then go for it and use --gidmap as well.

If I keep the current default behaviour, then how would you specify "map group 2000 as group 100000"? I guess it should be something like:

--gidmap "+100000:@2000:1" --uidmap 0:0:60000?

But just try to explain why is it that way to a beginner... Why they would need to provide a uidmap when they want to map a group? Where does the 60000 come from? Why the "0:0"? In my opinion it's a bad default.

Thinking about this uidmap syntax I would even consider to go beyond this extension and (1) accept "@groupName" instead of "@groupId", and (2) assume ":1" if no map size is given. Then the option could become --gidmap "+100000:@shareddata", even more explicit.

when you create a user namespace you still need to provide a mapping for the UIDs, even if you just care about the GIDs.

In general, it is desirable to provide access to as few IDs as possible to reduce the IDs shared with other containers.

If you provide a mapping that is not the identity mapping for the groups, it is desirable to do the same for the UIDs.
Given there is no better way to decide what IDs to use, the easiest is to copy the same mapping used for the groups, since in general the additional IDs assigned have the same values for UIDs and GIDs (in this regard your setup is not a typical one).

[zeehio]

Given there is no better way to decide what IDs to use, the easiest is to copy the same mapping used for the groups, since in general the additional IDs assigned have the same values for UIDs and GIDs

I understand now. Subordinated uids and gids usually match each other, so it makes sense to copy the mappings.

I was thinking about how to make all use cases behave as we would expect by default. One challenge we currently have is that the host<->intermediate mapping treats all entries in /etc/sub[ug]id in the same way, even though some entries refer to actual host groups that would benefit from having predictive mappings.

The approach we agreed so far was to not change the host<->intermediate mapping, and improve the expressiveness of the intermediate<->container mapping syntax, through the + and @ symbols. I am willing to keep working on that if that's the best we can do.

But what if instead of the current host<->intermediate default mapping 1:n we used a different strategy? Something like:

1. Parse /etc/login.defs to get the SUB_UID_MIN, SUB_UID_MAX, SUB_GID_MIN, SUB_GID_MAX ranges.
2. When creating the host<->intermediate mapping:
   1. Map the current UID (or GID) to 0, as we do now
   2. Map all subordinate ids in the SUB_UID_MIN-SUB_UID_MAX range to 1:n, as we do now
   3. Map all other subordinate ids to 1 000 000 + id, so they are predictable

This is assuming no user would have more than one million of subordinate ids, otherwise there would be a collision, or we could find the first power of ten available if above one million...

With this approach, we get a default with the best of both worlds. I believe this would make the intermediate namespace predictable for the ids that I believe should be predictable. If we then want to define the intermediate<->container mapping in some other way that would be always possible.

Would that work for you? I believe kubernetes is already parsing /etc/login.defs for some constants https://github.com/kubernetes/kubernetes/blob/ecf54b8bd5fd7051a0931920a92793716ff82b90/cmd/kubeadm/app/util/users/users_linux.go#L311 so following in that same direction makes sense to me...

[giuseppe]

But what if instead of the current host<->intermediate default mapping 1:n we used a different strategy? Something like:

1. Parse /etc/login.defs to get the SUB_UID_MIN, SUB_UID_MAX, SUB_GID_MIN, SUB_GID_MAX ranges.

2. When creating the host<->intermediate mapping:

   1. Map the current UID (or GID) to 0, as we do now
   2. Map all subordinate ids in the SUB_UID_MIN-SUB_UID_MAX range to 1:n, as we do now
   3. Map all other subordinate ids to 1 000 000 + id, so they are predictable

I think this is too complicated and it won't work when the system is configured to read the additional IDs from another source, like FreeIPA. Also at this point, changing the way we do the mappings can be considered breaking change

[zeehio]

Hi @TomSweeneyRedHat

I accepted all your suggestions and I added:

• a80b67bb49ae01103f79d2e7fe5c7fc5225b2a0b

Which renames flag to option at the --gidmap page as well for consistency.

I can squash all the commits and rebase if needed, just ping me if there is anything else I can do

[TomSweeneyRedHat]

@zeehio thanks for the updates, but now Lint is barking:

Skipped directories tunnel: pkg/api
pkg/util/utils.go:379:2: assigned to hidIsParent, but reassigned without using the value (wastedassign)
	hidIsParent := false
	^
make: *** [Makefile:277: golangci-lint] Error 1

[zeehio]

I fixed the linting issue. I don't easily see why these CI jobs are now failing, could you retry them?

I rebased again just in case

[giuseppe]

could you please squash the commits or split them in a way that each of them is a logical change?

[zeehio]

I squashed them. I left two commits:

One with a documentation clarification explaining that if a uidmap is given without gidmap then the mapping is copied.

Another one with all the other changes.

If for any reason this pull request were to be reverted (I hope not) it would be easy to leave the initial clarification.

Thanks for all the reviews and comments. I am super happy of having the opportunity to contribute to Podman!

[zeehio]

It seems the CI randomly fails, but the code should be fine. Could you retry the failing jobs? Thanks!

[zeehio]

After the last rebase no CI check failed.

[giuseppe]

thanks for the great work.

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, zeehio

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:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[giuseppe]

just one question, but we can address it later separately, how should work when you specify something like:

$ bin/podman run --uidmap=0:0:1000  --gidmap "0:1:100" --gidmap "+200000:2:1"  --rm alpine cat /proc/self/gid_map 
WARN[0000] Additional gid=1 is not present in the user namespace, skip setting it 
         0          1          1
         2          3         98
    200000          2          1

why isn't gid=1 mapped?

[zeehio]

just one question, but we can address it later separately, how should work when you specify something like:

$ bin/podman run --uidmap=0:0:1000  --gidmap "0:1:100" --gidmap "+200000:2:1"  --rm alpine cat /proc/self/gid_map 
WARN[0000] Additional gid=1 is not present in the user namespace, skip setting it 
         0          1          1
         2          3         98
    200000          2          1

why isn't gid=1 mapped?

This behaviour works as I meant to, but if you believe it should behave differently we can discuss it and it can be changed. Here is my rationale:

With --gidmap 0:1:100 you are explicitly saying "map 100 IDs starting at intermediate id 1 and at container id 0" which includes "map intermediate id 2 to container id 1". Then with --gidmap "+200000:2:1" you are overriding that first mapping by saying that intermediate id 2 should map to container id 200000 instead.

Since the + flag removes any previously given conflicting mapping, container id 1 becomes unmapped.

The reason for this behaviour is that it is impossible for podman to know if you would like to:

1. leave container id 1 unmapped
2. map intermediate id 101 (unused so far) to container id 1
3. map intermediate id 100 to container id 1 and leave container id 99 unmapped
4. Any other idea

Option 1. is safe because "it does not make assumptions about what you want" and "it is likely to give a warning".

Option 2. maps an additional intermediate id without telling you, and you had specified that 100 intermediate IDs should be mapped (and not 101), so it goes against your given specifications.

Option 3 leaves container id 99 unmapped, against your specifications

Any Option 4 will behave like options 2 and 3.

Therefore only option 1 made sense to me.

---

A different scenario is when you do not specify "0:1:100" and you directly use the "+200000:2:1". In such scenario it is clear that you only care about that one mapping and it is up to Podman to decide what to do with the remaining intermediate ids. In such scenario, Podman chooses to fill all unmapped container IDs starting from zero and until intermediate IDs are exhausted.

---

Maybe we need a flag f to be used like --gidmap="f0:1:100" that states "freely map 100 IDs starting at container id 0 and at intermediate id 1" and then Podman knows it should go for option 3, out of the described above.

If that is something interesting to implement I believe we should discuss it on a separate issue/pull request.

[giuseppe]

thanks for the explanation, no need to change it, it makes sense.

[TomSweeneyRedHat]

LGTM

[rhatdan]

/lgtm

Thanks @zeehio
Care to write a blog explaining this new feature?

[zeehio]

/lgtm

Thanks @zeehio Care to write a blog explaining this new feature?

I would love to! Do you have a blog/repo where I could create a PR with my entry?

Update: I just sent you an email with a link to a draft. I'd be happy to get feedback