Merge pull request #9630 from vrothberg/cp-rootless-eperms

podman cp: ignore EPERMs in rootless mode

cmd/podman/containers/cp.go

@@ -160,6 +160,25 @@ func copyFromContainer(container string, containerPath string, hostPath string)
 		}
 	}
 
+	// If we copy a directory via the "." notation and the host path does
+	// not exist, we need to make sure that the destination on the host
+	// gets created; otherwise the contents of the source directory will be
+	// written to the destination's parent directory.
+	//
+	// While we could cut it short on the host and do create the directory
+	// ourselves, we would run into problems trying to that the other way
+	// around when copying into a container.  Instead, to keep both
+	// implementations symmetrical, we need to massage the code a bit to
+	// let Buildah's copier package create the destination.
+	//
+	// Hence, whenever "." is the source and the destination does not exist,
+	// we copy the source's parent and let the copier package create the
+	// destination via the Rename option.
+	containerTarget := containerInfo.LinkTarget
+	if hostInfoErr != nil && containerInfo.IsDir && strings.HasSuffix(containerTarget, ".") {
+		containerTarget = filepath.Dir(containerTarget)
+	}
+
 	reader, writer := io.Pipe()
 	hostCopy := func() error {
 		defer reader.Close()
@@ -193,10 +212,10 @@ func copyFromContainer(container string, containerPath string, hostPath string)
 			ChownFiles:    &idPair,
 			IgnoreDevices: true,
 		}
