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 15 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: 17 additions & 4 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,27 @@ 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
RUN mkdir -p /home/coder/workdir

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

@ibnesayeed ibnesayeed May 3, 2019

Choose a reason for hiding this comment

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

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.

copy entrypoint.sh /home/coder/workdir/

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Docker instructions are written in upper case.

RUN sudo chmod +x /home/coder/workdir/entrypoint.sh

WORKDIR /home/coder/workdir

WORKDIR /home/coder/project

# 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.
Expand All @@ -50,4 +63,4 @@ VOLUME [ "/home/coder/project" ]
COPY --from=0 /src/packages/server/cli-linux-x64 /usr/local/bin/code-server
EXPOSE 8443

ENTRYPOINT ["dumb-init", "code-server"]
ENTRYPOINT ["dumb-init", "/home/coder/workdir/entrypoint.sh", "code-server"]
26 changes: 26 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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}:/home/coder/project" \
-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}:/home/coder/project" \
-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 "$@"