-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduced redis session handler does not work with non-root images #763
Comments
Signed-off-by: Stephan Müller <mail@stephanmueller.eu>
Signed-off-by: Stephan Müller <mail@stephanmueller.eu>
Here is a command to reproduce this behavior:
|
Any workarounds? I have this problem too |
Thanks for reporting this. I can reproduce the issue, but I'm not very happy with the proposed permission change in #764. |
I know changing the permissions is kind of a security issue. But running the image as root is even more. What definitely does not work:
Here are some options that I figured out:
Currently I think version 3 is the best trade-off. @J0WI what do you think? |
Why would you prefer 3 over 2? @tilosp what is your opinion? |
@J0WI I prefer 3 over 2 because if redis host is not set, there is a file called
All images in this project should run as non-root by default. Having this in mind, 4 is not an option. I will open a new PR that sets non-root users as default for all used images and examples. Not choosing 4 is a precondition for that. |
1 and 2 are not really an option because it wouldn't work for users other than root and www-data. Instead we would need to chmod 777 them. It there any difference between 1, 2 and 3 in terms of security? In each case php can modify it's own config.
The web server already runs as a non-root user (www-data) by default. |
Not it terms of security, but I'd avoid changing upstream files. |
I can't verify that, have a look at the commands:
The PID 1 process always runs as root by using the defaults.
Have a look at the following blog post. There is a good explanation, why running images as root is not a good practice:
If the
It makes no sense to use another user than How should we go an with this issue? |
Yes the entrypoint scipt and apache/fpm master process run as root by default. I meant that the processes that handle the requests and run the php code are run by a different user.
Yes it will reduce the attack surface in case there is a bug in apache/fpm that allows code execution in the master process. But it won't help in the case of a malicious docker image. To protect against a malicious docker image you would need to override to default user for example in the docker-compose file. Something like this should be done upstream in the php base image, then all php based images get the benefit.
This is valid use case and it is supported by the upstream image. It should work, have you tried it? If there is a permission problem it is a bug. |
I was not aware of that in docker-library/php#787 the permissions of folder I guess the upstream images will not set the user directive in the mid-term because that will break too many existing child images. So in my opinion, this should be treated proactive from the application side. After the discussions above, how should we follow? |
For anyone coming to this today -- you can easily work around this by creating a local Docker CLI:
Or in docker-compose: volumes:
- "./redis-session.ini:/usr/local/etc/php/conf.d/redis-session.ini" Nextcloud can then properly write to the file and Redis caching works. |
…he container Related to nextcloud/docker#763 Fixes #4
will this ever be fixed? |
Mounting a configmap works just fine. I run NC rootless |
@stavros-k would you mind to share your config with us? at least my config seems not to work
i guess an error like this
|
https://github.com/truecharts/charts/tree/master/charts/stable/nextcloud |
thats the truenas chart.. the redis-session is mounted directly as RO file with hardcoded values.. so it looks like the official image are just not usable in a secure way |
TrueCharts*, but no, while the container is custom (also available to see the changed code), nothing is changed in terms of user or permissions, at least not that I can remember. Just some extra utilities scripts are added and few packages. |
this line would fail because the mounted CM is read-only (which is the default in k8s) maybe the config in truenas is different.. |
Truecharts* And no, it's kubernetes, still CM is readonly Don't set redis host. Configure it with occ |
but this is automatically set by the official helm chart. and also using occ in a cloud environment seems not be a good practice.. having a workaround for workaround to work does really not sound like a stable and futureproof solution. i'm curious why this will not be fixed by the nextcloud image itself. @smueller18 has provided some good examples how to solve this 4 years ago. at least for me i have fixed this by fixing the official docker image with a single line
this can easily be added to the official images without any "security concerns" because it is still more secure than running the whole container as root |
chmod 770 /usr/local/etc/php/conf.d/redis-session.ini This makes the PHP config world writeable, including for the webserver itself. Apache and PHP-FPM both drop their privileges if the container is started as root. |
just a quick reminder that the helm chart is also community maintained, as is this docker image.
@stavros-k how do you set it with occ? I'm a collaborator over at the helm chart, and if there's a way to set this occ, I can work with the community and other maintainers there to suggest a solution for perhaps a post-install helm hook to run this occ command. 🙏 |
here is the redis script, you will also find other utils in this dir |
@stavros-k awesome! Thanks! For anyone in this repo trying to figure out the k8s side of things, as I've mentioned in nextcloud/helm#187 (comment) if you'd like, please submit a PR to the helm chart repo to remove the redis configMap and replace it with a job using a post-install hook annotation that runs the scripts that stavros-k has provided. As long as you test your code, we're happy to review it 💙 |
Please also note this script which is required You should also not even need a helm hook. As it will run before NC starts and run in the same container. |
Thanks so much for all the info! :)
Yep, we mount the config directory as a persistence volume if you have persistence turned on in your values.yaml for the nextcloud/helm chart, so the job just needs to mount the same volume. That's how I do it in my personal post install apps job here. That was before I knew about this handy post install hook folder though! Thinking out loud... So now I'm thinking in nextcloud/helm we could just mount a ConfigMap data key like this to use if you have podSecurityContext set to anything other than 0: data:
configure-redis.sh: |
echo '## Configuring Redis...'
occ config:system:set redis host --value="${NC_REDIS_HOST:?"NC_REDIS_HOST is unset"}"
occ config:system:set redis password --value="${NC_REDIS_PASS:?"NC_REDIS_PASS is unset"}"
occ config:system:set redis port --value="${NC_REDIS_PORT:-6379}"
occ config:system:set memcache.local --value="\\OC\\Memcache\\APCu"
occ config:system:set memcache.distributed --value="\\OC\\Memcache\\Redis"
occ config:system:set memcache.locking --value="\\OC\\Memcache\\Redis" Then we just mount that configMap into the |
so the hooks don't seem to work if we use a configMap even though the file is there and executable. I mounted this configMap as a test: ---
apiVersion: v1
kind: ConfigMap
metadata:
name: before-starting-scripts
data:
install-apps.sh: |
#!/bin/sh
echo "Installing Nextcloud apps..."
php /var/www/html/occ app:install oidc_login
php /var/www/html/occ app:install notes
php /var/www/html/occ app:install bookmarks
php /var/www/html/occ app:install deck
php /var/www/html/occ app:install side_menu
echo "Nextcloud apps installation complete." I'm using the helm chart, so I mounted the configmap via the values.yaml like this: nextcloud:
extraVolumes:
- name: before-starting-scripts
configMap:
name: before-starting-scripts
defaultMode: 0550
extraVolumeMounts:
- name: before-starting-scripts
mountPath: /docker-entrypoint-hooks.d/before-starting But the entrypoint.sh doesn't find the file. Here's the container logs: => Searching for scripts (*.sh) to run, located in the folder: /docker-entrypoint-hooks.d/before-starting
[14-Dec-2023 18:39:42] NOTICE: fpm is running, pid 1
[14-Dec-2023 18:39:42] NOTICE: ready to handle connections
127.0.0.1 - 14/Dec/2023:18:42:29 +0000 "GET /index.php" 200
127.0.0.1 - 14/Dec/2023:18:43:07 +0000 "GET /index.php" 200 here's some debug info from logging into the container: $ /docker-entrypoint-hooks.d/before-starting/install-apps.sh
Installing Nextcloud apps...
oidc_login already installed
notes already installed
bookmarks already installed
deck already installed
side_menu already installed
Nextcloud apps installation complete.
$ whoami
www-data
$ export hook_folder_path="/docker-entrypoint-hooks.d/before-starting"
$ find "${hook_folder_path}" -type f -maxdepth 1 -iname '*.sh' -print | sort
$ ls $hook_folder_path
install-apps.sh
$ ls -hal /docker-entrypoint-hooks.d/before-starting
total 12K
drwxrwsrwx 3 root www-data 4.0K Dec 14 18:39 .
drwxr-xr-x 7 root root 4.0K Dec 1 08:30 ..
drwxr-sr-x 2 root www-data 4.0K Dec 14 18:39 ..2023_12_14_18_39_40.2864972143
lrwxrwxrwx 1 root www-data 32 Dec 14 18:39 ..data -> ..2023_12_14_18_39_40.2864972143
lrwxrwxrwx 1 root www-data 22 Dec 14 18:39 install-apps.sh -> ..data/install-apps.sh I guess the find can't follow links? I don't know how to fix that find command, but it renders the docker hooks unusable via the configmaps via k8s right now :( We'll have to use helm hooks with jobs till this is resolved. Did some more reading, but since the configMap file is mounted as a symlink, the find command needs to have a max-depth of 2 to fix this. Example in the container to get this working: $ find "${hook_folder_path}" -type f -maxdepth 1 -iname '*.sh' -print | sort
$ find "${hook_folder_path}" -type f -maxdepth 2 -iname '*.sh' -print | sort
/docker-entrypoint-hooks.d/before-starting/..2023_12_14_18_39_40.2864972143/install-apps.sh |
Probably this: |
Just wanted to update my Nextcloud docker setup (with redis and mariaDB) and can't start it anymore now due to this issue. Since my last update before that was 25.0, I also cannot go back to the old version. |
I work around this by mounting
|
Having the exact same problem. Thank you for the fix. Nextcloud should up their game on how to correctly use docker and providing images. The whole upgrade process with nextcloud is also horrendous. If there would be a viable alternative I would gladly take it. |
This comment was marked as duplicate.
This comment was marked as duplicate.
OT: You may want to check out https://github.com/LorbusChris/nextcloud-quadlet for an alternative way of running a rootless setup. The stack is a bit different, using Valkey instead of Redis, and Envoy as proxy. I've been running this setup for ~4 months with automatic upgrades and without any issues. |
Unlikely, due the reason mentioned in #763 (comment) |
In commit 83ea69d, a redis session handler was intrododuced. But the file
/usr/local/etc/php/conf.d/redis-session.ini
can not be written when using a non-root image, because the folder is owned by root and the defaultwww-data
user does not have write permission.For running my nextcloud image, I use the default non-privileged user
www-data
.The text was updated successfully, but these errors were encountered: