fix: migrate Dockerfile from Docker to Quay Fedora#924
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDockerfile changed base image from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/state-sync/Dockerfile`:
- Line 1: The Dockerfile currently uses an unpinned base image and runs as root;
replace FROM quay.io/fedora/fedora:latest with a specific, pinned Fedora tag
(e.g., quay.io/fedora/fedora:<version>) to ensure reproducible builds, and add
non-root user setup: create a dedicated group/user, set a HOME, chown any app
directories, and switch to that user with USER so the container no longer runs
as root; update any relevant build/run steps in the Dockerfile (the FROM line
and user setup) to implement these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fc74d34-359a-4fe8-80b8-9d61020d99b3
📒 Files selected for processing (1)
components/runners/state-sync/Dockerfile
06d2732 to
0308682
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/runners/state-sync/Dockerfile (1)
1-14:⚠️ Potential issue | 🟠 MajorContainer still runs as root (missing
USER).There is no non-root user configuration, so runtime defaults to root. This is a security posture gap that should be fixed before release.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."Proposed fix
FROM quay.io/fedora/fedora:43 RUN dnf install -y rclone git jq && \ dnf clean all # Copy scripts COPY hydrate.sh /usr/local/bin/hydrate.sh COPY sync.sh /usr/local/bin/sync.sh # Make scripts executable RUN chmod +x /usr/local/bin/hydrate.sh /usr/local/bin/sync.sh + +# Run as non-root +RUN groupadd -r statesync && useradd -r -g statesync -d /home/statesync -m statesync && \ + chown statesync:statesync /usr/local/bin/hydrate.sh /usr/local/bin/sync.sh +USER statesync # Default to sync.sh (used by sidecar) ENTRYPOINT ["/usr/local/bin/sync.sh"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/state-sync/Dockerfile` around lines 1 - 14, The Dockerfile currently leaves the container running as root; to fix, add a non-root user and switch to it after installing packages and copying files: create a user (e.g., appuser) and group, chown /usr/local/bin/hydrate.sh and /usr/local/bin/sync.sh (and any other runtime directories) to that user, and add a USER appuser line before the ENTRYPOINT so the container runs unprivileged; ensure any files the scripts need to write have appropriate ownership/permissions and that HOME is set for the new user if required by the scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/runners/state-sync/Dockerfile`:
- Around line 1-14: The Dockerfile currently leaves the container running as
root; to fix, add a non-root user and switch to it after installing packages and
copying files: create a user (e.g., appuser) and group, chown
/usr/local/bin/hydrate.sh and /usr/local/bin/sync.sh (and any other runtime
directories) to that user, and add a USER appuser line before the ENTRYPOINT so
the container runs unprivileged; ensure any files the scripts need to write have
appropriate ownership/permissions and that HOME is set for the new user if
required by the scripts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: addd9fc2-8a46-4c6f-8bf7-762f7f2029a1
📒 Files selected for processing (1)
components/runners/state-sync/Dockerfile
Replace alpine:3.19 with quay.io/fedora/fedora:latest to avoid pulling from Docker Hub. Use dnf instead of apk to install rclone, git, and jq. Bash, curl, and ca-certificates are already included in the Fedora base image. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
0308682 to
279ab45
Compare
|
@ktdreyer — the review queue analysis indicates your feedback from March 18 may have been addressed in the March 18 (17:03 UTC) commit. Could you re-review? |
|
@ktdreyer mind clearing the "review requested" here so this can get queued? |
Replace alpine:3.19 with quay.io/fedora/fedora:latest to avoid pulling from Docker Hub. Use dnf instead of apk to install rclone, git, and jq. Bash, curl, and ca-certificates are already included in the Fedora base image. Signed-off-by: Martin Prpič <mprpic@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Replace alpine:3.19 with quay.io/fedora/fedora:latest to avoid pulling from Docker Hub. Use dnf instead of apk to install rclone, git, and jq. Bash, curl, and ca-certificates are already included in the Fedora base image.