OCPNODE-2439: Add support for BYOPKI #1658

harche · 2024-08-01T14:55:06Z

This PR modifies existing image policy enhancement for image verification to add support for BYOPKI use case.

mtrmac

A very brief skim:

ACK on the feature set, no concerns on security. Not having wildcards simplifies things.
I think “BYOPKI” is a possible name but not what I’d expect. “CertificateRootOfTrust” (my weak preference) or at most “PKIRootOfTrust” seems better from a quick guess.
WRT the “chain” field… I would very weakly prefer to not have that, so that users have to explicitly decide what is the root of trust and what are intermediates; that would eliminate a class of mistakes. I can well see an argument that it is more of an inconvenience than a security improvement.
The CertificateIdentityEntity name seems not ideal… “entity” means nothing, and maybe the type name should be more specific to pollute the wider namespace less.
(I didn’t carefully review details of the comments nor names of the objects otherwise.)

openshift-ci-robot · 2024-08-14T13:55:38Z

@harche: This pull request references OCPNODE-2439 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.

openshift-ci-robot · 2024-08-14T13:56:40Z

@harche: This pull request references OCPNODE-2439 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:

This PR modifies existing image policy enhancement for image verification to add support for BYOPKI use case.

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.

mtrmac

Primarily note the question whether certificate subject matching should be mandatory vs. whether we need to suport Rekor for trusted timestamps.

(AFAIK we didn’t get a detailed response from one of the customers motivating this work…)

The other comments are more about the mechanism of the thing.

A generic reminder: I don’t (currently) understand K8S API guidelines / restrictions, that either needs a separate expert reviewer, or I need to learn.

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

harche · 2024-08-28T20:18:36Z

@mtrmac thanks for those comments, I have updated the enhancement with those suggestions.

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

mtrmac

Thanks!

From my POV the primary outstanding question is the mandatory subject matching vs. Rekor feature requirements.

To confirm, is the current plan to make subject matching mandatory, and not add Rekor (yet)?

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

harche · 2024-08-29T19:37:25Z

Thanks!

From my POV the primary outstanding question is the mandatory subject matching vs. Rekor feature requirements.

To confirm, is the current plan to make subject matching mandatory, and not add Rekor (yet)?

Yes, the current plan is to make subject matching mandatory and consider Rekor at later point in time.

harche · 2024-08-29T19:53:24Z

@mtrmac Thanks for your extensive review.

saschagrunert · 2024-09-02T08:35:11Z

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

+
+    // PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates.
+    // +optional
+    PKI *PKI `json:"pki,omitempty"`


nit: let's fix the indentation here.

updated. thanks.

saschagrunert · 2024-09-02T08:37:00Z

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

+	// PKICertificateSubject must be set if any of the other fields are set.
+	// +required
+  // +kubebuilder:validation:XValidation:rule="self.CertificateAuthorityRootsData == '' || self.PKICertificateSubject != nil", message="pkiCertificateSubject must be set if caRootsData is specified"
+	PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`


how about just using CertificateSubject since we're already in the PKI struct?

Suggested change

PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`

CertificateSubject *CertificateSubject `json:"certificateSubject,omitempty"`

@mtrmac WDYT?

As far as correctness goes, sure, why not.

One concern to think about is that there are two audiences here:

Users writing YAML: for them the json field names matter, and we can rely on the nesting structure to avoid repetition. So certificateSubject is better than pkiCertificateSubject

Go programmers using the API package: all types are at the top level, so they should be unique standalone. So PKICertificateSubject is better than CertificateSubject, or maybe it should be PKIImagePolicyCertificateSubject in the extreme??

Note especially that the naming the top-level structure just PKI is not ideal… we can get away with that for FulcioCAWithRekor, but for PKI and the existing PublicKey… there’s some risk of clashes.

But, also, the YAML names are more important because they are user-facing; the Go type names are ~internal in a package with no stable API commitment (I think?) so if we ever encountered a conflict, we can rename things.

Suggested change

PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`

PKICertificateSubject *PKICertificateSubject `json:"certificateSubject,omitempty"`

Maybe only omit the PKI from the json annotation.

Note especially that the naming the top-level structure just PKI is not ideal… we can get away with that for FulcioCAWithRekor, but for PKI and the existing PublicKey… there’s some risk of clashes.

will CustomPKI instead of PKI make a difference? we can explain in the comment this means a PKI not from Sigstore.

In the context of openshift/api/config/v1alpha1 (or, worse, eventually, …/v1, I think PKI and CustomPKI are very similarly likely to clash — if there is going to be a clash, it’s going to be with some other non-policy.json PKI use.

See https://pkg.go.dev/github.com/openshift/api/config/v1 — there are things like NamedCertificate and CertInfo.

So I’d be tempted to use ImagePolicy* for all types (ImagePolicyPKI?), but that might well be an overreaction.

I do think the JSON/YAML field names are much more important; I’m not nearly as worried about the Go types.

saschagrunert · 2024-09-02T08:38:38Z

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

+	// +kubebuilder:validation:XValidation:rule="self == '' || self.CertificateAuthorityRootsData != ''", message="caIntermediatesData requires caRootsData to be set"
+	CertificateAuthorityIntermediatesData []byte `json:"caIntermediatesData,omitempty"`
+
+	// PKICertificateSubject must be set if any of the other fields are set.


Let's add a note what this field does.

Updated, thanksl

saschagrunert · 2024-09-02T08:38:52Z

@mrunalp PTAL, too

Signed-off-by: Harshal Patil <harpatil@redhat.com>

saschagrunert · 2024-09-03T15:11:03Z

@QiWang19 PTAL as well.

mrunalp

/lgtm

openshift-ci · 2024-09-19T15:38:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp

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 · 2024-09-19T16:06:20Z

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

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 Aug 1, 2024

harche marked this pull request as draft August 1, 2024 14:55

openshift-ci bot requested review from cybertron and tjungblu August 1, 2024 14:56

harche force-pushed the byopki branch 2 times, most recently from 88eaf2b to 894cfae Compare August 1, 2024 15:27

mtrmac reviewed Aug 5, 2024

View reviewed changes

harche force-pushed the byopki branch from 894cfae to a5ca039 Compare August 14, 2024 13:53

harche changed the title ~~WIP: Add support for BYOPKI~~ Add support for BYOPKI Aug 14, 2024

harche changed the title ~~Add support for BYOPKI~~ OCPNODE-2439: Add support for BYOPKI Aug 14, 2024

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

harche marked this pull request as ready for review August 14, 2024 13:56

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 Aug 14, 2024

openshift-ci bot requested review from celebdor and hasbro17 August 14, 2024 13:58

mtrmac reviewed Aug 28, 2024

View reviewed changes

harche force-pushed the byopki branch from db53510 to 48c1ba0 Compare August 28, 2024 20:15

wking reviewed Aug 28, 2024

View reviewed changes

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md Outdated Show resolved Hide resolved

harche force-pushed the byopki branch from 48c1ba0 to 9319d9e Compare August 29, 2024 13:35

QiWang19 reviewed Aug 29, 2024

View reviewed changes

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md Outdated Show resolved Hide resolved

QiWang19 force-pushed the byopki branch from 9319d9e to 2ebf1ec Compare August 29, 2024 18:52

mtrmac reviewed Aug 29, 2024

View reviewed changes

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md Outdated Show resolved Hide resolved

enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md Outdated Show resolved Hide resolved

harche force-pushed the byopki branch from 2ebf1ec to e0a144c Compare August 29, 2024 19:49

saschagrunert reviewed Sep 2, 2024

View reviewed changes

Add support for BYOPKI

3b62d2f

Signed-off-by: Harshal Patil <harpatil@redhat.com>

harche force-pushed the byopki branch from e0a144c to 3b62d2f Compare September 3, 2024 15:00

mrunalp approved these changes Sep 19, 2024

View reviewed changes

openshift-ci bot assigned mrunalp Sep 19, 2024

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

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

openshift-merge-bot bot merged commit 58f0eaa into openshift:master Sep 19, 2024
2 checks passed

mtrmac mentioned this pull request Sep 20, 2024

policy.json BYOPKI signature verification API containers/image#2579

Open

QiWang19 mentioned this pull request Dec 8, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPNODE-2439: Add support for BYOPKI #1658

OCPNODE-2439: Add support for BYOPKI #1658

harche commented Aug 1, 2024 •

edited

Loading

mtrmac left a comment

openshift-ci-robot commented Aug 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 14, 2024 •

edited by openshift-ci bot

Loading

mtrmac left a comment

harche commented Aug 28, 2024

mtrmac left a comment

harche commented Aug 29, 2024

harche commented Aug 29, 2024

saschagrunert Sep 2, 2024

harche Sep 3, 2024

saschagrunert Sep 2, 2024

harche Sep 3, 2024

mtrmac Sep 5, 2024

QiWang19 Sep 5, 2024

QiWang19 Sep 5, 2024

mtrmac Sep 6, 2024

saschagrunert Sep 2, 2024

harche Sep 3, 2024

saschagrunert commented Sep 2, 2024

saschagrunert commented Sep 3, 2024

mrunalp left a comment

openshift-ci bot commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

	PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`
	CertificateSubject *CertificateSubject `json:"certificateSubject,omitempty"`

OCPNODE-2439: Add support for BYOPKI #1658

OCPNODE-2439: Add support for BYOPKI #1658

Conversation

harche commented Aug 1, 2024 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 14, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Aug 14, 2024 • edited by openshift-ci bot Loading

mtrmac left a comment

Choose a reason for hiding this comment

harche commented Aug 28, 2024

mtrmac left a comment

Choose a reason for hiding this comment

harche commented Aug 29, 2024

harche commented Aug 29, 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

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 commented Sep 2, 2024

saschagrunert commented Sep 3, 2024

mrunalp left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

harche commented Aug 1, 2024 •

edited

Loading

openshift-ci-robot commented Aug 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 14, 2024 •

edited by openshift-ci bot

Loading