security: run Docker container as non-root user #510
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Dockerfile now creates and switches to a non-root user (sourcebot), adjusts ownership/permissions for app and data paths, and adds supervision-related scripts/config. The entrypoint script changes PostgreSQL initialization/startup to run without su, sets directory permissions to 0750, creates the postgres role, adds process liveness checks, and retains DB existence handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as Container Runtime
participant E as entrypoint.sh
participant PG as postgres (process)
participant DB as sourcebot DB
U->>E: Start container
Note over E: Ensure directories exist (0750)<br/>DATA_CACHE_DIR, DATABASE_DATA_DIR, REDIS_DATA_DIR
E->>E: initdb -D "$DATABASE_DATA_DIR" (if not initialized)
E->>PG: Start postgres -D "$DATABASE_DATA_DIR"
loop Wait for readiness
E->>PG: Check port/ready
E->>PG: Verify process alive
alt PG stopped
E->>U: Exit 1
end
end
E->>PG: createuser postgres -s (if missing)
E->>PG: Check DB existence
alt DB missing
E->>PG: createdb sourcebot
end
E->>U: Proceed to app startup (supervised)
Note over E,PG: Running as non-root user "sourcebot"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Dockerfile
Outdated
| chown -R postgres:postgres /run/postgresql && \ | ||
| chmod 775 /run/postgresql | ||
|
|
||
| # To run as non-root, the user must be past of postgres, redis and node. |
There was a problem hiding this comment.
| # To run as non-root, the user must be past of postgres, redis and node. | |
| # To run as non-root, the user must be part of postgres, redis and node groups |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Thanks for this PR!
What would the error message be in this case? And when in the execution flow would it this be hit? EDIT: I spun down the instance and changed the owner: And this the following: Any ideas on what's going on?
Is this something that could be done automatically during startup in the entrypoint? If not, it would be great to have a small upgrade doc (example) that we can link in the changelog so that users know what to do if they hit a error. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Dockerfile (1)
242-252: Typos and group/ownership consistency for non-root setup
- Fix typo “past” → “part”.
- Prefer chown user:group for /data and /app to keep primary group consistent.
- Use -p for /var/log/sourcebot creation and chown user:group.
-# To run as non-root, the user must be past of postgres, redis and node. +# To run as non-root, the user must be part of the postgres, redis, and node groups. RUN addgroup -g 1500 sourcebot && \ adduser -D -u 1500 -h /app -S sourcebot && \ adduser sourcebot postgres && \ adduser sourcebot redis && \ adduser sourcebot node && \ - chown -R sourcebot /data && \ - chown -R sourcebot /app && \ - mkdir /var/log/sourcebot && \ - chown sourcebot /var/log/sourcebot + chown -R sourcebot:sourcebot /data && \ + chown -R sourcebot:sourcebot /app && \ + mkdir -p /var/log/sourcebot && \ + chown -R sourcebot:sourcebot /var/log/sourcebot
🧹 Nitpick comments (4)
entrypoint.sh (4)
33-33: Quote paths and prefer install(1) for directory creationUsing mkdir -m with -p only applies the mode to the last component and paths aren’t quoted. Safer and clearer to use install -d with quotes.
- mkdir -m 0750 -p "$DATA_CACHE_DIR" + install -d -m 0750 -- "$DATA_CACHE_DIR"
46-46: Quote paths and prefer install(1) for directory creationSame mkdir -m with -p pattern; quote the var.
- mkdir -m 0750 -p $REDIS_DATA_DIR + install -d -m 0750 -- "$REDIS_DATA_DIR"
152-154: Unnecessary createuser after switching initdb to -U postgres; at minimum make idempotentIf you adopt initdb -U postgres, this block is redundant and should be removed. If you keep current initdb behavior, add --if-not-exists to avoid failure on restarts.
Option A (preferred, when using initdb -U postgres):
- # Running as non-root we need to ensure the postgres account is created. - createuser postgres -sOption B (keep current initdb user, but fix readiness as above):
- createuser postgres -s + createuser --if-not-exists postgres -s
139-141: POSIX sh: avoid echo -e; use printfBusyBox/ash does not standardize echo -e. Use printf for portability.
- echo -e "\e[34m[Info] Waiting for the database to be ready...\e[0m" + printf '\033[34m[Info] Waiting for the database to be ready...\033[0m\n'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(2 hunks)entrypoint.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
entrypoint.sh
[warning] 33-33: When used with -p, -m only applies to the deepest directory.
(SC2174)
[warning] 38-38: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 39-39: When used with -p, -m only applies to the deepest directory.
(SC2174)
[warning] 46-46: When used with -p, -m only applies to the deepest directory.
(SC2174)
[warning] 140-140: In POSIX sh, echo flags are undefined.
(SC3037)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
Dockerfile (1)
261-261: LGTM: switching to non-root user in the final stageUSER sourcebot placement looks correct relative to chown operations and before ENTRYPOINT.
| echo -e "\e[34m[Info] Initializing database at $DATABASE_D\ATA_DIR...\e[0m" | ||
| mkdir -m 0750 -p $DATABASE_DATA_DIR | ||
|
|
||
| initdb -D "$DATABASE_DATA_DIR" |
There was a problem hiding this comment.
Deadlock risk: initdb runs as “sourcebot” (superuser becomes sourcebot) but readiness/psql assume user “postgres”
As written, initdb creates DB superuser = current OS user (“sourcebot”). The subsequent until loop waits on pg_isready -U postgres (role doesn’t exist yet), so it can spin forever. Later createuser relies on connecting as “sourcebot”, but you never reach it if pg_isready is gated on a non-existent role. Also fix the log typo and avoid echo -e in /bin/sh.
Apply this to make the superuser “postgres”, fix the log, quote paths, and use printf:
- echo -e "\e[34m[Info] Initializing database at $DATABASE_D\ATA_DIR...\e[0m"
- mkdir -m 0750 -p $DATABASE_DATA_DIR
-
- initdb -D "$DATABASE_DATA_DIR"
+ printf '\033[34m[Info] Initializing database at %s...\033[0m\n' "$DATABASE_DATA_DIR"
+ install -d -m 0750 -- "$DATABASE_DATA_DIR"
+ initdb -D "$DATABASE_DATA_DIR" -U postgres📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo -e "\e[34m[Info] Initializing database at $DATABASE_D\ATA_DIR...\e[0m" | |
| mkdir -m 0750 -p $DATABASE_DATA_DIR | |
| initdb -D "$DATABASE_DATA_DIR" | |
| printf '\033[34m[Info] Initializing database at %s...\033[0m\n' "$DATABASE_DATA_DIR" | |
| install -d -m 0750 -- "$DATABASE_DATA_DIR" | |
| initdb -D "$DATABASE_DATA_DIR" -U postgres |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 38-38: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 39-39: When used with -p, -m only applies to the deepest directory.
(SC2174)
🤖 Prompt for AI Agents
In entrypoint.sh around lines 38 to 41, the script currently uses echo -e,
mis-typed DATABASE_D\ATA_DIR, and runs initdb so the DB superuser becomes the
current OS user (sourcebot), which deadlocks later because pg_isready and
createuser expect the "postgres" role; update the lines to: use printf instead
of echo -e, fix the variable to $DATABASE_DATA_DIR and quote it everywhere
(mkdir -m 0750 -p "$DATABASE_DATA_DIR" and initdb -D "$DATABASE_DATA_DIR"), and
ensure initdb creates the postgres superuser by passing the username option
(initdb --username=postgres or -U postgres) so subsequent pg_isready -U postgres
and createuser steps succeed.
| postgres -D "$DATABASE_DATA_DIR" & | ||
| until pg_isready -h localhost -p 5432 -U postgres; do | ||
| echo -e "\e[34m[Info] Waiting for the database to be ready...\e[0m" | ||
| sleep 1 | ||
|
|
||
| # As postgres runs in the background, we must check if it is still | ||
| # running, otherwise the "until" loop will be running indefinitely. | ||
| if ! pgrep -x "postgres" > /dev/null; then | ||
| echo "postgres failed to run" | ||
| exit 1 | ||
| break | ||
| fi | ||
| done | ||
|
|
There was a problem hiding this comment.
Readiness wait uses -U postgres before the role exists; also unreachable break
Wait without forcing a user, keep the liveness check, and use printf. Remove the unreachable break after exit 1.
- postgres -D "$DATABASE_DATA_DIR" &
- until pg_isready -h localhost -p 5432 -U postgres; do
- echo -e "\e[34m[Info] Waiting for the database to be ready...\e[0m"
+ postgres -D "$DATABASE_DATA_DIR" &
+ until pg_isready -q -h localhost -p 5432; do
+ printf '\033[34m[Info] Waiting for the database to be ready...\033[0m\n'
sleep 1
-
- # As postgres runs in the background, we must check if it is still
- # running, otherwise the "until" loop will be running indefinitely.
- if ! pgrep -x "postgres" > /dev/null; then
- echo "postgres failed to run"
- exit 1
- break
- fi
+ # As postgres runs in the background, ensure it hasn't died.
+ if ! pgrep -x "postgres" > /dev/null; then
+ echo "postgres failed to run"
+ exit 1
+ fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| postgres -D "$DATABASE_DATA_DIR" & | |
| until pg_isready -h localhost -p 5432 -U postgres; do | |
| echo -e "\e[34m[Info] Waiting for the database to be ready...\e[0m" | |
| sleep 1 | |
| # As postgres runs in the background, we must check if it is still | |
| # running, otherwise the "until" loop will be running indefinitely. | |
| if ! pgrep -x "postgres" > /dev/null; then | |
| echo "postgres failed to run" | |
| exit 1 | |
| break | |
| fi | |
| done | |
| postgres -D "$DATABASE_DATA_DIR" & | |
| until pg_isready -q -h localhost -p 5432; do | |
| printf '\033[34m[Info] Waiting for the database to be ready...\033[0m\n' | |
| sleep 1 | |
| # As postgres runs in the background, ensure it hasn't died. | |
| if ! pgrep -x "postgres" > /dev/null; then | |
| echo "postgres failed to run" | |
| exit 1 | |
| fi | |
| done |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 140-140: In POSIX sh, echo flags are undefined.
(SC3037)
🤖 Prompt for AI Agents
In entrypoint.sh around lines 138 to 151, the readiness loop calls pg_isready
with -U postgres before that role may exist and uses echo -e and an unreachable
break after exit; change the pg_isready invocation to not force a user (e.g.,
use: pg_isready -h localhost -p 5432), replace echo -e with printf for portable
colored output (e.g., printf '\e[34m[Info] Waiting for the database to be
ready...\e[0m\n'), keep the pgrep liveness check as-is to ensure postgres is
still running, and remove the unreachable break after the exit 1 so the control
flow is correct.
Running containers as a non-root user is a long standing security practice. The changes ensure that the sourcebot user is created and has the correct level of permissions to run all its dependencies (postgres, redis and node). Please note that as a side effect, existing mounted volumes would need to have their ownership reviewed or it may not be able to access the files. This is specially the case for previous versions that would create said files as 0:0. To fix that, users can run chown -R 1500:1500 /path/.sourcebot. The chmod may also need to be a bit more strict in such cases, so changing that is advised: chown -R 0750 /path/.sourcebot. Signed-off-by: Paulo Gomes <pjbgf@linux.com>
|
@brendan-kellam thanks for the review. Looking into your message I noticed two problems, once regarding the migration from existing dbs created using root and the second which was the attempt of creating the user again on follow-up executions. Both as now fixed. I made some changes and both workflows should work fine: Migrating from existing DBNon-root from scratch
Yes, the problem there is that you are mounting a dir which the application (running as However, we could improve the error messaging around it. For the doc changes, do you prefer that within the same PR or as a separate PR? |
|
For reference, this was deployed successfully using #370. The permissioning for the storage is automated via init container: 42a43,64
>
> {{- if $.Values.storage }}
> initContainers:
> - name: init-storage
> image: {{ include "sourcebot.image" $ }}
> args:
> - |
> echo "Init storage";
> set -x;
> chown -R 1500:1500 /data;
> chmod -R 0750 /data;
> ls -la /data;
> command:
> - sh
> - -c
> securityContext:
> runAsUser: 0
> volumeMounts:
> - name: sourcebot-data
> mountPath: /data
> {{- end }}
>PS: The same container image was used to avoid depending on another image, however that could be changed to a lighter image (e.g. busybox/alpine). |
Running containers as a non-root user is a long standing security practice. The changes ensure that the sourcebot user is created and has the correct level of permissions to run all its dependencies (postgres, redis and node).
Please note that as a side effect, existing mounted volumes would need to have their ownership reviewed or it may not be able to access the files. This is specially the case for previous versions that would create said files as 0:0.
To fix that, users can run
chown -R 1500:1500 /path/.sourcebot. The chmod may also need to be a bit more strict in such cases, so changing that is advised:chmod -R 0750 /path/.sourcebot.Fixes #302.
Summary by CodeRabbit