Skip to content

fix(RELEASE-2750): add sanity checks for binaries in Dockerfile - #966

Open
davidmogar wants to merge 1 commit into
mainfrom
release2750
Open

fix(RELEASE-2750): add sanity checks for binaries in Dockerfile#966
davidmogar wants to merge 1 commit into
mainfrom
release2750

Conversation

@davidmogar

@davidmogar davidmogar commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

RELEASE-2748 (edf74ad) already added curl -f and version checks
for downloaded binaries, so only add what it missed: pipefail so
curl | tar failures aren't swallowed, and bash -n syntax checks
for the oras select-oci-auth and get-reference-base scripts.

Document the binary sanity-check convention in CONTRIBUTING.md
and AGENTS.md for future Dockerfile changes.

Assisted-by: Claude

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Summary by Qodo

Add curl failure + binary sanity checks to Dockerfile installs

🐞 Bug fix 📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Fail Docker builds on HTTP download errors via curl -f.
• Validate installed binaries at build time using version/help or bash -n.
• Document Dockerfile binary-download guidelines in CONTRIBUTING and AGENTS.
Diagram

graph TD
  A["Dockerfile"] --> B["Download binaries (curl -Lf)"] --> C["Install binaries (tar/gunzip/chmod)"] --> D["Sanity checks (--version/--help/bash -n)"]
  A --> E["COPY from stages"] --> D
  A --> G["Docs: CONTRIBUTING/AGENTS"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add checksum/signature verification for downloads
  • ➕ Stronger supply-chain integrity than version/help checks
  • ➕ Detects tampering even when binaries execute successfully
  • ➖ Requires managing per-arch checksums/keys and update workflow
  • ➖ More maintenance when versions bump frequently
2. Prefer distro packages (dnf) where available
  • ➕ Leverages signed repos and standard update mechanisms
  • ➕ Reduces custom curl/tar logic in Dockerfile
  • ➖ Many tools may not be available or up-to-date in repos
  • ➖ Harder to pin exact upstream versions across architectures
3. Centralize verification in a helper script
  • ➕ Less repetition and easier to enforce consistent checks
  • ➕ Simplifies future additions of new tools
  • ➖ Adds another artifact to maintain and audit
  • ➖ Still needs per-tool command knowledge (version/help/bash -n)

Recommendation: The PR’s approach (curl -f + post-install execution/syntax checks) is a pragmatic, low-maintenance baseline that materially improves build-time safety with minimal complexity. Consider a follow-up for checksum/signature verification on the highest-risk externally downloaded binaries if supply-chain hardening is a goal.

Files changed (3) +45 / -13

Bug fix (1) +28 / -13
DockerfileFail fast on download errors and verify all installed binaries +28/-13

Fail fast on download errors and verify all installed binaries

• Updates curl invocations used to fetch binaries to include -f so HTTP errors fail the build. Adds post-install checks (e.g., --version/--help or bash -n) after downloads, multi-stage COPY operations, and gunzip installs to ensure tools are functional at build time.

Dockerfile

Documentation (2) +17 / -0
AGENTS.mdDocument Dockerfile binary download and validation rules +8/-0

Document Dockerfile binary download and validation rules

• Adds a Dockerfile section mandating curl -f for binary downloads and requiring post-install sanity checks for all installed/copied binaries. Specifies version/help checks for compiled tools and bash -n for shell scripts.

AGENTS.md

CONTRIBUTING.mdAdd contributor guidelines for Dockerfile curl -f and sanity checks +9/-0

Add contributor guidelines for Dockerfile curl -f and sanity checks

• Introduces explicit guidance for Dockerfile edits: enforce curl -f on binary downloads and require post-install validation to catch corrupt or wrong-architecture artifacts. Clarifies which checks to use for compiled binaries vs shell scripts.

CONTRIBUTING.md

@davidmogar

Copy link
Copy Markdown
Contributor Author

I condensed the Skills section description to stay within the AGENTS.md 60 non-empty line limit.

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Curl errors masked in pipes 🐞 Bug ☼ Reliability
Description
In Dockerfile, several downloads are done via curl ... | tar ... without enabling pipefail, so
the pipeline’s exit status can reflect only tar and ignore a non-zero curl exit code when tar
still succeeds. This can allow certain curl failure modes (e.g., transfer errors after enough data
was written) to pass the build even though curl reported an error.
Code

Dockerfile[R30-32]

+    curl -Lf https://gitlab.com/gitlab-org/cli/-/releases/v${GLAB_VERSION}/downloads/glab_${GLAB_VERSION}_linux_${GO_ARCH}.tar.gz | tar -C /usr -xzf - bin/glab &&\
+    curl -Lf https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_${GO_ARCH}.tar.gz  | tar -C /usr -xzf - --strip=1 gh_${GH_VERSION}_linux_${GO_ARCH}/bin/gh &&\
+    curl -Lf https://github.com/anchore/syft/releases/download/v${SYFT_VERSION}/syft_${SYFT_VERSION}_linux_${GO_ARCH}.tar.gz | tar -C /usr/bin/ -xzf - syft &&\
Relevance

●● Moderate

No close repo precedent found; enabling pipefail in Dockerfile can be slightly
invasive/shell-dependent despite reliability benefit.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The Dockerfile uses curl -Lf … | tar … pipelines for multiple binaries, and there is no SHELL
directive configuring pipefail earlier in the file; therefore the pipeline may not fail on
curl’s non-zero exit code if tar succeeds.

Dockerfile[1-20]
Dockerfile[21-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`curl | tar` pipelines in the Dockerfile don’t guarantee that a failing `curl` causes the `RUN` step to fail, because (without `pipefail`) the pipeline status can be taken from `tar`.

## Issue Context
This PR adds `curl -f` to fail on HTTP errors, but `-f` does not address non-HTTP curl failures, and pipeline semantics can still hide `curl`’s exit status.

## Fix Focus Areas
- Dockerfile[21-33]

## Suggested fix
Choose one of:
1) Set a pipefail-capable shell for the Dockerfile (e.g. `SHELL ["/bin/bash", "-o", "pipefail", "-c"]`) before these RUN steps.
2) Avoid pipes for downloads: `curl ... -o /tmp/file.tgz && tar -xzf /tmp/file.tgz ...` so `curl` is checked directly.

Ensure the change covers the `glab`, `gh`, and `syft` download/extract steps.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
✅ Compliance rules (platform): 25 rules

Grey Divider

Tip of the day
💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread Dockerfile Outdated
@davidmogar
davidmogar force-pushed the release2750 branch 2 times, most recently from c076d75 to 79b90b5 Compare September 1, 2026 13:13
Comment thread AGENTS.md Outdated
@theflockers

Copy link
Copy Markdown
Contributor

Typo in the Assisted-By.

RELEASE-2748 (edf74ad) already added curl -f and version checks
for downloaded binaries, so only add what it missed: pipefail so
curl | tar failures aren't swallowed, and bash -n syntax checks
for the oras select-oci-auth and get-reference-base scripts.

Document the binary sanity-check convention in CONTRIBUTING.md
and AGENTS.md for future Dockerfile changes.

Assisted-by: Claude
Signed-off-by: David Moreno García <damoreno@redhat.com>
@davidmogar

Copy link
Copy Markdown
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants