Skip to content

Better oci refs#2204

Merged
dgageot merged 2 commits intodocker:mainfrom
dgageot:better-oci-refs
Mar 21, 2026
Merged

Better oci refs#2204
dgageot merged 2 commits intodocker:mainfrom
dgageot:better-oci-refs

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Mar 21, 2026

  • Faster fetch artefacts with digits
  • Better cache key

dgageot added 2 commits March 21, 2026 17:44
ociSource.Read() was using the raw reference string to look up artifacts
in the content store, while remote.Pull() stores them under a normalized
key (e.g. 'agentcatalog/review-pr:latest'). This caused cache misses
when using fully qualified references like
'index.docker.io/agentcatalog/review-pr:latest'.

Extract NormalizeReference() from remote.Pull() and use it in
ociSource.Read() so equivalent references resolve to the same store key.

Assisted-By: docker-agent
When an OCI reference includes a digest (e.g. repo@sha256:abc...),
the content is immutable. If the artifact is already in the local
content store, serve it directly without any network call to the
registry, avoiding the ~500ms crane.Digest() round-trip.

Also fix content store resolution to handle digest references of
the form 'repo@sha256:...' by extracting the digest portion, so
artifacts stored under a tag ref can be retrieved by digest.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner March 21, 2026 17:07
Copy link
Copy Markdown

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🔴 CRITICAL

Found 1 critical issue in the changed code.

// For digest references, the content is immutable. If we already have
// the artifact locally, serve it directly without any network call.
if remote.IsDigestReference(a.reference) {
if data, loadErr := loadArtifact(store, storeKey); loadErr == nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY: Digest-pinned cache bypass prevents corruption detection

When a digest-pinned reference is found in cache (lines 133-137), the function returns early with that cached data without any corruption checking. The corruption detection logic at lines 156-158 only runs after attempting a Pull, but digest references skip the Pull when cached.

Impact: If a digest-pinned artifact in cache becomes corrupted, it will be served indefinitely because:

  1. The early return skips the Pull call that would update the cache
  2. The corruption check only runs in the normal flow, which digest refs bypass

This is a cache poisoning vulnerability for digest-pinned references. The store's GetArtifact can return ErrStoreCorrupted at multiple stages (missing tar, invalid layers, decompression failure, I/O errors), but the digest fast-path bypasses this detection.

Recommendation: Check for corruption before the early return, or ensure loadArtifact errors (including corruption) are handled in the digest-pinned path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

untrue

@dgageot dgageot merged commit e1d1873 into docker:main Mar 21, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants