-
Notifications
You must be signed in to change notification settings - Fork 500
Define KEP for supporting modern tls settings #8246
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: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
ack, I will try to give an initial pass this week. Otherwise after returning in January. Maybe @tenzen-y will be able to review in the meanwhile |
| // CurveP256 represents the P-256 elliptic curve | ||
| CurveP256 CurveID = "CurveP256" | ||
| // CurveP384 represents the P-384 elliptic curve | ||
| CurveP384 CurveID = "CurveP384" | ||
| // CurveP521 represents the P-521 elliptic curve | ||
| CurveP521 CurveID = "CurveP521" | ||
| // X25519 represents the X25519 elliptic curve | ||
| CurveX25519 CurveID = "X25519" |
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.
Is it constraining the options to only those listed here? Are we sure there will be no other new options?
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 the TLS 1.3 RFC, curve488 is missing for TLS 1.3 at least, but that's not a comprehensive list (this is here IIUC).
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.
https://github.com/golang/go/blob/release-branch.go1.25/src/crypto/tls/common.go#L145
If there are new options we can add new enum fields. They should only change when we update golang so I don't think this is too burdensome.
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.
Will we need to re-declare the constants in our code, or we could use the exposed constants?
They should only change when we update golang so I don't think this is too burdensome.
Yes, but we may forget to do so, making sure we remember is another step in the process. So, it would be better avoiding listing of all options in our code (if feasible).
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.
So we could with the alternative and maybe just skip validation of this and let the server fail if someone specifies a wrong value.
| // TLS 1.3 cipher suites | ||
| CipherSuiteTLSAES128GCMSHA256 CipherSuite = "TLS_AES_128_GCM_SHA256" | ||
| CipherSuiteTLSAES256GCMSHA384 CipherSuite = "TLS_AES_256_GCM_SHA384" | ||
| CipherSuiteTLSCHACHA20POLY1305SHA256 CipherSuite = "TLS_CHACHA20_POLY1305_SHA256" |
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.
Also, is it closed world, or open-world set? If closed, because I see // +kubebuilder:validation:Enum=TLS_RSA_WITH_AES_128_CBC_SHA;TLS_RSA_WITH_AES_256_CBC_SHA;... then how sure we are no new options will be added. It feels better to keep it open world, but I'm not totally sure what are the policies here.
Is there something analogous in the core k8s we could use as example?
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.
I see some examples in our component-base / apiserver.
I see mostly configs accepting a raw string and then doing the validation in the code.
I was thinking it would be better to provide a list of allowed 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.
We could switch to providing strings and validate.
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.
I added this as an option in alternatives.
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.
There's also some opinionatedness in the Go stl. For example, for TLS 1.3 there are five "official" cipher suites but Go supports three (with an ongoing discussion to add support for the CCM ones at golang/go#27484).
One thing that I noticed is that our enum lists some ciphers which are listed as "insecure" in the Go crypto library. For example TLS_RSA_WITH_AES_128_GCM_SHA256 is here in the InsecureCipherSuites list.
@kannon92 Do you remember what was the source of the list of ciphers on our end? The 1.3 ones are the same, but we have the four extra TLS_RSA_WITH... in our enum for 1.2.
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.
Yea for TLS 1.3 there is actually no configuration allowed AFAIK.
The issue I got from the field is that there are still lots of folks that use their own ciphersuites so I think 1.2 configuration is still important.
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.
What about the 4 insecure ciphers I mentioned though? Do we want to support them in Kueue? If so, do we want to add all the rest of the TLS 1.2 cipher suites that are insecure, but listed here https://pkg.go.dev/crypto/tls#pkg-constants.
Also - the kubelet configuration shows a warning when an insecure cipher suite is used. Something to consider here as well maybe?
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.
Yeah, the cipher suite configuration is ignored for 1.3. Which makes me think this should probably be clear to the user via a comment. For example, someone might try to disable TLS_CHACHA20_POLY1305_SHA256 for TLS 1.3 since it's not NIST approved.
Also, if I'd like to set minVersion to 1.2 and limit the allowed cipher suites for 1.2, it's not clear from the API whether I should also include the 1.3 cipher suites, i.e. specify:
[<allowlisted_tls_1.2_cipher_suites>, <all_tls_1.3_cipher_suites>]
or just
[<allowlisted_tls_1.2_cipher_suites>]
(the result will be the same though).
EDIT: Here's how the kubelet seems to handle it.
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.
I will drop insurecure ciphers.
I may have grabbed the wrong enums so thanks for reviewing this in detail.
I was going to update our API to just allow:
https://cs.opensource.google/go/go/+/refs/tags/go1.25.5:src/crypto/tls/cipher_suites.go;l=56
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.
I pushed up an update for this.
ba4319c to
620d023
Compare
|
/hold As I was working on the implementation, I realize that Curves are not yet present in upstream kubernetes. |
and curve preferences
620d023 to
7d599cc
Compare
|
I did some updates to this. I realize that curves are a lot more complicated than I thought. There is not yet support in upstream kubernetes so it would not be possible to implement this for visibility server. Visibility inherits settings for extended apiservers and curves are not an option yet. So for now, I refactored this to focus on TLSMinVersion and CipherSuite. Both are options already present in Kubernetes in both kubelet and apiserver. Kubelet: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubelet/config/v1beta1/types.go#L182 |
|
/hold cancel |
| - Environment variables are less suitable for complex list values | ||
| - Cannot be managed through GitOps workflows as easily | ||
|
|
||
| ### String based api |
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.
One thing to mention about this approach is that it seems to be used in the KubeletConfiguration API in core.
There's also a InitializeTLS function to take some inspiration from maybe? (handling insecure ciphers etc.)
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.
Yea what option do you think we should do?
Kubelet/APIServer have a lot of validation logic to make sure that people pass the right ciphers to the server.
I was thinking it would be better to have it at the API level and avoid the validation.
But I am open to whatever.
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.
If I was the one implementing, I'd probably lean towards string + validation since it would make the dependency on the tls package a bit more explicit (i.e. we rely on the tls package to define the allowed values), but I don't see a huge difference in practice because the set of cipher suites is not changing in practice. We also wouldn't have to decide which suites to support and just defer to CipherSuites and InsecureCipherSuites. And this approach is also used in KubeletConfiguration.
One hypothetical scenario where maybe this would make a difference is if the Go library deems some cipher insecure. For example the CBC ones in TLS 1.2 are now "weak" (example) even though they not "insecure" in the tls library, and I think are being deprecated in some environments outside of Go. So if Go follows suit and moves those ciphers to InsecureCiphers in the tls lib, we would get different behaviour based on the implementation we choose here. Although I'm not sure how likely this is to happen, I'd wager it's not very likely so this might be moot.
On the other hand, couldn't we move from enum to string in a backwards compatible way in the future? So since right now we don't foresee any more validations other than expecting specific values, the enum seems like a good place to start.
TL;DR - I don't have a super strong opinion. Maybe someone with more experience designing Kueue APIs can chime in. cc @mimowo
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.
IMO the main reason I want the enums is that the UX is a lot better to an admin who needs to configure the server.
They can check the API docs and see what values are allowed.
If we provide a string or []string they have to reverse engineer from the validation logic to know what are the correct enum 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.
/lgtm
One thing I'd also like to discuss further is documenting the fact you cannot change which cipher suites are enabled for 1.3, but I think this could be continued in the implementation PR.
I don't have a strong opinion on the strings/enum discussion, but I left my thoughts.
| - Environment variables are less suitable for complex list values | ||
| - Cannot be managed through GitOps workflows as easily | ||
|
|
||
| ### String based api |
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.
If I was the one implementing, I'd probably lean towards string + validation since it would make the dependency on the tls package a bit more explicit (i.e. we rely on the tls package to define the allowed values), but I don't see a huge difference in practice because the set of cipher suites is not changing in practice. We also wouldn't have to decide which suites to support and just defer to CipherSuites and InsecureCipherSuites. And this approach is also used in KubeletConfiguration.
One hypothetical scenario where maybe this would make a difference is if the Go library deems some cipher insecure. For example the CBC ones in TLS 1.2 are now "weak" (example) even though they not "insecure" in the tls library, and I think are being deprecated in some environments outside of Go. So if Go follows suit and moves those ciphers to InsecureCiphers in the tls lib, we would get different behaviour based on the implementation we choose here. Although I'm not sure how likely this is to happen, I'd wager it's not very likely so this might be moot.
On the other hand, couldn't we move from enum to string in a backwards compatible way in the future? So since right now we don't foresee any more validations other than expecting specific values, the enum seems like a good place to start.
TL;DR - I don't have a super strong opinion. Maybe someone with more experience designing Kueue APIs can chime in. cc @mimowo
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kannon92, kshalot 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 |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 51ffd9fbf820a8214738e5d03173c111db8f0767 |
|
as he is interested in helping out on API reviews for Kueue. |
What type of PR is this?
/kind feature
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
KEP for #8190
Special notes for your reviewer:
Does this PR introduce a user-facing change?