[OCPNODE-1639] Add ClusterImagePolicy, ImagePolicy to support signature verification #1402

QiWang19 · 2023-05-17T00:18:32Z

This enhancement proposes ClusterImagePolicy and ImagePolicy CRDs to provide a way to set image signature verification configuration on Openshift.
Epic: OCPNODE-1628: Sigstore Support - OpenShift Container Image Validation (Dev Preview)

QiWang19 · 2023-05-17T02:48:12Z

@saschagrunert could you take a look? I'm not sure how to best define the Graduation Criteria.

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md

QiWang19 · 2023-05-18T16:55:15Z

@saschagrunert PTAL

saschagrunert

LGTM, just the open point with the Hostname requires change from my point of view.

QiWang19 · 2023-05-22T14:16:27Z

@mrunalp @yuqi-zhang PTAL

mtrmac

I was unaware of this PR — so for now just a very quick first glance to make sure this is not lost.

I apologize if this was discussed previously; I’ll try to catch up with the previous discussions.

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md

yuqi-zhang

Seems generally ok from the MCO perspective, some nits/questions below:

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

JoelSpeed · 2023-10-10T17:11:47Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+}
+
+// Policy defines the verification policy for the items in the scopes list.
+// +kubebuilder:validation:XValidation:rule="(has(self.rootOfTrust) && has(self.rootOfTrust.policyType) && self.rootOfTrust.policyType == 'FulcioCAWithRekor') == has(self.fulcioSubject)",message="fulcioSubject must be set exactly when policyType is FulcioCAWithRekor"


This validation suggests to me that the discriminated union should be one level higher than it is. If there are types structs that need to be duplicated across the different union values that's something we can do

This isn't resolved, can be resolved in API review

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

JoelSpeed · 2023-10-10T17:16:13Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+Enhance the MCO container runtime config controller to manage ClusterImagePolicy and ImagePolicy CRs, and update the signature verification configurations:
+- Retrieves all the ClusterImagePolicy and ImagePolicy instances on the cluster.
+- Pre-validation: 
+  - Ensure that there are no scopes under the OCP image payload. If any are found, add an info-level log message to the machine config controller about the error encountered while adding an OCP image payload. Update the `status` of the CR to indicate that the policy will not be applied.


Can this be prevented by an admission webhook and/or CEL validation preventing certain strings?

Added the semantic validation using CEL expression.

I don't see this, can you expand this comment to explain that it will be validated at the API level

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

JoelSpeed · 2023-10-10T17:17:27Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+- Retrieves all the ClusterImagePolicy and ImagePolicy instances on the cluster.
+- Pre-validation: 
+  - Ensure that there are no scopes under the OCP image payload. If any are found, add an info-level log message to the machine config controller about the error encountered while adding an OCP image payload. Update the `status` of the CR to indicate that the policy will not be applied.
+  - Check for conflicts between cluster scope and namespace scope policies. If the namespaced ImagePolicy scope is equal to or nests inside an existing cluster-scoped ClusterImagePolicy CR, do not deploy the namespaced policy.


Can you provide an example of the feedback. Could this not be done via an admission webhook and prevent the object from even being stored in etcd?

I added the feedback to the example. https://github.com/openshift/enhancements/pull/1402/files#diff-fb631ca70e56b7e72dcd1e5e0880976a7a916abf88d8c3c220b7fc2599415f57R378
We have discussed avoiding the admission validation in the first version.

Can you add a note that this will be validated by an admission webhook before the feature is marked GA

QiWang19 · 2023-10-13T03:08:37Z

@JoelSpeed PTAL

QiWang19 · 2023-10-16T19:21:17Z

@JoelSpeed could you give another round of review?

JoelSpeed

I think in general I'm happy with the enhancement, but I would like it noted that the validations I've marked outside of the API section should be included and noted that this will be done.

The API itself I think will still need a few round of reviews. Each field should have a good level of godoc and validations applied, plus the comments I made about the level of the discriminated union are not yet resolved.
The API content while included here we can continue in the API repo PR now, we can retrospectively update this enhancement should it merge before the API is ready

