-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Added tls_cipher_suites, tls_prefer_server_ciphers config options to listener #2293
Conversation
Hi @roman-vynar, This looks really great. One thing: can you move the parsing logic into Thanks! |
|
||
func parseCiphers(cipherStr string) ([]uint16, error) { | ||
suites := []uint16{} | ||
ciphers := strings.Split(cipherStr, ":") |
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.
Can we use the inbuilt helper strutil.ParseDedupAndSortStrings()
here? Also, it might be better to delimit the entries with a ,
instead of a :
just to be consistent with several API field delimiters.
if v, ok := cipherMap[cipher]; ok { | ||
suites = append(suites, v) | ||
} else { | ||
return suites, fmt.Errorf("unsupported cipher '%s'", cipher) |
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.
We could use %q
instead of '%s'
.
@@ -545,6 +545,8 @@ func parseListeners(result *Config, list *ast.ObjectList) error { | |||
"tls_cert_file", | |||
"tls_key_file", | |||
"tls_min_version", | |||
"tls_cipher_suites", | |||
"tls_prefer_server_ciphers", |
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.
Can we rename this to tls_prefer_server_cipher_suites
?
separated with colon. The list of all available ciphersuites you can find | ||
[here](https://golang.org/src/crypto/tls/cipher_suites.go). | ||
|
||
* `tls_prefer_server_ciphers` (optional) - Controls whether the server selects |
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.
Can we rename this to tls_prefer_server_cipher_suites
?
Applied all the suggestions. |
suites := []uint16{} | ||
ciphers := strutil.ParseDedupAndSortStrings(cipherStr, ",") | ||
cipherMap := map[string]uint16{ | ||
"TLS_RSA_WITH_RC4_128_SHA": tls.TLS_RSA_WITH_RC4_128_SHA, // Grade C |
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'm going to merge this but remove the Grade C comments. It's not meaningful to anyone that doesn't know where those grades are coming from, and it seems odd without also adding in the grades of other ciphers, plus those comments can/will easily get out of date as people forget to update the grades of other ciphers over time.
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.
One thought here is to just remove 3DES and RC4 ciphers from the list all together. Given the security purpose of this program and that only tls 1.2 is allowed with the default vault binaries, anything that can connect would not need these ciphers.
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.
Not quite true. TLS is the default but the binaries have a config option to allow stepping down...not a compile time option.
Added
tls_cipher_suites
,tls_prefer_server_cipher_suites
config options to define preferred ciphersuites to enforce a high grade of security. Addresses #1193For example:
Golang supports 17 ciphersuites, 5 of them are grade C.
The example above excludes those and nmap is happy with that:
Please let me know if you want me to change/improve anything to support this.
Thanks.