fix(docker): read .env last, and restore RUN_AS_UID/GID from positional parameters

docker/entrypoint-go.sh

@@ -17,8 +17,8 @@ fi
 # Which user runs the two Go services, and what they are handed. supervisord
 # itself stays root so that mosquitto and caddy can still bind their ports.
 # Unprivileged operation is opt-in via RUN_AS_UID, taken from the container's
-# environment before .env is read; .env is then read (with the services
-# unprivileged, only if root owns it) and the data directory handed over. The
+# environment before .env is read; the data directory is handed over, and .env
+# is read last (with the services unprivileged, only if root owns it). The
 # order matters and lives in one function: see docker/select-service-user.sh,
 # and "Running the Go services unprivileged" in docs/deployment.md.
 . /select-service-user.sh

docker/select-service-user.sh

@@ -3,8 +3,8 @@
 # hands over the paths they have to write.
 #
 # Sourced by docker/entrypoint-go.sh, which calls prepare_service_user and
-# nothing else from here: the order of the steps (resolve the account, read
-# .env, hand over the data directory) is part of what keeps .env from choosing
+# nothing else from here: the order of the steps (resolve the account, hand
+# over the data directory, read .env) is part of what keeps .env from choosing
 # the account, so it lives in one function and the test drives that function.
 # docker/test-select-service-user.sh replaces getent/adduser/addgroup/deluser/
 # delgroup/id/chown/find/stat with stubs: keep every external command here
@@ -34,36 +34,63 @@ SERVICE_DATA_DIR=/app/data
 SERVICE_ENV_FILE=/app/data/.env
 SERVICE_MOUNTINFO=/proc/self/mountinfo
 
+# printf, not echo: dash's echo turns a \012 in a logged path into a newline.
 svc_log() {
-  echo "[entrypoint] $*" >&2
+  printf '%s\n' "[entrypoint] $*" >&2
 }
 
 # prepare_service_user
 #
-# The start-up sequence, in the only order that is safe:
+# The start-up sequence:
 #
 #   1. resolve the account, from RUN_AS_UID/RUN_AS_GID in the container's
 #      environment;
-#   2. read /app/data/.env (see load_env_file for when it is trusted);
-#   3. hand the data directory to the account, when it is not root.
+#   2. hand the data directory to the account, when it is not root;
+#   3. read /app/data/.env (see load_env_file for when it is trusted), then
+#      put RUN_AS_UID/RUN_AS_GID back to what the container had.
 #
 # Returns non-zero, with the reason on stderr, when RUN_AS_UID was set and no
 # account could be provided. On success exports CORESCOPE_SERVICE_USER.
 #
-# The resolved account is kept in the positional parameter, not a variable: the
-# sourced .env runs in this shell, and a line in it such as `account=root` would
-# otherwise change which account is handed the data directory and exported to
-# supervisord.
+# .env comes last, and what runs after it reads only this function's
+# positional parameters. A sourced file shares the shell's variables: every
+# global, the locals of load_env_file, which sources it, and, by dynamic
+# scoping, the locals of this function too, so `_account=root` or
+# `SERVICE_DATA_DIR=/` in .env would reach any step that still read them. It
+# also gets load_env_file's positional parameters, so `set -- root` there
+# lands in that function's $1 and not in this one's. Hence the account, the
+# container's RUN_AS_UID/RUN_AS_GID and the file's path are parked in $1..$6
+# before .env is read, the hand-over, which reads the SERVICE_* paths, is done
+# before it, and nothing else follows.
+#
+# A .env that passes the trust check is root's own code, and this does not
+# try to defend against it: it can redefine these functions or set PATH. What
+# it cannot do by assigning variables is change the account, where the
+# hand-over goes, or RUN_AS_UID/RUN_AS_GID as the services see them.
 prepare_service_user() {
   local _account
   _account=$(resolve_service_account) || return 1
-  set -- "$_account"
+  set -- "$_account" "${RUN_AS_UID+set}" "${RUN_AS_UID-}" \
+         "${RUN_AS_GID+set}" "${RUN_AS_GID-}" "$SERVICE_ENV_FILE"
   unset _account
 
-  load_env_file "$SERVICE_ENV_FILE" "$1"
   if [ "$1" != root ]; then
     claim_service_paths "$1"
   fi
+
+  load_env_file "$6" "$1"
+
+  # The account was resolved before .env was read, so a RUN_AS_UID or
+  # RUN_AS_GID in it must not reach supervisord and the services either: put
+  # back what the container had, set or unset.
+  if [ "${RUN_AS_UID+set}" != "$2" ] || [ "${RUN_AS_UID-}" != "$3" ] ||
+     [ "${RUN_AS_GID+set}" != "$4" ] || [ "${RUN_AS_GID-}" != "$5" ]; then
+    svc_log "ignoring RUN_AS_UID/RUN_AS_GID from $6: set them on the container"
+    svc_log "(docker run -e, or environment: in compose), where the services cannot remove them."
+    if [ -n "$2" ]; then RUN_AS_UID=$3; export RUN_AS_UID; else unset RUN_AS_UID; fi
+    if [ -n "$4" ]; then RUN_AS_GID=$5; export RUN_AS_GID; else unset RUN_AS_GID; fi
+  fi
+
   CORESCOPE_SERVICE_USER=$1
   export CORESCOPE_SERVICE_USER
 }
