Conversation
szaimen
left a comment
There was a problem hiding this comment.
Hey, thanks for working on this! I have an initial comment below. Will look more into detail into this next week.
59e4a6c to
0664ec8
Compare
| # Don't allow access to the AIO interface from the Nextcloud container | ||
| # Probably more cosmetic than anything but at least an attempt | ||
| if ! grep -q '# nextcloud-aio-block' /etc/apache2/httpd.conf; then | ||
| cat << APACHE_CONF >> /etc/apache2/httpd.conf | ||
| # nextcloud-aio-block-start | ||
| <Location /> | ||
| order allow,deny | ||
| deny from nextcloud-aio-nextcloud.nextcloud-aio | ||
| allow from all | ||
| </Location> | ||
| # nextcloud-aio-block-end | ||
| APACHE_CONF | ||
| fi |
There was a problem hiding this comment.
can we do something comparable with caddy?
There was a problem hiding this comment.
we could resolve the ip for example with dig and block the ip(s) we get
There was a problem hiding this comment.
we could resolve the ip for example with
digand block the ip(s) we get
I fear this is not going to work as the ip-addresses of the nextcloud-aio-nextcloud container might change or might not exist when the mastercontainer starts. Is there any other way to do this?
There was a problem hiding this comment.
Looks like there is a caddy plugin that could do this: https://github.com/muety/caddy-remote-host
There was a problem hiding this comment.
Is it worth recompiling caddy for this? also the last commit is 4 years ago, so is it still compatible with latest caddy?
There was a problem hiding this comment.
We can make Caddy only allow requests from the "mastercontainer's" IP-gateway. Which will always be the container host, as far as I understand.
That IP-Adress is trivial to get in the container's entrypoint (ip r | awk '/^default via/ { print $3 }'), and should be easy enough to set into the environment so that Caddy can use it:
https://:8443 {
@denied not remote_ip {$CONTAINER_HOST_IP}
abort @denied
…
That would disable access to the web UI from all other containers in the network, which would goes even beyond the current solution.
| sed -i 's|;listen.allowed_clients.*|listen.allowed_clients = 127.0.0.1,::1|' /usr/local/etc/php-fpm.d/www.conf; \ | ||
| grep -q 'listen =' /usr/local/etc/php-fpm.d/www.conf; \ | ||
| sed -i 's|listen =.*|;listen = /run/php.sock # handled in zz-docker.conf|' /usr/local/etc/php-fpm.d/www.conf; \ | ||
| grep -q 'listen =' /usr/local/etc/php-fpm.d/zz-docker.conf; \ |
There was a problem hiding this comment.
| grep -q 'listen =' /usr/local/etc/php-fpm.d/zz-docker.conf; \ | |
| grep -q '^listen =' /usr/local/etc/php-fpm.d/zz-docker.conf; \ |
There was a problem hiding this comment.
only for the zz-docker.conf?
There was a problem hiding this comment.
I'd say so, yes. Or we comment listen in zz-docker and add listen to the www-conf which might be better
There was a problem hiding this comment.
currently it is set twice:
/var/www/html # grep -r /usr/local/etc/ -e "listen\s="
/usr/local/etc/php-fpm.d/www.conf.default:listen = 127.0.0.1:9000
/usr/local/etc/php-fpm.d/www.conf.default:;pm.status_listen = 127.0.0.1:9001
/usr/local/etc/php-fpm.d/www.conf:listen = 127.0.0.1:9000
/usr/local/etc/php-fpm.d/www.conf:;pm.status_listen = 127.0.0.1:9001
/usr/local/etc/php-fpm.d/zz-docker.conf:listen = 9000
There was a problem hiding this comment.
in zz-docker.conf and www.conf, files ending with .default are ignored by fpm
There was a problem hiding this comment.
hm... lets maybe only set it in www.conf and comment it in zz-docker.conf
There was a problem hiding this comment.
can you adjust this please/create a suggestion?
0664ec8 to
bf02678
Compare
|
@Zoey2936 please also update Thanks in advance! :) |
should be done |
|
Is the only advantage that the mastercontainer is a bit smaller? 🤔 |
|
Hey @Zoey2936, my plan is to release AIO v12 beta tomorrow. Unfortunately I don't have enough time for this PR until then. So will need to postpone this. Sorry for that! |
what is the current state of the review? |
Signed-off-by: Zoey <zoey@z0ey.de>
Signed-off-by: Zoey <zoey@z0ey.de>
Signed-off-by: Zoey <zoey@z0ey.de>
Signed-off-by: Zoey <zoey@z0ey.de>
396acf3 to
7c093f3
Compare
| # nextcloud-aio-block-start | ||
| <Location /> | ||
| order allow,deny | ||
| deny from nextcloud-aio-nextcloud.nextcloud-aio |
There was a problem hiding this comment.
Check with Pablo if we can simply block the whole network range
There was a problem hiding this comment.
As suggested in the thread above we can make Caddy only serve its IP-gateway pretty easily, I think.
|
@Zoey2936 Do I understand correctly that using two Caddy instances with separate config files is due to Caddy's inability to have multiple blocks with catch-all host matchers as discussed in caddyserver/caddy#7311? In my spontaneous test it is possible to have one catch-all matcher (using on-demand internal TLS) side-by-side with blocks that include hostnames (using Let's Encrypt TLS). So if we configure Caddy initially only with the block for the AIO web UI, and only after we obtained the domain name from the web UI dynamically (e.g. via its HTTP API) inject the config blocks for ports 80 and 8443 – then we could get away with only one caddy instance, I think. Or do I miss something important? |
|
Yes the reason is caddyserver/caddy#7311 |
|
My previously question was kinda ignored, so I would like to ask again what the motivation is for this change. |
|
@Croydon Removing Apache would reduce the image size as well as the overall complexity, which we consider progress. If you see any arguments against this, please speak up. |
Signed-off-by: Zoey zoey@z0ey.de
see #6998