-
-
Notifications
You must be signed in to change notification settings - Fork 540
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
feat: Allow running container as non-root UID/GID for ownership issues (docker) #433
feat: Allow running container as non-root UID/GID for ownership issues (docker) #433
Conversation
@MaxymVlasov @yermulnik What do you think? |
tools/entrypoint.sh
Outdated
if ! su-exec "${uid}:${gid}" "/bin/bash" -c "test -w ${wdir} && test -r ${wdir}"; then | ||
echo "user:gid ${uid}:${gid} lacks permissions to ${wdir}/" | ||
exit 1 | ||
fi | ||
if ! su-exec "${uid}:${gid}" "/bin/bash" -c "test -w ${wdir}/.git/index && test -r ${wdir}/.git/index"; then | ||
echo "user:gid ${uid}:${gid} cannot write to ${wdir}/.git/index2" | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For brevity it seems use of
$USERID
instead of${uid}:${gid}
fits better. - Curly brackets around var name are used for brace expansion which is not a matter here, hence they are not needed. This item relates to the whole script and curly brackets should better be removed from all places where brace expansion is not used.
- Typo in line 33 (
…/index2"
<- the2
seems to be redundant). Converting path to a variable based on$wdir
could have helped to avoid this. - I'm not quite sure, hence the question: isn't this going to always fail if
USERID
var has non-existing UID/GID as value? What I mean is down the code you add UID/GID to the container system, so thatsu-exec
can use it, though you do the check before adding UID/GID to the system which seemingly is a failure point 🤔 /bin/bash
string is use multiple times across the script, thus might be a good idea to convert it to a variable.- Would be good to prepend failure massages with some identifier like
ERROR:
(this is not essential, though could help to improve UX). Also it may be a good idea to redirect such messages to stderr (echo … >&2
) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For brevity it seems use of
$USERID
instead of${uid}:${gid}
fits better.
Agreed, included with changes for 3 (wdir
) and 5 (bash
path) below in 5ad0a7d
2. Curly brackets around var name are used for brace expansion which is not a matter here, hence they are not needed. This item relates to the whole script and curly brackets should better be removed from all places where brace expansion is not used.
I chose using braces for consistency, and generally try to follow the google shell style guide which prefers double quoting and braces for variable expansion. But I am certainly not married to this, so will submit a commit with them removed where not necessary
3. Typo in line 33 (`…/index2"` <- the `2` seems to be redundant). Converting path to a variable based on `$wdir` could have helped to avoid this.
Agreed and thanks, (done with 1 and 5) 5ad0a7d
4. I'm not quite sure, hence the question: isn't this going to always fail if `USERID` var has non-existing UID/GID as value? What I mean is down the code you add UID/GID to the container system, so that `su-exec` can use it, though you do the check **before** adding UID/GID to the system which seemingly is a failure point 🤔
It shouldn't fail, as su-exec
doesn't require an existing user or group to successfully execute. We could call su-exec
out of the gate without doing the adduser, etc, but the su-exec
session wouldn't have a HOME
, or be a real user. These checks don't need to be before creating the user/group in the container, but I figured it made sense to check before bothering to do that work (especially as populating the user's HOME
will write some amount of data to the container).
The reason I think it makes sense to create a "real" user is to allow pre-populating things like terrascan
init information, giving a good location for pre-commit
cache, and if hook functionality ends up assuming it's the case.
I'm definitely open that my thinking here is wrong, or moving the checks, just let me know!
5. `/bin/bash` string is use multiple times across the script, thus might be a good idea to convert it to a variable.
Agreed (done with 1 and 3), 5ad0a7d
6. Would be good to prepend failure massages with some identifier like `ERROR: ` (this is not essential, though could help to improve UX). Also it may be a good idea to redirect such messages to stderr (`echo … >&2`) 🤔
Agreed, added function echo_error_and_exit
for error reporting with sending to stderr
with script abort 367f0a4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed ed40055 for the braces issue in 2 above
tools/entrypoint.sh
Outdated
username="${userinfo%%:*}" | ||
else | ||
username="${USERBASE}${uid}" | ||
if ! err="$(adduser -h "/home/${username}" -s "/bin/bash" -G "${groupname}" -D -u "${uid}" -k "${HOME}" "${username}")"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-D
looks redundant as of-k
- Wouldn't it make sense to copy dotfiles from
/etc/skel
(I guess it's Alpine's base location of skeleton dir) instead of from home dir of the user which whose permissionsadduser
is executed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-D
looks redundant as of-k
True, we could specify just -D if we use /etc/skel as skeleton
2. Wouldn't it make sense to copy dotfiles from `/etc/skel` (I guess it's Alpine's base location of skeleton dir) instead of from home dir of the user which whose permissions `adduser` is executed?
I used root's directory as skeleton as it already has terrascan init
output in it from the builder
stage, which is copied to later stages. An earlier commit I had copied it into /etc/skel
, and then used that for the adduser
, but it seemed redundant to just turn around and copy it again during the entrypoint
script. Putting it into /etc/skel would allow future configurations to just the user's environment to be placed during the docker image build, so just let me know if you'd want to go that route?
It looks reasonable for the described use case of mismatched ownership. |
Co-authored-by: George L. Yermulnik <[email protected]>
@yermulnik Thank you so much for your time and detailed feedback on these changes, I really appreciate it. I made one additional change that I think is appropriate for this PR, in 6b3f6a9, to make sure the container is running at root before going through the script, and provide a usage example if not. |
@tofupup Thanks for your comments. They make sense. Let me give another round of review to the PR and let's wait for @MaxymVlasov to add his thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider the below comments. Thanks!
@yermulnik Thanks again, I implemented all of your great feedback, except I just had one remaining question about the |
I'm waiting for the end of @yermulnik reviews iterations :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🤝 😂 |
Oops, didn't expect it's only one approving review is required for merging. Let Max be the final approver.
Ah, that was GH rate limits, not unzip issue, I remove that changes. Sorry |
8fd276f
to
8df4f20
Compare
su-exec now seems too unstable (https://semver.org/spec/v2.0.0.html#spec-item-4), to be able to pin only major version. Now installed 0.2-r1, in alpine edge exist 0.2-r2 package. I hope that will be no breaking changes in 0.2.x, so pin only MAJOR.MINOR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I have only one concern, as https://github.com/kelseyhightower/nocode lover: who will maintain tools/entrypoint.sh
if something will go wrong? But I hope, that there is nothing that can be dramatically changed and it is fairly well documented
# [1.75.0](v1.74.2...v1.75.0) (2022-09-07) ### Features * Allow running container as non-root UID/GID for ownership issues (docker) ([#433](#433)) ([abc2570](abc2570))
This PR is included in version 1.75.0 🎉 |
Wow! What impressive cooperation on the PR by @tofupup @yermulnik @MaxymVlasov ! |
Sincere thanks to @MaxymVlasov and @yermulnik for all of the time and detailed knowledge expended on this. For @MaxymVlasov, I will definitely try to keep on top of issues related to the new code, but as you say hopefully the comments help if someone else has to touch it. |
Put an
x
into the box if that apply:Description of your changes
To fix permission issues for the container running as root, the
terraform_wrapper_module_for_each
module is changed to add read permissions to generatedmain.tf
files. Also, an entrypoint wrapper script is added to create a user/group in the container that matches the desired UID and GID.There is a "naive" version that just patches
terraform_wrapper_module_for_each
and updates the README documentation with a changeddocker run
command at https://github.com/tofupup/pre-commit-terraform/tree/4f3d2386a2d10a1a99276bd6f69f9bbe3590a15e (first 2 commits in this branch in my fork)The entrypoint wrapper script supports su-ing to a non-root user. It should not break existing functionality/usage of the container. If the container is run without specifying the
USERID
environment variable it will run as root. Running as root and su-ing to another user gives flexibility for the UID and GID used to run the container, without having to pre-build an image with static values, and also allows creating a "real" user and group inside the container.Example run command:
Dockerfile
su-exec
, copy entrypoint script and configureENTRYPOINT
tools/entrypoint.sh
USERID
as0:0
, or not set, short circuits and runs pre-commit.workdir
, and will error if not/root
is used as the skeleton for new user's directory. This gets the .gitconfig that marks the workdir as safe, as well as avoids anotherterrascan init
.su-exec
is used to su to the requested user, instead ofgosu
..github/.container-structure-test-config.yaml
.github/workflows/build-image-test.yaml
hooks/terraform_wrapper_module_for_each.sh
create_tmp_file_tf
, as these files are moved into the repo. Other calls tomktemp
in other hooks are truly temporary files.README.md
USERID
and permissions/ownershipFixes #432
How can we test changes
All tests run using the terraform-aws-ec2-instance repo as base for testing. We compare sha256sums of all files after a run -a (excluding .git/ and .terraform/ directories and the files within, as they can vary without the contents being different). We also compare the stat output of all of the files (excluding .git/ as some filenames are variable) to verify permissions are the same, or what we're expecting.
build container with entrypoint script
❯ docker build --no-cache -t pre-commit-terraform:entrypoint --build-arg INSTALL_ALL=true .
current docker container
new container version
❯ docker run -v $(pwd):/lint -w /lint pre-commit-terraform:entrypoint --version pre-commit 2.20.0 ❯ docker run --entrypoint cat pre-commit-terraform:entrypoint /usr/bin/tools_versions_info pre-commit 2.20.0 Terraform v1.2.8 checkov 2.1.182 Infracost v0.10.11 terraform-docs version v0.16.0 1f686b1 linux/amd64 terragrunt version v0.38.9 terrascan version: v1.15.2 TFLint version 0.39.3 tfsec v1.27.6 tfupdate 0.6.7 hcledit 0.2.6
new container with no user environment variable specified
new container with USERID=0:0
Verify the output of the original container, and the new containers run with root permissions output the same files and permissions
Files not owned by UID 1000 after run
Run setting USERID to 1000:1000 (matching repository files)
new container verify running single hook
Run setting USERID to 1000:2000 (UID matches, but GID does not)
As expected the UIDs of all files are correct, but the GID of the same files that were set to root in the original run is now 2000.
Run with invalid USERID
Run with USERID 2000:3000, no permissions on repository
Run with USERID 2000:1000, no write permissions on repository, but do have read