Incorrectly parsed image references if they contain both a tag and digest

[chgl]

Describe the bug

I use image references in my manifests that contain both a tag and a sha256 digest, e.g.: example.com/library/image:v1.2.3@sha256:abcd.... This causes the admission webhook to fail with:

Error: INSTALLATION FAILED: admission webhook "connaisseur-svc.connaisseur.svc" denied the request: Unexpected Cosign exception for image "example.com/library/image:v1.2.3@sha256:1d076650cbf2f364235e49203a98015d35443d72e74f462085ea3bcedd25cb4c:v2.15.5": Error: parsing reference: could not parse reference: example.com/library/image:v1.2.3@sha256:1d076650cbf2f364235e49203a98015d35443d72e74f462085ea3bcedd25cb4c:v2.15.5
main.go:62: error during command execution: parsing reference: could not parse reference: example.com/library/image:v1.2.3@sha256:1d076650cbf2f364235e49203a98015d35443d72e74f462085ea3bcedd25cb4c:v2.15.5

So it appears that these images are incorrectly parsed. I believe this is caused in https://github.com/sse-secure-systems/connaisseur/blob/v2.6.2/connaisseur/image.py#L30-L65 as adding the following new test data to https://github.com/sse-secure-systems/connaisseur/blob/v2.6.2/tests/test_image.py#L115 causes the tests to fail:

        (
            "docker.io/library/image:v1.2.3@sha256:f8816ada742348e1adfcec5c2a180b675bf6e4a294e0feb68bd70179451e1242",
            "image",
            "v1.2.3",
            "f8816ada742348e1adfcec5c2a180b675bf6e4a294e0feb68bd70179451e1242",
            "library",
            "docker.io",
            fix.no_exc(),
        ),

https://github.com/sse-secure-systems/connaisseur/blob/v2.6.2/connaisseur/image.py#L84 has to be adapted to either return the full tag+digest or just the digest as it is passed to cosign verify later on (https://github.com/sse-secure-systems/connaisseur/blob/v2.6.2/connaisseur/validators/cosign/cosign_validator.py#L304).

Cosign itself does support both tags+digest when verifying, but drops the tag if both a tag+digest are specified e.g.

$ COSIGN_EXPERIMENTAL=1 cosign verify ghcr.io/chgl/fhir-server-exporter:v2.0.2@sha256:0d42bd5680994b2f3e376ef1159ec89ede6b1d59892fcd0f8079672304f99f7f

Verification for ghcr.io/chgl/fhir-server-exporter@sha256:0d42bd5680994b2f3e376ef1159ec89ede6b1d59892fcd0f8079672304f99f7f --
...

Expected behavior

It should be possible to verify image references with both a tag and a digest specified. If both are present, from an implementation point of view it's probably easiest to drop the tag and just use the digest when passing the image reference.

Optional: To reproduce

1. Install connaisseur
2. try deploying a manifest containing an image reference with both a tag and digest
3. fails with the above error message as the image being passed to cosign is incorrectly constructed

Optional: Versions (please complete the following information as relevant):

• OS:
• Kubernetes Cluster:
• Notary Server:
• Container registry:
• Connaisseur:
• Other:

Optional: Additional context

As a tiny side note, in https://github.com/sse-secure-systems/connaisseur/blob/v2.6.2/connaisseur/image.py#L84 hard-coding the digest to sha256 could possibly be an issue some time in the future as technically various types of digests are possible, see https://github.com/opencontainers/image-spec/blob/main/descriptor.md#digests - although admittedly I've never seen anything but sha256 in the wild.

[peterthomassen]

Thanks for reporting! Before going further, I have a few questions to understand the situation better.

I use image references in my manifests that contain both a tag and a sha256 digest, e.g.: example.com/library/image:v1.2.3@sha256:abcd....

Can you describe your use case for this?

It seems to me that this is a contradictory way of identifying things, much like referencing a file system object by both filename and inode number. What if the inode number doesn't belong to that filename? (What if the digest does not correspond to the tag?)

Expected behavior

It should be possible to verify image references with both a tag and a digest specified. If both are present, from an implementation point of view it's probably easiest to drop the tag and just use the digest when passing the image reference.

Why should that be possible? Do you have a reference that specifies that this is a proper way of referencing images? The OCI image-spec repo (that you linked w.r.t. digest type flexibility) does not seem to contain such a provision.

[chgl]

@peterthomassen thank you for your response! I agree with your concerns, especially since I could just be using digests and wouldn't have this problem in the first place.

There is some discussion and further issues e.g. at containers/buildah#1407

My use case for both is using renovate to automatically update my image references, while still keeping semantic information about the version update - ie. Being able to separate major updates from simple patches and creating separate PRs. So I am able to see human readable image versions (tags) while also profiting from immutability (digest). Here's one such PR created by renovate: https://github.com/chgl/charts/pull/236/files which updates the tag+corresponding digest. Here's another one just updating the digest, suggesting that the image doesn't use immutable tags: https://github.com/chgl/magniFHIR/pull/15/files

[peterthomassen]