-		if !containerInfo.IsDir && (!hostInfo.IsDir || hostInfoErr != nil) {
+		if (!containerInfo.IsDir && !hostInfo.IsDir) || hostInfoErr != nil {
 			// If we're having a file-to-file copy, make sure to
 			// rename accordingly.
-			putOptions.Rename = map[string]string{filepath.Base(containerInfo.LinkTarget): hostBaseName}
+			putOptions.Rename = map[string]string{filepath.Base(containerTarget): hostBaseName}
 		}
 		dir := hostInfo.LinkTarget
 		if !hostInfo.IsDir {
@@ -210,7 +229,7 @@ func copyFromContainer(container string, containerPath string, hostPath string)
 
 	containerCopy := func() error {
 		defer writer.Close()
-		copyFunc, err := registry.ContainerEngine().ContainerCopyToArchive(registry.GetContext(), container, containerInfo.LinkTarget, writer)
+		copyFunc, err := registry.ContainerEngine().ContainerCopyToArchive(registry.GetContext(), container, containerTarget, writer)
 		if err != nil {
 			return err
 		}
@@ -278,6 +297,19 @@ func copyToContainer(container string, containerPath string, hostPath string) er
 		containerBaseName = filepath.Base(containerInfo.LinkTarget)
 	}
 
+	// If we copy a directory via the "." notation and the container path
+	// does not exist, we need to make sure that the destination on the
+	// container gets created; otherwise the contents of the source
+	// directory will be written to the destination's parent directory.
+	//
+	// Hence, whenever "." is the source and the destination does not
+	// exist, we copy the source's parent and let the copier package create
+	// the destination via the Rename option.
+	hostTarget := hostInfo.LinkTarget
+	if containerInfoErr != nil && hostInfo.IsDir && strings.HasSuffix(hostTarget, ".") {
+		hostTarget = filepath.Dir(hostTarget)
+	}
+
 	var stdinFile string
 	if isStdin {
 		if !containerInfo.IsDir {
@@ -318,15 +350,16 @@ func copyToContainer(container string, containerPath string, hostPath string) er
 		}
 
 		getOptions := buildahCopiah.GetOptions{
-			// Unless the specified points to ".", we want to copy the base directory.
-			KeepDirectoryNames: hostInfo.IsDir && filepath.Base(hostPath) != ".",
+			// Unless the specified path points to ".", we want to
+			// copy the base directory.
+			KeepDirectoryNames: hostInfo.IsDir && filepath.Base(hostTarget) != ".",
 		}
-		if !hostInfo.IsDir && (!containerInfo.IsDir || containerInfoErr != nil) {
+		if (!hostInfo.IsDir && !containerInfo.IsDir) || containerInfoErr != nil {
 			// If we're having a file-to-file copy, make sure to
 			// rename accordingly.
-			getOptions.Rename = map[string]string{filepath.Base(hostInfo.LinkTarget): containerBaseName}
+			getOptions.Rename = map[string]string{filepath.Base(hostTarget): containerBaseName}
 		}
-		if err := buildahCopiah.Get("/", "", getOptions, []string{hostInfo.LinkTarget}, writer); err != nil {
+		if err := buildahCopiah.Get("/", "", getOptions, []string{hostTarget}, writer); err != nil {
 			return errors.Wrap(err, "error copying from host")
 		}
 		return nil

docs/source/markdown/podman-cp.1.md

@@ -57,6 +57,8 @@ If you use a : in a local machine path, you must be explicit with a relative or
 
 Using `-` as the *src_path* streams the contents of STDIN as a tar archive. The command extracts the content of the tar to the *DEST_PATH* in the container. In this case, *dest_path* must specify a directory. Using `-` as the *dest_path* streams the contents of the resource (can be a directory) as a tar archive to STDOUT.
 
+Note that `podman cp` ignores permission errors when copying from a running rootless container.  The TTY devices inside a rootless container are owned by the host's root user and hence cannot be read inside the container's user namespace.
+
 ## OPTIONS
 
 ## ALTERNATIVES

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/checkpoint-restore/go-criu v0.0.0-20190109184317-bdb7599cd87b
 	github.com/containernetworking/cni v0.8.1
 	github.com/containernetworking/plugins v0.9.1
-	github.com/containers/buildah v1.19.7
+	github.com/containers/buildah v1.19.8
 	github.com/containers/common v0.35.0
 	github.com/containers/conmon v2.0.20+incompatible
 	github.com/containers/image/v5 v5.10.2

go.sum

@@ -94,8 +94,8 @@ github.com/containernetworking/cni v0.8.1/go.mod h1:LGwApLUm2FpoOfxTDEeq8T9ipbpZ
 github.com/containernetworking/plugins v0.8.7/go.mod h1:R7lXeZaBzpfqapcAbHRW8/CYwm0dHzbz0XEjofx0uB0=
 github.com/containernetworking/plugins v0.9.1 h1:FD1tADPls2EEi3flPc2OegIY1M9pUa9r2Quag7HMLV8=
 github.com/containernetworking/plugins v0.9.1/go.mod h1:xP/idU2ldlzN6m4p5LmGiwRDjeJr6FLK6vuiUwoH7P8=
-github.com/containers/buildah v1.19.7 h1:/g11GlhTo177xFex+5GHlF22hq01SyWaJuSA26UGFNU=
-github.com/containers/buildah v1.19.7/go.mod h1:VnyHWgNmfR1d89/zJ/F4cbwOzaQS+6sBky46W7dCo3E=
+github.com/containers/buildah v1.19.8 h1:4TzmetfKPQF5hh6GgMwbAfrD50j+PAcsRiWDnx+gCI8=
+github.com/containers/buildah v1.19.8/go.mod h1:VnyHWgNmfR1d89/zJ/F4cbwOzaQS+6sBky46W7dCo3E=
 github.com/containers/common v0.33.4/go.mod h1:PhgL71XuC4jJ/1BIqeP7doke3aMFkCP90YBXwDeUr9g=
 github.com/containers/common v0.35.0 h1:1OLZ2v+Tj/CN9BTQkKZ5VOriOiArJedinMMqfJRUI38=
 github.com/containers/common v0.35.0/go.mod h1:gs1th7XFTOvVUl4LDPdQjOfOeNiVRDbQ7CNrZ0wS6F8=

libpod/container_copy_linux.go

@@ -14,7 +14,7 @@ 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/podman/v3/pkg/rootless"
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -62,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
 	}
@@ -81,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,
@@ -121,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())
 
@@ -134,11 +149,16 @@ 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
+			// 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 {
@@ -148,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
 

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
 }

test/e2e/cp_test.go

@@ -212,7 +212,6 @@ var _ = Describe("Podman cp", func() {
 
 	// Copy the root dir "/" of a container to the host.
 	It("podman cp the root directory from the ctr to an existing directory on the host ", func() {
-		SkipIfRootless("cannot copy tty devices in rootless mode")
 		container := "copyroottohost"
 		session := podmanTest.RunTopContainer(container)
 		session.WaitWithDefaultTimeout()