Skip to content
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

WIP: introduce unprivileged images #772

Closed
wants to merge 2 commits into from

Conversation

smueller18
Copy link

Following Docker best practices, all images should run as non-root user. This PR introduces

  1. an unprivileged variant for each of the existing nextcloud images so that the existing images will not have breaking changes
  2. replacement of nginx image with nginxinc/nginx-unprivileged
  3. replacement of nextcloud images in the example docker-compose files with unprivileged variants

Todos:

  • Use non-root images for postgres, mariadb and redis in example docker-compose files
  • Test each example docker-compose
  • Maybe update readme with unprivileged images
  • Implement new image tags (e.g. nextcloud:unprivileged)

Closes #760

@EWouters
Copy link

Nice work!

@pierreozoux
Copy link
Member

This is indeed nice.

Could you separate your PR on 2 commits, one with the changes and one with the update.sh?

This makes it easier to review.

Thanks :)

Signed-off-by: Stephan Müller <mail@stephanmueller.eu>
Signed-off-by: Stephan Müller <mail@stephanmueller.eu>
@smueller18
Copy link
Author

@pierreozoux I did a rebase on the latest master commit and seperated the changes into two seperat commits.

In the meantime, I changed my mind and think it is not worth the effort to support both, privileged and unprivileged container images. The default one should always be unprivileged. With that in mind, I stopped working on this PR because it is not target-oriented for me.

@pierreozoux
Copy link
Member

You are right, everything should be unpriviledged and readonly, imho too :)

@tilosp
Copy link
Member

tilosp commented Sep 4, 2020

wouldn't it be enough to add a environment var, that changes the port? Running as an unprivileged user is already possible with --user.

@smueller18
Copy link
Author

Using the --user option is possible for the fpm image but not for the apache image because port 80 is binded by default. Replacing port 80 with 8080 during runtime in the entrypoint based on an environment variable is not possible because the responsible file /etc/apache2/ports.conf is owned by root. So either it has to be changed during build time (as introduced in this PR) or ports.conf must be mounted during runtime which is not user friendly.

@jan-di
Copy link

jan-di commented Oct 17, 2020

wouldn't it be enough to add a environment var, that changes the port? Running as an unprivileged user is already possible with --user.

Additionally to what @smueller18 said, there are a few more cases where nextcloud don't work right when you set the --user parameter. For example (at least, with the apache image):

  • Redis cannot be used:
    /entrypoint.sh: 56: /entrypoint.sh: cannot create /usr/local/etc/php/conf.d/redis-session.ini: Permission denied
  • Apache IP-Rewrite cannot be disabled (as recommended when running behind a reverse proxy):
    Could not remove /etc/apache2/conf-enabled/remoteip.conf: Permission denied

I think the only clean solution to this would be to make the entrypoint script aware for UID/GID env variables. The entrypoint could make the changes above as root and then start the services under the unprivileged UID/GID. I think this is described in #359

J0WI added a commit to J0WI/docker-nextcloud that referenced this pull request Sep 1, 2022
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
J0WI added a commit to J0WI/docker-nextcloud that referenced this pull request Sep 1, 2022
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
J0WI added a commit that referenced this pull request Sep 6, 2022
fix: #359, #772, #1081, #1087, #1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
@J0WI
Copy link
Contributor

J0WI commented Sep 6, 2022

closing due #1812

@J0WI J0WI closed this Sep 6, 2022
pathob pushed a commit to pathob/nextcloud-docker that referenced this pull request Jan 14, 2024
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
ananace pushed a commit to ananace/docker-nextcloud that referenced this pull request May 10, 2024
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running as non-root user (Apache Privileged Ports)
6 participants