build: switch docker containers to non-root #1588
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several changes across multiple Dockerfiles and scripts. It creates a non-root user and group named Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/Dockerfile (1)
42-43: Establishing a Non-Root User for Enhanced Security
Creating a dedicated system group and user (app) usinggroupadd -randuseradd -r -g app appis an excellent security measure. Optionally, if your application might require a home directory for the user, consider using the-mflag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/Dockerfile(2 hunks)backend/db/readme.txt(0 hunks)
💤 Files with no reviewable changes (1)
- backend/db/readme.txt
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
backend/Dockerfile (7)
10-10: Clear Documentation for Dependency Installation
The comment at line 10 explicitly notes that dependencies are being installed as root, which helps clarify the intent for subsequent commands that require elevated privileges.
45-45: Proper Placement of WORKDIR
Moving theWORKDIR /codecommand after the non-root user creation ensures that directory-related operations that follow are aligned with the intended user context.
46-46: Securing Application Directory Permissions
The commandRUN mkdir -p /code && chown -R app:app /codecorrectly creates the working directory and enforces the right ownership, ensuring that the non-root user can interact with it without permission issues.
48-48: Copying Critical Files with Correct Ownership
UsingCOPY --chown=app:app pyproject.toml poetry.lock ./sets the proper ownership on these key files. This not only helps avoid permission conflicts during dependency installation but also leverages Docker’s caching layers efficiently.
50-51: Switching to Non-Root Execution
Transitioning to the non-root user withUSER appreinforces container security by ensuring that subsequent operations and the running application do not execute with root privileges.
53-56: Installing Python Dependencies Securely
TheRUN poetry install --no-root \command (along with the cache cleanup) runs as the non-root user. This adheres to best practices by limiting privileges during package installation.
57-57: Copying the Application Code with Correct Ownership
The commandCOPY --chown=app:app . .ensures that all application files are copied with the appropriate ownership, maintaining consistency and avoiding permission issues later in the container lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/ciso_assistant/settings.py (1)
108-115: Well-implemented directory existence check.The added code ensures the
dbdirectory exists and creates it if needed, which is essential when running as a non-root user. This is a more reliable approach than the previous method of using a readme.txt file to create the directory upon checkout. The implementation properly uses theparents=Trueandexist_ok=Trueparameters to make the operation idempotent and includes appropriate logging.This change supports the PR objective of switching the backend to a non-root user, which is a security best practice. By ensuring directories exist programmatically, you're preventing potential issues when running with reduced privileges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ciso_assistant/settings.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/startup-tests.yml (1)
139-148: Dynamic Polling: Enhance Failure Handling in Migration Wait LoopThe new "Wait for migrations" step is a great improvement, replacing a fixed sleep with dynamic polling. However, note that if the maximum attempts (30) are reached without a successful response from the backend, the loop simply exits—potentially allowing subsequent steps to run even though migrations haven’t completed. I recommend adding explicit error handling after the loop to exit with a clear error message if the backend remains unresponsive.
For example, you could add the following diff after the loop:
-until docker compose exec backend curl -f http://backend:8000/api/build || [ $attempt -eq $max_attempts ] -do - attempt=$((attempt+1)) - echo "Waiting for migrations... ($attempt/$max_attempts)" - sleep 10 -done +until docker compose exec backend curl -f http://backend:8000/api/build || [ $attempt -eq $max_attempts ] +do + attempt=$((attempt+1)) + echo "Waiting for migrations... ($attempt/$max_attempts)" + sleep 10 +done +if [ $attempt -eq $max_attempts ]; then + echo "ERROR: Migrations did not complete after $max_attempts attempts." + exit 1 +fiThis change makes sure that if the backend remains unresponsive after the designated attempts, the workflow will clearly fail rather than proceeding with potentially unstable conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/startup-tests.yml(1 hunks)backend/ciso_assistant/settings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/ciso_assistant/settings.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
ab-smith
force-pushed
the
rootless_docker
branch
from
3e216b7 to
050cdb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docker-compose-build.yml (1)
12-12: AUTH_TOKEN_TTL Environment Variable Added
The addition ofAUTH_TOKEN_TTL=7200to the backend's environment is a clear configuration change. Ensure that the backend application is updated to read and enforce this token expiry configuration and that it’s documented accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/Dockerfile(1 hunks)docker-compose-build.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
docker-compose-build.yml (4)
15-20: Backend Healthcheck Configuration
The new healthcheck block for the backend service, which uses acurlcommand to verify the/api/buildendpoint, is an effective way to ensure service readiness. Please verify that the/api/buildendpoint exists and reliably indicates the backend's healthy state within the specified timeout and retry parameters.
28-30: Updated Dependency Condition for Huey Service
The adjustment in thedepends_onsection for the huey service to rely on the backend's healthy state (usingcondition: service_healthy) strengthens the startup sequence and prevents race conditions. Verify that the backend’s healthcheck consistently reflects its operability.
55-57: Updated Dependency Condition for Frontend Service
By requiring the backend service to be healthy before starting the frontend, this update improves the reliability of service interdependencies. Make sure that any potential delays in backend startup do not negatively impact the frontend's availability beyond acceptable tolerances.
63-65: Updated Dependency Condition for Caddy Service
Setting the caddy service’sdepends_oncondition to wait for a healthy backend should help ensure proper reverse proxy configuration. It is advisable to monitor the startup order in a staging environment to confirm that all services become available as expected without undue delays.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/startup.sh (1)
2-2: EnsuredbDirectory Creation and PermissionsThe addition of
mkdir -p ./dbon line 2 correctly ensures that thedbdirectory exists before the script attempts to read or create the Django secret key. Since the PR objective is to run the backend as a non‑root user, please verify that the directory is created with appropriate ownership and access permissions for that non‑root user. If needed, consider adding commands (e.g., usingchownorchmod) to enforce the desired permissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/startup.sh(1 hunks)docker-compose-build.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose-build.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
ab-smith
force-pushed
the
rootless_docker
branch
from
08e8cb8 to
384c77c
Compare
ab-smith
force-pushed
the
rootless_docker
branch
from
384c77c to
54c4d12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/startup.sh (1)
11-17: Permissions Check for Secret Key File CreationSince the backend will now run as a non-root user, ensure that the
dbdirectory (and the filedb/django_secret_key) has the correct write permissions. Failure to adjust these permissions might result in runtime errors when the script attempts to create or update the Django secret key.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 15-15: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/Dockerfile(1 hunks)backend/startup.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/startup.sh (2)
2-5: Environment Variable Addition for MatplotlibThe addition of
export MPLCONFIGDIR=/tmpis clear and appropriately placed at the top of the script. This ensures that Matplotlib uses a writable configuration directory when running under the non-root user. Please verify that the/tmpdirectory has the proper write permissions for your non-root setup.
34-42: Gunicorn Command Execution under Non-Root ContextThe command for launching Gunicorn is correctly structured. Given the switch to a non-root backend, double-check that all relevant file permissions and port settings (e.g., binding to port 8000) are correctly configured for non-root execution.
ab-smith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/Dockerfile (1)
25-26: Non-root User Creation on Alpine Image: Consider Using Alpine-Native CommandsThe changes correctly introduce a non-root user by creating a group and user with UID and GID 1001, which enhances container security. However, since this image is based on Alpine Linux, the default tools are usually
addgroupandadduserrather thangroupaddanduseradd. Verify that these commands are available in your environment, or consider switching to the Alpine-native commands.Example diff if you choose to migrate:
- RUN groupadd --gid 1001 app && useradd --uid 1001 --gid 1001 app + RUN addgroup -g 1001 app && adduser -D -u 1001 -G app appenterprise/frontend/Dockerfile (1)
27-29: Consistent Non-root User Setup for Alpine-based Image: Evaluate Alpine CommandsThe updated lines correctly switch the execution context to a non-root user (UID/GID 1001) to improve security. As with the frontend Dockerfile, note that Alpine images typically use
addgroupandadduser. Please confirm thatgroupaddanduseraddare available in the node:22-alpine image, or consider using the Alpine-standard commands.Example diff:
- RUN groupadd --gid 1001 app && useradd --uid 1001 --gid 1001 app + RUN addgroup -g 1001 app && adduser -D -u 1001 -G app app
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
enterprise/backend/Dockerfile(1 hunks)enterprise/frontend/Dockerfile(1 hunks)frontend/Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
enterprise/backend/Dockerfile (1)
56-57: Secure Non-root User Implementation on Debian-based ImageThe changes correctly create and switch to a non-root user (UID/GID 1001) in a Debian-based environment. Since the base image (
python:3.12-slim) uses standard Debian utilities, the use ofgroupaddanduseraddis appropriate and aligns with best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/startup-tests.yml (1)
280-282: Updated Sleep Duration in Enterprise Docker Compose TestThe sleep duration in the "Config the Docker app" step for the enterprise job has been increased to 180 seconds. This should allow sufficient time for migrations when running under the non-root Docker configuration. Consider verifying if differing wait times between similar jobs (e.g., the non-enterprise job uses 120 seconds) are intentional. If these delays might be subject to further tuning or if the wait period becomes a recurring parameter across jobs, you might consider parameterizing the sleep duration to simplify future adjustments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/startup-tests.yml(1 hunks)enterprise/frontend/Dockerfile(1 hunks)frontend/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- enterprise/frontend/Dockerfile
- frontend/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/huey.sh (2)
3-7: Comment: Update Misleading CommentThe comment on line 6 refers to “Gunicorn configuration”, which is misleading given that the environment variables and subsequent command are related to Huey. Please update the comment to reflect that these are default values for the Huey task scheduler.
-# Set default values for Gunicorn configuration +# Set default values for Huey configuration
10-12: Comment: Verify Execution Context with Non-Root UserThe script executes
poetry run python manage.py run_hueywith parameters derived from the environment. Since the PR objective is to switch containers to non-root, please verify that the non-root user (e.g.app) has the necessary permissions to execute this command and access required files (like the./dbdirectory created earlier).docker-compose-build.yml (1)
29-33: Comment: Inconsistent Huey Command UsageThe huey service’s entrypoint (line 33) now invokes
poetry run python manage.py run_hueywithout passing environment-variable–derived parameters. Given that a new script (
huey.sh) was introduced to manage these defaults, it seems more consistent to have the container execute that script. Consider updating the entrypoint to call thehuey.shscript (which resides in the build context) rather than directly callingmanage.py run_huey. For example:- entrypoint: - - /bin/sh - - -c - - | - poetry run python manage.py run_huey + entrypoint: ["/bin/bash", "./huey.sh"]This change would ensure that the default values for
HUEY_WORKERSandHUEY_INTERVALare applied as intended.enterprise/docker-compose-build.yml (1)
33-37: Comment: Evaluate Consistency in Huey Service EntrypointSimilar to the previous file, the huey service here (lines 33-37) directly runs
poetry run python manage.py run_hueywhich bypasses the
huey.shscript that sets and applies the default values forHUEY_WORKERSandHUEY_INTERVAL. For consistency and to leverage the script’s defaults, consider modifying the entrypoint to callhuey.sh. For instance:- entrypoint: - - /bin/sh - - -c - - | - poetry run python manage.py run_huey + entrypoint: ["/bin/bash", "./huey.sh"]Before applying this change, please verify that the path to
huey.shis correct within the image built via./enterprise/backend/Dockerfile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/huey.sh(1 hunks)backend/startup.sh(1 hunks)docker-compose-build.yml(1 hunks)enterprise/docker-compose-build.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/startup.sh
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: CodeQL
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
docker-compose-build.yml (1)
15-20: Comment: Confirm Dockerfile Changes for Non-Root ExecutionAlthough this diff does not show Dockerfile modifications, the PR objective specifies switching to non-root containers. Please double-check that the referenced Dockerfile (in
./backend/Dockerfile) is updated accordingly (i.e. creates and switches to a non-root user) so that all services, includingbackendandhuey, run securely.enterprise/docker-compose-build.yml (1)
23-24: Comment: Service Dependency ClarificationAdding the
depends_ondirective for thehueyservice (lines 23-24) helps control the startup order. This is a good practice to ensure that thebackendservice is available before starting huey.
ab-smith
force-pushed
the
rootless_docker
branch
from
bc0fa88 to
a0248f8
Compare
ab-smith
force-pushed
the
rootless_docker
branch
3 times, most recently
from
9342f38 to
d22abf7
Compare
Summary by CodeRabbit