Add 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.
| 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
WalkthroughAdds a TLSCurve type and constants; introduces a Curves []TLSCurve field on TLSProfileSpec with defaults for Old/Intermediate/Modern; updates deepcopy, OpenAPI/Swagger, and numerous CRD manifests (APIServer, KubeletConfig, IngressController, feature-gated and payload variants) to expose and validate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
![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
Show resolved
Hide resolved
...z_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
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.
| // +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
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
eba1e60 to
4c8e011
Compare
@Miciah |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295:curvesschema is consistent with other APIServer CRDs.The enum, list constraints, and description match the TechPreview and Custom variants, keeping the API surface aligned. Documentation nit about the example has already been noted in the TechPreview CRD.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-364: Payload CRD’scurvesdefinition stays consistent with generated CRDs.The
curvesarray schema (enum, limits, description) aligns with the generated CustomNoUpgrade CRD and other variants, which should help avoid drift between payload and source manifests.
🧹 Nitpick comments (1)
openapi/openapi.json (1)
6006-6014: Verify that curve names and descriptions align with actual type definitions.The
curvesfield schema is duplicated across bothTLSProfileSpec(lines 6006–6014) andCustomTLSProfile(lines 10774–10782) with identical structures and descriptions. The schema correctly usesarraytype withatomiclist behavior and includes helpful YAML examples.However, the description references specific curve names (X25519, P-256, P-384, P-521, X25519MLKEM768) and default curves per profile (Old, Intermediate, Modern). Ensure these curve names match the actual
TLSCurveconstant definitions and that the default curves described are correctly implemented in the code.Since the description is identical in both locations, consider centralizing it (e.g., via a shared
$refdefinition for curves schema) to reduce duplication and maintenance burden:{ "definitions": { "com.github.openshift.api.config.v1.TLSCurvesField": { "description": "curves is used to specify the elliptic curves...", "type": "array", "items": { "type": "string", "default": "" }, "x-kubernetes-list-type": "atomic" } } }Then reference it from both schemas:
"curves": { "$ref": "#/definitions/com.github.openshift.api.config.v1.TLSCurvesField" }Also applies to: 10774-10782
📜 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 (6)
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml
- config/v1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.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.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/types_tlssecurityprofile.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.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.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonconfig/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.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.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (12)
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-179: Schema structure for curves field is sound and properly integrated.The curves field is correctly positioned within the custom TLS profile, with appropriate constraints (enum validation, atomic list type, maxItems: 5) and comprehensive documentation mapping to IANA TLS Supported Groups. The optional nature with fallback to TLS library defaults is a reasonable design choice.
operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml (1)
2016-2047: Curves field correctly exposed in both spec and status sections.The IngressController manifests properly reflect the architecture pattern: spec.tlsSecurityProfile.custom.curves captures user-configured curves, while status.tlsProfile.curves reflects the effective TLS curve configuration in use. This separation of intent (spec) from observation (status) is architecturally sound and consistent across both locations.
Also applies to: 3334-3364
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-364: Curves field implementation is consistent across all resource types.The APIServer CRD properly defines the curves field with identical constraints and documentation as KubeletConfig and IngressController, ensuring uniform API semantics across components. The inline example is appropriately scoped to APIServer context.
Summary
All three CRD manifest files properly integrate the new
Curvesfield for TLS profile customization with consistent schema design across KubeletConfig, IngressController, and APIServer resources.Strengths:
- ✅ Consistent enum values (X25519, P-256, P-384, P-521, X25519MLKEM768) across all files
- ✅ Proper validation constraints (maxItems: 5, atomic list type, string enum)
- ✅ Comprehensive documentation linking to IANA registry
- ✅ Optional field with sensible fallback to TLS library defaults
- ✅ Correct architectural pattern: curves in both spec (intent) and status (observation) for IngressController
- ✅ Post-quantum curve (X25519MLKEM768) included; X448 intentionally omitted (likely due to Go crypto/tls limitations noted in PR comments)
Observations requiring verification:
- The field documentation states that "Old, Intermediate, Modern profiles include by default" certain curves, but these curves are only configurable under the
Customprofile in the schemas provided. Verify that Go code (types_tlssecurityprofile.go, deepcopy, defaults logic) properly implements default curves for Old/Intermediate/Modern profiles.- The
maxItems: 5constraint exactly matches the current enum cardinality; consider whether this should be slightly higher to allow future curve additions without requiring schema updates.config/v1/zz_generated.deepcopy.go (1)
5997-6008: Deep-copy logic forCurvesis correct and consistent withCiphers.Slice allocation and
copymirror existing patterns and avoid aliasing; no issues here.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-364: CustomNoUpgrade CRD’scurvesfield matches the intended TLSCurve API.Enum values, list typing, and constraints mirror the other APIServer CRDs and the Go type, so this manifest stays in sync with the rest of the TLS profile surface.
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
333-364: LGTM! TLS curves field correctly defined.The curves field is properly configured with the complete enum including X25519MLKEM768 (the PQC hybrid curve). The validation constraints (maxItems: 5, atomic list type) and documentation align with the source type definition and other CRD manifests.
config/v1/types_tlssecurityprofile.go (3)
205-224: LGTM! TLSCurve type properly defined.The TLSCurve type and constants are well-documented with references to IANA registry and crypto/tls mapping. The kubebuilder enum annotation includes all five supported curves including the PQC hybrid X25519MLKEM768.
237-267: LGTM! Curves field well-documented with clear examples.The Curves field documentation clearly explains:
- Default behavior for Old/Intermediate/Modern profiles
- Custom profile behavior (no defaults)
- Fallback to library defaults when omitted
- Practical PQC-only configuration example
The validation constraints (optional, maxItems=5, listType=atomic) are appropriate.
338-343: LGTM! Default curve sets support post-quantum cryptography.The default curves for Old, Intermediate, and Modern profiles consistently include the hybrid PQC curve X25519MLKEM768 alongside classical curves (X25519, P-256, P-384), enabling quantum-resistant key exchange by default.
Also applies to: 360-365, 374-379
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml (1)
264-295: LGTM! Generated CRD schema matches source definition.The curves field in the ungated APIServer CRD is correctly generated with the complete enum including X25519MLKEM768 and appropriate validation constraints, consistent with the source type definition.
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
147-178: LGTM! KubeletConfig CRD now supports TLS curve configuration.The curves field is properly integrated into the KubeletConfig CRD's tlsSecurityProfile, enabling kubelet TLS connections to leverage post-quantum cryptography. The schema is consistent with other TLSSecurityProfile consumers across the platform.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295: LGTM! Default CRD enables post-quantum cryptography.The curves field is correctly defined with the complete enum including X25519MLKEM768. This enables clusters using the Default feature set to configure post-quantum hybrid key exchange, aligning with the PR's goal of supporting PQC curves.
| 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, X25519MLKEM768\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: atomic |
There was a problem hiding this comment.
Schema for tlsSecurityProfile.custom.curves looks correct; fix minor doc/example mismatch.
The field type/enum/maxItems/list-type are consistent with the new TLSCurve API and look fine. The description, however, says “For example, to use X25519 and P-256 (yaml):” but the embedded YAML shows a PQC‑only example using only X25519MLKEM768. Consider aligning the prose and example to avoid confusion.
🤖 Prompt for AI Agents
In
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
around lines 333 to 364, the prose says "For example, to use X25519 and P-256
(yaml):" but the embedded YAML shows a PQC-only example using only
X25519MLKEM768; update the doc so the prose and example match by either (A)
changing the prose to indicate this is a PQC-only example using X25519MLKEM768,
or (B) replacing the embedded YAML with a short example that actually lists
X25519 and P-256 under curves (and keeps the surrounding example text
consistent); ensure comments and inline notes reflect whichever example you
choose.
| }, | ||
| "profileCustomizations": { | ||
| "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles.", | ||
| "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by' default in the cluster, this field will be removed in 4.24.", |
There was a problem hiding this comment.
Unrelated schema changes mixed into this PR.
This PR is scoped to add TLS curves support, but the following hunks appear to address unrelated concerns and should likely be split into separate PRs for clarity and reviewability:
- Line 10595:
profileCustomizationsdeprecation note update - Line 24776:
OSImageStreamdescription update - Line 24801:
OSImageStreamStatusdefault addition - Lines 24836–24886: New
OSImageStreamSettype andOSImageStreamStatusstructure updates - Line 24895: OCI image reference description update
Confirm whether these changes are intentional. If so, update the PR description and title to reflect the broader scope. Otherwise, move them to separate PRs.
Also applies to: 24776-24776, 24801-24801, 24836-24886, 24895-24895
🤖 Prompt for AI Agents
openapi/openapi.json lines 10595, 24776, 24801, 24836-24886, 24895: unrelated
schema changes (profileCustomizations deprecation update, OSImageStream
description/status/defaults, new OSImageStreamSet type, OCI image reference
text) are mixed into this TLS-curves PR; either (A) if these edits were
intentional, update the PR title and description to list these schema changes
and their rationale and keep them in this branch, or (B) if they are accidental,
remove these hunks from this branch and create a separate PR containing only the
schema changes (or cherry-pick them to a new branch), then restore
openapi/openapi.json in this PR to only include TLS curves changes. Ensure
commit history and PR diff reflect the chosen approach and run the OpenAPI/CI
validation after moving or updating changes.
|
@davidesalerno: The following test 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. |
|
Replaces #2512 |
|
@davidesalerno please work with @richardsonnick to get a jira/rfe number associated with this PR. |
| // 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.
9 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