OCPNODE-2439: Add support for BYOPKI#1658
OCPNODE-2439: Add support for BYOPKI#1658openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
Conversation
88eaf2b to
894cfae
Compare
mtrmac
left a comment
There was a problem hiding this comment.
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
CertificateIdentityEntityname 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.)
|
@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. DetailsIn 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. |
|
@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. DetailsIn 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. |
mtrmac
left a comment
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
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
Show resolved
Hide resolved
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
Show resolved
Hide resolved
enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md
Show resolved
Hide resolved
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
enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md
Outdated
Show resolved
Hide resolved
|
@mtrmac thanks for those comments, I have updated the enhancement with those suggestions. |
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
mtrmac
left a comment
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
enhancements/api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md
Outdated
Show resolved
Hide resolved
Yes, the current plan is to make subject matching mandatory and consider Rekor at later point in time. |
|
@mtrmac Thanks for your extensive review. |
|
|
||
| // PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates. | ||
| // +optional | ||
| PKI *PKI `json:"pki,omitempty"` |
There was a problem hiding this comment.
nit: let's fix the indentation here.
| // 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"` |
There was a problem hiding this comment.
how about just using CertificateSubject since we're already in the PKI struct?
| PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"` | |
| CertificateSubject *CertificateSubject `json:"certificateSubject,omitempty"` |
There was a problem hiding this comment.
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
jsonfield names matter, and we can rely on the nesting structure to avoid repetition. SocertificateSubjectis better thanpkiCertificateSubject - Go programmers using the API package: all types are at the top level, so they should be unique standalone. So
PKICertificateSubjectis better thanCertificateSubject, or maybe it should bePKIImagePolicyCertificateSubjectin the extreme??- Note especially that the naming the top-level structure just
PKIis not ideal… we can get away with that forFulcioCAWithRekor, but forPKIand the existingPublicKey… there’s some risk of clashes.
- Note especially that the naming the top-level structure just
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.
There was a problem hiding this comment.
| PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"` | |
| PKICertificateSubject *PKICertificateSubject `json:"certificateSubject,omitempty"` |
Maybe only omit the PKI from the json annotation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // +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. |
There was a problem hiding this comment.
Let's add a note what this field does.
|
@mrunalp PTAL, too |
Signed-off-by: Harshal Patil <harpatil@redhat.com>
|
@QiWang19 PTAL as well. |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@harche: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This PR modifies existing image policy enhancement for image verification to add support for BYOPKI use case.