JoelSpeed · 2023-10-19T11:29:27Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+// +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9-_+.*@:/]+$')",message="invalid
+// image scope format, scope contained invalid characters, valid characters are [a-zA-Z0-9-*.@_]"
+// +kubebuilder:validation:XValidation:rule="size(self.split('/')[0].split('.')) == 1 ? self.split('/')[0].split('.')[0].split(':')[0] == 'localhost' : true",message="invalid image scope format,
+// scope must contain a fully qualified domain name or 'localhost'"
+// +kubebuilder:validation:XValidation:rule=`self.contains('*') ? self.matches('^\\*(?:\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+$') : true`,message="invalid image scope with wildcard, a wildcard
+// can only be at the start of the domain and does not contain subdomains"
+// +kubebuilder:validation:MaxLength=512
+type ImageScope string


Do you still have the original regex handy? I don't think these cover all of the cases, I just gave a couple of examples of the sort of validations i expected, this can be resolved in API review but we should make sure as many of the regex rules are covered as possible

JoelSpeed · 2023-10-19T11:29:42Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+}
+
+// Policy defines the verification policy for the items in the scopes list.
+// +kubebuilder:validation:XValidation:rule="(has(self.rootOfTrust) && has(self.rootOfTrust.policyType) && self.rootOfTrust.policyType == 'FulcioCAWithRekor') == has(self.fulcioSubject)",message="fulcioSubject must be set exactly when policyType is FulcioCAWithRekor"


This isn't resolved, can be resolved in API review

