nextcloud / all-in-one

📦 The official Nextcloud installation method. Provides easy deployment and maintenance with most features included in this one Nextcloud instance.
https://hub.docker.com/r/nextcloud/all-in-one
GNU Affero General Public License v3.0
5.64k stars 654 forks source link

Allow collabora to escalate privillege. #5620

Closed denppa closed 2 days ago

denppa commented 2 days ago

Without allowing this, collabora cannot fork the process and complains with the following in logs:

wsd-00001-00001 2024-11-20 00:31:26.654865 -0500 [ coolwsd ] INF  Waiting for a new child for a max of 20000ms| wsd/COOLWSD.cpp:4433

This is extensively discussed on Collabora in this issue.

However, it is possible that Nextcloud's build of collabora did not incoporate the change yet, in this file.

The issue mentioned that it is not ideal to run in privileged, but Collabora could not run otherwise in my Debian K8s cluster.

Can the NC engineers verify that the nextcloud-aio/collabora:latest build did incorporate that change? Because if not, then we will need to enable this escalation.

denppa commented 1 day ago

Hi, I fear this is not the setting you are looking for.

You are looking for this: https://stackoverflow.com/a/72731482

But we are not going to add this because the container needs to work without it and also does in our testing.

Perhaps, but Collabora cannot fork in the image built by nextcloud. It would be great if you can verify the build process of the image and whether it included the commit I mentioned.

szaimen commented 1 day ago

We are building on the "official" collabora image: https://github.com/nextcloud/all-in-one/blob/main/Containers/collabora/Dockerfile

denppa commented 1 day ago

We are building on the "official" collabora image: https://github.com/nextcloud/all-in-one/blob/main/Containers/collabora/Dockerfile

This is great, however, I would appreciate if this can be investigated a little. It may work on some clusters with their settings, but on mine Collabora did not. I opened an issue for this.

I followed the stackexchange and that works too, along with the change I made in my PR.

denppa commented 1 day ago

@szaimen

Perhaps this team is not too familiar with the Kubernetes architecture, when the capability "CAP_SYS_ADMIN" has been added to security context, Kubernetes will set allowPrivilegeEscalation to true:

allowPrivilegeEscalation: Controls whether a process can gain more privileges than its parent process. This bool directly >controls whether the no_new_privs flag gets set on >the container process. allowPrivilegeEscalation is always true when the container:

is run as privileged, or has CAP_SYS_ADMIN

Source: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/

So it does not make sense for the helm chart to set allowPrivilegeEscalation: false, which blocks the proper functionality of CAP_SYS_ADMIN. @szaimen Can you reopen this PR to be reconsidered?

Correction: Apparently, SYS_ADMIN doesn't mean anything in kubernetes, so CAP_SYS_ADMIN is needed, and that naturally blocks allowPrivilegeEscalation. I will add this info in #5601.

szaimen commented 1 day ago

Thanks for the pointer @denppa!

I'll leave this closed for now because this gets refactored with https://github.com/nextcloud/all-in-one/pull/5601 soon