Rework of start.sh script, take 2

base-notebook/start.sh

@@ -3,6 +3,7 @@
 # Distributed under the terms of the Modified BSD License.
 
 set -e
+echo "Running: start.sh" "$@"
 
 # Exec the specified command or fall back on bash
 if [ $# -eq 0 ]; then
@@ -11,140 +12,186 @@ else
     cmd=( "$@" )
 fi
 
+# The run-hooks function looks for .sh scripts to source and executable files to
+# run within a passed directory.
 run-hooks () {
-    # Source scripts or run executable files in a directory
     if [[ ! -d "${1}" ]] ; then
         return
     fi
-    echo "${0}: running hooks in ${1}"
+    echo "${0}: running hooks in ${1} as uid / gid: $(id -u) / $(id -g)"
     for f in "${1}/"*; do
         case "${f}" in
             *.sh)
-                echo "${0}: running ${f}"
+                echo "${0}: running script ${f}"
                 # shellcheck disable=SC1090
                 source "${f}"
                 ;;
             *)
                 if [[ -x "${f}" ]] ; then
-                    echo "${0}: running ${f}"
+                    echo "${0}: running executable ${f}"
                     "${f}"
                 else
-                    echo "${0}: ignoring ${f}"
+                    echo "${0}: ignoring non-executable ${f}"
                 fi
                 ;;
         esac
     done
     echo "${0}: done running hooks in ${1}"
 }
 
+
+# NOTE: This hook will run as the user the container was started with!
 run-hooks /usr/local/bin/start-notebook.d
 
-# Handle special flags if we're root
+# If the container started as the root user, then we have permission to refit
+# the jovyan user, and ensure file permissions, grant sudo rights, and such
+# things before we run the command passed to start.sh as the desired user
+# (NB_USER).
+#
 if [ "$(id -u)" == 0 ] ; then
+    # Environment variables:
+    # - NB_USER: the desired username and associated home folder
+    # - NB_UID: the desired user id
+    # - NB_GID: a group id we want our user to belong to
+    # - NB_GROUP: the groupname we want for the group
+    # - GRANT_SUDO: a boolean ("1" or "yes") to grant the user sudo rights
+    # - CHOWN_HOME: a boolean ("1" or "yes") to chown the user's home folder
+    # - CHOWN_EXTRA: a comma separated list of paths to chown
+    # - CHOWN_HOME_OPTS / CHOWN_EXTRA_OPTS: arguments to the chown commands
 
-    # Only attempt to change the jovyan username if it exists
+    # Refit the jovyan user to the desired the user (NB_USER)
     if id jovyan &> /dev/null ; then
-        echo "Set username to: ${NB_USER}"
-        usermod -d "/home/${NB_USER}" -l "${NB_USER}" jovyan
+        if ! usermod --home "/home/${NB_USER}" --login "${NB_USER}" jovyan 2>&1 | grep "no changes" > /dev/null; then
+            echo "Updated the jovyan user:"
+            echo "- username: jovyan       -> ${NB_USER}"
+            echo "- home dir: /home/jovyan -> /home/${NB_USER}"
+        fi
+    elif ! id -u "${NB_USER}" &> /dev/null; then
+        echo "ERROR: Neither the jovyan user or '${NB_USER}' exists."
+        echo "       This could be the result of stopping and starting, the"
+        echo "       container with a different NB_USER environment variable."
+        exit 1
+    fi
+    # Ensure the desired user (NB_USER) gets its desired user id (NB_UID) and is
+    # a member of the desired group (NB_GROUP, NB_GID)
+    if [ "${NB_UID}" != "$(id -u "${NB_USER}")" ] || [ "${NB_GID}" != "$(id -g "${NB_USER}")" ]; then
+        echo "Update ${NB_USER}'s UID:GID to ${NB_UID}:${NB_GID}"
+        # Ensure the desired group's existence
+        if [ "${NB_GID}" != "$(id -g "${NB_USER}")" ]; then
+            groupadd --force --gid "${NB_GID}" --non-unique "${NB_GROUP:-${NB_USER}}"
+        fi
+        # Recreate the desired user as we want it
+        userdel "${NB_USER}"
+        useradd --home "/home/${NB_USER}" --uid "${NB_UID}" --gid "${NB_GID}" --groups 100 --no-log-init "${NB_USER}"
     fi
 
-    # handle home and working directory if the username changed
+    # Move or symlink the jovyan home directory to the desired users home
+    # directory if it doesn't already exist, and update the current working
+    # directory to the new location if needed.
     if [[ "${NB_USER}" != "jovyan" ]]; then
-        # changing username, make sure homedir exists
-        # (it could be mounted, and we shouldn't create it if it already exists)
         if [[ ! -e "/home/${NB_USER}" ]]; then
-            echo "Copying home dir to /home/${NB_USER}"
+            echo "Attempting to copy /home/jovyan to /home/${NB_USER}..."
             mkdir "/home/${NB_USER}"
-            cp -a /home/jovyan/. "/home/${NB_USER}/" || ln -s /home/jovyan "/home/${NB_USER}"
+            if cp -a /home/jovyan/. "/home/${NB_USER}/"; then
+                echo "Success!"
+            else
+                echo "Failed!"
+                echo "Attempting to symlink /home/jovyan to /home/${NB_USER}..."
+                if ln -s /home/jovyan "/home/${NB_USER}"; then
+                    echo "Success!"
+                else
+                    echo "Failed!"
+                fi
+            fi
         fi
-        # if workdir is in /home/jovyan, cd to /home/${NB_USER}
+        # Ensure the current working directory is updated to the new path
         if [[ "${PWD}/" == "/home/jovyan/"* ]]; then
-            newcwd="/home/${NB_USER}/${PWD:13}"
-            echo "Setting CWD to ${newcwd}"
-            cd "${newcwd}"
+            new_wd="/home/${NB_USER}/${PWD:13}"
+            echo "Changing working directory to ${new_wd}"
+            cd "${new_wd}"
         fi
     fi
 
-    # Handle case where provisioned storage does not have the correct permissions by default
-    # Ex: default NFS/EFS (no auto-uid/gid)
-    if [[ "${CHOWN_HOME}" == "1" || "${CHOWN_HOME}" == 'yes' ]]; then
-        echo "Changing ownership of /home/${NB_USER} to ${NB_UID}:${NB_GID} with options '${CHOWN_HOME_OPTS}'"
+    # Optionally ensure the desired user get filesystem ownership of it's home
+    # folder and/or additional folders
+    if [[ "${CHOWN_HOME}" == "1" || "${CHOWN_HOME}" == "yes" ]]; then
+        echo "Ensuring /home/${NB_USER} is owned by ${NB_UID}:${NB_GID} ${CHOWN_HOME_OPTS:+(chown options: ${CHOWN_HOME_OPTS})}"
         # shellcheck disable=SC2086
         chown ${CHOWN_HOME_OPTS} "${NB_UID}:${NB_GID}" "/home/${NB_USER}"
     fi
     if [ -n "${CHOWN_EXTRA}" ]; then
         for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do
-            echo "Changing ownership of ${extra_dir} to ${NB_UID}:${NB_GID} with options '${CHOWN_EXTRA_OPTS}'"
+            echo "Ensuring ${extra_dir} is owned by ${NB_UID}:${NB_GID} ${CHOWN_HOME_OPTS:+(chown options: ${CHOWN_HOME_OPTS})}"
             # shellcheck disable=SC2086
             chown ${CHOWN_EXTRA_OPTS} "${NB_UID}:${NB_GID}" "${extra_dir}"
         done
     fi
 
