-
Notifications
You must be signed in to change notification settings - Fork 681
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
Parse equal-preference cipher groups #6461
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6461 +/- ##
=======================================
Coverage 81.63% 81.63%
=======================================
Files 133 133
Lines 15869 15869
=======================================
Hits 12955 12955
Misses 2620 2620
Partials 294 294 |
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, thanks @tsaarni
func isValidTLSCipher(cipherSpec string) bool { | ||
// Equal-preference group: [cipher1|cipher2|...] | ||
if strings.HasPrefix(cipherSpec, "[") && strings.HasSuffix(cipherSpec, "]") { | ||
for _, cipher := range strings.Split(cipherSpec[1:len(cipherSpec)-1], "|") { |
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.
nit: could use strings.Trim
instead of the slice indexing here just to be safe: https://pkg.go.dev/strings#Trim
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 add a couple negative tests for the new parsing logic otherwise just one nit and lgtm, nice fix!
Signed-off-by: Tero Saarni <[email protected]>
Fixed! Suddently codespell started to complain for unchanged files, although I thought codespell version in actions is fixed 😲 Anyways, I fixed the complaints. |
Signed-off-by: Tero Saarni <[email protected]> Signed-off-by: Saman Mahdanian <[email protected]>
This change corrects the parsing of cipher configuration by adding support for equal-preference cipher groups
[cipher1|cipher2|...]
:ValidTLSCiphers
list now contains only individual cipher names, not groups.ECDHE-ECDSA-CHACHA20-POLY1305
andECDHE-RSA-CHACHA20-POLY1305
individually, whereas previously they were only accepted as part of the hardcoded groups.Fixes #6380
Background:
Prior this change the list of valid TLS ciphers had hardcoded ciphers in bracketed format
contour/apis/projectcontour/v1alpha1/ciphersuites.go
Lines 44 to 45 in 0796cd9
This is simply a copy from Envoy documentation (link) but on further inspection Boringssl doc (link) says:
We should not use the hardcoded groups when validating user's cipher configuration, instead we should parse the groups into individual cipher names and validate them.