kekru / docker-remote-api-tls

Docker Image that forwards to the Docker API Socket and requires TLS Client authentication
https://hub.docker.com/r/kekru/docker-remote-api-tls
MIT License
76 stars 25 forks source link

Fix Docker Socket Permissions #2

Closed smiller171 closed 6 years ago

kekru commented 6 years ago

Hi @smiller171 thanks for your PR. Also thanks to @wershlak for #1, which I totally forgot.

Is the newly added nginx.conf a copy of the nginx.conf in the nginx:alpine image?

I would prefer to not copy the nginx.conf. Because if the original file changes, we would still use the old one. We could just replace the user in the nginx.conf with something like this in the Dockerfile:

RUN sed -i "s|user[[:space:]]+nginx;|user root;|g" /etc/nginx/nginx.conf

I'm not quite sure if it is a good idea to always use the root user. It would be better if we could choose the user on container startup.

Can we read the user from an environment variable?
Or can we configure nginx that it always uses the current user for its workers?

smiller171 commented 6 years ago

@kekru You wouldn't want to use the current user, as the master process needs to be run as root in order to use port 443 so the current user will be root in every functional configuration anyway. Beyond that I think it's a lot more dangerous to grant global access to the docker socket than it is to run the nginx process as root. That said I think I know how to variablize it. Taking a look now.

Not wanting to overwrite the parent nginx config completely is a good point. Sed is probably the way to go.

smiller171 commented 6 years ago

@kekru I looked at using -g "user ${NGINX_USER};" in the CMD entry, but unless I change it to spawn a shell that won't evaluate. I do think that hardcoding the root user here is fine, but I'm open to hearing arguments otherwise.

kekru commented 6 years ago

@smiller171 Yeah, you're right. My chmod 666 /var/run/docker.sock was not my best idea.

A problem when using the root user: We can not use an authorization plugin, like twistlock/authz, anymore, because it will grant or deny access based on the user.
But that isn't such a big use case I think.

I'll merge this PR now, maybe I'll adapt another change later on. Thanks for that :)