From f658ea3152b364bc43c09fb6095a11b458ed93ec Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 17 Jul 2017 19:16:50 +0200 Subject: [PATCH] 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 --- daemon/archive_tarcopyoptions_unix.go | 120 ++++++++++++++++++++++++-- integration/container/copy_test.go | 120 ++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 8 deletions(-) diff --git a/daemon/archive_tarcopyoptions_unix.go b/daemon/archive_tarcopyoptions_unix.go index 0e3ff93a0a..34ecc6e882 100644 --- a/daemon/archive_tarcopyoptions_unix.go +++ b/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 +} diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index d472490451..5cccb120e9 100644 --- a/integration/container/copy_test.go +++ b/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")