-    # Change UID:GID of NB_USER to NB_UID:NB_GID if it does not match
-    if [ "${NB_UID}" != "$(id -u "${NB_USER}")" ] || [ "${NB_GID}" != "$(id -g "${NB_USER}")" ]; then
-        echo "Set user ${NB_USER} UID:GID to: ${NB_UID}:${NB_GID}"
-        if [ "${NB_GID}" != "$(id -g "${NB_USER}")" ]; then
-            groupadd -f -g "${NB_GID}" -o "${NB_GROUP:-${NB_USER}}"
-        fi
-        userdel "${NB_USER}"
-        useradd --home "/home/${NB_USER}" -u "${NB_UID}" -g "${NB_GID}" -G 100 -l "${NB_USER}"
-    fi
-
-    # Enable sudo if requested
-    if [[ "${GRANT_SUDO}" == "1" || "${GRANT_SUDO}" == 'yes' ]]; then
-        echo "Granting ${NB_USER} sudo access and appending ${CONDA_DIR}/bin to sudo PATH"
-        echo "${NB_USER} ALL=(ALL) NOPASSWD:ALL" > /etc/sudoers.d/notebook
-    fi
+    # Update potentially outdated environment variables since image build
+    export XDG_CACHE_HOME="/home/${NB_USER}/.cache"
 
     # Add ${CONDA_DIR}/bin to sudo secure_path
     sed -r "s#Defaults\s+secure_path\s*=\s*\"?([^\"]+)\"?#Defaults secure_path=\"\1:${CONDA_DIR}/bin\"#" /etc/sudoers | grep secure_path > /etc/sudoers.d/path
 
-    # Exec the command as NB_USER with the PATH and the rest of
-    # the environment preserved
+    # Optionally grant passwordless sudo rights for the desired user
+    if [[ "$GRANT_SUDO" == "1" || "$GRANT_SUDO" == "yes" ]]; then
+        echo "Granting ${NB_USER} passwordless sudo rights!"
+        echo "${NB_USER} ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers.d/added-by-start-script
+    fi
+
+    # NOTE: This hook is run as the root user!
     run-hooks /usr/local/bin/before-notebook.d
-    echo "Executing the command:" "${cmd[@]}"
-    exec sudo -E -H -u "${NB_USER}" PATH="${PATH}" XDG_CACHE_HOME="/home/${NB_USER}/.cache" PYTHONPATH="${PYTHONPATH:-}" "${cmd[@]}"
+
+    echo "Running as ${NB_USER}:" "${cmd[@]}"
+    exec sudo --preserve-env --set-home --user "${NB_USER}" \
+        PATH="${PATH}" XDG_CACHE_HOME="/home/${NB_USER}/.cache" \
+        PYTHONPATH="${PYTHONPATH:-}" \
+        "${cmd[@]}"
+
+# The container didn't start as the root user, so we will have to act as the
+# user we started as.
 else
-    if [[ "${NB_UID}" == "$(id -u jovyan 2>/dev/null)" && "${NB_GID}" == "$(id -g jovyan 2>/dev/null)" ]]; then
-        # User is not attempting to override user/group via environment
-        # variables, but they could still have overridden the uid/gid that
-        # container runs as. Check that the user has an entry in the passwd
-        # file and if not add an entry.
-        STATUS=0 && whoami &> /dev/null || STATUS=$? && true
-        if [[ "${STATUS}" != "0" ]]; then
-            if [[ -w /etc/passwd ]]; then
-                echo "Adding passwd file entry for $(id -u)"
-                sed -e "s/^jovyan:/nayvoj:/" /etc/passwd > /tmp/passwd
-                echo "jovyan:x:$(id -u):$(id -g):,,,:/home/jovyan:/bin/bash" >> /tmp/passwd
-                cat /tmp/passwd > /etc/passwd
-                rm /tmp/passwd
-            else
-                echo 'Container must be run with group "root" to update passwd file'
-            fi
-        fi
+    # Warn about misconfiguration of: desired username, user id, or group id
+    if [[ -n "${NB_USER}" && "${NB_USER}" != "$(id -un)" ]]; then
+        echo "WARNING: container must be started as root to change the desired user's name with NB_USER!"
+    fi
+    if [[ -n "${NB_UID}" && "${NB_UID}" != "$(id -u)" ]]; then
+        echo "WARNING: container must be started as root to change the desired user's id with NB_UID!"
+    fi
+    if [[ -n "${NB_GID}" && "${NB_GID}" != "$(id -g)" ]]; then
+        echo "WARNING: container must be started as root to change the desired user's group id with NB_GID!"
+    fi
 
