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.

Sign up for GitHub

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

Jump to bottom

mastercontainer - limit access to php-fpm to localhost #3317

Merged
merged 1 commit into from
Sep 12, 2023
Merged

mastercontainer - limit access to php-fpm to localhost #3317

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Containers/mastercontainer/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ RUN set -ex; \
sed -i 's/^pm = dynamic/pm = ondemand/' /usr/local/etc/php-fpm.d/www.conf; \
sed -i 's/^pm.max_children =.*/pm.max_children = 80/' /usr/local/etc/php-fpm.d/www.conf; \
sed -i 's|access.log = /proc/self/fd/2|access.log = /proc/self/fd/1|' /usr/local/etc/php-fpm.d/docker.conf; \
grep -q ';listen.allowed_clients' /usr/local/etc/php-fpm.d/www.conf; \
sed -i 's|;listen.allowed_clients.*|listen.allowed_clients = 127.0.0.1,::1|' /usr/local/etc/php-fpm.d/www.conf; \
Comment on lines +59 to +60
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
grep -q ';listen.allowed_clients' /usr/local/etc/php-fpm.d/www.conf; \
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 = /var/run/php.sock|' /usr/local/etc/php-fpm.d/www.conf; \

And change the Apache PHP handler to: SetHandler "proxy:unix:/var/run/php.sock"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all right, lets do it like this then

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/docker-library/php/blob/e9e47fd185d6b36e491c713811437b0c17944faa/8.2/alpine3.18/fpm/Dockerfile#L249

Suggested change
grep -q ';listen.allowed_clients' /usr/local/etc/php-fpm.d/www.conf; \
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 = /var/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; \
sed -i 's|listen =.*|listen = /var/run/php.sock|' /usr/local/etc/php-fpm.d/zz-docker.conf; \

And change the Apache PHP handler to: SetHandler "proxy:unix:/var/run/php.sock"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

healthcheck also needs to check if /var/run/php.sock exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we just actually limit it to 127.0.0.1:::1? would that be fine for you? Sounds like the easier approach...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but perfomance is not critical for the mastercontainer... So I would honestly prefer an easier setup which in this case is limiting to localhost... Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it is simpler to limit ips, instead of using a socket

Copy link
Collaborator Author

@szaimen szaimen Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 lines of code vs 4? also we need to adjust the healthcheck but yeah if you really want we can also use a socket

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the socket

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zoey2936 lets switch to the socket in a follow-up.

I would welcome a PR :)

\
apk add --no-cache git; \
wget https://getcomposer.org/installer -O - | php -- --install-dir=/usr/local/bin --filename=composer; \
Expand Down