[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy #1457

QiWang19 · 2023-05-10T03:05:49Z

Add ImagePolicy struct of ImagePolicy CRD for image sigstore signature verification.
enhancement: openshift/enhancements#1402
Epic: https://issues.redhat.com/browse/OCPNODE-1628

openshift-ci · 2023-05-10T03:05:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2023-05-10T03:05:55Z

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.

config/v1alpha1/0000_10_config-operator_01_imagepolicy.crd.yaml

QiWang19 · 2023-05-23T04:31:30Z

@saschagrunert could you review the api?

config/v1alpha1/types_image_policy.go

JoelSpeed · 2023-11-23T14:46:15Z

config/v1alpha1/types_image_policy.go

+
+// PublicKey defines the root of trust based on a sigstore public key.
+type PublicKey struct {
+	// keyData contains inline base64 encoded data of the public key.


This one didn't get PEM encoding change

JoelSpeed · 2023-11-23T14:46:29Z

config/v1alpha1/types_image_policy.go

+	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:MaxLength=8192
+	KeyData string `json:"keyData"`
+	// rekorKeyData contains inline base64 data of the Rekor public key.


This should mention pem format

JoelSpeed · 2023-11-23T14:47:19Z

config/v1alpha1/types_image_policy.go

+	// oidcIssuer contains the expected OIDC issuer.
+	// Example: "https://expected.OIDC.issuer/"


Are we expecting the OIDC issuer to expose the well known endpoint under this path?

JoelSpeed · 2023-11-23T14:51:24Z

I think the OIDC issuer field needs a little more depth to the godoc there. I assume, since I'm semi familiar with OIDC, that the expectation is that the URL will be prepended to /.well-known/openid-configuration before making a request to discover the endpoints, right?

In that case I'd want to mention that the issue URL should be the full URL including any subpath that the issuer is hosted on and that the issuer URL will be used to discover jwt keys or whatever it's being used for

QiWang19 · 2023-11-27T14:37:55Z

Please remove the second commit, we should not be merging the types without the CRD and tests. What is the motivation for the second commit? Is this feature going to land in 4.15?

I am not sure the implementation for these CRDs can be finished before the feature freeze. The second commit can have the API built and we can develop and test the CRD functions on the cluster.
we can get the second commit back, is it ok to have the CRD registered as techpreview in 4.15, and not have underlying support for it?

QiWang19 · 2023-11-27T15:05:19Z

we have merged the feature gate #1470 for this API.

QiWang19 · 2023-11-27T15:37:47Z

I think the OIDC issuer field needs a little more depth to the godoc there. I assume, since I'm semi familiar with OIDC, that the expectation is that the URL will be prepended to /.well-known/openid-configuration before making a request to discover the endpoints, right?

In that case I'd want to mention that the issue URL should be the full URL including any subpath that the issuer is hosted on and that the issuer URL will be used to discover jwt keys or whatever it's being used for

@mtrmac Could you take a look? We added the check for oidcIssuer to just validate it is a valid URL. I am not sure if we can add other specific validation for it. I don't see explicit restrictions from the doc https://docs.sigstore.dev/certificate_authority/oidc-in-fulcio/#uri

mtrmac · 2023-11-27T18:52:33Z

I think the OIDC issuer field needs a little more depth to the godoc there. I assume, since I'm semi familiar with OIDC, that the expectation is that the URL will be prepended to /.well-known/openid-configuration before making a request to discover the endpoints, right?

…

… and that the issuer URL will be used to discover jwt keys or whatever it's being used for

It’s that kind of value, yes, but not necessarily precisely that one, and nothing is going to reach out to the URL. Verification happens off-line.

What is actually verified is that the Fulcio-issued certificate contains a (Fulcio-defined) certificate extension pointing at the user-specified URL. And when Fulcio issues certificates, it includes a value based on an URL inside the client-provided ID token. It is usually the "iss" field from the ID token, but there is a mapping mechanism, activated in the default configuration https://github.com/sigstore/fulcio/blob/4230267565c9c3082302cdca62b6bf612db11bd4/pkg/config/config.go#L397 .

So the closest direct spec is the iss field in https://openid.net/specs/openid-connect-core-1_0.html#IDToken ; but note https://openid.net/specs/openid-connect-core-1_0.html#GoogleIss .

c/image does not do any validation of the string, and requires a (byte-for-byte) exact match between the configuration and the certificate extension field. So I would caution against any effort to normalize the users’ input.

(I’m afraid I haven’t been paying close attention to this PR.)

QiWang19 · 2023-11-28T02:28:43Z

/test e2e-azure

QiWang19 · 2023-11-28T14:04:50Z

@JoelSpeed PTAL

JoelSpeed · 2023-11-28T14:32:11Z

@mtrmac Would it be fair then to add to the oidcIssuer field an expanded comment that explains what you've just said? That this should typically match the issuer field in the ...

That way users might have an idea of where they may be able to find this value or verify the value matches some other source at the least

mtrmac · 2023-11-28T14:57:18Z

The c/image documentation just says

Both oidcIssuer and subjectEmail are mandatory,
exactly specifying the expected identity provider, and the identity of the user obtaining the Fulcio certificate.

I don’t have a strong opinion on whether the details of the field mapping are worth emphasizing.

In the worst case, the signature enforcement currently fails with "Unexpected Fulcio OIDC issuer %q" on this code path, so users learn what value to use that way.

Add API for ClusterImagePolicy, ImagePolicy CRD Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2023-11-29T03:29:59Z

@JoelSpeed I updated the comment for oidcIssuer, PTAL

JoelSpeed

/lgtm

openshift-ci · 2023-12-12T12:30:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, QiWang19, 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~~ [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 · 2023-12-12T15:17:10Z

@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-bot · 2023-12-13T07:00:13Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.16.0-202312130634.p0.gef62af0.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

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 10, 2023

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 10, 2023

QiWang19 force-pushed the draft-sigstore branch from 419efd4 to c804735 Compare May 23, 2023 04:25

QiWang19 changed the title ~~[WIP] Add ClusterImageSigPolicy~~ Add ClusterImageSigPolicy May 23, 2023

QiWang19 commented May 23, 2023

View reviewed changes

config/v1alpha1/0000_10_config-operator_01_imagepolicy.crd.yaml Outdated Show resolved Hide resolved

QiWang19 marked this pull request as ready for review May 23, 2023 04:30

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 23, 2023

openshift-ci bot requested review from adambkaplan and mfojtik May 23, 2023 04:30

saschagrunert reviewed May 23, 2023

View reviewed changes

config/v1alpha1/types_image_policy.go Outdated Show resolved Hide resolved

config/v1alpha1/types_image_policy.go Outdated Show resolved Hide resolved

QiWang19 changed the title ~~Add ClusterImageSigPolicy~~ Add ImageSigPolicy May 26, 2023

QiWang19 changed the title ~~Add ImageSigPolicy~~ Add ImagePolicy May 26, 2023

QiWang19 force-pushed the draft-sigstore branch 4 times, most recently from 484198d to eb77406 Compare June 2, 2023 11:09

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 2, 2023

QiWang19 force-pushed the draft-sigstore branch from eb77406 to 1a45b5e Compare June 4, 2023 04:14

QiWang19 force-pushed the draft-sigstore branch 6 times, most recently from 2f22c0b to 0d6b7b0 Compare July 1, 2023 01:24

QiWang19 changed the title ~~Add ImagePolicy~~ [OCPNODE-1671] Add ImagePolicy Jul 3, 2023

QiWang19 force-pushed the draft-sigstore branch from 0d6b7b0 to ac546af Compare July 3, 2023 20:57

JoelSpeed reviewed Nov 23, 2023

View reviewed changes

QiWang19 force-pushed the draft-sigstore branch from a5af136 to 797d09e Compare November 27, 2023 21:05

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 27, 2023

QiWang19 force-pushed the draft-sigstore branch from 797d09e to 02db01b Compare November 27, 2023 21:14

Add ClusterImage,ImagePolicy CRD

0412357

Add API for ClusterImagePolicy, ImagePolicy CRD Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the draft-sigstore branch from 02db01b to 0412357 Compare November 28, 2023 20:31

JoelSpeed reviewed Dec 12, 2023

View reviewed changes

openshift-ci bot assigned JoelSpeed Dec 12, 2023

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

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

openshift-merge-bot bot merged commit ef62af0 into openshift:master Dec 12, 2023
16 checks passed

This was referenced Dec 12, 2023

OCPNODE-1671: Update openshift/api to add imagepolicy api openshift/client-go#262

Merged

OCPNODE-1671: Update api,client-go to add imagepolicy api openshift/cluster-config-operator#376

Merged

This was referenced Jan 4, 2024

Add imagepolicy crd to payload #1715

Closed

Register sigstore crds openshift/machine-config-operator#4095

Closed

QiWang19 mentioned this pull request Jan 12, 2024

Add ImagePolicyList and ClusterImagePolicyList to scheme known types #1726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy #1457

[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy #1457

QiWang19 commented May 10, 2023 •

edited

Loading

openshift-ci bot commented May 10, 2023

openshift-ci bot commented May 10, 2023

QiWang19 commented May 23, 2023

JoelSpeed Nov 23, 2023

JoelSpeed Nov 23, 2023

JoelSpeed Nov 23, 2023

JoelSpeed commented Nov 23, 2023

QiWang19 commented Nov 27, 2023

QiWang19 commented Nov 27, 2023 •

edited

Loading

QiWang19 commented Nov 27, 2023

mtrmac commented Nov 27, 2023

QiWang19 commented Nov 28, 2023

QiWang19 commented Nov 28, 2023

JoelSpeed commented Nov 28, 2023

mtrmac commented Nov 28, 2023

QiWang19 commented Nov 29, 2023

JoelSpeed left a comment

openshift-ci bot commented Dec 12, 2023

openshift-ci bot commented Dec 12, 2023

openshift-bot commented Dec 13, 2023

		// oidcIssuer contains the expected OIDC issuer.
		// Example: "https://expected.OIDC.issuer/"

[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy #1457

[OCPNODE-1671] Add ClusterImagePolicy and ImagePolicy #1457

Conversation

QiWang19 commented May 10, 2023 • edited Loading

openshift-ci bot commented May 10, 2023

openshift-ci bot commented May 10, 2023

QiWang19 commented May 23, 2023

JoelSpeed Nov 23, 2023

Choose a reason for hiding this comment

JoelSpeed Nov 23, 2023

Choose a reason for hiding this comment

JoelSpeed Nov 23, 2023

Choose a reason for hiding this comment

JoelSpeed commented Nov 23, 2023

QiWang19 commented Nov 27, 2023

QiWang19 commented Nov 27, 2023 • edited Loading

QiWang19 commented Nov 27, 2023

mtrmac commented Nov 27, 2023

QiWang19 commented Nov 28, 2023

QiWang19 commented Nov 28, 2023

JoelSpeed commented Nov 28, 2023

mtrmac commented Nov 28, 2023

QiWang19 commented Nov 29, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 12, 2023

openshift-ci bot commented Dec 12, 2023

openshift-bot commented Dec 13, 2023

QiWang19 commented May 10, 2023 •

edited

Loading

QiWang19 commented Nov 27, 2023 •

edited

Loading