Skip to content
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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 26, 2022
Merged

add secure ciphersuites for TLS config #1244

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
383e57c
add secure ciphersuites for TLS config
kangsheng89 Nov 11, 2022
0b2076c
Merge branch 'main' into safeciphersuites
kangsheng89 Nov 14, 2022
20268ba
add comment for the secureCipherSuite function
kangsheng89 Nov 14, 2022
05d3020
Merge branch 'main' into safeciphersuites
kangsheng89 Nov 16, 2022
7d84568
provide more descriptive comments on secureCipherSuite function
kangsheng89 Nov 16, 2022
b74d2de
ciphersuites and tls version setting can be configurable
kangsheng89 Nov 17, 2022
37fd215
add description for the function tlsConfigSetting
kangsheng89 Nov 21, 2022
914c205
Merge branch 'main' into safeciphersuites
kangsheng89 Nov 21, 2022
d424a23
Merge branch 'main' into safeciphersuites
kangsheng89 Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ func main() {

optionsTlSOptsFuncs := []func(*tls.Config){
func(config *tls.Config) { minTlsDefault(config) },
func(config *tls.Config) { secureCipherSuite(config) },
}

mgrOptions := ctrl.Options{
Expand Down Expand Up @@ -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) {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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

var cipherSuites = tls.CipherSuites()
cipherIdList := []uint16{}
for _, item := range cipherSuites {
cipherIdList = append(cipherIdList, item.ID)
}
cfg.CipherSuites = cipherIdList
}