Add TLS adherance feature gate #2680
Conversation
|
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 pull request introduces a new TLSAdherence feature for APIServer configuration. A new TLSAdherencePolicy type is added with two enum values: LegacyExternalAPIServerComponentsOnly and StrictAllComponents. The corresponding TLSAdherence field is added to APIServerSpec with feature-gate support. A new feature gate FeatureGateTLSAdherence is registered, enabled in DevPreviewNoUpgrade and TechPreviewNoUpgrade modes. Multiple CRD manifests across different feature gate profiles are updated to include the tlsAdherence field and feature gate entries. API documentation and OpenAPI generation code are updated accordingly. Additionally, ManagedBootImagesCPMS feature gate configuration is updated. 🚥 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
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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.
| "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.
config/v1/types_apiserver.go
Outdated
| // 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?
There was a problem hiding this comment.
I think these names are far better. Going with LegacyExternalAPIServerComponentsOnly and StrictAllComponents
| // 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?
There was a problem hiding this comment.
Yeah that should not be here. It has since been removed. This PR applies solely to the adherence fate.
| // | ||
| // 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. |
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
0cc2223 to
f627b61
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 295-318: The CRD docs claim tlsAdherence defaults to
"LegacyExternalAPIServerComponentsOnly" but the schema lacks a default; add the
kubebuilder default tag to the APIServer Go field (the tlsAdherence field on the
APIServer type) as +kubebuilder:default=LegacyExternalAPIServerComponentsOnly so
the generated CRD includes the default, and run codegen to update the YAML; also
ensure operators consuming the APIServer resource handle omitted/empty
tlsAdherence by treating it as Legacy until the default is live. Additionally,
add x-kubernetes-list-type: atomic to the curves list (to match ciphers) in the
generated CRD so both lists have consistent list-type semantics.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
352-364: Consider markingcurvesas an atomic list for consistency withciphers.
ciphersis explicitly atomic;curveswould benefit from the same list-type to avoid merge ambiguity in SSA. Prefer adding+listType=atomicon the Go field and regenerating manifests.♻️ Suggested manifest shape
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: atomic
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
richardsonnick
force-pushed
the
tls-adherance-fate
branch
from
f627b61 to
c7aecd0
Compare
| // | ||
| // When omitted, the default value is "LegacyExternalAPIServerComponentsOnly". | ||
| // +openshift:enable:FeatureGate=TLSAdherence | ||
| // +optional |
There was a problem hiding this comment.
We need to configure validation on this field, and set the default.
// +kubebuilder:validation:items:Enum:=LegacyExternalAPIServerComponentsOnly;StrictAllComponents
// +kubebuilder:default:=LegacyExternalAPIServerComponentsOnly
There was a problem hiding this comment.
Oh I see you are defining the Enum validation on the type, which is fine. Let's also include the +kubebuilder:default marker there as well.
| // TLSAdherenceStrictAllComponents means all components must honor the configured TLS | ||
| // profile. This mode is recommended for security-conscious deployments and is required | ||
| // for certain compliance frameworks. |
There was a problem hiding this comment.
Maybe we should include a note here (and in the tlsAdherence description) that component-specific configurations can override the tlsSecurityProfile configuration?
There was a problem hiding this comment.
How did these changes get here? Maybe need to rebase and push?
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