Skip to content
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

Open
wants to merge 7 commits into
base: branch-23.11
Choose a base branch
from

Conversation

drobison00
Copy link
Contributor

@drobison00 drobison00 commented Apr 27, 2022

Resolves #22

Now running

DOCKER_EXTRA_ARGS="--build-arg MORPHEUS_USER=[YOUR_USER_NAME]" ./docker/build_container_dev.sh

Will create a new default user for the development container, with sudo privileges.

@cwharris
Copy link
Contributor

cwharris commented Apr 29, 2022

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 pdmack changed the title Update docker build to support adding a non-root defaut user Update docker build to support adding a non-root default user May 2, 2022
@drobison00
Copy link
Contributor Author

drobison00 commented May 2, 2022

@cwharris
Running as root within a container will stop you from mounting X11 into the container or easy GUI applications, I use it for debugging.

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 drobison00 added enhancement Additional functionality added to an existing feature and removed 5 - Ready to Merge labels May 2, 2022
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a 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
Copy link
Contributor

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

Copy link
Contributor Author

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.

Comment on lines +204 to +214
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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Comment on lines +93 to +97
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
```
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mdemoret-nv
Copy link
Contributor

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>
@mdemoret-nv mdemoret-nv changed the base branch from branch-22.06 to branch-22.08 June 24, 2022 07:11
@mdemoret-nv mdemoret-nv requested review from a team as code owners June 24, 2022 07:11
@mdemoret-nv
Copy link
Contributor

Closing since this is out of date. Can reopen with updated code.

@pdmack pdmack self-assigned this Nov 7, 2022
@pdmack
Copy link
Contributor

pdmack commented Nov 7, 2022

Gonna poke at this again

@pdmack pdmack reopened this Nov 7, 2022
@pdmack
Copy link
Contributor

pdmack commented Nov 7, 2022

Might create another PR based on 22.11

mdemoret-nv pushed a commit to mdemoret-nv/Morpheus that referenced this pull request Jun 9, 2023
@mdemoret-nv mdemoret-nv changed the base branch from branch-22.08 to branch-23.11 July 14, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additional functionality added to an existing feature feature request New feature or request non-breaking Non-breaking change
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[FEA] Add ability to build containers that default to a user other than root.
5 participants