Add support for looking up user/groups via `getent` by estesp · Pull Request #27599 · moby/moby

estesp · 2016-10-20T19:49:11Z

Fixes #20191

Adds a path to user/uid and group/gid lookup to use the capabilities for getent to use host-configured external databases for passwd and group information.

This allows the processing of --userns-remap flag to find valid users and groups on such systems. Without this, user namespaces can't be enabled on these types of Linux hosts.

justincormack · 2016-10-20T21:34:29Z

getent is not specified by Posix. I am kind of reluctant to use this, given the monumental mess we have with shelling out to useradd/adduser, as they are not standardised either and have random syntax on different distros.

We definitely need to improve this, but not sure what the best answer is.

estesp · 2016-10-26T12:58:14Z

@justincormack understand the concerns.. to me, given the way I put this PR together, if getent is "broken", missing, etc. user and group parsing function will work as it does today relying on our custom parsing of local files. Given that, having increased support for externally configured user/group databases seems reasonable for the "path of least resistance" where getent works in the mainstream way. My admittedly generic understanding is that the use of getent seems much simpler than the complicated story of useradd/adduser and friends across distros.

runcom · 2016-10-26T13:00:50Z

I'm +1 on this - my system is also setup such as that my user is on remote db (sssd)

justincormack · 2016-10-26T21:03:53Z

Yes getent does seem more standard.

thaJeztah · 2016-10-28T07:54:03Z

pkg/idtools/idtools_unix.go

This error is now always suppressed; is that desirable?

I.e., the error from getEntUser will always mask this one

This seems fine

I had a version of the code that tried not to "hide" the orig. error but it was rather ugly, and in the end, ends up as a "can't find user" (getent would have done the same searching of local filesystem in default mode, so same error). If we think there are other potential edge case errors in this path that need to be exposed we can re-work

Alright, no problem, was just wondering if the original error would contain useful information

cpuguy83 · 2016-10-28T13:04:12Z

pkg/idtools/idtools_unix.go

just return getentUser?

cpuguy83 · 2016-10-28T13:04:37Z

pkg/idtools/idtools_unix.go

return getentGroup

cpuguy83 · 2016-10-28T13:04:51Z

pkg/idtools/idtools_unix.go

return getentGroup

When processing the --userns-remap flag, add the capability to call out to `getent` if the user and group information is not found via local file parsing code already in libcontainer/user. Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

thaJeztah · 2016-10-28T23:23:10Z

looks like it's updated @cpuguy83 @justincormack ptal 😇

justincormack · 2016-11-03T15:07:44Z

LGTM

runcom · 2016-11-03T15:10:27Z

LGTM

About github.com/opencontainers/runc/libcontainer/user: According to opencontainers/runc@195d8d5 this package has two functions: - Have a static implementation of user lookup, which is now supported in the os/user stdlib package with the osusergo build tag, but wasn't at the time. - Have extra functions that os/user doesn't have, but none of those are used in homedir. Since moby#11287, homedir depended directly on libcontainer's user package for CurrentUser(). This is being replaced with os/user.Current(), because all of our static binaries are compiled with the osusergo tag, and for dynamic libraries it is more correct to use libc's implementation than parsing /etc/passwd. About github.com/docker/docker/pkg/idtools: Only dependency was from GetStatic() which uses idtools.LookupUID(uid). The implementation of idtools.LookupUID just calls to github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks to exec-ing to getent (since moby#27599). This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out of supporting nss lookups in static binaries via exec-ing to getent for the homedir package. If homedir package users need to support nss lookups, they are advised to compile dynamically instead. Signed-off-by: Tibor Vass <tibor@docker.com>

