use ubuntu user instead of root for uv docker images#3491
Conversation
📝 WalkthroughWalkthroughDocker and entrypoint script updates shift execution context from root to non-root ubuntu user. Includes path migrations to /home/ubuntu, ownership changes with chown ubuntu:ubuntu, creation of ubuntu user with sudo privileges, and script refactoring to conditionally use sudo for privileged operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/cloud-entrypoint.sh (1)
88-93:⚠️ Potential issue | 🟠 MajorThe
mkdirandlncommands should use$SUDOfor consistency with the entrypoint's permission handling logic.The script detects non-root execution at the top (lines 3-8) and sets
SUDO=""orSUDO="sudo"accordingly. However, lines 88-93 don't use$SUDO, unlike other privileged operations in the same script (e.g., line 12). This inconsistency means the code will fail if the entrypoint runs as a non-root user without proper/workspaceownership, even though the script already has the defensive mechanism in place.Update lines 88-93 to use
$SUDO:Diff
if [ ! -d "/workspace/data/axolotl-artifacts" ]; then $SUDO mkdir -p /workspace/data/axolotl-artifacts fi if [ ! -L "/workspace/axolotl/outputs" ]; then $SUDO ln -sf /workspace/data/axolotl-artifacts /workspace/axolotl/outputs fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cloud-entrypoint.sh` around lines 88 - 93, The mkdir/ln calls in the entrypoint currently run without using the SUDO wrapper variable (SUDO) so they may fail when the script detected non-root execution; update the block that creates /workspace/data/axolotl-artifacts and the symlink /workspace/axolotl/outputs to prefix both mkdir -p and ln -sf with $SUDO (i.e., change mkdir -p and ln -sf to $SUDO mkdir -p and $SUDO ln -sf) so permission handling is consistent with the rest of the script and the SUDO variable set earlier is honored.
🧹 Nitpick comments (2)
docker/Dockerfile-uv-base (1)
24-28: Passwordless sudo grants full root access to the ubuntu user.The
NOPASSWD:ALLconfiguration allows any process running as ubuntu to execute any command as root without authentication. This is a common pattern for container entrypoints but reduces the security benefit of running as non-root.Consider restricting sudo to only the specific commands needed by the entrypoint:
🔒 Proposed restricted sudo configuration
RUN useradd -m -s /bin/bash -u 1000 ubuntu 2>/dev/null; \ usermod -aG sudo ubuntu && \ - echo 'ubuntu ALL=(ALL) NOPASSWD:ALL' > /etc/sudoers.d/ubuntu && \ + echo 'ubuntu ALL=(ALL) NOPASSWD: /usr/sbin/service ssh start, /usr/bin/tee /etc/rp_environment, /bin/sed -i * /etc/ssh/sshd_config, /bin/bash /slurm-init.sh' > /etc/sudoers.d/ubuntu && \ chmod 0440 /etc/sudoers.d/ubuntuThis limits the attack surface if the container is compromised, while still allowing the entrypoint to function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile-uv-base` around lines 24 - 28, The Dockerfile currently creates the ubuntu user and writes a full NOPASSWD:ALL sudoers entry (useradd/usermod and /etc/sudoers.d/ubuntu) which grants full root access; change the sudoers entry to restrict which commands the ubuntu user can run without a password (replace the broad "NOPASSWD:ALL" entry with a limited list or Cmnd_Alias of specific entrypoint-related commands such as the startup script, servicectl, or package commands required by the container), ensure the file written to /etc/sudoers.d/ubuntu is mode 0440 and owned by root, and test that the entrypoint still functions with the narrowed sudo privileges.docker/Dockerfile-cloud-uv (1)
27-28: Two entrypoint script locations are made executable.Line 27 makes
/workspace/axolotl/scripts/cloud-entrypoint.shexecutable (from the git clone), while line 28 makes/home/ubuntu/cloud-entrypoint.shexecutable (the copied version used by ENTRYPOINT). Both are needed but the workspace version appears unused at runtime.Consider removing the chmod for the workspace version if it's not used:
🧹 Remove unused chmod
- chmod +x /workspace/axolotl/scripts/cloud-entrypoint.sh && \ chmod +x /home/ubuntu/cloud-entrypoint.sh && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile-cloud-uv` around lines 27 - 28, The Dockerfile runs two chmod +x steps making the workspace entrypoint and the copied runtime entrypoint executable; remove the chmod for the workspace copy (the git-cloned cloud-entrypoint.sh) if it is not used at runtime, or alternatively keep both but add a clarifying comment and/or a conditional copy so only the runtime entrypoint (the one used by ENTRYPOINT) is made executable; locate the two lines performing "chmod +x" on the workspace cloud-entrypoint.sh and the home/ubuntu cloud-entrypoint.sh and remove or document the workspace chmod accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/cloud-entrypoint.sh`:
- Around line 88-93: The mkdir/ln calls in the entrypoint currently run without
using the SUDO wrapper variable (SUDO) so they may fail when the script detected
non-root execution; update the block that creates
/workspace/data/axolotl-artifacts and the symlink /workspace/axolotl/outputs to
prefix both mkdir -p and ln -sf with $SUDO (i.e., change mkdir -p and ln -sf to
$SUDO mkdir -p and $SUDO ln -sf) so permission handling is consistent with the
rest of the script and the SUDO variable set earlier is honored.
---
Nitpick comments:
In `@docker/Dockerfile-cloud-uv`:
- Around line 27-28: The Dockerfile runs two chmod +x steps making the workspace
entrypoint and the copied runtime entrypoint executable; remove the chmod for
the workspace copy (the git-cloned cloud-entrypoint.sh) if it is not used at
runtime, or alternatively keep both but add a clarifying comment and/or a
conditional copy so only the runtime entrypoint (the one used by ENTRYPOINT) is
made executable; locate the two lines performing "chmod +x" on the workspace
cloud-entrypoint.sh and the home/ubuntu cloud-entrypoint.sh and remove or
document the workspace chmod accordingly.
In `@docker/Dockerfile-uv-base`:
- Around line 24-28: The Dockerfile currently creates the ubuntu user and writes
a full NOPASSWD:ALL sudoers entry (useradd/usermod and /etc/sudoers.d/ubuntu)
which grants full root access; change the sudoers entry to restrict which
commands the ubuntu user can run without a password (replace the broad
"NOPASSWD:ALL" entry with a limited list or Cmnd_Alias of specific
entrypoint-related commands such as the startup script, servicectl, or package
commands required by the container), ensure the file written to
/etc/sudoers.d/ubuntu is mode 0440 and owned by root, and test that the
entrypoint still functions with the narrowed sudo privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a8753ec-fc32-4b9d-ab6f-55f0cec92a2a
📒 Files selected for processing (4)
docker/Dockerfile-cloud-uvdocker/Dockerfile-uvdocker/Dockerfile-uv-basescripts/cloud-entrypoint.sh
1 participant
Summary by CodeRabbit
Chores