NE-2334: Implement enhancement in OpenShift API to support for TLS curves in TLSProfile #2583
Conversation
|
Pipeline controller notification For optional jobs, comment |
|
Hello @davidesalerno! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
|
[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 |
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
8c07857 to
16a5ecc
Compare
|
/cc @sanchezl |
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
16a5ecc to
6d3bb10
Compare
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
As discussed in slack, let's create a featuregate and enhancement to attach to this.
| // curves: | ||
| // - X25519 | ||
| // - P-256 | ||
| // +optional |
There was a problem hiding this comment.
optional fields should have godoc around what happens if the field is not set (i.e. what is the default behaviour)
There was a problem hiding this comment.
I added a note to address this case. What do you think?
There was a problem hiding this comment.
Technically you don't need the NOTE: since this field is optional, in the godoc, since it will be explained as an optional field, but up to you if you want to highlight that. Otherwise looks fine to me
| // - P-256 | ||
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=20 |
There was a problem hiding this comment.
Could you help understand the two constraints here? Is there a list of valid curves that the API can validate against instead of arbitrarily allowing users to provide up to 20 20-length strings? What happens if the user provides a faulty curve?
There was a problem hiding this comment.
I added these constraints because they seem to be mandatory when I created this change otherwise I couldn't regenerate the CRD and so I introduced that based on the openssl values for this field.
I've just tried to remove them and regenerate the CRD and seems that they are not mandatory anymore so I'm going to remove them.
There was a problem hiding this comment.
Sorry, to clarify, the lists should have constraints. I was wondering based on what you originally had, whether there were up to 20 available curves. Based on #2583 (review) it seems we only want to support a certain set? If it's a well known list that's not too big, maybe we can add validation here that only valid ones can be set via the API.
There was a problem hiding this comment.
Based on what suggested by @sanchezl I introduced an enum type (TLSCurves) with a one-to-one mapping Go's crypto/tls library, containing only the allowed values and switched from string to TLSCurve for this array.
In this way each allowed value is well documented and there is a validation on the enum side.
| @@ -144,6 +144,21 @@ spec: | |||
| type: string | |||
| type: array | |||
| x-kubernetes-list-type: atomic | |||
| curves: | |||
There was a problem hiding this comment.
Since other objects (e.g. the kubeletconfig here) references tlsSecurityProfile type, would the curve be supported for all affected objects and controllers?
There was a problem hiding this comment.
yes, do you think there are some objects that needs some specific improvements for this change?
There was a problem hiding this comment.
Not sure on this one. For example, this would be an additional field you can specify when creating a kubeletconfig object, which then the MCO renders into a MachineConfig, which gets rolled out to disk.
Would the controller need to know that this is a special case field it needs to render when it renders the kubelet.conf?
There was a problem hiding this comment.
Add Default Curves to TLSProfiles
The Curves field has been added to the API schema, but all entries in the TLSProfiles map should be updated to include default curves. Currently, only the Ciphers field is populated in each profile.
Recommended Default Curves
All three TLS profiles (TLSProfileOldType, TLSProfileIntermediateType, TLSProfileModernType) should include these curves by default:
Curves: []string{
"X25519",
"P-256",
"P-384",
"X25519MLKEM768",
},Go TLS Constant to Configuration String Mapping
The PR should document the mapping between Go's crypto/tls constants and the configuration strings used in the Curves field:
| Go Constant | Configuration String | Description |
|---|---|---|
tls.X25519 |
X25519 |
Curve25519 (recommended) |
tls.CurveP256 |
P-256 |
NIST P-256 (secp256r1) |
tls.CurveP384 |
P-384 |
NIST P-384 (secp384r1) |
tls.X25519MLKEM768 |
X25519MLKEM768 |
Post-Quantum Cryptography hybrid |
Given that the cipher names expected in this configuration are based on OpenSSL names, I suggest OpenSSL versions of the strings. OpenSSL has a few alias defined for some of the curves. I've picked the aliases that match the NIST names when I thought it was appropriate.
Documentation Update Needed
The field documentation should include an example showing how to configure a PQC-only (Post-Quantum Cryptography) TLS profile. Users wanting to enforce PQC-only encryption would create a custom TLS profile:
# Example: Force PQC-only encryption
apiVersion: config.openshift.io/v1
kind: APIServer
spec:
tlsSecurityProfile:
type: Custom
custom:
ciphers:
- TLS_AES_128_GCM_SHA256
- TLS_AES_256_GCM_SHA384
- TLS_CHACHA20_POLY1305_SHA256
curves:
- X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve
minTLSVersion: VersionTLS13This configuration ensures that only connections using the post-quantum hybrid key exchange can be established.
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
6d3bb10 to
9754d52
Compare
📝 WalkthroughWalkthroughAdds a new TLSCurve string type and curve constants, and an optional 🚥 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
config/v1/types_tlssecurityprofile.go (1)
216-225: Clarify default behavior whenCurvesis unset
Curvesis marked+optional, but the godoc only explains usage and not what happens when the field is omitted (or effectively empty). That makes it unclear whether implementations fall back to library/platform defaults, profile‑specific defaults, or treat it as misconfiguration.Consider extending the comment to explicitly document the default, e.g. that omitting
curvesmeans “use the platform’s/default profile’s curve set (subject to change between releases)”.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
147-159: Schema added; verify MCO/kubelet honor it end‑to‑end.This addresses the earlier concern about KubeletConfig coverage. Please confirm Machine Config Operator plumbs TLSProfileSpec.Curves through to kubelet (or ignores safely) and document any TLS 1.3 caveats for kubelet.
🧹 Nitpick comments (2)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
333-345: Consistent addition; mirror example/documentation.Change is consistent. Consider adding a brief note on naming (IANA/openssl-style like X25519, P-256) and reference that operands may ignore/trim unsupported entries; also add curves to the example snippet above for parity.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-276: Default profile CRD aligned; minor doc tweak suggested.Looks good. As with other CRDs, consider adding curves to the example block and a brief note on accepted names to avoid user confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(4 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yamlconfig/v1/types_tlssecurityprofile.goconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yamloperator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yamlopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.deepcopy.go
🔇 Additional comments (19)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml (1)
264-276: TLS curves field mirrors existing ciphers semantics correctly
curvesis modeled as an atomic[]stringwith clear docs and consistent placement undertlsSecurityProfile.custom; this aligns with howciphersis exposed and looks good from an API and compatibility standpoint.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-345: Consistent exposure ofcurvesin CustomNoUpgrade APIServer CRDThe
curvesarray is defined identically to the feature-gated CRD ([]stringwithx-kubernetes-list-type: atomicand matching description), keeping profiles consistent across variants.config/v1/zz_generated.swagger_doc_generated.go (1)
2975-2982: Swagger doc forcurvesmatches schema and intentThe new
curvesentry inTLSProfileSpec’s Swagger doc accurately describes usage and mirrors the CRD text, keeping the public API documentation consistent with the new field.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
1980-1992: Curves schema for IngressController looks consistent and well‑shapedThe new
curvesarrays under.spec.tlsSecurityProfile.customand.status.tlsProfilemirror the existingciphersfield (description, list semantics, and placement) and align with the GoTLSProfileSpec. No issues from a schema or maintainability perspective.Also applies to: 3252-3264
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
333-345: APIServer TechPreviewcurvesfield matches TLS profile contractThe
curvesfield is added alongsidecipherswith matching description and list semantics, which keeps the CRD schema aligned withTLSProfileSpecand other manifests. Looks good.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
333-345: DevPreview APIServer CRDcurvesaddition is consistentThe
curvesproperty undertlsSecurityProfile.customis structurally identical to the existingciphersfield and to the TechPreview/other CRDs. This keeps the API surface uniform across feature sets.machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-160: KubeletConfig TLScurvesfield is correctly wired into the CRDThe new
curvesarray undertlsSecurityProfile.custommatches the existingciphersfield’s structure and the sharedTLSProfileSpeccontract, so the feature is exposed consistently to KubeletConfig consumers.config/v1/zz_generated.deepcopy.go (1)
6222-6233: TLSProfileSpec Curves deepcopy mirrors Ciphers pattern correctlyDeep copy of
Curvesuses the standardmake+copyguarded by a nil check, matchingCiphersand avoiding slice aliasing. Looks correct for the new field.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
135-159: KubeletConfig TLS custom.curves field is well-formed and consistentThe new
tlsSecurityProfile.custom.curvesarray mirrors the existingciphersschema (type, list-type, and style of description), so the CRD remains consistent and backward compatible.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
321-345: APIServer (CustomNoUpgrade) TLS curves schema matches existing patternsThe
custom.curvesfield is defined analogously tocustom.ciphers(string array, atomic list, clear description), so the extension of the TLS profile is coherent and safe.payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
252-276: APIServer (Default) curves field keeps TLS profile schemas alignedThe new
tlsSecurityProfile.custom.curvesfield is structurally identical to the one in other APIServer CRDs, maintaining consistency across feature sets without affecting existing behavior.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
321-345: DevPreview APIServer CRD curves field is consistent with other variantsThe
custom.curvesarray is defined consistently withciphersand with the corresponding fields in the Default/CustomNoUpgrade CRDs, so schema behavior stays uniform across feature sets.openapi/generated_openapi/zz_generated.openapi.go (2)
11831-11850: Curves field schema looks correct and consistent with existing TLS profile patternsArray-of-string with
x-kubernetes-list-type: "atomic"and a clear description fits how similar list fields (e.g., ciphers) are modeled here. No issues from a schema or maintainability perspective.
20311-20330: Duplicate curves field for TLSProfileSpec is correctly mirroredThis definition mirrors the earlier curves schema, keeping TLSProfileSpec and CustomTLSProfile aligned. The structure and description are consistent and look good.
operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml (2)
3235-3247: Status parity: ensure controller fills status.tlsProfile.curves.Schema addition is correct. Please confirm the ingress operator sets this field so users can observe effective groups at runtime.
1974-1986: Curves field is correctly implemented repo-wide; documentation suggestions are optional.Verification confirms the Curves field has been properly propagated:
- Go struct definition includes
Curves []stringwithjson:"curves,omitempty"(line 225 in types_tlssecurityprofile.go)- Generated deepcopy code includes Curves field handling
- CRD manifests include curves schema in both
spec.tlsSecurityProfile.customandstatus.tlsProfilesections with matching structure to ciphers (type array, x-kubernetes-list-type: atomic)- No CRDs have ciphers without curves—consistent all-or-nothing propagation
The original suggestions to clarify naming conventions (X25519, P-256, etc.) and extend example snippets are documentation improvements, not required code changes.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-345: LGTM; feature-gated schema stays aligned with Default/TPN variants.No issues. Keep the curves description aligned across CRDs to avoid drift.
Please run the repo-wide script from the ingresscontrollers comment to confirm no CRD missed curves.
openapi/openapi.json (2)
6019-6027: Curves field additions look correct, but clarify item default semantics.The
curvesfield is properly defined as an array of strings with appropriate metadata (x-kubernetes-list-type: atomic) and helpful example documentation. The field appears in two locations (hunks 1 & 2), which aligns with the PR's goal of adding curve support to multiple TLSProfile contexts.However, the
"default": ""for items (lines 6024, 11001) is semantically unclear—an empty string is not a valid elliptic curve identifier. Verify whether this is a required OpenAPI convention for array items or if it should be omitted. If retained, consider updating the description to clarify that operators should never rely on this default.Also applies to: 10996-11004
14289-14358: Clarify scope of FormatMarkerExamples schema addition.The new
FormatMarkerExamplesschema (hunk 3) and its reference (hunk 4) introduce comprehensive documentation for Kubebuilder format markers (14 format marker examples: base64, CIDR, date/time, email, hostname, IP addresses, MAC, password, URI, UUID variants). While the enriched summary indicates this "aligns with broader TLS/curves-related schema enhancements," the schema itself is disconnected from the curves functionality described in the PR objectives.Questions:
- Is FormatMarkerExamples part of the core curves feature, or is this a separate documentation addition that should be scoped in its own PR?
- Is this schema auto-generated or manually maintained? If auto-generated, confirm the generation tool was run.
- The CVE references (CVE-2021-29923, CVE-2024-24790) in the descriptions are helpful; confirm they are kept up-to-date across future releases.
Also applies to: 14448-14451
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
9754d52 to
58da6c8
Compare
openshift-ci
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-178: CRD enum forcurvesdoes not allow documentedX25519MLKEM768value.The
curvesdescription and example mentionX25519MLKEM768, but theitems.enumonly listsX25519,X448,P-256,P-384, andP-521. A KubeletConfig using the documented PQC example will therefore fail schema validation.Once you add
X25519MLKEM768to theTLSCurvekubebuilderEnuminconfig/v1/types_tlssecurityprofile.go, please regenerate this CRD so thecurves.items.enumincludesX25519MLKEM768as well.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
3270-3300: Duplicate of enum/docs mismatch in status.tlsProfile.curves.Apply the same enum addition here to keep status schema consistent with spec and docs.
enum: - X25519 - X448 - P-256 - P-384 - P-521 + - X25519MLKEM768payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-363: Enum mismatch: X25519MLKEM768 documented but not enumerated (same as Default variant).This file shows the identical documentation-enum mismatch as the Default CRD variant: the description references X25519MLKEM768 as a default curve, but the enum does not permit it.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-363: Enum mismatch: X25519MLKEM768 documented but not enumerated (consistent across all variants).Like the Default and CustomNoUpgrade variants, this featuregated CRD also documents X25519MLKEM768 as a default curve while omitting it from the enum validation list. This systematic issue across all CRD manifests suggests a source-level problem that needs correction.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
111-116: Avoid referencing Modern defaults where Modern isn’t supported here.This CRD states only Old and Intermediate are supported, but the curves description discusses Modern defaults. Consider trimming that sentence to reduce confusion.
Also applies to: 147-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(5 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(4 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- openapi/generated_openapi/zz_generated.openapi.go
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml
- operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.swagger_doc_generated.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlconfig/v1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/types_tlssecurityprofile.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (5)
config/v1/types_tlssecurityprofile.go (1)
239-268: Curves field docs and defaulting behavior look good.The
Curvescomment now clearly describes defaults for Old/Intermediate/Modern, states that Custom has no defaults, and explains behavior when the optional field is unset. This matches howTLSProfilesis initialized and addresses the prior request to document default behavior for an optional field.config/v1/zz_generated.deepcopy.go (1)
6222-6233: Deep-copy logic forCurvesis correct and consistent.The new deepcopy block for
Curvesmirrors the existingCiphershandling (allocate new slice, thencopy), soTLSProfileSpeccopies will not share slice storage.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-363: Based on my web search results, I have found critical information that makes the original review comment incorrect.The OpenShift APIServer CR specification does NOT include a user-facing "curves" field. The tlsSecurityProfile Custom profile only exposes
ciphersandminTLSVersion—curve preferences cannot be set by users via the APIServer CR.This means:
- The CRD file likely contains aspirational or incorrect documentation/examples
- Adding X25519MLKEM768 to an enum for a non-existent field won't solve any real problem
- The actual issue is that the documentation shows a curves configuration example that cannot be used
However, the file is auto-generated (indicated by the "zz_generated" prefix), so these issues likely stem from incorrect source specifications used to generate the CRD.
The APIServer CR tlsSecurityProfile Custom profile does not expose curve preferences. The documentation example showing curves configuration is incorrect—users cannot configure curves via the APIServer CR at all. This is a documentation/generation issue rather than a missing enum value.
The review comment's premise is flawed. Rather than adding X25519MLKEM768 to an enum for a non-existent field, the root cause—incorrect documentation showing unsupported configuration—should be addressed in the source specification that generates this CRD file.
Likely an incorrect or invalid review comment.
openapi/openapi.json (2)
6019-6027: Curves fields addition aligns with PR objectives.The "curves" field additions to CustomTLSProfile and TLSProfileSpec correctly mirror the structure of the existing "ciphers" field, with appropriate array type and
x-kubernetes-list-type: atomicannotation. The description is comprehensive, including default curves for each profile type and a practical YAML example with post-quantum cryptography curves.Also applies to: 10996-11004
14289-14358: Unexpected FormatMarkerExamples addition unrelated to PR objectives.Hunks 3 & 4 introduce a
FormatMarkerExamplesschema definition and field that are unrelated to the TLS curves feature described in the PR objectives. This appears to be an unintended inclusion in the PR.Please clarify:
- Are the FormatMarkerExamples changes intentional or accidental?
- Is this file auto-generated (e.g., via Swagger generation)? If so, these hunks should likely be removed and the file regenerated from source definitions rather than manually edited.
Also applies to: 14448-14451
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
Outdated
Show resolved
Hide resolved
...z_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
Outdated
Show resolved
Hide resolved
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
58da6c8 to
eba1e60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-179: Consider clarifying curves description vs Kubelet profile support noteThe
curvesdescription generically states that TLSProfiles Old, Intermediate, and Modern include default curves, while the surroundingtlsSecurityProfiledocs forKubeletConfigsay only Old and Intermediate profiles are currently supported. If Modern truly isn’t honored by kubelet, consider tweaking this description (or adding a brief note) to avoid implying Modern is fully supported here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(5 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(7 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml
- operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/types_tlssecurityprofile.gooperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlconfig/v1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yamlopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (13)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-364: curves schema for KMSEncryptionProvider looks correct and consistentThe
curvesfield mirrorsciphersstructurally, uses theTLSCurveenum with the expected values, and applies appropriatemaxItemsandx-kubernetes-list-type: atomic; this is a sound extension of the TLS profile schema.config/v1/zz_generated.deepcopy.go (1)
6201-6212: Deep-copy logic for Curves slice is correct
TLSProfileSpec.DeepCopyIntonow allocates a new[]TLSCurveandcopys elements whenCurvesis non-nil, matching the existing pattern forCiphersand providing correct value semantics for the new field.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
333-364: APIServer DevPreview CRD curves field is well-formed and aligned with APIThe added
curvesarray undertlsSecurityProfile.customcorrectly exposes theTLSCurveenum, matches the other APIServer CRDs, and provides a detailed description and example without altering existing behavior.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
2022-2053: Curves spec/status schemas are consistent and aligned with TLSCurve.Both
spec.tlsSecurityProfile.custom.curvesandstatus.tlsProfile.curvesshare the same enum set,maxItems: 5, andx-kubernetes-list-type: atomic, which keeps CRD validation and status reporting in sync.Also applies to: 3351-3381
config/v1/types_tlssecurityprofile.go (2)
205-224: TLSCurve enum and Curves field cleanly extend the TLS profile API.Enum values match the documented curves, and the optional
Curves []TLSCurvewithMaxItems=5andlistType=atomicintegrates without impacting existing TLSProfileSpec consumers.Also applies to: 237-267
338-343: Default TLSProfiles correctly seed curves to the documented set.Old, Intermediate, and Modern profiles all initialize
Curvesto{TLSCurveX25519, TLSCurveP256, TLSCurveP384, TLSCurveX25519MLKEM768}, which matches the comments and respects the MaxItems constraint.Also applies to: 360-365, 374-379
payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295: APIServer Default CRD curves field matches the TLSCurve API contract.The enum values,
maxItems: 5, andx-kubernetes-list-type: atomicalign with the GoTLSCurvedefinition and other manifests, so CRD validation will accept all supported curves consistently.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-364: CustomNoUpgrade APIServer CRD keeps curves schema in sync with TLSCurve.The
curvesfield’s enum,maxItems, and list-type mirror the core TLSCurve definition, ensuring this feature-set variant validates the same curve set as the default CRD.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
147-178: KubeletConfig tlsSecurityProfile.custom.curves schema is consistent with shared TLSCurve semantics.The enum values, list constraints, and description align with the common TLSProfileSpec behavior, so kubelet configs can configure the same validated curve set as other components.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml (1)
264-295: Curves field properly integrated with complete enum.The curves field addition is well-structured: all five supported curves are correctly enumerated (including X25519MLKEM768), the documentation clearly explains defaults and provides a PQC example, and the constraint (maxItems: 5) aligns with the available options. The field integrates properly alongside the existing ciphers and minTLSVersion configuration. Earlier concerns about X25519MLKEM768 being undocumented have been resolved by its inclusion in the enum and comprehensive documentation.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295: Consistent curves implementation across CRD variants.This CRD manifest properly mirrors the feature-gated variant, maintaining identical curves field definition, enum values, and documentation. The consistency between feature-gated and default CRD manifests ensures uniform API behavior across cluster configurations.
openapi/openapi.json (2)
25382-25382: Unable to verify PR scope due to repository access limitations.I attempted to verify whether the OSImageStream changes are unrelated to the stated TLS curves support objective, but encountered persistent repository access failures that prevent me from:
- Accessing the PR description and title to confirm the stated objective
- Examining the git diff to identify hunks 4-7 and their actual content
- Verifying the relationship between OSImageStream and TLS curves changes
Without access to PR metadata and the codebase, I cannot definitively confirm or refute the claim that these schema additions are out of scope. To complete this verification, you or the repository maintainers should:
- Review the PR description to confirm whether OSImageStream changes are listed as part of the intended scope
- If the PR objective is solely "Add TLS curves support," confirm whether OSImageStream is a prerequisite or truly a separate concern
- Check the git history to understand when these schema definitions were intended to be added
6027-6035: The review comment's core suggestion is sound, but the proposed enum values require verification against the actual Go source code.Based on web research:
P-521 is problematic: Red Hat/OpenShift documentation lists supported TLS 1.3 curves as X25519, P-256, and P-384. P-521 does not appear in their documentation, suggesting it may not be a supported curve in OpenShift's TLS configuration.
Naming convention mismatch: The proposed enum uses curve aliases (X25519, P-256, P-384) rather than RFC/IANA standardized names (x25519, secp256r1, secp384r1). OpenAPI schemas typically use standardized names for interoperability, though this may be intentional for this API.
X25519MLKEM768 caveat: This hybrid quantum-resistant curve is conditionally supported based on Go version (Go 1.24+), which may make it unsuitable for a fixed enum or require version-gating.
The review comment cannot be fully verified without access to the Go source code defining the TLSCurve type. Manual verification is needed to confirm:
- The exact list of supported curves in the Go source
- The naming convention used in the actual type definition
- Whether all proposed enum values are accurate and complete
|
https://github.com/openshift/api/compare/58da6c8c1090bcc312e8f1bd3d8bca87cd47227d..eba1e60731dc58f0c314e2ef10ed38dd3f528c00 dropped X448. Was that intentional, perhaps because Go's crypto/tls package doesn't include support? Do we have agreement from stakeholders that the current set of curves is sufficient? api/config/v1/types_tlssecurityprofile.go Lines 205 to 224 in eba1e60 |
| // TLSProfiles Old, Intermediate, Modern are including by default the following | ||
| // curves: X25519, P-256, P-384, X25519MLKEM768 | ||
| // TLSProfiles Custom do not include any curves by default. | ||
| // NOTE: since this field is optional, if no curves are specified, the default curves | ||
| // used by the underlying TLS library will be used. |
There was a problem hiding this comment.
| // TLSProfiles Old, Intermediate, Modern are including by default the following | |
| // curves: X25519, P-256, P-384, X25519MLKEM768 | |
| // TLSProfiles Custom do not include any curves by default. | |
| // NOTE: since this field is optional, if no curves are specified, the default curves | |
| // used by the underlying TLS library will be used. | |
| // The Old, Intermediate, Modern profiles include the following | |
| // curves: X25519, P-256, P-384, X25519MLKEM768. | |
| // The Custom profile does not include any curves by default. | |
| // NOTE: Since this field is optional, if no curves are specified, the default curves | |
| // used by the underlying TLS library will be used. |
Is that a "SHOULD" be used, or a "MUST" be used? That is, does a component violate the API contract if it simply does not allow ECDHE when curves is empty?
There was a problem hiding this comment.
I think that this depends on the component desiderata: if it must always be PQC compliant so if curves is empty the component have to use a default curve in order to allow ECDHE and explicity declare which one into its documentation, otherwise if it is allowed for the component to work using a standard DHE , if curves is empty there is no violation if it does not allow ECDHE
| // # Example: Force PQC-only encryption | ||
| // apiVersion: config.openshift.io/v1 | ||
| // kind: APIServer | ||
| // spec: | ||
| // tlsSecurityProfile: | ||
| // type: Custom | ||
| // custom: | ||
| // ciphers: | ||
| // - TLS_AES_128_GCM_SHA256 | ||
| // - TLS_AES_256_GCM_SHA384 | ||
| // - TLS_CHACHA20_POLY1305_SHA256 | ||
| // curves: | ||
| // - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve | ||
| // minTLSVersion: VersionTLS13 |
There was a problem hiding this comment.
Does this render all right in the oc explain output?
There was a problem hiding this comment.
Good question, we generally don't have cross-field examples, but I guess in this case, you cannot specify arbitrary curves that the ciphers you have don't support?
Would anything validate that the curves list is correct for the ciphers you have? Or are all curves supported by all ciphers? Would there ever be a need to say, specify a subset of curves per cipher?
(This might be completely invalid, but as an example, you would want X25519MLKEM768 for TLS_CHACHA20_POLY1305_SHA256, and P-384 for TLS_AES_256_GCM_SHA384, and only those combinations)
There was a problem hiding this comment.
IMO the user facing docs call out that the custom TLS profile "can cause some problems" if misconfigured which is a good hint to the administrator that they should:
- Know what they are doing
- Validate their config before using the profile in a production environment
I think the best approach here is to offer reasonable valid defaults while keeping custom profiles as a "power user" option.
There was a problem hiding this comment.
Maybe the example is better suited to the higher-order custom field since it is showing a more holistic configuration example?
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=5 | ||
| Curves []TLSCurve `json:"curves,omitempty"` |
There was a problem hiding this comment.
Does it make sense to allow duplicates, or should we have some validation to prevent that (something like // +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x == y))`,message="curves cannot contain duplicates")?
There was a problem hiding this comment.
I think it is a really good idea!
What do you think if we switch to listType=set
There was a problem hiding this comment.
I think sets should work to ensure that only one entry of each TLSCurve exists
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
eba1e60 to
4c8e011
Compare
| // TLSCurve is a named curve identifier that can be used in TLSProfile.Curves. | ||
| // There is a one-to-one mapping between these names and the curve IDs defined | ||
| // in crypto/tls package based on IANA's "TLS Supported Groups" registry: | ||
| // https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 |
There was a problem hiding this comment.
Looking at cryto/tls, I see the following values.
type CurveID uint16
const (
CurveP256 CurveID = 23
CurveP384 CurveID = 24
CurveP521 CurveID = 25
X25519 CurveID = 29
X25519MLKEM768 CurveID = 4588
SecP256r1MLKEM768 CurveID = 4587
SecP384r1MLKEM1024 CurveID = 4589
)
Why are we not including all the values?There was a problem hiding this comment.
There was a problem hiding this comment.
Ok so I think we can align to those values the available curve supported
davidesalerno
force-pushed
the
addCurvesForECDH
branch
2 times, most recently
from
830bcef to
2ed6523
Compare
openshift-ci
bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
openapi/openapi.json (1)
10169-10173: Unrelated schema change flagged in previous review.This
policyTypedescription update is unrelated to TLS curves support and falls under the same concern raised in the previous review about mixing unrelated schema changes into this PR. Either move this change to a separate PR or update the PR description to include it in scope.
🧹 Nitpick comments (1)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSCurvesConfiguration.yaml (1)
259-291: Aligncurvesdescription with enum values and maxItemsThe
curvesdescription (Line 259 onward) says Old/Intermediate/Modern profiles include 7 curves, listingSecP256r1MLKEM1024andSecP384r1MLKEM1024, but theTLSCurveenum here only allows 5 values (X25519,P-256,P-384,P-521,X25519MLKEM768) andmaxItemsis 5.If those SecP* hybrids are actually supported elsewhere, consider either:
- Adding them to the
TLSCurveenum and revisitingmaxItems, or- Clarifying in the description that these additional curves are only used implicitly by non-Custom profiles and are not valid values in
custom.curves.Right now the docs overstate what users can configure through this field.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (28)
config/v1/types_tlssecurityprofile.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSCurvesConfiguration.yamlconfig/v1/zz_generated.swagger_doc_generated.gomachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yamlopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/TLSCurvesConfiguration.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSCurvesConfiguration.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yamlconfig/v1/types_tlssecurityprofile.gopayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlopenapi/openapi.jsonmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (15)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSCurvesConfiguration.yaml (1)
1-11: CRD wiring and feature-gate annotation look consistentThe new APIServer TLSCurvesConfiguration CRD and its
feature-gate.release.openshift.io/TLSCurvesConfiguration: "true"annotation align with the feature-gate entry added elsewhere; no structural or validation issues stand out in the top-level schema.config/v1/zz_generated.featuregated-crd-manifests.yaml (1)
8-10: Feature gate entry for TLSCurvesConfiguration looks correctAdding
TLSCurvesConfigurationto the APIServer FeatureGates list is consistent with the new CRD annotation and keeps the wiring coherent.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yaml (1)
4-10: OKD feature-set annotation for kubeletconfigs is fineThe
release.openshift.io/feature-set: OKDannotation correctly scopes this CRD variant to the OKD feature set and does not affect the schema.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yaml (1)
4-10: Payload CRD OKD annotation matches generated manifestThe added
release.openshift.io/feature-set: OKDannotation is consistent with the generated kubeletconfigs-OKD CRD and should integrate cleanly with release tooling.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yaml (1)
4-11: OKD feature-set annotation on ingresscontrollers looks goodAdding
release.openshift.io/feature-set: OKDhere correctly scopes the OKD-specific ingresscontroller CRD flavor without impacting the schema.config/v1/zz_generated.deepcopy.go (1)
5997-6008: Deep-copy logic forTLSProfileSpec.Curvesis correctCurves slice is cloned with
make+copyfollowing the same pattern asCiphers, so no aliasing or correctness issues here.operator/v1/zz_generated.featuregated-crd-manifests.yaml (1)
178-179: IngressController feature gate wiring for TLS curves looks consistentAdding
TLSCurvesConfigurationunderingresscontrollers.operator.openshift.ioFeatureGates aligns with introducing a gated TLS-curve configuration path and matches surrounding patterns.machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml (1)
65-66: KubeletConfig feature gate registration for TLS curves is wired correctlyRegistering
TLSCurvesConfigurationforkubeletconfigs.machineconfiguration.openshift.iomatches the new TLS-curve support and follows existing FeatureGates conventions.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1-356: LGTM!The CRD structure is correct for the TechPreviewNoUpgrade feature set. The absence of the
curvesfield intlsSecurityProfile.customis expected since the TLSCurvesConfiguration feature gate is not enabled for this variant.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-Default.crd.yaml (1)
1-356: LGTM!The CRD structure is correct for the Default feature set. The
curvesfield is appropriately absent as the TLSCurvesConfiguration feature gate is not enabled for this variant.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-Default.crd.yaml (1)
1-356: LGTM!Generated CRD manifest correctly reflects the Default feature set without the
curvesfield, consistent with the feature gate configuration.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml (1)
148-180: Curves field correctly gated for CustomNoUpgrade.The
curvesfield is properly included with appropriate constraints (maxItems: 5,x-kubernetes-list-type: set) and enum validation. The description referencesSecP256r1MLKEM1024andSecP384r1MLKEM1024as default curves, but these aren't in the enum. This documentation mismatch originates from the Go source types and should be addressed there.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yaml (1)
117-163: Same concern:curvesfield absent from custom TLS profileThis DevPreviewNoUpgrade variant has the same
tlsSecurityProfile.customschema as the TechPreviewNoUpgrade variant—lacking thecurvesfield. If curves support is intended for KubeletConfig, both feature-gated variants should include it.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-356: LGTM - Consistent with other KubeletConfig CRD variantsThis payload-manifest CRD is structurally consistent with the corresponding
machineconfiguration/v1variants. The same observation about the absentcurvesfield applies here as noted in the other KubeletConfig CRDs.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml (1)
117-163: No issues found. The absence of thecurvesfield in TechPreviewNoUpgrade is intentional and part of the proper design. The curves support is provided through the feature-gated CRD variant (TLSCurvesConfiguration), and the CustomNoUpgrade variant correctly includes thecurvesfield in its schema, confirming that TLS curves support is properly being added for KubeletConfig as part of this PR.
| curves: | ||
| description: "curves is used to specify the elliptic curves | ||
| that are used during\nthe TLS handshake. Operators may | ||
| remove entries their operands do\nnot support.\n\nTLSProfiles | ||
| Old, Intermediate, Modern are including by default the following\ncurves: | ||
| X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, | ||
| SecP384r1MLKEM1024.\nTLSProfiles Custom do not include any | ||
| curves by default.\nNOTE: since this field is optional, | ||
| if no curves are specified, the default curves\nused by | ||
| the underlying TLS library will be used.\n\nFor example, | ||
| to use X25519 and P-256 (yaml):\n\n# Example: Force PQC-only | ||
| encryption\napiVersion: config.openshift.io/v1\nkind: APIServer\nspec:\n | ||
| \ tlsSecurityProfile:\n type: Custom\n custom:\n ciphers:\n\t | ||
| \ - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n | ||
| \ - TLS_CHACHA20_POLY1305_SHA256\n curves:\n | ||
| \ - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant | ||
| curve\n minTLSVersion: VersionTLS13" | ||
| items: | ||
| description: |- | ||
| TLSCurve is a named curve identifier that can be used in TLSProfile.Curves. | ||
| There is a one-to-one mapping between these names and the curve IDs defined | ||
| in crypto/tls package based on IANA's "TLS Supported Groups" registry: | ||
| https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 | ||
| enum: | ||
| - X25519 | ||
| - P-256 | ||
| - P-384 | ||
| - P-521 | ||
| - X25519MLKEM768 | ||
| type: string | ||
| maxItems: 5 | ||
| type: array | ||
| x-kubernetes-list-type: set |
There was a problem hiding this comment.
Mismatch between description and enum values for curves
The description states that Old/Intermediate/Modern profiles include 7 curves by default:
X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024
However, the enum (lines 356-361) only defines 5 values:
- X25519, P-256, P-384, P-521, X25519MLKEM768
Missing from enum: SecP256r1MLKEM1024, SecP384r1MLKEM1024
Additionally, maxItems: 5 (line 363) would prevent users from specifying all 7 curves mentioned in the description.
Please either update the enum and maxItems to include all mentioned curves, or correct the description to match the actual supported values.
🤖 Prompt for AI Agents
In
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
around lines 333-365, the `curves` description lists 7 curves but the enum only
defines 5 and maxItems is 5; update the enum to include SecP256r1MLKEM1024 and
SecP384r1MLKEM1024 and change maxItems to 7 so the schema matches the
description (alternatively, if those two curves are not supported, edit the
descriptive text to list only the five supported curves and keep maxItems at 5).
Ensure the enum entries exactly match the names in the description and adjust
maxItems accordingly.
...zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
| curves: | ||
| description: "curves is used to specify the elliptic curves | ||
| that are used during\nthe TLS handshake. Operators may | ||
| remove entries their operands do\nnot support.\n\nTLSProfiles | ||
| Old, Intermediate, Modern are including by default the following\ncurves: | ||
| X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, | ||
| SecP384r1MLKEM1024.\nTLSProfiles Custom do not include any | ||
| curves by default.\nNOTE: since this field is optional, | ||
| if no curves are specified, the default curves\nused by | ||
| the underlying TLS library will be used.\n\nFor example, | ||
| to use X25519 and P-256 (yaml):\n\n# Example: Force PQC-only | ||
| encryption\napiVersion: config.openshift.io/v1\nkind: APIServer\nspec:\n | ||
| \ tlsSecurityProfile:\n type: Custom\n custom:\n ciphers:\n\t | ||
| \ - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n | ||
| \ - TLS_CHACHA20_POLY1305_SHA256\n curves:\n | ||
| \ - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant | ||
| curve\n minTLSVersion: VersionTLS13" | ||
| items: | ||
| description: |- | ||
| TLSCurve is a named curve identifier that can be used in TLSProfile.Curves. | ||
| There is a one-to-one mapping between these names and the curve IDs defined | ||
| in crypto/tls package based on IANA's "TLS Supported Groups" registry: | ||
| https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 | ||
| enum: | ||
| - X25519 | ||
| - P-256 | ||
| - P-384 | ||
| - P-521 | ||
| - X25519MLKEM768 | ||
| type: string | ||
| maxItems: 5 | ||
| type: array | ||
| x-kubernetes-list-type: set |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the actual manifest file
find . -name "TLSCurvesConfiguration.yaml" -type fRepository: openshift/api
Length of output: 433
🏁 Script executed:
# Also search for related TLS configuration files
find . -path "*machineconfiguration*" -name "*TLS*.yaml" -o -name "*tls*.yaml" | head -20Repository: openshift/api
Length of output: 199
🏁 Script executed:
# Search for TLSCurve and TLSProfile definitions in the codebase
rg -i "tlscurve|tlsprofile" --type yaml -A 5 | head -100Repository: openshift/api
Length of output: 15423
🏁 Script executed:
# Look for the feature gate mentioned in the scratchpad
rg "TLSCurvesConfiguration" --type yaml -B 5 -A 10Repository: openshift/api
Length of output: 11976
🏁 Script executed:
cat -n ./machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yaml | head -200Repository: openshift/api
Length of output: 11739
🏁 Script executed:
# Also check the line count to understand file size
wc -l ./machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yamlRepository: openshift/api
Length of output: 203
🏁 Script executed:
# Look for source types that define TLSCurve
find . -path "*/machineconfiguration*" -name "*.go" | xargs grep -l "TLSCurve" 2>/dev/null | head -10Repository: openshift/api
Length of output: 39
🏁 Script executed:
# Also check config/v1 for TLSCurve definitions since APIServer also uses it
find . -path "*/config/v1*" -name "*.go" | xargs grep -l "TLSCurve\|type.*Curve" 2>/dev/null | head -10Repository: openshift/api
Length of output: 133
🏁 Script executed:
# Read the source type definition for TLSCurve
cat ./config/v1/types_tlssecurityprofile.go | head -300Repository: openshift/api
Length of output: 9598
🏁 Script executed:
# Check if there's a TLSCurve type definition
rg "type TLSCurve|const.*Curve" config/v1/types_tlssecurityprofile.go -A 10Repository: openshift/api
Length of output: 400
🏁 Script executed:
# Get more context on the TLSCurve constants to see all of them
rg "TLSCurve.*=" config/v1/types_tlssecurityprofile.go | head -20Repository: openshift/api
Length of output: 349
🏁 Script executed:
# Check the TLSProfileSpec in the source to see the full comment and MaxItems
sed -n '200,280p' ./config/v1/types_tlssecurityprofile.goRepository: openshift/api
Length of output: 3274
🏁 Script executed:
# Verify the source of the generated manifests - check if they are indeed auto-generated
head -20 ./machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yamlRepository: openshift/api
Length of output: 736
🏁 Script executed:
# Check if there are any machineconfiguration-specific type definitions that might override TLSCurve
find . -path "*/machineconfiguration*" -name "types*.go" | xargs grep -l "TLSCurve\|TLSProfile" 2>/dev/nullRepository: openshift/api
Length of output: 41
🏁 Script executed:
# Verify the kubebuilder validation on TLSCurve matches the actual enum in the manifest
grep -A 2 "kubebuilder:validation:Enum" ./config/v1/types_tlssecurityprofile.go | grep "TLSCurve" -A 1Repository: openshift/api
Length of output: 78
🏁 Script executed:
# Check the exact naming in the source comments
grep -B 5 -A 5 "SecP256r1MLKEM\|P256r1MLKEM\|SecP384r1MLKEM\|P384r1MLKEM" ./config/v1/types_tlssecurityprofile.goRepository: openshift/api
Length of output: 1305
Sync curves description and enum with the five currently supported TLSCurve values
The curves schema has documentation inconsistencies that should be resolved:
-
The description references seven curve names (
X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024), but the enum only permits five (X25519,P-256,P-384,P-521,X25519MLKEM768). The two PQC hybrid curves should either be added to the enum (andmaxItemsadjusted), or removed from the description to avoid misleading users. -
The example narrative states "to use X25519 and P-256 (yaml)" but the YAML snippet configures only
X25519MLKEM768, which contradicts the preceding sentence.
Update the description to match the canonical five-curve enum defined in config/v1/types_tlssecurityprofile.go, and align the example with the narrative or vice versa.
🤖 Prompt for AI Agents
In
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yaml
around lines 148-180, the human-readable description lists seven curves while
the enum contains five and the example YAML is inconsistent; update the
description to only mention the canonical five curves (X25519, P-256, P-384,
P-521, X25519MLKEM768) to match config/v1/types_tlssecurityprofile.go, and fix
the example narrative and YAML so they align (either change the narrative to say
it demonstrates X25519MLKEM768, or change the YAML to include both X25519 and
P-256), leaving maxItems and enum unchanged.
| "curves": { | ||
| VendorExtensible: spec.VendorExtensible{ | ||
| Extensions: spec.Extensions{ | ||
| "x-kubernetes-list-type": "set", | ||
| }, | ||
| }, | ||
| 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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n# Example: Force PQC-only encryption apiVersion: config.openshift.io/v1 kind: APIServer spec:\n tlsSecurityProfile:\n type: Custom\n custom:\n ciphers:\n\t - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n - TLS_CHACHA20_POLY1305_SHA256\n curves:\n - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve\n minTLSVersion: VersionTLS13", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: "", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix YAML indentation and description inconsistency in source file.
Both hunks have identical issues in the description text:
-
Tab character in YAML example (Line 12290 and 20411): The YAML example contains
\t(tab character) before- TLS_AES_128_GCM_SHA256. YAML does not allow tabs for indentation. Users copying this example will encounter parsing errors. -
Inconsistent example description: The description states "For example, to use X25519 and P-256" but the actual example only demonstrates X25519MLKEM768. This inconsistency may confuse users about how to specify multiple curves.
Since this is generated code (filename prefix zz_generated), please apply fixes to the source documentation in config/v1/types_tlssecurityprofile.go:
- Replace the tab character with spaces (typically 2 or 4 spaces for YAML indentation)
- Either add P-256 to the example or update the introductory text to match the actual example
🔎 Expected fix in source file
The description in the source file should be corrected to use proper YAML spacing and consistent example text. For example:
- ciphers:\n\t - TLS_AES_128_GCM_SHA256
+ ciphers:\n - TLS_AES_128_GCM_SHA256And either:
-For example, to use X25519 and P-256 (yaml):
+For example, to force PQC-only encryption (yaml):Or add P-256 to the curves list in the example.
Also applies to: 20404-20423
🤖 Prompt for AI Agents
In openapi/generated_openapi/zz_generated.openapi.go around lines 12283-12302
(and the similar hunk at 20404-20423), the generated description contains a tab
character before "- TLS_AES_128_GCM_SHA256" and an inconsistent example text
that says "to use X25519 and P-256" while only showing X25519MLKEM768; fix the
source at config/v1/types_tlssecurityprofile.go by replacing the tab with spaces
(use 2 or 4 spaces for YAML indentation) and make the example consistent—either
add P-256 to the curves list shown or change the introductory sentence to
reflect only X25519MLKEM768—and then regenerate the openapi file so the
corrected text appears in zz_generated.openapi.go.
|
Based on the discussions in openshift/enhancements#1894, it sounds like this is potentially blocked on upstream kubelet/apiserver support for curves? Would it make sense to introduce this type but only apply it for the TLS configuration in ingress? Or would we like to hold this until we can ensure all components would conform to this? |
|
@yuqi-zhang I discussed this with some engineers internally and we decided that the upstream changes should not block this api change. However, we will need to do this at some point to ensure consistency. |
I don't really understand how the API will plumb through to kubelet / apiserver if upstream components don't actually set the servers to use these TLS curves. |
|
I had to revert TLS Curve support for Kueue because I can't really add support for configuring TLS curves if there isn't support for this in SecureServing as extended apiservers won't be protected. |
|
Copying your comment from the slack thread: So based on that, we are ok with TechPreview only curve support for TLS 1.2, and only for ingress and apiserver. We don't allow TLS 1.3 + curve, or curve + anything kubelet related. In that case, since this PR is currently adding curves to the tlsprofile in kubeletconfig as well, perhaps we should split those types such that tlsprofile in kubeletconfig doesn't have curve option, which I think is Kevin's concern. Please let me know if any of that is incorrect, thanks! |
|
My main concern is similar to @everettraven (openshift/enhancements#1894 (comment)). Kube-APIServer / Kubelet upstream components do not yet have the flags to set this on their respective servers. So unless I am missing something we would need to carry some kind of patch downstream that adds these CLI parameters so that the servers actually respect the API we set. Otherwise we end up having an API that doesn't actually configure the kubernetes components to use the right tls curves. |
|
"So based on that, we are ok with TechPreview only curve support for TLS 1.2, and only for ingress and apiserver. We don't allow TLS 1.3 + curve, or curve + anything kubelet related." To my knowledge this comment is correct. We are forced to keep this as TechPreview until TLS1.3 curve specification support is provided upstream. |
| // curves is used to specify the elliptic curves that are used during | ||
| // the TLS handshake. Operators may remove entries their operands do | ||
| // not support. |
There was a problem hiding this comment.
| // curves is used to specify the elliptic curves that are used during | |
| // the TLS handshake. Operators may remove entries their operands do | |
| // not support. | |
| // curves is an optional field used to specify the elliptic curves that are used during | |
| // the TLS handshake. Operators may remove entries their operands do | |
| // not support. |
| // TLSProfiles Old, Intermediate, Modern are including by default the following | ||
| // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. |
There was a problem hiding this comment.
I would probably specify this information on the actual profiles themselves like is done for the existing ciphers field.
This keeps documentation location consistent and omits what seems like an unnecessary reference to a higher-order field.
| // | ||
| // TLSProfiles Old, Intermediate, Modern are including by default the following | ||
| // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. | ||
| // TLSProfiles Custom do not include any curves by default. |
There was a problem hiding this comment.
Same comment as above, this seems better suited to the higher-order field documentation.
| // # Example: Force PQC-only encryption | ||
| // apiVersion: config.openshift.io/v1 | ||
| // kind: APIServer | ||
| // spec: | ||
| // tlsSecurityProfile: | ||
| // type: Custom | ||
| // custom: | ||
| // ciphers: | ||
| // - TLS_AES_128_GCM_SHA256 | ||
| // - TLS_AES_256_GCM_SHA384 | ||
| // - TLS_CHACHA20_POLY1305_SHA256 | ||
| // curves: | ||
| // - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve | ||
| // minTLSVersion: VersionTLS13 |
There was a problem hiding this comment.
Maybe the example is better suited to the higher-order custom field since it is showing a more holistic configuration example?
| // NOTE: since this field is optional, if no curves are specified, the default curves | ||
| // used by the underlying TLS library will be used. |
There was a problem hiding this comment.
What do you think of saying something like:
When omitted, this means no opinion and the platform is left to choose reasonable defaults which are subject to change over time and may be different per platform component depending on the underlying TLS libraries they use.
It generally follows our more standard "omission means no opinion" for configuration APIs while clearing stating that the default behavior depends on the underlying implementation per-component.
| // +listType=set | ||
| // +kubebuilder:validation:MaxItems=5 | ||
| // +openshift:enable:FeatureGate=TLSCurvesConfiguration | ||
| Curves []TLSCurve `json:"curves,omitempty"` |
There was a problem hiding this comment.
Would there be any difference in setting this to curves: [] and omitting the field altogether?
Keeping it tech preview until support is provided by core components seems reasonable to me. I suspect we would like to get the Kubernetes API server and the Kubelet to support this configuration as part of this effort since both of those configuration APIs rely on this type and any changes are subsequently reflected as configuration options on those APIs as well. I don't know that this needs to necessarily wait until TLS 1.3 curve configuration support if we are able to block configuring fields for now when minTLSVersion is set to 1.3. |
openshift-merge-robot
added
the
needs-rebase
|
/assign |
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
2ed6523 to
aa5a4df
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/v1/zz_generated.featuregated-crd-manifests.yaml (1)
172-193: AddFeatureGateTLSCurvesConfigurationto features/features.go — The gate name is correctly wired to CRD variant generation via the+openshift:enable:FeatureGate=TLSCurvesConfigurationannotation and the generated manifests are properly tagged, but the FeatureGate itself is not registered as a variable infeatures/features.go. Compare withFeatureGateKMSEncryptionProvider(line 661) to see the required pattern: define the gate withnewFeatureGate("TLSCurvesConfiguration")and callmustRegister()to make it available to the feature gate control system.
🤖 Fix all issues with AI agents
In @config/v1/types_tlssecurityprofile.go:
- Around line 179-180: The comment for TLSCurveP256r1MLKEM768 is incorrect (it
references P256r1MLKEM1024); update the comment to match the constant and value
by replacing "P256r1MLKEM1024" with "P256r1MLKEM768" so the doc comment for
TLSCurveP256r1MLKEM768 accurately reflects the constant string "P256r1MLKEM768".
- Around line 165-166: The kubebuilder Enum on type TLSCurve is missing the
constants TLSCurveP256r1MLKEM768 and TLSCurveP384r1MLKEM1024; either add those
two names to the +kubebuilder:validation:Enum list for TLSCurve so the CRD
validates them, or remove the unused constants TLSCurveP256r1MLKEM768 and
TLSCurveP384r1MLKEM1024 if they are not intended to be valid values; update the
annotation next to type TLSCurve and ensure the constant names match exactly.
In @openapi/openapi.json:
- Around line 6304-6312: In the "curves" description string for the OpenAPI
schema entry "curves", replace the mixed tab/space indentation in the YAML
example so all list items under "ciphers" and their indenting use consistent
spaces (remove the tab before "- TLS_AES_128_GCM_SHA256"), and change the
example value for "minTLSVersion" from "VersionTLS13" to "VersionTLS12" so the
example matches the documented allowed maximum; update the description text in
the "curves" property accordingly.
In
@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml:
- Around line 64-66: Fix the typo in the CRD description for the KubeletConfig
resource: change "shoud" to "should" in the description under the
machineConfigPoolSelector field so the user-facing API schema reads
"machineConfigPoolSelector selects which pools the KubeletConfig should apply
to." Ensure you update the multi-line description string associated with
machineConfigPoolSelector in the KubeletConfig CRD.
- Around line 148-180: The CRD's "curves" schema (items.enum and maxItems)
conflicts with its description/examples: the description mentions additional
curve constants like SecP256r1MLKEM1024 and SecP384r1MLKEM1024 and implies more
than five entries, but items.enum only allows five values and maxItems: 5 will
reject those examples; fix by either expanding the items.enum to include the
documented curve identifiers and remove or increase maxItems accordingly (or set
no maxItems) OR update the description/examples to only reference the five
allowed enums and set maxItems to the intended limit, then regenerate the
payload CRDs so all variants stay consistent (locate and update the "curves"
block in the TLSProfile/curves schema).
- Around line 113-116: The CRD and docs are inconsistent about TLS 1.3 support:
either remove VersionTLS13 from the enum and from the example minTLSVersion
usage (and keep the doc text saying max is VersionTLS12), or update the
documentation text (the sentence referencing "maximum available minTLSVersion is
VersionTLS12") to permit VersionTLS13 and ensure examples reflect VersionTLS13;
locate and change occurrences of the enum value VersionTLS13 and the example
minTLSVersion: VersionTLS13, or update the documentation sentence so the schema
and example match the declared support.
🧹 Nitpick comments (2)
config/v1/types_tlssecurityprofile.go (2)
224-227:MaxItems=5may be too restrictive.With 7 curve constants defined (and potentially more for PQC in the future), limiting to 5 items may prevent valid configurations. Consider whether this limit should be increased, or document the rationale for the restriction.
301-306: Default curves exclude P-521 from all profiles.The kubebuilder enum includes
P-521, but none of the default profiles (Old, Intermediate, Modern) include it. IfP-521is a valid curve, consider whether it should be included in at least the Old profile for broader compatibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (14)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/TLSCurvesConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/TLSCurvesConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/TLSCurvesConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (14)
config/v1/types_tlssecurityprofile.goconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.swagger_doc_generated.gomachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- config/v1/zz_generated.swagger_doc_generated.go
- openapi/generated_openapi/zz_generated.openapi.go
- payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-Default.crd.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(166-166)
🔇 Additional comments (8)
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yaml (1)
4-12: OKD feature-set annotation looks correct.config/v1/zz_generated.deepcopy.go (1)
6241-6254: DeepCopy forTLSProfileSpec.Curvesis correct.Copies the slice and preserves nil vs empty semantics.
config/v1/zz_generated.featuregated-crd-manifests.yaml (1)
1-14: FeatureGate addition is correct and properly wired.The
TLSCurvesConfigurationFeatureGate is defined inconfig/v1/types_tlssecurityprofile.go(line 226) with the annotation+openshift:enable:FeatureGate=TLSCurvesConfigurationgating theCurvesfield, and is consistently included across the generated CRD manifests for apiservers, ingresscontrollers, and kubeletconfigs. The entry in the reviewed file is properly generated.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml (1)
178-180: The premise of this review comment is incorrect. The kubeletconfigs CRD does not expose acurvePreferencesfield. The fieldcurvePreferencesdoes not exist in Kubernetes KubeletConfiguration. Additionally, the claim about order being significant for TLS cipher suites is inaccurate; Go'scrypto/tlspackage does not preserve the order specified inConfig.CipherSuites—it applies its own ordering rules based on security, hardware capabilities, and protocol version.If this is referring to
tlsCipherSuites(which does exist in kubeletconfigs), thex-kubernetes-list-type: setdeclaration is appropriate because order does not affect how the TLS implementation selects ciphers. If the field is something else, the review comment requires clarification about which specific field is being discussed and why order would be semantically significant for it.Likely an incorrect or invalid review comment.
machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml (1)
65-66: Verify upstream kubelet support for TLS curve configuration.The feature gate is being added for kubeletconfigs, but PR discussion indicates upstream kubelet/apiserver may lack support for configuring server TLS curves. Please confirm that this feature gate will properly gate the curves field until upstream Kubernetes provides the necessary support.
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml (2)
1-12: CRD metadata and structure look correct.The CRD is properly configured with appropriate annotations for TechPreviewNoUpgrade feature set, and the structure follows the expected pattern for KubeletConfig resources.
117-163: Thecurvesfield is already present in the feature-gated variant.The TLSCurvesConfiguration feature gate for kubeletconfigs includes the
curvesfield in its custom TLS profile. The architecture correctly separates:
- TechPreviewNoUpgrade: Base variant without curves
- TLSCurvesConfiguration.yaml: Feature-gated variant that adds curves support
- CustomNoUpgrade: Stable/custom level variant with curves
The
curvesfield includes the same validation as other components (enum values: X25519, P-256, P-384, P-521, X25519MLKEM768, with maxItems: 5). This is the intended design—the feature gate properly gates the curves field for kubeletconfigs.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1-12: CRD metadata and structure are correct for DevPreviewNoUpgrade.The annotations and feature set marking are appropriate.
| // TLSProfiles Old, Intermediate, Modern are including by default the following | ||
| // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. |
There was a problem hiding this comment.
Documentation doesn't match actual defaults.
The comment states that default profiles include "SecP256r1MLKEM1024, SecP384r1MLKEM1024", but the actual default curve lists (lines 301-306, 323-328, 337-342) only include: X25519, P-256, P-384, X25519MLKEM768.
Either update the documentation to match the actual defaults, or add the missing curves to the default profiles.
🔧 Proposed fix for documentation
- // TLSProfiles Old, Intermediate, Modern are including by default the following
- // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024.
+ // TLSProfiles Old, Intermediate, Modern are including by default the following
+ // curves: X25519, P-256, P-384, X25519MLKEM768.📝 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.
| // TLSProfiles Old, Intermediate, Modern are including by default the following | |
| // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. | |
| // TLSProfiles Old, Intermediate, Modern are including by default the following | |
| // curves: X25519, P-256, P-384, X25519MLKEM768. |
| curves: | ||
| description: "curves is used to specify the elliptic curves | ||
| that are used during\nthe TLS handshake. Operators may | ||
| remove entries their operands do\nnot support.\n\nTLSProfiles | ||
| Old, Intermediate, Modern are including by default the following\ncurves: | ||
| X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, | ||
| SecP384r1MLKEM1024.\nTLSProfiles Custom do not include any | ||
| curves by default.\nNOTE: since this field is optional, | ||
| if no curves are specified, the default curves\nused by | ||
| the underlying TLS library will be used.\n\nFor example, | ||
| to use X25519 and P-256 (yaml):\n\n# Example: Force PQC-only | ||
| encryption\napiVersion: config.openshift.io/v1\nkind: APIServer\nspec:\n | ||
| \ tlsSecurityProfile:\n type: Custom\n custom:\n ciphers:\n\t | ||
| \ - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n | ||
| \ - TLS_CHACHA20_POLY1305_SHA256\n curves:\n | ||
| \ - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant | ||
| curve\n minTLSVersion: VersionTLS13" | ||
| items: | ||
| description: |- | ||
| TLSCurve is a named curve identifier that can be used in TLSProfile.Curves. | ||
| There is a one-to-one mapping between these names and the curve IDs defined | ||
| in crypto/tls package based on IANA's "TLS Supported Groups" registry: | ||
| https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 | ||
| enum: | ||
| - X25519 | ||
| - P-256 | ||
| - P-384 | ||
| - P-521 | ||
| - X25519MLKEM768 | ||
| type: string | ||
| maxItems: 5 | ||
| type: array | ||
| x-kubernetes-list-type: set |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Go files that might define TLSCurve types
find . -type f -name "*.go" | head -20Repository: openshift/api
Length of output: 778
🏁 Script executed:
# Search for TLSCurve type definitions
rg -t go "type TLSCurve" -A 10Repository: openshift/api
Length of output: 818
🏁 Script executed:
# Search for curve constant definitions
rg -t go "X25519MLKEM768|P256r1MLKEM|SecP256r1MLKEM|P384r1MLKEM" -B 2 -A 2Repository: openshift/api
Length of output: 8037
🏁 Script executed:
cat -n config/v1/types_tlssecurityprofile.go | sed -n '1,250p'Repository: openshift/api
Length of output: 11337
🏁 Script executed:
# Look at the TLSProfile defaults to see which curves are actually used
rg -t go "DefaultOldTLSProfile|DefaultIntermediateTLSProfile|DefaultModernTLSProfile" -A 20Repository: openshift/api
Length of output: 39
Inconsistency between enum values and curve names in description.
The Go type definition (config/v1/types_tlssecurityprofile.go) defines 7 TLSCurve constants, but the kubebuilder validation enum at line 165 only lists 5: X25519;P-256;P-384;P-521;X25519MLKEM768. The missing constants are P256r1MLKEM768 and P384r1MLKEM1024.
Additionally, the curves description mentions SecP256r1MLKEM1024 and SecP384r1MLKEM1024, but the actual Go constant values are P256r1MLKEM768 and P384r1MLKEM1024—causing a name mismatch.
To align the CRD with the Go types:
- Add
P256r1MLKEM768andP384r1MLKEM1024to the enum (lines 340 and 343) - Update
maxItemsfrom 5 to 7 - Correct the curve names in the description to match the actual Go constants
| description: |- | ||
| machineConfigPoolSelector selects which pools the KubeletConfig shoud apply to. | ||
| A nil selector will result in no pools being selected. |
There was a problem hiding this comment.
Typo: “shoud” → “should”.
Minor, but this is user-facing API schema text.
🤖 Prompt for AI Agents
In
@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
around lines 64 - 66, Fix the typo in the CRD description for the KubeletConfig
resource: change "shoud" to "should" in the description under the
machineConfigPoolSelector field so the user-facing API schema reads
"machineConfigPoolSelector selects which pools the KubeletConfig should apply
to." Ensure you update the multi-line description string associated with
machineConfigPoolSelector in the KubeletConfig CRD.
| If unset, the default is based on the apiservers.config.openshift.io/cluster resource. | ||
| Note that only Old and Intermediate profiles are currently supported, and | ||
| the maximum available minTLSVersion is VersionTLS12. | ||
| properties: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the CRD file mentioned in the review
find . -name "*kubeletconfigs-CustomNoUpgrade.crd.yaml" -type fRepository: openshift/api
Length of output: 266
🏁 Script executed:
# Get the file size first
wc -l ./payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yamlRepository: openshift/api
Length of output: 151
🏁 Script executed:
# Read the file to examine the TLS configuration sections
cat -n ./payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml | head -200Repository: openshift/api
Length of output: 11753
🏁 Script executed:
# Read lines 158-195 to see the example mentioned in the review
cat -n ./payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml | sed -n '158,195p'Repository: openshift/api
Length of output: 2518
Schema contradicts documentation: TLS 1.3 support is inconsistent.
Documentation states "the maximum available minTLSVersion is VersionTLS12" (lines 113-115, 189), yet VersionTLS13 is included in the enum (lines 190-194) and explicitly used in the curves example (line 164: minTLSVersion: VersionTLS13). Update either the documentation to allow TLS 1.3 or remove it from the enum and example if TLS 1.3 is not actually supported.
🤖 Prompt for AI Agents
In
@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
around lines 113 - 116, The CRD and docs are inconsistent about TLS 1.3 support:
either remove VersionTLS13 from the enum and from the example minTLSVersion
usage (and keep the doc text saying max is VersionTLS12), or update the
documentation text (the sentence referencing "maximum available minTLSVersion is
VersionTLS12") to permit VersionTLS13 and ensure examples reflect VersionTLS13;
locate and change occurrences of the enum value VersionTLS13 and the example
minTLSVersion: VersionTLS13, or update the documentation sentence so the schema
and example match the declared support.
| curves: | ||
| description: "curves is used to specify the elliptic curves | ||
| that are used during\nthe TLS handshake. Operators may | ||
| remove entries their operands do\nnot support.\n\nTLSProfiles | ||
| Old, Intermediate, Modern are including by default the following\ncurves: | ||
| X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, | ||
| SecP384r1MLKEM1024.\nTLSProfiles Custom do not include any | ||
| curves by default.\nNOTE: since this field is optional, | ||
| if no curves are specified, the default curves\nused by | ||
| the underlying TLS library will be used.\n\nFor example, | ||
| to use X25519 and P-256 (yaml):\n\n# Example: Force PQC-only | ||
| encryption\napiVersion: config.openshift.io/v1\nkind: APIServer\nspec:\n | ||
| \ tlsSecurityProfile:\n type: Custom\n custom:\n ciphers:\n\t | ||
| \ - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n | ||
| \ - TLS_CHACHA20_POLY1305_SHA256\n curves:\n | ||
| \ - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant | ||
| curve\n minTLSVersion: VersionTLS13" | ||
| items: | ||
| description: |- | ||
| TLSCurve is a named curve identifier that can be used in TLSProfile.Curves. | ||
| There is a one-to-one mapping between these names and the curve IDs defined | ||
| in crypto/tls package based on IANA's "TLS Supported Groups" registry: | ||
| https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8 | ||
| enum: | ||
| - X25519 | ||
| - P-256 | ||
| - P-384 | ||
| - P-521 | ||
| - X25519MLKEM768 | ||
| type: string | ||
| maxItems: 5 | ||
| type: array | ||
| x-kubernetes-list-type: set |
There was a problem hiding this comment.
curves schema/doc mismatch (enum + maxItems contradicts description and examples).
The description documents defaults/values (SecP256r1MLKEM1024, SecP384r1MLKEM1024, etc.) that are not permitted by the enum, and maxItems: 5 contradicts the longer documented list. This will cause valid-looking configs to be rejected by the API server.
Proposed direction (apply at the source-of-truth, then regenerate)
- Either expand the enum to include the documented curve constants and bump/remove
maxItems, or update the description/examples to only reference the allowed enum values and a matching max. - After updating, regenerate payload CRDs so all variants stay consistent.
🤖 Prompt for AI Agents
In
@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
around lines 148 - 180, The CRD's "curves" schema (items.enum and maxItems)
conflicts with its description/examples: the description mentions additional
curve constants like SecP256r1MLKEM1024 and SecP384r1MLKEM1024 and implies more
than five entries, but items.enum only allows five values and maxItems: 5 will
reject those examples; fix by either expanding the items.enum to include the
documented curve identifiers and remove or increase maxItems accordingly (or set
no maxItems) OR update the description/examples to only reference the five
allowed enums and set maxItems to the intended limit, then regenerate the
payload CRDs so all variants stay consistent (locate and update the "curves"
block in the TLSProfile/curves schema).
…rves in TLSProfile Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
aa5a4df to
9aa8697
Compare
davidesalerno
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@davidesalerno: This pull request references NE-2334 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. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@features.md`:
- Around line 1-2: The table header in features.md is malformed: a stray "git"
entry and missing leading pipe cause the alignment row and subsequent data rows
to mismatch; fix by either removing the extra "git" column from the header or
adding a leading pipe and inserting a corresponding alignment cell in the second
row and all data rows so column counts match, focusing on the header label
"FeatureGate" and the alignment row starting with "| ------" to ensure every
column has a matching pipe-delimited header and alignment cell.
In `@openapi/openapi.json`:
- Around line 6350-6358: The description for the "curves" property is
inconsistent: it claims "For example, to use X25519 and P-256" but the YAML
snippet only shows "- X25519MLKEM768"; update the "curves" description within
the curves property to either (a) change the explanatory sentence to match the
shown example (e.g., reference X25519MLKEM768 only) or (b) expand the YAML
example to include both entries (e.g., add "- X25519MLKEM768" and "- P-256") so
the text and sample are consistent; modify the string value under the
"description" for the curves schema to implement the chosen fix.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 334-335: The documentation in the CRD lists P-521 among the
default curves but the actual defaults are defined in the TLSProfiles map
(symbol TLSProfiles) in the Go type that builds those profiles
(types_tlssecurityprofile.go) and currently only include X25519, P-256, P-384,
X25519MLKEM768; fix by either adding "P-521" to the curve lists for the
Old/Intermediate/Modern entries in the TLSProfiles map (so the code and docs
match) or update this YAML documentation text to remove P-521 so it matches the
current TLSProfiles defaults; refer to the TLSProfiles symbol and the default
profile entries (Old, Intermediate, Modern) when making the change.
In
`@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 130-156: The CRD schema for tlsSecurityProfile.custom is missing
the curves property so spec.tlsSecurityProfile.custom.curves will be rejected;
add a new property named "curves" alongside "ciphers" and "minTLSVersion" under
the same properties block for tlsSecurityProfile.custom, giving it a descriptive
"description", set type to "array" with "items: type: string" and
"x-kubernetes-list-type: atomic" (matching the ciphers pattern) so the CRD
accepts/prunes correctly and the feature gate can be exercised for
KubeletConfig.
♻️ Duplicate comments (6)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
328-361: maxItems constraint is inconsistent with available enum values.The enum defines 7 curve values (
X25519,P-256,P-384,P-521,X25519MLKEM768,P256r1MLKEM768,P384r1MLKEM1024), butmaxItems: 5prevents users from specifying all available curves simultaneously. Consider updatingmaxItemsto 7 to match the enum count.config/v1/zz_generated.swagger_doc_generated.go (1)
3005-3009: Fix the curves example to match the narrative (generated doc).The example in Line 3008 says “to use X25519 and P-256” but the YAML only includes
X25519MLKEM768, so it’s misleading. Since this file is generated, update the source comment inconfig/v1/types_tlssecurityprofile.goand regenerate.openapi/generated_openapi/zz_generated.openapi.go (2)
12433-12452: Schema structure is correct; description inconsistency already flagged.The OpenAPI schema structure for the
curvesfield is properly defined withx-kubernetes-list-type: "set"and correct array typing.The description inconsistency (stating "to use X25519 and P-256" while the example shows only
X25519MLKEM768) was already identified in a previous review. Please address the source documentation fix inconfig/v1/types_tlssecurityprofile.goand regenerate.
20907-20926: Consistent with hunk 1 - same fix applies.This is the duplicate schema definition for
TLSProfileSpec. The same description inconsistency applies here and will be resolved when the source file is corrected.config/v1/types_tlssecurityprofile.go (1)
200-201: Documentation doesn't match actual default profile curves.The comment states defaults include
X25519, P-256, P-384, P-521, X25519MLKEM768, but the actualTLSProfilesmap (lines 289-294, 311-316, 325-330) only includes:
TLSCurveX25519TLSCurveP256TLSCurveP384TLSCurveX25519MLKEM768P-521 (
TLSCurveP521) is not in the defaults. This propagates to CRD documentation as well.📝 Proposed fix (if P-521 should not be in defaults)
- // TLSProfiles Old, Intermediate, Modern are including by default the following - // curves: X25519, P-256, P-384, P-521, X25519MLKEM768 + // TLSProfiles Old, Intermediate, Modern are including by default the following + // curves: X25519, P-256, P-384, X25519MLKEM768openapi/openapi.json (1)
11331-11339: Same misleading example applies here.This schema entry has the same description inconsistency noted in the
CustomTLSProfileschema (lines 6350-6358). The fix should be applied consistently to both locations.The schema structure itself (array type,
"set"list-type for uniqueness) is appropriate for curve configuration.
| git | FeatureGate | Default on Hypershift | Default on SelfManagedHA | DevPreviewNoUpgrade on Hypershift | DevPreviewNoUpgrade on SelfManagedHA | OKD on Hypershift | OKD on SelfManagedHA | TechPreviewNoUpgrade on Hypershift | TechPreviewNoUpgrade on SelfManagedHA | | ||
| | ------ | --- | --- | --- | --- | --- | --- | --- | --- | |
There was a problem hiding this comment.
Fix the feature matrix header alignment.
Line 1 introduces a “git” column and drops the leading pipe, but the alignment row (Line 2) and data rows don’t include that extra column. This breaks the table layout.
Proposed fix
-git | FeatureGate | Default on Hypershift | Default on SelfManagedHA | DevPreviewNoUpgrade on Hypershift | DevPreviewNoUpgrade on SelfManagedHA | OKD on Hypershift | OKD on SelfManagedHA | TechPreviewNoUpgrade on Hypershift | TechPreviewNoUpgrade on SelfManagedHA |
-| ------ | --- | --- | --- | --- | --- | --- | --- | --- |
+| FeatureGate | Default on Hypershift | Default on SelfManagedHA | DevPreviewNoUpgrade on Hypershift | DevPreviewNoUpgrade on SelfManagedHA | OKD on Hypershift | OKD on SelfManagedHA | TechPreviewNoUpgrade on Hypershift | TechPreviewNoUpgrade on SelfManagedHA |
+| --- | --- | --- | --- | --- | --- | --- | --- | --- |📝 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.
| git | FeatureGate | Default on Hypershift | Default on SelfManagedHA | DevPreviewNoUpgrade on Hypershift | DevPreviewNoUpgrade on SelfManagedHA | OKD on Hypershift | OKD on SelfManagedHA | TechPreviewNoUpgrade on Hypershift | TechPreviewNoUpgrade on SelfManagedHA | | |
| | ------ | --- | --- | --- | --- | --- | --- | --- | --- | | |
| | FeatureGate | Default on Hypershift | Default on SelfManagedHA | DevPreviewNoUpgrade on Hypershift | DevPreviewNoUpgrade on SelfManagedHA | OKD on Hypershift | OKD on SelfManagedHA | TechPreviewNoUpgrade on Hypershift | TechPreviewNoUpgrade on SelfManagedHA | | |
| | --- | --- | --- | --- | --- | --- | --- | --- | --- | |
🤖 Prompt for AI Agents
In `@features.md` around lines 1 - 2, The table header in features.md is
malformed: a stray "git" entry and missing leading pipe cause the alignment row
and subsequent data rows to mismatch; fix by either removing the extra "git"
column from the header or adding a leading pipe and inserting a corresponding
alignment cell in the second row and all data rows so column counts match,
focusing on the header label "FeatureGate" and the alignment row starting with
"| ------" to ensure every column has a matching pipe-delimited header and
alignment cell.
| "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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768 TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519MLKEM768", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "x-kubernetes-list-type": "set" | ||
| }, |
There was a problem hiding this comment.
Misleading example in description.
The description text says "For example, to use X25519 and P-256" but the YAML example only shows - X25519MLKEM768. Either update the explanatory text to match what the example demonstrates, or expand the example to show both 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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768 TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519MLKEM768",
+ "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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768 TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519\n - P-256",📝 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.
| "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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768 TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519MLKEM768", | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "default": "" | |
| }, | |
| "x-kubernetes-list-type": "set" | |
| }, | |
| "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.\n\nTLSProfiles Old, Intermediate, Modern are including by default the following curves: X25519, P-256, P-384, P-521, X25519MLKEM768 TLSProfiles Custom do not include any curves by default. NOTE: since this field is optional, if no curves are specified, the default curves used by the underlying TLS library will be used.\n\nFor example, to use X25519 and P-256 (yaml):\n\n curves:\n - X25519\n - P-256", | |
| "type": "array", | |
| "items": { | |
| "type": "string", | |
| "default": "" | |
| }, | |
| "x-kubernetes-list-type": "set" | |
| }, |
🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 6350 - 6358, The description for the
"curves" property is inconsistent: it claims "For example, to use X25519 and
P-256" but the YAML snippet only shows "- X25519MLKEM768"; update the "curves"
description within the curves property to either (a) change the explanatory
sentence to match the shown example (e.g., reference X25519MLKEM768 only) or (b)
expand the YAML example to include both entries (e.g., add "- X25519MLKEM768"
and "- P-256") so the text and sample are consistent; modify the string value
under the "description" for the curves schema to implement the chosen fix.
| TLSProfiles Old, Intermediate, Modern are including by default the following | ||
| curves: X25519, P-256, P-384, P-521, X25519MLKEM768 |
There was a problem hiding this comment.
Documentation lists P-521 but it's not in the actual default profiles.
The documentation states the default profiles include X25519, P-256, P-384, P-521, X25519MLKEM768, but examining config/v1/types_tlssecurityprofile.go (lines 289-294, 311-316, 325-330), the actual TLSProfiles map only includes 4 curves: X25519, P-256, P-384, X25519MLKEM768. P-521 is not present in the defaults.
Either add P-521 to the default profile curves in the Go code, or update the documentation here to match the actual defaults.
🤖 Prompt for AI Agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml`
around lines 334 - 335, The documentation in the CRD lists P-521 among the
default curves but the actual defaults are defined in the TLSProfiles map
(symbol TLSProfiles) in the Go type that builds those profiles
(types_tlssecurityprofile.go) and currently only include X25519, P-256, P-384,
X25519MLKEM768; fix by either adding "P-521" to the curve lists for the
Old/Intermediate/Modern entries in the TLSProfiles map (so the code and docs
match) or update this YAML documentation text to remove P-521 so it matches the
current TLSProfiles defaults; refer to the TLSProfiles symbol and the default
profile entries (Old, Intermediate, Modern) when making the change.
| properties: | ||
| ciphers: | ||
| description: |- | ||
| ciphers is used to specify the cipher algorithms that are negotiated | ||
| during the TLS handshake. Operators may remove entries their operands | ||
| do not support. For example, to use DES-CBC3-SHA (yaml): | ||
| ciphers: | ||
| - DES-CBC3-SHA | ||
| items: | ||
| type: string | ||
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| minTLSVersion: | ||
| description: |- | ||
| minTLSVersion is used to specify the minimal version of the TLS protocol | ||
| that is negotiated during the TLS handshake. For example, to use TLS | ||
| versions 1.1, 1.2 and 1.3 (yaml): | ||
| minTLSVersion: VersionTLS11 | ||
| enum: | ||
| - VersionTLS10 | ||
| - VersionTLS11 | ||
| - VersionTLS12 | ||
| - VersionTLS13 | ||
| type: string | ||
| type: object |
There was a problem hiding this comment.
Add curves to tlsSecurityProfile.custom schema.
Without this, spec.tlsSecurityProfile.custom.curves will be rejected/pruned by the CRD, so the new feature gate can’t be exercised for KubeletConfig.
🔧 Proposed schema update
properties:
ciphers:
description: |-
ciphers is used to specify the cipher algorithms that are negotiated
during the TLS handshake. Operators may remove entries their operands
do not support. For example, to use DES-CBC3-SHA (yaml):
ciphers:
- DES-CBC3-SHA
items:
type: string
type: array
x-kubernetes-list-type: atomic
+ curves:
+ description: |-
+ curves is used to specify the elliptic curve algorithms negotiated
+ during ECDHE handshakes. Operators may remove entries their operands
+ do not support.
+ items:
+ type: string
+ # Keep enum values in sync with config/v1 TLSCurve constants
+ type: array
+ x-kubernetes-list-type: atomic
minTLSVersion:
description: |-
minTLSVersion is used to specify the minimal version of the TLS protocol
that is negotiated during the TLS handshake. For example, to use TLS
versions 1.1, 1.2 and 1.3 (yaml):🤖 Prompt for AI Agents
In
`@payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-TechPreviewNoUpgrade.crd.yaml`
around lines 130 - 156, The CRD schema for tlsSecurityProfile.custom is missing
the curves property so spec.tlsSecurityProfile.custom.curves will be rejected;
add a new property named "curves" alongside "ciphers" and "minTLSVersion" under
the same properties block for tlsSecurityProfile.custom, giving it a descriptive
"description", set type to "array" with "items: type: string" and
"x-kubernetes-list-type: atomic" (matching the ciphers pattern) so the CRD
accepts/prunes correctly and the feature gate can be exercised for
KubeletConfig.
|
@davidesalerno: 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. |
12 participants
This change will add a new Curves field to the TLSProfile specification.
This is required in order to support new PQC curves, we need a config for explicitly setting the supported elliptic curves algorithms ("curve suite") that are negotiated during the SSL/TLS handshake with ECDHE.
This PR is related to openshift/cluster-ingress-operator#1287 and openshift/router#678