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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5427f06
adding sr229s
satlus Apr 12, 2019
9fe6c36
Fixed the entrypoint copy command
satlus Apr 12, 2019
051b5ea
Renaming entrypoint to get free syntax highlighting
satlus Apr 13, 2019
09c5556
Docker file updated to point to entrypoint.sh
satlus Apr 13, 2019
908d293
Merge branch 'master' into satlus-nonrootuser
satlus Apr 25, 2019
1504dfd
Removed entrypoint without .sh extension
satlus Apr 25, 2019
42bb4da
added code-server dir to ignore list
satlus Apr 25, 2019
efe1ca8
entrypoint.sh is working for defaults `docker run` in README
satlus Apr 25, 2019
70424c8
There was an attempt.. it seems the usermod approach won't work
satlus Apr 26, 2019
4878c74
Merge remote-tracking branch 'upstream/master' into satlus-nonrootuser
satlus Apr 26, 2019
3190626
Adding docker run commands used for testing
satlus Apr 26, 2019
08ca5be
fixing bug in usermod -g
satlus Apr 26, 2019
81ec63b
Merge branch 'master' into satlus-nonrootuser
satlus May 3, 2019
7ed7305
non root execution works via boxboat/fixuid
satlus May 3, 2019
b328da9
Fixuid is now configurable via entrypoint script
satlus May 3, 2019
dfce54b
I added back the mkdir for /home/coder/project
satlus May 4, 2019
b232cc5
Fixed lowercase COPY instruction
satlus May 5, 2019
daa96b8
WORKDIR no longer required, added support for externally mounting /ho…
satlus May 6, 2019
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/lib
code-server
node_modules
dist
out
Expand Down
14 changes: 14 additions & 0 deletions DockerLaunchCommands.MD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Docker launch commands for coder
Copy link

Choose a reason for hiding this comment

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

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.


## default coder uid:guid
docker run -it -p 127.0.0.1:8443:8443 \
-v "${PWD}/code-server:/home/coder/project" \
satlus-code-server:latest --allow-http --no-auth

## passing `id(-g|-u)` through the system
docker run -it -p 127.0.0.1:8443:8443 \
-v "${PWD}/code-server:/home/coder/project" \
-v "${PWD}/code-server/.cache:/home/coder/.cache" \
-v "${PWD}/code-server/.local:/home/coder/.local" \
-u $(id -u):$(id -g) \
satlus-code-server:latest --allow-http --no-auth
18 changes: 14 additions & 4 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,24 @@ 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.

Copy link

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.

Copy link

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
Copy link

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.

Copy link

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 •
edited
Loading

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.


WORKDIR /home/coder/project
WORKDIR /home/coder/workdir

# 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 +60,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", "fixuid", "code-server"]