Address image CVE burden: float bases, trim runtime, unify Go toolchain - #4
anthonytwh wants to merge 19 commits into
Conversation
Analysis of the ISS v2.12.7 scan (2026-08-28, 13,548 unique findings across 8 images) identifying five root causes and a prioritized remediation plan. Key findings: - 74% of all findings come from the libguestfs supermin appliance kernel in forklift-virt-v2v, reported grype-only and matched against upstream kernel.org ranges rather than RHEL errata - 35 of 40 Containerfiles never run a package update, so the pinned base digest is the permanent patch level (3.5 to 12 months stale) - weak dependencies pull webkit2gtk3, qt5, gstreamer and libsoup into runtime images - four different Go toolchains across the image set - must-gather and console-plugin are consumed prebuilt and cannot be patched
Acts on P1-P3 of docs/security/cve-remediation-plan.md, driven by the ISS v2.12.7 scan (13,548 unique findings, 3,043 crit/high across 8 images). P1 - base images now move again. Every upstream base went from a frozen build-timestamp tag to a floating tag (ubi9-minimal:9.7-1778562320 -> ubi9-minimal:latest, and equivalents for ubi9, ubi8, nginx-126). The pins were 3.5 to 12 months stale and, since 35 of 40 Containerfiles ran no package update, the pinned digest was the permanent patch level. Also removed the four literal kernel NEVRA pins from .konflux/virt-v2v/runtime/rpms.in.yaml so refresh-rpm-lockfiles can advance the appliance kernel instead of holding it frozen forever. Trade-off: builds are no longer bit-reproducible from the Containerfile alone. The registry.redhat.io bases in the -downstream files are left pinned; those are Konflux/Mintmaker-managed. P2 - runtime trim plus update. All 40 install sites now disable weak dependencies, and the non-hermetic runtime stages update first. Measured on centos:stream9, virt-v2v goes from 324 packages / 2.2 GB to 297 / 1.4 GB with nothing added, dropping webkit2gtk3-jsc, libproxy-webkitgtk4, linux-firmware, nfs-utils, rpcbind and friends. passt and libvirt-daemon-config-network are now installed explicitly, because libguestfs only recommends them and the image runs LIBGUESTFS_BACKEND=libvirt:qemu:///session, which breaks without them. The nbdkit metapackage is dropped safely: virt-v2v hard-requires every plugin it actually uses. Note microdnf accepts --setopt=install_weak_deps=0 but rejects =False, so all sites were normalised to =0. P3 - one Go toolchain. All 32 builder stages move to go-toolset:1.26; the shipped images previously spanned Go 1.22.5, 1.24.6, 1.25.9 and 1.26.3. Module go directives unified on 1.26, and the CVE-bearing dependencies bumped: x/crypto v0.50.0 -> v0.55.0 and x/net v0.53.0 -> v0.58.0 in the main module, x/crypto v0.40.0 -> v0.55.0, x/net v0.42.0 -> v0.58.0 and containerd/v2 v2.1.4 -> v2.3.4 in the validation module. vendor/ regenerated. build/validation/Containerfile no longer curls a prebuilt opa binary from GitHub releases with no checksum. It builds OPA from source with the repo's own toolchain, matching what Containerfile-downstream already did. That curl was what pinned the validation image to Go 1.22.5, the oldest toolchain in the product. Verified: forklift-controller, forklift-api, validation, ova-provider-server and populator-controller all build; the shipped opa binary reports go1.26.7 (Red Hat 1.26.7-1.el9_8) X:strictfipsruntime and links containerd/v2 v2.3.4, x/crypto v0.55.0, x/net v0.58.0; weak-dependency deltas measured in a real container rather than assumed. Not verified locally: virt-v2v and ovirt-populator need a Red Hat subscription, and the Konflux hermetic builds need rpms.lock.yaml regeneration with entitled repos.
There was a problem hiding this comment.
🟡 Changes recommended
Downstream virt-v2v runtime images disable weak deps but don’t explicitly install passt and libvirt-daemon-config-network despite using LIBGUESTFS_BACKEND=libvirt:qemu:///session, which can cause runtime breakage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements the P1–P3 CVE remediation steps by (1) allowing base images to float (with explicit update steps), (2) trimming runtime dependencies by disabling weak deps and explicitly installing required packages, and (3) unifying the repository on a single Go toolchain (go-toolset 1.26) with updated vendored dependencies. It also removes an unverified OPA binary download in the validation image by building OPA from source using the repo’s Go toolchain.
Changes:
- Switch multiple Containerfiles from pinned UBI/nginx digests to
:latestand add/update package update+cleanup steps; normalize weak-deps handling (install_weak_deps=0). - Unify Go toolchain usage to
go-toolset:1.26and update Go module/vendored dependencies accordingly. - Rework validation image build to compile OPA from source rather than downloading a prebuilt binary.
File summaries
| File | Description |
|---|---|
| go.mod | Bumps module go directive to 1.26.0 and updates golang.org/x/* versions. |
| vendor/modules.txt | Reflects updated vendored module versions (x/crypto, x/net, x/tools, etc.). |
| vendor/golang.org/x/tools/** | Updates x/tools internals for newer Go/export formats and iter APIs. |
| vendor/golang.org/x/net/** | Updates http2/idna/html logic and adds go1.27 transition build tags. |
| vendor/golang.org/x/crypto/** | Security/robustness updates across ssh/pbkdf2/poly1305/cryptobyte. |
| vendor/golang.org/x/sys/** | Regenerated syscall/types/constants across OS/arch targets. |
| build/validation/Containerfile | Replaces unverified curl download of opa with a source build using repo toolchain; updates base to ubi9-minimal:latest and adds microdnf update. |
| build/validation/Containerfile-downstream | Moves builder to go-toolset:1.26. |
| build/virt-v2v/Containerfile | Floats base images to ubi9:latest, disables weak deps, adds explicit passt and libvirt-daemon-config-network, and installs catatonit in the main install transaction. |
| build/virt-v2v/Containerfile-upstream | Adds dnf update, disables weak deps, and explicitly installs passt + libvirt-daemon-config-network. |
| build/virt-v2v/Containerfile-upstream-xfs | Same runtime-trim pattern plus explicit dependencies. |
| build/virt-v2v/Containerfile-upstream-fedora | Same runtime-trim pattern plus explicit dependencies. |
| build/virt-v2v/Containerfile-downstream | Updates to go-toolset:1.26 and normalizes weak deps setting (but currently missing explicit passt/network packages in runtime install). |
| build/virt-v2v/Containerfile-downstream-fssupport | Adds install_weak_deps=0 to hermetic installs (but currently missing explicit passt/network packages in pinned RPM set). |
| build/virt-v2v-rhel9/Containerfile | Floats base images to ubi9:latest, disables weak deps, adds explicit passt and libvirt-daemon-config-network, and installs catatonit in the main install transaction. |
| build/virt-v2v-rhel9/Containerfile-downstream | Updates to go-toolset:1.26 and normalizes weak deps setting (but currently missing explicit passt/network packages in runtime install). |
| .konflux/virt-v2v/runtime/rpms.in.yaml | Removes kernel NEVRA pins so lockfile refresh can advance the appliance kernel. |
| .konflux/forklift-operator-bundle/go.mod | Unifies Konflux operator-bundle go directive to 1.26.0. |
| build/forklift-api/Containerfile | Floats runtime base to ubi9-minimal:latest, adds microdnf update, and moves builder to go-toolset:1.26. |
| build/forklift-controller/Containerfile | Same toolchain/base update pattern as other Go-built images. |
| build/populator-controller/Containerfile | Same toolchain/base update pattern; normalizes weak deps disabling for tar install. |
| build/ova-provider-server/Containerfile | Same toolchain/base update pattern; normalizes weak deps disabling for tar install. |
| build/ova-proxy/Containerfile | Same toolchain/base update pattern; floats base to ubi9-minimal:latest. |
| build/openstack-populator/Containerfile | Same toolchain/base update pattern; adds microdnf update. |
| build/hyperv-provider-server/Containerfile | Same toolchain/base update pattern; adds microdnf update. |
| build/ovirt-populator/Containerfile | Floats ubi8/ubi base to :latest, adds dnf update, and moves builder to go-toolset:1.26. |
| build/ovirt-populator/Containerfile-upstream | Adds dnf update, disables weak deps, and moves builder to go-toolset:1.26. |
| build/forklift-cli-download/Containerfile | Moves builder to go-toolset:1.26 and floats nginx base to ubi9/nginx-126:latest. |
| build/vsphere-copy-offload-populator/Containerfile | Moves builder to go-toolset:1.26 and floats runtime base to ubi9-minimal:latest with microdnf update. |
| build/forklift-operator-index/Containerfile | Floats builder base to ubi9-minimal:latest, adds microdnf update, disables weak deps for gettext. |
| build/deep-inspection/Containerfile | Moves builder to go-toolset:1.26, adds dnf update, disables weak deps in install step. |
| build/deep-inspection-rhel9/Containerfile | Moves builder to go-toolset:1.26, adds dnf update, disables weak deps in install step. |
Review details
- Files reviewed: 41/238 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two follow-ups found while evaluating how to test these builds in CI. golang.org/x/oauth2 v0.23.0 -> v0.36.0. CVE-2025-22868 (HIGH, unbounded memory in x/oauth2/jws token parsing, fixed in 0.27.0). Missed by the earlier bump because it is an indirect dependency that the x/crypto and x/net upgrades did not pull forward. It was the only finding trivy reported against the rebuilt virt-v2v image; that image now scans clean. CI workflows pinned the Go version as a literal, with a "keep this in sync with go.mod" comment that had already gone stale - pull-request.yml was on 1.25.9 and release-branch-pull-request.yml on 1.24.4 while go.mod asked for 1.26. Moving the main module to the go.mod directive, and the vendor-verify job to the vsphere-copy-offload submodule it actually builds. Four workflows already did this; now all of them do, so the pin cannot drift again.
There was a problem hiding this comment.
🟡 Changes recommended
CI/toolchain configuration is inconsistent (outdated actions/setup-go@v4 in key workflows and a Go 1.25 version file used for vendoring while builders moved to Go 1.26), which risks breaking builds and contradicts the PR’s “unified toolchain” intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
build/forklift-controller/Containerfile-downstream:1
- This downstream Containerfile changed from a build-specific go-toolset tag to the floating :1.26 tag. The PR description notes that -downstream images should remain pinned (Konflux/Mintmaker-managed) to preserve reproducibility; using a floating tag here makes the downstream build non-deterministic and may diverge from that plan.
Recommend pinning this base (e.g., to a specific go-toolset build tag or digest) consistently across downstream Containerfiles.
- Files reviewed: 44/251 changed files
- Comments generated: 3
- Review effort level: Lite
Review flagged that disabling weak deps on the virt-v2v images drops passt and libvirt-daemon-config-network, which libguestfs only recommends and which the libvirt:qemu:///session backend needs. Three files were named; they need three different answers. Containerfile-downstream-fssupport - this one was a real regression introduced by this branch. That transaction installs from Brew RPM URLs over the network and previously ran with weak deps on, so both packages were being pulled in. Reverted to weak-deps-enabled with a comment explaining why. A forward fix (weak deps off plus both packages explicit) could not be validated here, since UBI10 will not run under emulation on an arm64 host, so the known-good behaviour was restored rather than shipping an unverified change. virt-v2v/Containerfile-downstream and virt-v2v-rhel9/Containerfile-downstream - pre-existing, not introduced here. Both already ran with install_weak_deps=False before this branch, so both packages were already absent. These are hermetic Konflux builds, so adding them to the install would fail unless they are in rpms.lock.yaml. Staged both in the matching rpms.in.yaml so the next refresh-rpm-lockfiles pulls them into the lock; the Containerfile change has to follow that refresh, not precede it. Also moved the explanatory comment out of a shell line-continuation, where it would have been fragile at best. setup-go bumped to v6 in pull-request.yml and release-branch-pull-request.yml as suggested. Extended the same fix to verify-manifests.yml (was v4) and validate-forklift-controller-crd.yaml (was v5): both resolve the root go.mod, so bumping it to 1.26 put them at the same risk even though they are not in this diff. release-cli.yml is left alone - it resolves the kubectl-mtv module, which is still on 1.25. The golangci-lint vendor-verify job keeps go-version-file pointed at cmd/vsphere-copy-offload-populator/go.mod rather than the root: that job runs go mod vendor inside that submodule, so it should use that module's own directive. The submodule being on 1.25 is a real gap, but the fix is bumping the module, not misconfiguring the job.
There was a problem hiding this comment.
🔵 Needs a closer look
CI now selects Go from cmd/vsphere-copy-offload-populator/go.mod (Go 1.25) for one job, undermining the stated “single Go toolchain” objective and risking inconsistent CI outcomes.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
build/validation/Containerfile:14
- The OPA build step doesn’t use BuildKit cache mounts, unlike the other Go build stages in this repo. This will force a full module/build cache download/compile on every image build, increasing CI time and load on module infrastructure.
.github/workflows/golangci-lint.yaml:68
- The offload vendor verification job now derives the Go version from
cmd/vsphere-copy-offload-populator/go.mod, which currently declaresgo 1.25.0. This reintroduces multiple Go toolchains in CI (main jobs use the rootgo.modat Go 1.26) and can cause inconsistent build/lint behavior versus the Containerfiles that now usego-toolset:1.26.
- uses: actions/setup-go@v6
with:
go-version-file: cmd/vsphere-copy-offload-populator/go.mod
cache: false
- Files reviewed: 47/254 changed files
- Comments generated: 0 new
- Review effort level: Lite
…nputs None of the workflows in .github/workflows run in this fork. The only checks on a PR here are CodeQL, Copilot review and the CLA, all injected at org level rather than from the repo, so the setup-go and go-version-file changes were churn against dormant upstream config. Reverted. Same for the Konflux inputs: .konflux/forklift-operator-bundle/go.mod and the two virt-v2v runtime rpms.in.yaml files feed .tekton pipelines that run in Red Hat's rh-mtv-1-tenant and cannot run here. The kernel NEVRA unpin and the passt/libvirt-daemon-config-network staging both go with them; the plan doc now records them as deliberately not done rather than claiming otherwise. .konflux/validation/go.mod and go.sum are kept, and are the one thing under .konflux that is not pipeline config. build/validation/Containerfile copies both in to build the OPA binary that ships in forklift-validation, so reverting them would put x/crypto v0.40.0, x/net v0.42.0 and containerd/v2 v2.1.4 back into a shipped image - the opposite of the point of this branch. Verified the image still builds and reports go1.26.7 with the bumped dependencies. Added a scope section to the plan doc covering what is untouched and why, including the note that those workflows would need setup-go@v6 and go-version-file before they could run green against a go 1.26 module - a prerequisite for standing up CI here, not a change worth making while dormant.
Builds and pushes every image to gcr.io/unique-caldron-775/anthony so they can be pulled by a scanner. Delegates to the existing push-all-images chain via target-specific REGISTRY and REGISTRY_ORG overrides, which propagate to the prerequisite build/push targets, so no target definitions are duplicated. REGISTRY_TAG still applies, so images land as gcr.io/unique-caldron-775/anthony/<name>:devel-amd64 by default. Override with REGISTRY_TAG=... or point somewhere else with GCR_REGISTRY_ORG=...
GCR treats everything after the project id as part of the image name, so the anthony/ segment was convention rather than a requirement. Images now go to gcr.io/unique-caldron-775/<name> directly. Kept as a variable, so a per-user path is still one flag away if the project gets crowded: make push-all-images-gcr GCR_REGISTRY_ORG=unique-caldron-775/$(USER)
There was a problem hiding this comment.
🔵 Needs a closer look
The change set combines broad base-image floating, runtime package update behavior changes, and large vendored dependency updates, which warrants human verification of build and runtime impacts across the affected images.
Review details
- Files reviewed: 40/247 changed files
- Comments generated: 1
- Review effort level: Lite
| @@ -1,4 +1,4 @@ | |||
| FROM registry.redhat.io/ubi9/go-toolset:1.25.9-1778604137 AS builder | |||
| FROM registry.redhat.io/ubi9/go-toolset:1.26 AS builder | |||
Keeps the images out of the shared unique-caldron-775 project root rather than sitting alongside everything else in it. Still overridable via GCR_REGISTRY_ORG.
There was a problem hiding this comment.
🟡 Changes recommended
Multiple Containerfiles use microdnf install --setopt=install_weak_deps=0 ..., but --setopt must be passed before the subcommand in microdnf, otherwise image builds will fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 40/247 changed files
- Comments generated: 16
- Review effort level: Lite
|
|
||
| FROM registry.redhat.io/ubi10/ubi-minimal:10.1-1778576723 | ||
| RUN microdnf install -y \ | ||
| RUN microdnf install -y --setopt=install_weak_deps=0 \ |
…bly helps Two fixes from a full make build-all-images run. build-vsphere-copy-offload-populator-image was failing with "inconsistent vendoring". cmd/vsphere-copy-offload-populator carries `replace github.com/kubev2v/forklift => ../../`, so raising the root module's dependencies raises that submodule's effective requirements too. Its Containerfile runs `go mod download` before `make build`, which rewrites the submodule go.mod to the raised versions, and the following vendor-mode build then finds vendor/modules.txt stale. Resynced the submodule with go mod tidy + go mod vendor, which also carries x/crypto v0.50.0 -> v0.55.0 into a shipped image. Root go.mod goes back to `go 1.25.0`. The 1.26 bump there was cosmetic - every bumped dependency (x/crypto v0.55.0, x/net v0.58.0, x/oauth2 v0.36.0, x/tools v0.48.0) declares only go 1.25.0. Because of the replace above it forced the submodule to 1.26 as well, and the go directive selects GODEBUG defaults, so Go 1.26's stricter net/url host:port parsing started rejecting a malformed URL in internal/vantara. That suite passes on main and failed on this branch; with the directive back at 1.25.0 all 10 packages pass again. Builders stay on go-toolset:1.26, so shipped binaries still carry the 1.26 stdlib - the directive is a floor, not the build version. .konflux/validation stays at 1.26.3, which containerd v2.3.4 genuinely requires; it has no replace and is independent. install_weak_deps is now applied only where it measurably changes the image. Measured with dnf install --assumeno: virt-v2v 324 -> 297 packages and 2.2 GB -> 1.4 GB, deep-inspection 408 -> 330 packages, both worth keeping. ovirt-populator was 8 packages either way, and tar on ubi9-minimal pulls in exactly one package - itself - so the flag was a no-op on all ten ubi-minimal images. Removed there. microdnf -y update alone gets current Red Hat libraries, matching what calico-base already does. Files that used the =False spelling before this branch keep it, so the diff stays minimal; the two microdnf sites use =0, which microdnf requires. Verified: vsphere-copy-offload-populator, forklift-controller and forklift-api all build, and the vsphere-copy-offload test suite is green.
There was a problem hiding this comment.
🟡 Changes recommended
The vendored ForEachElement change can incorrectly skip method traversal due to an overly broad TypeParams() != nil check, which risks breaking x/tools behavior at runtime/build time.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 43/459 changed files
- Comments generated: 0 new
- Review effort level: Lite
Pushes the same 15 images to quay.io/tigeradev so Red Hat Quay's own vulnerability rendering can be compared against the ISS results. Flat namespace deliberately: Quay repositories are namespace/name only, with no nested paths, so there is no tigera-forklift/ segment as there is on GCR. This also matches the existing quay.io/kubev2v/<name> convention the upstream workflow uses. Reuses TIGERA_IMAGES, so the operator index is excluded for the same reason as the GCR target: its build runs opm render against the bundle in the registry, which needs the bundle already pushed and pullable by the build container.
There was a problem hiding this comment.
🔵 Needs a closer look
Several downstream images now float to :latest without consistently running package updates, and there are stated PR goals (pinned downstream bases / unified go directives) that currently conflict with what’s in the diff.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
build/ova-proxy/Containerfile-downstream:14
- This runtime stage also switched to a floating base (
ubi9-minimal:latest) but has nomicrodnf updatestep. Add an update+clean layer so rebuilt images actually pick up fixed RPMs even if the base tag lags behind.
build/validation/Containerfile:5 - PR description states that module
godirectives are unified as part of the Go toolchain consolidation, but the repo root still declaresgo 1.25.0(seego.mod:3) while images and vendored deps have moved forward. If the project intends to require Go 1.26, thegodirectives should be bumped in the root module (and any shipping submodules) to match the toolchain policy.
build/forklift-controller/Containerfile-downstream:15
- The runtime stage switched to a floating base (
ubi9-minimal:latest), but it still doesn't runmicrodnf update. Depending on when the base image was published, this can leave known-fixed CVEs in the shipped image. Runmicrodnf -y updatebefore installingtar(and keepclean all).
build/vsphere-copy-offload-populator/Containerfile-downstream:15 - PR description says the
registry.redhat.iobases in*-downstreamContainerfiles are left pinned (Konflux/Mintmaker-managed), but this file now floatsubi9-minimal:latest. Please either re-pin this base (if downstream should stay Mintmaker-managed) or update the PR description/plan so expectations match what the downstream build will do.
build/ova-provider-server/Containerfile-downstream:14 - This downstream runtime stage uses
ubi9-minimal:latestbut still skipsmicrodnf update, which can leave fixed CVEs present depending on base publish timing. Consider applying the same update-before-install pattern across other*-downstreamimages withubi*-minimal:latest(e.g. populator-controller, hyperv-provider-server).
- Files reviewed: 43/459 changed files
- Comments generated: 0 new
- Review effort level: Lite
aaaaaaaalex
left a comment
There was a problem hiding this comment.
Reviewed against the release tree. The load-bearing claims hold up: passt and libvirt-daemon-config-network are explicitly installed on both virt-v2v variants we ship, so the weak-deps trim can't break guest conversion; OPA is now pinned and checksum-verified through go.mod/go.sum instead of an unverified curl; both microdnf sites use =0, so the one tool that rejects =False never sees it. The fedora-variant disable is documented rather than deleted, and the new image targets' reason for excluding the operator index matches behavior we've hit ourselves.
Two things to fix before merge, one of them in the branch history:
An 81 MB compiled binary is committed: cmd/kubectl-mtv/kubectl-mtv (mode 755). It has to come out by rewriting the branch — a follow-up deletion commit still leaves 81 MB in history forever — and a .gitignore entry should keep it from coming back. (Filed in the body because a review comment can't anchor to a binary file.)
The description has drifted from the branch. It still describes the kernel-NEVRA unpinning in rpms.in.yaml and the CI workflow bumps, both reverted in 56aaad8 (the comment threads say so; the body doesn't), and the "all 40 sites normalised to =0" claim doesn't match the branch (see inline comment). Worth updating since the body becomes the merge commit's story.
🤖 Generated with Claude Code
|
|
||
| RUN subscription-manager refresh && \ | ||
| dnf update -y && \ | ||
| dnf install -y --setopt=install_weak_deps=False \ |
There was a problem hiding this comment.
The description says all install sites were normalised to install_weak_deps=0, but the branch has 21 sites of which 13 dnf sites (this one included) still say =False. Nothing is broken — dnf accepts both spellings and the two microdnf sites are already =0 — but the normalisation this claim promises didn't land. Suggest finishing it (13 one-character-class edits) or dropping the claim from the description.
|
|
||
| ### P4 — must-gather / console-plugin — **NOT ACTIONED** | ||
|
|
||
| Pending a decision on whether `must-gather` is used in the Calico Enterprise L2 migration |
There was a problem hiding this comment.
Two things on the must-gather paragraph.
Wording: suggest "whether must-gather is used in the L2 migration flow" — the sentence doesn't need to name a product.
On the pending decision itself: suggest deciding in favour of building must-gather in this fork rather than consuming it prebuilt. kubev2v/forklift-must-gather is Apache-2.0 with its own Containerfile, so nothing blocks it, and the mirror path has already bitten us once — the digest our first release mirrored turned out to be two years stale. Building it here also puts the largest remaining actionable block (299 crit/high in the scan) under the same floating-base and weak-deps regime as every other image in this PR.
| # Forklift Image CVE Remediation Plan | ||
|
|
||
| Basis: ISS scan `forklift-latest / v2.12.7 / results_image_all_2026-08-28::21:29:57:055` | ||
| (`gs://iss-upload/forklift-latest/v2.12.7/.../results/raw_results.csv`), 8 images, |
There was a problem hiding this comment.
The gs://iss-upload/... bucket path adds nothing for readers of this repo — the scan identifier on the line above already names the dataset. Suggest dropping the path.
Review flagged that the description claimed all install sites were normalised to install_weak_deps=0 while 15 dnf/yum sites still read =False. Nothing was broken - dnf and yum both accept either spelling, and the two microdnf sites, which reject =False, were already =0 - but the claim did not match the branch. Finished the normalisation rather than dropping the claim. All 23 sites now read =0. The 15 changed sites sit in four files this branch already modifies, so the diff footprint does not grow, and it removes the trap of someone copying a =False onto a microdnf line. Confirmed yum accepts =0 in a centos:stream9 container, since two of the sites use yum rather than dnf. Also added .gitignore entries for the two Go binaries that `go build ./...` drops into a module directory when run there. cmd/kubectl-mtv/kubectl-mtv was committed by accident in d6bbe18 - an 80 MB darwin/arm64 build produced by a local verification step and swept in by `git add -A`. The ignore rule does not take effect until the file leaves the index, which needs a history rewrite; this commit only stops it recurring.
There was a problem hiding this comment.
🔵 Needs a closer look
There is a confirmed potential panic in newly added vendored helper code and several downstream runtime Containerfiles still omit a package update step needed to meet the PR’s stated CVE-reduction intent.
Review details
Suppressed comments (4)
build/populator-controller/Containerfile-downstream:14
- This runtime stage installs
tarbut leaves themicrodnf updatestep commented out, so already-installed base RPMs won’t get security errata at build time. Since the goal of this PR is CVE reduction, runmicrodnf -y updatebefore installing additional packages.
build/forklift-controller/Containerfile-downstream:15 - This runtime stage installs
tarbut leaves themicrodnf updatestep commented out, so already-installed base RPMs won’t get security errata at build time. Since the goal of this PR is CVE reduction, runmicrodnf -y updatebefore installing additional packages.
build/ova-provider-server/Containerfile-downstream:14 - This runtime stage installs
tarbut leaves themicrodnf updatestep commented out, so already-installed base RPMs won’t get security errata at build time. Since the goal of this PR is CVE reduction, runmicrodnf -y updatebefore installing additional packages.
build/hyperv-provider-server/Containerfile-downstream:14 - This runtime stage installs
tarbut leaves themicrodnf updatestep commented out, so already-installed base RPMs won’t get security errata at build time. Since the goal of this PR is CVE reduction, runmicrodnf -y updatebefore installing additional packages.
- Files reviewed: 43/460 changed files
- Comments generated: 0 new
- Review effort level: Lite
Most of this PR edits files the fork never builds38 Containerfiles change here. Only 14 of them are ever built by this fork: The other 24 are dead changes in this repo:
Two consequences worth acting on:
Two unrelated items from the same pass:
Also, for the record: Copilot's seven |
Review pointed out that 38 Containerfiles change here while only 14 are built by this fork, and that a measurement in the P2 table credited an image the fork does not produce. The deep-inspection row is now marked: it is not in TIGERA_IMAGES, so nothing here builds or pushes it, and it was in neither the 08-28 nor the 09-03 scan. The measurement is real and is why the flag stays in that file, but it is not an improvement this fork can claim. Every other figure in the results tables comes from an image in TIGERA_IMAGES, which is also exactly the scanned set. Both the plan doc and the PR description now state why the other 24 files are edited anyway: consistency with upstream. Letting the two lineages diverge makes future rebases harder and leaves a reader unable to tell which file is authoritative. Nothing in those 24 should be read as an image this fork hardened. Also reverted the package update added to the operator-index builder stage. That stage's only output is operator.yaml, and operator-index is not in TIGERA_IMAGES, so the update contradicted the rule applied everywhere else on this branch - builder stages get no update because only their output leaves them. The :latest base pin stays.
There was a problem hiding this comment.
🔵 Needs a closer look
The remediation plan still claims the root module uses a Go 1.26 directive, while the final implementation intentionally keeps it at Go 1.25.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/security/cve-remediation-plan.md:215
- The final module directives are not unified on 1.26:
go.modis stillgo 1.25.0, while only.konflux/validation/go.modisgo 1.26.3. The implementation intentionally kept the root directive at 1.25 because it controls GODEBUG behavior for the replaced vsphere submodule, so this plan text (and the follow-up explanation below) now contradicts the code and the PR description; please update it to document the intentional split and the fact that the 1.26 builder still supplies the stdlib.
- Files reviewed: 43/460 changed files
- Comments generated: 0 new
- Review effort level: Lite
Removed in a3051a8 on the grounds that a builder stage whose only output is operator.yaml gains nothing from a package update. That reasoning was too narrow: it optimises for CVE effect in an image this fork does not build, at the cost of the consistency rule applied across the other 23 unbuilt Containerfiles. The cost of keeping it is a few seconds of build time. The benefit is that the stage picks up a newer base when one lands, and that a reader is not left wondering why this one file diverges. Noted in the plan doc so the next person does not prune it again for the same wrong reason.
|
Checked all of it against the branch. Your counts are exact: The 15th entry in The 24 are deliberate, and that is now written downThey are kept in step with upstream on purpose. Letting the two lineages diverge makes every future rebase harder and leaves a reader unable to tell which file is authoritative. You were right that nothing in the diff signalled it — there is now a "Which files this fork actually builds" section in both the PR description and That extends to builder-stage updates in files nothing here builds, like You caught a real numberThe I checked the rest rather than assuming — every image named in the results tables maps to a The assumption behind "not built = no effect", now verifiedFor an unbuilt Containerfile to affect one of the 14, a built image would have to pull content from another image. Every Both resolve to stages declared in the same file; zero cross-image The 80 MB binary is gone — force-pushed
Verified before pushing: Confirmed on the remote too — the binary no longer appears in the PR file list. For the record on how it got in: This force-push invalidated the inline comment anchors on this PR. Unavoidable with any rewrite, but worth knowing if a thread now points somewhere odd. Still open: signoff0 of 17 non-merge commits carry a
And thanks for the Copilot noteThat matches what I found independently: |
51dd320 to
c3f19ac
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
build/virt-v2v/Containerfile-upstream:28
- This shipped
virt-v2vtarget still builds its four Go binaries in the earlierquay.io/centos/centos:stream9stage with the distrogolangpackage (lines 2–13), rather thango-toolset:1.26. The Makefile'sbuild-virt-v2v-imagetarget uses thisContainerfile-upstream, so changing the runtime packages here does not achieve the PR's one-toolchain goal for the binaries copied into the final image. Please migrate this builder to the same 1.26 toolset (while preserving the requiredlibvirt-develheaders) or explicitly exclude this shipped variant from the claim.
build/virt-v2v/Containerfile-upstream-xfs:28 - The xfs variant has the same gap: its shipped Go binaries are compiled by
quay.io/centos/centos:stream9'sgolangpackage in the builder above, not bygo-toolset:1.26. This file is used for thevirt-v2v-xfsimage, so the P3 statement that all shipped builder stages use one Go toolchain is not true for this target. Migrate the builder to the 1.26 toolset or narrow the stated scope.
docs/security/cve-remediation-plan.md:164 - This plan text no longer matches the Containerfiles: the downstream UBI bases were changed to
:latestas well (for example, this branch changes the downstream vsphere populator base), so saying theregistry.redhat.iobases were left pinned misstates the reproducibility and scope of P1. Please update the plan to describe the actual downstream state.
docs/security/cve-remediation-plan.md:215
- The final module directives were deliberately not unified on 1.26: the root module remains
go 1.25.0, while.konflux/validation/go.modisgo 1.26.3. This section currently documents the reverted state and contradicts both the actual go.mod files and the rationale for avoiding Go 1.26's changed GODEBUG defaults.
docs/security/cve-remediation-plan.md:202 - This syntax note is stale: the branch normalizes every
install_weak_depssite to=0, not just two microdnf sites, and the current files no longer retainFalse. Please update the note so future readers do not infer that some remaining sites use the rejected microdnf spelling.
docs/security/cve-remediation-plan.md:225 - The dependency table omits the
golang.org/x/oauth2bump from v0.23.0 to v0.36.0, even though this PR updates it to remediate CVE-2025-22868. Without this row, the security plan does not account for one of the Go findings addressed by the change.
- Files reviewed: 43/459 changed files
- Comments generated: 0 new
- Review effort level: Lite
| dnf update -y && \ | ||
| dnf install -y --setopt=install_weak_deps=0 \ |
| dnf update -y && \ | ||
| dnf install -y --setopt=install_weak_deps=0 \ |
Updating nginx rewrites /var/log/nginx to the package's root:root 0711, discarding the base image's chown to 1001:0, so the container cannot open its logs and crash-loops on startup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (10)
Restore nginx log directory ownership after dnf update · Newgo build ... github.com/open-policy-agent/opabuilds the module root package, which is not the…microdnftreats--setoptas a global/application option; placing it after theinstall… This shippedvirt-v2v-xfsbuild still compiles the Go binaries in the CentOS Stream 9 builder… This shippedvirt-v2vbuild still compiles the Go binaries in the CentOS Stream 9 builder (`FROM… This downstream runtime stage usesubi9-minimal:latestbut doesn’t runmicrodnf update(it’s… This downstream runtime stage is onubi9-minimal:latestbut doesn’t runmicrodnf update(it’s… This downstream runtime stage now usesubi9-minimal:latestbut still skipsmicrodnf update… This downstream runtime stage is now based on a floating tag (ubi9-minimal:latest) but it still… PR description states that registry.redhat.io base images in *-downstream Containerfiles remain…
| RUN dnf -y update && \ | ||
| dnf -y clean all |


Acts on P1–P3 of the remediation plan added in the first commit. Driven by the ISS v2.12.7 scan (
forklift-latest/v2.12.7/results_image_all_2026-08-28::21:29:57:055): 13,548 unique findings, 3,043 crit/high across 8 images.Full analysis and the reasoning behind each change is in
docs/security/cve-remediation-plan.md. Review that first — the code changes are mechanical once the causes are clear.What the scan actually says
74% of the entire finding count (9,963 of 13,548) is the libguestfs supermin appliance kernel inside
forklift-virt-v2v, reported by grype only, matched against upstream kernel.org ranges instead of RHEL errata. That block is deliberately not addressed here — grype inflates every ISS report, so it is being handled as a scanner-level review rather than per-product suppression. Written up separately inimage-scan-service.Strip it out and the addressable set is 3,585 findings / 689 crit/high, of which 473 have a published fix.
Changes
P1 — base images can move again
35 of 40 Containerfiles ran no package update, so the pinned base digest was the permanent patch level. Pins were 3.5–12 months stale.
Every UBI base in the repo is now
:latest— upstream and downstream,registry.access.redhat.comandregistry.redhat.io. No timestamp pin remains. Each:latesttag was verified to resolve before being adopted:ubi9-minimal,ubi9/ubi-minimal,ubi8-minimal,ubi8/ubi,ubi9,ubi10,ubi10/ubi-minimal,ubi9/nginx-126.go-toolsetstays on:1.26, deliberately. That tag already floats across Red Hat rebuilds, so it picks up every toolchain refresh;:latestwould additionally float the Go minor, which breaks the single-toolchain goal and re-risks the GODEBUG behaviour change described under P3.Trade-off, flagged for reviewers: builds are no longer bit-reproducible from the Containerfile alone. This is the right default for a security-driven rebuild with no bot moving digests, but it is a deliberate choice, not an oversight.
P2 — runtime trim, scoped by measurement
--setopt=install_weak_depsis applied only where it measurably changes the image. It was initially added everywhere; that was wrong, and the no-op sites were removed ind6bbe183a. Measured with--assumeno:virt-v2v(CentOS Stream)deep-inspection(CentOS Stream) †ovirt-populatortar(×10)taronlytaronly†
deep-inspectionis not inTIGERA_IMAGES, so this fork neither builds nor pushes it, and it was in neither scan. The measurement is real and is why the flag stays in that file, but it is not an improvement this fork can claim.taron ubi9-minimal pulls in exactly one package — itself. The flag was pure noise on every ubi-minimal image;microdnf -y updatealone is what gets current Red Hat libraries there.Where the flag is kept, two former weak dependencies are installed explicitly, because dropping them would be a functional regression rather than a size win:
passtandlibvirt-daemon-config-network. libguestfs only recommends them, and the image runsLIBGUESTFS_BACKEND=libvirt:qemu:///session, which breaks without them. This is the one place the trim could have caused a regression.nbdkitmetapackage drops, and that is safe.virt-v2vhard-requires every plugin it uses (nbdkit-server,-basic-filters,-basic-plugins,-curl-plugin,-nbd-plugin,-python-plugin,-ssh-plugin,-vddk-plugin); all retained. The metapackage was only a recommendation oflibvirt-daemon-driver-qemu.Package updates were added to every shipped stage that lacked one — including the virt-v2v appliance stages, whose blob is copied into the final image and where upstream had drifted from downstream. Builder stages deliberately get none: only the compiled binary leaves them, and
golangis already current at1.26.7-1.el9_8via the floating tag, so an update there cannot change a shipped image's findings.Note:
microdnfrejectsinstall_weak_deps=Falseand requires=0;dnfandyumaccept both. All 23 sites use=0so the two cannot be confused.P3 — one Go toolchain
Shipped images spanned Go 1.22.5, 1.24.6, 1.25.9 and 1.26.3. All 32 builder stages now use
go-toolset:1.26, so every shipped binary carries the 1.26 stdlib.golang.org/x/cryptogolang.org/x/netgolang.org/x/oauth2golang.org/x/cryptogolang.org/x/netcontainerd/v2golang.org/x/cryptox/oauth2was CVE-2025-22868 (HIGH) and was the only finding trivy reported against the rebuilt virt-v2v image; it now scans clean on that axis.godirectives are not uniform, on purpose. The root module isgo 1.25.0and.konflux/validationisgo 1.26.3(whichcontainerd/v2 v2.3.4requires). Setting the root to 1.26 looked tidier and actively broke things:cmd/vsphere-copy-offload-populatorcarriesreplace github.com/kubev2v/forklift => ../../, so the root directive raised that submodule to 1.26, and the directive selects GODEBUG defaults — Go 1.26's stricternet/urlhost:port parsing then failed nineinternal/vantaratests that pass onmain. The directive is a floor, not the build version, so 1.25.0 at the root still yields a 1.26 stdlib. Reverted ind6bbe183a.That same commit fixes a related break: raising the root's dependencies raises the submodule's effective requirements through that
replace, and its Containerfile runsgo mod downloadbeforemake build, which rewrites itsgo.modand leavesvendor/stale. Resynced.build/validation/Containerfilerewritten. It previously did:An unverified network fetch with no checksum, which also pinned the validation image to whatever Go the OPA project released with — 1.22.5, the oldest toolchain in the product. It now builds OPA from source with the repo's own toolchain, matching what
Containerfile-downstreamalready did. OPA itself stays at v1.8.0; upgrading it changes validation behaviour and belongs in its own PR.forklift-operator — the one image that was getting worse
forklift-operatorwas the only image whose crit/high went up between scans, and the only one carrying stale curl (7.76.1-40.el9against7.76.1-40.el9_8.5everywhere else). All four actionable curl CVEs in the scan were on it alone.Its update step was commented out. Worth recording why uncommenting it as written fails: the base ships
microdnf, notdnf, so the line errored with exit 127. Switched tomicrodnf, verified by querying the rpmdb in the built image:Confirmed in the follow-up scan: CVE-2026-8286, -9547, -1965 and -3783 all went PRESENT → GONE.
Build and push targets
build-all-images-tigeraandpush-all-images-tigerabuild/push the set togcr.io/unique-caldron-775/tigera-forklift, andpush-all-images-quaytoquay.io/tigeradev, so the images can be scanned outside the upstreamquay.io/kubev2vnamespace. They setREGISTRY/REGISTRY_ORGas target-specific variables, which propagate to the existing prerequisite targets, so no target definitions are duplicated.build-all-imagesandpush-all-imagesare unchanged.Both derive from one
TIGERA_IMAGESlist of 15, excluding the operator index: that build runsopm render <bundle>inside the build container, which pulls the bundle from the registry with no credentials available to it. Pushing the bundle first does not help — verified it was pushed before the index build ran and the pull still 403'd, because the GCR repo is private and the in-container pull is anonymous. Upstream avoids this only becausequay.io/kubev2vis public. The index holdsopmplus catalog YAML, so there is nothing in it to scan.The unused
virt-v2vfedora variant is commented out rather than deleted: no Makefile target referenced it and it appeared in no scan, while still needing a Fedora major bump to stay supported.Verification
docker buildofforklift-controller,forklift-api,validation,cli-download,ova-provider-server,populator-controller,vsphere-copy-offload-populator,forklift-operator,virt-v2v(Containerfile-upstream) andovirt-populator(Containerfile-upstream) — all passmake build-all-images-tigera: 15/15, zero errors. All 15 pushed and each manifest verified pullable from GCR and Quayopabinary readsgo1.26.7 (Red Hat 1.26.7-1.el9_8) X:strictfipsruntime, linkingcontainerd/v2 v2.3.4,x/crypto v0.55.0,x/net v0.58.0cmd/vsphere-copy-offload-populatortest suite: 10/10 packages passwebkit2gtk3-jsc,libproxy-webkitgtk4,linux-firmware,nfs-utils,rpcbind,vim-enhancedgone;virt-v2v,passt,libvirt-daemon-config-networkand everynbdkit-*plugin presentmicrodnf/yum--setoptsyntax, and every:latesttag's existence all checked in real containers rather than assumedNot verified locally:
-downstreambuilds needrpms.lock.yamlregeneration with entitled reposContainerfile-downstream-fssupportneeds Red Hat internal Brew accessvirt-v2v/ ubi8ovirt-populatorvariants need a Red Hat subscription;virt-v2vis not in the UBI repos at all, which is why the CentOS StreamContainerfile-upstreamvariants exist and are what our targets buildWhich files this fork actually builds
38 Containerfiles change here. 14 of them are built by this fork. The
build-all-images-tigera/push-all-images-tigera/push-all-images-quayrules fan outover
TIGERA_IMAGES, and each passes-f build/<name>/ContainerfileorContainerfile-upstream*.downstreamappears nowhere in the Makefile's image rules.The other 24 are edited deliberately, for consistency with upstream rather than for effect
here — 17
Containerfile-downstreamfiles (consumed only by the.tektonPipelineRuns,and Konflux does not run on this fork), the three entitlement-gated variants where the fork
builds the
-upstreamflavour instead,deep-inspection/*(not inTIGERA_IMAGES), and thefedora variant this branch comments out entirely.
Letting the two lineages diverge would make every future rebase against upstream harder and
leave a reader unable to tell which file is authoritative. The trade-off is a diff larger
than the effective change, so: nothing in the 24 should be read as an image this fork
hardened.
Consequence for the numbers: no CVE figure in this PR or in
docs/security/cve-remediation-plan.mdis credited to a-downstream,deep-inspectionorentitlement-gated image. Every results table covers images in
TIGERA_IMAGES, which is alsoexactly the set that was scanned.
Deliberately out of scope
image-scan-service.5.14.0-741.el9is already the newest CentOS Stream 9 build, so this is a matching problem, not patch lag.konflux/virt-v2v/runtime/rpms.in.yaml— not done. It only affects the hermetic Konflux build, which runs in Red Hat's tenant and cannot be exercised or verified here. Reverted in56aaad8d8along with the other Konflux and CI-workflow changes, which are dormant in this forkmust-gatherandconsole-pluginare consumed prebuilt fromquay.io/kubev2vwith no Containerfile here.must-gatheris the largest remaining actionable block at 299 crit/high, 106 of themwebkit2gtk3. Pending a decision on whether it is used in the L2 migration flowSpotted, not changed
build/virt-v2v/Containerfile-downstreamandContainerfile-downstream-fssupportinstall RPMs with--setopt=sslverify=false. Limited exposure in a hermetic build, but a bad default to carry. Left alone because verifying the change needs a hermetic build.--setopt=install_weak_depsonContainerfile-downstream-fssupportwas reverted: that transaction previously ran with weak deps on, so disabling them droppedpasst, and UBI10 will not run under emulation here to validate a forward fix.Measured effect
Like-for-like on the six images present in both the 08-28 and 09-03 scans:
forklift-controllerforklift-validationpopulator-controllerforklift-operatorovirt-populatorforklift-virt-v2vThe Go-service images dropped 71–78%.
virt-v2vis flat because 94% of its findings are the deferred appliance-kernel block.Read per-image numbers, not the total: the later scan covers 15 images rather than 8, and
virt-v2v-xfsalone contributes another 10,694 findings identical tovirt-v2v. The residual is dominated by three classes no rebuild can move — the appliance kernel (~10k, grype/NVD mismatch), 338 curl entries marked "No fix available", and 53 twistcli findings (31 HIGH) where a Red Hat Satellite advisory's CVE list is cross-joined ontolibcomps.