-
Notifications
You must be signed in to change notification settings - Fork 590
Add TLS adherance feature gate #2680
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| apiVersion: apiextensions.k8s.io/v1 | ||
| name: "APIServer" | ||
| crdName: apiservers.config.openshift.io | ||
| featureGates: | ||
| - TLSAdherence | ||
| tests: | ||
| onCreate: | ||
| - name: Should be able to create with tlsAdherence set to LegacyExternalAPIServerComponentsOnly | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: LegacyExternalAPIServerComponentsOnly | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| audit: | ||
| profile: Default | ||
| tlsAdherence: LegacyExternalAPIServerComponentsOnly | ||
| - name: Should be able to create with tlsAdherence set to StrictAllComponents | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: StrictAllComponents | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| audit: | ||
| profile: Default | ||
| tlsAdherence: StrictAllComponents | ||
| - name: Should reject invalid tlsAdherence value | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: InvalidValue | ||
| expectedError: 'spec.tlsAdherence: Unsupported value: "InvalidValue": supported values: "LegacyExternalAPIServerComponentsOnly", "StrictAllComponents"' | ||
| onUpdate: | ||
| - name: Should be able to update tlsAdherence from LegacyExternalAPIServerComponentsOnly to StrictAllComponents | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: LegacyExternalAPIServerComponentsOnly | ||
| updated: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: StrictAllComponents | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| audit: | ||
| profile: Default | ||
| tlsAdherence: StrictAllComponents | ||
| - name: Should be able to update tlsAdherence from StrictAllComponents to LegacyExternalAPIServerComponentsOnly | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: StrictAllComponents | ||
| updated: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: LegacyExternalAPIServerComponentsOnly | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| audit: | ||
| profile: Default | ||
| tlsAdherence: LegacyExternalAPIServerComponentsOnly | ||
| - name: Should not allow removing tlsAdherence once set | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| tlsAdherence: StrictAllComponents | ||
| updated: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: {} | ||
| expectedError: "tlsAdherence may not be removed once set" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ type APIServer struct { | |
| Status APIServerStatus `json:"status"` | ||
| } | ||
|
|
||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=TLSAdherence,rule="has(oldSelf.tlsAdherence) ? has(self.tlsAdherence) : true",message="tlsAdherence may not be removed once set" | ||
| type APIServerSpec struct { | ||
| // servingCert is the TLS cert info for serving secure traffic. If not specified, operator managed certificates | ||
| // will be used for serving secure traffic. | ||
|
|
@@ -62,6 +63,37 @@ type APIServerSpec struct { | |
| // The current default is the Intermediate profile. | ||
| // +optional | ||
| TLSSecurityProfile *TLSSecurityProfile `json:"tlsSecurityProfile,omitempty"` | ||
| // tlsAdherence controls which components in the cluster adhere to the TLS security profile | ||
| // configured on this APIServer resource. | ||
| // | ||
| // Valid values are "LegacyExternalAPIServerComponentsOnly" and "StrictAllComponents". | ||
| // | ||
| // When set to "LegacyExternalAPIServerComponentsOnly", only the externally exposed | ||
| // API server components (kube-apiserver, openshift-apiserver, oauth-apiserver) honor the configured | ||
| // TLS profile. Other components continue to use their individual TLS configurations. | ||
| // | ||
| // When set to "StrictAllComponents", all components must honor the configured TLS profile | ||
| // unless they have a component-specific TLS configuration that overrides it. | ||
| // This mode is recommended for security-conscious deployments and is required for | ||
| // certain compliance frameworks. | ||
| // | ||
| // Note: Some components such as Kubelet and IngressController have their own dedicated TLS | ||
| // configuration mechanisms via KubeletConfig and IngressController CRs respectively. When these | ||
| // component-specific TLS configurations are set, they take precedence over the cluster-wide | ||
| // tlsSecurityProfile. When not set, these components fall back to the cluster-wide default. | ||
| // | ||
| // Components that encounter an unknown value for tlsAdherence should treat it as "StrictAllComponents" | ||
| // and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | ||
| // | ||
| // This field is optional. | ||
| // When omitted, this means the user has no opinion and the platform is left to choose reasonable defaults. | ||
| // These defaults are subject to change over time. | ||
| // The current default is LegacyExternalAPIServerComponentsOnly. | ||
| // | ||
| // Once set, this field may be changed to a different value, but may not be removed. | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional | ||
| TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a recent call we discussed the fact that we might need different TLSSecurityProfile structs for APIServer vs Ingress. If we had separate structs, would this field still be a sibling of the TLSSecurityProfile, or would it be a member of the TLSSecurityProfile?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that this adherence field is only applicable for the APIServer type because it will be the central configuration type. Ingress/Kubelet specific configurations would be overrides to the central configuration and Ingress and Kubelet would be expected to strictly adhere to those. @joelanford @richardsonnick please correct me if I'm wrong here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is my understanding as well. The ingress/kubelet configurations are overrides to the (central authoritative) apiserver type
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that relationship, that wasn't actually the key part of my question
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, still a sibling. |
||
| // audit specifies the settings for audit configuration to be applied to all OpenShift-provided | ||
| // API servers in the cluster. | ||
| // +optional | ||
|
|
@@ -237,6 +269,32 @@ const ( | |
| type APIServerStatus struct { | ||
| } | ||
|
|
||
| // TLSAdherencePolicy defines which components adhere to the TLS security profile. | ||
| // Implementors should use the ShouldAllComponentsAdhere helper function from library-go | ||
| // rather than checking these values directly. | ||
| // +kubebuilder:validation:Enum=LegacyExternalAPIServerComponentsOnly;StrictAllComponents | ||
| type TLSAdherencePolicy string | ||
richardsonnick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const ( | ||
richardsonnick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // TLSAdherencePolicyNoOpinion represents an empty/unset value for tlsAdherence. | ||
| // This value cannot be explicitly set and is only present when the field is omitted. | ||
| // When the field is omitted, the cluster defaults to LegacyExternalAPIServerComponentsOnly | ||
| // behavior. Components should treat this the same as LegacyExternalAPIServerComponentsOnly. | ||
| TLSAdherencePolicyNoOpinion TLSAdherencePolicy = "" | ||
|
|
||
| // TLSAdherencePolicyLegacyExternalAPIServerComponentsOnly means only the externally exposed | ||
| // API server components (kube-apiserver, openshift-apiserver, oauth-apiserver) honor | ||
| // the configured TLS profile. Other components continue to use their individual TLS | ||
| // configurations. | ||
| TLSAdherencePolicyLegacyExternalAPIServerComponentsOnly TLSAdherencePolicy = "LegacyExternalAPIServerComponentsOnly" | ||
|
|
||
| // TLSAdherencePolicyStrictAllComponents means all components must honor the configured TLS | ||
| // profile unless they have a component-specific TLS configuration that overrides it. | ||
| // This mode is recommended for security-conscious deployments and is required | ||
| // for certain compliance frameworks. | ||
| TLSAdherencePolicyStrictAllComponents TLSAdherencePolicy = "StrictAllComponents" | ||
| ) | ||
|
|
||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to configure validation on this field, and set the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see you are defining the Enum validation on the type, which is fine. Let's also include the
+kubebuilder:defaultmarker there as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, reading the tlsAdherence description again, it seems like you are wanting it to be possible for an empty string to be allowed to be persisted for this field in etcd, which would allow us in theory to change the default semantic later in the future from legacy to strict without a change in the actual tlsAdherence value.
In practice, I think the problem with that idea is that each component essentially has their own implementation of this API and we'd have to do a really good job coordinating a semantic switch from legacy to strict.
What do folks think about:
LegacyExternalAPIServerComponentsOnlyto etcd by default.StrictAllComponentsand will not have to worry about the possibility of""with changing semantics later.StrictAllComponentsas the new default, we make sure newly installed clusters get that, and we potentially block upgrades of clusters that still useLegacyExternalAPIServerComponentsOnly? Or maybe there's some other way to encourage users to switch toStrictAllComponents?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For configuration APIs we generally try to avoid setting the default via
+kubebuilder:defaultmarker so that we can change the default behavior of the "no-opinion" mechanism.In this case, I don't think we will be doing that but I still wouldn't use the
+kubebuilder:defaultmarker because then we won't be able to distinguish whether or not the user intentionally set the field or if we defaulted it on their behalf and you technically get locked into that default being a contractual default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was hoping you'd have some good advice here. It is the intent to eventually make all components honor the cluster-wide default, so I supposed we'll just have to deal with trying to coordinate a semantic change across a bunch of components at once when the time comes.
To facilitate that, I'd suggest a library-go function somehow like this:
And then when we want to change the semantic, we'd update that function to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That aligns with what I envisioned.
Can we make sure to update the EP with a mention of this kind of check/library that implementors are expected to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a mention of this helper function.
Also created a draft diff in library-go to add this function: openshift/library-go#2114
Will need to land this first to get the TLSAdherencePolicy type into library-go