image: serialize RegistryImageReferences when checking signatures #8605

haircommander · 2024-09-13T19:16:30Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

So we can check a signature of an image that has both a tag and a digest

Which issue(s) this PR fixes:

fixes #8603

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug where signature checking failed if an image specified both a tag and a digest

haircommander · 2024-09-13T19:16:52Z

internal/storage/references/registry_reference.go

+// Such image references are supported by docker but, due to their ambiguity, explicitly not by containers/image.
+//
+// This should only be called from internal/storage.
+func RegistryImageReferenceFromRawSerialized(rawNamed reference.Named) (RegistryImageReference, error) {


not crazy about this name, @mtrmac do you have any suggestions?

See the other comment for a bit higher-level view…

… which still leaves open the naming of this canonicalization. My starting point is canonicalize{TagDigest,DockerLike} somewhere in the name, but that might get awfully long.

codecov · 2024-09-13T19:22:56Z

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 48.85%. Comparing base (ac758bb) to head (4a2d29e).
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8605      +/-   ##
==========================================
- Coverage   48.91%   48.85%   -0.07%     
==========================================
  Files         153      153              
  Lines       17372    17371       -1     
==========================================
- Hits         8498     8486      -12     
- Misses       7807     7819      +12     
+ Partials     1067     1066       -1

haircommander · 2024-09-13T19:34:04Z

/retest

mtrmac

ACK to the overall functionality this achieves.

WRT the implementation … my question is “what rules are implied by the RegistryImageReference data type”? I think it should be one of:

:tag@digest references are unambiguously allowed by the data type.
- In that case the mapping of :tag@digest to @digest-only must be in consumers of RegistryImageReference, i.e. in PullImage/IsRunningImageAllowed (and what value should be used in MetricImagePullsSuccessesInc, with or without the tag?)
- or maybe RegistryImageReference should have a RawCanonicalized method that returns a value suitable for c/image, and most callers of Raw should migrate
:tag@digest references are unambiguously invalid in the data type. In that case
- the constructor of RegistryImageReferenceFromRaw should reject that value to enforce the rule
- It would be consistent for ParseRegistryImageReferenceFromOutOfProcessData to reject the data as well
- We’d probably need some kind of …FromRawCanonicalizing and Parse…CanonicalizingFromOutOfProcesss… helpers, so that top-level callers don’t manipulate values directly.

The current PR is ambiguous about the permissibility of such values — the text parser canonicalizes them, but …FromRaw does not.

I weakly prefer the variant where :tag@digest values are invalid in RegistryImageReference, forcing its users to explicitly choose, by the constructor they call, whether they want to accept such values or not. If nothing else, the parseImageParent/parseImageChild RPC should never generate such values, and it can use the stricter parser/constructor.

mtrmac · 2024-09-13T19:34:40Z

internal/storage/references/registry_reference.go

+// Such image references are supported by docker but, due to their ambiguity, explicitly not by containers/image.
+//
+// This should only be called from internal/storage.
+func RegistryImageReferenceFromRawSerialized(rawNamed reference.Named) (RegistryImageReference, error) {


See the other comment for a bit higher-level view…

… which still leaves open the naming of this canonicalization. My starting point is canonicalize{TagDigest,DockerLike} somewhere in the name, but that might get awfully long.

mtrmac · 2024-09-13T19:38:45Z

test/policy.bats

 	[[ "$output" == *"SignatureValidationFailed"* ]]
 }
+
+@test "allow signed image with restrictive policy on container creation7 if already pulled (by tag and ID)" {


This is fundamentally confusing things. “Image ID” (StorageImageID) is, historically (nowadays not always) the sha256 digest of the config, and usually expressed without the leading sha256:.

“image digest” is the sha256 of the manifest. The two are ~never the same for a single image.

In busybox:latest@sha256:…, the second value is the manifest digest. So assuming the value from crictl images -q is an image ID, using it in the manifest digest syntax should never find a match; and the test name is misleading.

(Also for the other added test.)

haircommander · 2024-09-16T15:51:11Z

I agree the library should take care of canonicalizing. In fact, I just replaced *FromRaw with this behavior. Also i believe I updated the test to test digest and not storage ID. PTAL again @mtrmac

mtrmac

I’d prefer two variants of constructors, one only in the “XOR” form, one canonicalizing — but this works, up to you.

Assuming this design, code LGTM, just some documentation suggestions.

mtrmac · 2024-09-16T15:59:05Z

internal/storage/references/registry_reference.go

+// It does two validations on rawNamed:
+// - It validates the image either has a tag or a digest, but not both (preferring digest)
+// - Verifies the image is not only a name.


(The first one is a silent fix-up, and the second one causes an error; the phrasing suggests to me that both are treated equally.)

mtrmac · 2024-09-16T16:01:13Z

internal/storage/references/registry_reference.go

+	if isTagged && isDigested {
+		canonical, err := reference.WithDigest(reference.TrimNamed(rawNamed), canonical.Digest())
+		if err != nil {
+			return RegistryImageReference{}, err


(The only way this can fail is if canonical.Digest() is invalid, and that shouldn’t have happened by induction, so I’d be fine with a panic here. But returning an error explicitly is perfectly valid as well.)

that's better because then I don't need to change the function signature

Also using panic does not leave around (supposedly) 100% unreachable error handling code paths, considering that CRI-O is measuring code coverage.

mtrmac · 2024-09-16T16:02:40Z

internal/storage/references/registry_reference.go

 	}
 	ref = reference.TagNameOnly(ref)
-	return RegistryImageReferenceFromRaw(ref), nil
+	return RegistryImageReferenceFromRaw(ref)


The documentation of ParseRegistryImageReferenceFromOutOfProcessData should document that it removes the tag from tag@digest inputs.

mtrmac · 2024-09-16T16:03:31Z

internal/storage/image.go

-		// Strip the tag from ambiguous image references that have a
-		// digest as well (e.g.  `image:tag@sha256:123...`).  Such
-		// image references are supported by docker but, due to their
-		// ambiguity, explicitly not by containers/image.


(Absolutely non-blocking: Consider preserving some form of this comment, to have a documentation for the reason we modify the inputs.)

mtrmac · 2024-09-16T16:29:38Z

internal/storage/references/registry_reference.go

 // It is only intended for communication with OUT-OF-PROCESS APIs,
 // like registry references provided by CRI by Kubelet.
+// It will modify the reference if both digest and tag are specified, stripping the tag and leaving the digest.
+// It will also verifies the image is not only a name. If it is only a name, the function errors.


The function does not fail in such a situation: The TagNameOnly call turns name-only references into name:latest

mtrmac

Do you also want to fix the linter-highlighted typo?

LGTM either way.

So we can check a signature of an image that has both a tag and a digest Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2024-09-16T16:47:53Z

whoops, fixed!

mtrmac

/lgtm

haircommander · 2024-09-16T17:20:12Z

/cherry-pick release-1.31

openshift-cherrypick-robot · 2024-09-16T17:20:16Z

@haircommander: once the present PR merges, I will cherry-pick it on top of release-1.31 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.31

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kwilczynski · 2024-09-16T17:25:27Z

/approve
/lgtm

openshift-ci · 2024-09-16T17:25:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [haircommander]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

haircommander · 2024-09-17T13:10:26Z

/retest

haircommander · 2024-09-17T19:55:16Z

/retest

haircommander · 2024-09-18T16:42:27Z

/retest

openshift-cherrypick-robot · 2024-09-18T20:28:26Z

@haircommander: new pull request created: #8618

In response to this:

/cherry-pick release-1.31

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

haircommander requested a review from mrunalp as a code owner September 13, 2024 19:16

openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Sep 13, 2024

openshift-ci bot requested review from QiWang19 and littlejawa September 13, 2024 19:16

haircommander commented Sep 13, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2024

mtrmac reviewed Sep 13, 2024

View reviewed changes

BenoitKnecht mentioned this pull request Sep 16, 2024

helm: Render valid image specs when tag is empty cilium/cilium#34891

Merged

8 tasks

haircommander force-pushed the serialize-signatures branch from d5053b0 to 0841f69 Compare September 16, 2024 15:49

mtrmac reviewed Sep 16, 2024

View reviewed changes

haircommander force-pushed the serialize-signatures branch from 0841f69 to 2a3a5fc Compare September 16, 2024 16:25

mtrmac reviewed Sep 16, 2024

View reviewed changes

haircommander force-pushed the serialize-signatures branch from 2a3a5fc to b7c5e6d Compare September 16, 2024 16:40

mtrmac reviewed Sep 16, 2024

View reviewed changes

image: serialize RegistryImageReferences when checking signatures

4a2d29e

So we can check a signature of an image that has both a tag and a digest Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the serialize-signatures branch from b7c5e6d to 4a2d29e Compare September 16, 2024 16:47

mtrmac reviewed Sep 16, 2024

View reviewed changes

openshift-ci bot assigned mtrmac Sep 16, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2024

openshift-ci bot assigned kwilczynski Sep 16, 2024

openshift-merge-bot bot merged commit a6805d8 into cri-o:main Sep 18, 2024
76 of 84 checks passed

openshift-cherrypick-robot mentioned this pull request Sep 18, 2024

[release-1.31] image: serialize RegistryImageReferences when checking signatures #8618

Merged

github-actions bot mentioned this pull request Dec 22, 2024

Bump crio from v1.30.3 to v1.32.0 kokyhm/kubespray#109

Open

image: serialize RegistryImageReferences when checking signatures #8605

image: serialize RegistryImageReferences when checking signatures #8605

Uh oh!

Conversation

haircommander commented Sep 13, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

haircommander commented Sep 13, 2024

Uh oh!

mtrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haircommander commented Sep 16, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

haircommander commented Sep 16, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

haircommander commented Sep 16, 2024

Uh oh!

openshift-cherrypick-robot commented Sep 16, 2024

Uh oh!

kwilczynski commented Sep 16, 2024

Uh oh!

openshift-ci bot commented Sep 16, 2024

Uh oh!

haircommander commented Sep 17, 2024

Uh oh!

haircommander commented Sep 17, 2024

Uh oh!

haircommander commented Sep 18, 2024

Uh oh!

Uh oh!

openshift-cherrypick-robot commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Sep 13, 2024 •

edited

Loading

mtrmac left a comment •

edited

Loading