From 2abfef3809abc59e8d29bcbcf2b5e0aa7141fb6d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 5 Mar 2021 10:33:27 +0100 Subject: podman cp: ignore EPERMs in rootless mode Ignore permission errors when copying from a rootless container. TTY devices inside rootless containers are owned by the host's root user which is "nobody" inside the container's user namespace rendering us unable to even read them. Enable the integration test which was temporarily disabled for rootless users. Signed-off-by: Valentin Rothberg --- libpod/container_copy_linux.go | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'libpod') diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go index 66ccd2f1f..9dd7e7e9c 100644 --- a/libpod/container_copy_linux.go +++ b/libpod/container_copy_linux.go @@ -14,6 +14,7 @@ import ( "github.com/containers/buildah/pkg/chrootuser" "github.com/containers/buildah/util" "github.com/containers/podman/v3/libpod/define" + "github.com/containers/podman/v3/pkg/rootless" "github.com/containers/storage" "github.com/containers/storage/pkg/idtools" "github.com/docker/docker/pkg/archive" @@ -139,6 +140,11 @@ func (c *Container) copyToArchive(ctx context.Context, path string, writer io.Wr ChownDirs: idPair, ChownFiles: idPair, Excludes: []string{"dev", "proc", "sys"}, + // Ignore EPERMs when copying from rootless containers + // since we cannot read TTY devices. Those are owned + // by the host's root and hence "nobody" inside the + // container's user namespace. + IgnoreUnreadable: rootless.IsRootless() && c.state.State == define.ContainerStateRunning, } return c.joinMountAndExec(ctx, func() error { -- cgit v1.2.3-54-g00ecf From a61d70cf8ef8666be87758dd3875a01ccdcc4c53 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 5 Mar 2021 11:57:14 +0100 Subject: podman cp: fix ownership Make sure the files are chowned to the host/container user, depending on where things are being copied to. Fixes: #9626 Signed-off-by: Valentin Rothberg --- libpod/container_copy_linux.go | 64 ++++++++++++++++++------------------------ test/system/065-cp.bats | 17 +++++++++++ 2 files changed, 45 insertions(+), 36 deletions(-) (limited to 'libpod') diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go index 9dd7e7e9c..5c275c641 100644 --- a/libpod/container_copy_linux.go +++ b/libpod/container_copy_linux.go @@ -15,7 +15,6 @@ import ( "github.com/containers/buildah/util" "github.com/containers/podman/v3/libpod/define" "github.com/containers/podman/v3/pkg/rootless" - "github.com/containers/storage" "github.com/containers/storage/pkg/idtools" "github.com/docker/docker/pkg/archive" "github.com/opencontainers/runtime-spec/specs-go" @@ -63,15 +62,16 @@ func (c *Container) copyFromArchive(ctx context.Context, path string, reader io. } } - decompressed, err := archive.DecompressStream(reader) + // Make sure we chown the files to the container's main user and group ID. + user, err := getContainerUser(c, mountPoint) if err != nil { unmount() return nil, err } + idPair := idtools.IDPair{UID: int(user.UID), GID: int(user.GID)} - idMappings, idPair, err := getIDMappingsAndPair(c, mountPoint) + decompressed, err := archive.DecompressStream(reader) if err != nil { - decompressed.Close() unmount() return nil, err } @@ -82,10 +82,10 @@ func (c *Container) copyFromArchive(ctx context.Context, path string, reader io. defer unmount() defer decompressed.Close() putOptions := buildahCopiah.PutOptions{ - UIDMap: idMappings.UIDMap, - GIDMap: idMappings.GIDMap, - ChownDirs: idPair, - ChownFiles: idPair, + UIDMap: c.config.IDMappings.UIDMap, + GIDMap: c.config.IDMappings.GIDMap, + ChownDirs: &idPair, + ChownFiles: &idPair, } return c.joinMountAndExec(ctx, @@ -122,11 +122,25 @@ func (c *Container) copyToArchive(ctx context.Context, path string, writer io.Wr return nil, err } - idMappings, idPair, err := getIDMappingsAndPair(c, mountPoint) + // We optimistically chown to the host user. In case of a hypothetical + // container-to-container copy, the reading side will chown back to the + // container user. + user, err := getContainerUser(c, mountPoint) + if err != nil { + unmount() + return nil, err + } + hostUID, hostGID, err := util.GetHostIDs( + idtoolsToRuntimeSpec(c.config.IDMappings.UIDMap), + idtoolsToRuntimeSpec(c.config.IDMappings.GIDMap), + user.UID, + user.GID, + ) if err != nil { unmount() return nil, err } + idPair := idtools.IDPair{UID: int(hostUID), GID: int(hostGID)} logrus.Debugf("Container copy *from* %q (resolved: %q) on container %q (ID: %s)", path, resolvedPath, c.Name(), c.ID()) @@ -135,10 +149,10 @@ func (c *Container) copyToArchive(ctx context.Context, path string, writer io.Wr getOptions := buildahCopiah.GetOptions{ // Unless the specified points to ".", we want to copy the base directory. KeepDirectoryNames: statInfo.IsDir && filepath.Base(path) != ".", - UIDMap: idMappings.UIDMap, - GIDMap: idMappings.GIDMap, - ChownDirs: idPair, - ChownFiles: idPair, + UIDMap: c.config.IDMappings.UIDMap, + GIDMap: c.config.IDMappings.GIDMap, + ChownDirs: &idPair, + ChownFiles: &idPair, Excludes: []string{"dev", "proc", "sys"}, // Ignore EPERMs when copying from rootless containers // since we cannot read TTY devices. Those are owned @@ -154,29 +168,7 @@ func (c *Container) copyToArchive(ctx context.Context, path string, writer io.Wr }, nil } -// getIDMappingsAndPair returns the ID mappings for the container and the host -// ID pair. -func getIDMappingsAndPair(container *Container, containerMount string) (*storage.IDMappingOptions, *idtools.IDPair, error) { - user, err := getContainerUser(container, containerMount) - if err != nil { - return nil, nil, err - } - - idMappingOpts, err := container.IDMappings() - if err != nil { - return nil, nil, err - } - - hostUID, hostGID, err := util.GetHostIDs(idtoolsToRuntimeSpec(idMappingOpts.UIDMap), idtoolsToRuntimeSpec(idMappingOpts.GIDMap), user.UID, user.GID) - if err != nil { - return nil, nil, err - } - - idPair := idtools.IDPair{UID: int(hostUID), GID: int(hostGID)} - return &idMappingOpts, &idPair, nil -} - -// getContainerUser returns the specs.User of the container. +// getContainerUser returns the specs.User and ID mappings of the container. func getContainerUser(container *Container, mountPoint string) (specs.User, error) { userspec := container.Config().User diff --git a/test/system/065-cp.bats b/test/system/065-cp.bats index 88ed983d8..24d8a9fd1 100644 --- a/test/system/065-cp.bats +++ b/test/system/065-cp.bats @@ -88,6 +88,7 @@ load helpers run_podman rmi -f $cpimage } + @test "podman cp file from host to container tmpfs mount" { srcdir=$PODMAN_TMPDIR/cp-test-file-host-to-ctr mkdir -p $srcdir @@ -113,6 +114,22 @@ load helpers } +@test "podman cp file from host to container and check ownership" { + srcdir=$PODMAN_TMPDIR/cp-test-file-host-to-ctr + mkdir -p $srcdir + content=cp-user-test-$(random_string 10) + echo "content" > $srcdir/hostfile + userid=$(id -u) + + run_podman run --user=$userid --userns=keep-id -d --name cpcontainer $IMAGE sleep infinity + run_podman cp $srcdir/hostfile cpcontainer:/tmp/hostfile + run_podman exec cpcontainer stat -c "%u" /tmp/hostfile + is "$output" "$userid" "copied file is chowned to the container user" + run_podman kill cpcontainer + run_podman rm -f cpcontainer +} + + @test "podman cp file from container to host" { srcdir=$PODMAN_TMPDIR/cp-test-file-ctr-to-host mkdir -p $srcdir -- cgit v1.2.3-54-g00ecf From 1f2f7e74590090fe8ba68b25f83f2ca08e2f201d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 9 Mar 2021 08:54:21 +0100 Subject: podman cp: evaluate symlink correctly when copying from container When copying from a container, make sure to evaluate the symlinks correctly. Add tests copying a symlinked directory from a running and a non-running container to execute both path-resolution paths. Signed-off-by: Valentin Rothberg --- libpod/container_stat_linux.go | 42 +++++++++++++++++++++++++++++++++--------- test/system/065-cp.bats | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-) (limited to 'libpod') diff --git a/libpod/container_stat_linux.go b/libpod/container_stat_linux.go index 307b75c14..0b4d9e2df 100644 --- a/libpod/container_stat_linux.go +++ b/libpod/container_stat_linux.go @@ -64,6 +64,13 @@ func (c *Container) stat(ctx context.Context, containerMountPoint string, contai containerPath = "/." } + // Wildcards are not allowed. + // TODO: it's now technically possible wildcards. + // We may consider enabling support in the future. + if strings.Contains(containerPath, "*") { + return nil, "", "", copy.ErrENOENT + } + if c.state.State == define.ContainerStateRunning { // If the container is running, we need to join it's mount namespace // and stat there. @@ -88,7 +95,8 @@ func (c *Container) stat(ctx context.Context, containerMountPoint string, contai } if statInfo.IsSymlink { - // Evaluated symlinks are always relative to the container's mount point. + // Symlinks are already evaluated and always relative to the + // container's mount point. absContainerPath = statInfo.ImmediateTarget } else if strings.HasPrefix(resolvedPath, containerMountPoint) { // If the path is on the container's mount point, strip it off. @@ -143,15 +151,31 @@ func secureStat(root string, path string) (*copier.StatForItem, error) { if len(globStats) != 1 { return nil, errors.Errorf("internal error: secureStat: expected 1 item but got %d", len(globStats)) } - - stat, exists := globStats[0].Results[glob] // only one glob passed, so that's okay - if !exists { - return nil, copy.ErrENOENT + if len(globStats) != 1 { + return nil, errors.Errorf("internal error: secureStat: expected 1 result but got %d", len(globStats[0].Results)) } - var statErr error - if stat.Error != "" { - statErr = errors.New(stat.Error) + // NOTE: the key in the map differ from `glob` when hitting symlink. + // Hence, we just take the first (and only) key/value pair. + for _, stat := range globStats[0].Results { + var statErr error + if stat.Error != "" { + statErr = errors.New(stat.Error) + } + // If necessary evaluate the symlink + if stat.IsSymlink { + target, err := copier.Eval(root, path, copier.EvalOptions{}) + if err != nil { + return nil, errors.Wrap(err, "error evaluating symlink in container") + } + // Need to make sure the symlink is relative to the root! + target = strings.TrimPrefix(target, root) + target = filepath.Join("/", target) + stat.ImmediateTarget = target + } + return stat, statErr } - return stat, statErr + + // Nothing found! + return nil, copy.ErrENOENT } diff --git a/test/system/065-cp.bats b/test/system/065-cp.bats index 43b21587f..73e807843 100644 --- a/test/system/065-cp.bats +++ b/test/system/065-cp.bats @@ -333,6 +333,46 @@ load helpers } +@test "podman cp symlinked directory from container" { + destdir=$PODMAN_TMPDIR/cp-weird-symlink + mkdir -p $destdir + + # Create 3 files with random content in the container. + local -a randomcontent=( + random-0-$(random_string 10) + random-1-$(random_string 15) + ) + + run_podman run -d --name cpcontainer $IMAGE sleep infinity + run_podman exec cpcontainer sh -c "echo ${randomcontent[0]} > /tmp/containerfile0" + run_podman exec cpcontainer sh -c "echo ${randomcontent[1]} > /tmp/containerfile1" + run_podman exec cpcontainer sh -c "mkdir /tmp/sub && cd /tmp/sub && ln -s .. weirdlink" + + # Commit the image for testing non-running containers + run_podman commit -q cpcontainer + cpimage="$output" + + # RUNNING container + # NOTE: /dest does not exist yet but is expected to be created during copy + run_podman cp cpcontainer:/tmp/sub/weirdlink $destdir/dest + run cat $destdir/dest/containerfile0 $destdir/dest/containerfile1 + is "${lines[0]}" "${randomcontent[0]}" "eval symlink - running container" + is "${lines[1]}" "${randomcontent[1]}" "eval symlink - running container" + + run_podman kill cpcontainer + run_podman rm -f cpcontainer + run rm -rf $srcdir/dest + + # CREATED container + run_podman create --name cpcontainer $cpimage + run_podman cp cpcontainer:/tmp/sub/weirdlink $destdir/dest + run cat $destdir/dest/containerfile0 $destdir/dest/containerfile1 + is "${lines[0]}" "${randomcontent[0]}" "eval symlink - created container" + is "${lines[1]}" "${randomcontent[1]}" "eval symlink - created container" + run_podman rm -f cpcontainer +} + + @test "podman cp file from host to container volume" { srcdir=$PODMAN_TMPDIR/cp-test-volume mkdir -p $srcdir -- cgit v1.2.3-54-g00ecf