moby / buildkit

concurrent, cache-efficient, and Dockerfile-agnostic builder toolkit
https://github.com/moby/moby/issues/34227
Apache License 2.0
8.07k stars 1.13k forks source link

RUN --mount=type=cache should inherit ownership/permissions from mountpoint #1237

Open thaJeztah opened 4 years ago

thaJeztah commented 4 years ago

When using RUN --mount=type=cache, the uid/gid of the mount defaults to 0:0 (root), even if the mountpoint is an existing directory with different permissions.

While it's possible to manually override the uid/gid for the mount, I think it would make sense to default to the permissions of the mountpoint (if exists), and otherwise fall back to the current behaviour (root).

Example / reproduction steps:

# syntax=docker/dockerfile:1.1.3-experimental

FROM busybox

ARG UID=1000
ARG GID=1000

RUN addgroup -g ${GID} foo
RUN adduser -D -u ${UID} -G foo foo
RUN mkdir -p /mycache && chown -R "${UID}:${GID}" /mycache

# this works (as we're root):
RUN mkdir -p /mycache/root-nomount/bar

# this works (we're still root):
RUN --mount=type=cache,target=/mycache \
        mkdir -p /mycache/root-withmount/bar

USER ${UID}:${GID}

# without `--mount`, this works, because `/mycache` was created with
# the correct permissions:
RUN mkdir -p /mycache/foo-nomount/bar

# with `--mount`, this works if we manually set the uid/gid, but this does not
# work well (currently) with dynamically set UID/GID
RUN --mount=type=cache,target=/mycache,uid=1000,gid=1000 \
        mkdir -p /mycache/foo-withmount-manually/bar

# but this fails:
#
# > [stage-0 9/9] RUN --mount=type=cache,target=/mycache         mkdir -p /mycache/foo-withmount/bar:
#15 0.607 mkdir: can't create directory '/mycache/foo-withmount/': Permission denied
RUN --mount=type=cache,target=/mycache \
        mkdir -p /mycache/foo-withmount/bar
DOCKER_BUILDKIT=1 docker build .

[+] Building 11.9s (15/15) FINISHED                                                                                                                                                           
 => [internal] load build definition from Dockerfile                                                                                                                                     0.1s
 => => transferring dockerfile: 1.07kB                                                                                                                                                   0.0s
 => [internal] load .dockerignore                                                                                                                                                        0.1s
 => => transferring context: 2B                                                                                                                                                          0.0s
 => resolve image config for docker.io/docker/dockerfile:1.1.3-experimental                                                                                                              1.4s
 => docker-image://docker.io/docker/dockerfile:1.1.3-experimental@sha256:888f21826273409b5ef5ff9ceb90c64a8f8ec7760da30d1ffbe6c3e2d323a7bd                                                2.8s
 => => resolve docker.io/docker/dockerfile:1.1.3-experimental@sha256:888f21826273409b5ef5ff9ceb90c64a8f8ec7760da30d1ffbe6c3e2d323a7bd                                                    0.0s
 => => sha256:888f21826273409b5ef5ff9ceb90c64a8f8ec7760da30d1ffbe6c3e2d323a7bd 2.03kB / 2.03kB                                                                                           0.0s
 => => sha256:ee85655c57140bd20a5ebc3bb802e7410ee9ac47ca92b193ed0ab17485024fe5 527B / 527B                                                                                               0.0s
 => => sha256:80b5f664ac0c5f6b89608f7b0afd9548ac5b2d4cd631b7b723d9f2edca8676d9 897B / 897B                                                                                               0.0s
 => => sha256:73e11b97eeae87ce062a224a9ff2d7f8a919d3b50e014a3f33a79b0c219a7003 8.82MB / 8.82MB                                                                                           2.3s
 => => extracting sha256:73e11b97eeae87ce062a224a9ff2d7f8a919d3b50e014a3f33a79b0c219a7003                                                                                                0.3s
 => [internal] load metadata for docker.io/library/busybox:latest                                                                                                                        0.0s
 => [stage-0 1/9] FROM docker.io/library/busybox                                                                                                                                         0.0s
 => => resolve docker.io/library/busybox:latest                                                                                                                                          0.0s
 => [internal] settings cache mount permissions                                                                                                                                          0.1s
 => [stage-0 2/9] RUN addgroup -g 1000 foo                                                                                                                                               0.9s
 => [stage-0 3/9] RUN adduser -D -u 1000 -G foo foo                                                                                                                                      0.9s
 => [stage-0 4/9] RUN mkdir -p /mycache && chown -R "1000:1000" /mycache                                                                                                                 0.8s
 => [stage-0 5/9] RUN mkdir -p /mycache/root-nomount/bar                                                                                                                                 1.0s
 => [stage-0 6/9] RUN --mount=type=cache,target=/mycache         mkdir -p /mycache/root-withmount/bar                                                                                    0.8s
 => [stage-0 7/9] RUN mkdir -p /mycache/foo-nomount/bar                                                                                                                                  0.8s
 => [stage-0 8/9] RUN --mount=type=cache,target=/mycache,uid=1000,gid=1000         mkdir -p /mycache/foo-withmount-manually/bar                                                          0.8s
 => ERROR [stage-0 9/9] RUN --mount=type=cache,target=/mycache         mkdir -p /mycache/foo-withmount/bar                                                                               0.7s
