Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

c8d: Refuse images with digest algo when tagging#46492

Merged
thaJeztah merged 1 commit into
moby:masterfrom
rumpl:c8d-tag-digest-name
Sep 17, 2023
Merged

c8d: Refuse images with digest algo when tagging#46492
thaJeztah merged 1 commit into
moby:masterfrom
rumpl:c8d-tag-digest-name

Conversation

@rumpl

@rumpl rumpl commented Sep 15, 2023

Copy link
Copy Markdown
Member

- What I did

Checked if the tag resembles a digest algorithm name before tagging and returning an error.

- How I did it

- How to verify it

Run

$ make TEST_FILTER='TestTagUsingDigestAlgorithmAsName' TEST_IGNORE_CGROUP_CHECK=1 DOCKERCLI_INTEGRATION_VERSION=v24.0.5  DOCKER_GRAPHDRIVER=overlayfs TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@rumpl rumpl added status/2-code-review area/images Image Service kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Sep 15, 2023
@rumpl rumpl added this to the 25.0.0 milestone Sep 15, 2023
Comment on lines +419 to +422
refName := reference.FamiliarName(ref)
if refName == string(digest.Canonical) {
return errdefs.InvalidParameter(errors.New("refusing to create an ambiguous tag using digest algorithm as name"))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change looks good (and probably similar to what we did/do in the non-containerd version?).

We should look at follow-ups though, as ideally we'd take other algorithms into account as well, as they may occur as well.

Let me write it down here, but I'll open a separate ticket from it;

The spec defines a couple of algorithms that are registered in the spec (sha256, sha512);
https://github.com/opencontainers/image-spec/blob/v1.1.0-rc5/descriptor.md#registered-algorithms, so those are good candidates to always ignore (as the spec defines that the can be used).

It also describes some other (more esoteric) ones as "optional" https://github.com/opencontainers/image-spec/blob/v1.1.0-rc5/descriptor.md#digests

And potentially other algorithms (sha384, and blake3 variants) as well;

So, I think we may need some follow-ups;

  • Perhaps go-digest could have a utility function that returns the list of registered algorithms (assuming that's the canonical list of algorithms that we accept: https://github.com/opencontainers/go-digest/blob/316f76db9b0f1b3620c3380cc34f75d262dea6db/algorithm.go#L78-L96)
  • A similar utility could/should be added to https://github.com/distribution/reference. Baring an OCI specification for image-references, I think we should consider that to be the canonical implementation for image references
  • So it should at least have a utility to check for ambiguous references
  • And possibly invalidate such references automatically
  • for any "image reference spec", it would be hard to define a "dynamic" list though. and unfortunately, the go-digest regex is overly broad, so can't be used to detect whether we're dealing with a (possible) algorithm, or an image name (it looks like some context on that RegEx was removed in opencontainers/go-digest@b9e02e0 🤔)
  • So maybe such a spec must define a minimum set of algorithms to consider "reserved" (MUST be rejected), and possibly a recommendation on what patterns to consider an algorithm (which MAY be rejected).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit a1833d8 into moby:master Sep 17, 2023
@thaJeztah thaJeztah deleted the c8d-tag-digest-name branch September 17, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/images Image Service containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

2 participants