@@ -162,11 +189,11 @@ resolve_service_account() {
 # writable by no one else. Running as root changes nothing — whoever could
 # write the file could already run code as root.
 #
-# Local names are underscored: the file is sourced into this function.
+# Nothing runs here after the file: it shares this function's variables and
+# positional parameters, so anything read afterwards would be whatever it
+# chose to leave. prepare_service_user does the rest from its own parameters.
 load_env_file() {
   local _env_file="$1" _env_account="$2"
-  local _uid_before="${RUN_AS_UID-}" _gid_before="${RUN_AS_GID-}"
-  local _uid_was="${RUN_AS_UID+set}" _gid_was="${RUN_AS_GID+set}"
 
   [ -f "$_env_file" ] || return 0
   if [ "$_env_account" != root ] && ! env_file_trusted "$_env_file"; then
@@ -179,17 +206,6 @@ load_env_file() {
   set -a
   . "$_env_file"
   set +a
-
-  # The account was resolved before this file was read, so a RUN_AS_UID or
-  # RUN_AS_GID in it must not reach supervisord and the services either: put
-  # back what the container had, set or unset.
-  if [ "${RUN_AS_UID+set}" != "$_uid_was" ] || [ "${RUN_AS_UID-}" != "$_uid_before" ] ||
-     [ "${RUN_AS_GID+set}" != "$_gid_was" ] || [ "${RUN_AS_GID-}" != "$_gid_before" ]; then
-    svc_log "ignoring RUN_AS_UID/RUN_AS_GID from $_env_file: set them on the container"
-    svc_log "(docker run -e, or environment: in compose), where the services cannot remove them."
-    if [ -n "$_uid_was" ]; then RUN_AS_UID=$_uid_before; export RUN_AS_UID; else unset RUN_AS_UID; fi
-    if [ -n "$_gid_was" ]; then RUN_AS_GID=$_gid_before; export RUN_AS_GID; else unset RUN_AS_GID; fi
-  fi
 }
 
 # env_file_trusted <path>

docker/test-select-service-user.sh

@@ -5,19 +5,25 @@
 # Every branch is exercised without docker and without root: getent, adduser,
 # addgroup, deluser, delgroup, id, chown, find and stat are replaced by stubs
 # on PATH, and the mount table is a fixture file. The account stubs read and
-# write a passwd/group pair of files seeded with the accounts the real image has — verified against alpine:3.20 with
-# mosquitto, supervisor and caddy installed, plus the Dockerfile's corescope —
-# so an account created mid-run is visible to the lookups that follow it. chown
-# and find record their arguments rather than touching the filesystem, which is
-# what lets the ownership hand-over be asserted exactly.
+# write a passwd/group pair of files seeded with the accounts the real image
+# has (verified against alpine:3.20 with mosquitto, supervisor and caddy
+# installed, plus the Dockerfile's corescope), so an account created mid-run is
+# visible to the lookups that follow it. chown and find record their arguments
+# rather than touching the filesystem, which is what lets the ownership
+# hand-over be asserted exactly.
 #
-# The driver calls prepare_service_user, the one function docker/entrypoint-go.sh
-# calls: resolve the account, read .env, hand over the data directory. A static
-# check at the end pins that the entrypoint calls it and nothing else from the
-# script, so what gets tested is the sequence the container runs: a non-zero
-# return must never be read as root, and .env cannot choose the account.
+# The driver calls prepare_service_user, the one function the entrypoint
+# (docker/entrypoint-go.sh) calls: resolve the account, hand over the data
+# directory, read .env. A static check at the end pins that the entrypoint
+# calls it and nothing else from the script, so what gets tested is the
+# sequence the container runs: a non-zero return must never be read as root,
+# and .env cannot choose the account.
 #
 # Run: bash docker/test-select-service-user.sh
+# TEST_SHELL picks the shell the sequence runs in, default sh; the image runs
+# busybox ash, so TEST_SHELL=/path/to/busybox-sh is worth a run. A busybox
+# built to prefer its own applets over PATH (Ubuntu's package) bypasses the
+# stubs and fails most cases; Debian's and Alpine's look up PATH first.
 # Exits non-zero if any case fails.
 set -u
 
@@ -209,7 +215,7 @@ STUB
 # the services would see them.
 drive() {
     make_fixture
-    OUT=$(env -i PATH="$FIX/bin:/usr/bin:/bin" FIX="$FIX" SCRIPT="$SCRIPT" "$@" sh -c '
+    OUT=$(env -i PATH="$FIX/bin:/usr/bin:/bin" FIX="$FIX" SCRIPT="$SCRIPT" "$@" "${TEST_SHELL:-sh}" -c '
         . "$SCRIPT"
         SERVICE_ENV_FILE="$FIX/env"
         SERVICE_MOUNTINFO="$FIX/mountinfo"
@@ -419,6 +425,53 @@ CORESCOPE_SERVICE_USER=root' drive RUN_AS_UID=1000 STUB_STAT='0 -rw-r--r--'
 assert_eq "account" "corescope" "$OUT"
 assert_has "the data dir was still handed over to it" "$CALLS" "chown -h 1000:1000"
 
+# The file is sourced inside load_env_file, which has positional parameters of
+# its own, so a `set --` in it cannot reach the account prepare_service_user
+# keeps in $1. Sourcing it in prepare_service_user instead would make this fail.
+CASE=".env runs set -- root"
+FIX_ENV='set -- root' drive RUN_AS_UID=1000 STUB_STAT='0 -rw-r--r--'
+assert_eq "account" "corescope" "$OUT"
+assert_has "the data dir was still handed over to it" "$CALLS" "chown -h 1000:1000"
+
+# A sourced file shares every variable of the shell, the locals of the
+# functions on the call stack included. The container's RUN_AS_UID/RUN_AS_GID
+# are therefore put back from positional parameters, not from variables a line
+# in .env could pre-empt. The uid and gid differ so that a mixed-up parameter
+# shows.
+CASE=".env assigns the names the restore could have used"
+FIX_ENV='_uid_before=0
+_uid_was=set
+_gid_before=0
+_gid_was=set
+_env_file=/nowhere
+_env_account=root
+SERVICE_ENV_FILE=/nowhere
+RUN_AS_UID=0
+RUN_AS_GID=0' drive RUN_AS_UID=1000 RUN_AS_GID=1001 STUB_STAT='0 -rw-r--r--'
+assert_eq "account" "corescope" "$OUT"
+assert_eq "both put back" "RUN_AS_UID=1000 RUN_AS_GID=1001" "$FINAL_ENV"
+assert_has "ignored, and the log names the real file" "$LOG" "ignoring RUN_AS_UID/RUN_AS_GID from $FIX/env"
+
+# Each of the two is compared on its own: a .env that changes only the gid is
+# noticed too.
+CASE="RUN_AS_GID alone in a trusted .env, 1000:1000 on the container"
+FIX_ENV='RUN_AS_GID=0' drive RUN_AS_UID=1000 RUN_AS_GID=1000 STUB_STAT='0 -rw-r--r--'
+assert_eq "gid put back" "RUN_AS_UID=1000 RUN_AS_GID=1000" "$FINAL_ENV"
+assert_has "ignored, and said so" "$LOG" "ignoring RUN_AS_UID/RUN_AS_GID"
+
+# The hand-over reads the SERVICE_* paths, so it happens before .env is read:
+# a .env that points them elsewhere must find the data directory already dealt
+# with, and .env itself already left out of it.
+CASE=".env points the SERVICE_* paths elsewhere"
+FIX_ENV='SERVICE_DATA_DIR=/
+SERVICE_ENV_FILE=/nowhere
+SERVICE_MOUNTINFO=/dev/null' drive RUN_AS_UID=1000 STUB_STAT='0 -rw-r--r--'
+assert_eq "account" "corescope" "$OUT"
+assert_has "the data dir was handed over, .env left out" "$CALLS" \
+    "find /app/data ( -path $FIX/env ) -prune -o ! -user 1000 -exec chown -h 1000:1000 {} +"
+assert_lacks "nothing else was walked" "$CALLS" "find / ("
+assert_lacks "and the real .env was the one excluded" "$CALLS" "/nowhere"
+
 # ----- 15. a file mounted into the data directory keeps its host owner -----
 # The documented single-file config mount, plus one with a space in its path,
 # as the kernel escapes it. The data directory's own mount is not excluded.
@@ -463,6 +516,12 @@ assert_eq "startup continues" 0 "$RC"
 assert_has "warning names the count" "$LOG" "WARNING: 2 path(s) in /app/data could not be given to uid 1000"
 assert_has "and the paths" "$LOG" "  /app/data/meshcore.db"
 
+# A logged path is printed as it is: dash's echo would turn the \012 into a
+# newline and cut the path in two.
+CASE="chown refused on a path with a backslash escape"
+drive RUN_AS_UID=1000 STUB_LEFTOVER='/app/data/x\012y'
+assert_has "path logged intact" "$LOG" '  /app/data/x\012y'
+
 # ----- 17. the entrypoint runs the sequence through prepare_service_user ----
 # The driver above tests the function; this pins that the entrypoint uses it
 # and does not rebuild the sequence from the steps, in an order of its own.
@@ -473,6 +532,7 @@ assert_has "calls prepare_service_user" "$EP" "prepare_service_user || exit 1"
 assert_lacks "does not resolve the account itself" "$EP" "resolve_service_account"
 assert_lacks "does not read .env itself" "$EP" "load_env_file"
 assert_lacks "does not hand over paths itself" "$EP" "claim_service_paths"
+assert_lacks "does not set the account itself" "$EP" "CORESCOPE_SERVICE_USER="
 
 # ----- report ---------------------------------------------------------------
 echo "select-service-user: $PASS assertions passed, $FAIL failed"