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

Add SNI support #107

Merged
merged 1 commit into from Dec 23, 2019
Merged

Add SNI support #107

merged 1 commit into from Dec 23, 2019

Conversation

@adamflott
Copy link
Contributor

adamflott commented Dec 19, 2019

Our setup uses SNI for routing, and this is a quick hack to include it as a command line option.

Feedback welcomed.

Copy link
Member

itamarhaber left a comment

LGTM. @yossigo @YaacovHazan wdut?

@@ -839,6 +847,7 @@ void usage() {
" --key=FILE Use specified private key for TLS\n"
" --cacert=FILE Use specified CA certs bundle for TLS\n"
" --tls-skip-verify Skip verification of server certificate\n"
" --sni=STRING Add an SNI header\n"

This comment has been minimized.

Copy link
@ushachar

ushachar Dec 19, 2019

Contributor

@itamarhaber do you see any reason to toggle this on/off? Why not always add it when doing TLS?

This comment has been minimized.

Copy link
@yossigo

yossigo Dec 22, 2019

Contributor

@ushachar I think that'll work for the mainstream cases, but there's probably a long tail of cases where you'd want a custom name, specify an IP as a hostname + SNI (e.g. no DNS resolution), etc.

We could consider to use the hostname by default (if it's a name and not an IP) if we still have the option to control that manually.

@adamflott

This comment has been minimized.

Copy link
Contributor Author

adamflott commented Dec 23, 2019

I chatted with one of the TLS RFC contributors and he said you should always include the SNI, but for my use case I need to override the connection hostname with a different value.

Thanks for reviewing! This is utility is going to see a lot of use at Akamai :)

@ushachar ushachar merged commit 6393b92 into RedisLabs:master Dec 23, 2019
3 checks passed
3 checks passed
build-ubuntu (ubuntu-latest)
Details
build-ubuntu (ubuntu-16.04)
Details
build-macos (macos-latest)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.