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.

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

Add passwd/group file entries when running images as assigned user ID not in passwd file. #553

Closed
wants to merge 6 commits into from
Closed

Add passwd/group file entries when running images as assigned user ID not in passwd file. #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb9aeae
Add user/group entries when image is being run as arbitrarily assigne…
GrahamDumpleton Feb 15, 2018
fe721bf
Only add group file entry if the assigned UID is in list of groups as…
GrahamDumpleton Feb 15, 2018
0caa751
Merge branch 'master' of github.com:jupyter/docker-stacks into fix-pa…
GrahamDumpleton Feb 17, 2018
836c318
Merge branch 'master' of github.com:jupyter/docker-stacks into fix-pa…
GrahamDumpleton Feb 17, 2018
4b6de74
Only fix up passwrd/group entries when group ID is root.
GrahamDumpleton Feb 17, 2018
02187ee
Merge branch 'master' of github.com:jupyter/docker-stacks into fix-pa…
GrahamDumpleton Feb 17, 2018
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
7 changes: 6 additions & 1 deletion base-notebook/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,12 @@ ENV PATH=$CONDA_DIR/bin:$PATH \
ADD fix-permissions /usr/local/bin/fix-permissions
# Create jovyan user with UID=1000 and in the 'users' group
# and make sure these dirs are writable by the `users` group.
# Make /etc/passwd and /etc/group writable so can add entries
# for uid/gid when container run as arbitrarily assigned uid.
RUN useradd -m -s /bin/bash -N -u $NB_UID $NB_USER && \
mkdir -p $CONDA_DIR && \
chown $NB_USER:$NB_GID $CONDA_DIR && \
chmod g+w /etc/passwd /etc/group && \
Copy link
Member

Choose a reason for hiding this comment

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

This makes it trivial for any user to become root in their container:

cp /etc/passwd /tmp/passwd
rootpass=`openssl passwd -1 root`
cat /tmp/passwd | sed "s/root:x/root:${rootpass}/" > /etc/passwd

Now the user can su root with the password 'root'

I think we need another solution that doesn't grant universal su root access.

This does only affect putting users into the root group, so not the default case, but it does mean that putting users into the root group is equivalent to letting the users run as root. Is that the intended effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check, but I don't believe that will work in secure environments like Kubernetes/OpenShift where you are blocked from running setuid executables and various related system calls behind the ability to change real user/group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is restricted in Kubernetes/OpenShift.

jovyan@image:~$ su root
Password:
initgroups: Operation not permitted

This is because secure container environments will drop capabilities.

                "securityContext": {
                    "capabilities": {
                        "drop": [
                            "KILL",
                            "MKNOD",
                            "SETGID",
                            "SETUID"
                        ]
                    },
                    "privileged": false,
                    "runAsUser": 1000030000,
                    "seLinuxOptions": {
                        "level": "s0:c6,c0"
                    }
                },

fix-permissions $HOME && \
fix-permissions $CONDA_DIR

Expand Down Expand Up @@ -94,13 +97,15 @@ EXPOSE 8888
WORKDIR $HOME

# Configure container startup
ENTRYPOINT ["tini", "--"]
ENTRYPOINT ["container-entrypoint"]
CMD ["start-notebook.sh"]

# Add local files as late as possible to avoid cache busting
COPY start.sh /usr/local/bin/
COPY start-notebook.sh /usr/local/bin/
COPY start-singleuser.sh /usr/local/bin/
COPY container-entrypoint /usr/local/bin/
COPY fix-passwd-entry /usr/local/bin/
COPY jupyter_notebook_config.py /etc/jupyter/
RUN fix-permissions /etc/jupyter/

Expand Down
15 changes: 15 additions & 0 deletions base-notebook/container-entrypoint
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/bin/bash
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.

# Add entries to /etc/passwd and /etc/group if being run as user ID
# other than which the image has specified by the USER statement.

fix-passwd-entry
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for this logic to live in the start* scripts after tini runs instead of introducing additional scripts? I understand that Python and other programs may balk when the UID is not found in /etc/users and /etc/group, but does tini have that problem?

