CNTRLPLANE-1751: Configurable PKI for OpenShift Internal Certificates #1882
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sanchezl
force-pushed
the
custom-pki
branch
3 times, most recently
from
d51e417 to
1df538d
Compare
sanchezl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request references CNTRLPLANE-1750 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 sub-task to target the "4.21.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. |
|
/jira refresh |
|
@sanchezl: This pull request references CNTRLPLANE-1750 which is a valid jira issue. 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
sanchezl
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@sanchezl: No Jira issue is referenced in the title of this pull request. 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. |
sanchezl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request references CNTRLPLANE-1751 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 sub-task to target the "4.21.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. |
…rs and certificate overrides. Replace webhooks with in-process validation for improved performance and simplicity.
…d enforce validation rules with unions and CEL.
…ructure, extensibility, and clarity.
…atures. Add details about configuring cryptographic parameters while maintaining defaults.
…GA, and refine graduation criteria for OpenShift 4.21 release
…rified upgrade scenarios, and refined operational guidance.
|
/cc @dinhxuanvu |
…and updated validation rules.
…rtificate names as a list for improved diffs.
…or's special behavior, future extensibility with dynamic certificate registration, and proposed validation framework.
| ``` | ||
| - Compliance checking could be addressed in a future enhancement with a dedicated compliance-checker component if needed | ||
|
|
||
| ## Open Questions |
There was a problem hiding this comment.
Not sure the best place to ask this, so asking here - how does this impact/interact with existing CA configuration options like https://github.com/openshift/api/blob/c2a41ea924bd8227622363c3d12f456cd2186924/config/v1/types_apiserver.go#L38-L48 ?
At a brief glance, this would be more of an override scenario (i.e prefer the configuration in the linked type) but I want to make sure I'm properly understanding that - is that accurate?
Are there any other implementations like this one that we are aware of for certificate customization of core platform components we need to take into consideration?
There was a problem hiding this comment.
This enhancement only affects OpenShift’s internal certificate generation, not user-provided certificates, trust anchors, TLS parameters, or external CA integration. I have added more details to the enhancement.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Initial pass of the API
|
|
||
| The compatibility level is enforced through the `+openshift:compatibility-gen:level` annotation and will be validated by the API review process. The annotation will change from `level=4` to `level=1` when the API is promoted to v1. | ||
|
|
||
| #### PKI Resource |
There was a problem hiding this comment.
Just a note that for now I'm going to keep my review more high-level and focused on the overall structure of the API.
There are a handful of additional small things that would picked up by the linter if you created a PR with these changes against the openshift/api repo.
If you open a PR against openshift/api let me know and we can keep discussions on the API surface focused in one area moving forward.
| // defaults specifies the default certificate configuration | ||
| // for all certificates unless overridden by category or specific | ||
| // certificate configuration. | ||
| // If not specified, uses platform defaults (typically RSA 2048). |
There was a problem hiding this comment.
Do we have any known defaulting behavior for core components that do not follow RSA 2048?
| // key specifies the cryptographic parameters for the certificate's key pair. | ||
| // +kubebuilder:validation:Required | ||
| Key KeyConfig `json:"key"` | ||
|
|
||
| // Future extensibility: fields like Lifetime, Rotation, Extensions | ||
| // can be added here without restructuring the API. |
There was a problem hiding this comment.
Because future extensibility is noted here, I think it is worth discussing how this works in general.
Adding new API fields will need to be added as optional.
Do you envision a future where this configuration option allows for setting other configuration fields but not key?
For example, in a future where rotation options are added, would it be reasonable for something like:
certificate:
rotation: ...to be valid?
There was a problem hiding this comment.
Great point. Updated.
|
|
||
| // KeyConfig specifies cryptographic parameters for key generation. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="(self.algorithm == 'RSA' && has(self.rsa) && !has(self.ecdsa)) || (self.algorithm == 'ECDSA' && has(self.ecdsa) && !has(self.rsa))",message="algorithm must match the configuration: use rsa field for RSA, ecdsa field for ECDSA" |
There was a problem hiding this comment.
The pattern we normally follow here is: https://github.com/openshift/api/blob/c2a41ea924bd8227622363c3d12f456cd2186924/example/v1/types_stable.go#L140
We will typically do one of those expressions for each discriminator-discriminant pair.
| // +kubebuilder:validation:XValidation:rule="self.certificateName in [ | ||
| // 'admin-kubeconfig-signer', | ||
| // 'kubelet-bootstrap-kubeconfig-signer', | ||
| // 'kube-apiserver-localhost-signer', | ||
| // 'kube-apiserver-service-network-signer', | ||
| // 'kube-apiserver-lb-signer', | ||
| // 'root-ca', | ||
| // 'kube-apiserver-to-kubelet-signer', | ||
| // 'kube-control-plane-signer', | ||
| // 'aggregator-signer', | ||
| // 'kubelet-signer', | ||
| // 'aggregator-ca', | ||
| // 'etcd-signer', | ||
| // 'etcd-metrics-signer', | ||
| // 'service-ca', | ||
| // 'csr-signer-ca', | ||
| // 'kube-apiserver-localhost-server', | ||
| // 'kube-apiserver-service-network-server', | ||
| // 'kube-apiserver-lb-server', | ||
| // 'kube-apiserver-internal-lb-server', | ||
| // 'machine-config-server', | ||
| // 'ironic-server', | ||
| // 'etcd-peer-server', | ||
| // 'etcd-server', | ||
| // 'etcd-metrics-server', | ||
| // 'admin-kubeconfig-client', | ||
| // 'kubelet-client', | ||
| // 'kube-apiserver-to-kubelet-client', | ||
| // 'kube-control-plane-kube-controller-manager-client', | ||
| // 'kube-control-plane-kube-scheduler-client', | ||
| // 'aggregator-client', | ||
| // 'apiserver-proxy-client', | ||
| // 'journal-gatewayd-client', | ||
| // ]",message="certificateName must be a well-known certificate name" |
There was a problem hiding this comment.
If this is a static list that doesn't change frequently, this is probably better suited as an enum constraint.
There was a problem hiding this comment.
Adding to this, I've got no idea whether or not the CRD generators handle a multi-line marker like this. I suspect not.
| type PKIStatus struct { | ||
| // No status fields are currently defined. Each certificate-generating operator | ||
| // independently consumes the PKI configuration and reports status through | ||
| // its own ClusterOperator status resource. | ||
| } |
There was a problem hiding this comment.
If there is no status associated with this resource, it is OK to not have a status struct. Just make sure you also remove the status subresource for the API.
… instead of specific version numbers for flexibility.
| Spec PKISpec `json:"spec"` | ||
| } | ||
|
|
||
| type PKISpec struct { |
There was a problem hiding this comment.
My only concern with the current spec structure is that we have only optional fields which makes the empty value valid (i.e spec: {}) - which we generally try to avoid because it makes nothing mean something.
Reading the enhancement, it seems like this is going to be an initial resource stamped out by some cluster operator with some sort of default configuration (or one overridden by configuration via installer).
I'm wondering if it would make sense to have a required discriminated union here so that the empty spec isn't valid.
Something like:
spec:
policy: PlatformDefaultswhich signals to the platform components that there is no opinion from the end-user and they should continue using whatever their defaults are.
When an end-user wants to configure this they do something like:
spec:
policy: Custom
custom:
defaults: ...
categories: ...
overrides: ...I don't know that policy is the right name for the field here, but hopefully it still gets my point across.
…and update relevant documentation accordingly.
|
@sanchezl: 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. |
| Type PKIManagementType `json:"type"` | ||
|
|
||
| // Embed PKIProfile to provide reusable certificate parameter fields. | ||
| // Go API utilities can use PKIProfile to programmatically define custom profiles. | ||
| PKIProfile `json:",inline"` |
There was a problem hiding this comment.
For discriminated unions we generally encourage the use of a field whose name matches the discriminator. This allows us to expand in the future to support other values that may require configuration without causing an overlap for the fields that need to be supplied.
I'd expect this to end up looking something like:
type: Custom
custom:
...5 participants
Summary
This enhancement introduces the ability to configure cryptographic parameters (key algorithm, key size, and elliptic curves) for certificates and keys generated internally by OpenShift components.
Motivation
Enterprise customers increasingly face stringent security compliance requirements that mandate specific cryptographic parameters for PKI infrastructure. Currently, OpenShift uses hardcoded defaults (primarily RSA 2048-bit keys) with no mechanism for administrators to adjust these parameters to meet organizational security requirements or compliance mandates.
Key Features
PKIconfiguration resource inconfig.openshift.io/v1alpha1Proposal Details
The enhancement includes:
PKICRD inconfig.openshift.io/v1alpha1API groupConfigurablePKIfeature gate for controlled rolloutTracking
enhancements/security/internal-pki-config.md