remote,build: ignore if .containerignore or .dockerignore is a symlink outside of buildContext

[flouthoc]

Drop support for remote use-cases when .containerignore or .dockerignore is a symlink pointing to arbitrary location on host.

remote,build: do not allow to process arbitrary symlinks on host for remote builds.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [flouthoc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

Can you just use securejoin "github.com/cyphar/filepath-securejoin"?

I think this will throw an error if .dockerignore or .containerignore point at a link outside of the root path. That way I could have a link within the context dir.

[flouthoc]

I think this will throw an error if .dockerignore or .containerignore point at a link outside of the root path

@rhatdan I think SecureJoin does not throws error if link is outside the root path, it just resolves the symlink and concats with the root. So final path is created something as /root/some/abs/resolved/symlink. In such cases since path does not exists so no error is relyed to user it just silently ignores the .containerignore or .dockerignore file without notifying the user.

However it works correctly if symlink is inside the root or relative to root.

Following is the diff which i tried

diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go
index aabc7290d..63d354ad1 100644
--- a/pkg/bindings/images/build.go
+++ b/pkg/bindings/images/build.go
@@ -27,6 +27,7 @@ import (
        "github.com/containers/storage/pkg/ioutils"
        "github.com/docker/go-units"
        "github.com/hashicorp/go-multierror"
+       securejoin "github.com/cyphar/filepath-securejoin"
        jsoniter "github.com/json-iterator/go"
        "github.com/sirupsen/logrus"
 )
@@ -754,16 +755,16 @@ func errIfSymlink(path string) error {
 }
 
 func parseDockerignore(root string) ([]string, error) {
-       path := filepath.Join(root, ".containerignore")
-       err := errIfSymlink(path)
+       path, err := securejoin.SecureJoin(root, ".containerignore")
+       //err := errIfSymlink(path)
        if err != nil {
                return nil, err
        }
        ignore, err := os.ReadFile(path)
        if err != nil {
                var dockerIgnoreErr error
-               path = filepath.Join(root, ".dockerignore")
-               symlinkErr := errIfSymlink(path)
+               path, symlinkErr := securejoin.SecureJoin(root, ".dockerignore")
+               //symlinkErr := errIfSymlink(path)
                if symlinkErr != nil {
                        return nil, symlinkErr
                }

[flouthoc]

That way I could have a link within the context dir.

@rhatdan even though secure-join is not working as expected but I have modified function to errIfSymlinkOutsideRoot which should handle this use-case.

[rhatdan]

But wouldn't securejoin work ok since when you attempt to open the path, it would fail with file does not exist?

If I set .containerignore->/etc/shadow; don't you get an error like
/ROOTDIR/etc/shadow does not exist

[flouthoc]

But wouldn't securejoin work ok since when you attempt to open the path, it would fail with file does not exist?

If I set .containerignore->/etc/shadow; don't you get an error like /ROOTDIR/etc/shadow does not exist

@rhatdan I think It does not fails cause parseDockerIgnore ignores IsNotExists https://github.com/containers/podman/blob/main/pkg/bindings/images/build.go#L751, I think its because the ignore files are not direct input by users but detected by podman internally.

[rhatdan]

OK
LGTM
But tests are failing.

[flouthoc]

@rhatdan They were flakes CI is green now.

[rhatdan]

@containers/podman-maintainers PTAL

[eriksjolund]

I tried out f9ae0309e4213bc9094054bde0249c17bd681736
It's possible to detect that the directory /etc/testdir exists on the host (that is outside of the context directory).
The build fails when the directory does not exist. I don't know if this matters or not.

$ pwd
/home/test/srcdir
$ ls -la
total 8
drwxr-xr-x. 2 test test   62 Oct 28 07:31 .
drwx------. 8 test test 4096 Oct 28 07:24 ..
-rw-r--r--. 1 test test   13 Oct 28 07:30 Dockerfile
lrwxrwxrwx. 1 test test   42 Oct 28 07:22 .dockerignore -> /etc/testdir/../../proc/self/cwd/emptyfile
-rw-r--r--. 1 test test    0 Oct 28 07:21 emptyfile
$ cat Dockerfile
FROM scratch
$

case 1: The directory /etc/testdir exists

$ ls -ld /etc/testdir
drwxr-xr-x. 2 root root 6 Oct 28 07:32 /etc/testdir
$ pwd
/home/test/srcdir
$ podman --remote build --no-cache .
STEP 1/1: FROM scratch
COMMIT
--> 1ef5892c2c0
1ef5892c2c024279067db4deba42eb22e528d4876ddc9bc084294b2363aff67d
$

case 2: The directory /etc/testdir does not exist

$ ls -ld /etc/testdir
ls: cannot access '/etc/testdir': No such file or directory
$ pwd
/home/test/srcdir
$ podman --remote build --no-cache .
Error: unable to resolve symlink /home/test/srcdir/.dockerignore: lstat /etc/testdir: no such file or directory
$

about the Podman version

$ podman version
Client:       Podman Engine
Version:      4.3.0-dev
API Version:  4.3.0-dev
Go Version:   go1.18.6
Git Commit:   f9ae0309e4213bc9094054bde0249c17bd681736
Built:        Fri Oct 28 07:52:08 2022
OS/Arch:      linux/amd64

[flouthoc]

@eriksjolund That was one of the reason to mask this error #16315 (comment) , I think we should mask this error.

[flouthoc]

@eriksjolund Try again plz.

[eriksjolund]

@eriksjolund Try again plz.

@flouthoc I'm not able to do that right now but I could check during the weekend. I'm also a bit curious to investigate if there is any way to combine github.com/cyphar/filepath-securejoin with some additional checks in parseDockerIgnore().

Another thing: I think it would be more secure to use the resolved path resolvedPath

resolvedPath, _ := filepath.EvalSymlinks(path)

when reading the file. The symlinks might have changed after the check (TOCTOU).

[eriksjolund]

I think it would be good to aim for docker buildkit compatibility (i.e. running DOCKER_BUILDKIT=1 docker build ...)

It looks like docker resolves a .dockerignore symlink as if the build context directory is the root of the file system.

$ docker --version
Docker version 20.10.17, build 100c701
$ pwd
/Users/test/dir
$ ls -la
total 16
drwxr-xr-x  7 test  test  224 Oct 28 20:04 .
drwxr-xr-x  3 test  test   96 Oct 28 19:42 ..
lrwxr-xr-x  1 test  test    7 Oct 28 20:04 .dockerignore -> /ignore
-rw-r--r--  1 test  test   41 Oct 28 19:42 Dockerfile
-rw-r--r--  1 test  test    0 Oct 28 19:42 a1
-rw-r--r--  1 test  test    0 Oct 28 19:42 a2
-rw-r--r--  1 test  test    3 Oct 28 20:03 ignore
$ cat Dockerfile
FROM docker.io/library/alpine
COPY / /dir
$ cat ignore
a1
$ DOCKER_BUILDKIT=1 docker build -q --no-cache -t test .
sha256:4eda78871f8fe0e92e4190fdbe27477b5e7e7cb74d860e8e3e721e38a24523eb
$ docker run --rm test ls /dir
Dockerfile
a2
ignore
$ ln -sf ../ignore .dockerignore
$ ls -la
total 16
drwxr-xr-x  7 test  test  224 Oct 28 20:04 .
drwxr-xr-x  3 test  test   96 Oct 28 19:42 ..
lrwxr-xr-x  1 test  test    7 Oct 28 20:14 .dockerignore -> ../ignore
-rw-r--r--  1 test  test   41 Oct 28 19:42 Dockerfile
-rw-r--r--  1 test  test    0 Oct 28 19:42 a1
-rw-r--r--  1 test  test    0 Oct 28 19:42 a2
-rw-r--r--  1 test  test    3 Oct 28 20:03 ignore
$ DOCKER_BUILDKIT=1 docker build -q --no-cache -t test .
sha256:9f77e227de43139bba6edcca4543909d39efca7bd410f641b2facc0655bf5a01
$ docker run --rm test ls /dir
Dockerfile
a2
ignore
$

[eriksjolund]

@flouthoc with your new commit 4b242b2 the error message became

Error: /home/test/srcdir/.dockerignore cannot be a symlink outside of build context

instead of the previous result from #16315 (comment)

Error: unable to resolve symlink /home/test/srcdir/.dockerignore: lstat /etc/testdir: no such file or directory

About using github.com/cyphar/filepath-securejoin

from #16315 (comment)

In such cases since path does not exists so no error is relyed to user it just silently ignores the .containerignore or .dockerignore file without notifying the user.

One alternative could be to use filepath-securejoin and at the same time introduce new a command-line option for allowing .dockerignore to be a symlink.
Users using a symlink .dockerignore would then be surprised by a failing build instead of a non-working .dockerignore when they upgrade the Podman version.
After a few Podman releases, allowing symlink .dockerignore could be made the default setting.

A side-note regarding Buildkit: At first sight it seems that filepath-securejoin behaves in the same way as Buildkit regarding following symlinks. That conclusion is just drawn from the two Buildkit tests in
#16315 (comment)
(In other words, the behaviour might differ when looking more closely)

About using filepath.EvalSymlinks(path)

@flouthoc 4b242b2 (that is using filepath.EvalSymlinks(path)) is fine by me. Paths outside of the build context directory may be traversed but I think that commit changes the current Podman functionality the least.

[flouthoc]

I think this should become green now.

[rhatdan]

@flouthoc are bud tests failing or are they flakes?

[flouthoc]

I think last test is a flake.

[rhatdan]

@containers/podman-maintainers PTAL

[rhatdan]

@eriksjolund PTAL

[rhatdan]

@vrothberg @mheon PTAL

[rhatdan]

@giuseppe @flouthoc @vrothberg PTAL

[eriksjolund]

(feedback from reading the code)

It looks like the resulting excludes from ParseDockerignore may be dependent on the order of the items in the array containerfiles. I'm thinking of the situation when podman build --file containerfile1 --file containerfile2 . is used and at the same time the files containerfile1.containerignore and containerfile2.containerignore exist. Maybe this is expected behavoiur.

[flouthoc]

@eriksjolund Yes this is expected and keeping the original behavior but if it needs to be changed I think it should be part of a new issue.

The original issue is also fixed at buildah end here so this PR only makes sure that we dont tar up symlink from podman-remote : containers/buildah#4468

[eriksjolund]

@eriksjolund Yes this is expected and keeping the original behavior but if it needs to be changed I think it should be part of a new issue.

SGTM

Yesterday I tried out local (i.e. non-remote) Podman and got different results when using either
the filename

.dockerignore

or the filename

Dockerfile.dockerignore

If the file Dockerfile.dockerignore is a symlink pointing to for instance ../testfile,
local podman will use ../testfile

If I repeat the experiment but use the filename .dockerignore instead of the filename Dockerfile.dockerignore, in other words

$ rm Dockerfile.dockerignore
$ ln -s ../testfile .dockerignore

local podman will not use ../testfile

I think restricting local podman would be good but the scope for this PR is the remote case.

[sanmai-NL]

How is 'an arbitrary location outside the build context' defined? A symlink that resolves to a path outside the build context?

[Mingli-Yu]

Does the issue resolved in podman 4.4.2? Thanks!

[rhatdan]

@flouthoc @eriksjolund is this ready to merge?

[eriksjolund]

@flouthoc @eriksjolund is this ready to merge?

LGTM

[rhatdan]

/lgtm

[lsm5]

/cherrypick v4.4.1-rhel

[openshift-cherrypick-robot]

@lsm5: new pull request created: #20901

In response to this:

/cherrypick v4.4.1-rhel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.