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

feat: Allow running container as non-root UID/GID for ownership issues (docker) #433

Merged
merged 29 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
38f2a48
Add read permissions to tmpfile in wrapper hook
tofupup Sep 1, 2022
4f3d238
Update docs for running docker with user
tofupup Sep 1, 2022
cd766e1
Add initial docker entrypoint script
tofupup Sep 2, 2022
c3a6529
Add entrypoint user creation and permission checks
tofupup Sep 2, 2022
78fdb40
Add entrypoint.sh to trigger github image test
tofupup Sep 2, 2022
f7871ca
Add entrypoint container structure tests
tofupup Sep 2, 2022
1ac51f8
Fix whitespace in entrypoint.sh
tofupup Sep 2, 2022
64ccfa1
Update docker documentation in README
tofupup Sep 2, 2022
9266a93
Clean up pip cache from skeleton files
tofupup Sep 2, 2022
0b4d72a
Add su-exec container structure test
tofupup Sep 3, 2022
281c53e
Use /root for new user homedir skeleton
tofupup Sep 3, 2022
dc3f996
Merge remote-tracking branch 'upstream/master' into fix-wrapper-permi…
tofupup Sep 4, 2022
a019428
Clean up Dockerfile apk installs
tofupup Sep 5, 2022
0988930
Clean up root su-exec
tofupup Sep 5, 2022
5ad0a7d
Clean up variable references
tofupup Sep 5, 2022
367f0a4
Create function for error reporting
tofupup Sep 5, 2022
ed40055
Remove extraneous braces from variables
tofupup Sep 5, 2022
6b3f6a9
Add check to insure container is running as root
tofupup Sep 5, 2022
12d8526
Correct initial UID check
tofupup Sep 6, 2022
c5e4d01
Split long gid error into multiline
tofupup Sep 6, 2022
070b8a2
Split long uid failure error message
tofupup Sep 6, 2022
ea6f65f
Split long id -G error message
tofupup Sep 6, 2022
87541f4
Split long addgroup error message
tofupup Sep 6, 2022
ab15d72
Fix bad pattern match for group info
tofupup Sep 6, 2022
527a521
Separate error message array from string
tofupup Sep 6, 2022
8df4f20
Fix entrypoint container test with new error msg
tofupup Sep 6, 2022
44263d0
Pin su-exec more strictly
MaxymVlasov Sep 7, 2022
c16e2f4
Reorder docs and fix minor issues
MaxymVlasov Sep 7, 2022
4064ba1
Update README.md
antonbabenko Sep 7, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
*
!.dockerignore
!Dockerfile
!tools/entrypoint.sh
13 changes: 13 additions & 0 deletions .github/.container-structure-test-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ commandTests:
args: [ "version" ]
expectedOutput: [ "([0-9]+\\.){2}[0-9]+\\n$" ]

- name: "entrypoint.sh"
envVars:
- key: "USERID"
value: "1000:1000"
command: "/entrypoint.sh"
args: [ "-V" ]
expectedOutput: ["^user:gid 1000:1000 lacks permissions to //\\n$"]
exitCode: 1

- name: "su-exec"
command: "su-exec"
expectedOutput: ["^Usage: su-exec user-spec command \\[args\\]\\n$"]

fileExistenceTests:
- name: 'terrascan init'
path: '/root/.terrascan/pkg/policies/opa/rego/github/github_repository/privateRepoEnabled.rego'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/build-image-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ jobs:
with:
files: |
Dockerfile
.dockerignore
tools/entrypoint.sh

