-
Notifications
You must be signed in to change notification settings - Fork 584
Add support for TLS curves in TLSProfile #2583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -157,6 +157,31 @@ const ( | |||||||||||||||||
| TLSProfileCustomType TLSProfileType = "Custom" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| // 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 | ||||||||||||||||||
| // | ||||||||||||||||||
| // +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521;X25519MLKEM768 | ||||||||||||||||||
| type TLSCurve string | ||||||||||||||||||
|
Comment on lines
+165
to
+166
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kubebuilder enum missing two defined constants. The 🔧 Proposed fix (if all curves should be valid)-// +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521;X25519MLKEM768
+// +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521;X25519MLKEM768;P256r1MLKEM768;P384r1MLKEM1024
type TLSCurve string📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| const ( | ||||||||||||||||||
| // TLSCurveX25519 represents X25519. | ||||||||||||||||||
| TLSCurveX25519 TLSCurve = "X25519" | ||||||||||||||||||
| // TLSCurveP256 represents P-256 (secp256r1). | ||||||||||||||||||
| TLSCurveP256 TLSCurve = "P-256" | ||||||||||||||||||
| // TLSCurveP384 represents P-384 (secp384r1). | ||||||||||||||||||
| TLSCurveP384 TLSCurve = "P-384" | ||||||||||||||||||
| // TLSCurveP521 represents P-521 (secp521r1). | ||||||||||||||||||
| TLSCurveP521 TLSCurve = "P-521" | ||||||||||||||||||
| // TLSCurveX25519MLKEM768 represents X25519MLKEM768. | ||||||||||||||||||
| TLSCurveX25519MLKEM768 TLSCurve = "X25519MLKEM768" | ||||||||||||||||||
| // TLSCurveP256r1MLKEM1024 represents P256r1MLKEM1024 (secp256r1MLKEM1024). | ||||||||||||||||||
| TLSCurveP256r1MLKEM768 TLSCurve = "P256r1MLKEM768" | ||||||||||||||||||
|
Comment on lines
+179
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment typo: says "MLKEM1024" but constant is "MLKEM768". The comment states 🔎 Proposed fix- // TLSCurveP256r1MLKEM1024 represents P256r1MLKEM1024 (secp256r1MLKEM1024).
+ // TLSCurveP256r1MLKEM768 represents P256r1MLKEM768 (secp256r1MLKEM768).
TLSCurveP256r1MLKEM768 TLSCurve = "P256r1MLKEM768"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good bot. I'm unclear as to which this constant is supposed to actually represent
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this value doesn't seem to be represented in the allowed values based on the enum marker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @everettraven You're absolutely correct! 🎯 Both of the new post-quantum cryptography (PQC) curve constants are missing from the enum validation marker:
The current enum marker on line 210: // +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521;X25519MLKEM768Should be updated to include both new curves: // +kubebuilder:validation:Enum=X25519;P-256;P-384;P-521;X25519MLKEM768;P256r1MLKEM768;P384r1MLKEM1024Without this fix, Kubernetes CRD validation will reject any attempts to use these new PQC curves, even though the constants are defined. This needs to be corrected before merge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@everettraven You've identified an important issue! The 🏁 Script executed: Length of output: 10645 Based on the search results, I found multiple inconsistencies that need clarification from @davidesalerno: Issues Found:
Questions for @davidesalerno:
Without these constants being in the enum, users cannot actually use them even though they're defined in the code. 🧬 Code Graph Analysis Results[config/v1/zz_generated.swagger_doc_generated.go:2867-2869] [config/v1/zz_generated.swagger_doc_generated.go:2851-2853] [config/v1/zz_generated.swagger_doc_generated.go:2859-2861] [config/v1/zz_generated.swagger_doc_generated.go:2843-2845] [config/v1/zz_generated.swagger_doc_generated.go:2878-2880]
Comment on lines
+179
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment/constant name mismatch. The comment says "P256r1MLKEM1024" but the constant value is 🔧 Proposed fix- // TLSCurveP256r1MLKEM1024 represents P256r1MLKEM1024 (secp256r1MLKEM1024).
+ // TLSCurveP256r1MLKEM768 represents P256r1MLKEM768 (secp256r1MLKEM768).
TLSCurveP256r1MLKEM768 TLSCurve = "P256r1MLKEM768"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| // TLSCurveP384r1MLKEM1024 represents P384r1MLKEM1024 (secp384r1MLKEM1024). | ||||||||||||||||||
| TLSCurveP384r1MLKEM1024 TLSCurve = "P384r1MLKEM1024" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
Comment on lines
+165
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n config/v1/types_tlssecurityprofile.go | sed -n '210,228p'Repository: openshift/api Length of output: 973 🏁 Script executed: # Check the exact kubebuilder annotation
rg -n "kubebuilder:validation:Enum" config/v1/types_tlssecurityprofile.go -A 1
# Check all TLSCurve constants
rg -n "TLSCurve\w+\s+TLSCurve\s*=" config/v1/types_tlssecurityprofile.go -B 1Repository: openshift/api Length of output: 1120 🏁 Script executed: # Check if P256r1MLKEM768 and P384r1MLKEM1024 are used elsewhere
rg "P256r1MLKEM768|P384r1MLKEM1024" --type go -iRepository: openshift/api Length of output: 3577 Add The constants 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good bot. This needs addressing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @everettraven Thank you for confirming! I'll plan the fix to add the missing curves to the validation enum. [plan_code_changes] Task: Add missing TLS curves to kubebuilder enum validation
|
||||||||||||||||||
|
|
||||||||||||||||||
| // TLSProfileSpec is the desired behavior of a TLSSecurityProfile. | ||||||||||||||||||
| type TLSProfileSpec struct { | ||||||||||||||||||
| // ciphers is used to specify the cipher algorithms that are negotiated | ||||||||||||||||||
|
|
@@ -168,6 +193,38 @@ type TLSProfileSpec struct { | |||||||||||||||||
| // | ||||||||||||||||||
| // +listType=atomic | ||||||||||||||||||
| Ciphers []string `json:"ciphers"` | ||||||||||||||||||
| // curves is used to specify the elliptic curves that are used during | ||||||||||||||||||
| // the TLS handshake. Operators may remove entries their operands do | ||||||||||||||||||
| // not support. | ||||||||||||||||||
|
Comment on lines
+196
to
+198
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| // | ||||||||||||||||||
| // TLSProfiles Old, Intermediate, Modern are including by default the following | ||||||||||||||||||
| // curves: X25519, P-256, P-384, P-521, X25519MLKEM768, SecP256r1MLKEM1024, SecP384r1MLKEM1024. | ||||||||||||||||||
|
Comment on lines
+200
to
+201
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Comment on lines
+200
to
+201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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
Suggested change
|
||||||||||||||||||
| // TLSProfiles Custom do not include any curves by default. | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above, this seems better suited to the higher-order field documentation. |
||||||||||||||||||
| // NOTE: since this field is optional, if no curves are specified, the default curves | ||||||||||||||||||
| // used by the underlying TLS library will be used. | ||||||||||||||||||
|
Comment on lines
+203
to
+204
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of saying something like:
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. |
||||||||||||||||||
| // | ||||||||||||||||||
| // For example, to use X25519 and P-256 (yaml): | ||||||||||||||||||
| // | ||||||||||||||||||
| // # 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 | ||||||||||||||||||
|
Comment on lines
+208
to
+221
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this render all right in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
I think the best approach here is to offer reasonable valid defaults while keeping custom profiles as a "power user" option.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the example is better suited to the higher-order custom field since it is showing a more holistic configuration example? |
||||||||||||||||||
| // | ||||||||||||||||||
| // +optional | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optional fields should have godoc around what happens if the field is not set (i.e. what is the default behaviour)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a note to address this case. What do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically you don't need the |
||||||||||||||||||
| // +listType=set | ||||||||||||||||||
| // +kubebuilder:validation:MaxItems=5 | ||||||||||||||||||
| // +openshift:enable:FeatureGate=TLSCurvesConfiguration | ||||||||||||||||||
| Curves []TLSCurve `json:"curves,omitempty"` | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would there be any difference in setting this to |
||||||||||||||||||
| // 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): | ||||||||||||||||||
|
|
@@ -241,6 +298,12 @@ var TLSProfiles = map[TLSProfileType]*TLSProfileSpec{ | |||||||||||||||||
| "AES256-SHA", | ||||||||||||||||||
| "DES-CBC3-SHA", | ||||||||||||||||||
| }, | ||||||||||||||||||
| Curves: []TLSCurve{ | ||||||||||||||||||
| TLSCurveX25519, | ||||||||||||||||||
| TLSCurveP256, | ||||||||||||||||||
| TLSCurveP384, | ||||||||||||||||||
| TLSCurveX25519MLKEM768, | ||||||||||||||||||
| }, | ||||||||||||||||||
| MinTLSVersion: VersionTLS10, | ||||||||||||||||||
| }, | ||||||||||||||||||
| TLSProfileIntermediateType: { | ||||||||||||||||||
|
|
@@ -257,6 +320,12 @@ var TLSProfiles = map[TLSProfileType]*TLSProfileSpec{ | |||||||||||||||||
| "DHE-RSA-AES128-GCM-SHA256", | ||||||||||||||||||
| "DHE-RSA-AES256-GCM-SHA384", | ||||||||||||||||||
| }, | ||||||||||||||||||
| Curves: []TLSCurve{ | ||||||||||||||||||
| TLSCurveX25519, | ||||||||||||||||||
| TLSCurveP256, | ||||||||||||||||||
| TLSCurveP384, | ||||||||||||||||||
| TLSCurveX25519MLKEM768, | ||||||||||||||||||
| }, | ||||||||||||||||||
| MinTLSVersion: VersionTLS12, | ||||||||||||||||||
| }, | ||||||||||||||||||
| TLSProfileModernType: { | ||||||||||||||||||
|
|
@@ -265,6 +334,12 @@ var TLSProfiles = map[TLSProfileType]*TLSProfileSpec{ | |||||||||||||||||
| "TLS_AES_256_GCM_SHA384", | ||||||||||||||||||
| "TLS_CHACHA20_POLY1305_SHA256", | ||||||||||||||||||
| }, | ||||||||||||||||||
| Curves: []TLSCurve{ | ||||||||||||||||||
| TLSCurveX25519, | ||||||||||||||||||
| TLSCurveP256, | ||||||||||||||||||
| TLSCurveP384, | ||||||||||||||||||
| TLSCurveX25519MLKEM768, | ||||||||||||||||||
| }, | ||||||||||||||||||
| MinTLSVersion: VersionTLS13, | ||||||||||||||||||
| }, | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,6 +325,39 @@ spec: | |
| type: string | ||
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| 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 | ||
|
Comment on lines
+328
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mismatch between description and enum values for The description states that Old/Intermediate/Modern profiles include 7 curves by default:
However, the enum (lines 356-361) only defines 5 values:
Missing from enum: Additionally, 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 |
||
| minTLSVersion: | ||
| description: |- | ||
| minTLSVersion is used to specify the minimal version of the TLS protocol | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at cryto/tls, I see the following values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I think we can align to those values the available curve supported