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

Add user to container to support running as non-root #428

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

Conversation

kinghrothgar
Copy link
Contributor

@kinghrothgar kinghrothgar commented Jul 1, 2022

Running docker containers as root in production is not good security practice for a lot of reasons. It is difficult to use the images provide without running as root there is no user to run it as. This PR adds a user so that one can run hound as non-root.

I did not change the default user of the container so this should have no effect on those already utilizing the container and expecting it to run as root.

This will allow one to easily run the container as non-root with Docker, Kuberntes, etc (See README change).

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

The PR fulfills these requirements:

  • All tests are passing?
  • New/updated tests are included?
  • If any static assets have been updated, has ui/bindata.go been regenerated?
  • Are there doc blocks for functions that I updated/created?

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Copy link
Contributor

@salemhilal salemhilal left a comment

Choose a reason for hiding this comment

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

Hey! Thank you for all of the PRs you've opened, and apologies for the radio silence on our end (there were a few overlapping sabbaticals and vacations).

Supporting running this image as non-root seems like a good idea. I'm a little unclear on the comment you left and I'd like to understand that a bit more. That being said, no real concerns here. Help me understand that comment and then this is good to merge.

@@ -13,6 +13,11 @@ RUN apk update \
&& rm -f /var/cache/apk/* \
&& rm -rf /go/src /go/pkg

# Hardcode gid and uid so that it never changes. This changing will break
# users running this as nonroot in production as you run it with the uid directly,
# not the user name.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a little unclear (I'm admittedly less versed in user management) — When you say "This change will break users running this as nonroot", do you mean running the docker image as nonroot? I don't totally understand what the breaking change here is.

# users running this as nonroot in production as you run it with the uid directly,
# not the user name.
RUN addgroup -g 1000 hound && adduser -u 1000 -G hound -D hound

Copy link

Choose a reason for hiding this comment

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

Why not add USER 1000:1000 right here? Then you can skip the advice in the readme. Is there an actual reason to run hound as root, like ever?

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