-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
TLS min/max version updates #518
Conversation
not my cleanest code ever, but RFR @mreiferson @twmb |
did you see my comments on #513? I'm fine if you want to add |
} | ||
if v, exists := cfg["tls_min_version"]; exists { | ||
var t tlsVersionOption | ||
err := t.Set(fmt.Sprintf("%v", v)) |
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, yes, we do need this - whooops 😁
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.
it might make sense to "promote" this translation behavior up into go-options
itself in some generic way.
if the destination type implements a "set" interface then use that for coercion?
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.
Yeah that's my general thought. I need to review better how we landed w/ that and validation for reader-options in go-nsq
. I'll ping when i have taken a pass at updating.
Ok, did some refactoring to We should improve config validation in a future PR. |
type config map[string]interface{} | ||
|
||
// Validate settings in the config file, and fatal on errors | ||
func (cfg config) Validate() { |
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 sure Validate
is the right word here, but I don't have any better ideas
this came out pretty cool - LGTM after that minor nitpick |
maxDeflateLevel = flagSet.Int("max-deflate-level", 6, "max deflate compression level a client can negotiate (> values == > nsqd CPU usage)") | ||
snappyEnabled = flagSet.Bool("snappy", true, "enable snappy feature negotiation (client compression)") | ||
) | ||
var () |
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.
Why keep this line?
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.
ha. oops.
good catch
This should be followed with a corresponding change to go-nsq that adds dots to the min version flag name and also adds max version. 👍 |
cd97e1b
to
a817603
Compare
Thanks for the feedback @mreiferson @twmb RFM after travis goes green. |
TLS min/max version updates
Switch to allow
tls_min_version="tls1.0"
from a config file instead of opaque integer (and same for manually setting Config objects).Default min version tls1.0 to avoid allowing ssl3.0 by default, and specify max TLS version of 1.2 to take advantage of TLS_FALLBACK_SCSV prior to Go 1.5
for more discussion see: #513