-        # Warn if the user isn't going to be able to write files to ${HOME}.
-        if [[ ! -w /home/jovyan ]]; then
-            echo 'Container must be run with group "users" to update files'
-        fi
-    else
-        # Warn if looks like user want to override uid/gid but hasn't
-        # run the container as root.
-        if [[ -n "${NB_UID}" && "${NB_UID}" != "$(id -u)" ]]; then
-            echo "Container must be run as root to set NB_UID to ${NB_UID}"
-        fi
-        if [[ -n "${NB_GID}" && "${NB_GID}" != "$(id -g)" ]]; then
-            echo "Container must be run as root to set NB_GID to ${NB_GID}"
+    # Warn about misconfiguration of: granting sudo rights
+    if [[ "${GRANT_SUDO}" == "1" || "${GRANT_SUDO}" == "yes" ]]; then
+        echo "WARNING: container must be started as root to grant sudo permissions!"
+    fi
+
+    # Attempt to ensure the user uid we currently run as has a named entry in
+    # the /etc/passwd file, as it avoids software crashing on hard assumptions
+    # on such entry. Writing to the /etc/passwd was allowed for the root group
+    # from the Dockerfile during build.
+    #
+    # ref: https://github.com/jupyter/docker-stacks/issues/552
+    if ! whoami &> /dev/null; then
+        echo "There is no entry in /etc/passwd for our UID. Attempting to fix..."
+        if [[ -w /etc/passwd ]]; then
+            echo "Renaming old jovyan user to nayvoj ($(id -u jovyan):$(id -g jovyan))"
+            sed --in-place "s/^jovyan:/nayvoj:/" /etc/passwd
+
+            echo "jovyan:x:$(id -u):$(id -g):,,,:/home/jovyan:/bin/bash" >> /etc/passwd
+            echo "Added new jovyan user ($(id -u):$(id -g)). Fixed UID!"
+        else
+            echo "WARNING: unable to fix missing /etc/passwd entry because we don't have write permission."
         fi
     fi
 
-    # Warn if looks like user want to run in sudo mode but hasn't run
-    # the container as root.
-    if [[ "${GRANT_SUDO}" == "1" || "${GRANT_SUDO}" == 'yes' ]]; then
-        echo 'Container must be run as root to grant sudo permissions'
+    # Warn if the user isn't able to write files to ${HOME}
+    if [[ ! -w /home/jovyan ]]; then
+        echo "WARNING: no write access to /home/jovyan. Try starting the container with group 'users' (100)."
     fi
 
-    # Execute the command
+    # NOTE: This hook is run as the user we started the container as!
     run-hooks /usr/local/bin/before-notebook.d
     echo "Executing the command:" "${cmd[@]}"
     exec "${cmd[@]}"

base-notebook/test/test_container_options.py

@@ -44,7 +44,8 @@ def test_uid_change(container):
         command=["start.sh", "bash", "-c", "id && touch /opt/conda/test-file"],
     )
     # usermod is slow so give it some time
-    c.wait(timeout=120)
+    rv = c.wait(timeout=120)
+    assert rv == 0 or rv["StatusCode"] == 0
     assert "uid=1010(jovyan)" in c.logs(stdout=True).decode("utf-8")
 
 
@@ -56,7 +57,8 @@ def test_gid_change(container):
         environment=["NB_GID=110"],
         command=["start.sh", "id"],
     )
-    c.wait(timeout=10)
+    rv = c.wait(timeout=10)
+    assert rv == 0 or rv["StatusCode"] == 0
     logs = c.logs(stdout=True).decode("utf-8")
     assert "gid=110(jovyan)" in logs
     assert "groups=110(jovyan),100(users)" in logs
@@ -77,7 +79,9 @@ def test_nb_user_change(container):
     time.sleep(10)
     LOGGER.info(f"Checking if the user is changed to {nb_user} by the start script ...")
     output = running_container.logs(stdout=True).decode("utf-8")
-    assert f"Set username to: {nb_user}" in output, f"User is not changed to {nb_user}"
+    assert (
+        f"username: jovyan       -> {nb_user}" in output
+    ), f"User is not changed to {nb_user}"
 
     LOGGER.info(f"Checking {nb_user} id ...")
     command = "id"
