-
Notifications
You must be signed in to change notification settings - Fork 145
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
Update docker build to support adding a non-root default user #68
base: branch-23.11
Are you sure you want to change the base?
Conversation
…r dev container. Builds passing
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? |
@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/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <42954918+mdemoret-nv@users.noreply.github.com>
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.