-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Support for custom tls cipher suites in api server and kubelet #48859
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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/test-infra repository. I understand the commands that are listed here. |
CLA signed |
|
||
func NewCipherSuitesFlag() *CipherSuitesFlag { | ||
return &CipherSuitesFlag{ | ||
"TLS_RSA_WITH_RC4_128_SHA": 0x0005, |
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.
prefer tls constants here
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.
see https://github.com/liggitt/origin/blob/master/pkg/cmd/server/crypto/crypto.go#L72-L112 for an example, and https://github.com/liggitt/origin/blob/master/pkg/cmd/server/crypto/crypto_test.go#L16-L54 for a test to make sure the allowed values stays up to date with the ciphers supported by the TLS stack
|
||
// CipherSuites is the list of allowed cipher suites for the server. | ||
// Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). | ||
CipherSuites string |
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.
this should probably go in SecureServingOptions and be []string, tied to a string slice flag
Parameter set to []string, new test added and using constants instead of numbers |
|
||
// CipherSuites is the list of allowed cipher suites for the server. | ||
// Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). | ||
CipherSuites []string |
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.
still needs to be moved to SecureServingOptions
@@ -246,6 +255,16 @@ func (s *SecureServingOptions) applyServingInfoTo(c *server.Config) error { | |||
} | |||
} | |||
|
|||
if len(s.ServerCert.CipherSuites) != 0 { | |||
csFlag := utilflag.NewCipherSuitesFlag() | |||
cipherSuites, err := csFlag.StrToUInt16(s.ServerCert.CipherSuites) |
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.
this can just be a util function, no need for a flag type now
Sorry, i missed that part. Updated and type removed using just a function instead. |
/unassign |
fs.StringSliceVar(&s.CipherSuites, "tls-cipher-suites", s.CipherSuites, | ||
"Comma-separated list of cipher suites for the server. "+ | ||
"Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). "+ | ||
"If ommited, the default Go cipher suites will be used") |
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.
omitted
|
||
// ciphers maps strings into tls package cipher constants in | ||
// https://golang.org/pkg/crypto/tls/#pkg-constants | ||
var ciphers = map[string]uint16{ |
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.
put these somewhere the apiserver and kubelet can both reference 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.
After adding support for the kubelet, i didn't need to move this. It is accesible from the kubelet code, indeed, it was already imported so i didn't need to import anything.
Let me know if you still prefer to put it in another location, and if so, what would be the best one?
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
\o/ |
thanks @liggitt ! |
Yeeees! Merged ;) Thanks all! |
@@ -442,6 +442,10 @@ func AddKubeletConfigFlags(fs *pflag.FlagSet, c *kubeletconfig.KubeletConfigurat | |||
"If --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key "+ | |||
"are generated for the public address and saved to the directory passed to --cert-dir.") | |||
fs.StringVar(&c.TLSPrivateKeyFile, "tls-private-key-file", c.TLSPrivateKeyFile, "File containing x509 private key matching --tls-cert-file.") | |||
fs.StringSliceVar(&c.TLSCipherSuites, "tls-cipher-suites", c.TLSCipherSuites, | |||
"Comma-separated list of cipher suites for the server. "+ | |||
"Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). "+ |
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.
List the values in this help message?
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, that's a good idea
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 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.
Thanks for following up
@@ -134,6 +137,11 @@ func (s *SecureServingOptions) AddFlags(fs *pflag.FlagSet) { | |||
"Controllers. This must be a valid PEM-encoded CA bundle. Altneratively, the certificate authority "+ | |||
"can be appended to the certificate provided by --tls-cert-file.") | |||
|
|||
fs.StringSliceVar(&s.TLSCipherSuites, "tls-cipher-suites", s.TLSCipherSuites, | |||
"Comma-separated list of cipher suites for the server. "+ | |||
"Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). "+ |
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.
Same comment
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384": tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384": tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, | ||
"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, |
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 humans are supposed to type these things in, maybe omit the "TLS_" prefix, as it only adds verbosity?
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.
those map to IANA names (https://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-parameters-4)... I'd keep them
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.
looks like TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305
and TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
were added in golang lacking the hash portion of the suite IDs (_SHA256
)
but that's due to the same suffix was added last minute in the CHACHA20_POLY1305 draft:
https://tools.ietf.org/rfcdiff?url2=draft-ietf-tls-chacha20-poly1305-04.txt
🤦♂️
xref #81145
// Supported cipher | ||
clientCiphers: []uint16{tls.TLS_RSA_WITH_AES_256_CBC_SHA}, | ||
expectedError: false, | ||
}, |
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 would prefer to see two tests for each cipher, one verifying it works when present, one verifying it doesn't when not.
Ok will take care of these two things in another PR (the values in the help message and the tests) |
Any chance of this getting cherry-picked to earlier versions? |
Given that it is a new feature and the size of the change, that is unlikely |
@lavalamp regarding the tests changes, i've might find an issue in how Go handles the ciphers. From the list of Go supported 22 cipher suites here https://golang.org/pkg/crypto/tls/#pkg-constants there are only 10 that actually work and 12 don't work. Let me explain. If you create a simple http server with the default Go TLS configuration, and you list the supported ciphers with a tool like openssl or nmap, only 10 show up. You just need to run this simple server:
And use nmap to list the available ciphers:
The list of ciphers that down show up and never work are:
So, if you try a request using openssl lile: You get the TLS negotiation and the simple http server prints
This means i can't implement those tests testing all the ciphers, because those 12, just don't work. And of course, they were not working before this PR was merged either. To me this is a bug in Go, unless i'm missing something. |
Some ciphers are only available when using serving certificates with certain characteristics. |
I know this is new function, but as this is security related it would be good if we could cherry pick to 1.8 & 1.9 so that we can let customers using general security scanner get rid of the security violations. |
Automatic merge from submit-queue (batch tested with PRs 58920, 58327, 60577, 49388, 62306). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Possible cipher suites values and tls versions in help for apiserver and kubelet **What this PR does / why we need it**: Addresses a suggestion made by @lavalamp to list the possible TLS cipher suites in the kubelet and apiserver helps: #48859 (comment) **Which issue(s) this PR fixes** NONE **Special notes for your reviewer**: This pull request only adds to the help message the possible values of the TLS Cipher suites for Kubelet and api server. It is an addition to the already merged PR #48859 The help output looks like this: ``` --tls-cert-file string File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). If HTTPS serving is enabled, and --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key are generated for the public address and saved to the directory specified by --cert-dir. --tls-cipher-suites strings Comma-separated list of cipher suites for the server. Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). If omitted, the default Go cipher suites will be use. Possible values: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA --tls-min-version string Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. --tls-private-key-file string File containing the default x509 private key matching --tls-cert-file. ``` **Release note**: ```release-note NONE ```
Automatic merge from submit-queue (batch tested with PRs 58920, 58327, 60577, 49388, 62306). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Possible cipher suites values and tls versions in help for apiserver and kubelet **What this PR does / why we need it**: Addresses a suggestion made by @lavalamp to list the possible TLS cipher suites in the kubelet and apiserver helps: kubernetes/kubernetes#48859 (comment) **Which issue(s) this PR fixes** NONE **Special notes for your reviewer**: This pull request only adds to the help message the possible values of the TLS Cipher suites for Kubelet and api server. It is an addition to the already merged PR kubernetes/kubernetes#48859 The help output looks like this: ``` --tls-cert-file string File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). If HTTPS serving is enabled, and --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key are generated for the public address and saved to the directory specified by --cert-dir. --tls-cipher-suites strings Comma-separated list of cipher suites for the server. Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). If omitted, the default Go cipher suites will be use. Possible values: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA --tls-min-version string Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. --tls-private-key-file string File containing the default x509 private key matching --tls-cert-file. ``` **Release note**: ```release-note NONE ``` Kubernetes-commit: 22919ae7e1b5e55dd347d39d14bac629fbfe0e42
Automatic merge from submit-queue (batch tested with PRs 58920, 58327, 60577, 49388, 62306). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Possible cipher suites values and tls versions in help for apiserver and kubelet **What this PR does / why we need it**: Addresses a suggestion made by @lavalamp to list the possible TLS cipher suites in the kubelet and apiserver helps: kubernetes/kubernetes#48859 (comment) **Which issue(s) this PR fixes** NONE **Special notes for your reviewer**: This pull request only adds to the help message the possible values of the TLS Cipher suites for Kubelet and api server. It is an addition to the already merged PR kubernetes/kubernetes#48859 The help output looks like this: ``` --tls-cert-file string File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). If HTTPS serving is enabled, and --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key are generated for the public address and saved to the directory specified by --cert-dir. --tls-cipher-suites strings Comma-separated list of cipher suites for the server. Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). If omitted, the default Go cipher suites will be use. Possible values: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA --tls-min-version string Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. --tls-private-key-file string File containing the default x509 private key matching --tls-cert-file. ``` **Release note**: ```release-note NONE ``` Kubernetes-commit: 22919ae7e1b5e55dd347d39d14bac629fbfe0e42
Automatic merge from submit-queue (batch tested with PRs 58920, 58327, 60577, 49388, 62306). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Possible cipher suites values and tls versions in help for apiserver and kubelet **What this PR does / why we need it**: Addresses a suggestion made by @lavalamp to list the possible TLS cipher suites in the kubelet and apiserver helps: kubernetes/kubernetes#48859 (comment) **Which issue(s) this PR fixes** NONE **Special notes for your reviewer**: This pull request only adds to the help message the possible values of the TLS Cipher suites for Kubelet and api server. It is an addition to the already merged PR kubernetes/kubernetes#48859 The help output looks like this: ``` --tls-cert-file string File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). If HTTPS serving is enabled, and --tls-cert-file and --tls-private-key-file are not provided, a self-signed certificate and key are generated for the public address and saved to the directory specified by --cert-dir. --tls-cipher-suites strings Comma-separated list of cipher suites for the server. Values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants). If omitted, the default Go cipher suites will be use. Possible values: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,TLS_RSA_WITH_RC4_128_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_3DES_EDE_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_RC4_128_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA --tls-min-version string Minimum TLS version supported. Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. --tls-private-key-file string File containing the default x509 private key matching --tls-cert-file. ``` **Release note**: ```release-note NONE ``` Kubernetes-commit: 22919ae7e1b5e55dd347d39d14bac629fbfe0e42
Could you advice what k8s version should we use, which includes these TLS cipher fixes? |
What this PR does / why we need it:
This pull request aims to solve the problem of users not able to set custom cipher suites in the api server.
Several users have requested this given that some default ciphers are vulnerable.
There is a discussion in #41038 of how to implement this. The options are:
I implemented the second option, if the ciphers are not passed by parameter, the Go default ones will be used (same behavior as now).
Which issue this PR fixes
fixes #41038
Special notes for your reviewer:
The ciphers in Go tls config are constants and the ones passed by parameters are a comma-separated list. I needed to create the
type CipherSuitesFlag
to support that conversion/mapping, because i couldn't find any way to do this type of reflection in Go.If you think there is another way to implement this, let me know.
If you want to test it out, this is a ciphers combination i tested without the weak ones:
If this is merged i will implement the same for the Kubelet.
Release note: