OCPNODE-2339: Add PKI field to (cluster)imagepolicy #2088

QiWang19 · 2024-11-07T21:12:03Z

Add PKI to clusterimagepolicy with DevPreview featuregate SigstoreImageVerificationPKI.
This field allows verification of image signature created by cosign BYOPKI feature.
The enhancement: openshift/enhancements@3b62d2f
hold this PR before containers/image#2579

openshift-ci · 2024-11-07T21:12:07Z

Hello @QiWang19! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci-robot · 2024-11-07T21:13:14Z

@QiWang19: This pull request references OCPNODE-2339 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

QiWang19 · 2024-11-08T18:03:25Z

/test all

QiWang19 · 2024-11-08T19:03:49Z

/test verify-crd-schema

QiWang19 · 2024-11-08T19:04:16Z

/test integration

QiWang19 · 2024-11-08T19:30:26Z

/test all

QiWang19 · 2024-11-08T21:48:26Z

/test verify-crd-schema

QiWang19 · 2024-11-08T21:48:37Z

/test integration

QiWang19 · 2024-11-09T17:28:16Z

/test verify-crd-schema

QiWang19 · 2024-11-09T17:28:24Z

/test integration

wking · 2024-11-13T16:56:26Z

config/v1alpha1/types_image_policy.go

 type PolicyType string

 const (
 	PublicKeyRootOfTrust         PolicyType = "PublicKey"
 	FulcioCAWithRekorRootOfTrust PolicyType = "FulcioCAWithRekor"
+	PKIRootOfTrust               PolicyType = "PKI"


Is there some way to make this enum-entry gated too? It's currently getting added to the tech-preview CRD manifest:

"FulcioCAWithRekor" indicates that the policy is based on the Fulcio certification and incorporates a Rekor verification. "PKI" is a DevPreview feature that indicates that the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI). enum: - PublicKey - FulcioCAWithRekor - PKI

QiWang19 · 2024-11-15T17:17:48Z

/test integration

QiWang19 · 2024-11-18T17:12:43Z

/test integration

QiWang19 · 2024-11-18T19:15:15Z

/test integration

QiWang19 · 2024-11-18T22:07:47Z

/test integration

QiWang19 · 2024-11-18T23:07:42Z

/test integration

QiWang19 · 2024-12-08T23:41:37Z

/test integration

QiWang19 · 2024-12-09T01:28:13Z

/test integration

QiWang19 · 2024-12-09T05:22:53Z

@JoelSpeed could you help take a look? I am not sure the integration test failures: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_api/2088/pull-ci-openshift-api-master-integration/1865931550843998208

...ig/v1alpha1/tests/clusterimagepolicies.config.openshift.io/SigstoreImageVerificationPKI.yaml

JoelSpeed · 2024-12-10T17:39:12Z

config/v1alpha1/types_image_policy.go

+	// +kubebuilder:validation:XValidation:rule="string(self).startsWith('-----BEGIN CERTIFICATE-----')",message="the caRootsData must start with base64 encoding of '-----BEGIN CERTIFICATE-----'."
+	// +kubebuilder:validation:XValidation:rule="string(self).endsWith('-----END CERTIFICATE-----\\n') || string(self).endsWith('-----END CERTIFICATE-----')",message="the caRootsData must end with base64 encoding of '-----END CERTIFICATE-----'."


Nice validation addition, thanks

config/v1alpha1/types_image_policy.go

JoelSpeed · 2024-12-10T17:42:51Z

config/v1alpha1/types_image_policy.go

+	// caRootsData contains base64-encoded data of a certificate bundle PEM file, which contains one or more CA roots in the PEM format. The total length of the data must not exceed 8192 bytes.
+	// +required
+	// +kubebuilder:validation:MaxLength=8192
+	// +kubebuilder:validation:XValidation:rule="string(self).startsWith('-----BEGIN CERTIFICATE-----')",message="the caRootsData must start with base64 encoding of '-----BEGIN CERTIFICATE-----'."


Potentially want a check that you have the same number of begin and end certificate lines?

self.findAll('-----BEGIN CERTIFICATE-----').size() == self.findAll('-----END CERTIFICATE-----').size()

config/v1alpha1/types_image_policy.go

JoelSpeed · 2024-12-10T17:44:51Z

config/v1alpha1/types_image_policy.go

+	// +kubebuilder:validation:XValidation:rule="string(self).endsWith('-----END CERTIFICATE-----\\n') || string(self).endsWith('-----END CERTIFICATE-----')",message="the caRootsData must end with base64 encoding of '-----END CERTIFICATE-----'."
+	CertificateAuthorityRootsData []byte `json:"caRootsData"`
+	// caIntermediatesData contains base64-encoded data of a certificate bundle PEM file, which contains one or more intermediate certificates in the PEM format. The total length of the data must not exceed 8192 bytes.
+	// caIntermediatesData requires CertificateAuthorityRoots to be set.


