Support tag-based refs in ec.oci.blob#3214
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated OCI artifact blob retrieval to try digest parsing first; if that fails and the ref ends with Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant OCI as OCI Client
participant Reg as Registry
participant Img as Image
participant Lyr as LayerStore
Client->>OCI: Request blob for refStr
OCI->>Reg: Attempt parse refStr as digest
alt digest parsed
OCI->>Reg: Fetch layer by digest
Reg-->>OCI: Layer blob
OCI->>Lyr: Compute digest (expectedDigest)
OCI-->>Client: Return blob + expectedDigest
else digest parse fails and refStr ends with .sbom/.att/.sig
OCI->>Reg: Resolve refStr as tag -> Image
Reg-->>Img: Image reference
OCI->>Img: Image.Layers()
Img-->>OCI: [layer1, ...]
OCI->>Lyr: Compute digest of layer1 (expectedDigest)
OCI-->>Client: Return layer1 blob + expectedDigest
else other/unsupported ref
OCI-->>Client: Return nil (no OPA error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/rego/oci/oci.go (1)
503-506:⚠️ Potential issue | 🟠 MajorDon't persist blob cache entries under mutable tag refs.
Lines 503-506 and 641-642 still key the cache by
refStr. That was safe for digest refs, but.sbom/.att/.sigtags are mutable, so a repointed tag can keep serving stale blob data here. The same problem now spills intoec.oci.blob_files, since its cache key is also built from the original tag string.Possible fix
- if cached, found := cc.blobCache.Load(refStr); found { - logger.Debug("Blob served from cache") - return cached.(*ast.Term), nil - } + cacheable := false + if _, err := name.NewDigest(refStr); err == nil { + cacheable = true + } + if cacheable { + if cached, found := cc.blobCache.Load(refStr); found { + logger.Debug("Blob served from cache") + return cached.(*ast.Term), nil + } + } … - cc.blobCache.Store(refStr, term) + if cacheable { + cc.blobCache.Store(refStr, term) + }If you want cache reuse for legacy tags, normalize the key to an immutable digest first instead of using the tag string.
</details> Also applies to: 522-582, 641-642 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@internal/rego/oci/oci.goaround lines 503 - 506, The blob cache is currently
keyed by the incoming refStr (used in cc.blobCache.Load and where
ec.oci.blob_files builds its key), which allows mutable tag refs like
.sbom/.att/.sig to serve stale blobs after tags are repointed; change the cache
key to the immutable digest instead: resolve the incoming refStr to its manifest
digest up-front (fail-safe: if you cannot resolve to a digest, do not cache),
then use that resolved digest string as the key for cc.blobCache.Store/Load and
for ec.oci.blob_files lookups; ensure any existing code paths that build keys
from refStr are updated to use the resolved digest (and skip caching when digest
resolution fails).</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (1)
internal/rego/oci/oci_test.go (1)
107-154: Add a negative case for non-cosign*.sbom/.att/.sigtags.The new matrix only proves
:latestis rejected. It won't catch a broader tag likerepo:release.sbom, which is the edge case most likely to regress the fallback gate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/rego/oci/oci_test.go` around lines 107 - 154, Add negative test cases to the existing table in oci_test.go that cover non-cosign tags ending with .sbom/.att/.sig (e.g., use uri ast.StringTerm("registry.local/spam:release.sbom"), "registry.local/spam:release.att", "registry.local/spam:release.sig") similar to the existing "non-cosign tag reference returns nil without image fetch" entry: set data to `{"spam":"maps"}` and err: true so the test asserts no image fetch/fallback occurs for these suffixes; add the entries alongside the other cases in the testCases slice so functions that inspect uri (the code paths referenced by the test table) will catch regressions.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@internal/rego/oci/oci.go`: - Around line 524-535: The current fallback treats any ref ending in .sbom/.att/.sig as a legacy cosign tag and parses it as an image, which incorrectly accepts arbitrary tags like repo:release.sbom; update the branch around refStr and name.ParseReference to first validate the tag shape matches the legacy digest-derived cosign pattern used by ec.oci.image_tag_refs (e.g., the tag suffix plus the expected digest-derived prefix/hex format) before calling name.ParseReference and extracting the first layer; if the tag does not match that stricter pattern, keep the current nil return path to avoid expanding ec.oci.blob to ordinary tags. --- Outside diff comments: In `@internal/rego/oci/oci.go`: - Around line 503-506: The blob cache is currently keyed by the incoming refStr (used in cc.blobCache.Load and where ec.oci.blob_files builds its key), which allows mutable tag refs like .sbom/.att/.sig to serve stale blobs after tags are repointed; change the cache key to the immutable digest instead: resolve the incoming refStr to its manifest digest up-front (fail-safe: if you cannot resolve to a digest, do not cache), then use that resolved digest string as the key for cc.blobCache.Store/Load and for ec.oci.blob_files lookups; ensure any existing code paths that build keys from refStr are updated to use the resolved digest (and skip caching when digest resolution fails). --- Nitpick comments: In `@internal/rego/oci/oci_test.go`: - Around line 107-154: Add negative test cases to the existing table in oci_test.go that cover non-cosign tags ending with .sbom/.att/.sig (e.g., use uri ast.StringTerm("registry.local/spam:release.sbom"), "registry.local/spam:release.att", "registry.local/spam:release.sig") similar to the existing "non-cosign tag reference returns nil without image fetch" entry: set data to `{"spam":"maps"}` and err: true so the test asserts no image fetch/fallback occurs for these suffixes; add the entries alongside the other cases in the testCases slice so functions that inspect uri (the code paths referenced by the test table) will catch regressions.🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID:
33b242e0-7fbd-44a0-b07c-6230246c6061📒 Files selected for processing (2)
internal/rego/oci/oci.gointernal/rego/oci/oci_test.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The ec.oci.blob builtin only accepted digest refs (repo@sha256:...) via name.NewDigest(). Tag refs returned by ec.oci.image_tag_refs (e.g. repo:sha256-abc.sbom) were rejected, causing errors during legacy cosign SBOM discovery. Add a fallback for cosign tag suffixes (.sbom, .att, .sig) that resolves the tag to an image and extracts the first layer. Preserve the original digest-based verification for digest refs and use layer-reported digest for tag refs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
You can't tag a blob. If you want to get to a blob that is inside an Image Manifest which you only have a tag reference to, you can use
|
|
Should we perhaps create an |
A rego helper function sounds like a good idea! |
3 participants
The ec.oci.blob builtin only accepted digest refs (
repo@sha256:...) vianame.NewDigest(). Tag refs returned byec.oci.image_tag_refs(e.g.repo:sha256-abc.sbom) were rejected, causing errors during legacy cosign SBOM discovery.Add a fallback for cosign tag suffixes (.sbom, .att, .sig) that resolves the tag to an image and extracts the first layer.
Preserve the original digest-based verification for digest refs and use layer-reported digest for tag refs.