Add TLS adherance feature gate #2680
Conversation
Co-authored-by: Bryce Palmer <everettraven@gmail.com>
This commit introduces the TLSAdherence feature gate and tlsAdherence field on the APIServer config resource. The tlsAdherence field controls how strictly components in the cluster adhere to the TLS security profile configured on the APIServer resource. Valid values: - Legacy (default): backward-compatible behavior where components may fall back to their individual defaults if conflicts arise - Strict: enforces strict adherence to the TLS configuration Enhancement: openshift/enhancements#1910
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @richardsonnick! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis change introduces TLS configuration enhancements to OpenShift API server and kubelet configurations. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
size/XL
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@config/v1/types_apiserver.go`:
- Around line 65-84: Update the TLSAdherence field documentation to include the
exclusion note from the CRD manifests: explicitly state that Kubelet and
IngressController are excluded from tlsAdherence control and instead use their
own TLS config (KubeletConfig and IngressController CRs). Edit the comment above
the TLSAdherence TLSAdherencePolicy field (the docblock that documents
TLSAdherence) to append a short sentence noting this exclusion so generated API
docs reflect the same caveat as the CRD manifests.
In `@features.md`:
- Line 78: The table row containing the "TLSAdherence" feature is missing a
space before the first pipe after the feature name, violating markdownlint table
spacing; edit the row that starts with "TLSAdherence" and insert a single space
before the following "|" so it reads "TLSAdherence |" (preserving the rest of
the cells exactly) to conform to the configured table style.
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 12447-12461: Add a kubebuilder enum validation for the Curves
field so only supported TLS curve identifiers are accepted: add a comment marker
like +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521 immediately above the
Curves []string field in the struct in config/v1/types_tlssecurityprofile.go
(mirror the pattern used for TLSProtocolVersion), then re-run
controller-gen/openapi generation to update
openapi/generated_openapi/zz_generated.openapi.go so the "curves" schema
includes the enum restriction.
🧹 Nitpick comments (5)
config/v1/types_tlssecurityprofile.go (1)
169-178: Consider adding listType=atomic for Curves for SSA consistency.
This mirrors theCipherslist semantics and avoids any unexpected merge behavior.🔧 Proposed tweak
// curves: // - X25519 // - P-256 // + // +listType=atomic Curves []string `json:"curves,omitempty"`openapi/generated_openapi/zz_generated.openapi.go (1)
9102-9108: Add enum validation fortlsAdherenceto match the documented valid values.The schema only declares a free-form string even though the description defines just
"Legacy"and"Strict". That means invalid values will pass admission and only be handled at runtime. Consider adding kubebuilder enum (and, if desired, a default) on the source type so the generated OpenAPI enforces the allowed values.[Suggested fix (apply in
config/v1/types_apiserver.go, not this generated file):]🔧 Example annotation update
// +optional +// +kubebuilder:validation:Enum=Legacy;Strict +// +kubebuilder:default=Legacy TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"`payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
259-270: Consider addingx-kubernetes-list-typeannotation for consistency.The
curvesfield is missing thex-kubernetes-list-typeannotation that theciphersfield has (line 258 showsx-kubernetes-list-type: atomicfor ciphers). For consistency and to ensure proper list merge behavior, consider adding the same annotation.📝 Suggested fix
curves: description: |- curves is used to specify the elliptic curves that are used during the TLS handshake. Operators may remove entries their operands do not support. For example, to use X25519 and P-256 (yaml): curves: - X25519 - P-256 items: type: string type: array + x-kubernetes-list-type: atomicpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
287-298: Same annotation consistency issue as Default CRD.The
curvesfield here also lacks thex-kubernetes-list-type: atomicannotation thatciphershas. See the comment on the Default CRD for the suggested fix.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
355-366: Same annotation consistency issue.The
curvesfield here also lacksx-kubernetes-list-type: atomic. This is consistent across all the CRD files in this PR but differs from theciphersfield pattern.
| // tlsAdherence controls how strictly components in the cluster adhere to the TLS security profile | ||
| // configured on this APIServer resource. | ||
| // | ||
| // Valid values are "Legacy" and "Strict". | ||
| // | ||
| // When set to "Legacy" (the default), components attempt to honor the configured TLS profile | ||
| // but may fall back to their individual defaults if conflicts arise. This mode is intended for | ||
| // clusters that need to maintain compatibility with existing configurations during migration. | ||
| // | ||
| // When set to "Strict", all components must strictly honor the configured TLS profile. | ||
| // This mode is recommended for security-conscious deployments and is required for | ||
| // certain compliance frameworks. | ||
| // | ||
| // Components that encounter an unknown value for tlsAdherence should treat it as "Strict" | ||
| // and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | ||
| // | ||
| // When omitted, the default value is "Legacy". | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional | ||
| TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"` |
There was a problem hiding this comment.
Documentation inconsistency with CRD manifests.
The Go type documentation for TLSAdherence does not mention that Kubelet and IngressController components are excluded from tlsAdherence control, but the CRD manifests (TechPreviewNoUpgrade.crd.yaml and DevPreviewNoUpgrade.crd.yaml) include this important note:
"Note: The Kubelet and IngressController components are excluded from tlsAdherence control as they have their own dedicated TLS configuration mechanisms via KubeletConfig and IngressController CRs respectively."
Consider adding this exclusion note to the Go type documentation to maintain consistency and ensure generated API documentation reflects this important caveat.
📝 Suggested documentation addition
// Components that encounter an unknown value for tlsAdherence should treat it as "Strict"
// and log a warning to ensure forward compatibility while defaulting to the more secure behavior.
//
+ // Note: The Kubelet and IngressController components are excluded from tlsAdherence control
+ // as they have their own dedicated TLS configuration mechanisms via KubeletConfig and
+ // IngressController CRs respectively.
+ //
// When omitted, the default value is "Legacy".📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // tlsAdherence controls how strictly components in the cluster adhere to the TLS security profile | |
| // configured on this APIServer resource. | |
| // | |
| // Valid values are "Legacy" and "Strict". | |
| // | |
| // When set to "Legacy" (the default), components attempt to honor the configured TLS profile | |
| // but may fall back to their individual defaults if conflicts arise. This mode is intended for | |
| // clusters that need to maintain compatibility with existing configurations during migration. | |
| // | |
| // When set to "Strict", all components must strictly honor the configured TLS profile. | |
| // This mode is recommended for security-conscious deployments and is required for | |
| // certain compliance frameworks. | |
| // | |
| // Components that encounter an unknown value for tlsAdherence should treat it as "Strict" | |
| // and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | |
| // | |
| // When omitted, the default value is "Legacy". | |
| // +openshift:enable:FeatureGate=TLSAdherence | |
| // +optional | |
| TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"` | |
| // tlsAdherence controls how strictly components in the cluster adhere to the TLS security profile | |
| // configured on this APIServer resource. | |
| // | |
| // Valid values are "Legacy" and "Strict". | |
| // | |
| // When set to "Legacy" (the default), components attempt to honor the configured TLS profile | |
| // but may fall back to their individual defaults if conflicts arise. This mode is intended for | |
| // clusters that need to maintain compatibility with existing configurations during migration. | |
| // | |
| // When set to "Strict", all components must strictly honor the configured TLS profile. | |
| // This mode is recommended for security-conscious deployments and is required for | |
| // certain compliance frameworks. | |
| // | |
| // Components that encounter an unknown value for tlsAdherence should treat it as "Strict" | |
| // and log a warning to ensure forward compatibility while defaulting to the more secure behavior. | |
| // | |
| // Note: The Kubelet and IngressController components are excluded from tlsAdherence control | |
| // as they have their own dedicated TLS configuration mechanisms via KubeletConfig and | |
| // IngressController CRs respectively. | |
| // | |
| // When omitted, the default value is "Legacy". | |
| // +openshift:enable:FeatureGate=TLSAdherence | |
| // +optional | |
| TLSAdherence TLSAdherencePolicy `json:"tlsAdherence,omitempty"` |
🤖 Prompt for AI Agents
In `@config/v1/types_apiserver.go` around lines 65 - 84, Update the TLSAdherence
field documentation to include the exclusion note from the CRD manifests:
explicitly state that Kubelet and IngressController are excluded from
tlsAdherence control and instead use their own TLS config (KubeletConfig and
IngressController CRs). Edit the comment above the TLSAdherence
TLSAdherencePolicy field (the docblock that documents TLSAdherence) to append a
short sentence noting this exclusion so generated API docs reflect the same
caveat as the CRD manifests.
| | OnPremDNSRecords| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | ||
| | SELinuxMount| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | ||
| | SignatureStores| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | ||
| | TLSAdherence| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | |
There was a problem hiding this comment.
Fix table pipe spacing to satisfy markdownlint.
The new row is missing a space before the pipe after the feature name, which violates the configured table style.
🔧 Suggested fix
-| TLSAdherence| | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> | | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> |
+| TLSAdherence | | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> | | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | TLSAdherence| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | | |
| | TLSAdherence | | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> | | | <span style="background-color: `#519450`">Enabled</span> | <span style="background-color: `#519450`">Enabled</span> | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
78-78: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
78-78: Table column style
Table pipe has extra space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@features.md` at line 78, The table row containing the "TLSAdherence" feature
is missing a space before the first pipe after the feature name, violating
markdownlint table spacing; edit the row that starts with "TLSAdherence" and
insert a single space before the following "|" so it reads "TLSAdherence |"
(preserving the rest of the cells exactly) to conform to the configured table
style.
| "curves": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "curves is used to specify the elliptic curves that are used during the TLS handshake. Operators may remove entries their operands do not support. For example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519\n - P-256", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: "", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Go 1.21 1.22 crypto/tls supported curve names CurveID identifiers
💡 Result:
In Go 1.21 and Go 1.22, crypto/tls exposes these supported (named group) CurveID constants:
| TLS group / curve name | Go constant | CurveID (uint16) |
|---|---|---|
| NIST P-256 (secp256r1) | tls.CurveP256 |
23 |
| NIST P-384 (secp384r1) | tls.CurveP384 |
24 |
| NIST P-521 (secp521r1) | tls.CurveP521 |
25 |
| X25519 | tls.X25519 |
29 |
These are the CurveID identifiers defined by crypto/tls for those versions. [1]
Sources:
[1] https://go.googlesource.com/go/+/go1.14.8/src/crypto/tls/common.go
🏁 Script executed:
# First, find the source code that generates this OpenAPI schema
# The file is auto-generated, so we need to find what generates it
fd -t f "*.go" | head -20 | xargs grep -l "curves is used to specify" 2>/dev/null | head -5Repository: openshift/api
Length of output: 340
🏁 Script executed:
# Search for the source field definition
rg -A 5 -B 5 "curves.*TLS.*handshake" --type goRepository: openshift/api
Length of output: 3653
🏁 Script executed:
# Check for OpenAPI generation tooling and look for the source type
find . -name "*openapi*" -o -name "*swagger*" | grep -v generated | head -10Repository: openshift/api
Length of output: 427
🏁 Script executed:
# Look for struct definitions with a "curves" field that might be the source
rg "Curves\s*\[\]" --type go | head -20Repository: openshift/api
Length of output: 359
🏁 Script executed:
# Read the source file that defines the Curves field
cat -n config/v1/types_tlssecurityprofile.goRepository: openshift/api
Length of output: 11992
🏁 Script executed:
# Look for validation rules or constants in the codebase
rg "enum.*curve|Curve.*enum|CurveID|X25519|P-256|P-384|P-521" --type go config/v1/ | head -30Repository: openshift/api
Length of output: 568
🏁 Script executed:
# Search for validation struct tags or validation rules in similar fields
rg "json:.*omitempty.*validation|kubebuilder:validation" --type go config/v1/types_tlssecurityprofile.go -A 3 -B 3Repository: openshift/api
Length of output: 490
🏁 Script executed:
# Check how other string arrays are validated in the codebase
rg '\[\]string.*json' --type go config/v1/ -A 2 | grep -A 2 "kubebuilder" | head -40Repository: openshift/api
Length of output: 39
Add validation for curves field to restrict entries to supported TLS curve identifiers.
The Curves field in config/v1/types_tlssecurityprofile.go (line 178) accepts any string with no validation, allowing invalid curve names to be accepted during config validation but fail later at TLS handshake time. Add a +kubebuilder:validation:Enum marker similar to the pattern used for TLSProtocolVersion to enforce only supported curves: X25519, P-256, P-384, P-521 (as supported by Go 1.21+ crypto/tls).
🤖 Prompt for AI Agents
In `@openapi/generated_openapi/zz_generated.openapi.go` around lines 12447 -
12461, Add a kubebuilder enum validation for the Curves field so only supported
TLS curve identifiers are accepted: add a comment marker like
+kubebuilder:validation:Enum=X25519;P-256;P-384;P-521 immediately above the
Curves []string field in the struct in config/v1/types_tlssecurityprofile.go
(mirror the pattern used for TLSProtocolVersion), then re-run
controller-gen/openapi generation to update
openapi/generated_openapi/zz_generated.openapi.go so the "curves" schema
includes the enum restriction.
|
@richardsonnick: The following tests failed, say
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. |
| // When set to "Legacy" (the default), components attempt to honor the configured TLS profile | ||
| // but may fall back to their individual defaults if conflicts arise. This mode is intended for | ||
| // clusters that need to maintain compatibility with existing configurations during migration. |
There was a problem hiding this comment.
I thought Legacy was going to essentially mean "only the externally exposed API server components", and Strict will mean "ALL cluster components with the exception of Kubelet-related and Ingress-related servers"?
I also think we should update GoDoc for the TLSSecurityProfile field, which currently says "for externally exposed servers". With TLSAdherence in the mix, what the profile is for depends on the TLSAdherence value.
But that should doc change should also only apply when the feature gate is enabled. I think @JoelSpeed said there's a way to do that?
There was a problem hiding this comment.
I also wonder if we should improve the naming of the enums to more accurately describe what they do? For example:
- LegacyExternalAPIServerComponentsOnly
- StrictAllComponents
Or something along those lines?
| // curves is used to specify the elliptic curves that are used during | ||
| // the TLS handshake. Operators may remove entries their operands do | ||
| // not support. For example, to use X25519 and P-256 (yaml): | ||
| // | ||
| // curves: | ||
| // - X25519 | ||
| // - P-256 | ||
| Curves []string `json:"curves,omitempty"` |
There was a problem hiding this comment.
Based on my read of openshift/enhancements#1894, it seems like very few components currently handle TLS curve configuration.
And I don't think we will be able to promote this full set of API changes unless components handle all of MinTLSVersion, CipherSuites, and Curves.
I am worried that the upstream's lack of curves support might take some time and would therefore block the rest of the feature around honoring the min TLS and cipher suites configuration.
Should we remove Curves from this PR or configure it to be present with a separate feature gate?
| // | ||
| // curves: | ||
| // - X25519 | ||
| // - P-256 |
There was a problem hiding this comment.
If/when we do add Curves field, it needs to be introduced behind a feature gate.
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Curves should be removed or included behind a feature gate. My understanding is that we can't add a field to the Default CRD without first going through TechPreviewNoUpgrade.
|
Adding to @joelanford 's comment - I'd prefer we keep the curves field entirely separate here to keep this well scoped. IIRC there is already a PR that is looking to add the curves field. |
4 participants
User description
Add TLSAdherence feature gate for cluster-wide TLS configuration
This PR introduces the
TLSAdherencefeature gate and adds thetlsAdherencefield to theAPIServerconfig resource (apiserver.config.openshift.io/v1).Summary
The
tlsAdherencefield controls how strictly components in the cluster adhere to the TLS security profile configured on the APIServer resource.Valid values:
Feature Gate Details
TLSAdherenceDevPreviewNoUpgrade,TechPreviewNoUpgradeRelated