-
Notifications
You must be signed in to change notification settings - Fork 582
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -202,6 +202,27 @@ 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 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // TLSProfileSpec is the desired behavior of a TLSSecurityProfile. | ||||||||||||||||||||||
| type TLSProfileSpec struct { | ||||||||||||||||||||||
| // ciphers is used to specify the cipher algorithms that are negotiated | ||||||||||||||||||||||
|
|
@@ -213,6 +234,37 @@ 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. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // 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. | ||||||||||||||||||||||
|
Comment on lines
+241
to
+245
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
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
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 think that this depends on the component desiderata: if it must always be PQC compliant so if |
||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // 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
+249
to
+262
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. |
||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // +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=atomic | ||||||||||||||||||||||
| // +kubebuilder:validation:MaxItems=5 | ||||||||||||||||||||||
| Curves []TLSCurve `json:"curves,omitempty"` | ||||||||||||||||||||||
|
Comment on lines
+265
to
+267
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 it make sense to allow duplicates, or should we have some validation to prevent that (something like
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 think it is a really good idea! What do you think if we switch to
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 think sets should work to ensure that only one entry of each TLSCurve exists |
||||||||||||||||||||||
| // 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): | ||||||||||||||||||||||
|
|
@@ -283,6 +335,12 @@ var TLSProfiles = map[TLSProfileType]*TLSProfileSpec{ | |||||||||||||||||||||
| "AES256-SHA", | ||||||||||||||||||||||
| "DES-CBC3-SHA", | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| Curves: []TLSCurve{ | ||||||||||||||||||||||
| TLSCurveX25519, | ||||||||||||||||||||||
| TLSCurveP256, | ||||||||||||||||||||||
| TLSCurveP384, | ||||||||||||||||||||||
| TLSCurveX25519MLKEM768, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| MinTLSVersion: VersionTLS10, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| TLSProfileIntermediateType: { | ||||||||||||||||||||||
|
|
@@ -299,6 +357,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: { | ||||||||||||||||||||||
|
|
@@ -307,6 +371,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 |
|---|---|---|
|
|
@@ -330,6 +330,38 @@ 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, 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 | ||
|
Comment on lines
+333
to
+364
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. Schema for The field type/enum/maxItems/list-type are consistent with the new 🤖 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