Validate this with CEL

has(self.caIntermediatesData) ? has(self.caRootsData) : true

Also, you are using the go type name for CertificateAuthorityRoots, use the serialised version

I tested it a bit, maybe it's not necessary? it seems we can not trigger validation if required caRootsData hasn't being set

spec: scopes: - example.com policy: rootOfTrust: policyType: PKI pki: caRootsData: **** pkiCertificateSubject: email: [email protected] hostname: invalid-email The ClusterImagePolicy "pki-policy" is invalid: * spec.policy.rootOfTrust.pki.caRootsData: Required value * <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation

You're right, caRootsData is required so this isn't needed, good point

JoelSpeed

Looking pretty good, would like the email and hostname godoc updating with validation requirements and one more test for the new validation, then I think this is good to go

JoelSpeed · 2024-12-12T12:11:34Z

config/v1alpha1/types_image_policy.go

+	// +kubebuilder:validation:MaxLength:=320
+	// +kubebuilder:validation:XValidation:rule=`self.matches('^\\S+@\\S+$')`,message="invalid email address in pkiCertificateSubject"
+	Email string `json:"email,omitempty"`
+	// hostname specifies the expected hostname imposed on the subject to which the certificate was issued, and it must match the hostname listed in the Subject Alternative Name (SAN) DNS field of the certificate.


Suggested change

// hostname specifies the expected hostname imposed on the subject to which the certificate was issued, and it must match the hostname listed in the Subject Alternative Name (SAN) DNS field of the certificate.

// hostname specifies the expected hostname imposed on the subject to which the certificate was issued, and it must match the hostname listed in the Subject Alternative Name (SAN) DNS field of the certificate.

// The hostname should be a valid dns 1123 subdomain name, optionally prefixed by '*.', and at most 253 characters in length.

// It should consist only of lowercase alphanumeric characters, hyphens, periods and the optional preceding asterisk.

JoelSpeed · 2024-12-12T12:12:18Z

config/v1alpha1/types_image_policy.go

