Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/lib
code-server
node_modules
dist
out
Expand Down
21 changes: 15 additions & 6 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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 && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

adduser --uid 1000 --ingroup coder --home /home/coder --shell /bin/sh --disabled-password --gecos "" coder && \
echo "coder ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers.d/nopasswd

RUN USER=coder && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use su -c instead or gosu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polarathene I was referring to sr229.

GROUP=coder && \
curl -SsL https://github.com/boxboat/fixuid/releases/download/v0.4/fixuid-0.4-linux-amd64.tar.gz | tar -C /usr/local/bin -xzf - && \
chown root:root /usr/local/bin/fixuid && \
chmod 4755 /usr/local/bin/fixuid && \
mkdir -p /etc/fixuid && \
printf "user: $USER\ngroup: $GROUP\n" > /etc/fixuid/config.yml

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, will be in the next commit


# 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" ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - will test and update tomorrow

Copy link
Author

@satlus satlus May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


COPY --from=0 /src/packages/server/cli-linux-x64 /usr/local/bin/code-server
EXPOSE 8443

ENTRYPOINT ["dumb-init", "code-server"]
ENTRYPOINT ["dumb-init", "entrypoint.sh", "code-server"]
28 changes: 27 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

```

- Code on your Chromebook, tablet, and laptop with a consistent dev environment.
Expand All @@ -31,6 +31,32 @@ Use [sshcode](https://github.com/codercom/sshcode) for a simple setup.

See docker oneliner mentioned above. Dockerfile is at [/Dockerfile](/Dockerfile).

**Run as Non-root user dynamically mapped at runtime in docker**
You can configure code server to run as a UID:GID of your choice. This uses the [boxboat/fixuid](https://github.com/boxboat/fixuid) utility to dynmaically remap the coder uid/gid at runtime. This is especially useful in environments where UIDs change, affect volume mount permissions, and process ownership. You can enable this feature easily with env variables, and the `docker -u` cli flag.

WARNING: there are some concerns around [security](https://github.com/boxboat/fixuid/issues/1) with this approach, ensure you understand the implications

Example 1: Run as the host UID:GID, by setting the FIXUID docker env var
```bash
docker run -it -p 127.0.0.1:8443:8443 \
-v "${PWD}/code-server:/home/coder" \
-u $(id -u):$(id -g) \
-e FIXUID=y \
codercom/code-server:latest --allow-http --no-auth
```

Example 2: Same as above, but disable the fixuid warning message
```bash
docker run -it -p 127.0.0.1:8443:8443 \
-v "${PWD}/code-server:/home/coder" \
-u $(id -u):$(id -g) \
-e FIXUID=y \
-e FIXUID_QUIET=y \
codercom/code-server:latest --allow-http --no-auth
```



### Binaries

1. [Download a binary](https://github.com/cdr/code-server/releases) (Linux and OS X supported. Windows coming soon)
Expand Down
19 changes: 19 additions & 0 deletions entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/bin/bash

export HOME=/home/coder

if [[ -z $FIXUID ]];
then
echo "fixuid flag not set..."
else
echo "fixuid is set..."
if [[ -z $FIXUID_QUIET ]];
then
fixuid
else
fixuid -q
fi
fi

echo "starting coder..."
exec "$@"