fix(ci): scan platform manifests, not the index, in the image gate (#2266) - #2272
Conversation
Gregory91G
added
github_actions
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe image CVE gate now resolves pushed image indexes into platform manifests, skips unknown-platform and attestation manifests, scans each valid digest with Trivy, falls back to index scanning when needed, and fails when no manifest is scanned. ChangesImage CVE gate
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DigestList
participant ImageTools
participant ManifestFilter
participant Trivy
DigestList->>ImageTools: Inspect image digest
ImageTools->>ManifestFilter: Return manifest metadata
ManifestFilter->>Trivy: Scan each valid platform digest
Trivy-->>ManifestFilter: Return scan result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/image-cve-gate/action.yml:
- Around line 51-58: Update the jq selection in the targets assignment to
require a non-null platform object before excluding unknown operating systems,
and distinguish plain manifests from indexes when choosing the fallback. Use the
original digest only when the inspected document is a plain manifest; leave
targets empty for indexes with no eligible children so the existing scanned == 0
failure path runs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 526c6459-3908-4978-9979-49324011dd5c
📥 Commits
Reviewing files that changed from the base of the PR and between 27cdfbb and 51901c613cfc9639921dc342bf4326e58e26b7d2.
📒 Files selected for processing (1)
.github/actions/image-cve-gate/action.yml
| targets="$(docker buildx imagetools inspect --raw "${IMAGE}@sha256:$(basename "$f")" | jq -r ' | ||
| if .manifests then | ||
| .manifests[] | ||
| | select(.platform.os != "unknown") | ||
| | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | ||
| | .digest | ||
| else empty end')" | ||
| [ -n "$targets" ] || targets="sha256:$(basename "$f")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Distinguish a plain manifest from an index with no eligible children.
Line 54 includes entries with no declared platform because null != "unknown" is true in jq. Line 58 then scans an index when all its children were excluded. This bypasses the scanned == 0 failure path and can reintroduce the Trivy index-resolution failure.
Only use the original digest when the inspected document is a plain manifest. For an index, require a platform object and leave targets empty when no eligible child exists.
Proposed fix
- targets="$(docker buildx imagetools inspect --raw "${IMAGE}`@sha256`:$(basename "$f")" | jq -r '
- if .manifests then
+ raw_manifest="$(docker buildx imagetools inspect --raw "${IMAGE}`@sha256`:$(basename "$f")")"
+ if jq -e '.manifests | type == "array"' >/dev/null <<<"$raw_manifest"; then
+ targets="$(jq -r '
.manifests[]
- | select(.platform.os != "unknown")
+ | select((.platform? | type) == "object")
+ | select(.platform.os? != null and .platform.os != "unknown")
| select(.annotations["vnd.docker.reference.type"] != "attestation-manifest")
- | .digest
- else empty end')"
- [ -n "$targets" ] || targets="sha256:$(basename "$f")"
+ | .digest' <<<"$raw_manifest")"
+ else
+ targets="sha256:$(basename "$f")"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| targets="$(docker buildx imagetools inspect --raw "${IMAGE}@sha256:$(basename "$f")" | jq -r ' | |
| if .manifests then | |
| .manifests[] | |
| | select(.platform.os != "unknown") | |
| | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | |
| | .digest | |
| else empty end')" | |
| [ -n "$targets" ] || targets="sha256:$(basename "$f")" | |
| raw_manifest="$(docker buildx imagetools inspect --raw "${IMAGE}`@sha256`:$(basename "$f")")" | |
| if jq -e '.manifests | type == "array"' >/dev/null <<<"$raw_manifest"; then | |
| targets="$(jq -r ' | |
| .manifests[] | |
| | select((.platform? | type) == "object") | |
| | select(.platform.os? != null and .platform.os != "unknown") | |
| | select(.annotations["vnd.docker.reference.type"] != "attestation-manifest") | |
| | .digest' <<<"$raw_manifest")" | |
| else | |
| targets="sha256:$(basename "$f")" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/image-cve-gate/action.yml around lines 51 - 58, Update the
jq selection in the targets assignment to require a non-null platform object
before excluding unknown operating systems, and distinguish plain manifests from
indexes when choosing the fallback. Use the original digest only when the
inspected document is a plain manifest; leave targets empty for indexes with no
eligible children so the existing scanned == 0 failure path runs.
…2266) A push-by-digest build publishes one index per platform, each holding the platform manifest plus an attestation child. The gate handed those index digests to trivy, which resolves an index against the runner's own platform and fatals on the arm64 one -- "no child with platform linux/amd64 in index". The amd64 index scanned clean in the same job, so an attestation child is not what trivy chokes on; the cross-architecture resolution is. The gate now expands each index and scans the platform manifests by their own digest, which needs no --platform and drops the attestation children on the way. Digests that are already a plain manifest are scanned as they are, so a build without provenance still works. A counter fails the step if the expansion ever yields nothing, keeping the "a gate that scanned nothing is not a pass" property the empty-directory check already gives. Verified against the digests from the failing run: both now scan (debian 12.15, 91 packages each) and exit 0. A multi-arch index yields two scans with the attestation children skipped, findings still exit 1, and a plain manifest digest takes the fallback. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
No behaviour change. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
2f7a6c3 to
4dfc325
Compare
2 participants
Fixes #2266 and supersedes #2267 — with this in, the gate no longer needs to be non-blocking.
What actually breaks
Not attestation manifests. I pulled both digests from the failing job and looked them up in the registry:
Export digestwritessteps.build.outputs.digest, and with provenance on, buildx returns the digest of an index, not of a manifest. There is one such index per architecture, and each already contains an attestation child.Trivy runs on an amd64 runner and resolves an index against the host platform. The amd64 index has the child it wants and scanned clean — that is the "first digest scanned clean" in the issue. The arm64 index has no
linux/amd64child, hence the fatal. The attestation child sits in both indexes, including the one that scanned fine, so it is not what trivy chokes on.The fix
Expand each index and scan the platform manifests by their own digest. No
--platformto derive, and the attestation children fall out of the same filter:Both predicates are kept deliberately. On our images they are equivalent, but they fail differently: the platform one covers an attestation child that ever declares a real platform, the annotation one covers a platform child that ever loses its platform. Media type is not usable as a discriminator here — the image child and the attestation child are both
application/vnd.oci.image.manifest.v1+json.A digest that is already a plain manifest falls back to being scanned as-is, so a build with provenance disabled keeps working. A
scannedcounter fails the step if expansion yields nothing, extending the existing "a gate that scanned nothing is not a pass" guard from the empty-directory case to the empty-expansion one.Test plan
Ran the action's script verbatim, against the live registry.
52b63ee7…,818f534b…): both scan, debian 12.15 detected, 91 packages each, exit 0. Previously the second one fatalled.insight-front:latest, 2 platform + 2 attestation children): 2 scans, 2 OS detections, attestation children skipped.alpine:3.10index atHIGH,CRITICAL→ exit 1..manifests→ scanned as-is, exit 1.insight-toolbox(2 platforms, 10 fixable HIGH after fix(images): bump the toolbox to Node 24 and upgrade npm in place (#2166) #2168):CRITICAL→ exit 0 on both,HIGH→ exit 1 on both.non-blockingtext survives and--exit-code 1is back.actionlint: 7 findings on the base, 7 on the branch, identical set.mainbuild after merge tags images again.Cost note
Scanning
insight-toolboxpulls Trivy's Java DB (~1.4 GB) on top of the vulnerability DB, because the image carries Java artifacts and the Java analyzer runs even under--scanners vuln. Each merge job is a fresh runner, somerge-toolboxpays that download every build — roughly a minute beyond the 30–45 s estimated in #2238. The other six jobs are unaffected.Sequencing
#2267 makes the gate non-blocking as a stop-gap and is in the merge queue. If it lands first I will rebase and drop its
|| echo ::warning::line; if this lands first, #2267 can be closed unmerged. Either order works, they just should not both end up onmain— the enforcement has to come back on.Summary by CodeRabbit