Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop root privileges in Docker container #2825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jtesta
Copy link

@jtesta jtesta commented Sep 5, 2023

By default, Docker runs containers with root privileges (!). This isn't necessary for shellcheck. This PR causes the container to be run as an unprivileged user instead.

FYI, the highest possible UID and GID (65535) must be used in this patch since the final scratch image does not include /etc/passwd, /etc/group, nor the support code to resolve names to UIDs/GIDs.

@koalaman
Copy link
Owner

koalaman commented Oct 8, 2023

This change risks breaking CI and requires workarounds for anyone who's not checking world-readable files. Are there any Docker guidelines or conventions that recommend this approach?

@jtesta
Copy link
Author

jtesta commented Oct 9, 2023

The Center for Internet Security (CIS) Benchmark for Docker states in section 4.1 that containers should be run as non-root whenever possible (see https://www.cisecurity.org/benchmark/docker). Furthermore, running as non-root by default would be applying the Principle of Least Privilege.

As for filesystem permissions, default Ubuntu systems have a umask of 0002 (meaning files are already world-readable). So this would not be a problem. In the event that this is changed, though, users can add -u $(id -u):$(id -g) to their docker run command, which would run the container as the host user.

Because most users simply copy/paste from the documentation, we can very easily add the -u part to https://github.com/koalaman/shellcheck/blob/master/README.md?plain=1#L214. Adding -u to the CI config would also be easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants