Merge functionality from `github.com/moby/moby/pkg/idtools` into `user` #182

dmcgowan · 2025-01-10T20:55:55Z

Merges the idtools package functionality from github.com/moby/moby/pkg/idtools into the user package. The package was previously trimmed down and is simplified further to better fit moby/sys.

Now the package interface will just expand with:

MkdirAllAndChown with WithOnlyNew as a MkdirOpt
MkdirAndChown also supporting the opts
IdentityMapping its functions RootPair, ToHost, ToContainer, Empty

Note the Identity type is gone since SID was not used in the package or by any of the functions. It is more common to just pass along the uid and gid.

The `pkg/idtools` package supports the creation of user(s) for retrieving /etc/sub{u,g}id ranges and creation of the UID/GID mappings provided to clone() to add support for user namespaces in Docker. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)

Signed-off-by: John Howard <jhoward@microsoft.com>

This fixes errors in ownership on directory creation during build that can cause inaccessible files depending on the paths in the Dockerfile and non-existing directories in the starting image. Add tests for the mkdir variants in pkg/idtools Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)

Since Docker is already skipping newlines in /etc/sub{uid,gid}, this patch skips commented out lines - otherwise Docker fails to start. Add unit test also. Signed-off-by: Antonio Murdaca <runcom@redhat.com>

Change user/group creation to use flags to adduser/useradd to enforce it being a system user. Use system user defaults that auto-create a matching group. These changes allow us to remove all group creation code, and in doing so we also removed the code that finds available uid, gid integers and use post-creation query to gather the system-generated uid and gid. The only added complexity is that today distros don't auto-create subordinate ID ranges for a new ID if it is a system ID, so we now need to handle finding a free range and then calling the `usermod` tool to add the ranges for that ID. Note that this requires the distro supports the `-v` and `-w` flags on `usermod` for subordinate ID range additions. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)

This should not have been in init() as it causes these lookups to happen in all reexecs of the Docker binary. The only time it needs to be resolved is when a user is added, which is extremely rare. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

Warn the user and fail daemon start if the graphdir path has any elements which will deny access to the remapped root uid/gid. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

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>

This remove a dependency on `go-check` (and more) when using `pkg/idtools`. `pkg/integration` should never be called from any other package then `integration`. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

There is no case which would resolve in this error. The root user always exists, and if the id maps are empty, the default value of 0 is correct. Signed-off-by: Daniel Nephin <dnephin@docker.com>

Signed-off-by: John Howard <jhoward@microsoft.com>

Switch some more usage of the Stat function and the Stat_t type from the syscall package to golang.org/x/sys. Those were missing in PR #33399. Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

Signed-off-by: Danyal Khaliq <danyal.khaliq@tenpearls.com>

In some cases (e.g. NFS), a chown may technically be a no-op but still return `EPERM`, so only call `chown` when neccessary. This is particularly problematic for docker users bind-mounting an NFS share into a container. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