- name: Build if Dockerfile changed
if: steps.changed-files-specific.outputs.any_changed == 'true'
Expand Down
7 changes: 5 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,14 @@ RUN if [ "$(grep -o '^terraform-docs SKIPPED$' /usr/bin/tools_versions_info)" =
; fi && \
# Fix git runtime fatal:
# unsafe repository ('/lint' is owned by someone else)
git config --global --add safe.directory /lint
git config --global --add safe.directory /lint && \
apk add --no-cache su-exec=~0
tofupup marked this conversation as resolved.
Show resolved Hide resolved

COPY tools/entrypoint.sh /entrypoint.sh

ENV PRE_COMMIT_COLOR=${PRE_COMMIT_COLOR:-always}

ENV INFRACOST_API_KEY=${INFRACOST_API_KEY:-}
ENV INFRACOST_SKIP_UPDATE_CHECK=${INFRACOST_SKIP_UPDATE_CHECK:-false}

ENTRYPOINT [ "pre-commit" ]
ENTRYPOINT [ "/entrypoint.sh" ]
44 changes: 42 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ If you are using `pre-commit-terraform` already or want to support its developme
* [terraform_wrapper_module_for_each](#terraform_wrapper_module_for_each)
* [terrascan](#terrascan)
* [tfupdate](#tfupdate)
* [Docker Usage](#docker-usage)
* [File Permissions](#file-permissions)
* [Other Settings](#other-settings)
* [1. Module short name for terraform_wrapper_module_for_each](#1-module-short-name-for-terraformwrappermoduleforeach)
* [Authors](#authors)
* [License](#license)
* [Additional information for users from Russia and Belarus](#additional-information-for-users-from-russia-and-belarus)
Expand Down Expand Up @@ -227,16 +231,18 @@ pre-commit run -a

Or, using Docker ([available tags](https://github.com/antonbabenko/pre-commit-terraform/pkgs/container/pre-commit-terraform/versions)):

**NOTE:** This command uses your user id and group id for the docker container to use to access the local files. If the files are owned by another user, update the ```USERID``` environment variable. See [File Permissions](#file-permissions) for more information.

```bash
TAG=latest
docker run -v $(pwd):/lint -w /lint ghcr.io/antonbabenko/pre-commit-terraform:$TAG run -a
docker run -e "USERID=$(id -u):$(id -g)" -v $(pwd):/lint -w /lint ghcr.io/antonbabenko/pre-commit-terraform:$TAG run -a
```

Execute this command to list the versions of the tools in Docker:

```bash
TAG=latest
docker run --entrypoint cat ghcr.io/antonbabenko/pre-commit-terraform:$TAG /usr/bin/tools_versions_info
docker run --rm --entrypoint cat ghcr.io/antonbabenko/pre-commit-terraform:$TAG /usr/bin/tools_versions_info
```

## Available Hooks
Expand Down Expand Up @@ -779,6 +785,40 @@ Sample configuration:
Check [`tfupdate` usage instructions](https://github.com/minamijoyo/tfupdate#usage) for other available options and usage examples.
No need to pass `--recursive .` as it is added automatically.

## Docker Usage

### File Permissions

A mismatch between the Docker container's user and the local repository file ownership can cause permission issues in the repository where pre-commit is run. The container runs as the ```root``` user by default, and uses an entrypoint script to assume a user ID and group ID if specified by environment variable ```USERID```.

The [recommended command](#4-run) to run the Docker container is:

```bash
TAG=latest
docker run -e "USERID=$(id -u):$(id -g)" -v $(pwd):/lint -w /lint ghcr.io/antonbabenko/pre-commit-terraform:$TAG run -a
```

which uses your current session's user ID and group ID to set the variable in the run command. Without this setting, you may find files and directories owned by ```root``` in your local repository.

If the local repository is using a different user or group for permissions, you can modify the USERID to the user ID and group ID needed. **Do not use the username or groupname in the environment variable, as it has no meaning in the container.** You can get the current directory's owner user ID and group ID from the 3rd (user) and 4th (group) columns in ```ls``` output:

```bash
$ ls -aldn .
drwxr-xr-x 9 1000 1000 4096 Sep 1 16:23 .
```

### Other Settings

#### 1. Module short name for ```terraform_wrapper_module_for_each```

The [terraform_wrapper_module_for_each](#terraformwrappermoduleforeach) hook attempts to determine the module's short name to be inserted into the generated ```README.md``` files for the ```source``` URLs. Since the container uses a bind mount at a static location, it can cause this short name to be incorrect. If the generated name is incorrect, it can be set by providing the ```module-repo-shortname``` option to the hook.

```yaml
- id: terraform_wrapper_module_for_each
args:
- '--args=--module-repo-shortname=ec2-instance' # module repo short name
```

## Authors

This repository is managed by [Anton Babenko](https://github.com/antonbabenko) with help from these awesome contributors:
Expand Down
3 changes: 3 additions & 0 deletions hooks/terraform_wrapper_module_for_each.sh
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,9 @@ function create_tmp_file_tf {
mv "$tmp_file" "$tmp_file.tf"
tmp_file_tf="$tmp_file.tf"

# mktemp creates with no group/other read permissions
chmod a+r "$tmp_file_tf"

echo "$CONTENT_MAIN_TF" > "$tmp_file_tf"
}

Expand Down
80 changes: 80 additions & 0 deletions tools/entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#!/usr/bin/env bash
#exit on error
set -e

readonly USERBASE="run"

# make sure USERID makes sense as UID:GID
# it looks like the alpine distro limits UID and GID to 256000, but
# could be more, so we accept any valid integers
USERID=${USERID:-"0:0"}
if [[ ! $USERID =~ ^[0-9]+:[0-9]+$ ]]; then
echo "USERID environment variable invalid, format is userid:groupid. Received: \"${USERID}\""
exit 1
fi

# separate uid and gid
uid=${USERID%%:*}
gid=${USERID##*:}

# if requested UID:GID is root, go ahead and run without other processing
if [[ ${uid} == 0 && ${gid} == 0 ]]; then
exec su-exec "0:0" pre-commit "$@"
fi
tofupup marked this conversation as resolved.
Show resolved Hide resolved

# make sure workdir and some files are readable/writable by the provided UID/GID
# combo, otherwise will have errors when processing hooks
wdir="$(pwd)"
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For brevity it seems use of $USERID instead of ${uid}:${gid} fits better.
  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.
  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.
  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 🤔
  5. /bin/bash string is use multiple times across the script, thus might be a good idea to convert it to a variable.
  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) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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

Copy link
Contributor Author

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


# check if group by this GID already exists, if so get the name since adduser
# only accepts names
if groupinfo="$(getent group "${gid}")"; then
groupname="${groupinfo%%:*}"
else
# create group in advance in case GID is different than UID
groupname="${USERBASE}${gid}"
if ! err="$(addgroup -g "${gid}" "${groupname}" 2>&1)"; then
echo "failed to create gid \"${gid}\" with name \"${groupname}\""
echo "command output: ${err}"
exit 1
fi
fi

# check if user by this UID already exists, if so get the name since id
# only accepts names
if userinfo="$(getent passwd "${uid}")"; then
username="${userinfo%%:*}"
else
username="${USERBASE}${uid}"
if ! err="$(adduser -h "/home/${username}" -s "/bin/bash" -G "${groupname}" -D -u "${uid}" -k "${HOME}" "${username}")"; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. -D looks redundant as of -k
  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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. -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?

echo "failed to create uid \"${uid}\" with name \"${username}\" and group \"${groupname}\""
echo "command output: ${err}"
exit 1
fi
fi

# it's possible it was not in the group specified, add it
if ! idgroupinfo="$(id -G "${username}" 2>&1)"; then
echo "failed to get group list for username \"${username}\""
echo "command output: ${idgroupinfo}"
exit 1
fi
if [[ ! " ${idgroupinfo} " =~ [:blank:]${gid}[:blank:] ]]; then
if ! err="$(addgroup "${username}" "${groupname}")"; then
echo "failed to add user \"${username}\" to group \"${groupname}\""
echo "command output: ${err}"
exit 1
fi
fi

# user and group of specified UID/GID should exist now, and user should be
# a member of group, so execute pre-commit
exec su-exec "${uid}:${gid}" pre-commit "$@"
tofupup marked this conversation as resolved.
Show resolved Hide resolved