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

refs #000: install ftp extension. #79

Merged
merged 5 commits into from
Jan 16, 2025
Merged

refs #000: install ftp extension. #79

merged 5 commits into from
Jan 16, 2025

Conversation

noemi-mancini
Copy link
Member

@noemi-mancini noemi-mancini commented Jan 15, 2025

PR Type

Enhancement


Description

  • Added support for PHP 8.4.2 and 8.3.15 versions with complete Docker configuration
  • Installed FTP extension with OpenSSL support in PHP images
  • Added new configuration files and templates for PHP 8.4:
    • PHP core settings (memory, timezone, upload limits)
    • PHP-FPM configurations
    • Optional extensions (APCu, Redis, Memcached, Xdebug)
    • Mailhog integration
  • Updated CI/CD workflow to include new PHP versions
  • Added corresponding Makefile targets for building new PHP versions
  • Fixed typo in guess_folder.sh script comment

Changes walkthrough 📝

Relevant files
Configuration changes
6 files
docker-entrypoint.sh
Add new entrypoint script for PHP 8.4 configuration           
+67/-0   
docker-publish.yml
Add PHP 8.4.2 and 8.3.15 to build matrix                                 
+3/-1     
Makefile
Add build targets for PHP 8.3.15 and 8.4.2                             
+12/-0   
Documentation
1 files
guess_folder.sh
Fix typo in comment                                                                           
+1/-1     
Enhancement
2 files
Dockerfile
Add FTP extension support with OpenSSL                                     
+4/-2     
Dockerfile
Add new PHP 8.4 Dockerfile with extensions                             
+128/-0 
Additional files
10 files
apcu.ini +1/-0     
mailhog.ini +1/-0     
memcached.ini +2/-0     
redis.ini +1/-0     
xdebug.ini +4/-0     
docker.ini +9/-0     
expose.ini +1/-0     
opcache.ini +9/-0     
zz2-docker-custom.conf +8/-0     
zz3-structured-logs.conf +2/-0     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

@Bladedu Bladedu marked this pull request as ready for review January 16, 2025 10:58
@Bladedu Bladedu merged commit 6b8b64a into master Jan 16, 2025
14 checks passed
@sparkfabrik-ai-bot
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The PHP configuration in 8.4/conf/docker.ini exposes PHP configuration details through environment variables. Consider implementing stricter validation for these environment variables to prevent potential information disclosure. Additionally, the default PHP-FPM configuration in zz2-docker-custom.conf exposes detailed access logs that might contain sensitive information.

⚡ Recommended focus areas for review

Error Handling

The script lacks error handling for critical operations like file copying and environment variable substitution. Failed operations could lead to incorrect PHP configuration.

if [ "${MEMCACHED_ENABLE}" = "1" ]; then
	cp /usr/local/etc/php/conf.disabled/memcached.ini /usr/local/etc/php/conf.d/memcached.ini
else
	rm -f /usr/local/etc/php/conf.d/memcached.ini || true
fi

if [ "${REDIS_ENABLE}" = "1" ]; then
	cp /usr/local/etc/php/conf.disabled/redis.ini /usr/local/etc/php/conf.d/redis.ini
else
	rm -f /usr/local/etc/php/conf.d/redis.ini || true
fi

if [ "${MAILHOG_ENABLE}" = "1" ]; then
	cp /usr/local/etc/php/conf.disabled/mailhog.ini /usr/local/etc/php/conf.d/mailhog.ini
else
	rm -f /usr/local/etc/php/conf.d/mailhog.ini || true
fi

if [ "${XDEBUG_ENABLE}" = "1" ]; then
	cp /usr/local/etc/php/conf.disabled/xdebug.ini /usr/local/etc/php/conf.d/xdebug.ini
else
	rm -f /usr/local/etc/php/conf.d/xdebug.ini || true
fi

if [ "${LDAP_ENABLE}" = "0" ]; then
	rm -f /usr/local/etc/php/conf.d/docker-php-ext-ldap.ini || true
fi