idtools don't chown if not needed

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Standard golang's `os.MkdirAll()` function returns "not a directory" error in case a directory to be created already exists but is not a directory (e.g. a file). Our own `idtools.MkdirAs*()` functions do not replicate the behavior. This is a bug since all `Mkdir()`-like functions are expected to ensure the required directory exists and is indeed a directory, and return an error otherwise. As the code is using our in-house `system.Stat()` call returning a type which is incompatible with that of golang's `os.Stat()`, I had to amend the `system` package with `IsDir()`. A test case is also provided. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt to `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there" - for `MkdirAll()`, `IsExist` error should NEVER be ignored. Mostly, this commit just removes ignoring the IsExist error, as it should not be ignored. Also, there are a couple of cases then IsExist is handled as "directory already exist" which is wrong. As a result, some code that never worked as intended is now removed. NOTE that `idtools.MkdirAndChown()` behaves like `os.MkdirAll()` rather than `os.Mkdir()` -- so its description is amended accordingly, and its usage is handled as such (i.e. IsExist error is not ignored). For more details, a quote from my runc commit 6f82d4b (July 2015): TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: > MkdirAll creates a directory named path, along with any necessary > parents, and returns nil, or else returns an error. If path > is already a directory, MkdirAll does nothing and returns nil. This means two things: 1. If a directory to be created already exists, no error is returned. 2. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. 3. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in moby#2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of moby#1, IsExist check after MkdirAll is not needed. Because of moby#2 and moby#3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Files that are suffixed with `_linux.go` or `_windows.go` are already only built on Linux / Windows, so these build-tags were redundant. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Remove dead code and redundant build tags

Signed-off-by: Daniel Nephin <dnephin@docker.com>

gty-migrate-from-testify --ignore-build-tags Signed-off-by: Daniel Nephin <dnephin@docker.com>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The implementation of CanAccess() is very rudimentary, and should not be used for anything other than a basic check (and maybe not even for that). It's only used in a single location in the daemon, so move it there, and un-export it to not encourage others to use it out of context. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The `execCmd()` utility was a basic wrapper around `exec.Command()`. Inlining it makes the code more transparent. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Removed pre-go1.17 build-tags with go fix; go mod init go fix -mod=readonly ./... rm go.mod Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

When running a `docker cp` to copy files to/from a container, the lookup of the `getent` executable happens within the container's filesystem, so we cannot re-use the results. Unfortunately, that also means we can't preserve the results for any other uses of these functions, but probably the lookup should not be "too" costly. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools: remove sync.Once, and include lookup error

The github.com/opencontainers/runc/libcontainer/user package was moved to a separate module. While there's still uses of the old module in our code-base, runc itself is migrating to the new module, and deprecated the old package (for runc 1.2). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

migrate to github.com/moby/sys/user

pkg/idtools/usergroupadd_linux.go:94:2: shadow: declaration of "ranges" shadows declaration at line 25 (govet) ranges, err := parseSubuid(name) ^ pkg/idtools/usergroupadd_linux.go:131:2: shadow: declaration of "ranges" shadows declaration at line 25 (govet) ranges, err := parseSubuid("ALL") ^ pkg/idtools/usergroupadd_linux.go:140:2: shadow: declaration of "ranges" shadows declaration at line 25 (govet) ranges, err := parseSubgid("ALL") ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools/idtools_unix_test.go:188:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar) tc := tc ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/system: deprecate MkdirAll and remove custom volume GUID handling

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Separare idtools functionality that is used internally from the functionlality used by importers. The `pkg/idtools` package is now much smaller and more generic. Signed-off-by: Derek McGowan <derek@mcg.dev> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Split idtools to an internal package and package to be moved

Signed-off-by: Derek McGowan <derek@mcg.dev>

austinvazquez · 2025-01-23T19:50:20Z

Resolves #146?

kolyshkin

Left some nits (can be addressed as a followup).

My biggest question though is why not place it into a separate moby/sys/idtools package?

kolyshkin · 2025-01-23T19:54:22Z

user/idtools.go

+	"os"
+)
+
+// MkDirOpt is a type for options to pass to Mkdir calls


nit: s/MkDir/Mkdir/

kolyshkin · 2025-01-23T19:55:07Z

user/idtools.go

+	onlyNew bool
+}
+
+// WithOnlynew is an option for MkdirAllAndChown that will only change ownership and permissions


nit: s/WithOnlynew/WithOnlyNew/

kolyshkin · 2025-01-23T20:00:01Z

user/idtools_unix.go

+			if dirPath == "/" {
+				break
+			}
+			if _, err = os.Stat(dirPath); err != nil && os.IsNotExist(err) {


nit: seems like err != nil check is redundant here.

kolyshkin · 2025-01-23T20:00:56Z

user/idtools_unix.go

+	// chown the full directory path if it exists
+	var paths []string
+	if os.IsNotExist(err) {
+		paths = []string{path}


nit: why create a new slice instead of using append?

kolyshkin · 2025-01-23T20:02:57Z

user/idtools_unix_test.go

+	dirName, err := os.MkdirTemp("", "mkdirall")
+	if err != nil {
+		t.Fatalf("Couldn't create temp dir: %v", err)
+	}
+	defer os.RemoveAll(dirName)


nit: use t.TempDir

kolyshkin · 2025-01-23T20:10:59Z

user/idtools_unix_test.go

+			return fmt.Errorf("couldn't create path: %s; error: %w", fullPath, err)
+		}
+		if err := os.Chown(fullPath, node.uid, node.gid); err != nil {
+			return fmt.Errorf("couldn't chown path: %s; error: %w", fullPath, err)


nit: no need to wrap os errors as they already have context (path and operation).

kolyshkin · 2025-01-23T20:11:10Z

user/idtools_unix_test.go

+	for path, node := range tree {
+		fullPath := filepath.Join(base, path)
+		if err := os.MkdirAll(fullPath, 0o755); err != nil {
+			return fmt.Errorf("couldn't create path: %s; error: %w", fullPath, err)


it: no need to wrap os errors as they already have context (path and operation).

kolyshkin · 2025-01-23T20:11:48Z

user/idtools_unix_test.go

+
+	dirInfos, err := os.ReadDir(base)
+	if err != nil {
+		return nil, fmt.Errorf("couldn't read directory entries for %q: %w", base, err)


ditto (no need to wrap os errors)

kolyshkin · 2025-01-23T20:13:11Z

user/idtools_unix_test.go

+	file, err := os.CreateTemp("", t.Name())
+	if err != nil {
+		t.Fatalf("Couldn't create temp dir: %v", err)
+	}
+	defer os.Remove(file.Name())


ditto (use t.TempDir)

kolyshkin · 2025-01-23T20:13:42Z

user/idtools_unix_test.go

+	}
+}
+
+func RequiresRoot(t *testing.T) {


nit: this should not be a public function.

Removed duplicated structs Removed deprecated functionality Simplified calls which previously relied on unused functionality Signed-off-by: Derek McGowan <derek@mcg.dev>

tianon · 2025-01-24T01:23:18Z

This new code is unrelated to the existing user code, right? Can we please use a different package for it?

tianon · 2025-01-24T01:24:02Z

See also #145

tianon · 2025-01-24T01:28:12Z

I guess with no new deps it is a bit of a different case 🤔👍

dmcgowan · 2025-01-24T01:29:31Z

This new code is unrelated to the existing user code, right? Can we please use a different package for it?

Well that is what is up for discussion. I think it makes sense for this logic to go along with the IDMap type, however, it is arguable that the ID map logic itself doesn't fit next to the uid lookup and goes better with userns stuff or elsewhere.

estesp and others added 30 commits October 9, 2015 17:44

Windows: Daemon broken on master

b39b044

Signed-off-by: John Howard <jhoward@microsoft.com>

pkg: idtools: fix subid files parsing

2eab947

Since Docker is already skipping newlines in /etc/sub{uid,gid}, this patch skips commented out lines - otherwise Docker fails to start. Add unit test also. Signed-off-by: Antonio Murdaca <runcom@redhat.com>

Don't start daemon in userns mode if graphdir inaccessible

f67ec21

Warn the user and fail daemon start if the graphdir path has any elements which will deny access to the remapped root uid/gid. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

Remove use of pkg/integration in pkg/idtools

896159a

This remove a dependency on `go-check` (and more) when using `pkg/idtools`. `pkg/integration` should never be called from any other package then `integration`. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

pkg: return directly without ifs where possible

f389d73

Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com>

Partial refactor of UID/GID usage to use a unified struct.

8a9ffb3

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove unused functions from archive.

ef675ed

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Convert tarAppender to the newIDMappings.

a048609

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove ToHost and replace it with IDMappings.ToHost

da0b07e

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove MkdirAllNewAs and update tests.

9124b45

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Remove error return from RootPair

013fefa

There is no case which would resolve in this error. The root user always exists, and if the id maps are empty, the default value of 0 is correct. Signed-off-by: Daniel Nephin <dnephin@docker.com>

LCOW: Create layer folders with correct ACL

8a07e96

Signed-off-by: John Howard <jhoward@microsoft.com>

Switch Stat syscalls to x/sys/unix

d1b011c

Switch some more usage of the Stat function and the Stat_t type from the syscall package to golang.org/x/sys. Those were missing in PR #33399. Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

Increase Coverage of pkg/idtools

df681c4

Signed-off-by: Danyal Khaliq <danyal.khaliq@tenpearls.com>

Merge pull request #35059 from cpuguy83/35002_handle_bind_create_errors

75a95c3

idtools don't chown if not needed

Update idtools tests to test non-deprecated functions

b7d71bf

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Minor refactor in idtools

5ac3c73

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Remove deprecated MkdirAllAs(), MkdirAs()

111ed2c

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Remove redundant build-tags

400e461

Files that are suffixed with `_linux.go` or `_windows.go` are already only built on Linux / Windows, so these build-tags were redundant. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Merge pull request #35823 from thaJeztah/remove-dead-code

a7ddc1e

Remove dead code and redundant build tags

Add canonical import comment

20e76a8

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Automated migration using

b286fb1

gty-migrate-from-testify --ignore-build-tags Signed-off-by: Daniel Nephin <dnephin@docker.com>

thaJeztah and others added 20 commits October 15, 2022 22:42

pkg/idtools: simplify if-statement

54804cf

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools: setPermissions() accept Identity as argument

66de297

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools: remove execCmd() utility

5fb47b5

The `execCmd()` utility was a basic wrapper around `exec.Command()`. Inlining it makes the code more transparent. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

remove pre-go1.17 build-tags

18279d1

Removed pre-go1.17 build-tags with go fix; go mod init go fix -mod=readonly ./... rm go.mod Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

run getent with a noop stdin

3aca343

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

pkg/idtools: use string-literals for easier grep'ing

a8e5f1c

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Merge pull request #46417 from thaJeztah/idtools_preserve_error

bc7d145

pkg/idtools: remove sync.Once, and include lookup error

Merge pull request #46711 from thaJeztah/switch_user

5b99a6c

migrate to github.com/moby/sys/user

pkg/idtools: remove redundant capturing of loop vars (copyloopvar)

758bc52

pkg/idtools/idtools_unix_test.go:188:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar) tc := tc ^ Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools: remove uses of deprecated system.MkdirAll

3d38759

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Merge pull request #49162 from thaJeztah/pkg_system_volume_uuid

fdf2879

pkg/system: deprecate MkdirAll and remove custom volume GUID handling

pkg/idtools: use lazyregexp to compile regexes on first use

5805d70

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools: rewrite to use moby/sys/user

9ecbe7b

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Merge pull request #49087 from dmcgowan/split-idtools-internal

7e518ec

Split idtools to an internal package and package to be moved

Merge idtools from github.com/moby/moby/pkg/idtools

8e08d6b

Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the merge-idtools branch 2 times, most recently from 56dd888 to d482c8a Compare January 10, 2025 20:59

kolyshkin reviewed Jan 23, 2025

View reviewed changes

Update interface to fit into user package

db55716

Removed duplicated structs Removed deprecated functionality Simplified calls which previously relied on unused functionality Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the merge-idtools branch from d482c8a to db55716 Compare January 24, 2025 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge functionality from `github.com/moby/moby/pkg/idtools` into `user` #182

Merge functionality from `github.com/moby/moby/pkg/idtools` into `user` #182

dmcgowan commented Jan 10, 2025

austinvazquez commented Jan 23, 2025

kolyshkin left a comment

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

kolyshkin Jan 23, 2025

tianon commented Jan 24, 2025

tianon commented Jan 24, 2025

tianon commented Jan 24, 2025 via email

dmcgowan commented Jan 24, 2025

Merge functionality from github.com/moby/moby/pkg/idtools into user #182

Are you sure you want to change the base?

Merge functionality from github.com/moby/moby/pkg/idtools into user #182

Conversation

dmcgowan commented Jan 10, 2025

austinvazquez commented Jan 23, 2025

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Jan 24, 2025

tianon commented Jan 24, 2025

tianon commented Jan 24, 2025 via email

dmcgowan commented Jan 24, 2025

Merge functionality from `github.com/moby/moby/pkg/idtools` into `user` #182

Merge functionality from `github.com/moby/moby/pkg/idtools` into `user` #182