+// +kubebuilder:validation:XValidation:rule="has(self.email) || has(self.hostname)", message="at least one of email or hostname must be set in pkiCertificateSubject"
+// +openshift:enable:FeatureGate=SigstoreImageVerificationPKI
+type PKICertificateSubject struct {
+	// email specifies the expected email address imposed on the subject to which the certificate was issued, and must match the email address listed in the Subject Alternative Name (SAN) field of the certificate.


How would you explain the validation requirements of this field? Must be a valid email address and be at most 320 characters in length?

JoelSpeed · 2024-12-12T12:12:47Z

config/v1alpha1/types_image_policy.go

+	// +kubebuilder:validation:MaxLength=8192
+	// +kubebuilder:validation:XValidation:rule="string(self).startsWith('-----BEGIN CERTIFICATE-----')",message="the caRootsData must start with base64 encoding of '-----BEGIN CERTIFICATE-----'."
+	// +kubebuilder:validation:XValidation:rule="string(self).endsWith('-----END CERTIFICATE-----\\n') || string(self).endsWith('-----END CERTIFICATE-----')",message="the caRootsData must end with base64 encoding of '-----END CERTIFICATE-----'."
+	// +kubebuilder:validation:XValidation:rule="string(self).findAll('-----BEGIN CERTIFICATE-----').size() == string(self).findAll('-----END CERTIFICATE-----').size()",message="caRootsData must be base64 encoding of valid PEM format data contain the same number of '-----BEGIN CERTIFICATE-----' and '-----END CERTIFICATE-----' markers."


Please make sure this validation is tested

…olicy Add PKI to clusterimagepolicy with DevPreview featuregate SigstoreImageVerificationPKI. This field allow verification of image signature created by cosign BYOPKI feature. Signed-off-by: Qi Wang <[email protected]>

openshift-ci · 2024-12-13T20:43:44Z

@QiWang19: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-serial	`f2c0387`	link	true	`/test e2e-aws-serial`
ci/prow/okd-scos-e2e-aws-ovn	`f2c0387`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-serial-techpreview	`f2c0387`	link	true	`/test e2e-aws-serial-techpreview`

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

JoelSpeed · 2024-12-15T10:46:24Z

/lgtm
/override ci/prow/verify-crd-schema

New required fields are within an optional struct, so are valid. This is an error in the detection.

openshift-ci · 2024-12-15T10:47:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, QiWang19

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

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

openshift-ci · 2024-12-15T10:47:20Z

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to this:

/lgtm
/override ci/prow/verify-crd-schema

New required fields are within an optional struct, so are valid. This is an error in the detection.

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.

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 7, 2024

openshift-ci bot requested review from knobunc and sjenning November 7, 2024 21:12

QiWang19 changed the title ~~Add PKI field to (cluster)imagepolicy~~ OCPNODE-2339: Add PKI field to (cluster)imagepolicy Nov 7, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 7, 2024

QiWang19 marked this pull request as draft November 8, 2024 16:41

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 Nov 8, 2024

QiWang19 force-pushed the byopki-api branch from 2b20927 to 3ad0c06 Compare November 8, 2024 18:03

QiWang19 force-pushed the byopki-api branch from 7e78ef6 to 4eac6f9 Compare November 9, 2024 17:27

wking reviewed Nov 13, 2024

View reviewed changes

QiWang19 force-pushed the byopki-api branch from 4a82b16 to 536d063 Compare November 18, 2024 05:26

QiWang19 force-pushed the byopki-api branch 2 times, most recently from e43b2c9 to 1a30d73 Compare November 18, 2024 19:14

QiWang19 force-pushed the byopki-api branch from 1a30d73 to 36753e7 Compare November 18, 2024 22:07

QiWang19 force-pushed the byopki-api branch from 36753e7 to 3f7784d Compare November 18, 2024 23:07

QiWang19 force-pushed the byopki-api branch 7 times, most recently from 151a1ee to 6dae8df Compare December 8, 2024 23:17

QiWang19 marked this pull request as draft December 8, 2024 23:40

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 Dec 8, 2024

QiWang19 force-pushed the byopki-api branch from 6dae8df to 800e4ba Compare December 8, 2024 23:41

QiWang19 force-pushed the byopki-api branch from 800e4ba to de24f05 Compare December 9, 2024 01:13

QiWang19 marked this pull request as ready for review December 9, 2024 01:28

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 Dec 9, 2024

JoelSpeed reviewed Dec 10, 2024

View reviewed changes

...ig/v1alpha1/tests/clusterimagepolicies.config.openshift.io/SigstoreImageVerificationPKI.yaml Outdated Show resolved Hide resolved

JoelSpeed reviewed Dec 10, 2024

View reviewed changes

QiWang19 force-pushed the byopki-api branch 2 times, most recently from 926b445 to 2a9ff48 Compare December 11, 2024 22:48

JoelSpeed reviewed Dec 12, 2024

View reviewed changes

QiWang19 force-pushed the byopki-api branch from 2a9ff48 to f2c0387 Compare December 13, 2024 16:30

openshift-ci bot assigned JoelSpeed Dec 15, 2024

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPNODE-2339: Add PKI field to (cluster)imagepolicy #2088

OCPNODE-2339: Add PKI field to (cluster)imagepolicy #2088

QiWang19 commented Nov 7, 2024 •

edited

Loading

openshift-ci bot commented Nov 7, 2024

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 9, 2024

QiWang19 commented Nov 9, 2024

wking Nov 13, 2024

QiWang19 commented Nov 15, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Dec 8, 2024

QiWang19 commented Dec 9, 2024

QiWang19 commented Dec 9, 2024

JoelSpeed Dec 10, 2024

JoelSpeed Dec 10, 2024

JoelSpeed Dec 10, 2024

QiWang19 Dec 11, 2024

JoelSpeed Dec 12, 2024

JoelSpeed left a comment

JoelSpeed Dec 12, 2024

JoelSpeed Dec 12, 2024

JoelSpeed Dec 12, 2024

openshift-ci bot commented Dec 13, 2024 •

edited

Loading

JoelSpeed commented Dec 15, 2024

openshift-ci bot commented Dec 15, 2024

openshift-ci bot commented Dec 15, 2024

		// +kubebuilder:validation:XValidation:rule="string(self).startsWith('-----BEGIN CERTIFICATE-----')",message="the caRootsData must start with base64 encoding of '-----BEGIN CERTIFICATE-----'."
		// +kubebuilder:validation:XValidation:rule="string(self).endsWith('-----END CERTIFICATE-----\\n') \|\| string(self).endsWith('-----END CERTIFICATE-----')",message="the caRootsData must end with base64 encoding of '-----END CERTIFICATE-----'."

OCPNODE-2339: Add PKI field to (cluster)imagepolicy #2088

Are you sure you want to change the base?

OCPNODE-2339: Add PKI field to (cluster)imagepolicy #2088

Conversation

QiWang19 commented Nov 7, 2024 • edited Loading

openshift-ci bot commented Nov 7, 2024

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 8, 2024

QiWang19 commented Nov 9, 2024

QiWang19 commented Nov 9, 2024

Choose a reason for hiding this comment

QiWang19 commented Nov 15, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Nov 18, 2024

QiWang19 commented Dec 8, 2024

QiWang19 commented Dec 9, 2024

QiWang19 commented Dec 9, 2024

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

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

openshift-ci bot commented Dec 13, 2024 • edited Loading

JoelSpeed commented Dec 15, 2024

openshift-ci bot commented Dec 15, 2024

openshift-ci bot commented Dec 15, 2024

QiWang19 commented Nov 7, 2024 •

edited

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Dec 13, 2024 •

edited

Loading