Image verification for namespaced policies #8212

harche · 2024-05-23T15:32:01Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for verification using namespaced policies during container creation.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add support for validating signatures on container creation. Now, if there is a namespaced policy in the `signature_policy_dir`, CRI-O will validate the signature defined in `signature_policy_dir`/`NS`.json for pods in namespace `NS`

saschagrunert · 2024-05-28T14:11:35Z

@harche feel free to ping here if you need assistance or review.

rphillips · 2024-07-12T16:20:32Z

internal/storage/image.go

+		if err != nil && !errors.Is(err, storage.ErrImageUnknown) {
+			return fmt.Errorf("error retrieving image %s: %w", configImage, err)
+		}
+		if img != nil {


perhaps double check the length of img here.

It should >= 1 in this context.

@rphillips img is of Image type,

cri-o/vendor/github.com/containers/storage/images.go

Line 32 in a934077

type Image struct {

Hence the check for if it is nil.

Right, but the length of the array could be zero, and we are indexing into it.

@rphillips something like,

if img != nil && img.Names != nil && len(img.Names) > 0

?

I'm going to retract my comment here... I think the better approach is to check if err != nil if the alternative case, on line 542, and remove the if img != nil entirely.

you can have err == nil and img == nil at the same time. It just means that, there was no error looking up in the image store but the image doesn't exist in that store.

saschagrunert

The unit test mocks needs to be updated (see GitHub actions).

Is this still work in progress (WIP)?

internal/storage/image.go

harche · 2024-07-17T15:10:19Z

/retest-required

Signed-off-by: Peter Hunt <pehunt@redhat.com>

which protects us against cases where the user_defined_image isn't specified (like for container restores) Signed-off-by: Peter Hunt <pehunt@redhat.com>

We already have access to the storage reference, so we should use it Signed-off-by: Peter Hunt <pehunt@redhat.com>

…mage Signed-off-by: Peter Hunt <pehunt@redhat.com>

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander · 2024-09-09T19:55:48Z

/lgtm

(had to update the CR test)

haircommander · 2024-09-09T19:58:05Z

/override ci/prow/ci-rhel-critest

openshift-ci · 2024-09-09T19:58:10Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-rhel-critest

In response to this:

/override ci/prow/ci-rhel-critest

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 · 2024-09-09T20:14:19Z

/retest

haircommander · 2024-09-09T20:25:03Z

/override ci/prow/ci-rhel-critest
/retest

openshift-ci · 2024-09-09T20:25:10Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-rhel-critest

In response to this:

/override ci/prow/ci-rhel-critest
/retest

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 · 2024-09-09T20:38:21Z

/retest

haircommander · 2024-09-09T20:38:32Z

/override ci/prow/ci-rhel-critest

openshift-ci · 2024-09-09T20:38:38Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-rhel-critest

In response to this:

/override ci/prow/ci-rhel-critest

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 · 2024-09-09T20:47:59Z

/override ci/prow/ci-rhel-critest

openshift-ci · 2024-09-09T20:48:04Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-rhel-critest

In response to this:

/override ci/prow/ci-rhel-critest

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.

mtrmac

Late review … now comprehensive.

I’ll file a PR for some parts, and an issue for the remaining outstanding things.

mtrmac · 2024-09-10T20:37:45Z

server/image_pull_test.go

-	imageID, err := storage.ParseStorageImageIDFromOutOfProcessData("2a03a6059f21e150ae84b0973863609494aad70f0a80eaeb64bddd8d92465812")
-	Expect(err).ToNot(HaveOccurred())
-	otherImageID, err := storage.ParseStorageImageIDFromOutOfProcessData("3a03a6059f21e150ae84b0973863609494aad70f0a80eaeb64bddd8d92465812")
+	canonicalImageCandidate, err := reference.WithDigest(imageCandidate.Raw(), digest.Digest("sha256:340d9b015b194dc6e2a13938944e0d016e57b9679963fdeb9ce021daac430221"))


This is not representative; PullImage returns a name@digest, this is name:tag@digest.

Fixed in #8590 .

mtrmac · 2024-09-10T20:43:20Z

test/image.bats

@@ -26,7 +26,7 @@ function teardown() {
 @test "run container in pod with image ID" {
 	start_crio
 	pod_id=$(crictl runp "$TESTDATA"/sandbox_config.json)
-	jq '.image.image = "'"$REDIS_IMAGEID"'"' \
+	jq '.image.image = "'"$REDIS_IMAGEID"'" | .image.user_specified_image = "'"$REDIS_IMAGEDIGEST"'"' \


In the three instances in this file, image.user_specified_image is already set (to name:tag) in container_config.json; is there a specific need to override that to a digest value?

Or is that just for consistency with the other tests that override both .image.image and .image.user_specified_image?

mtrmac · 2024-09-10T20:45:24Z

test/image.bats

+        select(.repoTags[0] == "quay.io/crio/hello-wasm:latest") |
+        "WASM_IMAGEID=" + .id + "\n" +
+        "WASM_IMAGEDIGEST=" + .repoDigests[0] + "\n" +
+	"REDIS_IMAGEREF=" + .repoDigests[0]' <<< "$json")"


Is this a copy&paste error?

mtrmac · 2024-09-10T20:48:55Z

test/nri/runtime.go

@@ -328,7 +332,8 @@ type ContainerOption func(*cri.ContainerConfig) error
 func WithImage(image string) ContainerOption {


(I can’t see any users of this.)

mtrmac · 2024-09-10T20:56:37Z

internal/storage/runtime.go

@@ -316,7 +316,7 @@ func (r *runtimeService) CreatePodSandbox(systemContext *types.SystemContext, po
 		if imageAuthFile != "" {
 			sourceCtx.AuthFilePath = imageAuthFile
 		}
-		ref, err = r.storageImageServer.PullImage(context.Background(), pauseImage, &ImageCopyOptions{
+		ref, _, err = r.storageImageServer.PullImage(context.Background(), pauseImage, &ImageCopyOptions{


This is probably good enough because the pause image is ~trusted, and because MCO sets up the signature policy in a way that doesn’t let users loosen it, IIRC.

Ideally the returned repo@digest would be used to determine img below, so that the code uses exactly the just-pulled image.

And then the first return value of PullImage becomes unused and can be dropped entirely, simplifying the code.

Perhaps the repo@digest return value should be a RegistryImageReference, not a raw reference.Canonical.

Done in #8590.

mtrmac · 2024-09-10T21:37:42Z

internal/storage/image.go

+	//
+	// Arguments:
+	// - ctx: The context for controlling the function's execution
+	// - systemContext: server's system context for the given namespace


…, notably might have a customized SignaturePolicyPath.

Fixed in #8590.

mtrmac · 2024-09-10T21:38:59Z

internal/storage/image.go

+		}
+	}()
+
+	if err := svc.checkSignature(ctx, systemContext, policyContext, imageName, imageID); err != nil {


(I’m not sure why splitting this function into two is valuable, but it also doesn’t hurt.)

mtrmac · 2024-09-10T21:39:11Z

internal/storage/image.go

+	return nil
+}
+
+func (svc *imageService) checkSignature(ctx context.Context, sys *types.SystemContext, policyContext *signature.PolicyContext, imageName RegistryImageReference, imageID StorageImageID) error {


s/imageName/userSpecifiedImage/

Fixed in #8590.

mtrmac · 2024-09-10T21:51:24Z

server/container_create_linux.go

+		return nil, fmt.Errorf("get context for namespace: %w", err)
+	}
+
+	if systemCtx.SignaturePolicyPath != "" {


Hum, do we want/need this condition here? It basically hard-codes the MCO behavior that the per-namespace policy is never more restrictive than the system policy.

That is fine for us right now, but it might not be true forever.

Does this exist for the container restore RootFSImage{Name,Ref} uses?

I guess that, at the very least, the two conditions (here and in the container restore path) should document this assumption.

Commented in #8590 .

mtrmac · 2024-09-10T21:54:02Z

server/container_create_linux.go

+		// and it is meaningless to try to verify an image that isn't even an image
+		// (like a checkpointed file is).


I don’t think we get here in the container restore path where Image.Image is at tar file; we have already successfully found a c/storage imgResult at this point.

mtrmac · 2024-09-11T00:35:59Z

I have filed #8590 with various cleanups; I’d be happy to hand off finishing that.
And #8591 for possible new tests; I’m definitely leaving that part to CRI-O maintainers.

harche requested a review from mrunalp as a code owner May 23, 2024 15:32

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 23, 2024

openshift-ci bot requested review from klihub and sohankunkerkar May 23, 2024 15:32

openshift-ci bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label May 23, 2024

harche force-pushed the sigstore_namespace branch from f3285a9 to d104419 Compare May 23, 2024 15:51

harche marked this pull request as draft May 23, 2024 16:06

harche force-pushed the sigstore_namespace branch 3 times, most recently from 8126007 to 8231d9f Compare June 4, 2024 14:36

harche force-pushed the sigstore_namespace branch from 8231d9f to aa5a6c6 Compare June 20, 2024 18:09

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2024

kwilczynski assigned harche Jul 11, 2024

harche force-pushed the sigstore_namespace branch 2 times, most recently from 8853f7b to 6fae2e4 Compare July 12, 2024 15:56

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2024

harche marked this pull request as ready for review July 12, 2024 15:59

openshift-ci bot requested a review from littlejawa July 12, 2024 15:59

rphillips reviewed Jul 12, 2024

View reviewed changes

harche force-pushed the sigstore_namespace branch from 6fae2e4 to f0c7e69 Compare July 12, 2024 19:07

saschagrunert reviewed Jul 15, 2024

View reviewed changes

internal/storage/image.go Outdated Show resolved Hide resolved

harche force-pushed the sigstore_namespace branch from f0c7e69 to 51c7108 Compare July 17, 2024 14:25

harche changed the title ~~WIP: image verificaiton for namespaced policies~~ Image verification for namespaced policies Jul 17, 2024

harche force-pushed the sigstore_namespace branch from 51c7108 to bbbc96d Compare July 17, 2024 14:36

haircommander added 8 commits September 9, 2024 15:55

test: fix crun-wasm test to handle requirement of user_specified_image

1444e69

Signed-off-by: Peter Hunt <pehunt@redhat.com>

store canonical ref differently

7a67eb7

Signed-off-by: Peter Hunt <pehunt@redhat.com>

server: use cached restore value instead of recomputing

ec8545d

Signed-off-by: Peter Hunt <pehunt@redhat.com>

server: only check signatures if namespaced policy is defined

0dd7eaf

which protects us against cases where the user_defined_image isn't specified (like for container restores) Signed-off-by: Peter Hunt <pehunt@redhat.com>

server: use imageID instead of a random digest

5007524

We already have access to the storage reference, so we should use it Signed-off-by: Peter Hunt <pehunt@redhat.com>

server: document difference between userRequestedImage/userSpecifiedI…

1a9d364

…mage Signed-off-by: Peter Hunt <pehunt@redhat.com>

server/restore: mark signature validation incompatible with restore

a379923

Signed-off-by: Peter Hunt <pehunt@redhat.com>

test: fix CR test by unsetting SIGNATURE_POLICY

fbd73b3

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander force-pushed the sigstore_namespace branch from eac32f6 to fbd73b3 Compare September 9, 2024 19:55

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

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

openshift-merge-bot bot merged commit ac758bb into cri-o:main Sep 9, 2024
80 of 82 checks passed

mtrmac reviewed Sep 10, 2024

View reviewed changes

This was referenced Sep 11, 2024

Cleanups on top of #8212 #8590

Open

sigstore-related missing tests #8591

Open

mtrmac mentioned this pull request Sep 13, 2024

[V1.31.0] Error: checking signature : Docker references with both a tag and digest are currently not supported #8603

Closed

This was referenced Sep 19, 2024

Bump crio from v1.30.3 to v1.31.0 kokyhm/kubespray_dependencies_bump_playground#8

Open

Bump crio from v1.30.3 to v1.31.0 kokyhm/kubespray#20

Open

		@@ -328,7 +332,8 @@ type ContainerOption func(*cri.ContainerConfig) error
		func WithImage(image string) ContainerOption {

		// and it is meaningless to try to verify an image that isn't even an image
		// (like a checkpointed file is).

Image verification for namespaced policies #8212

Image verification for namespaced policies #8212

Conversation

harche commented May 23, 2024 • edited by haircommander Loading

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?

saschagrunert commented May 28, 2024

rphillips Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert left a comment • edited Loading

Choose a reason for hiding this comment

harche commented Jul 17, 2024

haircommander commented Sep 9, 2024

haircommander commented Sep 9, 2024

openshift-ci bot commented Sep 9, 2024

haircommander commented Sep 9, 2024

haircommander commented Sep 9, 2024

openshift-ci bot commented Sep 9, 2024

haircommander commented Sep 9, 2024

haircommander commented Sep 9, 2024

openshift-ci bot commented Sep 9, 2024

haircommander commented Sep 9, 2024

openshift-ci bot commented Sep 9, 2024

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Sep 11, 2024

harche commented May 23, 2024 •

edited by haircommander

Loading

rphillips Jul 12, 2024 •

edited

Loading

saschagrunert left a comment •

edited

Loading