Reject a tar manifest whose descriptor carries no refs - #791
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
DescribedImage.Ref and DescribedImageIndex.Ref both return Refs[0] with no length check. Refs comes straight from the manifest.json inside a tar, so reading a bundle tar whose descriptor has an empty or absent Refs panics with index out of range [0] with length 0 Neither Ref has an error to return, and every descriptor this package writes carries exactly one ref, so check for it while parsing the manifest instead and report which descriptor is at fault. Signed-off-by: Arpit Jain <arpitjain099@gmail.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
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.
DescribedImage.RefandDescribedImageIndex.Refboth do this:Refsis unmarshalled from themanifest.jsoninside a tar (TarReader.getIdsFromManifest->NewImageRefDescriptorsFromBytes), so it is whatever the file says. A descriptor with"Refs": [], or with the field absent, panics as soon as anything reads the ref:TarReader.Readalone does not trigger it, butPresentLayersand the copy path both callRef()on every item, soimgpkg copy --taron a truncated or hand-edited bundle crashes rather than reporting a bad tar. Someone consuming an air-gapped bundle they did not build is exactly the case where the file cannot be assumed well-formed.Neither
Ref()has an error to return, and changing them to hand back an empty string only moves the failure toregname.NewDigest("")with a worse message. Since every descriptor written bybuildImage/buildImageIndexcarries exactly one ref, this checks the invariant while parsing the manifest and names the descriptor that is missing it:Images and indexes nested inside an index are walked too, since
buildIndexcallsNewDescribedImageon those the same way.Three cases added in
pkg/imgpkg/imagedesc. Before the change, parsing a descriptor with no refs succeeds andRef()panics; after it, parsing fails with the message above.go test ./pkg/imgpkg/...passes apart fromTestLabelsinpkg/imgpkg/cmd, which also fails on a clean checkout here becauseIMGPKG_E2E_IMAGEandIMGPKG_E2E_RELOCATION_REPOare not set.