Update docker build to support adding a non-root default user #68
Conversation
…r dev container. Builds passing
drobison00
requested review from
BartleyR,
mdemoret-nv,
pdmack and
raykallen
as code owners
drobison00
added
feature request
|
What's the purpose of this? I would have expected a non-root "morpheus" user, but I'm confused why it is useful to specify an arbitrary username at container build time via an arg. Is this so we can use our host user name to avoid chown issues with mounted dirs? |
pdmack
changed the title
|
@cwharris Another significant issue is that it is really annoying to run a dev container and have it create files in your current host user directory as the 'root' user. This allows someone to create the container with a user name/id that matches the user id of the user launching the container, so the permissions align; however, I should actually add a way to specify the UID/GID of the created user as well, I'll update the MR. For more detailed discussion: https://jtreminio.com/blog/running-docker-containers-as-current-host-user/ |
drobison00
added
enhancement
There was a problem hiding this comment.
This looks like a nice improvement. Have a few suggestions. Before merging, I would like to get @pdmack to weigh in on this one regarding any packaging/security concerns this may raise.
| fi | ||
|
|
||
| # Allow default user to execute sudo commands in container | ||
| RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers |
There was a problem hiding this comment.
Shouldnt this go in the else branch of the above if statement? Instead of the line echo "Skipping user creation..." ;\. This would save a layer
There was a problem hiding this comment.
Yeah, good catch. I'll move it into the check.
| RUN if [ ! -z "${MORPHEUS_USER}" ] && [ "${MORPHEUS_USER}" != "root" ] ; then \ | ||
| useradd -ms /workspace ${MORPHEUS_USER} \ | ||
| && usermod -aG sudo ${MORPHEUS_USER} \ | ||
| && apt-get update \ | ||
| && apt-get install -y sudo \ | ||
| && apt-get clean \ | ||
| && echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers ; \ | ||
| else \ | ||
| echo "Skipping user creation..." ;\ | ||
| fi | ||
|
|
There was a problem hiding this comment.
Can we move this into one of the above layers (such as base or conda_env) to avoid duplicating it in both the development and runtime targets? We dont need to set the USER until needed but we certainly can create the user in the base image.
There was a problem hiding this comment.
Yes, this makes sense. I'll move it up.
| 1. If you would like to build the container to have a default user, other than root, add the following to your | ||
| `build_container_dev.sh` call. | ||
| ```bash | ||
| DOCKER_EXTRA_ARGS="--build-arg MORPHEUS_USER=[YOUR_USER_NAME]" ./docker/build_container_dev.sh | ||
| ``` |
There was a problem hiding this comment.
I think this is pretty safe to include by default in the build_container_dev.sh and omit from build_container_release.sh is it not? We could just add the following to build_container_dev.sh:
DOCKER_EXTRA_ARGS="${DOCKER_EXTRA_ARGS} --build-arg MORPHEUS_USER=$USER"
Or better yet, make a new option DOCKER_IMAGE_USER in build_container.sh which will set the new build arg.
There was a problem hiding this comment.
Generally speaking, it probably makes sense to specify a user for the release container too, just from a security standpoint, since we do things like mount volumes by default. We could set it to something default, like 'morpheus'. I don't think there is any downside to doing so.
|
Also, it's worth noting that the Docker best practices guide cautions against installing sudo in a container and suggests an alternative: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#user |
Co-authored-by: Michael Demoret <[email protected]>
|
Closing since this is out of date. Can reopen with updated code. |
|
Gonna poke at this again |
|
Might create another PR based on 22.11 |
Resolves #22
Now running
Will create a new default user for the development container, with sudo privileges.