@@ -108,21 +112,30 @@ def test_nb_user_change(container):
 
 
 def test_chown_extra(container):
-    """Container should change the UID/GID of CHOWN_EXTRA."""
+    """Container should change the UID/GID of a comma separated
+    CHOWN_EXTRA list of folders."""
     c = container.run(
         tty=True,
         user="root",
         environment=[
             "NB_UID=1010",
             "NB_GID=101",
-            "CHOWN_EXTRA=/opt/conda",
+            "CHOWN_EXTRA=/home/jovyan,/opt/conda/bin",
             "CHOWN_EXTRA_OPTS=-R",
         ],
-        command=["start.sh", "bash", "-c", "stat -c '%n:%u:%g' /opt/conda/LICENSE.txt"],
+        command=[
+            "start.sh",
+            "bash",
+            "-c",
+            "stat -c '%n:%u:%g' /home/jovyan/.bashrc /opt/conda/bin/jupyter",
+        ],
     )
     # chown is slow so give it some time
-    c.wait(timeout=120)
-    assert "/opt/conda/LICENSE.txt:1010:101" in c.logs(stdout=True).decode("utf-8")
+    rv = c.wait(timeout=120)
+    assert rv == 0 or rv["StatusCode"] == 0
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "/home/jovyan/.bashrc:1010:101" in logs
+    assert "/opt/conda/bin/jupyter:1010:101" in logs
 
 
 def test_chown_home(container):
@@ -131,18 +144,19 @@ def test_chown_home(container):
     c = container.run(
         tty=True,
         user="root",
-        environment=["CHOWN_HOME=yes", "CHOWN_HOME_OPTS=-R"],
-        command=[
-            "start.sh",
-            "bash",
-            "-c",
-            "chown root:root /home/jovyan && ls -alsh /home",
+        environment=[
+            "CHOWN_HOME=yes",
+            "CHOWN_HOME_OPTS=-R",
+            "NB_USER=kitten",
+            "NB_UID=1010",
+            "NB_GID=101",
         ],
+        command=["start.sh", "bash", "-c", "stat -c '%n:%u:%g' /home/kitten/.bashrc"],
     )
-    c.wait(timeout=120)
-    assert "Changing ownership of /home/jovyan to 1000:100 with options '-R'" in c.logs(
-        stdout=True
-    ).decode("utf-8")
+    rv = c.wait(timeout=120)
+    assert rv == 0 or rv["StatusCode"] == 0
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "/home/kitten/.bashrc:1010:101" in logs
 
 
 def test_sudo(container):
@@ -224,3 +238,29 @@ def test_container_not_delete_bind_mount(container, tmp_path):
     assert rv == 0 or rv["StatusCode"] == 0
     assert p.read_text() == "some-content"
     assert len(list(tmp_path.iterdir())) == 1
+
+
+@pytest.mark.skip(reason="not yet implemented; TODO: cherry-pick b44b7ab")
+def test_jupyter_env_vars_to_unset_as_root(container):
+    """Environment variables names listed in JUPYTER_ENV_VARS_TO_UNSET
+    should be unset in the final environment."""
+    c = container.run(
+        tty=True,
+        user="root",
+        environment=[
+            "JUPYTER_ENV_VARS_TO_UNSET=SECRET_ANIMAL,UNUSED_ENV,SECRET_FRUIT",
+            "FRUIT=bananas",
+            "SECRET_FRUIT=mango",
+            "SECRET_ANIMAL=cats",
+        ],
+        command=[
+            "start.sh",
+            "bash",
+            "-c",
+            "echo I like $FRUIT and ${SECRET_FRUIT:-stuff}, and love ${SECRET_LOVE:-to keep secrets}!",
+        ],
+    )
+    rv = c.wait(timeout=10)
+    assert rv == 0 or rv["StatusCode"] == 0
+    logs = c.logs(stdout=True).decode("utf-8")
+    assert "I like bananas and stuff, and love to keep secrets!" in logs