refactor: update Containerfile to support non-root user execution and improve security#368
refactor: update Containerfile to support non-root user execution and improve security#368mostlygeek merged 2 commits intomostlygeek:mainfrom ryan-steed-usa:containerfile-non-root
Conversation
WalkthroughUpdates the Docker Containerfile: bumps LS_VER 89→170, adds UID/GID and USER_HOME ARGs, conditionally creates a system user/group and sets ownership for /root and /app, uses COPY --chown for the config, and switches to the created non-root user; ENTRYPOINT command remains the same. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Docker as Docker Build
participant Image as Built Image
Dev->>Docker: Build with ARGs LS_VER, UID, GID, USER_HOME
Docker->>Docker: Set LS_VER, create directories (/app, USER_HOME)
alt UID/GID != 0
Docker->>Docker: create group (GID) and user (UID) with HOME=USER_HOME
Docker->>Docker: chown -R /app and USER_HOME to UID:GID
Docker->>Docker: COPY --chown=$UID:$GID config.example.yaml -> /app/config.yaml
else UID/GID == 0
Docker->>Docker: keep root ownership, plain COPY config
end
Docker->>Image: finalize image, set USER to UID (if created)
Image->>Image: ENTRYPOINT runs same command as before
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (1)
docker/llama-swap.Containerfile (1)
23-24: Consider conditionally running chown only for non-root users.When building with the default
UID=0andGID=0, thechowncommand is redundant since directories are already owned by root. While this doesn't cause issues, wrapping it in a conditional would be a minor optimization.-RUN mkdir --parents $HOME /app -RUN chown --recursive $UID:$GID $HOME /app +RUN mkdir --parents $HOME /app && \ + if [ $UID -ne 0 ]; then \ + chown --recursive $UID:$GID $HOME /app; \ + fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/llama-swap.Containerfile(1 hunks)
🔇 Additional comments (5)
docker/llama-swap.Containerfile (5)
7-10: LGTM!The default build arguments provide backward compatibility by defaulting to root user execution (UID=0, GID=0).
27-27: LGTM!The
USERdirective correctly switches to the configured user, and maintains backward compatibility by staying as root whenUID=0.
35-35: LGTM!The
--chownflag ensures the configuration file has the correct ownership, whether running as root or a non-root user.
38-38: LGTM!The ENTRYPOINT formatting change has no behavioral impact.
5-5: Version 170 exists and is valid. The release was published on 2025-10-26, confirming the version bump is legitimate. All platform artifacts are available, including checksums for download integrity verification.
|
Closes #369 |
… improve security - Updated LS_VER argument from 89 to 170 to use the latest version - Added UID/GID arguments with default values of 0 (root) for backward compatibility - Added USER_HOME environment variable set to /root - Implemented conditional user/group creation logic that only runs when UID/GID are not 0 - Created necessary directory structure with proper ownership using mkdir and chown commands - Switched to non-root user execution for improved security posture - Updated COPY instruction to use --chown flag for proper file ownership
|
Hi, thanks for this contribution. I think hard coding |
- Changed default UID and GID from 0 (root) to 10001 for security best practices - Updated USER_HOME from /root to /app to avoid running as root user
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker/llama-swap.Containerfile (1)
22-24: Consider simplifying directory creation.With
USER_HOME=/appandHOME=$USER_HOME, themkdircommand creates/apptwice. While harmless (thanks to--parents), you could simplify to:-RUN mkdir --parents $HOME /app +RUN mkdir --parents /appAlternatively, keep the current form if you anticipate
USER_HOMEbeing set to something other than/appin future builds.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/llama-swap.Containerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 368
File: docker/llama-swap.Containerfile:14-20
Timestamp: 2025-10-27T16:04:23.381Z
Learning: In docker/llama-swap.Containerfile, the user/group creation logic intentionally supports adding a non-root user (non-zero UID) to the root group (GID=0) as a valid configuration. This allows flexible UID/GID customization where users can specify only a UID or both UID and GID.
📚 Learning: 2025-10-27T16:04:23.381Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 368
File: docker/llama-swap.Containerfile:14-20
Timestamp: 2025-10-27T16:04:23.381Z
Learning: In docker/llama-swap.Containerfile, the user/group creation logic intentionally supports adding a non-root user (non-zero UID) to the root group (GID=0) as a valid configuration. This allows flexible UID/GID customization where users can specify only a UID or both UID and GID.
Applied to files:
docker/llama-swap.Containerfile
🔇 Additional comments (2)
docker/llama-swap.Containerfile (2)
14-20: User creation logic looks good.The conditional user/group creation logic correctly implements the flexible UID/GID customization discussed in previous reviews. With the defaults of 10001 for both UID and GID, the container will run as non-root by default while still allowing build-time customization.
Based on learnings.
26-35: Excellent implementation of user switching and file ownership.The sequence is correct:
- Ownership of
/appis set (line 24) before switching usersUSERdirective switches to the non-root user- Downloads happen as the non-root user in their owned directory
COPY --chownensures correct ownership of the config fileThis properly enables non-root execution while maintaining necessary permissions.
| # Set default UID/GID arguments | ||
| ARG UID=10001 | ||
| ARG GID=10001 | ||
| ARG USER_HOME=/app | ||
|
|
There was a problem hiding this comment.
PR description inconsistency with actual defaults.
The PR description states "defaults of 0 for backward compatibility," but the actual defaults are UID=10001, GID=10001. However, these non-zero defaults align with the maintainer's preference to hardcode uid:10001 and gid:10001, and the maintainer noted this should remain backward-compatible since llama-swap doesn't write to disk.
Consider updating the PR description to reflect the actual default values.
🤖 Prompt for AI Agents
In docker/llama-swap.Containerfile around lines 7 to 11 the ARG defaults are
UID=10001 and GID=10001 but the PR description incorrectly claims defaults of 0;
update the PR description to state the actual defaults are UID=10001 and
GID=10001, and optionally note the maintainer rationale that these non-zero
defaults are intentional and remain backward-compatible because llama-swap does
not write to disk.
|
@ryan-steed-usa thanks for the contribution |
|
I've updated the arguments as suggested but note that llama.cpp might write to Depending on container runtime, and container configuration, a simple command like this might suffice to update ownership: <container runtime, i.e. docker or podman> exec -u root -it <container name> bash -c 'chown -R app:app <mounted volumes>'For example (but my deployment also required additional OS level ownership update due to rootless configuration, which I've omitted): podman exec -u root -it llama-swap bash -c 'chown -R app:app /models /app/.cache /app/config.yaml'I think that some container hosting systems have, in the past, required the root user in containers. These users could build images using arguments like: --build-arg="UID=0" --build-arg="GID=0" --build-arg="USER_HOME=/root"Let me know if these potential caveats are undesirable and I'll revert. |
|
let’s see if there are issued filed. It’s good security practice to run as a lesser privileged user. For mounting in shared volumes it’s ok to expect user/group permissions are set correctly. |
|
I couldn't agree more. Thank you! |
… improve security (mostlygeek#368) Set default container user/group to lower privilege app user * refactor: update Containerfile to support non-root user execution and improve security - Updated LS_VER argument from 89 to 170 to use the latest version - Added UID/GID arguments with default values of 0 (root) for backward compatibility - Added USER_HOME environment variable set to /root - Implemented conditional user/group creation logic that only runs when UID/GID are not 0 - Created necessary directory structure with proper ownership using mkdir and chown commands - Switched to non-root user execution for improved security posture - Updated COPY instruction to use --chown flag for proper file ownership * chore: update containerfile to use non-root user with proper UID/GID - Changed default UID and GID from 0 (root) to 10001 for security best practices - Updated USER_HOME from /root to /app to avoid running as root user
2 participants
Summary
Example usage
The user defaults to root with a home directory of
/rootso hopefully this won't affect any existing builds. These changes make possible building the container with a dedicated UID/GID. Personally I prefer a rootless Podman setup, with limited container user, using a build command like:And run with:
Summary by CodeRabbit