AST-165915 - linux/amd64 and silently reports scan success - #1573
Open
cx-dmitri-rivin wants to merge 4 commits into
Open
cx-dmitri-rivin wants to merge 4 commits into
cx-dmitri-rivin wants to merge 4 commits into
Conversation
…resolved
containers-resolver records an image it could not analyze as a "Failed" entry in
containers-resolution.json and still returns nil from Resolve. runContainerResolver
only checked that return value, so the CLI compressed and uploaded a resolution
file carrying no package data, the scan was marked Completed with 0 findings, and
the pipeline exited 0 - byte for byte indistinguishable from a genuinely clean
scan. A GitLab job with no human reading the log passed its security gate on an
image that was never scanned.
This is the second defect in AST-165915 and it is not specific to the arm64
platform mismatch that motivated the ticket: any resolution failure behaves the
same way. Reproduced with a public image and a bad tag, no arm64 and no private
registry involved:
cx scan create ... --container-images debian:non-existent-tag-999
-> status Completed, findings 0, exit 0
reportUnresolvedContainerImages now reads the resolution file after a successful
resolve and reports every "Failed" entry. Images named explicitly through
--container-images return an error, because the pipeline asked for them by name
and not scanning one has to fail. Images merely discovered inside the scanned
sources only warn, preserving the warn-rather-than-fail behaviour AST-146648
deliberately chose for private images the CLI cannot reach.
The resolution payload is parsed through a small local struct rather than by
importing containers-syft-packages-extractor, so this adds no dependency.
Note the arm64 half of the ticket is fixed in the libraries and still needs the
containers-resolver bump, which is pending that repo's release.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eclaring them The unresolved-image check declared its own "UserInput" constant and an inline struct for the image locations. Both already exist in containers-types, which is already a direct dependency of the CLI and is on the depguard allowlist in .golangci.yml, so redeclaring them was duplication with a real risk of drifting from the producer. Only the ContainerResolution/ContainerImage envelope is still restated locally. That type lives in containers-syft-packages-extractor, which the CLI depends on only indirectly and which is not on the allowlist - importing it would make the CLI a direct consumer of that module. The same split exists upstream, where the extractor declares its own ImageLocation rather than using the one in containers-types; moving the resolution payload into containers-types would remove both copies, but that is a cross-repo change beyond this fix. go.mod and go.sum are unchanged: nothing new was added, the CLI just stopped re-declaring what it already had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unresolved-image check declared a local struct mirroring containers-resolution.json. That was a copy of a format this repo does not own, and it had already drifted once during development: the "status" key is lower-case while every neighbouring key is not, which a hand-written mirror only gets right by luck. ContainerResolution/ContainerImage now come from containers-syft-packages-extractor - the package that writes the file - so the CLI cannot disagree with the producer about its own format. The extractor exports no origin constant, so types.UserInput from containers-types is still what classifies an image as explicitly requested. This is not a new dependency. The module is already in the graph and already linked into the binary through containers-resolver; the only change is that go.mod stops marking it indirect. go.sum is untouched and the binary is unaffected. Added to the depguard allowlist next to containers-images-extractor, which is imported the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sion tests
The bump activates the platform half of the ticket: containers-resolver v1.0.37
calls AnalyzeImages instead of AnalyzeImagesWithPlatform("linux/amd64"), and
containers-syft-packages-extractor v1.0.27 no longer defaults an empty platform
specifier to linux/amd64. A locally built single-architecture linux/arm64 image
now resolves instead of failing, verified against the released tag on an arm64
Docker host: status "Resolved", 14 packages.
No new modules enter the graph. syft stays at v1.21.0 and stereoscope at v0.1.0,
so none of the aws-sdk-go-v2 or cloud.google.com/go trees that reviewers objected
to come back. Stripped binary size is unchanged: 80,447,458 -> 80,447,810 bytes,
a difference of 352 bytes. zerolog moves v1.34.0 -> v1.35.1 because v1.0.37
requires it; that is the only transitive change.
scan_ast165915_test.go drives the real runContainerResolver entry point rather
than the helper, so it covers the path the ticket describes end to end. The
resolution payloads are captured verbatim from live containers-resolver runs
against a real Docker daemon, including the lower-case "status" key that a
hand-written fixture gets right only by luck. Coverage:
- a resolved arm64 image lets the scan proceed, image reaching the resolver
with its docker: prefix intact
- the customer's platform mismatch stops the scan instead of completing with
0 findings and exit 0
- case 00286204's generic reproduction, a public image with a bad tag, fails
the same way - the fix is not arm64-specific
- a Failed entry never coexists with a nil error, which was the actual defect
- an image only discovered in the scanned sources still merely warns, per
AST-146648
Confirmed these fail without the fix: disabling the check makes all three
defect-2 tests fail, and re-enabling it makes them pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
By submitting this pull request, you agree to the terms within the Checkmarx Code of Conduct. Please review the contributing guidelines for guidance on creating high-quality pull requests.
Description
Please provide a summary of the changes and the related issue. Include relevant motivation and context.
Type of Change
Related Issues
Link any related issues or tickets.
Checklist
Screenshots (if applicable)
Add screenshots to help explain your changes.
Additional Notes
Add any other relevant information.