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.
Needs 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)
| // - 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.
| 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.
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.
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
6d3bb10 to
9754d52
Compare
WalkthroughThis PR introduces a new 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 Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 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