Non root devcontainer #458
Conversation
texhnolyze
force-pushed
the
feature/non-root-dev-container
branch
from
b58a99e to
c180773
Compare
|
When testing with a fresh container, i get this: 🐋 ~/colcon_ws/src/bitbots_main gc --allow-empty -m "Foo"
ruff.................................................(no files to check)Skipped
ruff-format..........................................(no files to check)Skipped
clang-format.........................................(no files to check)Skipped
cppcheck.............................................(no files to check)Skipped
cmake-format.........................................(no files to check)Skipped
cmake-lint...........................................(no files to check)Skipped
check for merge conflicts............................(no files to check)Skipped
check toml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
detect private key...................................(no files to check)Skipped
fatal: cannot lock ref 'HEAD': Unable to create '/home/bitbots/colcon_ws/src/bitbots_main/.git/refs/heads/feature/non-root-dev-container.lock': Permission denied |
texhnolyze
force-pushed
the
feature/non-root-dev-container
branch
2 times, most recently
from
e9c612d to
5818cea
Compare
|
Interesting, I am unable to reproduce the issue. I would assume that the lock file can either not be created due to some issues with permissions or because you have some other git process running already, which has already created the file. |
texhnolyze
force-pushed
the
feature/non-root-dev-container
branch
from
9c2695e to
a6251c0
Compare
starting from `~` instead of using the whole path
to prevent issues when interacting with the repository both from within the container and outside the container, due to permissions not being correct
in `Dockerfile`, because the `updateRemoteUserUID` setting of the devcontainer does not change the `GID` of the `containerUser` dynamically to the one of the host user if the group exists in the container already microsoft/vscode-remote-release#2402. In our case the `containerUser` is set to `bitbots`, because it automatically uses the last `USER` instruction from the `Dockerfile` and the `remoteUser` inherits from `containerUser`. For reference see: microsoft/vscode-remote-release#1155
texhnolyze
force-pushed
the
feature/non-root-dev-container
branch
from
a6251c0 to
89cb2ad
Compare
WalkthroughThe pull request introduces significant changes to the development container setup in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (3)
.devcontainer/devcontainer.json (1)
Line range hint
28-41: Review security implications of privileged container configuration.The container is running with elevated privileges (
--privileged) and extensive capabilities. While this might be necessary for development purposes, consider:
- Document why these permissions are required
- Consider if a more restrictive set of capabilities would suffice
- Evaluate if all mounted devices are necessary
Consider adding a comment block explaining the security implications and requirements for each privileged setting. For example:
"runArgs": [ + // Required for temporary runtime files "--tmpfs", "/tmp:exec,mode=01777", + // Required for hardware acceleration and GUI "--privileged", "--net=host", "--device=/dev/dri:/dev/dri", "--volume=/tmp/.X11-unix:/tmp/.X11-unix", + // Required for debugging "--cap-add=SYS_PTRACE", "--security-opt", "seccomp=unconfined" ],.devcontainer/zshrc (1)
Line range hint
89-95: Update security-related instructions
- The instruction
xhost local:rootshould be updated since we're no longer using the root user. Consider usingxhost local:$(id -u)instead.- The SSH key generation command uses an empty passphrase (
-N ""). Consider recommending a passphrase for better security.Apply this update to the echo statements:
-echo "Create a ssh key with 'ssh-keygen -q -f $HOME/.ssh/id_rsa -N \"\" && cat $HOME/.ssh/id_rsa.pub'." +echo "Create a SSH key with 'ssh-keygen -t ed25519 -f $HOME/.ssh/id_ed25519' and add a secure passphrase when prompted." +echo "Display your public key with 'cat $HOME/.ssh/id_ed25519.pub'." -echo "Run 'xhost local:root' in a terminal on the host machine to enable GUI applications (e.g. rviz2) in the container. This needs to be done after every restart of the host machine." +echo "Run 'xhost local:$(id -u)' in a terminal on the host machine to enable GUI applications (e.g. rviz2) in the container. This needs to be done after every restart of the host machine.".devcontainer/Dockerfile (1)
92-92: Consider security implications of granting passwordless sudoGranting the user
$userpasswordless sudo access by adding"$user ALL=(ALL) NOPASSWD: ALL"to/etc/sudoerscan pose security risks if the container is compromised. While this setup may be acceptable for a development environment, it's important to ensure that this configuration doesn't inadvertently make its way into production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.devcontainer/Dockerfile(2 hunks).devcontainer/devcontainer.json(1 hunks).devcontainer/zshrc(1 hunks)
🧰 Additional context used
🪛 Hadolint
.devcontainer/Dockerfile
[error] 89-89: ShellCheck only supports sh/bash/dash/ksh scripts. Sorry!
(SC1071)
[error] 97-97: ShellCheck only supports sh/bash/dash/ksh scripts. Sorry!
(SC1071)
[error] 100-100: ShellCheck only supports sh/bash/dash/ksh scripts. Sorry!
(SC1071)
🔇 Additional comments (5)
.devcontainer/devcontainer.json (2)
Line range hint 43-43: Address reported Git lock file permission issues.
While postCreateCommand sets safe.directory, it doesn't fully address the reported Git lock file permission issues. Consider adding additional Git configuration or directory permission settings.
Let's verify the Git configuration in the Dockerfile:
#!/bin/bash
# Description: Check Git-related configurations and permissions
# Test: Look for Git configurations and directory permissions
# Check for Git configurations
rg "git config" .devcontainer/
# Check for Git directory permission settings
rg "chown.*\.git" .devcontainer/Consider adding these Git-specific configurations:
"postCreateCommand": "git config --global --add safe.directory '*' && \
+ git config --global core.sharedRepository group && \
+ find . -type d -name .git -exec chmod g+rwx {} +"23-24: Workspace path changes align with non-root user requirements.
The changes to workspaceMount and workspaceFolder correctly reflect the new non-root user setup, moving from /root to /home/bitbots. This aligns with the PR's objective of addressing file permission issues.
Let's verify the directory permissions will work as expected:
✅ Verification successful
Workspace directory setup is correctly configured for non-root user
The Dockerfile confirms that:
- The workspace directory
/home/bitbots/colcon_ws/srcis created with the correct user ownership - The
WORKDIRis properly set to/home/bitbots/colcon_ws - The user
bitbotsis created with configurable UID/GID - The workspace paths in devcontainer.json align with this setup
The changes in the workspace mount and folder paths are verified to work correctly with the container's user and directory structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workspace directory structure and permissions in Dockerfile
# Test: Check if the Dockerfile creates the necessary directories with correct permissions
rg -l "RUN mkdir -p.*colcon_ws" .devcontainer/
rg "WORKDIR.*colcon_ws" .devcontainer/
rg "chown.*bitbots.*colcon_ws" .devcontainer/
Length of output: 263
Script:
#!/bin/bash
# Let's check the Dockerfile content directly to verify directory setup
cat .devcontainer/Dockerfile
# Also check for any other files that might contain directory setup
fd Dockerfile .devcontainer/
Length of output: 2869
.devcontainer/zshrc (2)
53-53: LGTM! Improved prompt readability
The change from %/ to %~ with better spacing enhances readability by using tilde notation for home directory paths.
Line range hint 2-2: Verify permissions for user-specific paths
Since we're switching to a non-root user, ensure that the following paths have correct permissions for the bitbots user:
$HOME/.local/bin$HOME/.oh-my-zsh- Plugin script at
$COLCON_WS/src/bitbots_main/scripts/ros.plugin.sh
Also applies to: 38-42
✅ Verification successful
Let me verify if these paths are expected to be created during container initialization by checking the Dockerfile and related setup scripts.
No permission issues expected with the current setup
The Dockerfile shows that:
- The
bitbotsuser is properly created with correct UID/GID - Oh-my-zsh is installed during container build with correct user ownership
.local/binwill be created automatically with correct permissions when pip installs packages- The ROS plugin script reference is part of the repository setup and will be handled during workspace initialization
The paths mentioned in the review comment will be created with correct permissions during container initialization and first use, so no explicit permission fixes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify permissions for critical paths and files
# Check permissions for .local/bin and .oh-my-zsh
echo "Checking user-specific directory permissions:"
ls -ld $HOME/.local/bin $HOME/.oh-my-zsh 2>/dev/null || echo "Directories don't exist yet"
# Check plugin script permissions
echo -e "\nChecking ROS plugin script permissions:"
ls -l $COLCON_WS/src/bitbots_main/scripts/ros.plugin.sh 2>/dev/null || echo "Plugin script not found"
Length of output: 429
Script:
#!/bin/bash
# Check Dockerfile and setup scripts for directory creation
echo "Checking Dockerfile for directory creation:"
rg -l "mkdir.*\.local|mkdir.*\.oh-my-zsh" .
echo -e "\nChecking for oh-my-zsh installation:"
rg -l "sh -c.*zsh|install.*zsh" .
echo -e "\nChecking setup scripts for ROS plugin:"
rg -l "ros\.plugin\.sh" .
echo -e "\nChecking complete Dockerfile content:"
cat .devcontainer/Dockerfile
Length of output: 3502
.devcontainer/Dockerfile (1)
3-5: Introduction of build arguments for user configuration is appropriate
The addition of ARG user=bitbots, ARG uid=1000, and ARG gid=1000 provides flexibility in setting up the non-root user. This change enhances compatibility with the host system's user and group IDs.
| # Remove the users group, because when it exists on the host system | ||
| # the devcontainer will not dynamically update the containerUser GID, | ||
| # when the host user is part of the users group. | ||
| # Then create a bitbots user with home directory and add allow it to use sudo | ||
| RUN groupdel users \ | ||
| && useradd -m -U -u "$uid" -G sudo -s /bin/zsh $user \ | ||
| && groupmod -g "$gid" $user \ | ||
| && echo "$user ALL=(ALL) NOPASSWD: ALL" >> /etc/sudoers | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issues with setting SHELL to zsh affecting RUN commands
Setting the SHELL to /bin/zsh -c earlier in the Dockerfile impacts all subsequent RUN commands. Since zsh syntax differs from sh/bash, this might cause compatibility issues with scripts or commands that assume a sh-compatible shell. Additionally, static analysis tools like Hadolint and ShellCheck may report errors, as they may not fully support zsh, leading to messages like "ShellCheck only supports sh/bash/dash/ksh scripts."
Consider keeping the default SHELL for RUN commands as /bin/sh -c or /bin/bash -c to maintain compatibility and allow static analysis tools to function properly. You can set the default shell for the new user to zsh using the -s /bin/zsh option in useradd, which you've already done.
Apply the following changes:
# Remove or modify the SHELL command to use sh or bash
- SHELL ["/bin/zsh", "-c"]
+ SHELL ["/bin/bash", "-c"]Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Hadolint
[error] 89-89: ShellCheck only supports sh/bash/dash/ksh scripts. Sorry!
(SC1071)
Summary
This fixes the issue, that when utilizing the devcontainer but at the same time interacting with the repository from the host we run into issues, as files within the repo have the wrong
UID/GIDfrom within the container and are not readable, because we use therootuser in the container.Proposed changes
bitbotsuser withuid=1000, gid=1000UID,GIDby utilizingDockerfileARGbitbotsuser to run privileged commands by adjusting/etc/sudoersusersgroup in the containerWith these changes, the devcontainer will use the
bitbotsuser internally, which is initially setup withuid=1000, gid=1000, but vscode will switch theuidandgiddynamically to the one of the host user (see microsoft/vscode-remote-release#1155).Checklist