-
Notifications
You must be signed in to change notification settings - Fork 79
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
draft: feat: Run as non-root by default to allow easier deployment on the restricted environments like OpenShift, Kubernetes and any corporate (or just secure) environments where root privileges are not allowed #308
base: master
Are you sure you want to change the base?
Conversation
…stricted environments like OpenShift, Kubernetes and any corporate (or just secure) environments where root privileges are not allowed
This may probably resolve issues like #194, because no any process will be running with other privileges than the application, so there will be no chance of accidentially creating a file as root. |
@@ -59,7 +59,7 @@ ARG RUNTIME_DEPS="\ | |||
php7-xmlwriter \ | |||
php7-zip \ | |||
sqlite \ | |||
supervisor \ | |||
multirun \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if HumHub has been fully tested with multirun, you should probably run some tests before switching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case crond
is used instead of supervisor
.
After some more testing I see volume mount points are having non-deterministic permissions, sometimes I get Here is my testing script: HUMHUB_VERSION=1.13.0
build-prod:
docker build . --target humhub_nginx --build-arg HUMHUB_VERSION=${HUMHUB_VERSION} -t docker.io/mriedmann/humhub:stable-nginx
docker build . --target humhub_phponly --build-arg HUMHUB_VERSION=${HUMHUB_VERSION} -t docker.io/mriedmann/humhub:stable-phponly
rm-prod:
docker-compose -f docker-compose.prod.yml rm -s -v -f
docker volume rm -f humhub-docker_assets humhub-docker_modules humhub-docker_mysql humhub-docker_themes humhub-docker_uploads
run-prod: build-prod rm-prod
docker-compose -f docker-compose.prod.yml up
build-aio:
docker build . --target humhub_allinone --build-arg HUMHUB_VERSION=${HUMHUB_VERSION} -t docker.io/mriedmann/humhub:latest
|
Is it running on a reverse proxy? |
Yes, I'm testing the variant with separate nginx and php-fpm if you are asking abut this. |
Are there any noticeable differences in the logs between From my understanding of HumHub itself, it can be picky with reverse proxies. 🤔 |
Yeah, with |
On all-in-one variant it looks pretty fine I see, at least in docker-compose, I will have to check on Kubernetes on CRI-O.
Startup logs
|
Hi! Thank you both so much for your work. I will try to have a look asap and comment/approve it. |
I marked is as draft so it is not an asap, take your time. I will play with it little bit more and will let you know as I don't want to break somebody's instance with this change 😃 |
I started making a Helm Chart (https://github.com/riotkit-org/k8s-humhub) at first I will support root image, then I will experiment with non-root after I will get the root image working enough stable. |
@@ -159,7 +174,13 @@ RUN chmod +x /usr/local/bin/php-fpm-healthcheck \ | |||
&& adduser --uid 100 -g 101 -S nginx | |||
|
|||
EXPOSE 9000 | |||
USER nginx | |||
CMD ["multirun", "/usr/sbin/php-fpm7 --fpm-config /etc/php-fpm.d/pool.conf -O -F", "tail -f /var/www/localhost/htdocs/protected/runtime/logs/app.log", "crond -f -L /proc/self/fd/2"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tailing on this file is not the best idea ... it still writes to the top-layer potentially filling up the nodes local storage. We should use a symlink to /proc/self/fd/2 for that (if possible).
mkdir -p /var/www/localhost/htdocs/uploads /var/www/localhost/htdocs/assets /var/www/localhost/htdocs/protected/modules /var/www/localhost/htdocs/themes /var/www/localhost/htdocs/protected/config && \ | ||
mkdir -p /run/nginx /run/php-fpm && \ | ||
touch /var/www/localhost/htdocs/protected/runtime/logs/app.log && \ | ||
chown 100:101 -R /var/www/localhost/htdocs/protected/runtime/logs /run/nginx /run/php-fpm \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for an openshift setup using the group 0 might be better than 101 because openshift gives the user a "random" high-id and the group 0. If we just allow the group 0 to read/write most parts of the files this should work in most scenarios.
I would really like to see this change and the helm chart come to (more) life, but I have no time to finish it. Is anyone interested in helping with this? Is there even a need for it? Please Emoji-Vote if you would be interested in using this, thx. |
I'm going to be honest, I have no idea what you are doing with the code, but I use the docker container in truecharts with truenas scale, and I love it. If sorry you have no time, but if I could upvote it a million times I would. I'm having an issue with my deployment because of supervisord troubles and if this would fix those, it would light up my world. |
I had another look and I will try to get this going. I am still no big fan of multirun but reducing the size and complexity and getting a non-root container is a big improvement. I hope I can get to it over the weekend. |
Wow, sorry for abadoning the topic. I "slept" for almost a year. My instance still works on a feature branch, damn it 😁 |
I must at least finish the Helm Chart :) |
Regarding the non-root container - I feel there could be some pitfalls on container startup, like chmod/chown or cache wipe, let's check it more strictly. |
I've also considered this as well for my own Dockerfile, currently this is how I've implemented it; |
Hi,
The container does not necessarily start as root and then switches permissions to nginx user.
I simplified it by starting everything as nginx from the beginning.
Additionally I reduced the end image size and memory usage (~20 MB less) by migrating from
supervisord
tomultirun
which is a container-native init process, that does not depend on Python.Benefits:
~20 MB
)398MB
->348MB
for all-in-one variant)I was testing it initially with
docker-compose
, I will test also on Kubernetes and will then "undraft" the PR. I'm creating the PR at this moment to know what do you think about such a big change.