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.
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). |
|
Sorry for being slow on this PR! Will get back on this soon |
Ok sounds good. I think one of the errors I was making was that I was running chmod / chown on the For the migration path, I was still hitting the following issue: I've confirmed that /data is owned by 1500, so I'm not exactly sure what's going on there.
Let's do it in a separate PR. I can take that on as there are some other things I'm planning on changing w.r.t, local deployments (mainly recommending using docker compose w/ named volumes, which afaik will play nicer with permissions). overall LGTM - thanks for your contribution! |
|
Hey @pjbgf - would you be able to get this merged today? |
|
Closing since shipped this in #599 |
3 participants
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