The discussion you linked makes the point that two different behaviors may be plausible. I see that there are some implementations that lean on the side you're describing as expected behavior, but I don't see that community consensus has been established on this matter (or at least, if it has, then it has not been recorded).

Given that, it's possible that consensus/specification will end up with the other interpretation, or with a "don't do this -- use <something else> for annotations" outcome. In that case, if we do an implementation now, we'd be stuck, and would have to do a breaking change to be compliant. And compliance is important for interoperability.

IMO, the right way to approach this is to first establish community consensus / specification (have you tried approaching the OCI image-spec people?), and then implement whatever is agreed upon. If specification requires some tentative implementations, I can imagine adding this with an experimental: true flag or something.

What do you think, @phbelitz @Starkteetje @xopham ?

[chgl]

I appreciate the concern as it made me dive a bit deeper into the topic:

The distribution spec suggests that either a tag OR a digest should be used when interacting with a registry to pull artifacts, see https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests. However, the distribution toolkit implementation grammar allows both tags and images to be used at the same time: https://github.com/distribution/distribution/blob/main/reference/reference.go#L6 (via https://stackoverflow.com/questions/53848875/using-container-image-tags-together-with-digests).

There is also opencontainers/distribution-spec#320 which discusses the use of enforcing immutable tags which partially resulted in a new proposal for a working group to specify OCI references (opencontainers/tob#114).

I don't believe we would risk implementing something that is not backwards-compatible as the "bug" is just a slightly incorrect image reference parsing which causes an invalid reference to be passed to the signature validators. For example, if the original image reference used in a manifest was example.com/library/image:v1.2.3@sha256:1d076650cbf2f364235e49203a98015d35443d72e74f462085ea3bcedd25cb4c, the code in image.py reconstructs it in its __str__ method to example.com/library/image:v1.2.3@sha256:1d076650cbf2f364235e49203a98015d35443d72e74f462085ea3bcedd25cb4c:v1.2.3 - note the v1.2.3 being incorrectly appended to the end of the digest.

Maybe my initial suggestion to drop the tag and just use the digest if both are specified (as cri-o and others do internally, see https://github.com/cri-o/cri-o/pull/3060/files#diff-73f26dcabd7e433970fc0612ff0068d2d8f7cd61dda790bcd603195197aaad97R480-R482) wasn't such a good idea and instead the image reference (with tag+digest) should simply be passed to the validators as-is so one doesn't end up changing or re-interpreting anything specified in the original manifests.

[xopham]

An interesting situation and surprising that it exists 😅 Without having delved into the details, it seems to me that we have no standard to follow here and one is generally free to do as one likes. That makes me a bit hesitant to be creative, as it is hard to know what people would expect to happen.
If we were to support this, there is essentially 2 questions to answer if tag+digest are provided:

1. how to read it if both are provided: Drop the tag? Verify tag to digest? Drop the digest? Just pass on to the validator (notaryv1 or cosign)?
2. how do we interpret and apply the result: Deny or overwrite if tag and digest do not match? As there is a digest, do we only give a yes or no answer whether the digest checks out?

I'd presume that the best way forward may be to pass the whole construct to the validator and go for only validating but not modifying the request as there is a digest and the user intended to have that construct validated and the user would have to take care that it can actually be interpreted by the container runtime which I'd rather not want to take care of.

[chgl]

1. how to read it if both are provided: Drop the tag? Verify tag to digest? Drop the digest? Just pass on to the validator (notaryv1 or cosign)?

I believe the easiest to implement would be to just drop the tag and use only the digest. Although at least the cosign validator can handle both tags+digest being present.

2. how do we interpret and apply the result: Deny or overwrite if tag and digest do not match? As there is a digest, do we only give a yes or no answer whether the digest checks out?

Do we need to interpret the result in any way? I am not too familiar with the internals of connaisseur, I thought it would just be an "OK" or "not OK" response from the validators causing the webhook to accept or reject the pods accordingly. Here dropping the tag would again be the path of least resistance.

I've taken a stab at the smallest possible changes necessary in #763 - this will just use the image digest if both are set.

[richgerrard]

Please consider the proposed fix at your earliest convenience, @xopham

For clarity, from the OCI spec:

- Digest: a unique identifier created from a cryptographic hash of a Blob's content. Digests are defined under the OCI Image Spec [apdx-3](https://github.com/sse-secure-systems/connaisseur/issues/749#appendix)
- Tag: a custom, human-readable manifest identifier

I'm all for immutability, but I'm a human being first and foremost. When I'm operating my enterprise clusters, my observability tools suite only display a tangled mess of digest references following the mutating webhook implementation.

My goal is to know what is deployed in my cluster without wasting time. My human brain knows that tags are mutable. My human brain knows that tags are not semvers. My human brain prefers a shorthand for identifying a manifest. My human brain is why we wish to include tags AND digests in manifest files together.

You can already sort out the validation based on the immutable part, and it's perfectly clear that this is a bug that requires an immediate fix. I'll argue later if your mutating webhook also fails to preserve my tag value, but for now: please consider releasing a fix for this bug at your earliest convenience.

It's for the humans. Thank you!

[chgl]

Should be closed by #763 now. Thanks everyone for the work and inputs!