nextcloud / docker

⛴ Docker image of Nextcloud
https://hub.docker.com/_/nextcloud/
GNU Affero General Public License v3.0
5.98k stars 1.82k forks source link

SVG imagemagick Nextcloud 21.0 Docker #1414

Closed sparklyballs closed 1 year ago

sparklyballs commented 3 years ago

With the recent update to the vanilla library Nextcloud docker image on x86_64, i.e docker pull nextcloud, bumping the version of nextcloud to 21.0, some warnings appear,of which all but one were trivial to solve.

Whilst I know that the issue of svg compatibility and imagemagick has been raised several times, and at risk of this being marked as duplicate etc and closed , I feel now is the time to actually address it by either adding whatever is required to the image or removing this new warning.

Screenshot from 2021-02-20 11-02-20

And I understand that current advice is to modify a local Dockerfile to add the required packages etc, this isn't always an easy solution, and that (at least for now) there is a specific warning appearing about this, to suggest that people effectively create their own image locally seems a little silly.

modzilla99 commented 2 years ago

I agree with @telmich if this message is shown as a warning, so should the absence of smbclient, which is needed for external storage.

ericzolf commented 2 years ago

If imagemagick covers optional features, those features should be an option and the warning about missing imagemagick should only appear if the option has been set.

JS-Media-Creation commented 2 years ago

If you use docker-compose and just wants to see the green checkmark you can add this line to your docker-compose.yaml

But please keep in mind that installing imagemagick does NOT make your Nextcloud safer. It makes it MORE UNSAFE.

For myself I like to support @TLATER's statement:

"I would propose the warning be inverted, i.e., you get a warning if you have imagemagick installed and Nextcloud is configured to use it for SVG previews."

services:
  nextcloud:
    image: nextcloud
    entrypoint: sh -c "apt update && apt install -y libmagickcore-6.q16-6-extra && /entrypoint.sh apache2-foreground"

all checks passed

TLATER commented 2 years ago

If you use docker-compose you can simply add this line to your docker-compose.yaml

Heh, I was wondering how sensible just leaving the warning is from a security perspective, in case someone gets the idea to just install it without reading context. I didn't expect people making it to this issue tracker to fall for that.

To repeat from very high up in this thread: This isn't in the default image because of potential security vulnerabilities in imagemagick. The warning is about potentially missing functionality without the package, but you're not supposed to actually install it without properly assessing the problems (missing SVG preview vs potential exploits).

The warning should probably be removed, because it's tempting for users to just hack past it, defying the whole purpose of removing the package in the first place.

kerberizer commented 2 years ago

Heh, I was wondering how sensible just leaving the warning is from a security perspective, in case someone gets the idea to just install it without reading context. I didn't expect people making it to this issue tracker to fall for that. (...)

Couldn't have said it better myself. Putting that message about the missing SVG support—on its own—in my opinion was a reasonable decision. But even reasonable and well-meant decisions sometimes turn out to do more harm than good.

For better or worse, many people in this business are perfectionists. Call it OCD if you like, but that's how it is: people get freaked out by anything that isn't green. And treat warnings as something that is best fixed (like errors must be). They will seek ways to replace that anxiety-inducing orange with the relaxing green—even in ways that are less-than-ideal or outright dangerous.

It's actually ironic: you get an orange warning when you don't have the library—which is a security-risk—installed, but once you do install that security risk in your NextCloud instance you get a reassuring green checkmark. I'd say it's even dangerous.

So, may I again propose to keep the message, but “downgrade” it to INFO? Hopefully, this may strike the right balance.

TLATER commented 2 years ago

So, may I again propose to keep the message, but “downgrade” it to INFO? Hopefully, this may strike the right balance.

Actually, given the strong dis-recommendation here and in the docs, and the fact that several people in this thread are now running potentially insecure Nextcloud instances without fully understanding the consequences, I would propose the warning be inverted, i.e., you get a warning if you have imagemagick installed and Nextcloud is configured to use it for SVG previews.

