Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

52 changes: 51 additions & 1 deletion config/v1alpha1/types_image_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ type Policy struct {
// +union
// +kubebuilder:validation:XValidation:rule="has(self.policyType) && self.policyType == 'PublicKey' ? has(self.publicKey) : !has(self.publicKey)",message="publicKey is required when policyType is PublicKey, and forbidden otherwise"
// +kubebuilder:validation:XValidation:rule="has(self.policyType) && self.policyType == 'FulcioCAWithRekor' ? has(self.fulcioCAWithRekor) : !has(self.fulcioCAWithRekor)",message="fulcioCAWithRekor is required when policyType is FulcioCAWithRekor, and forbidden otherwise"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=SigstoreImageVerificationPKI,rule="has(self.policyType) && self.policyType == 'PKI' ? has(self.pki) : !has(self.pki)",message="pki is required when policyType is PKI, and forbidden otherwise"
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 sigstore publicKey and may optionally use a Rekor verification.
// "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). This value is enabled by turning on the SigstoreImageVerificationPKI feature gate.
// +unionDiscriminator
// +required
PolicyType PolicyType `json:"policyType"`
Expand All @@ -88,14 +90,20 @@ type PolicyRootOfTrust struct {
// https://github.com/sigstore/fulcio and https://github.com/sigstore/rekor
// +optional
FulcioCAWithRekor *FulcioCAWithRekor `json:"fulcioCAWithRekor,omitempty"`
// pki defines the root of trust based on Bring Your Own Public Key Infrastructure (BYOPKI) Root CA(s) and corresponding intermediate certificates.
// +optional
// +openshift:enable:FeatureGate=SigstoreImageVerificationPKI
PKI *PKI `json:"pki,omitempty"`
}

// +kubebuilder:validation:Enum=PublicKey;FulcioCAWithRekor
// +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=PublicKey;FulcioCAWithRekor
// +openshift:validation:FeatureGateAwareEnum:featureGate=SigstoreImageVerificationPKI,enum=PublicKey;FulcioCAWithRekor;PKI
type PolicyType string

const (
PublicKeyRootOfTrust PolicyType = "PublicKey"
FulcioCAWithRekorRootOfTrust PolicyType = "FulcioCAWithRekor"
PKIRootOfTrust PolicyType = "PKI"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// PublicKey defines the root of trust based on a sigstore public key.
Expand Down Expand Up @@ -143,6 +151,48 @@ type PolicyFulcioSubject struct {
SignedEmail string `json:"signedEmail"`
}

// PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates.
type PKI struct {
// 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 characters.
// +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-----'."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

// +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-----'."
Comment on lines +159 to +160
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice validation addition, thanks

// +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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure this validation is tested

CertificateAuthorityRootsData []byte `json:"caRootsData"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why caRootsData as the serialised version instead of certificateAuthorityRootsData?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We chose caRootsData to maintain consistency with cosign cli --ca-roots, --ca-intermediates for the PKI verification, and this follow the existing naming pattern fucilCAData

QiWang19 marked this conversation as resolved.
Show resolved Hide resolved
// 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 characters.
// caIntermediatesData requires caRootsData to be set.
// +optional
// +kubebuilder:validation:XValidation:rule="string(self).startsWith('-----BEGIN CERTIFICATE-----')",message="the caIntermediatesData 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 caIntermediatesData 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="caIntermediatesData must be base64 encoding of valid PEM format data contain the same number of '-----BEGIN CERTIFICATE-----' and '-----END CERTIFICATE-----' markers."
// +kubebuilder:validation:MaxLength=8192
CertificateAuthorityIntermediatesData []byte `json:"caIntermediatesData,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of separating intermediates vs just providing one complete chain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a discussion in the enhancement review where it was decided to explicitly separate intermediates and root data to avoid potential mistakes openshift/enhancements#1658 (review) we did not include the certificate-chain field.


// pkiCertificateSubject defines the requirements imposed on the subject to which the certificate was issued.
// +required
PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject"`
}

// PKICertificateSubject defines the requirements imposed on the subject to which the certificate was issued.
// +kubebuilder:validation:XValidation:rule="has(self.email) || has(self.hostname)", message="at least one of email or hostname must be set in pkiCertificateSubject"
QiWang19 marked this conversation as resolved.
Show resolved Hide resolved
// +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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// The email should be a valid email address and at most 320 characters in length.
// +optional
// +kubebuilder:validation:MaxLength:=320
// +kubebuilder:validation:XValidation:rule=`self.matches('^\\S+@\\S+$')`,message="invalid email address in pkiCertificateSubject"
Email string `json:"email,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a sensible maximum length applying

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set the limit to 320, local-part+domain from https://www.rfc-editor.org/rfc/rfc5321.html#section-4.5.3.1.2

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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.
// +optional
// +kubebuilder:validation:MaxLength:=253
// +kubebuilder:validation:XValidation:rule="self.startsWith('*.') ? !format.dns1123Subdomain().validate(self.replace('*.', '', 1)).hasValue() : !format.dns1123Subdomain().validate(self).hasValue()",message="hostname should be a valid dns 1123 subdomain name, optionally prefixed by '*.'. It should consist only of lowercase alphanumeric characters, hyphens, periods and the optional preceding asterisk."
Hostname string `json:"hostname,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a sensible maximum length. I believe 253 is common for hostnames if they are following kube conventions on subdomains

}

// PolicyIdentity defines image identity the signature claims about the image. When omitted, the default matchPolicy is "MatchRepoDigestOrExact".
// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'ExactRepository') ? has(self.exactRepository) : !has(self.exactRepository)",message="exactRepository is required when matchPolicy is ExactRepository, and forbidden otherwise"
// +kubebuilder:validation:XValidation:rule="(has(self.matchPolicy) && self.matchPolicy == 'RemapIdentity') ? has(self.remapIdentity) : !has(self.remapIdentity)",message="remapIdentity is required when matchPolicy is RemapIdentity, and forbidden otherwise"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,101 @@ spec:
- fulcioSubject
- rekorKeyData
type: object
pki:
description: pki defines the root of trust based on Bring
Your Own Public Key Infrastructure (BYOPKI) Root CA(s) and
corresponding intermediate certificates.
properties:
caIntermediatesData:
description: |-
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 characters.
caIntermediatesData requires caRootsData to be set.
format: byte
maxLength: 8192
type: string
x-kubernetes-validations:
- message: the caIntermediatesData must start with base64
encoding of '-----BEGIN CERTIFICATE-----'.
rule: string(self).startsWith('-----BEGIN CERTIFICATE-----')
- message: the caIntermediatesData must end with base64
encoding of '-----END CERTIFICATE-----'.
rule: string(self).endsWith('-----END CERTIFICATE-----\n')
|| string(self).endsWith('-----END CERTIFICATE-----')
- message: caIntermediatesData must be base64 encoding
of valid PEM format data contain the same number of
'-----BEGIN CERTIFICATE-----' and '-----END CERTIFICATE-----'
markers.
rule: string(self).findAll('-----BEGIN CERTIFICATE-----').size()
== string(self).findAll('-----END CERTIFICATE-----').size()
caRootsData:
description: 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 characters.
format: byte
maxLength: 8192
type: string
x-kubernetes-validations:
- message: the caRootsData must start with base64 encoding
of '-----BEGIN CERTIFICATE-----'.
rule: string(self).startsWith('-----BEGIN CERTIFICATE-----')
- message: the caRootsData must end with base64 encoding
of '-----END CERTIFICATE-----'.
rule: string(self).endsWith('-----END CERTIFICATE-----\n')
|| string(self).endsWith('-----END CERTIFICATE-----')
- message: caRootsData must be base64 encoding of valid
PEM format data contain the same number of '-----BEGIN
CERTIFICATE-----' and '-----END CERTIFICATE-----'
markers.
rule: string(self).findAll('-----BEGIN CERTIFICATE-----').size()
== string(self).findAll('-----END CERTIFICATE-----').size()
pkiCertificateSubject:
description: pkiCertificateSubject defines the requirements
imposed on the subject to which the certificate was
issued.
properties:
email:
description: |-
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.
The email should be a valid email address and at most 320 characters in length.
maxLength: 320
type: string
x-kubernetes-validations:
- message: invalid email address in pkiCertificateSubject
rule: self.matches('^\\S+@\\S+$')
hostname:
description: |-
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.
maxLength: 253
type: string
x-kubernetes-validations:
- message: hostname should be a valid dns 1123 subdomain
name, optionally prefixed by '*.'. It should consist
only of lowercase alphanumeric characters, hyphens,
periods and the optional preceding asterisk.
rule: 'self.startsWith(''*.'') ? !format.dns1123Subdomain().validate(self.replace(''*.'',
'''', 1)).hasValue() : !format.dns1123Subdomain().validate(self).hasValue()'
type: object
x-kubernetes-validations:
- message: at least one of email or hostname must be set
in pkiCertificateSubject
rule: has(self.email) || has(self.hostname)
required:
- caRootsData
- pkiCertificateSubject
type: object
policyType:
description: |-
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 sigstore publicKey and may optionally use a Rekor verification.
"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). This value is enabled by turning on the SigstoreImageVerificationPKI feature gate.
enum:
- PublicKey
- FulcioCAWithRekor
- PKI
type: string
publicKey:
description: publicKey defines the root of trust based on
Expand All @@ -136,6 +223,10 @@ spec:
- policyType
type: object
x-kubernetes-validations:
- message: pki is required when policyType is PKI, and forbidden
otherwise
rule: 'has(self.policyType) && self.policyType == ''PKI'' ?
has(self.pki) : !has(self.pki)'
- message: publicKey is required when policyType is PublicKey,
and forbidden otherwise
rule: 'has(self.policyType) && self.policyType == ''PublicKey''
Expand Down
Loading