Simply looking to keep the number of separate workflow scripts we need to maintain low.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for adding it in an ENTRYPOINT script and doing it in there is that it gets applied even if someone overrides the CMD to do something else. For example, in JupyterHub, the CMD for the notebook images is overridden to execute jupyterhub-singleuser directly, bypassing start.sh.

Copy link
Member

Choose a reason for hiding this comment

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

This will depend on the Spawner. It is true of KubeSpawner, but isn't true of DockerSpawner in general (deployments may override CMD, but it is not overridden by default).

Doing this as an entrypoint makes sense to me. I think the same might be said for much of what's in start.sh, though. Effectively, putting this and only this in entrypoint is saying that the passwd entry is more important than and should be harder to opt-out of than anything in start.sh. That may be right, but I think we should think carefully about this in the future.


# Run 'tini' as a mini supervisor. This will manage the actual
# application process, passing on signals received by the container, and
# also reaping zombie processes. Must use 'exec' so that it inherits
# process ID 1 of the container.

exec tini -- "$@"
28 changes: 28 additions & 0 deletions base-notebook/fix-passwd-entry
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#!/bin/bash
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.

# By default the image would run as user ID 1000. If the user is run as
# an arbitrarily assigned user ID by the container platform it will be
# much higher and the lack of entries in /etc/passwd and /etc/group can
# cause failure of any Python code which doesn't tolerate having no
# entries for uid/gid. Assume that if running with larger uid that we
# need to add entries into the respective files for that user. Have the
# check be greater than 100000 to allow room in case custom images
# derived from this image were adding additional explict users/groups of
# their own.

NB_UID=`id -u`
NB_GID=`id -g`

if [ $NB_GID -eq 0 -a $NB_UID -ge 100000 ]; then
cat /etc/passwd | sed -e "s/^jovyan:/nayvoj:/" > /tmp/passwd
echo "jovyan:x:$NB_UID:$NB_GID:,,,:/home/jovyan:/bin/bash" >> /tmp/passwd
cat /tmp/passwd > /etc/passwd
rm /tmp/passwd

id -G | grep -q -w $NB_UID; STATUS=$?
if [ $STATUS -eq 0 ]; then
echo "jovyan:x:$NB_UID:" >> /etc/group
fi
Copy link
Member

Choose a reason for hiding this comment

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

We have similar logic to this in start.sh, and it currently enables:

  • setting user id
  • setting user name
  • granting sudo access

The start.sh logic requires launching the image as the root user rather than the root group. Effectively, we have a fork of two different behaviors with the same goal - one dedicated for openshift/some kubernetes deployments (this one) and one for most others (in start.sh). They accomplish the same thing in different ways and in different code paths. Perhaps we should reconcile them. It would probably be nice if we could accomplish the existing root-requiring logic in start.sh via root group rather than root user. Would you like to take a stab at that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem still gets back to fact that JupyterHub only runs jupyterhub-singleuser and not start-singleuser.sh. Because of my use case for JupyterHub that isn't a complete stumbling block though as it is already necessary to configure KubeSpawner to set supplemental group of users on the container since docker-stacks images aren't setup to use group root for everything, but rely on groups users when user ID is not 1000. Since already doing that, no drama to also override the startup command to be start-singleuser.sh. Since it internally uses start.sh to run jupyterhub-singleuser then integrating it with start.sh is probably okay.

I can't see that this will change the need to make passwd and group files writable to root group. The only way around that is messy and requires compiled C programs that are setuid so can become effective user of root, but not real user as kernel setuid() call blocked, such that can update files in that C program. Am not confident that isn't blocked by some configurations for running containers.

Even if make passwd and group writable, my recollection is that commands like usermod and groupmod don't work as that change is not enough. When I tested that it may have been on CentOS though so possible that Debian is different. So chance that have to still make changes to passwd and group manually. I will do some more testing and see what can be done using start.sh.

fi