Hopefully eventually enough people get annoyed by the lack of SVG previews/the constant security warning that eventually we get an alternative implementation, and the whole problem disappears.

I also think there is consensus in the community here, and all it takes is a PR (which I don't promise to write myself, because I might not actually get to it and don't want anyone to feel like they can't just jump in and create it right now).

JS-Media-Creation commented 2 years ago

I'm sorry for the misunderstanding of my previous post. You're completely right that this warning makes no sense at all. It should be inverted like @TLATER said. I just wanted to share a solution to get the green checkmark , because I think for many people this is like a challenge which they want to pass. In fact of that it would be even more importantly to rethink this warning.

I've edited my post before so that the context is given.

berlincount commented 2 years ago

This is also broken when using a helm-based installation of nextcloud.

FingerlessGlov3s commented 2 years ago

Also getting this with the nextcloud-apache, hopefully it can be added to the container one day

Edit: Just read up on the security issue with it.

rwese commented 2 years ago

It should be safe to apply, but I do not take responsibility if you wipe any data.

I use fpm-alpine and wanted to get rid of the svg and default_phone_region warnings on new installs.

See Dockerfile.

docker-compose.yml:

services:
  app:
    build:
      context: .
      dockerfile: Dockerfile
      args:
        IMAGE_VERSION: 23
        DEFAULT_PHONE_REGION: 'AT'
...
$ docker-compose build
$ docker-compose up -d

If your container was already running you must copy it the default_phone_region.config.php to /var/www/html/config:

This is required since you usually mount the /var/www/html to a volume and nextcloud has an rsync entrypoint to initialize the volume, or update it on version change.

(nextcloud-docker) $ docker-compose exec --user www-data app sh -c "cp /usr/src/nextcloud/config/default_phone_region.config.php /var/www/html/config"
kerberizer commented 2 years ago

I wonder how many more “hey, guys, here's how to get the warning go away—never mind security!” we will get before someone finally decides that in the current situation this warning is doing more harm than good...

FingerlessGlov3s commented 2 years ago

Yes there is a potential security issue, I assume not fixed upstream yet though or if it can be.

The message in the UI says Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it. yet it does not mention the security issue with it. So people just installing the missing svg support do not know they have opened up the security issue. Although I would hope when they go to search for the missing packages required they see the various messages about the security issue, although they may not search it if they know which packages are required.

I would recommend adding by enabling this support you are adding a potential security risk, if it said that I wouldn't even be here. If the security issue gets fixed then the message can be reverted and package included in the image, but until then I think its a good idea to advise people in the UI itself.

Edit: For any wondering, I have not enabled SVG support, because of the security issue.

TLATER commented 2 years ago

Please read past just the last 5 comments or so. This has been discussed at length.

Imagemagick should not be added to this image, and the warning should be changed or flipped so you get a warning for using imagemagick. Open a PR to nextcloud if you want to move along the discussion.

Re-confirming that you also have this problem just adds to the noise that makes people ignore prior discussion, and pings a lot of people's inboxes. Yes, we all have this problem, and anyone installing this image will. There are a number of hacks in this thread for "fixing" it already, too, likely including yours. None of them should probably be used, unless you really know you want to ignore upstream advice.

Here's the relevant bits of info:

Should this issue be locked to prevent more sharing of footguns?

nijel commented 2 years ago

I think the root cause is that it takes more than a year to change the warning (or remove it). Also, keeping this issue open makes people think the issue is actually in the Docker image, not with the warning...

muebau commented 2 years ago

As a workaround one could add a "patch" to the own Dockerfile to simply remove a few lines after 427 https://github.com/nextcloud/server/blob/c605ef1f432dcd48ba4b3b8cc82551c234dc4f64/core/js/setupchecks.js#L427 to remove the warning for now. 😁

nushkovg commented 2 years ago

Those running the 23.0.3-fpm-alpine image, install the required package to fix the warning by running apk add php7-pecl-imagick. Or build your own Docker image which extends the base fpm-alpine image for reliability. This is for ARM64, I am sure there is the same package (with another name maybe) for AMD64 architecture.

mschoettle commented 2 years ago

Please read this comment: https://github.com/nextcloud/docker/issues/1414#issuecomment-1050769928

TLATER commented 2 years ago

Oh, this is interesting. @pierreozoux' PR to upstream brought up this comment from @tcitworld: https://github.com/nextcloud/server/pull/31742#discussion_r837407416=

Seems to me like the security concerns I had are actually unfounded, given imagemagick is in fact only used to handle built-in (and therefore non-malicious) svgs? The default config.php seems to suggest that, too.

In other words, this warning is about lacking imagemagick for the purposes of rendering some icons, which is much more reasonable than what was represented in this thread.

If that's true, I'd much rather see imagemagick actually included in the image, after all.

mat-m commented 2 years ago

On docker-compose install based on nextcloud:latest (currently 24.0.1):

# apt list | grep mag

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

imagemagick-6-common/now 8:6.9.11.60+dfsg-1.3 all [installed,local]
libmagic-mgc/now 1:5.39-3 amd64 [installed,local]
libmagic1/now 1:5.39-3 amd64 [installed,local]
libmagickcore-6.q16-6/now 8:6.9.11.60+dfsg-1.3 amd64 [installed,local]
libmagickwand-6.q16-6/now 8:6.9.11.60+dfsg-1.3 amd64 [installed,local]

When I tried to install the "extra" lib, I got:

# apt install libmagickcore-6.q16-6-extra
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Package libmagickcore-6.q16-6-extra is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source

E: Package 'libmagickcore-6.q16-6-extra' has no installation candidate

So it looks like it was delibarately blocked...

And it adds some libs too:

  fontconfig gsfonts libcairo2 libdatrie1 libdjvulibre-text libdjvulibre21 libfribidi0
  libgraphite2-3 libharfbuzz0b libilmbase25 libjxr-tools libjxr0 libopenexr25 libpango-1.0-0
  libpangocairo-1.0-0 libpangoft2-1.0-0 libpixman-1-0 libthai-data libthai0 libwmf0.2-7
  libxcb-render0 libxcb-shm0 libxrender1
martadinata666 commented 2 years ago

do you run apt update first?

mat-m commented 2 years ago

do you runapt update first?

Not at first, to understand why it was not there. With the message is not available, but is referred to by another package, either they forced the imagemagick install with a broken dependency, or they removed it afterwards. I can't imagine another scenario where a dependency is not installed.

martadinata666 commented 2 years ago

do you runapt update first?

Not at first, to understand why it was not there. With the message is not available, but is referred to by another package, either they forced the imagemagick install with a broken dependency, or they removed it afterwards. I can't imagine another scenario where a dependency is not installed.

so without apt update there no information about available package in repo. and libmagic-extra doesnt directly depend to libmagic-core, cause libmagick-extra not installed when installing libmagick-core. details here https://packages.debian.org/bullseye/libmagickcore-6.q16-6-extra

mat-m commented 2 years ago

so without apt update there no information about available package in repo. and libmagic-extra doesnt directly depend to libmagic-core, cause libmagick-extra not installed when installing libmagick-core. details here https://packages.debian.org/bullseye/libmagickcore-6.q16-6-extra

Which is strange, since there are two apt-get update in the Dockerfile, lines 7 and 27

martadinata666 commented 2 years ago

so without apt update there no information about available package in repo. and libmagic-extra doesnt directly depend to libmagic-core, cause libmagick-extra not installed when installing libmagick-core. details here https://packages.debian.org/bullseye/libmagickcore-6.q16-6-extra

Which is strange, since there are two apt-get update in the Dockerfile, lines 7 and 27

and the info wiped out at line89 and 152 rm - rf /var/lib/apt/lists/*cmiiw

mat-m commented 2 years ago

Ha, you're right. And since imagemagick is installed through pecl, I don't even know how the lib is actually fetched. So if we want it, we basically need to add it explicitly in the docker file.

ghost commented 2 years ago

is this going to get added? i still see the warning with the newest version of 24.x

jmptbl commented 2 years ago

I found this discussion while looking for a solution that allows the theming app to generate theme icons from SVG images. At first when I saw Nextcloud's warning in the overview page about php-imagick lacking SVG support, I just left it because no one really cares about preview images of SVG files. That feature isn't even enabled by default, so I'm not sure why Nextcloud's overview page needs to warn about it. Maybe that warning should only appear if someone goes through the effort of enabling that preview feature.

The theming app does warn about the missing support as well, and there it's arguably more useful. So why is the official image still missing a complete imagemagick? On the concern of security, surely there's a big difference between the Docker image having the support installed, and Nextcloud being configured to actually use it? It's not enabled by default, so why is the Docker image still lacking imagemagick? This makes no sense.

PrivatePuffin commented 2 years ago

unless you really know you want to ignore upstream advice.

End-users-advocate: Upstream advice is screaming to me that my device should be configured to support this.

CorneliousJD commented 2 years ago

unless you really know you want to ignore upstream advice.

End-users-advocate: Upstream advice is screaming to me that my device should be configured to support this.

This in my opinion is exactly the problem. The warning should be removed...

This is the official Nextcloud container, and it comes with a warning that says the recommend installing it for better suppport, but really they recommend NOT installing it due to a security issue, which is why it's not bundled with the container image.

The real fix here is to remove the warning.

I'm curious why this has not yet been done and over a year later the same warning is still present and nothing has changed? End-users will see the warning, and want to install it so they get that nice green check-mark. The warning even says to install it.

Warning should just be removed. :/

jmptbl commented 2 years ago

This in my opinion is exactly the problem. The warning should be removed...

This is the official Nextcloud container, and it comes with a warning that says the recommend installing it for better suppport, but really they recommend NOT installing it due to a security issue, which is why it's not bundled with the container image.

The real fix here is to remove the warning.

I'm curious why this has not yet been done and over a year later the same warning is still present and nothing has changed? End-users will see the warning, and want to install it so they get that nice green check-mark. The warning even says to install it.

My understanding is that adding SVG support poses no security threat, because SVG previews are disabled by default. And having SVG support allows the theming module to generate rasters from SVG files. Therefore the warning should exist to inform users that without SVG support there is missing functionality. And the Docker image should install the SVG support because it poses no security threat and allows all of Nextcloud's functionality to work, which would also prevent the warning from displaying.

PrivatePuffin commented 2 years ago

This in my opinion is exactly the problem. The warning should be removed... This is the official Nextcloud container, and it comes with a warning that says the recommend installing it for better suppport, but really they recommend NOT installing it due to a security issue, which is why it's not bundled with the container image. The real fix here is to remove the warning. I'm curious why this has not yet been done and over a year later the same warning is still present and nothing has changed? End-users will see the warning, and want to install it so they get that nice green check-mark. The warning even says to install it.

My understanding is that adding SVG support poses no security threat, because SVG previews are disabled by default. And having SVG support allows the theming module to generate rasters from SVG files. Therefore the warning should exist to inform users that without SVG support there is missing functionality. And the Docker image should install the SVG support because it poses no security threat and allows all of Nextcloud's functionality to work, which would also prevent the warning from displaying.

I kinda agree here, the toggle can (and is) made on many levels that do not inherently block functionality or bother users by their defaults.

But either or: Either add it or remove the warning.

Everyone is kinda in agreement that something should be done, so it's kinda weird nothing is being done.

pierreozoux commented 2 years ago

Ok, I'm in favor of adding svg support, then this discussion is over.

Can anybody submit a PR to add svg on each flavor of the container?

Thanks!