libcontainer/user: add supplementary groups only for non-numeric users

[vrothberg]

As described in opencontainers/image-spec#492 by @cyphar

Signed-off-by: Valentin Rothberg vrothberg@suse.com

[cyphar]

This comes from umoci, and I thought this semantic change made sense (adding supplementary groups if someone explicitly used a numeric uid doesn't seem right). PTAL?

/cc @crosbymichael

[crosbymichael]

LGTM

I think kube is a big users of sgroups so we need to make sure this is not going to affect something that they depend on.

[cyphar]

LGTM. We can ping someone from the kube team if you want to verify before merging?

[vrothberg]

ping

[cyphar]

@crosbymichael

It looks like they don't use libcontainer/user directly anymore, it's just used by github.com/docker/go-connections. So I reckon this is fine to merge, but we should probably ping @vishh.

% git grep 'libcontainer/user'
Godeps/Godeps.json:                     "ImportPath": "github.com/opencontainers/runc/libcontainer/user",
Godeps/LICENSES:= vendor/github.com/opencontainers/runc/libcontainer/user licensed under: =
vendor/github.com/docker/go-connections/sockets/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:go_default_library",
vendor/github.com/docker/go-connections/sockets/unix_socket.go: "github.com/opencontainers/runc/libcontainer/user"
vendor/github.com/opencontainers/runc/libcontainer/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:go_default_library",
vendor/github.com/opencontainers/runc/libcontainer/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:all-srcs",
vendor/github.com/opencontainers/runc/libcontainer/init_linux.go:       "github.com/opencontainers/runc/libcontainer/user"

[mlaventure]

This is breaking my attempt to have docker use runc master atm.

I do not see the rational for depriving a user of its additional gids for the simple reason that it was referenced by its numeric ID instead of its human readable alias.

[mlaventure]

ping @cyphar @crosbymichael 😇

[justincormack]

We only use numeric ids in LinuxKit, so this also breaks our ability to use supplemental groups. I really don't understand the rationale.

[cyphar]

The concept of getting a supplementary group from /etc/passwd for a numeric ID doesn't make all that much sense (to me at least) -- not the least of which because there isn't a unique relationship from entry --> ID so it's possible that there's ambiguity about what supplementary groups a particular UID might have if they were referenced as a user.

To be fair however, sudo does appear to do the same thing:

% sudo -u '#1000' id
uid=1000(cyphar) gid=100(users) groups=100(users),10(wheel),16(dialout),469(libvirt),473(vboxusers),475(docker)

Maybe there is a justification for it...

[mlaventure]

In practice, there should only be a single entry for a given UID. And since standard tools seem to assume so, I think it'd make sense for this repo packages to do the same.

Bottom line, it would be nice if the maintainers could vote on reverting this PR 👼

[crosbymichael]

I'm fine reverting this as it causes issues in practice and wasn't really fixing a problem in the first place.

[mlaventure]

Created a PR reverting this change #1548

[cyphar]

Merged it. Thanks @mlaventure.