when created, workdir is owned by user, not root

[ndeloof]

closes docker/cli#3464

- What I did
If not present on image filesystem, and created on purpose, working directory is owned by container's USER, not root

- How I did it
get user from container.Config and use this Identity to setup working directory

- How to verify it
docker run -w /foo alpine touch /foo/bar

- Description for the changelog
working directory, if created, is owned by container's USER

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Looks like this is failing on Windows (maybe it doesn't expect the UID/GID when creating; wondering if this was an existing bug that happened to be working with root (uid/gid 0:0 therefore ignored? 🤔); just an initial hunch

=== RUN   TestCreateWorkingDirForUser
    create_test.go:555: assertion failed: error is not nil: Error response from daemon: container bab4875be8b71f227669cd1de1c40034f6bb127d96b860e9468b1a8e0126371e encountered an error during hcs::System::CreateProcess: failure in a Windows system call: The user name or password is incorrect. (0x52e)
--- FAIL: TestCreateWorkingDirForUser (1.60s)

[thaJeztah]

I think the SetupWorkingDirectory is ran within the "host's" context; in that case, I'm wondering if this needs to remap the UID/GID to match the container's user in case user-namespaces are enabled.

daemon.idMapping.RootPair() calls GetRootUIDGID(), which calls toHost() to perform the mapping from "container user" to "host user";

moby/pkg/idtools/idtools.go

Lines 60 to 65 in 0a3336f

	func GetRootUIDGID(uidMap, gidMap []IDMap) (int, int, error) {
	uid, err := toHost(0, uidMap)
	if err != nil {
	return -1, -1, err
	}
	gid, err := toHost(0, gidMap)

There is an IdentityMapping.ToHost() method (but it expects UIDMaps to be propagated, so we need to make sure that's the case);

moby/pkg/idtools/idtools.go

Line 133 in 0a3336f

func (i IdentityMapping) ToHost(pair Identity) (Identity, error) {

[ndeloof]

tested with daemon configured with userns-remap, and indeed need to convert Identity to host IDs

[thaJeztah]

Chatted about this change in the maintainers meeting and everyone was OK with making this change (there was no concern about changing the permissions); we didn't do a code-review (and I mentioned it was currently failing)

[thaJeztah]

LOL.. what's that symbol between create and windows (create°windows)? 😂 🤔

[ndeloof]

something weird happened with my IDE...

[thaJeztah]

These failures look related; wondering what's causing them (could it be a bug somewhere, where docker build is looking up up the user on the host instead of within the container?)

=== RUN   TestDockerSuite/TestBuildBuildTimeArgExpansion
    docker_cli_build_test.go:4336: assertion failed: 
        Command:  /usr/local/cli/docker build -t bldvarstest --build-arg WDIR=/tmp/ --build-arg AFILE=addFile --build-arg CFILE=copyFile --build-arg foo=bar --build-arg EPORT=9999 --build-arg USER=testUser --build-arg VOL=/testVol/ .
        ExitCode: 1
        Error:    exit status 1
        Stdout:   Sending build context to Docker daemon  4.096kB

        Step 1/15 : FROM busybox
         ---> 1c35c4412082
        Step 2/15 : ARG WDIR
         ---> Running in 1b399eaf3ffe
        Removing intermediate container 1b399eaf3ffe
         ---> af930ee2e9bf
        Step 3/15 : WORKDIR ${WDIR}
         ---> Running in 4837dde83adb
        Removing intermediate container 4837dde83adb
         ---> 7064d71be17f
        Step 4/15 : ARG AFILE
         ---> Running in 4ea8c26f6197
        Removing intermediate container 4ea8c26f6197
         ---> a49ed2d47684
        Step 5/15 : ADD ${AFILE} testDir/
         ---> f45488412ab2
        Step 6/15 : ARG CFILE
         ---> Running in c96251a4eff7
        Removing intermediate container c96251a4eff7
         ---> ea04d95dee9c
        Step 7/15 : COPY $CFILE testDir/
         ---> e74cc106d17d
        Step 8/15 : ARG foo
         ---> Running in a09c9dc14781
        Removing intermediate container a09c9dc14781
         ---> c02e8056bbb0
        Step 9/15 : ENV foo=${foo}
         ---> Running in 7d1dcf19c1ab
        Removing intermediate container 7d1dcf19c1ab
         ---> 9140004e03a5
        Step 10/15 : ARG EPORT
         ---> Running in dcb03d8e4265
        Removing intermediate container dcb03d8e4265
         ---> d82c63f930a3
        Step 11/15 : EXPOSE $EPORT
         ---> Running in c5919d5da3e4
        Removing intermediate container c5919d5da3e4
         ---> fdeaae486947
        Step 12/15 : ARG USER
         ---> Running in a46039ab199a
        Removing intermediate container a46039ab199a
         ---> fc206960845f
        Step 13/15 : USER $USER
        
        Stderr:   unable to find user testUser: no matching entries in passwd file
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    --- FAIL: TestDockerSuite/TestBuildBuildTimeArgExpansion (2.53s)

=== RUN   TestDockerSuite/TestBuildEnvironmentReplacementUser
    docker_cli_build_test.go:65: assertion failed: 
        Command:  /usr/local/cli/docker build -t testbuildenvironmentreplacement -
        ExitCode: 1
        Error:    exit status 1
        Stdout:   Sending build context to Docker daemon  2.048kB

        Step 1/3 : FROM scratch
         ---> 
        Step 2/3 : ENV user foo
         ---> Running in eaea0205ec60
        Removing intermediate container eaea0205ec60
         ---> adc615e75217
        Step 3/3 : USER ${user}
        
        Stderr:   unable to find user foo: no matching entries in passwd file
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    --- FAIL: TestDockerSuite/TestBuildEnvironmentReplacementUser (0.54s)

[thaJeztah]

oh, actually; I think those tests only define a user, but the user doesn't exist in the container; I guess previously that didn't cause an issue if no RUN was executed after? (or something along those lines 🤔)

[thaJeztah]

(sorry for my rambling) It could have revealed another issue; I see the Dockerfile defines a WORKDIR in an early step, but that's before the USER directive; could it be it's trying to chown the WORKDIR after the user has changed? (if so, that's probably not correct, as the WORKDIR was defined before the switch of user 🤔

[ndeloof]

AFAICT the issue is that getUser search for unix user in /etc/password on container filesystem. both USER and WORKDIR directives define container's metadata, but don't actually run adduser or equivalent to create a unix user that could later be retrieved.
Doesn't seem there's a possible workaround for this :'(

[thaJeztah]

but don't actually run adduser or equivalent to create a unix user that could later be retrieved.

So that part is expected; when specifying a user, it's expected for that user to exist.

The thing I'm a bit confused about (and the bit that is a bit unclear, and should be looked into - also to compare what BuildKit does)

• If WORKDIR is only metadata, then the "implicit" creation of the WORKDIR (if it doesn't exist) won't be executed until one of these;
  • the first ADD after the WORKDIR is set
  • the first COPY after the WORKDIR is set
  • the first RUN after the WORKDIR is set
• Which means that the WORKDIR would be create with the USER at that point in the Dockerfile 🤔

FROM busybox
USER 123:456

# this *defines* the WORKDIR (at this point, USER is 123:456
# but... it doesn't _create_ the workdir (?) so only metadata
WORKDIR /foo
USER 456:789

# here the WORKDIR is needed, so will be created with owner 456:789 ?
RUN echo hello

For the failing test; the Dockerfile looks like;

FROM busybox
ARG WDIR
WORKDIR ${WDIR}
ARG AFILE
ADD ${AFILE} testDir/
ARG CFILE
COPY $CFILE testDir/
ARG foo
ENV foo=${foo}
ARG EPORT
EXPOSE $EPORT
ARG USER
USER $USER

What I don't understand is why that passed before;

If USER is only metadata; at what moment is it expected to be resolved to a uid / gid? (and how / when does BuildKit do this?)

1. When USER is specified: resolve the USER (if it's a name, convert to uid:gid)
2. OR On the first RUN after USER? (first use)

The above would mean that there may be different outcomes, depending on the order

[thaJeztah]

First of all, the "fun" bit in this PR is of course, that the PR was intended to change docker run --workdir, but due to the Classic Builder also using the same function, the docker build tests started failing; posibly we need to look into some of that (more later).

I chatted about the failures with @tonistiigi in the maintainers meeting, and it was a fun trip down "memory" lane.

• in the past, both WORKDIR and USER were intended to be metadata-only changes
• for USER, the given username/uid and (optionally) groupname/gid are stored in the image-config, and do not have to evaluated until used (RUN / docker run).
  • This could allow an image to set a user that does not yet exist, which is both "bad" (image could potentially have an invalid user) and a "feature" (docker run -v /etc/passwd:/etc/passwd myimage would evaluate the user the moment the container is started).
• For WORKDIR, this changed when we added support for Windows containers; Windows was not able to make metadata-only changes, and required the WORKDIR to be an existing directory. Because of that, every step in the Dpckerfile (even metadata-only changes) now needed a container, and the WORKDIR to be created (as part of the WORKDIR instruction). For consistency, this change was made for both Windows and Linux containers / images.
• As a side-effect;
  • the WORKDIR was now thus created immediately
  • but (see this PR), also recreated on every Dockerfile instruction that created a container (see above; effectively, every instruction (?))

However, before this PR, the user that was used to create the WORKDIR was always root:root (or the equivalent on Windows), so even though that user was always looked up, it would never fail (as root always exists). But now that this PR starts to look up the user, it starts failing on every Dockerfile instruction.

This is indeed a tricky one (besides that we are wasting time re-looking up, and re-creating the WORKDIR), so we need to do some thinking how to resolve.

• One option could be to make sure that the WORKDIR instruction creates the container with different options (or handles the creation itself)
• Another option could be to have an internal createContainer() that is specific for docker build (classic builder) and/or that we adjust the options that are passed (to be looked into if that's an option)

[bigfoot90]

Any news on this? I'm also facing with this issue