About github.com/opencontainers/runc/libcontainer/user: According to opencontainers/runc@195d8d5 this package has two functions: - Have a static implementation of user lookup, which is now supported in the os/user stdlib package with the osusergo build tag, but wasn't at the time. - Have extra functions that os/user doesn't have, but none of those are used in homedir. Since moby/moby#11287, homedir depended directly on libcontainer's user package for CurrentUser(). This is being replaced with os/user.Current(), because all of our static binaries are compiled with the osusergo tag, and for dynamic libraries it is more correct to use libc's implementation than parsing /etc/passwd. About github.com/docker/docker/pkg/idtools: Only dependency was from GetStatic() which uses idtools.LookupUID(uid). The implementation of idtools.LookupUID just calls to github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks to exec-ing to getent (since moby/moby#27599). This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out of supporting nss lookups in static binaries via exec-ing to getent for the homedir package. If homedir package users need to support nss lookups, they are advised to compile dynamically instead. Signed-off-by: Tibor Vass <tibor@docker.com> Upstream-commit: a8608b5b67c77169276863cf31c6bc89a9ab3d8c Component: engine

About github.com/opencontainers/runc/libcontainer/user: According to opencontainers/runc@195d8d5 this package has two functions: - Have a static implementation of user lookup, which is now supported in the os/user stdlib package with the osusergo build tag, but wasn't at the time. - Have extra functions that os/user doesn't have, but none of those are used in homedir. Since moby#11287, homedir depended directly on libcontainer's user package for CurrentUser(). This is being replaced with os/user.Current(), because all of our static binaries are compiled with the osusergo tag, and for dynamic libraries it is more correct to use libc's implementation than parsing /etc/passwd. About github.com/docker/docker/pkg/idtools: Only dependency was from GetStatic() which uses idtools.LookupUID(uid). The implementation of idtools.LookupUID just calls to github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks to exec-ing to getent (since moby#27599). This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out of supporting nss lookups in static binaries via exec-ing to getent for the homedir package. If homedir package users need to support nss lookups, they are advised to compile dynamically instead. Signed-off-by: Tibor Vass <tibor@docker.com> Signed-off-by: zach <Zachary.Joyner@linux.com>

GordonTheTurtle added the status/0-triage label Oct 20, 2016

estesp added status/2-code-review and removed status/0-triage labels Oct 20, 2016

estesp mentioned this pull request Oct 20, 2016

userns remapping fails on externally authenticated Linux host... #20191

Closed

estesp force-pushed the getent-path branch from 8d39318 to f857182 Compare October 26, 2016 12:52

estesp force-pushed the getent-path branch from f857182 to 96de7e6 Compare October 27, 2016 22:50

thaJeztah reviewed Oct 28, 2016

View reviewed changes

cpuguy83 reviewed Oct 28, 2016

View reviewed changes

pkg/idtools/idtools_unix.go Outdated

Copy link

Member

cpuguy83 Oct 28, 2016

Choose a reason for hiding this comment

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

just return getentUser?

cpuguy83 reviewed Oct 28, 2016

View reviewed changes

pkg/idtools/idtools_unix.go Outdated

Copy link

Member

cpuguy83 Oct 28, 2016

Choose a reason for hiding this comment

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

return getentGroup

cpuguy83 reviewed Oct 28, 2016

View reviewed changes

pkg/idtools/idtools_unix.go Outdated

Copy link

Member

cpuguy83 Oct 28, 2016

Choose a reason for hiding this comment

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

return getentGroup

estesp force-pushed the getent-path branch from 96de7e6 to 6cb8392 Compare October 28, 2016 23:06

runcom added status/4-merge and removed status/2-code-review labels Nov 3, 2016

justincormack merged commit 81683e8 into moby:master Nov 3, 2016

thaJeztah added this to the 1.13.0 milestone Nov 3, 2016

thaJeztah added the impact/changelog label Nov 3, 2016

jaredbancroft mentioned this pull request Mar 4, 2019

[rootlesskit:parent] error: failed to setup UID/GID map: failed to compute uid map: no matching entries in passwd file rootless-containers/rootlesskit#48

Closed

tiborvass mentioned this pull request Sep 23, 2019

homedir: remove idtools and libcontainer's user package dependencies #39975

Merged

Conversation

estesp commented Oct 20, 2016

Uh oh!

justincormack commented Oct 20, 2016

Uh oh!

estesp commented Oct 26, 2016

Uh oh!

runcom commented Oct 26, 2016

Uh oh!

justincormack commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 28, 2016

Uh oh!

justincormack commented Nov 3, 2016

Uh oh!

runcom commented Nov 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants