podman cp: fix ownership

Make sure the files are chowned to the host/container user, depending on where things are being copied to. Fixes: containers#9626 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg · Mar 10, 2021 · ca08a72 · ca08a72
1 parent 1671dac
commit ca08a72
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 36 deletions.
diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go
@@ -14,7 +14,6 @@ import (
 	"github.com/containers/buildah/pkg/chrootuser"
 	"github.com/containers/buildah/util"
 	"github.com/containers/podman/v3/libpod/define"
-	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -62,15 +61,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
 	}
@@ -81,10 +81,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,
@@ -121,11 +121,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())
 
@@ -134,10 +148,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"},
 		}
 		return c.joinMountAndExec(ctx,
@@ -148,29 +162,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
@@ -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