minio / minio

MinIO is a high-performance, S3 compatible object store, open sourced under GNU AGPLv3 license.
https://min.io/download?license=agpl&platform=linux
GNU Affero General Public License v3.0
48.25k stars 5.52k forks source link

Run as non-root user inside Docker container #5742

Closed nitisht closed 5 years ago

nitisht commented 6 years ago

Not using template on purpose

Minio should run as a non-root user inside Docker container for new deployments. Pre-existing Docker container deployments where users are updating to newer images, should continue to run without changes.

harshavardhana commented 6 years ago

didn't we have an older bug on this?

bitva77 commented 6 years ago

Is this possible? I have a scenario where I need the files Minio is writing to be owned by a specific UID. It would be great if Minio could just run as that UID. That possible?

Thanks!

precurse commented 6 years ago

This is most definitely possible. Running with root privileges is a very bad idea, even if the process is isolated in a container. Projects like plexmediaserver-docker and docker-transmission are a couple projects off the top of my head that allow specifying a user and group id of the running process at runtime. If not specified, it falls back to another unprivileged user.

I'll look at updating the Dockerfile to add this functionality if nobody else does.

rmoriz commented 6 years ago

Running minio as root is very bad. However I don't see an easy solution to keep it backwards compatible.

the entrypoint is just a wrapper for minio so there is no way to know what volumes are mounted and if they need to be chowned.

precurse commented 6 years ago

Starting up as root isn't an issue, but rather the not dropping privileges after container startup. It's fine to start with root, do things like UID/GID chown'ing, then drop to the user that minio runs as.

Those projects I referred to use this method. None of their Dockerfiles drop the privileges -- that's done at runtime for this exact purpose :)

gjonespf commented 6 years ago

I notice the PR for this is to use a named "minio" user with a set UID/GID. Is great to at least shift away from root, but is there any reason why we can't specify the UID/GID for this when running minio container, to ensure Minio can only read and write to folders with privs associated and allow better interop? Especially useful where you may want to use a specific copy of Minio to write to a specific volume with UID/GID set, to interop with e.g. nginx hosting (www-data:1000), or some other container. In terms of implementation, you could leave everything same in the PR (e.g. username) and allow setting UID/GID as part of the entrypoint script. If the owner/UID doesn't match for minio.sys you can still fail hard.

nitisht commented 6 years ago

@gjonespf while this will allow greater flexibility, it also will open scope for users to mishandle the UID/GID rendering their data inaccessible. I am discussing with the team internally to see how best to handle this

gjonespf commented 6 years ago

Just to be clear, I love your work guys 👍 Chiming in here as it's something I'm passionate about.

@nitisht To answer your question, does it give users an opportunity to shoot themselves in the foot? Sure. But as a user, if you're fiddling with these settings, you should probably be aware that it may break things anyway. At the end of the day, as long as I can customise a dockerfile to do what I need, I'll be happy. I imagine that others are similar. I'm pointing this particular gripe out as I feel it's a really useful use case that others would definitely make use of.

As I see it, you basically have two scenarios for the storage folder:

  1. Where minio has created the minio homedir, and minio basically has carte blanche.
  2. Where you're mounting an existing volume and minio needs to check it has privs to do its work.

It's this second case I think that's certainly most common for how I'm using minio (90% of my current use). In this case, you're going to run into issues anyway if the storage folder is setup for root, and you try dropping privs to the minio user. Minio should probably fail fast here if it doesn't have privs with a useful error. Minio has no idea how many files are in the storage dir, or how long it will take to update them, so chowning is probably not a good idea. I guess you could retain a default of running as root in this case, but suggest if the intention is to be secure by default, then having minio fail by default here would be better option (perhaps with an "insecure" parameter to continue to run as root). And as a user of this docker image, that means you're going to have to ensure that the files in the mounted directory are readable by the arbitrary UID/GID you're using in the image. Any kind of interop with these files or the docker volume associated will also have to use this UID/GID, or alternatively have open enough perms that it won't be an issue.

For me, not having the UID/GID overridable renders the current minio docker image unusable for a bunch of common (very useful) interop scenarios, such as the one I've outlined above (https://github.com/minio/minio/issues/5742#issuecomment-411903181). Where you're using minio as a tool alongside another container, you're pretty likely to run into this issue and in this case you currently have no choice but to make your own custom docker image. I guess as long as the way this PR is implemented still allows for someone to define a custom dockerfile to set the UID/GID, it's not a big deal.

nitisht commented 6 years ago

Just to be clear, I love your work guys +1 Chiming in here as it's something I'm passionate about.

Comments are most welcome @gjonespf :) . Thank you for the kind words!

Minio should probably fail fast here if it doesn't have privs with a useful error. Minio has no idea how many files are in the storage dir, or how long it will take to update them, so chowning is probably not a good idea

Yes, this is what we are intend to do.

in this case you currently have no choice but to make your own custom docker image.

yes, we understand this problem and want to have a clean / effective way for users to set the UID/GID. Mostly as environment variable(s) - which if not set will fall back to default values.

gjonespf commented 6 years ago

Thank you for the kind words!

You are most welcome, it's an amazing piece of kit! 😄

yes, we understand this problem and want to have a clean / effective way for users to set the UID/GID. Mostly as environment variable(s) - which if not set will fall back to default values.

Thanks, that sounds excellent. I figured you were aware of the complexity, but it wasn't clear from your previous comment whether you were going to persue allowing override of the UID/GID (it sounded like you weren't). Looking at the current PR, the UID/GID don't appear overridable so I thought it best to clarify the need.

kannappanr commented 6 years ago

@sinhaashish Do you have any updates on this issue?

sinhaashish commented 6 years ago

@sinhaashish Do you have any updates on this issue?

PR is already under review.

bicquet commented 5 years ago

fwiw, here's a workaround to waiting on any minio PRs, where i ended up extending the dockerfile and regularly rebuilding it through our ci. it's been working fine (as a stopgap?) for us.

https://gist.github.com/justbk/384f41766b787afda84d623c95f4ba10

extending the minio dockerfile in this way allows/maintains non-root user host system file ownership of everything passed through the s3 api

when you build the image, make sure to pass in appropriate values for:

ARG MINIO_USER
ARG MINIO_UID
ARG MINIO_GID

then start it up with something like:

/usr/bin/docker run \
  --restart unless-stopped \
  -p ${MINIO_HOST_PORT}:9000 \
  --name ${MINIO_CONTAINER_NAME} \
  --user ${MINIO_UID}:${MINIO_GID} \
  -e "MINIO_ACCESS_KEY=${MINIO_ACCESS_KEY}" \
  -e "MINIO_SECRET_KEY=${MINIO_SECRET_KEY}" \
  -v ${MINIO_VOLUME_CONF}:/home/${MINIO_USER}/.minio \
  -v ${MINIO_VOLUME_DATA}:/data \
  ${MINIO_DOCKER_IMAGE} gateway nas /data
sinhaashish commented 5 years ago

PR https://github.com/minio/minio/pull/7569#issuecomment-488781548 fixes this issue

harshavardhana commented 5 years ago

Fixed by #7569