docker arbitrary user support via PUID and PGID#9633
docker arbitrary user support via PUID and PGID#9633kon-foo wants to merge 8 commits intopgadmin-org:masterfrom
PUID and PGID#9633Conversation
… su-exec to drop privileges
WalkthroughRemoves final-stage Changes
Sequence Diagram(s)sequenceDiagram
participant Container as Container Runtime
participant Entrypoint as entrypoint.sh
participant System as System (users/groups, filesystem)
participant Gunicorn as Gunicorn
Container->>Entrypoint: start (env: PUID/PGID)
Entrypoint->>Entrypoint: detect effective UID
alt running as root
Entrypoint->>System: ensure group with PGID exists
Entrypoint->>System: modify pgadmin UID/GID to PUID/PGID
Entrypoint->>Entrypoint: set SU_EXEC to su-exec wrapper
Entrypoint->>System: safe_chown runtime paths if ownership differs
Entrypoint->>Gunicorn: exec via $SU_EXEC (TLS/non-TLS preserved)
else running as non-root
Entrypoint->>Entrypoint: preserve/append OpenShift /etc/passwd entry
Entrypoint->>System: skip UID/GID reassignment
Entrypoint->>Gunicorn: exec directly (no su-exec)
end
Gunicorn-->>Container: server running
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/docker/entrypoint.sh`:
- Around line 199-201: The chown block should avoid errors for missing /certs,
avoid always-recursing /var/lib/pgadmin, and honor a custom config path; update
the block to: check existence of /certs before attempting chown, check current
owner of /var/lib/pgadmin (using stat) and only run chown -R if it differs from
"$PUID:$PGID" to avoid costly repeated recursion, and replace the hardcoded
/pgadmin4/config_distro.py with the PGADMIN_CUSTOM_CONFIG_DISTRO_FILE variable
(if set and the file exists) and chown that file non-recursively; ensure all
chown commands still run only when uid is 0.
🧹 Nitpick comments (3)
pkg/docker/entrypoint.sh (3)
5-19: Consider validatingPUID/PGIDare numeric before use.If a user sets
PUID=abc,usermodfails silently (line 12 swallows errors), thensu-exec abc:0will fail at the very end of the script with a cryptic error. Fail-fast with a clear message would improve the user experience.🛡️ Proposed validation
PUID=${PUID:-5050} PGID=${PGID:-0} + +# Validate PUID/PGID are numeric +if ! echo "$PUID" | grep -qE '^[0-9]+$'; then + echo "Error: PUID must be a numeric value, got '$PUID'" >&2 + exit 1 +fi +if ! echo "$PGID" | grep -qE '^[0-9]+$'; then + echo "Error: PGID must be a numeric value, got '$PGID'" >&2 + exit 1 +fi if [ "$(id -u)" = "0" ]; then
155-162: DB initialization runs as root before ownership is fixed.
run_pgadmin.py(line 157) andload-servers(line 113) execute as UID 0 when the container starts as root, creating files in/var/lib/pgadminowned by root. Thechown -Rat line 199 corrects this, but if the script fails between these points the volume is left root-owned.An alternative is to move the
chown+su-execwrapping earlier, so that the initialization itself runs as the target user. This would also eliminate the need for the expensive recursivechown.
12-12: Silentusermodfailure could mislead operators.
2>/dev/null || truesuppresses all diagnostics. Ifusermodfails for an unexpected reason (e.g., shadow/passwd locked), the script continues and line 16 still prints the intended UID/GID — but thepgadminuser was never actually reassigned. Gunicorn will still launch viasu-execwith the numeric UID (so it works), but any files created during init (lines 157–187) will be owned by the old pgadmin UID.Consider logging the failure:
Proposed change
- usermod -o -u "$PUID" -g "$PGID" pgadmin 2>/dev/null || true + if ! usermod -o -u "$PUID" -g "$PGID" pgadmin 2>/dev/null; then + echo "Warning: failed to reassign pgadmin user to UID=$PUID GID=$PGID" >&2 + fi
|
@kon-foo, please fix the issues reported by CodeRabbit. |
… has the correct ownership before chowning it
|
@khushboovashi I added a |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/docker/entrypoint.sh (2)
2-3: Consider validatingPUIDandPGIDare numeric.Non-numeric values will propagate silently to
addgroup,usermod,su-exec,chown, andstatcomparisons, potentially causing cryptic failures. A simple guard up front would make misconfiguration obvious.💡 Example guard
PUID=${PUID:-5050} PGID=${PGID:-0} + +case "$PUID" in ''|*[!0-9]*) echo "Error: PUID must be numeric, got '$PUID'" >&2; exit 1;; esac +case "$PGID" in ''|*[!0-9]*) echo "Error: PGID must be numeric, got '$PGID'" >&2; exit 1;; esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` around lines 2 - 3, The script sets PUID and PGID but doesn't validate they're numeric; add an early guard in entrypoint.sh that checks PUID and PGID are integers (e.g., using a regex or POSIX-safe test) and prints a clear error and exits non-zero if either is non-numeric. Update the top of the script where PUID and PGID are assigned (references: PUID, PGID) to perform the validation before any calls to addgroup, usermod, su-exec, chown, or stat so misconfiguration fails fast with a helpful message.
5-19: Root initialization block looks correct overall; one note onusermoderror suppression.The group-creation guard,
usermod -ofor non-unique UID support, andSU_EXECcomposition are sound. The2>/dev/null || trueonusermod(line 12) will silently swallow real failures (e.g., if thepgadminuser is missing from the image due to a Dockerfile regression). Consider logging a warning on failure instead of a blanket suppress:💡 Example
- usermod -o -u "$PUID" -g "$PGID" pgadmin 2>/dev/null || true + if ! usermod -o -u "$PUID" -g "$PGID" pgadmin 2>&1; then + echo "Warning: failed to reassign pgadmin user to UID=$PUID GID=$PGID" >&2 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` around lines 5 - 19, The usermod call currently silences all failures (usermod ... pgadmin 2>/dev/null || true), which can hide real issues like a missing pgadmin user; update the root-init block so that after attempting usermod (functionality around usermod, getent, addgroup, PUID, PGID, pgadmin, and SU_EXEC) failures are detected and logged: run usermod without swallowing stderr, capture its exit code, and if non-zero emit a clear warning/error message via echo (or logger) mentioning pgadmin and the PUID/PGID values, but continue gracefully (or exit if you prefer) so regressions are visible in container startup logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 30-49: The safe_chown function is comparing
current_uid/current_gid to the wrong positional parameters (it currently checks
against "$1" and "$2"), causing chown to run every time; update the function to
compare current_uid to the UID argument and current_gid to the GID argument
(i.e., use the second and third positional params or assign them to local
uid/gid vars) before calling chown -R, referencing the safe_chown function and
the current_uid/current_gid/target/owner symbols to locate where to change the
if-condition.
---
Nitpick comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 2-3: The script sets PUID and PGID but doesn't validate they're
numeric; add an early guard in entrypoint.sh that checks PUID and PGID are
integers (e.g., using a regex or POSIX-safe test) and prints a clear error and
exits non-zero if either is non-numeric. Update the top of the script where PUID
and PGID are assigned (references: PUID, PGID) to perform the validation before
any calls to addgroup, usermod, su-exec, chown, or stat so misconfiguration
fails fast with a helpful message.
- Around line 5-19: The usermod call currently silences all failures (usermod
... pgadmin 2>/dev/null || true), which can hide real issues like a missing
pgadmin user; update the root-init block so that after attempting usermod
(functionality around usermod, getent, addgroup, PUID, PGID, pgadmin, and
SU_EXEC) failures are detected and logged: run usermod without swallowing
stderr, capture its exit code, and if non-zero emit a clear warning/error
message via echo (or logger) mentioning pgadmin and the PUID/PGID values, but
continue gracefully (or exit if you prefer) so regressions are visible in
container startup logs.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/docker/entrypoint.sh (2)
2-3: Consider validating thatPUIDandPGIDare numeric.Non-numeric values (e.g.,
PUID=foo) will cascade into cryptic failures fromaddgroup,usermod,chown, andsu-execwith no clear indication of the root cause. A quick guard would improve UX:Example validation
PUID=${PUID:-5050} PGID=${PGID:-0} + +case "$PUID" in ''|*[!0-9]*) echo "ERROR: PUID must be numeric (got '$PUID')" >&2; exit 1;; esac +case "$PGID" in ''|*[!0-9]*) echo "ERROR: PGID must be numeric (got '$PGID')" >&2; exit 1;; esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` around lines 2 - 3, Add a validation guard in entrypoint.sh immediately after the PUID and PGID assignments to ensure both are numeric; check PUID and PGID (the PUID and PGID variables used later with addgroup, usermod, chown and su-exec) with a simple numeric check (e.g., regex or shell arithmetic test) and if either is non-numeric emit a clear error to stderr and exit non-zero so downstream commands don't produce cryptic failures. Ensure the check covers empty/non-integer values and prints which variable is invalid to aid debugging.
12-12: Consider logging whenusermodfails to aid debugging of non-standard configurations.The
2>/dev/null || truesuppresses all errors fromusermod. While theshadowpackage is installed and thepgadminuser exists, sousermodwill succeed in normal cases, silently hiding failures makes it harder to debug if someone passes an invalid PUID or PGID (e.g., non-numeric values). Althoughsu-execandchownuse numeric IDs and will work regardless, some applications might callgetpwuid()orwhoamiinside the container, which could fail with a confusing error if the passwd entry doesn't match the execution UID.Consider logging a warning for visibility into potential misconfigurations:
Suggested improvement
- usermod -o -u "$PUID" -g "$PGID" pgadmin 2>/dev/null || true + if ! usermod -o -u "$PUID" -g "$PGID" pgadmin 2>/dev/null; then + echo "WARNING: failed to update pgadmin user to UID=$PUID GID=$PGID; ensure values are numeric" >&2 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` at line 12, The usermod call in entrypoint.sh currently suppresses all errors with "2>/dev/null || true", which hides failures when invalid PUID/PGID are passed; change it so failures are logged but the script still continues: run usermod -o -u "$PUID" -g "$PGID" pgadmin, capture its exit status and stderr, and if it fails call logger or echo a warning that includes the failing command, the provided PUID/PGID values and the captured error message (then continue execution). Reference the usermod invocation and the PUID/PGID/pgadmin variables in the entrypoint.sh to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 2-3: Add a validation guard in entrypoint.sh immediately after the
PUID and PGID assignments to ensure both are numeric; check PUID and PGID (the
PUID and PGID variables used later with addgroup, usermod, chown and su-exec)
with a simple numeric check (e.g., regex or shell arithmetic test) and if either
is non-numeric emit a clear error to stderr and exit non-zero so downstream
commands don't produce cryptic failures. Ensure the check covers
empty/non-integer values and prints which variable is invalid to aid debugging.
- Line 12: The usermod call in entrypoint.sh currently suppresses all errors
with "2>/dev/null || true", which hides failures when invalid PUID/PGID are
passed; change it so failures are logged but the script still continues: run
usermod -o -u "$PUID" -g "$PGID" pgadmin, capture its exit status and stderr,
and if it fails call logger or echo a warning that includes the failing command,
the provided PUID/PGID values and the captured error message (then continue
execution). Reference the usermod invocation and the PUID/PGID/pgadmin variables
in the entrypoint.sh to locate and update the code.
|
Regarding the "Nitpick" comment by @coderabbitai Consider logging when usermod fails to aid debugging |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
Your implementation strikes a good balance between error visibility and graceful handling. 👍 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
Both concerns have been resolved! 👍 🧠 Learnings used |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/docker/entrypoint.sh (1)
31-50:safe_chownownership comparison is correct — past issue resolved.The comparison
[ "$current_uid" = "$2" ] && [ "$current_gid" = "$3" ]now correctly references the UID and GID positional parameters (fixed in d3f2eb5). The existence guard at line 37 also cleanly handles paths that may not be present (e.g.,/certsin non-TLS setups).One optional improvement:
chown -Ris always used, even when$targetis a regular file (e.g.,$CONFIG_DISTRO_FILE_PATH). For files,-Ris harmless, but you could avoid the flag for non-directories to make intent clearer:♻️ Optional: skip `-R` for regular files
+ if [ -d "$target" ]; then chown -R "$owner" "$target" + else + chown "$owner" "$target" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/docker/entrypoint.sh` around lines 31 - 50, The safe_chown helper currently always calls chown -R "$owner" "$target"; modify safe_chown so it detects whether "$target" is a directory or a regular file (use test -d / -f) and only adds the -R flag for directories, calling chown "$owner" "$target" for files; keep the existing existence check and UID/GID comparison logic in safe_chown to avoid changing behavior for missing or already-owned paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/docker/entrypoint.sh`:
- Around line 31-50: The safe_chown helper currently always calls chown -R
"$owner" "$target"; modify safe_chown so it detects whether "$target" is a
directory or a regular file (use test -d / -f) and only adds the -R flag for
directories, calling chown "$owner" "$target" for files; keep the existing
existence check and UID/GID comparison logic in safe_chown to avoid changing
behavior for missing or already-owned paths.
2 participants
This PR adds support for
PUIDandPGIDenvironment variables as popularised by linuxserver.io images. This allows users to control the UID and GID of the process running gunicorn inside the container.Why
Currently, when running pgAdmin via Docker with bind-mounted volumes, the mounted directories must be owned by the hardcoded 5050 user on the host and inside the container. This creates two issues:
Permission Headaches:
Users are forced to manually chown directories on the host to 5050 before starting the container. This is cumbersome, especially if running docker as a non-root users and potentially lose access to the mounted dir.
Security Risks:
The hardcoded 5050 UID might already be in use on the host system. Running the container as this UID could unintentionally grant pgAdmin access to files or resources owned by that UID or vise versa.
Solution
UID0 by default.PUID5050 andGUID0 to keep current behaviour when no env vars are set."$(id -u)" = "0", aka no--userflag was used to run the image:PGIDexists and reassign the existing pgadmin user toPUIDandPGID.chownrequired files and dirs to said user.$PUID:$PGIDThe check for id=0 makes sure nothing changes for users, that use the
--userflag oruser:in compose to run the image.Compatability
Without PUID/PGID set, the behaviour is identical to the current image.
Tests
For each test, I cheked:
Further tests conducted:
Open Questions:
chowned? Currently/run/pgadmin,/var/lib/pgadmin,/certsandpgadmin4/config_distro.pyare covered.I have been using this for a couple of weeks now, but I am only using a very small subset of PgAdmin functionality.
Summary by CodeRabbit
New Features
Chores