------                                                                                                                                                                                        
 > [stage-0 9/9] RUN --mount=type=cache,target=/mycache         mkdir -p /mycache/foo-withmount/bar:
#14 0.543 mkdir: can't create directory '/mycache/foo-withmount/': Permission denied
------
failed to solve with frontend dockerfile.v0: failed to solve with frontend gateway.v0: rpc error: code = Unknown desc = failed to build LLB: executor failed running [/bin/sh -c mkdir -p /mycache/foo-withmount/bar]: runc did not terminate sucessfully
thaJeztah commented 4 years ago

@AkihiroSuda @tonistiigi @tiborvass ptal

tonistiigi commented 4 years ago

This isn't really possible as uid/gid is part of the cache. Therefore it can't be different for same mount appearing multiple times. Another issue is that USER can be username that only makes sense in the relation of /etc/passwd that probably is not on the mount. Looking that up is slow(and actually not possible as all mounts are prepared independently) and again may be different for different contexts.

thaJeztah commented 4 years ago

@tonistiigi I don't think a lookup is needed. I didn't mean for it to lookup the current image's USER but that BuildKit should look at the permissions of the target, so in the example above, the target (mount point) is /mycache;

(note that it should not do anything with possibly existing content in the cache; only the ownership of the target directory / mount-point)

Therefore it can't be different for same mount appearing multiple times

Isn't that an existing issue? What is the current behavior if I use the same target with different uid/gid combinations?

RUN --mount=type=cache,target=/mycache,uid=123,gid=123 \
    echo "do something"

RUN --mount=type=cache,target=/mycache,uid=456,gid=456 \
    echo "do something"
tonistiigi commented 4 years ago

I read it wrong but the issues remain. Both in the sense of multiple mounts colliding and mounts being prepared independently.

Isn't that an existing issue?

Yes, I believe this is the case. That's why it's better for it to be explicit so the user can point set a different id in this case. Actually, it would probably be safer to enforce this by default, so that in the example above the cache would not be shared.

wdhorton commented 1 year ago

I ran into this today, it is surprising to me that the permissions for the mount don't match the target path if it already exists. It is also incredible hard to figure out that this is happening. If this behavior is intended there should at least be a warning logged if the permissions of the target path aren't root

Klwilson2272 commented 7 months ago

Is there a example of how to get a --mount=type=cache to work with anything other than root. As the recommendation to create containers that run as non-root. The mount option seems to be overwriting the existing permissions with root:root making the usage impossible for non root users. Any progress?

bor8 commented 6 months ago

Directly specifying user ID (UID) and group ID (GID) not only simplifies user management but significantly bolsters security by ensuring precise control over access permissions, thereby mitigating the risk of unauthorized access:

FROM maven:3.9.6-eclipse-temurin-17
RUN groupadd -g 998 builder && \
useradd --system --create-home --home-dir /home/builder -u 998 -g builder builder
USER builder
WORKDIR /home/builder
ENV MAVEN_CONFIG /home/builder/.m2
COPY pom.xml .
COPY src src
RUN --mount=type=cache,uid=998,gid=998,target=/home/builder/.m2 \
touch /home/builder/.m2/LOCATE_THIS_FILE_XYZ_1 && \
mvn dependency:go-offline && \
mvn test-compile dependency:copy-dependencies -DoutputDirectory=target/dependency
CMD ls -al /home/builder/target

I hope this addresses the issue at hand. Unfortunately, specifying the UID and GID appears to be mandatory, rather than optional.