add secure ciphersuites for TLS config #1244
Conversation
|
|
|
|
main.go
Outdated
| @@ -292,3 +293,12 @@ func addDependencies(_ context.Context, mgr ctrl.Manager, cfg config.Config, v v | |||
| func minTlsDefault(cfg *tls.Config) { | |||
| cfg.MinVersion = defaultMinTLSVersion | |||
| } | |||
|
|
|||
| func secureCipherSuite(cfg *tls.Config) { | |||
There was a problem hiding this comment.
Could you please add a comment on this function and describe what it does?
I personally don't know why all ciphers have to be specified.
There was a problem hiding this comment.
By description from the comments, it should be taken in safe cipher. But after i run the test, seems like manager just directly using the cipher configuration by tls version only. Instead of using the ciphers from tls.CipherSuites() which is safe from vulnerability.
There was a problem hiding this comment.
@pavolloffay do u have any more concerned, or may b do u have any more ideas to improve on this?
There was a problem hiding this comment.
// This function return list of CipherSuite's ID without security issues.
I was more interested in the comment why this is needed
There was a problem hiding this comment.
Ok let me update the comments on the code and it will be more clear for reader/maintainer
|
@pavolloffay can u help to check why docker buildx fail? |
|
@kangsheng89 Should we consider the same here? jaegertracing/jaeger-operator#2119 (comment) |
|
@pavolloffay After some consideration, i make changes so that TLS setting for version and ciphersuite is configurable from cmd argument. I am using https://github.com/kubernetes/component-base/blob/master/cli/flag/ciphersuites_flag.go help to validate the TLS settings from cmd argument |
|
@pavolloffay any concerns or feedbacks to improve this? |
* add secure ciphersuites for TLS config * add comment for the secureCipherSuite function * provide more descriptive comments on secureCipherSuite function * ciphersuites and tls version setting can be configurable * add description for the function tlsConfigSetting
to fix #1238