Support non-root users (runtime UID remapping) #640

satlus · 2019-05-03T15:44:37Z

Describe in detail the problem you had and how this PR fixes it

Our environment restricts docker execution to images that can only with a non root user docker run -u $UID.... This PR adds boxboat/fixuid to allow for runtime mapping of host->container UID

Is there an open issue you can link to?

#439

Checking this non working code for the record. The idea is to usermod to change the uid:gid in entrypoint script. There are two approaches I took to do this: 1. pass the -u flag to docker run and try to do the usermod. Won't work because we're not in the passwd file aand so user mod fails 2. pass env vars with the host uid:gid you want to run as, and then usermod to change in-flight (fails, as we're trying to change the uid while logged in/running proc It seems eding the container /etc/passwd file is the only workable approach. One more thing to try is to run eentrypoint.sh before dumbinit - will do that next

I added fixuid to the image to support non root execution.

DockerLaunchCommands.MD

ibnesayeed · 2019-05-03T15:55:51Z

Dockerfile

 # We create first instead of just using WORKDIR as when WORKDIR creates, the user is root.
-RUN mkdir -p /home/coder/project
+RUN mkdir -p /home/coder/workdir


Is renaming the directory necessary here? I don't see any problem, but there might be documentation elsewhere which might go out of sync due to this change.

I think it's a good idea, especially if we want to restrict container FS writes to the coder user. We're making the work directory a sub-path of the $HOME of the coder directory, as all docker instructions will be invoked as the coder user at this point. It also means that we can volume mount the /home/coder/project directory without risking masking/overwriting the entrypoint.sh script.

If you are creating a yet another directory, shouldn't one already existing be left alone as it is? If you are simply renaming it to something else in the same location then I am not sure what are you gaining from it, but I might be missing something.

check my latest commit - the workdir is now used only to hold the entrypoint.sh script

I think you are missing my point here. There was a /home/coder/project directory before where users were supposed to mount their code from the host machine. You have simply replaced it with a new name. You should have left it as it was and created /home/coder/workdir (or something more appropriately named) directory next to it where you can place your entrypoint script so it does not get overwritten. In my opinion, a better option would be to leave the directory as they were before your PR and place your entrypoint script in something like /usr/local/bin/ so you can refer to it without an absolute path from anywhere and have the least chance of being overwritten due to bind mounts.

Dockerfile

ibnesayeed · 2019-05-03T15:59:16Z

DockerLaunchCommands.MD

@@ -0,0 +1,14 @@
+# Docker launch commands for coder


You might want to fix the title cases of headings and subheads to match with the style of the README file. Also, I personally do no like upper-case file extensions (as ignore patterns are easier if extensions are kept lower-cased) and CamelCased file names, but that could just me.

nhooyr · 2019-05-03T16:26:07Z

Not sure if this is a good idea, the reamde for fixuid says:

fixuid should only be used in development Docker containers. DO NOT INCLUDE in a production container image

Not sure why.

ibnesayeed · 2019-05-03T16:32:11Z

Not sure if this is a good idea, the reamde for fixuid says:
fixuid should only be used in development Docker containers. DO NOT INCLUDE in a production container image
Not sure why.

@nhooyr editors are generally used in development environment, unless someone deploys it on a server and makes it available to a larger user-base.

ibnesayeed · 2019-05-03T16:44:02Z

@nhooyr: Not sure why.

boxboat/fixuid#1

satlus · 2019-05-03T20:35:41Z

I'm cleaning this up to make fixuid configurable via an entrypoint.sh script and disabled by default. If a user enables (via env var), it will log the warning message about non prod use. The user can disable this warning if the risk is acceptable (also via env var). I am testing this out and will update the PR asap.

As a note, we believe the risk is acceptable in our environment, particularly with other controls we have in place, and the fact this will be a developer only tool

Added the entrypoint script back in to make fixuid configurable. By default the container will run without invoking fixuid. If the user sets the FIXUID env var in the container it will set the coder user to UID:GID values passed by the `docker -u` cli argument. If the user also sets the FIXUID_QUIET env var, it will disable the warning message. Also cleaned up stuff based on comments on the PR

ibnesayeed · 2019-05-03T22:23:42Z

Dockerfile

 # We create first instead of just using WORKDIR as when WORKDIR creates, the user is root.
-RUN mkdir -p /home/coder/project
+RUN mkdir -p /home/coder/workdir
+copy entrypoint.sh /home/coder/workdir/


Suggested change

copy entrypoint.sh /home/coder/workdir/

COPY entrypoint.sh /home/coder/workdir/

Docker instructions are written in upper case.

ibnesayeed · 2019-05-03T22:30:24Z

Dockerfile

 # We create first instead of just using WORKDIR as when WORKDIR creates, the user is root.
-RUN mkdir -p /home/coder/project
+RUN mkdir -p /home/coder/workdir


I think you are missing my point here. There was a /home/coder/project directory before where users were supposed to mount their code from the host machine. You have simply replaced it with a new name. You should have left it as it was and created /home/coder/workdir (or something more appropriately named) directory next to it where you can place your entrypoint script so it does not get overwritten. In my opinion, a better option would be to leave the directory as they were before your PR and place your entrypoint script in something like /usr/local/bin/ so you can refer to it without an absolute path from anywhere and have the least chance of being overwritten due to bind mounts.

satlus · 2019-05-04T02:27:19Z

@ibnesayeed - appreciate the feedback, but if you check my latest commit in the Dockerfile, the VOLUME bind mount is pointed to /home/coder/project, as per the original in master. The WORKDIR is a different directory, where the entrypoint.sh is copied (and only used for that purpose). What's the issue?

ibnesayeed · 2019-05-04T02:58:24Z

@satlus: @ibnesayeed - appreciate the feedback, but if you check my latest commit in the Dockerfile, the VOLUME bind mount is pointed to /home/coder/project, as per the original in master. The WORKDIR is a different directory, where the entrypoint.sh is copied (and only used for that purpose). What's the issue?

Removing the mkdir command and only relying on WORKDIR has the following side effect as per an existing comment:

# We create first instead of just using WORKDIR as when WORKDIR creates, the user is root.

satlus · 2019-05-04T11:39:51Z

I added it back in, but per the commit history the only reason this was needed in the past was because the WORKDIR and VOLUME paths resolved to the same location - /home/coder/project. This is no longer the case (as it should be), and workdir is only used to house the entrypoint. There really is no need for this going forward IMO.

Funnily, in testing, without the mkdir, the WORKDIR was still created as the coder user. Here's an example from a first start with fixuid disabled:

coder@d13691a6f654:~$ cd /home/coder
coder@d13691a6f654:~$ ls -la
total 36
drwxr-xr-x 1 coder coder 4096 May  4 10:45 .
drwxr-xr-x 1 root  root  4096 May  3 15:17 ..
-rw-r--r-- 1 coder coder  220 May  3 15:17 .bash_logout
-rw-r--r-- 1 coder coder 3771 May  3 15:17 .bashrc
drwxr-xr-x 3 coder coder 4096 May  4 10:45 .cache
drwxr-xr-x 3 coder coder 4096 May  4 10:45 .local
-rw-r--r-- 1 coder coder  807 May  3 15:17 .profile
drwxr-xr-x 4 coder coder  128 Apr 25 16:47 project
-rw------- 1 coder coder 1024 May  4 10:45 .rnd
drwxr-xr-x 1 coder coder 4096 May  3 21:08 workdir <== THIS
coder@d13691a6f654:~$

When fixuid is enabled it will recursively chown the home directory of the user, including workdir, making this even less important.

ibnesayeed · 2019-05-04T12:09:47Z

@satlus in that case I would get rid of that mkdir command and the comment that says why it was important. Personally, I know WORKDIR and some other Docker commands run in the scope of the user if a USER is declared in a prior instruction, but the comment was confusing me.

That said, I would perhaps COPY the entrypoint.sh file into /usr/local/bin/. This would:

No need to create yet another directory specifically for this
Minimize the chances being overwritten if the user chooses the mount the whole HOME directory inside
The ENTRYPOINT instruction can be changed to ["dumb-init", "entrypoint.sh", "code-server"] without the need of absolute or relative path

…me/coder The entrypoint.sh script is now copied to /usr/local/bin, companion to the code-server binary. The WORKDIR directory is no longer created as nothing is copied to it. Lastly, we've modified the external volume mount to export the entire home directory. This will avoid UFS layer creation for .cache and .local directory writes, which appears to happen by default.

satlus · 2019-05-06T01:33:02Z

@satlus in that case I would get rid of that mkdir command and the comment that says why it was important. Personally, I know WORKDIR and some other Docker commands run in the scope of the user if a USER is declared in a prior instruction, but the comment was confusing me.

That said, I would perhaps COPY the entrypoint.sh file into /usr/local/bin/. This would:

@ibnesayeed Good suggestions, implemented.

In testing I noticed that we'd have to provide explicit volume mounts for /home/coder/.cache and /home/coder/.local when enabling non-root use. This seems less than ideal from a usability perspective, and as it has the effect of writing transient data inside of the container by default. To mitigate this I went ahead and changed the VOLUME mount to export the entire /home/coder directory (and updated docker CLI run examples in the README). This seems like smarter way to ensure config and runtime data are separated from the container image appropriately.

Would definitely appreciate the committers commenting here - it seems this is getting a bit broader in scope than originally anticipated.

ibnesayeed · 2019-05-06T02:51:15Z

Dockerfile

-WORKDIR /home/coder/project
+# Setup our entrypoint
+COPY entrypoint.sh /usr/local/bin/
+RUN sudo chmod +x /usr/local/bin/entrypoint.sh


These two COPY and RUN commands can be moved above the USER command so that the entrypoint.sh file is copied as root under the /usr/local/bin/ directory and chmod needs no sudo.

Good call, will be in the next commit

ibnesayeed · 2019-05-06T02:52:19Z

README.md

@@ -9,7 +9,7 @@

 Try it out:
 ```bash
-docker run -it -p 127.0.0.1:8443:8443 -v "${PWD}:/home/coder/project" codercom/code-server --allow-http --no-auth
+docker run -it -p 127.0.0.1:8443:8443 -v "${PWD}/code-server:/home/coder" codercom/code-server --allow-http --no-auth


I am not too sure about this change. Are we assuming that there will be code-server available under ${PWD}/?

It will be created if it doesn't exist - I think it's better than polluting the $PWD with the rnd, .local and cache as it does now.

ibnesayeed · 2019-05-06T02:55:41Z

Dockerfile


 # This assures we have a volume mounted even if the user forgot to do bind mount.
 # So that they do not lose their data if they delete the container.
-VOLUME [ "/home/coder/project" ]
+VOLUME [ "/home/coder" ]


Can we change this to:

Suggested change

VOLUME [ "/home/coder" ]

VOLUME [ "/home/coder/project", "/home/coder/.cache", "/home/coder/.local" ]

Instead of creating a volume for the whole home directory?

Yes - will test and update tomorrow

Saw some odd things after testing this change and i'm not sure we should go this route. The first is that if we decide to bind mount /home/coder, to have the full home directory on the local filesystem, duplicate folders are created:

code-server$ !find find ./code-server/ -maxdepth 1 -type d ./code-server/ ./code-server//project, ./code-server//.local ./code-server//.local, ./code-server//.cache, ./code-server//.cache

The second is comma suffixed directories are dupes and don't have data. I'm wondering if this is a side-effect of us bind mounting a parent path of previously defined volume containers. Taking a step back, do we really want to create three anonymous volume containers here by default? I'd rather just have the home directory be portable as an anonymous volume container, and recommend a compose file or docker run commands where the volumes are explicitly created for this purpose.

sr229

Some changes.

sr229 · 2019-06-17T15:26:42Z

Dockerfile

@@ -34,20 +34,29 @@ RUN locale-gen en_US.UTF-8
 # configured in /etc/default/locale so we need to set it manually.
 ENV LC_ALL=en_US.UTF-8

-RUN adduser --gecos '' --disabled-password coder && \
+RUN addgroup --gid 1000 coder && \


by default the first user to be made in Docker build is 1000 already, I don't see the need for extra steps.

by default the first user to be made in Docker build is 1000 already

This is false. Docker does not enforce a default uid of 1000. The Node images that are being used as a base image however do create a node user of 1000:1000.

That said, this doesn't need to create a new user for 1000:1000, I'm not sure what is up with all the other options in addition to that. The intention appears to be replacing node with coder which is used later on. Looks like they're referencing this addition from fixuid installation docs without much thought..

sr229 · 2019-06-17T15:27:13Z

Dockerfile

 	echo "coder ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers.d/nopasswd

+RUN USER=coder && \


use su -c instead or gosu.

gosu is actually preferred as it mirrors --user flag exactly and fixes weird tty and signal forwarding issues. See https://github.com/tianon/gosu. FYI Tianon creates a lot of official Docker Images on hub.docker.com.

@sr229 @SuperSandro2000 this is setting temporary env vars for USER and GROUP as per instructions of how to use fixuid, which afaik is only intended for the last line in the command:

printf "user: $USER\ngroup: $GROUP\n" > /etc/fixuid/config.yml

Rather than add an additional config file externally. Has nothing to do with gosu, which you say to use "instead of", but there is no mention of gosu in the Dockerfile you're reviewing...?(unless there was in prior commit history, doesn't seem like it since the review from @sr229 is June 18th and last commit was May 6th)

Just in case there is any confusion, fixuid is for remapping existing file ownership of the config user:group to the given uid:gid from --user flag in docker run command. gosu is intended for dropping privileges from root to another user to run a command as instead:

The core use case for gosu is to step down from root to a non-privileged user during container startup (specifically in the ENTRYPOINT, usually). - Source

@polarathene I was referring to sr229.

ptoulouse · 2019-06-19T19:58:27Z

As an alternative solution to fixuid, you can differ the creation of the coder user and group to the entrypoint.sh script.

Create the user based on a user id and group id environment variable (often seen: PUID, PGID) with default values of 1000 and 1000: PUID=${PUID:-1000}, PGID=${PGID:-1000}
chown -R coder:coder /whatever/directory
runuser -u coder -- code-server-command

satlus · 2019-07-12T15:36:09Z

As an alternative solution to fixuid, you can differ the creation of the coder user and group to the entrypoint.sh script.

Create the user based on a user id and group id environment variable (often seen: PUID, PGID) with default values of 1000 and 1000: PUID=${PUID:-1000}, PGID=${PGID:-1000}

chown -R coder:coder /whatever/directory

runuser -u coder -- code-server-command

We looked at a similar strategy but it won't work if we want init.d in the container to be non-root (the entire point of passing -u to docker at runtime). Unless I'm missing something, this won't satisfy the requirement.

carlos-sarmiento · 2019-08-02T17:02:07Z

Any chance of this PR landing anytime soon? I'm facing this issue and would love to have a solution

sr229 · 2019-08-02T17:15:37Z

@carlos-sarmiento from what info I can gather, PRs are on hold until we can merge v2. If you really want to test, chinodesuuu/coder:satlus is a feature parity replica of this PR.

carlos-sarmiento · 2019-08-03T05:54:44Z

@sr229 is there any idea on when v2 might land?

sr229 · 2019-08-03T06:00:08Z

@sr229 is there any idea on when v2 might land?

v2 is quite in early stages, and barely feature-parity with v1, mainly because of the new VSCode Web architecture (I believe which is VS Online), you can check #857.

nhooyr

@satlus can you try opening a largish project? Does fixuid slow container startup down considerably? iirc that's why I remember not integrating it.

satlus · 2019-08-27T15:47:07Z

@satlus can you try opening a largish project? Does fixuid slow container startup down considerably? iirc that's why I remember not integrating it.

Happy to do some testing here, but I can see how fixuid's behavior could slow startup significantly (basically to recursively chown all files from /) . To mitigate this on the fixuid side we can restrict paths to recurse via fixuid's 'path' env or yaml config (https://github.com/boxboat/fixuid#specify-paths-and-behavior-across-devices), but if the project directory is large it may be a fixed cost we'd have to eat. Two things to consider (1) it's opt in via env var, in which case there is zero change to the default behavior, (2) if given the option of not running at all in a restricted environment, and having something work with slow startup time, some users may be willing tolerate the slow startup. We should at least warn users of the side effect in this case. That aside, I will find some time in the next few days to test things out. I'm also curious if after the initial fixuid chown we can get away without invoking it on subsequent container starts.

nhooyr · 2019-09-20T18:45:23Z

Our environment restricts docker execution to images that can only with a non root user docker run -u $UID.... This PR adds boxboat/fixuid to allow for runtime mapping of host->container UID

If you run with UID 1000, things should always just work as the image itself always sets itself to run with uid 1000 by default. You can always access root with sudo.

https://github.com/cdr/code-server/blob/22058c5f861d6f39bc42a768d5cb65ffdf696ed1/Dockerfile#L46

Is that not enough for your use case?

nhooyr · 2019-09-20T18:50:55Z

I guess what I'm really suggesting is ensuring your files/folders outside the container are also UID 1000 as that's the only real way to do this without jank with Docker.

nhooyr · 2019-09-20T18:53:03Z

The other being using user namespacing and explicitly mapping some external UID to UID 1000 inside the container. See https://docs.docker.com/engine/security/userns-remap/

ptoulouse · 2019-09-20T19:00:46Z

I guess what I'm really suggesting is ensuring your files/folders outside the container are also UID 1000 as that's the only real way to do this without jank with Docker.

What if I am not user 1000 on the base system? I won't be able to read the files. What about systems with multiple users? Linuxserver,io has solved the issue using s6_overlay.

nhooyr · 2019-09-20T19:06:53Z

What if I am not user 1000 on the base system? I won't be able to read the files.

You're using docker, so you should be accessing the files inside the container. If you need to still access them outside the container, use sudo or start the container with the entrypoint as bash. It's the way docker works best right now.

sr229 · 2020-01-22T12:31:17Z

@code-asher Let's cherry-pick this PR then pull a new PR that adds this PR's changes.

nhooyr · 2020-01-27T18:49:00Z

I'm gonna take care of this myself in the coming days.

Thank you @satlus and @ibnesayeed for your work.

satlus added 14 commits April 12, 2019 10:02

adding sr229s

5427f06

Fixed the entrypoint copy command

9fe6c36

Renaming entrypoint to get free syntax highlighting

051b5ea

Docker file updated to point to entrypoint.sh

09c5556

Merge branch 'master' into satlus-nonrootuser

908d293

Removed entrypoint without .sh extension

1504dfd

added code-server dir to ignore list

42bb4da

entrypoint.sh is working for defaults docker run in README

efe1ca8

Merge remote-tracking branch 'upstream/master' into satlus-nonrootuser

4878c74

Adding docker run commands used for testing

3190626

fixing bug in usermod -g

08ca5be

Merge branch 'master' into satlus-nonrootuser

81ec63b

non root execution works via boxboat/fixuid

7ed7305

I added fixuid to the image to support non root execution.

satlus requested review from code-asher, kylecarbs and nhooyr as code owners May 3, 2019 15:44

satlus mentioned this pull request May 3, 2019

Support docker run with --user $(id -u):$(id -g) cli arg (shared process error) #439

Closed

ibnesayeed suggested changes May 3, 2019

View reviewed changes

nhooyr mentioned this pull request May 3, 2019

will not work on multi user systems coder/sail#52

Closed

ibnesayeed suggested changes May 3, 2019

View reviewed changes

I added back the mkdir for /home/coder/project

dfce54b

satlus added 2 commits May 5, 2019 09:30

Fixed lowercase COPY instruction

b232cc5

ibnesayeed suggested changes May 6, 2019

View reviewed changes

sr229 suggested changes Jun 17, 2019

View reviewed changes

nhooyr mentioned this pull request Jun 19, 2019

GH-673: Revert "Remove chmod on project dir" #679

Closed

nhooyr reviewed Aug 19, 2019

View reviewed changes

nhooyr mentioned this pull request Sep 20, 2019

Enforce user permissions for user data and extension dirs on container startup #992

Closed

sr229 assigned sr229 and code-asher Jan 22, 2020

nhooyr closed this Jan 27, 2020

codsane mentioned this pull request Apr 19, 2020

fix coder Cloudbox/Community#218

Closed

	copy entrypoint.sh /home/coder/workdir/
	COPY entrypoint.sh /home/coder/workdir/

	VOLUME [ "/home/coder" ]
	VOLUME [ "/home/coder/project", "/home/coder/.cache", "/home/coder/.local" ]

		echo "coder ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers.d/nopasswd

		RUN USER=coder && \

Support non-root users (runtime UID remapping) #640

Support non-root users (runtime UID remapping) #640

Conversation

satlus commented May 3, 2019

Describe in detail the problem you had and how this PR fixes it

Is there an open issue you can link to?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibnesayeed May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhooyr commented May 3, 2019

ibnesayeed commented May 3, 2019

ibnesayeed commented May 3, 2019

satlus commented May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibnesayeed May 3, 2019 • edited Loading

Choose a reason for hiding this comment

satlus commented May 4, 2019

ibnesayeed commented May 4, 2019

satlus commented May 4, 2019

ibnesayeed commented May 4, 2019

satlus commented May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satlus May 6, 2019 • edited Loading

Choose a reason for hiding this comment

sr229 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptoulouse commented Jun 19, 2019

satlus commented Jul 12, 2019

carlos-sarmiento commented Aug 2, 2019

sr229 commented Aug 2, 2019

carlos-sarmiento commented Aug 3, 2019

sr229 commented Aug 3, 2019

nhooyr left a comment

Choose a reason for hiding this comment

satlus commented Aug 27, 2019

nhooyr commented Sep 20, 2019

nhooyr commented Sep 20, 2019

nhooyr commented Sep 20, 2019

ptoulouse commented Sep 20, 2019

nhooyr commented Sep 20, 2019

sr229 commented Jan 22, 2020

nhooyr commented Jan 27, 2020

ibnesayeed May 3, 2019 •

edited

Loading

satlus commented May 3, 2019 •

edited

Loading

ibnesayeed May 3, 2019 •

edited

Loading

satlus commented May 6, 2019 •

edited

Loading

satlus May 6, 2019 •

edited

Loading