-
Notifications
You must be signed in to change notification settings - Fork 0
feat(slb-214): self hosted image processing [thumbor] #491
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
base: release
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @elistone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request for a quick overview. This PR introduces self-hosted image processing capabilities using Thumbor, addressing the need for more control over image hosting, reducing reliance on third-party services, and potentially lowering costs. The core changes involve integrating Thumbor and a caching proxy into the local development environment via docker-compose.yml
and ensuring related files are ignored in .gitignore
. This setup allows for on-the-fly image generation and processing from local URLs.
Highlights
- Self-Hosted Image Processing: Introduces Thumbor for self-hosted image processing, enabling dynamic image manipulation and serving.
- Docker Compose Integration: Adds new
thumbor
andthumbor-proxy
services todocker-compose.yml
to run the image processing stack locally. - Caching Layer: Includes a Nginx-based proxy with caching (
thumbor-proxy
) to improve performance for processed images. - Local Development Setup: Configures volumes and environment variables for the Docker services to integrate with local file paths for images, storage, and cache.
Changelog
- .gitignore
- Added
/thumbor
to the ignore list (line 16) to prevent committing local Thumbor data, images, and storage directories.
- Added
- docker-compose.yml
- Removed the explicit
version: '2.3'
declaration (lines 1-2). - Added a new service
thumbor
(starting line 122) using theghcr.io/minimalcompact/thumbor:latest
image, configured with environment variables for processes, CORS, WebP, storage paths (/data/images
,/data/storage
), and a security key. - Mounted local directories
./thumbor/thumbor_data
,./thumbor/thumbor_images
(read-only), and./thumbor/thumbor_storage
as volumes for thethumbor
service (lines 139-141). - Added a new service
thumbor-proxy
(starting line 147) using theghcr.io/minimalcompact/thumbor-nginx-proxy-cache
image, configured as a proxy pointing to thethumbor
service with caching settings. - Mounted
/var/run/docker.sock
and./thumbor/cache
as volumes for thethumbor-proxy
service (lines 161-162). - Exposed ports
8088
and9443
for thethumbor-proxy
service (lines 164-165). - Added a local volume definition for
thumbor
(starting line 170).
- Removed the explicit
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Images need a tweak,
Thumbor makes them sleek.
Docker containers hum,
Processed pics will come.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces self-hosted image processing using Thumbor, which is a valuable addition for gaining more control over image handling and potentially reducing costs. The setup includes Thumbor itself and an Nginx proxy for caching, which is a good approach.
I've identified a few areas for improvement, primarily concerning security and configuration best practices. Addressing these will help ensure the setup is robust and secure, even for local development environments that might serve as templates.
Summary of Findings
- Security: Insecure Default
THUMBOR_SECURITY_KEY
: Thethumbor
service uses a defaultTHUMBOR_SECURITY_KEY
ofchangeme
, which is a critical security vulnerability. This key must be overridden with a strong, unique value. - Security: Docker Socket Mount: The
thumbor-proxy
service mounts the Docker socket. This grants potentially excessive permissions and should be reviewed for necessity. - Reproducibility: Use of
latest
Tag for Docker Images: Boththumbor
andthumbor-proxy
services use thelatest
tag (or an implicit latest) for their images. Pinning to specific versions is recommended for reproducible and stable builds. - Security: Permissive CORS Configuration:
CORS_ALLOW_ORIGIN
is set to*
for thethumbor
service, which is overly permissive. It should be restricted if possible, even in local development. - Security: Implicit
ALLOW_UNSAFE_URLS
: Thumbor defaults toALLOW_UNSAFE_URLS = True
. For better security, explicitly setting this toFalse
and requiring signed URLs is advisable. - Configuration: Unused Named Volume: A named volume
thumbor
is defined but not used by any service. It should be removed to simplify the configuration. - Configuration:
.gitignore
update (Low Severity - Not Commented): Thethumbor
directory has been added to.gitignore
. This is good practice for ignoring local data/cache files. - Configuration: Docker Compose Version Key (Low Severity - Not Commented): The
version: '2.3'
key was removed fromdocker-compose.yml
. This is aligned with modern Docker Compose practices where the version key is often optional.
Merge Readiness
This pull request makes good progress in setting up self-hosted image processing. However, due to the identified critical
security issue with THUMBOR_SECURITY_KEY
and the high
severity concern regarding the Docker socket mount, I recommend that these changes not be merged until these points are addressed. The medium severity issues should also be considered for improvement to enhance the robustness and security of the setup.
As a reviewer, I am not authorized to approve pull requests. Please ensure these concerns are discussed and resolved, and that the code receives further review and approval before merging.
docker-compose.yml
Outdated
STORAGE: thumbor.storages.file_storage | ||
THUMBOR_FILE_LOADER_ROOT_PATH: /data/images | ||
THUMBOR_FILE_STORAGE_ROOT_PATH: /data/storage | ||
THUMBOR_SECURITY_KEY: ${THUMBOR_SECURITY_KEY:-changeme} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default THUMBOR_SECURITY_KEY
is set to changeme
. This is a significant security risk, even in a local development environment. If this key is known or easily guessable, it could allow unauthorized users to generate signed URLs, potentially leading to abuse of the image processing service (e.g., denial of service by requesting numerous image operations).
Could we ensure this key is always set to a strong, unique value via an environment variable and avoid a weak default in the docker-compose.yml
? For instance, you could remove the default fallback entirely, forcing it to be set in a .env
file.
THUMBOR_SECURITY_KEY: ${THUMBOR_SECURITY_KEY:?Please set a strong THUMBOR_SECURITY_KEY in your .env file}
docker-compose.yml
Outdated
PROXY_CACHE_MEMORY_SIZE: 500m | ||
PROXY_CACHE_INACTIVE: 300m | ||
volumes: | ||
- /var/run/docker.sock:/tmp/docker.sock:ro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thumbor-proxy
service mounts the Docker socket (/var/run/docker.sock
) in read-only mode. Access to the Docker socket, even read-only, can provide a container with a lot of information about the host and other containers, and potentially be a step in an exploit chain if the container itself is compromised.
Is this Docker socket mount strictly necessary for the thumbor-nginx-proxy-cache
image to function in this context? If it's for dynamic discovery or interaction with other services managed by Amazee/Lagoon, it might be standard, but it's worth confirming its purpose and if less privileged alternatives exist.
docker-compose.yml
Outdated
environment: | ||
VIRTUAL_HOST: thumbor-${COMPOSE_PROJECT_NAME:-slbtemplate}.docker.amazee.io | ||
THUMBOR_NUM_PROCESSES: 4 | ||
CORS_ALLOW_ORIGIN: '*' | ||
AUTO_WEBP: 'True' | ||
RESULT_STORAGE: thumbor.result_storages.no_storage | ||
RESULT_STORAGE_STORES_UNSAFE: 'True' | ||
STORAGE: thumbor.storages.file_storage | ||
THUMBOR_FILE_LOADER_ROOT_PATH: /data/images | ||
THUMBOR_FILE_STORAGE_ROOT_PATH: /data/storage | ||
THUMBOR_SECURITY_KEY: ${THUMBOR_SECURITY_KEY:-changeme} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thumbor's default behavior is ALLOW_UNSAFE_URLS = True
, meaning it will process images even if the URL isn't signed with the THUMBOR_SECURITY_KEY
. While the security key is still important, allowing unsafe URLs can make it easier for unintended image processing requests if the service is accessible.
For enhanced security and control, especially if this setup might influence other environments, have you considered explicitly setting ALLOW_UNSAFE_URLS: 'False'
in the environment variables for the thumbor
service? This would require all image requests to be properly signed.
docker-compose.yml
Outdated
thumbor: | ||
driver: local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A named volume thumbor
is defined in the top-level volumes
section, but it doesn't appear to be used by any of the services. The thumbor
and thumbor-proxy
services use bind mounts for their data persistence (e.g., ./thumbor/thumbor_data:/data
, ./thumbor/cache:/var/cache/nginx
).
Could this unused volume definition be removed to avoid confusion and keep the configuration clean?
Description of changes
Setting up a way to use self host image processing in this case using thumbor, so we can pass urls locally and have images generated on the fly.
Motivation and context
To have more control over hosting files, not have to upload files and not have to pay for third parties.
How has this been tested?