JoelSpeed · 2023-10-19T11:30:30Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+type PolicyRootOfTrust struct {
+	// policyType serves as the union's discriminator. Users are required to assign a value to this field, choosing one of the policy types that define the root of trust.
+	// "PublicKey" indicates that the policy relies on a PGP publicKey and may optionally use a Rekor verification.
+	// "FulcioCAWithRekor" indicates that the policy is based on the Fulcio certification and incorporates a Rekor verification.


You don't need to state Rekor here if there's no option to have Fulcio without, what about FulcioCA instead?

JoelSpeed · 2023-10-19T11:30:50Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+type PublicKey struct {
+	// keyData contains inline base64 encoded data of the public key.
+	// +kubebuilder:validation:Required
+	KeyData string `json:"keyData"`


Will need a maximum length, what's a reasonable maximum length for this? 8192?

JoelSpeed · 2023-10-19T11:31:01Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+	KeyData string `json:"keyData"`
+	// rekorKeyData contains inline base64 data of the Rekor public key.
+	// +optional
+	RekorKeyData string `json:"rekorKeyData,omitempty"`


Will need a maximum length

JoelSpeed · 2023-10-19T11:31:50Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+	// fulcioCAData contains inline base64 data for the fulcio CA certificate.
+	// +kubebuilder:validation:Required
+	FulcioCAData string `json:"fulcioCAData"`
+	// rekorKeyData contains inline base64 data of the Rekor public key.
+	// +kubebuilder:validation:Required
+	RekorKeyData string `json:"rekorKeyData"`


Maximum lengths on these too

JoelSpeed · 2023-10-19T11:32:50Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'ExactRepository') ? has(self.exactRepository) : true",message="must set exactRepository if matchPolicy is ExactRepository"
+// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'RemapIdentity') ? has(self.remapIdentity) : true",message="must set remapIdentity if matchPolicy is RemapIdentity"


The other unionmembers should be forbidden otherwise, see the examples in the API repo examples API group

JoelSpeed · 2023-10-19T11:34:12Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+Enhance the MCO container runtime config controller to manage ClusterImagePolicy and ImagePolicy CRs, and update the signature verification configurations:
+- Retrieves all the ClusterImagePolicy and ImagePolicy instances on the cluster.
+- Pre-validation: 
+  - Ensure that there are no scopes under the OCP image payload. If any are found, add an info-level log message to the machine config controller about the error encountered while adding an OCP image payload. Update the `status` of the CR to indicate that the policy will not be applied.


I don't see this, can you expand this comment to explain that it will be validated at the API level

JoelSpeed · 2023-10-19T11:34:34Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+- Retrieves all the ClusterImagePolicy and ImagePolicy instances on the cluster.
+- Pre-validation: 
+  - Ensure that there are no scopes under the OCP image payload. If any are found, add an info-level log message to the machine config controller about the error encountered while adding an OCP image payload. Update the `status` of the CR to indicate that the policy will not be applied.
+  - Check for conflicts between cluster scope and namespace scope policies. If the namespaced ImagePolicy scope is equal to or nests inside an existing cluster-scoped ClusterImagePolicy CR, do not deploy the namespaced policy.


Can you add a note that this will be validated by an admission webhook before the feature is marked GA

QiWang19 · 2023-10-19T14:05:17Z

@JoelSpeed PTAL, Fixed the reviews for the enhancement and will address the API changes in the API PR.

JoelSpeed · 2023-10-19T16:39:34Z

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md

+
+### Graduation Criteria
+
+Before GA, we should explore relocating CRD validations to the admission webhook to ensure the scopes are not under OCP image payload repository are configured.


This isn't quite what I thought we had agreed, I would have said we agreed on

Suggested change

Before GA, we should explore relocating CRD validations to the admission webhook to ensure the scopes are not under OCP image payload repository are configured.

Before GA, validation will be implemented to make sure that scopes cannot be committed to an ImagePolicy that are under the scope of a ClusterImagePolicy, this will prevent conflicts between cluster scoped and namespace scoped policy and prevent namespaces attempting to override the global policy.

Additionally, it will be validated that neither an ImagePolicy nor ClusterImagePolicy sets a scope that could conflict with pulling from an OpenShift image payload repository.

The important part is that this validation isn't in the CRD, it's additional validation that we cannot currently do within the CRD IIUC

This enhancement propose ClusterImagePolicy and ImagePolicy CRDs to provide a way to set image signature verification configuration on Openshift. Signed-off-by: Qi Wang <[email protected]>

JoelSpeed · 2023-10-20T11:50:14Z

I'm happy to LGTM this at this point, with the caveats that the API still needs review, and that will be continued in the API PR itself, and that I would like to see this introduced as tech preview and the validations we've discussed in the graduation criteria are honoured as we move to GA, but I think we are all on the same page there.

Is there any more feedback required from SMEs before this merges?

QiWang19 · 2023-10-20T17:53:05Z

No, there is no more feedback required from SMEs before this merges.

saschagrunert · 2023-10-23T07:24:03Z

@JoelSpeed are we happy to /lgtm and /approve?

JoelSpeed · 2023-10-23T09:10:46Z

No objections from me, but @mrunalp is down as the approve so he should be the one to merge this

mrunalp · 2023-10-23T15:03:28Z

/lgtm
/approve

openshift-ci · 2023-10-23T15:04:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, saschagrunert

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~~ [mrunalp]

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

openshift-ci · 2023-10-23T15:55:04Z

@QiWang19: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from adambkaplan and ashcrow May 17, 2023 00:18

QiWang19 changed the title ~~Add ImagePolicy to support signature verification~~ [WIP] Add ImagePolicy to support signature verification May 17, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2023

QiWang19 force-pushed the imagepolicy branch 6 times, most recently from 6b828c3 to 4e757d5 Compare May 17, 2023 02:45

saschagrunert reviewed May 17, 2023

View reviewed changes

QiWang19 changed the title ~~[WIP] Add ImagePolicy to support signature verification~~ [WIP] [OCPNODE-1639] Add ImagePolicy to support signature verification May 17, 2023

QiWang19 force-pushed the imagepolicy branch from 4e757d5 to b7e401e Compare May 17, 2023 22:09

QiWang19 force-pushed the imagepolicy branch from b7e401e to 1c12427 Compare May 19, 2023 03:05

saschagrunert approved these changes May 19, 2023

View reviewed changes

QiWang19 force-pushed the imagepolicy branch from 1c12427 to 2070d8d Compare May 21, 2023 05:27

QiWang19 changed the title ~~[WIP] [OCPNODE-1639] Add ImagePolicy to support signature verification~~ [OCPNODE-1639] Add ImagePolicy to support signature verification May 21, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2023

saschagrunert approved these changes May 22, 2023

View reviewed changes

saschagrunert mentioned this pull request May 25, 2023

Add SigstoreImageVerification feature gate openshift/api#1470

Merged

mtrmac reviewed May 29, 2023

View reviewed changes

yuqi-zhang reviewed May 29, 2023

View reviewed changes

QiWang19 force-pushed the imagepolicy branch 2 times, most recently from 45ba130 to 3453044 Compare June 1, 2023 05:52

wking reviewed Jun 8, 2023

View reviewed changes

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md Outdated Show resolved Hide resolved

wking reviewed Jun 8, 2023

View reviewed changes

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md Outdated Show resolved Hide resolved

wking reviewed Jun 8, 2023

View reviewed changes

enhancements/api-review/add-ImagePolicy-to-config.openshift.io-v1alpha1.md Outdated Show resolved Hide resolved

QiWang19 force-pushed the imagepolicy branch 2 times, most recently from 922554d to edacd13 Compare October 8, 2023 04:15

JoelSpeed reviewed Oct 10, 2023

View reviewed changes

QiWang19 force-pushed the imagepolicy branch 4 times, most recently from 850bc41 to c6c69bc Compare October 13, 2023 02:52

QiWang19 force-pushed the imagepolicy branch from c6c69bc to b28cb12 Compare October 16, 2023 19:06

JoelSpeed reviewed Oct 19, 2023

View reviewed changes

QiWang19 force-pushed the imagepolicy branch from b28cb12 to 55a1bcb Compare October 19, 2023 13:52

JoelSpeed reviewed Oct 19, 2023

View reviewed changes

Add ClusterImagePolicy,ImagePolicy to support signature verification

7258382

This enhancement propose ClusterImagePolicy and ImagePolicy CRDs to provide a way to set image signature verification configuration on Openshift. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the imagepolicy branch from 55a1bcb to 7258382 Compare October 19, 2023 16:52

QiWang19 changed the title ~~[OCPNODE-1639] Add ImagePolicy to support signature verification~~ [OCPNODE-1639] Add ClusterImagePolicy, ImagePolicy to support signature verification Oct 20, 2023

QiWang19 mentioned this pull request Oct 20, 2023

[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy openshift/api#1457

Merged

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

openshift-ci bot merged commit 6bd4d8c into openshift:master Oct 23, 2023

QiWang19 mentioned this pull request May 2, 2024

Update ClusterImagePolicy enhancement to match the actual implementation #1622

Merged

wking mentioned this pull request Jun 17, 2024

OCPBUGS-35528: keys: Update Red Hat keys to use SHA256 signatures openshift/cluster-update-keys#57

Merged

		// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'ExactRepository') ? has(self.exactRepository) : true",message="must set exactRepository if matchPolicy is ExactRepository"
		// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'RemapIdentity') ? has(self.remapIdentity) : true",message="must set remapIdentity if matchPolicy is RemapIdentity"


		### Graduation Criteria

		Before GA, we should explore relocating CRD validations to the admission webhook to ensure the scopes are not under OCP image payload repository are configured.

-Before GA, we should explore relocating CRD validations to the admission webhook to ensure the scopes are not under OCP image payload repository are configured.
+Before GA, validation will be implemented to make sure that scopes cannot be committed to an ImagePolicy that are under the scope of a ClusterImagePolicy, this will prevent conflicts between cluster scoped and namespace scoped policy and prevent namespaces attempting to override the global policy.
+Additionally, it will be validated that neither an ImagePolicy nor ClusterImagePolicy sets a scope that could conflict with pulling from an OpenShift image payload repository.

[OCPNODE-1639] Add ClusterImagePolicy, ImagePolicy to support signature verification #1402

[OCPNODE-1639] Add ClusterImagePolicy, ImagePolicy to support signature verification #1402

Conversation

QiWang19 commented May 17, 2023 • edited Loading

QiWang19 commented May 17, 2023

QiWang19 commented May 18, 2023

saschagrunert left a comment • edited Loading

Choose a reason for hiding this comment

QiWang19 commented May 22, 2023

mtrmac left a comment

Choose a reason for hiding this comment

yuqi-zhang 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

QiWang19 commented Oct 13, 2023

QiWang19 commented Oct 16, 2023

JoelSpeed 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

QiWang19 commented Oct 19, 2023

JoelSpeed Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

JoelSpeed commented Oct 20, 2023

QiWang19 commented Oct 20, 2023

saschagrunert commented Oct 23, 2023

JoelSpeed commented Oct 23, 2023

mrunalp commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

QiWang19 commented May 17, 2023 •

edited

Loading

saschagrunert left a comment •

edited

Loading

JoelSpeed Oct 19, 2023 •

edited

Loading