Fix parsing of user/group during copy operation

If a container was started with

- a numeric uid
- both a user and group (username:groupname)
- uid and gid (uid:gid)

The copy action failed, because the "username:groupname"
was looked up using getent.

This patch;

- splits `user` and `group` before looking up
- if numeric `uid` or `gid` is used and lookup fails,
  the `uid` / `gid` is used as-is

The code also looked up the user and group on the host
instead of in the container when using getent; this patch
fixes the lookup to only use the container's /etc/passwd
and /etc/group instead.

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

daemon/archive_tarcopyoptions_unix.go

@@ -3,26 +3,130 @@
 package daemon // import "github.com/docker/docker/daemon"
 
 import (
+	"fmt"
+	"math"
+	"strconv"
+	"strings"
+
 	"github.com/docker/docker/container"
-	"github.com/docker/docker/internal/usergroup"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/idtools"
+	"github.com/moby/sys/user"
 )
 
-func (daemon *Daemon) tarCopyOptions(container *container.Container, noOverwriteDirNonDir bool) (*archive.TarOptions, error) {
-	if container.Config.User == "" {
+func (daemon *Daemon) tarCopyOptions(ctr *container.Container, noOverwriteDirNonDir bool) (*archive.TarOptions, error) {
+	if ctr.Config.User == "" {
 		return daemon.defaultTarCopyOptions(noOverwriteDirNonDir), nil
 	}
 
-	user, err := usergroup.LookupUser(container.Config.User)
+	uid, gid, err := getUIDGID(ctr.Config.User)
 	if err != nil {
-		return nil, err
+		return nil, errdefs.InvalidParameter(err)
 	}
 
-	identity := idtools.Identity{UID: user.Uid, GID: user.Gid}
-
 	return &archive.TarOptions{
 		NoOverwriteDirNonDir: noOverwriteDirNonDir,
-		ChownOpts:            &identity,
+		ChownOpts:            &idtools.Identity{UID: uid, GID: gid},
 	}, nil
 }
+
+// getUIDGID resolves the UID and GID of a given container's Config.User,
+// which can contain a user name (or ID) and, optionally, group (or ID).
+//
+// usergrp is a username or uid, and optional group, in the format `user[:group]`.
+// Both `user` and `group` can be provided as an `uid` / `gid`, so the following
+// formats are supported:
+//
+// - username            - valid username from /etc/passwd
+// - username:groupname  - valid username; valid groupname from /etc/passwd, /etc/group
+// - uid                 - 32-bit unsigned int valid Linux UID value
+// - uid:gid             - uid value; 32-bit unsigned int Linux GID value
+// - username:gid        - valid username from getent(1), gid value; 32-bit unsigned int Linux GID value
+// - uid:groupname       - 32-bit unsigned int valid Linux UID value, valid groupname from /etc/group
+//
+// If only a username (or uid) is provided, an attempt is made to look up the gid
+// for that username using /etc/passwd
+func getUIDGID(ctrUser string) (uid int, gid int, _ error) {
+	userNameOrID, groupNameOrID, _ := strings.Cut(ctrUser, ":")
+
+	// Align with behavior of docker run, which treats an empty username
+	// or groupname as default (0 (root)).
+	//
+	//	docker run --rm --user ":33" alpine id
+	//	uid=0(root) gid=33 groups=33
+	//
+	//	docker run --rm --user "33:" alpine id
+	//	uid=33 gid=0(root) groups=0(root)
+	if userNameOrID != "" {
+		var err error
+		uid, gid, err = lookupUser(userNameOrID)
+		if err != nil {
+			return 0, 0, err
+		}
+	}
+	if groupNameOrID != "" {
+		var err error
+		gid, err = lookupGID(groupNameOrID)
+		if err != nil {
+			return 0, 0, err
+		}
+	}
+	return uid, gid, nil
+}
+
+// getIDOrName checks whether nameOrID is a ID (integer) or a Name.
+// It assumes nameOrID is a name when failing to parse as integer,
+// in which case a non-empty name is returned.
+func getIDOrName(nameOrID string) (id int, name string) {
+	if uid, err := strconv.ParseUint(nameOrID, 10, 32); err == nil && uid <= math.MaxInt32 {
+		// uid provided
+		return int(uid), ""
+	}
+	// not an id, assume name
+	return 0, nameOrID
+}
+
+func lookupUser(nameOrID string) (uid, gid int, _ error) {
+	userID, userName := getIDOrName(nameOrID)
+	if userName != "" {
+		u, err := user.LookupUser(userName)
+		if err != nil {
+			return 0, 0, fmt.Errorf("failed to look up user %q in container: %w", userName, err)
+		}
+		return u.Uid, u.Gid, nil
+	}
+
+	u, err := user.LookupUid(userID)
+	if err != nil {
+		// Match behavior of "docker run": when using a UID for the
+		// user, resolving the user and its primary group is best-effort.
+		// If a user with the given UID is found, we use its primary
+		// group, otherwise use it as-is and use the default (0) as
+		//  GID.
+		//
+		//	docker run --rm --user 12345 ubuntu id
+		//	uid=12345 gid=0(root) groups=0(root)
+		//
+		//	docker run --rm ubuntu cat /etc/passwd | grep www-data
+		//	www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
+		//
+		//	docker run --rm --user 33 ubuntu id
+		//	uid=33(www-data) gid=33(www-data) groups=33(www-data)
+		return userID, 0, nil
+	}
+	return u.Uid, u.Gid, nil
+}
+
+func lookupGID(nameOrID string) (int, error) {
+	groupID, groupName := getIDOrName(nameOrID)
+	if groupName == "" {
+		// GID is passed, no need to look up
+		return groupID, nil
+	}
+	group, err := user.LookupGroup(groupName)
+	if err != nil {
+		return 0, fmt.Errorf("failed to look up group %q in container: %w", nameOrID, err)
+	}
+	return group.Gid, nil
+}

integration/container/copy_test.go

@@ -3,10 +3,12 @@ package container // import "github.com/docker/docker/integration/container"
 import (
 	"archive/tar"
 	"bytes"
+	"context"
 	"encoding/json"
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/docker/docker/api/types"
@@ -89,6 +91,124 @@ func TestCopyEmptyFile(t *testing.T) {
 	defer rdr.Close()
 }
 
+func TestCopyToContainerCopyUIDGID(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
+	ctx := setupTest(t)
+
+	apiClient := testEnv.APIClient()
+	imageID := makeTestImage(ctx, t)
+
+	tests := []struct {
+		doc      string
+		user     string
+		expected string
+	}{
+		{
+			doc:      "image default",
+			expected: "2375:2376",
+		},
+		{
+			// Align with behavior of docker run, which treats a UID with
+			// empty groupname as default (0 (root)).
+			//
+			//	docker run --rm --user "7777:" alpine id
+			//	uid=7777 gid=0(root) groups=0(root)
+			doc:      "trailing colon",
+			user:     "7777:",
+			expected: "7777:0",
+		},
+		{
+			// Align with behavior of docker run, which treats a GID with
+			// empty username as default (0 (root)).
+			//
+			//	docker run --rm --user ":7777" alpine id
+			//	uid=0(root) gid=7777 groups=7777
+			doc:      "leading colon",
+			user:     ":7777",
+			expected: "0:7777",
+		},
+		{
+			doc:      "known UID",
+			user:     "2375",
+			expected: "2375:2376",
+		},
+		{
+			doc:      "unknown UID",
+			user:     "7777",
+			expected: "7777:0",
+		},
+		{
+			doc:      "UID and GID",
+			user:     "2375:2376",
+			expected: "2375:2376",
+		},
+		{
+			doc:      "username and groupname",
+			user:     "testuser:testgroup",
+			expected: "2375:2376",
+		},
+		{
+			doc:      "username",
+			user:     "testuser",
+			expected: "2375:2376",
+		},
+		{
+			doc:      "username and GID",
+			user:     "testuser:7777",
+			expected: "2375:7777",
+		},
+		{
+			doc:      "UID and groupname",
+			user:     "7777:testgroup",
+			expected: "7777:2376",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			cID := container.Run(ctx, t, apiClient, container.WithImage(imageID), container.WithUser(tc.user))
+			defer container.Remove(ctx, t, apiClient, cID, containertypes.RemoveOptions{Force: true})
+
+			// tar with empty file
+			dstDir, preparedArchive := makeEmptyArchive(t)
+			err := apiClient.CopyToContainer(ctx, cID, dstDir, preparedArchive, containertypes.CopyToContainerOptions{
+				CopyUIDGID: true,
+			})
+			assert.NilError(t, err)
+
+			res, err := container.Exec(ctx, apiClient, cID, []string{"stat", "-c", "%u:%g", "/empty-file.txt"})
+			assert.NilError(t, err)
+			assert.Equal(t, res.ExitCode, 0)
+			assert.Equal(t, strings.TrimSpace(res.Stdout()), tc.expected)
+		})
+	}
+}
+
+func makeTestImage(ctx context.Context, t *testing.T) (imageID string) {
+	t.Helper()
+	apiClient := testEnv.APIClient()
+	tmpDir := t.TempDir()
+	buildCtx := fakecontext.New(t, tmpDir, fakecontext.WithDockerfile(`
+		FROM busybox
+		RUN addgroup -g 2376 testgroup && adduser -D -u 2375 -G testgroup testuser
+		USER testuser:testgroup
+	`))
+	defer buildCtx.Close()
+
+	resp, err := apiClient.ImageBuild(ctx, buildCtx.AsTarReader(t), types.ImageBuildOptions{})
+	assert.NilError(t, err)
+	defer resp.Body.Close()
+
+	err = jsonmessage.DisplayJSONMessagesStream(resp.Body, io.Discard, 0, false, func(msg jsonmessage.JSONMessage) {
+		var r types.BuildResult
+		assert.NilError(t, json.Unmarshal(*msg.Aux, &r))
+		imageID = r.ID
+	})
+	assert.NilError(t, err)
+	assert.Assert(t, imageID != "")
+	return imageID
+}
+
 func makeEmptyArchive(t *testing.T) (string, io.ReadCloser) {
 	tmpDir := t.TempDir()
 	srcPath := filepath.Join(tmpDir, "empty-file.txt")