Add conditional logic to drop permissions for unprivileged Docker#798
Add conditional logic to drop permissions for unprivileged Docker#798jpiccari wants to merge 2 commits intoDonkie:masterfrom
Conversation
jpiccari
force-pushed
the
features/unprivileged-docker
branch
from
08180f6 to
76ca920
Compare
|
@Donkie this feels like a easy one to merge and would resolve an open issue. I know this project isn't your main focus right now, but let me know if there is anything further you need to get this one in. |
|
Ya this looks good, sorry for the long delay. Have you verified that this doesn't break any existing installs? I'm thinking about the id change of the group. The group of the volume mount that people have made will be different. Is the group id change to 1001 really necessary? |
|
Thanks for working on this! I was curious about the approach so I tested it locally. It looks like in the default case (PUID/PGID=1000), the condition evaluates to false, gosu gets skipped, and uvicorn ends up running as root instead of the app user. I think this might be a regression from the current behavior. I'm also not sure if checking whether PUID/PGID differs from their defaults would solve what the linked issue is asking for, which is to run rootless. Would it be simpler to check |
|
@Donkie now I'm the one with the long reply, sorry about that. Yes I did regression test, it works normally and when run as other users. I also specifically tested in a k8s cluster that enforces non-root containers (as in the linked issue). All cases work fine. I have not done upgrade regression checks though. The change to 1001 came during testing, I noticed some unexpected results when building the docker and/or when running the entrypoint script to update the app group ID. The UID will be the same, so I expect no issue but also didn't test it explicitly. Since the official image only supports the entrypoint run as root maybe we can add some change to the entrypoint (when run as root) to validate the folder ownership and permissions. I can pick that up if you can confirm the approach you think should be taken. @jdh313 The behavior you describe seems expected to me. I expect users to provide a user/group to docker/k8s when running containers. Setting PUID/GID is meant only for adding the specified UID/GID to the app user/group (this requires root privileges or similar). I can't think of a reason someone would want to do this, but figured I'd keep it configurable in case someone had needs outside my expectations. |
|
Hi, I'm new to unprivileged Docker (and Spoolman) and I was having issues with file/folder permissions when running Spoolman with your PR. What are the intended/expected folder (esp data folder) permission requirements in unprivileged mode? I seem to recall running |
|
Thanks for the detailed response, @jpiccari . I see the logic — PUID/PGID as a root-only remapping feature, with The established pattern for this is checking the actual runtime UID rather than env var values. The official Postgres image, Sentry's Symbolicator, and others all use a variation of: if [ "$(id -u)" = '0' ]; then
exec gosu <app-user> "$@"
fiLinuxServer.io (who originated PUID/PGID) takes the same approach — those vars only apply when running as root. An Happy to help with the implementation, or defer to @Donkie on direction. |
|
@jdh313 I think I see some recurring concerns that you have and maybe I can help provide context. The GID change is required. The issue is that the Many ways to manage the check. Your specific recommendation comes with the issue that it silently disables a documented feature. Following the principle of least surprise, we should at least include some additional check if the user is trying to run a non-root container and set a custom PUID/PGID. However, this check now looks very similar to the one in the PR. Not sure what issue @eib is having but I was unable to reproduce it. Some detailed steps would help to understand what is happening. However, I think it is very unlikely for users to migrate between root/non-root containers so not sure how much that sort of setup should be explored. |
|
I want to make sure we're on the same page about the root case first, since that's the default for most users. Right now, without this PR, In this PR, that default case skips both |
|
That is my bad I was having bad tunnel vision on the other things and was completely overlooking that. There is typos in the condition, it should be checking if the current UID/GID matches the requested PUID/PGID like this: [ $PUID -ne $(id -u) -o $PUID -ne $(id -g) ]Before I update the PR, let's figure finalize the decision points.
For me, 1 is a must since the current behavior (2 groups sharing the same GID) is somewhat undefined. Consider the For the For the checks on updating the UID/GID, I think this needs to be separate from the |
|
|
Seems % docker run python:3.14-slim-bookworm getent group
root:x:0:
daemon:x:1:
bin:x:2:
sys:x:3:
adm:x:4:
tty:x:5:
disk:x:6:
lp:x:7:
mail:x:8:
news:x:9:
uucp:x:10:
man:x:12:
proxy:x:13:
kmem:x:15:
dialout:x:20:
fax:x:21:
voice:x:22:
cdrom:x:24:
floppy:x:25:
tape:x:26:
sudo:x:27:
audio:x:29:
dip:x:30:
www-data:x:33:
backup:x:34:
operator:x:37:
list:x:38:
irc:x:39:
src:x:40:
shadow:x:42:
utmp:x:43:
video:x:44:
sasl:x:45:
plugdev:x:46:
staff:x:50:
games:x:60:
users:x:100:
nogroup:x:65534:PUID/PGID are always set (default values at the top of the entrypoint), so checking if they are set is not an option. You can check if they are set to the real values of the app user/group, or you can check if they are set to the hardcoded defaults at the top of the |
|
Could you test leaving the And good catch on the defaults! I totally missed that. What if you did something like this: if [ "$(id -u)" = "0" ]; then
PUID=${PUID:-1000}
PGID=${PGID:-1000}
# usermod/groupmod goes here
exec gosu app "$@"
else
if [ -n "${PUID}" ] || [ -n "${PGID}" ]; then
echo "ERROR: PUID/PGID requires root." >&2
exit 1
fi
exec "$@"
fiBasically: If run as root, get the UID/GID or use 1000 as default. Else (running as user) if a UID/GID is defined, error. |
diff --git a/Dockerfile b/Dockerfile
index 7f03515..a1f0fc4 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -44,8 +44,7 @@ RUN apt-get update && apt-get install -y \
&& rm -rf /var/lib/apt/lists/*
# Add local user so we don't run as root
-RUN groupmod -g 1001 users \
- && useradd -u 1000 -U app \
+RUN useradd -u 1000 -U app \
&& usermod -G users app \
&& mkdir -p /home/app/.local/share/spoolman \
&& chown -R app:app /home/app/.local/share/spoolman
@@ -74,4 +73,4 @@ RUN echo "GIT_COMMIT=${GIT_COMMIT}" > build.txt \This adjustment to the dockerfile has the following impact on users and groups. % docker run --rm spoolman getent passwd
root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
bin:x:2:2:bin:/bin:/usr/sbin/nologin
sys:x:3:3:sys:/dev:/usr/sbin/nologin
sync:x:4:65534:sync:/bin:/bin/sync
games:x:5:60:games:/usr/games:/usr/sbin/nologin
man:x:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:x:8:8:mail:/var/mail:/usr/sbin/nologin
news:x:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:x:13:13:proxy:/bin:/usr/sbin/nologin
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
backup:x:34:34:backup:/var/backups:/usr/sbin/nologin
list:x:38:38:Mailing List Manager:/var/list:/usr/sbin/nologin
irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
_apt:x:42:65534::/nonexistent:/usr/sbin/nologin
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
app:x:1000:1000::/home/app:/bin/sh% docker run --rm spoolman getent group
root:x:0:
daemon:x:1:
bin:x:2:
sys:x:3:
adm:x:4:
tty:x:5:
disk:x:6:
lp:x:7:
mail:x:8:
news:x:9:
uucp:x:10:
man:x:12:
proxy:x:13:
kmem:x:15:
dialout:x:20:
fax:x:21:
voice:x:22:
cdrom:x:24:
floppy:x:25:
tape:x:26:
sudo:x:27:
audio:x:29:
dip:x:30:
www-data:x:33:
backup:x:34:
operator:x:37:
list:x:38:
irc:x:39:
src:x:40:
shadow:x:42:
utmp:x:43:
video:x:44:
sasl:x:45:
plugdev:x:46:
staff:x:50:
games:x:60:
users:x:100:app
nogroup:x:65534:
app:x:1000:Basically, Playing around with the logic a bit I think this is the most clear experience for users. If they set PUID/PGID and run non-root, they will get an error that is hopefully clear (feedback on the exact error message welcome) that their configuration is not valid. I went digging in the #!/bin/sh
PUID=${PUID:-1000}
PGID=${PGID:-1000}
SPOOLMAN_PORT=${SPOOLMAN_PORT:-8000}
SPOOLMAN_HOST=${SPOOLMAN_HOST:-0.0.0.0}
fail() {
echo "$1" >&2
exit 1
}
if [ "$(id -u app)" -ne "$PUID" ]; then
usermod -o -u "$PUID" app ||
fail "Failed to update app UID to $PUID"
fi
if [ "$(id -g app)" -ne "$PGID" ]; then
groupmod -o -g "$PGID" app ||
fail "Failed to update app GID to $PGID"
fi
if [ "$(id -u)" -eq 0 ]; then
exec gosu "app" uvicorn spoolman.main:app --host $SPOOLMAN_HOST --port $SPOOLMAN_PORT "$@"
else
exec uvicorn spoolman.main:app --host $SPOOLMAN_HOST --port $SPOOLMAN_PORT "$@"
fi |
|
Thanks for testing with On the entrypoint script, I wanted to make sure I wasn't biased toward the approach I was picturing, so I asked Claude to run a structured debate, giving two separate agents each approach and asking them to advocate for it, with a final "fact check" agent to check both before asking the main agent to make a ruling. It returned the following analysis. Of course, neither myself nor AI are infallible, and I'm definitely happy to discuss further. Debate: Your Approach (top-level PUID/PGID) vs. Root-Scoped PUID/PGIDFor: Top-level PUID/PGID (your version)
For: Root-scoped PUID/PGID
Fact-Check Notes
Verdict (78% confidence)Root-scoped is the better design. The top-level version has a structural flaw: it crashes on every non-root invocation, which is the use case this PR exists to solve. If the Trade-off acknowledged: if a user migrates from Docker Compose (with Here's what the root-scoped version looks like: #!/bin/sh
SPOOLMAN_PORT=${SPOOLMAN_PORT:-8000}
SPOOLMAN_HOST=${SPOOLMAN_HOST:-0.0.0.0}
fail() {
echo "$1" >&2
exit 1
}
if [ "$(id -u)" = "0" ]; then
PUID=${PUID:-1000}
PGID=${PGID:-1000}
if [ "$(id -g app)" -ne "$PGID" ]; then
groupmod -o -g "$PGID" app ||
fail "Failed to update app GID to $PGID"
fi
if [ "$(id -u app)" -ne "$PUID" ]; then
usermod -o -u "$PUID" app ||
fail "Failed to update app UID to $PUID"
fi
exec gosu "app" uvicorn spoolman.main:app --host $SPOOLMAN_HOST --port $SPOOLMAN_PORT "$@"
else
if [ -n "${PUID}" ] || [ -n "${PGID}" ]; then
fail "ERROR: PUID/PGID requires running as root. Remove PUID/PGID or run the container as root."
fi
exec uvicorn spoolman.main:app --host $SPOOLMAN_HOST --port $SPOOLMAN_PORT "$@"
fi |
jpiccari
force-pushed
the
features/unprivileged-docker
branch
from
602e0f0 to
aaac97a
Compare
This change enables the container to run in unprivileged mode (without --privileged or additional capabilities) by making permission drops conditional: - Changed users group GID from 1000 to 1001 to avoid conflicts with the app user's primary group (UID 1000) - Only modify user/group IDs when PUID/PGID environment variables differ from the default value of 1000, avoiding unnecessary privilege requirements - Skip su-exec entirely when running as the default user, since changing user context requires additional capabilities that unprivileged containers don't have This allows the container to work in restricted environments like Kubernetes with security contexts or rootless Docker while maintaining backward compatibility for users who customize PUID/PGID. Fixes Donkie#791
jpiccari
force-pushed
the
features/unprivileged-docker
branch
from
aaac97a to
1375586
Compare
|
It would be silly to debate with a stochastic next token generator, so I guess this is my last post in this thread. Thanks again for your work @Donkie, hope to see some kind of rootless container support in the future so I can move back to tracking your official images. My last update added the entrypoint changes I mentioned and removes the I've also opened a feature request with
|
|
I think having both the users and app groups was an oversight on my part, we shouldn't need to mess with both. |
| RUN groupmod -g 1000 users \ | ||
| && useradd -u 1000 -U app \ | ||
| RUN useradd -u 1000 -U app \ | ||
| && usermod -G users app \ |
There was a problem hiding this comment.
based on the discussions, you can remove this usermod part
| --mount=type=bind,source=uv.lock,target=uv.lock \ | ||
| --mount=type=bind,source=pyproject.toml,target=pyproject.toml \ | ||
| uv sync --locked --no-install-project | ||
|
|
There was a problem hiding this comment.
When testing locally with podman, I had to add
# Add local user so we don't run as root
RUN useradd -u 1000 -U app
here to allow it to build, so could be good to add.
There was a problem hiding this comment.
I ran into the same issue after a rebase, but I solved it by removing the --chown from the python-builder stage. These weren't needed and all COPY that move files into python-runner have --chown so the final image is identical and you don't need to add the user. This is a local change I have already, but let me know your thoughts.
diff --git a/Dockerfile b/Dockerfile
index 3cc8a0a..85b3e69 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -25,9 +25,9 @@ RUN --mount=type=cache,target=/root/.cache/uv \
uv sync --locked --no-install-project
# Copy and install app
-COPY --chown=app:app migrations /home/app/spoolman/migrations
-COPY --chown=app:app spoolman /home/app/spoolman/spoolman
-COPY --chown=app:app alembic.ini README.md uv.lock pyproject.toml /home/app/spoolman/
+COPY migrations /home/app/spoolman/migrations
+COPY spoolman /home/app/spoolman/spoolman
+COPY alembic.ini README.md uv.lock pyproject.toml /home/app/spoolman/
RUN --mount=type=cache,target=/root/.cache/uv \
uv sync --locked
|
tested a bit and it should work fine with existing user's setup so I think we can continue with this, just handle my two comments |
I did several tests with different repos, I think your concern about backward compatibility is really important. The main thing that will trip users up is if they switch their deployment configuration.
Scenario 2 works as long as the container is run rootless with the same UID/GID as the root container used when dropping permissions. PUID/PGUID changes or running as some UID/GID other than 1000 may have issues.
I'll take a look tomorrow and add those changes in. |
|
I think all the changes you are looking for are in, and/or addressed in some other way. The matrix in the comment above shows the supported migration paths. I don't think anyone will have issues unless they attempt migrating in weird ways. |
4 participants
This change enables the container to run in unprivileged mode (without --privileged or additional capabilities) by making permission drops conditional:
This allows the container to work in restricted environments like Kubernetes with security contexts or rootless Docker while maintaining backward compatibility for users who customize PUID/PGID.
Fixes #791