getsentry / docker-sentry

Docker Official Image packaging for Sentry
https://sentry.io
Other
349 stars 150 forks source link

Possibility to run as non-root user #171

Closed maxdietrich closed 5 years ago

maxdietrich commented 5 years ago

Hello, I would like to deploy Sentry to a Red Hat Openshift cluster (Kubernetes). The problem I have is, that this image by default runs as the root user, which is a security issue and generally not encouraged in Openshift. Is there an option to run this as a non-root user? I can't seem to find anything about that in the documentation.

BYK commented 5 years ago

Hi! We use gosu but I'm not sure whether we really need root permissions or if we drop them afterwards. My only guess would be for accessing any protected ports but that should also not be necessary as docker allows port mapping. @mattrobenolt can you help here?

@maxdietrich, in the meantime, you can try removing the gosu command from the entrypoint file and use USER in the docker file to switch to a different user. You'd need to create this user and set a specific GID and UID as recommended here: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#user

maxdietrich commented 5 years ago

Thanks for your response! In the mean time we decided to go for the cloud hosted solution instead of running it ourselves though. I'd leave the issue open until that question is resolved, since this could probably affect others who would like to run this in Openshift as well, but feel free to close it if you want.

BYK commented 5 years ago

I'll talk to @mattrobenolt to get clarification and then keep it open or close accordingly. Glad you got around the issue (bonus points for using the cloud version 😁 )

mattrobenolt commented 5 years ago

So yeah, out of the box, it drops permissions with gosu if it is run by root. But you can also run it as a user with docker run --user if you’d like and it should be just fine. It’d bypass using gosu and just use whatever user you specify.

mattrobenolt commented 5 years ago

For context, this happens here. https://github.com/getsentry/docker-sentry/blob/master/9.1/docker-entrypoint.sh#L19

BYK commented 5 years ago

Closing as I think @mattrobenolt's answer is sufficient and there's no action we can take immediately.

alexanderadam commented 5 years ago

Just for clarification:

  1. What exactly is the advantage of not having the image run as user per default?
  2. And what exactly is the advantage of not having the switch to a proper unprivileged user within the Dockerfile (and therefore not even having an attack surface if the command is overriden)?
BYK commented 5 years ago

@alexanderadam we optimize for defaults and Docker's default is to run as root by default. If we require people to use a less privileged user, I'm sure we'll get more issues as people tend to skip reading the docs most of the time.

alexanderadam commented 5 years ago

@BYK this is probably a misunderstanding regarding Docker's "default". root is usually only required for setting up the image because administrative tools require super user rights for obvious reasons.

But in the end of building the Dockerfile, the image should switch to a non-privileged user (this way no entrypoint/commend could get privileged access). The official documentation is very clear about users and mention it even the "Best Practices" part:

If a service can run without privileges, use USER to change to a non-root user.

And I can't see any reason to let Sentry run as root. So if you really want to follow Docker's recommendation, you should avoid using root.

If we require people to use a less privileged user, I'm sure we'll get more issues as people tend to skip reading the docs most of the time.

Do you have any particular reasons for that assumption? Is there any functionality that wouldn't work without having root?

BYK commented 5 years ago

I don't think there would be any issues without root as we already don't run Sentry w/ root as @mattrobenolt pointed out above: https://github.com/getsentry/docker-sentry/blob/master/9.1/docker-entrypoint.sh#L19

It is just done in the entrypoint file. I'm guessing we can move that logic to the last step of the image builds if I understand your comment correctly?

mattrobenolt commented 5 years ago

The main reason is for the file mount: https://github.com/getsentry/docker-sentry/blob/master/9.1/docker-entrypoint.sh#L20-L21

This needs to be owned by the user sentry is running at so it has permissions to read/write. If you just mount a VOLUME on your own, you'd need to match the uid/guid of the user running inside the container. You can do this on your own, but out of the box, it'll otherwise be the wrong permission.

alexanderadam commented 5 years ago

@mattrobenolt I guess adding documentation to use a proper uid (or at least a writable directory) for Docker volumes might be okay, doesn't it? Or do I oversimplify here?

mattrobenolt commented 5 years ago

That is definitely an option, and we'd have to guarantee that our uid+guid doesn't change. We don't currently enforce that.

alexanderadam commented 5 years ago

That is definitely an option

Awesome, that sounds good!

we'd have to guarantee that our uid+guid

Of course. You mean something like useradd -r -m -g sentry sentry --uid 1001, right?

mattrobenolt commented 5 years ago

Yeah, most likely. Not sure off the top of my head.

alexanderadam commented 5 years ago

So would it be okay to reopen this issue then, now that it's validity got 'restored'? :wink:

mattrobenolt commented 5 years ago

I mean, if you insist. I'm not sure this is a real issue. This is just a pattern that a lot of the official images follow.

I guess the only actionable thing here is for us to update documentation and explicitly choose a uid+guid for our sentry user, since we still won't be changing the default USER instruction.

alexanderadam commented 5 years ago

oh, then I misunderstood this conversation. Why exactly wouldn't you consider adding USER to the Dockerfile?

mattrobenolt commented 5 years ago

This needs to be owned by the user sentry is running at so it has permissions to read/write. If you just mount a VOLUME on your own, you'd need to match the uid/guid of the user running inside the container. You can do this on your own, but out of the box, it'll otherwise be the wrong permission.

It's a bad default if it's just broken. This isn't always a trivial thing to do depending on your deployment.

This is basically the pattern of every official docker image, anything that writes data to disk.

mattrobenolt commented 5 years ago

See https://github.com/docker-library/postgres/blob/master/12/docker-entrypoint.sh#L31-L49

They all provide the ability for you to do your own thing by passing --user, which we support. But without having root privileges when it starts up, the onboarding experience is bad since it's broken, and forcing people to mount a volume with very specific permissions is not always possible. For example, I have no idea how you'd achieve this in Kubernetes without some initContainer that ran as root instead, just to change permissions. In which case, that's literally what's happening here.

mattrobenolt commented 5 years ago

I looked into this a bit more to make sure I wasn't overlooking anything, and you'd need to do this in Kubernetes if the container ran as a non-root user: https://stackoverflow.com/a/51195446

At this point, you have to spin up a container before your main container as root, just to run chown, then the actual container is free to run as a non-root user since permissions were swapped.

So if the goal is to not run a container as root, well, you still need to run a container as root. Just a different container. So changing this behavior out of the box is only going to cause other issues for similar scenarios.

If this is a concern if yours, you can very well just pass --user manually and do what you want. That's why the container was designed to work this way. If this behavior doesn't work, let us know and I'll be sure to fix it. But I don't want to change the default behaviors here. The processes already don't run as root. The only thing that runs as root is the docker-entrypoint.sh file, which quickly exec's and drops it's own permissions. You can work around this clearly by passing your own --user.

mattrobenolt commented 5 years ago

I think this would also work? https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod

But in this case again, you just set all permissions right there yourself. You can just easily set the entire securityContext at one time for all bits to be the same permission including the volume.

If we have a different default USER, that means someone would still need to configure a securityContext anyways. I'd argue that while oyu're in there overriding securityContext, you might as well just set them all. There'd be no reason to just set fsGroup without runAsUser and runAsGroup.

I'm happy to document these options, but strong -1 on changing the default since this is more complex. I'm not even sure if you can mimic this with a normal docker volume, which you'd use in docker-compose. I don't know much about that environment.