From d175fbfdb460315d2a3f8b0c679101b604411ef8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 9 Mar 2021 08:59:43 +0100 Subject: [PATCH 1/5] vendor buildah@v1.19.8 Signed-off-by: Valentin Rothberg --- go.mod | 2 +- go.sum | 4 +- .../github.com/containers/buildah/buildah.go | 2 +- .../containers/buildah/copier/copier.go | 42 +++++++++++++++---- vendor/modules.txt | 2 +- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index 4e8c6e8bfe..8223d6ad95 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 751d26ce3f..d52b85817b 100644 --- a/go.sum +++ b/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= diff --git a/vendor/github.com/containers/buildah/buildah.go b/vendor/github.com/containers/buildah/buildah.go index 77d313c586..427950c5ca 100644 --- a/vendor/github.com/containers/buildah/buildah.go +++ b/vendor/github.com/containers/buildah/buildah.go @@ -28,7 +28,7 @@ const ( Package = "buildah" // Version for the Package. Bump version in contrib/rpm/buildah.spec // too. - Version = "1.19.7" + Version = "1.19.8" // The value we use to identify what type of information, currently a // serialized Builder structure, we are using as per-container state. // This should only be changed when we make incompatible changes to diff --git a/vendor/github.com/containers/buildah/copier/copier.go b/vendor/github.com/containers/buildah/copier/copier.go index b5e107d4ba..52d8133c71 100644 --- a/vendor/github.com/containers/buildah/copier/copier.go +++ b/vendor/github.com/containers/buildah/copier/copier.go @@ -284,6 +284,7 @@ type GetOptions struct { KeepDirectoryNames bool // don't strip the top directory's basename from the paths of items in subdirectories Rename map[string]string // rename items with the specified names, or under the specified names NoDerefSymlinks bool // don't follow symlinks when globs match them + IgnoreUnreadable bool // ignore errors reading items, instead of returning an error } // Get produces an archive containing items that match the specified glob @@ -1035,6 +1036,14 @@ func copierHandlerStat(req request, pm *fileutils.PatternMatcher) *response { return &response{Stat: statResponse{Globs: stats}} } +func errorIsPermission(err error) bool { + err = errors.Cause(err) + if err == nil { + return false + } + return os.IsPermission(err) || strings.Contains(err.Error(), "permission denied") +} + func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMatcher, idMappings *idtools.IDMappings) (*response, func() error, error) { statRequest := req statRequest.Request = requestStat @@ -1111,6 +1120,12 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa options.ExpandArchives = false walkfn := func(path string, info os.FileInfo, err error) error { if err != nil { + if options.IgnoreUnreadable && errorIsPermission(err) { + if info != nil && info.IsDir() { + return filepath.SkipDir + } + return nil + } return errors.Wrapf(err, "copier: get: error reading %q", path) } // compute the path of this item @@ -1150,7 +1165,13 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa symlinkTarget = target } // add the item to the outgoing tar stream - return copierHandlerGetOne(info, symlinkTarget, rel, path, options, tw, hardlinkChecker, idMappings) + if err := copierHandlerGetOne(info, symlinkTarget, rel, path, options, tw, hardlinkChecker, idMappings); err != nil { + if req.GetOptions.IgnoreUnreadable && errorIsPermission(err) { + return nil + } + return err + } + return nil } // walk the directory tree, checking/adding items individually if err := filepath.Walk(item, walkfn); err != nil { @@ -1170,6 +1191,9 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa // dereferenced, be sure to use the name of the // link. if err := copierHandlerGetOne(info, "", filepath.Base(queue[i]), item, req.GetOptions, tw, hardlinkChecker, idMappings); err != nil { + if req.GetOptions.IgnoreUnreadable && errorIsPermission(err) { + continue + } return errors.Wrapf(err, "copier: get: %q", queue[i]) } itemsCopied++ @@ -1250,7 +1274,7 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str if options.ExpandArchives && isArchivePath(contentPath) { f, err := os.Open(contentPath) if err != nil { - return errors.Wrapf(err, "error opening %s", contentPath) + return errors.Wrapf(err, "error opening file for reading archive contents") } defer f.Close() rc, _, err := compression.AutoDecompress(f) @@ -1321,17 +1345,21 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str hdr.Mode = int64(*options.ChmodFiles) } } + var f *os.File + if hdr.Typeflag == tar.TypeReg { + // open the file first so that we don't write a header for it if we can't actually read it + f, err = os.Open(contentPath) + if err != nil { + return errors.Wrapf(err, "error opening file for adding its contents to archive") + } + defer f.Close() + } // output the header if err = tw.WriteHeader(hdr); err != nil { return errors.Wrapf(err, "error writing header for %s (%s)", contentPath, hdr.Name) } if hdr.Typeflag == tar.TypeReg { // output the content - f, err := os.Open(contentPath) - if err != nil { - return errors.Wrapf(err, "error opening %s", contentPath) - } - defer f.Close() n, err := io.Copy(tw, f) if err != nil { return errors.Wrapf(err, "error copying %s", contentPath) diff --git a/vendor/modules.txt b/vendor/modules.txt index 2450446c35..c988d641be 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -72,7 +72,7 @@ github.com/containernetworking/plugins/pkg/utils/hwaddr github.com/containernetworking/plugins/pkg/utils/sysctl github.com/containernetworking/plugins/plugins/ipam/host-local/backend github.com/containernetworking/plugins/plugins/ipam/host-local/backend/allocator -# github.com/containers/buildah v1.19.7 +# github.com/containers/buildah v1.19.8 github.com/containers/buildah github.com/containers/buildah/bind github.com/containers/buildah/chroot From 2abfef3809abc59e8d29bcbcf2b5e0aa7141fb6d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 5 Mar 2021 10:33:27 +0100 Subject: [PATCH 2/5] 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 --- docs/source/markdown/podman-cp.1.md | 2 ++ libpod/container_copy_linux.go | 6 ++++++ test/e2e/cp_test.go | 1 - 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/source/markdown/podman-cp.1.md b/docs/source/markdown/podman-cp.1.md index 56511c2446..bafbbdf3b5 100644 --- a/docs/source/markdown/podman-cp.1.md +++ b/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 diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go index 66ccd2f1f4..9dd7e7e9cc 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 { diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go index c0fb61544a..c0fb3f8878 100644 --- a/test/e2e/cp_test.go +++ b/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() From a61d70cf8ef8666be87758dd3875a01ccdcc4c53 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 5 Mar 2021 11:57:14 +0100 Subject: [PATCH 3/5] 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(-) diff --git a/libpod/container_copy_linux.go b/libpod/container_copy_linux.go index 9dd7e7e9cc..5c275c641f 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 88ed983d8d..24d8a9fd1b 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 From 31b11b5cd62093d86891d32e0912661814d6b78b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 5 Mar 2021 15:57:51 +0100 Subject: [PATCH 4/5] podman cp: fix copying to a non-existent dir Copy is full of perils. Some of them are the nuances when copying directories. Who would have thought that * cp dir foo * cp dir/ foo * cp dir/. foo are all supposed to yield the same result when foo does not exist. `podman cp` now supports all three notations, which required to massage the front-end code in `cmd/podman` a bit. The tests have been extended and partially rewritten to test container->host and host->container copy operations. Signed-off-by: Valentin Rothberg --- cmd/podman/containers/cp.go | 49 ++++++++++++++++++++++++----- test/system/065-cp.bats | 62 +++++++++++++++++++------------------ 2 files changed, 73 insertions(+), 38 deletions(-) diff --git a/cmd/podman/containers/cp.go b/cmd/podman/containers/cp.go index 7a62d982c0..27aacc6e53 100644 --- a/cmd/podman/containers/cp.go +++ b/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 diff --git a/test/system/065-cp.bats b/test/system/065-cp.bats index 24d8a9fd1b..43b21587fe 100644 --- a/test/system/065-cp.bats +++ b/test/system/065-cp.bats @@ -192,20 +192,19 @@ load helpers @test "podman cp dir from host to container" { - dirname=dir-test - srcdir=$PODMAN_TMPDIR/$dirname - mkdir -p $srcdir + srcdir=$PODMAN_TMPDIR + mkdir -p $srcdir/dir/sub local -a randomcontent=( random-0-$(random_string 10) random-1-$(random_string 15) ) - echo "${randomcontent[0]}" > $srcdir/hostfile0 - echo "${randomcontent[1]}" > $srcdir/hostfile1 + echo "${randomcontent[0]}" > $srcdir/dir/sub/hostfile0 + echo "${randomcontent[1]}" > $srcdir/dir/sub/hostfile1 # "." and "dir/." will copy the contents, so make sure that a dir ending # with dot is treated correctly. - mkdir -p $srcdir. - cp $srcdir/* $srcdir./ + mkdir -p $srcdir/dir. + cp -r $srcdir/dir/* $srcdir/dir. run_podman run -d --name cpcontainer --workdir=/srv $IMAGE sleep infinity run_podman exec cpcontainer mkdir /srv/subdir @@ -216,12 +215,15 @@ load helpers # format is: | | | tests=" - | / | /dir-test | copy to root - . | / | /dir-test. | copy dotdir to root - / | /tmp | /tmp/dir-test | copy to tmp - /. | /usr/ | /usr/ | copy contents of dir to usr/ - | . | /srv/dir-test | copy to workdir (rel path) - | subdir/. | /srv/subdir/dir-test | copy to workdir subdir (rel path) + dir | / | /dir/sub | copy dir to root + dir. | / | /dir./sub | copy dir. to root + dir/ | /tmp | /tmp/dir/sub | copy dir/ to tmp + dir/. | /usr/ | /usr/sub | copy dir/. usr/ + dir/sub | . | /srv/sub | copy dir/sub to workdir (rel path) + dir/sub/. | subdir/. | /srv/subdir | copy dir/sub/. to workdir subdir (rel path) + dir | /newdir1 | /newdir1/sub | copy dir to newdir1 + dir/ | /newdir2 | /newdir2/sub | copy dir/ to newdir2 + dir/. | /newdir3 | /newdir3/sub | copy dir/. to newdir3 " # RUNNING container @@ -230,12 +232,10 @@ load helpers if [[ $src == "''" ]];then unset src fi - run_podman cp $srcdir$src cpcontainer:$dest - run_podman exec cpcontainer ls $dest_fullname - run_podman exec cpcontainer cat $dest_fullname/hostfile0 - is "$output" "${randomcontent[0]}" "$description (cp -> ctr:$dest)" - run_podman exec cpcontainer cat $dest_fullname/hostfile1 - is "$output" "${randomcontent[1]}" "$description (cp -> ctr:$dest)" + run_podman cp $srcdir/$src cpcontainer:$dest + run_podman exec cpcontainer cat $dest_fullname/hostfile0 $dest_fullname/hostfile1 + is "${lines[0]}" "${randomcontent[0]}" "$description (cp -> ctr:$dest)" + is "${lines[1]}" "${randomcontent[1]}" "$description (cp -> ctr:$dest)" done < <(parse_table "$tests") run_podman kill cpcontainer run_podman rm -f cpcontainer @@ -247,7 +247,7 @@ load helpers unset src fi run_podman create --name cpcontainer --workdir=/srv $cpimage sleep infinity - run_podman cp $srcdir$src cpcontainer:$dest + run_podman cp $srcdir/$src cpcontainer:$dest run_podman start cpcontainer run_podman exec cpcontainer cat $dest_fullname/hostfile0 $dest_fullname/hostfile1 is "${lines[0]}" "${randomcontent[0]}" "$description (cp -> ctr:$dest)" @@ -280,17 +280,19 @@ load helpers run_podman commit -q cpcontainer cpimage="$output" - # format is: | | + # format is: | | | tests=" - /srv | /srv/subdir | copy /srv - /srv/ | /srv/subdir | copy /srv/ - /srv/. | /subdir | copy /srv/. - /srv/subdir/. | | copy /srv/subdir/. - /tmp/subdir. | /subdir. | copy /tmp/subdir. +/srv | | /srv/subdir | copy /srv +/srv | /newdir | /newdir/subdir | copy /srv to /newdir +/srv/ | | /srv/subdir | copy /srv/ +/srv/. | | /subdir | copy /srv/. +/srv/. | /newdir | /newdir/subdir | copy /srv/. to /newdir +/srv/subdir/. | | | copy /srv/subdir/. +/tmp/subdir. | | /subdir. | copy /tmp/subdir. " # RUNNING container - while read src dest_fullname description; do + while read src dest dest_fullname description; do if [[ $src == "''" ]];then unset src fi @@ -300,7 +302,7 @@ load helpers if [[ $dest_fullname == "''" ]];then unset dest_fullname fi - run_podman cp cpcontainer:$src $destdir + run_podman cp cpcontainer:$src $destdir$dest is "$(< $destdir$dest_fullname/containerfile0)" "${randomcontent[0]}" "$description" is "$(< $destdir$dest_fullname/containerfile1)" "${randomcontent[1]}" "$description" rm -rf $destdir/* @@ -310,7 +312,7 @@ load helpers # CREATED container run_podman create --name cpcontainer --workdir=/srv $cpimage - while read src dest_fullname description; do + while read src dest dest_fullname description; do if [[ $src == "''" ]];then unset src fi @@ -320,7 +322,7 @@ load helpers if [[ $dest_fullname == "''" ]];then unset dest_fullname fi - run_podman cp cpcontainer:$src $destdir + run_podman cp cpcontainer:$src $destdir$dest is "$(< $destdir$dest_fullname/containerfile0)" "${randomcontent[0]}" "$description" is "$(< $destdir$dest_fullname/containerfile1)" "${randomcontent[1]}" "$description" rm -rf $destdir/* From 1f2f7e74590090fe8ba68b25f83f2ca08e2f201d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 9 Mar 2021 08:54:21 +0100 Subject: [PATCH 5/5] 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(-) diff --git a/libpod/container_stat_linux.go b/libpod/container_stat_linux.go index 307b75c14e..0b4d9e2df1 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 43b21587fe..73e807843c 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