if [ "${APCU_ENABLE}" = "1" ]; then
	cp /usr/local/etc/php/conf.disabled/apcu.ini /usr/local/etc/php/conf.d/apcu.ini
else
	rm -f /usr/local/etc/php/conf.d/apcu.ini || true
fi
Security Configuration

The PHP configuration allows dynamic installation of extensions and uses root user by default. Consider implementing more restrictive default configurations.

RUN export XDEBUG_DEPS="linux-headers" && \
    export PHP_EXTRA_DEPS="libxml2-dev icu-dev libmemcached-dev cyrus-sasl-dev libpng libjpeg-turbo freetype-dev libpng-dev libjpeg-turbo-dev libzip-dev oniguruma-dev libwebp-dev ldb-dev libldap openldap-dev openssl-dev" && \
    apk update && \
    apk add ${PHP_EXTRA_DEPS} ${PHPIZE_DEPS} ${XDEBUG_DEPS} gettext && \
    docker-php-ext-configure gd --with-freetype=/usr/include/ --with-jpeg=/usr/include/ --with-webp=/usr/include && \
    docker-php-ext-configure ldap && \
    docker-php-ext-configure ftp && \
    docker-php-ext-install bcmath gd intl mbstring pcntl pdo pdo_mysql soap zip ldap ftp && \
    pecl install xdebug-${XDEBUG_VERSION} && \
    pecl install redis-${PHPREDIS_VERSION} && \
    pecl install igbinary memcached-${MEMCACHE_VERSION} --disable-memcached-sasl && \
    apk del ${PHPIZE_DEPS} ${XDEBUG_DEPS} && \
    rm -rf /var/cache/apk/*
Version Pinning

Several dependencies and packages are installed without specific version pinning, which could lead to inconsistent builds and potential compatibility issues.

apk update && \
apk add ${PHP_EXTRA_DEPS} ${PHPIZE_DEPS} ${XDEBUG_DEPS} gettext && \
docker-php-ext-configure gd --with-freetype=/usr/include/ --with-jpeg=/usr/include/ --with-webp=/usr/include && \

@sparkfabrik-ai-bot
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Security
Add missing OpenSSL configuration for secure FTP extension setup

The FTP extension configuration is missing OpenSSL parameters which could lead to
build failures or insecure FTP connections.

8.4/Dockerfile [47-48]

-docker-php-ext-configure ftp && \
+export OPENSSL_LIBS="-L/usr -lssl -lcrypto -lz" && export OPENSSL_CFLAGS="-I/usr/include" && \
+docker-php-ext-configure ftp --with-openssl-dir=/usr/include/ && \
 docker-php-ext-install bcmath gd intl mbstring pcntl pdo pdo_mysql soap zip ldap ftp
Suggestion importance[1-10]: 8

Why: The suggestion addresses a security concern by adding necessary OpenSSL configuration for FTP, which is important for ensuring secure FTP connections. Missing SSL configuration could lead to security vulnerabilities.

8
Possible issue
Reorder PHP extension configuration to ensure proper dependency resolution during build

The FTP configuration with OpenSSL should be moved before installing PHP extensions
to ensure proper dependency resolution and avoid potential build failures.

8.3/Dockerfile [47-49]

 export OPENSSL_LIBS="-L/usr -lssl -lcrypto -lz" && export OPENSSL_CFLAGS="-I/usr/include" && \
 docker-php-ext-configure ftp --with-openssl-dir=/usr/include/ && \
+docker-php-ext-configure gd --with-freetype=/usr/include/ --with-jpeg=/usr/include/ --with-webp=/usr/include && \
+docker-php-ext-configure ldap && \
 docker-php-ext-install bcmath gd intl mbstring pcntl pdo pdo_mysql soap zip ldap ftp
Suggestion importance[1-10]: 5

Why: The suggestion proposes a logical reordering of extension configurations to ensure dependencies are properly set up, which could prevent potential build issues. While valid, the impact is moderate as the current setup still works.

5

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.

2 participants