RFC: remove user/group name support from libcontainer

[kolyshkin]

Description

There is a way to specify user, group, and additional groups in the OCI spec here. It is very clear from the spec that all the fields are numeric UIDs and GIDs.

Yet,

1. In libcontainer these fields are strings:
   

   runc/libcontainer/process.go

   Lines 29 to 35 in b322e31

   	// User will set the uid and gid of the executing process running inside the container
   	// local to the container's user and group configuration.
   	User string
   	// AdditionalGroups specifies the gids that should be added to supplementary groups
   	// in addition to those that the user belongs to.
   	AdditionalGroups []string

2. So, runc converts from numeric UID and GID to a string (like "$UID:$GID"):

runc/utils_linux.go

Lines 51 to 52 in b322e31

	// TODO: fix libcontainer's API to better support uid/gid in a typesafe way.
	User: fmt.Sprintf("%d:%d", p.User.UID, p.User.GID),

and does the same for additional GIDs:

runc/utils_linux.go

Lines 72 to 74 in b322e31

	for _, gid := range p.User.AdditionalGids {
	lp.AdditionalGroups = append(lp.AdditionalGroups, strconv.FormatUint(uint64(gid), 10))
	}

3. libcontainer parses the whole nine yards of /etc/passwd and /etc/group to find out the numeric UID and GID back from the User string, and does the same for every additional GID.

I imagine that makes every runc run and runc exec slower than it could be, and is totally unnecessary.

I propose to remove this functionality of resolving user/group names from libcontainer, and only read /etc/passwd in case $HOME is not set.

[kolyshkin]

Related to #3181, which was filed 2 years and 4 days ago and has some very relevant comments.

[tianon]

To be clear, the implied proposal is that this code moves into moby/moby, containerd, and other callers? (I think most of them rely on runc for this code, but maybe it's already been copied/reimplemented?)

[thaJeztah]

At a quick glance, I think there's two parts here;

• runc doing the parsing
• runc (libcontainer) providing code to do the parsing

For the former, I can see that being ok (at least I think we may already be passing uid/gid)

For the latter, I see (at least at quick glance) that

• both containerd and moby are using libcontainer/user as canonical implementation for parsing.
• depend on the types (I think the type is embedded in various parts, so somewhat hard to get rid of)

Perhaps it would be worth to;

• consider moving the user package separate (at least for containerd, it's the only dependency on runc from a Go perspective)
• splitting the concerns; we could decide for runc to no longer do the parsing, but the user package could still provide the utilities to do so (considering it the canonical implementation for parsing)

[cyphar]

It's been a while since I reworked the user code -- do we use the $HOME detection at all? Or is absolutely everything ignored by runc? I wouldn't be surprised if we ignore everything tbh.

I would be in favour of moving the code to a containerd/ or the moby/sys repo. This code was part of the original libcontainer codebase within docker and we just carried it through to runc. I think Docker also depends on libcontainer/user (unless that has been completely moved to containerd).

[kolyshkin]

1. Yes, I think moby/sys would be a good place to move libcontainer/user to (and mark it deprecated in runc, and remove in 1.3).
2. Yes, we still parse $HOME from /etc/passwd but that can be achieved by the standard os/user package AFAIK.

[kolyshkin]

OK here's the rough plan:

1. Merge libcontainer/userns: simplify, and separate from "user" package. #2868
2. Move libcontainer/user to moby/sys, mark deprecated in runc.
3. Remove /etc/passwd and /etc/group parsing from runc, except for when $HOME is needed.
4. Switch to using os/user to get $HOME from /etc/passwd.

[thaJeztah]

(whoops; was writing up a reply, but looks like I didn't post it); here goes;

I would be in favour of moving the code to a containerd/ or the moby/sys repo.

Move libcontainer/user to moby/sys

Yes, I'd be "+1" to move it there; agreed that that would be a good fit (other modules in that repository also deal with e.g. mountinfo parsing). I'm also happy to add additional maintainers to that repository.

If we do the move, we should preserve Git history (as we did with the other package).

Switch to using os/user to get $HOME from /etc/passwd.

Note that os.UserHomeDir() does not lookup the homedir, but depends on $HOME to be set (https://pkg.go.dev/os#UserHomeDir).

For os.LookupId(), I wonder if that would work, because we need to lookup the HOME based on the container environment, not the "current" environment?

We also must be careful with stdlib, as it has some sync.Once sprinkled in that package; if the ID we're looking up matches the "current" user;
https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/os/user/lookup.go;l=48-49

It will use a cached version of that information;
https://cs.opensource.google/go/go/+/refs/tags/go1.21.0:src/os/user/lookup.go;l=21-22

mark deprecated in runc.

For the parsing utilities, we can provide temporary aliases (and mark them deprecated). It runc itself stops using them, it's ok to remove those aliases in a follow-up release (they'll act to help to "alert" consumers, and to help them transition to the new location, without breaking immediately)

For the types, we must look carefully; is there any code that needs to interact with runc's types from a Go perspective (and those types to contain one of the libcontainer/user types)?

There's 4 types, and 3 errors that are exported;

• User
• SubID
• IDMap
• ExecUser
• ErrNoPasswdEntries
• ErrNoGroupEntries
• ErrRange

Looking at those, only ExecUser is referenced outside of libcontainer/user, and from a quick glance, only referenced from within libcontainer/, and from non-exported functions, so I THINK from a "Go" perspective we should be fine?

[thaJeztah]

I opened;

• integrate github.com/opencontainers/runc/libcontainer/user moby/sys#132
• Deprecate libcontainer/user, and migrate to github.com/moby/sys/user #4017

[cyphar]

For os.LookupId(), I wonder if that would work, because we need to lookup the HOME based on the container environment, not the "current" environment?

I was wondering the same thing, but we actually do the lookup after pivot_root(2) so we are doing a lookup in the container rootfs and thus you don't need the ability to pass a custom path (which is the main purpose of libcontainer/user). The sync.Once thing is quite unfortunate though... Luckily we don't call user.Current anywhere AFAIK, but it's possible some library we use might. Grr...