-
Notifications
You must be signed in to change notification settings - Fork 459
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
add secure ciphersuites for TLS config #1244
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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