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.

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

nsqd: add --tls-min-version config option #513

Merged
merged 3 commits into from
Dec 10, 2014
Merged

nsqd: add --tls-min-version config option #513

merged 3 commits into from
Dec 10, 2014

Conversation

twmb
Copy link
Contributor

@twmb twmb commented Dec 9, 2014

This adds a new tls-min-version flag that enables nsqd users
to specify a min tls version acceptable. The arguments to this
flag, , follow the format of Version in the tls package.

This commit adds --tls-min-version=ssl30 to the test.sh flags.

This PR also go fmt's nsq_to_nsq, which has been unformatted for a while, and reduces code clutter from tlsRequiredOption.String.

closes #512.

This changes nothing output wise, but removes unnecessary code
and makes this method more generic for the future.
@twmb
Copy link
Contributor Author

twmb commented Dec 9, 2014

If tls-min-version is acceptable here, I will work on the same patch for go-nsq.

@twmb
Copy link
Contributor Author

twmb commented Dec 9, 2014

RFR @mreiferson

@mreiferson
Copy link
Member

@twmb thanks!

Got an error in the travis run:

2014/12/09 23:00:31 ERROR: option resolution failed to coerce 768 for TLSMinVersion (<uint16 Value>) - invalid type

@mreiferson mreiferson changed the title Tls min version 512 nsqd: add --tls-min-version config option Dec 10, 2014
@twmb
Copy link
Contributor Author

twmb commented Dec 10, 2014

I was looking at it. I can't repro in dev and the message it gives (can't coerce 768 into uint16) doesn't make sense on surface inspection.

This adds a new tls-min-version flag that enables nsqd users
to specify a min tls version acceptable. The arguments to this
flag, <v>, follow the format of Version<v> in the tls package.

This commit adds --tls-min-version=ssl30 to the test.sh flags.
@mreiferson
Copy link
Member

LGTM, thanks!

mreiferson added a commit that referenced this pull request Dec 10, 2014
nsqd: add --tls-min-version config option
@mreiferson mreiferson merged commit c67815c into nsqio:master Dec 10, 2014
@twmb twmb deleted the tls_min_version_512 branch December 10, 2014 21:25
@jehiah
Copy link
Member

jehiah commented Dec 20, 2014

It turns out that (until Go 1.5) if we don't specify max version we don't get TLS_FALLBACK_SCSV via https://twitter.com/benburkert/status/546063006975533057

@mreiferson do you think we should add a -tls-max-version or just set that value by default (or both)?

@mreiferson
Copy link
Member

@jehiah let's just set the default for now? want to open the PR?

@mreiferson
Copy link
Member

@jehiah relatedly, we probably shouldn't default --tls-min-version to ssl30, right?

@jehiah
Copy link
Member

jehiah commented Dec 20, 2014

I Also realized as I started looking at this that specifying the flag type as an int type doesn't translate for the config file where you'd have to set 771 to get tls1.2. PR coming shortly to improve that too

@mreiferson
Copy link
Member

Nah, you can set it to a string - it looks a bit convoluted but it works...

@mreiferson
Copy link
Member

Sorry, I read that wrong, the config file can be set to a string but you're right, if you were setting Config directly (using nsqd as a package?) you would need to set an int...

It would be nice to be able to set a string there, too...

@mreiferson
Copy link
Member

... take 3 - can't you just use the constants if you were setting Config directly? I actually don't think it makes sense to set strings there - unless I'm not understanding something?

@twmb
Copy link
Contributor Author

twmb commented Dec 22, 2014

Yeah, it'd make sense to take the tls constants. Sorry the code is a bit convoluted, I thought it'd make more sense to take "ssl30" from the command line as opposed to 0, but also I thought it'd make more sense to use tls.VersionSSL30 if using nsqd as a package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nsq: enable ability to specify min acceptable tls version
3 participants