From 7b186dcb9e79c15a4d5506db8922281cd8034a58 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 22 Jan 2021 15:38:51 +0100 Subject: libpod: add (*Container).ResolvePath() Add an API to libpod to resolve a path on the container. We can refactor the code that was originally written for copy. Other functions are requiring a proper path resolution, so libpod seems like a reasonable home for sharing that code. Signed-off-by: Valentin Rothberg --- libpod/container_api.go | 29 ++++++++ libpod/container_path_resolution.go | 127 ++++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 libpod/container_path_resolution.go (limited to 'libpod') diff --git a/libpod/container_api.go b/libpod/container_api.go index 0d62a2dd7..951227a4f 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -776,3 +776,32 @@ func (c *Container) ShouldRestart(ctx context.Context) bool { } return c.shouldRestart() } + +// ResolvePath resolves the specified path on the root for the container. The +// root must either be the mounted image of the container or the already +// mounted container storage. +// +// It returns the resolved root and the resolved path. Note that the path may +// resolve to the container's mount point or to a volume or bind mount. +func (c *Container) ResolvePath(ctx context.Context, root string, path string) (string, string, error) { + logrus.Debugf("Resolving path %q (root %q) on container %s", path, root, c.ID()) + + // Minimal sanity checks. + if len(root)*len(path) == 0 { + return "", "", errors.Wrapf(define.ErrInternal, "ResolvePath: root (%q) and path (%q) must be non empty", root, path) + } + if _, err := os.Stat(root); err != nil { + return "", "", errors.Wrapf(err, "cannot locate root to resolve path on container %s", c.ID()) + } + + if !c.batched { + c.lock.Lock() + defer c.lock.Unlock() + + if err := c.syncContainer(); err != nil { + return "", "", err + } + } + + return c.resolvePath(root, path) +} diff --git a/libpod/container_path_resolution.go b/libpod/container_path_resolution.go new file mode 100644 index 000000000..68f174278 --- /dev/null +++ b/libpod/container_path_resolution.go @@ -0,0 +1,127 @@ +package libpod + +import ( + "path/filepath" + "strings" + + securejoin "github.com/cyphar/filepath-securejoin" + "github.com/opencontainers/runtime-spec/specs-go" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" +) + +// resolveContainerPaths resolves the container's mount point and the container +// path as specified by the user. Both may resolve to paths outside of the +// container's mount point when the container path hits a volume or bind mount. +func (container *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) { + // Let's first make sure we have a path relative to the mount point. + pathRelativeToContainerMountPoint := containerPath + if !filepath.IsAbs(containerPath) { + // If the containerPath is not absolute, it's relative to the + // container's working dir. To be extra careful, let's first + // join the working dir with "/", and the add the containerPath + // to it. + pathRelativeToContainerMountPoint = filepath.Join(filepath.Join("/", container.WorkingDir()), containerPath) + } + resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint) + pathRelativeToContainerMountPoint = strings.TrimPrefix(pathRelativeToContainerMountPoint, mountPoint) + pathRelativeToContainerMountPoint = filepath.Join("/", pathRelativeToContainerMountPoint) + + // Now we have an "absolute container Path" but not yet resolved on the + // host (e.g., "/foo/bar/file.txt"). As mentioned above, we need to + // check if "/foo/bar/file.txt" is on a volume or bind mount. To do + // that, we need to walk *down* the paths to the root. Assuming + // volume-1 is mounted to "/foo" and volume-2 is mounted to "/foo/bar", + // we must select "/foo/bar". Once selected, we need to rebase the + // remainder (i.e, "/file.txt") on the volume's mount point on the + // host. Same applies to bind mounts. + + searchPath := pathRelativeToContainerMountPoint + for { + volume, err := findVolume(container, searchPath) + if err != nil { + return "", "", err + } + if volume != nil { + logrus.Debugf("Container path %q resolved to volume %q on path %q", containerPath, volume.Name(), searchPath) + + // TODO: We really need to force the volume to mount + // before doing this, but that API is not exposed + // externally right now and doing so is beyond the scope + // of this commit. + mountPoint, err := volume.MountPoint() + if err != nil { + return "", "", err + } + if mountPoint == "" { + return "", "", errors.Errorf("volume %s is not mounted, cannot copy into it", volume.Name()) + } + + // We found a matching volume for searchPath. We now + // need to first find the relative path of our input + // path to the searchPath, and then join it with the + // volume's mount point. + pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath) + absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume) + if err != nil { + return "", "", err + } + return mountPoint, absolutePathOnTheVolumeMount, nil + } + + if mount := findBindMount(container, searchPath); mount != nil { + logrus.Debugf("Container path %q resolved to bind mount %q:%q on path %q", containerPath, mount.Source, mount.Destination, searchPath) + // We found a matching bind mount for searchPath. We + // now need to first find the relative path of our + // input path to the searchPath, and then join it with + // the source of the bind mount. + pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath) + absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount) + if err != nil { + return "", "", err + } + return mount.Source, absolutePathOnTheBindMount, nil + + } + + if searchPath == "/" { + // Cannot go beyond "/", so we're done. + break + } + + // Walk *down* the path (e.g., "/foo/bar/x" -> "/foo/bar"). + searchPath = filepath.Dir(searchPath) + } + + // No volume, no bind mount but just a normal path on the container. + return mountPoint, resolvedPathOnTheContainerMountPoint, nil +} + +// findVolume checks if the specified container path matches a volume inside +// the container. It returns a matching volume or nil. +func findVolume(c *Container, containerPath string) (*Volume, error) { + runtime := c.Runtime() + cleanedContainerPath := filepath.Clean(containerPath) + for _, vol := range c.Config().NamedVolumes { + if cleanedContainerPath == filepath.Clean(vol.Dest) { + return runtime.GetVolume(vol.Name) + } + } + return nil, nil +} + +// findBindMount checks if the specified container path matches a bind mount +// inside the container. It returns a matching mount or nil. +func findBindMount(c *Container, containerPath string) *specs.Mount { + cleanedPath := filepath.Clean(containerPath) + for _, m := range c.Config().Spec.Mounts { + if m.Type != "bind" { + continue + } + if cleanedPath == filepath.Clean(m.Destination) { + mount := m + return &mount + } + } + return nil +} -- cgit v1.2.3-54-g00ecf From 0f668aa0857815f388ef1bb880efa4b2e889edaf Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 21 Jan 2021 15:41:14 +0100 Subject: workdir presence checks A container's workdir can be specified via the CLI via `--workdir` and via an image config with the CLI having precedence. Since images have a tendency to specify workdirs without necessarily shipping the paths with the root FS, make sure that Podman creates the workdir. When specified via the CLI, do not create the path, but check for its existence and return a human-friendly error. NOTE: `crun` is performing a similar check that would yield exit code 127. With this change, however, Podman performs the check and yields exit code 126. Since this is specific to `crun`, I do not consider it to be a breaking change of Podman. Fixes: #9040 Signed-off-by: Valentin Rothberg --- libpod/container_internal_linux.go | 63 +++++++++++++++++++++++++------- libpod/container_path_resolution.go | 47 ++++++++++++++++++++++-- pkg/specgen/generate/container.go | 14 ------- pkg/specgen/generate/container_create.go | 12 +++++- test/e2e/run_working_dir_test.go | 6 +-- test/system/030-run.bats | 21 +++++++++++ test/system/070-build.bats | 4 +- 7 files changed, 127 insertions(+), 40 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index b41a3fa38..6c9489a08 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -174,25 +174,60 @@ func (c *Container) prepare() error { return err } - // Ensure container entrypoint is created (if required) - if c.config.CreateWorkingDir { - workdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir()) - if err != nil { - return errors.Wrapf(err, "error creating path to container %s working dir", c.ID()) - } - rootUID := c.RootUID() - rootGID := c.RootGID() + // Make sure the workdir exists + if err := c.resolveWorkDir(); err != nil { + return err + } - if err := os.MkdirAll(workdir, 0755); err != nil { - if os.IsExist(err) { - return nil + return nil +} + +// resolveWorkDir resolves the container's workdir and, depending on the +// configuration, will create it, or error out if it does not exist. +// Note that the container must be mounted before. +func (c *Container) resolveWorkDir() error { + workdir := c.WorkingDir() + + // If the specified workdir is a subdir of a volume or mount, + // we don't need to do anything. The runtime is taking care of + // that. + if isPathOnVolume(c, workdir) || isPathOnBindMount(c, workdir) { + logrus.Debugf("Workdir %q resolved to a volume or mount", workdir) + return nil + } + + _, resolvedWorkdir, err := c.resolvePath(c.state.Mountpoint, workdir) + if err != nil { + return err + } + logrus.Debugf("Workdir %q resolved to host path %q", workdir, resolvedWorkdir) + + // No need to create it (e.g., `--workdir=/foo`), so let's make sure + // the path exists on the container. + if !c.config.CreateWorkingDir { + if _, err := os.Stat(resolvedWorkdir); err != nil { + if os.IsNotExist(err) { + return errors.Errorf("workdir %q does not exist on container %s", workdir, c.ID()) } - return errors.Wrapf(err, "error creating container %s working dir", c.ID()) + // This might be a serious error (e.g., permission), so + // we need to return the full error. + return errors.Wrapf(err, "error detecting workdir %q on container %s", workdir, c.ID()) } + } + + // Ensure container entrypoint is created (if required). + rootUID := c.RootUID() + rootGID := c.RootGID() - if err := os.Chown(workdir, rootUID, rootGID); err != nil { - return errors.Wrapf(err, "error chowning container %s working directory to container root", c.ID()) + if err := os.MkdirAll(resolvedWorkdir, 0755); err != nil { + if os.IsExist(err) { + return nil } + return errors.Wrapf(err, "error creating container %s workdir", c.ID()) + } + + if err := os.Chown(resolvedWorkdir, rootUID, rootGID); err != nil { + return errors.Wrapf(err, "error chowning container %s workdir to container root", c.ID()) } return nil diff --git a/libpod/container_path_resolution.go b/libpod/container_path_resolution.go index 68f174278..805b3b947 100644 --- a/libpod/container_path_resolution.go +++ b/libpod/container_path_resolution.go @@ -13,6 +13,11 @@ import ( // resolveContainerPaths resolves the container's mount point and the container // path as specified by the user. Both may resolve to paths outside of the // container's mount point when the container path hits a volume or bind mount. +// +// It returns a bool, indicating whether containerPath resolves outside of +// mountPoint (e.g., via a mount or volume), the resolved root (e.g., container +// mount, bind mount or volume) and the resolved path on the root (absolute to +// the host). func (container *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) { // Let's first make sure we have a path relative to the mount point. pathRelativeToContainerMountPoint := containerPath @@ -97,8 +102,8 @@ func (container *Container) resolvePath(mountPoint string, containerPath string) return mountPoint, resolvedPathOnTheContainerMountPoint, nil } -// findVolume checks if the specified container path matches a volume inside -// the container. It returns a matching volume or nil. +// findVolume checks if the specified containerPath matches the destination +// path of a Volume. Returns a matching Volume or nil. func findVolume(c *Container, containerPath string) (*Volume, error) { runtime := c.Runtime() cleanedContainerPath := filepath.Clean(containerPath) @@ -110,8 +115,25 @@ func findVolume(c *Container, containerPath string) (*Volume, error) { return nil, nil } -// findBindMount checks if the specified container path matches a bind mount -// inside the container. It returns a matching mount or nil. +// isPathOnVolume returns true if the specified containerPath is a subdir of any +// Volume's destination. +func isPathOnVolume(c *Container, containerPath string) bool { + cleanedContainerPath := filepath.Clean(containerPath) + for _, vol := range c.Config().NamedVolumes { + if cleanedContainerPath == filepath.Clean(vol.Dest) { + return true + } + for dest := vol.Dest; dest != "/"; dest = filepath.Dir(dest) { + if cleanedContainerPath == dest { + return true + } + } + } + return false +} + +// findBindMounts checks if the specified containerPath matches the destination +// path of a Mount. Returns a matching Mount or nil. func findBindMount(c *Container, containerPath string) *specs.Mount { cleanedPath := filepath.Clean(containerPath) for _, m := range c.Config().Spec.Mounts { @@ -125,3 +147,20 @@ func findBindMount(c *Container, containerPath string) *specs.Mount { } return nil } + +/// isPathOnBindMount returns true if the specified containerPath is a subdir of any +// Mount's destination. +func isPathOnBindMount(c *Container, containerPath string) bool { + cleanedContainerPath := filepath.Clean(containerPath) + for _, m := range c.Config().Spec.Mounts { + if cleanedContainerPath == filepath.Clean(m.Destination) { + return true + } + for dest := m.Destination; dest != "/"; dest = filepath.Dir(dest) { + if cleanedContainerPath == dest { + return true + } + } + } + return false +} diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index cc3f7928c..31d317bf8 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -203,20 +203,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat } s.Annotations = annotations - // workdir - if s.WorkDir == "" { - if newImage != nil { - workingDir, err := newImage.WorkingDir(ctx) - if err != nil { - return nil, err - } - s.WorkDir = workingDir - } - } - if s.WorkDir == "" { - s.WorkDir = "/" - } - if len(s.SeccompProfilePath) < 1 { p, err := libpod.DefaultSeccompPath() if err != nil { diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 4f36744ca..1bc050b00 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -272,10 +272,18 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen. if s.Entrypoint != nil { options = append(options, libpod.WithEntrypoint(s.Entrypoint)) } - // If the user did not set an workdir but the image did, ensure it is - // created. + // If the user did not specify a workdir on the CLI, let's extract it + // from the image. if s.WorkDir == "" && img != nil { options = append(options, libpod.WithCreateWorkingDir()) + wd, err := img.WorkingDir(ctx) + if err != nil { + return nil, err + } + s.WorkDir = wd + } + if s.WorkDir == "" { + s.WorkDir = "/" } if s.StopSignal != nil { options = append(options, libpod.WithStopSignal(*s.StopSignal)) diff --git a/test/e2e/run_working_dir_test.go b/test/e2e/run_working_dir_test.go index 7d8db361c..59538448e 100644 --- a/test/e2e/run_working_dir_test.go +++ b/test/e2e/run_working_dir_test.go @@ -2,7 +2,6 @@ package integration import ( "os" - "strings" . "github.com/containers/podman/v2/test/utils" . "github.com/onsi/ginkgo" @@ -41,12 +40,9 @@ var _ = Describe("Podman run", func() { }) It("podman run a container using non existing --workdir", func() { - if !strings.Contains(podmanTest.OCIRuntime, "crun") { - Skip("Test only works on crun") - } session := podmanTest.Podman([]string{"run", "--workdir", "/home/foobar", ALPINE, "pwd"}) session.WaitWithDefaultTimeout() - Expect(session.ExitCode()).To(Equal(127)) + Expect(session.ExitCode()).To(Equal(126)) }) It("podman run a container on an image with a workdir", func() { diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 29dc95dc3..dcf1da370 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -589,4 +589,25 @@ json-file | f is "${lines[1]}" "$rand" "Container runs successfully despite warning" } +@test "podman run - check workdir" { + # Workdirs specified via the CLI are not created on the root FS. + run_podman 126 run --rm --workdir /i/do/not/exist $IMAGE pwd + # Note: remote error prepends an attach error. + is "$output" "Error: .*workdir \"/i/do/not/exist\" does not exist on container.*" + + testdir=$PODMAN_TMPDIR/volume + mkdir -p $testdir + randomcontent=$(random_string 10) + echo "$randomcontent" > $testdir/content + + # Workdir does not exist on the image but is volume mounted. + run_podman run --rm --workdir /IamNotOnTheImage -v $testdir:/IamNotOnTheImage $IMAGE cat content + is "$output" "$randomcontent" "cat random content" + + # Workdir does not exist on the image but is created by the runtime as it's + # a subdir of a volume. + run_podman run --rm --workdir /IamNotOntheImage -v $testdir/content:/IamNotOntheImage/foo $IMAGE cat foo + is "$output" "$randomcontent" "cat random content" +} + # vim: filetype=sh diff --git a/test/system/070-build.bats b/test/system/070-build.bats index 9e5e3ee1d..cee28658f 100644 --- a/test/system/070-build.bats +++ b/test/system/070-build.bats @@ -163,10 +163,12 @@ EOF https_proxy=https-proxy-in-env-file EOF + # NOTE: it's important to not create the workdir. + # Podman will make sure to create a missing workdir + # if needed. See #9040. cat >$tmpdir/Containerfile <