-
Notifications
You must be signed in to change notification settings - Fork 617
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
Fabio can connect to Consul using TLS for secure communication. #391
Conversation
This change updates Fabio's configuration parameters to allow use and specification of TLS certificates for HTTPS. In Consul setups where full TLS is configured using CA private this allows Fabio to work in a secure manner. Closes fabiolb#276
The test failures seem to be sporadic and I do not believe they are related to my changes. |
@magiconair would it be possible to get some feedback on this PR? |
Yes. I’ll have a look later today. Sorry for the delay |
Why do you need a key? This is for making TLS connections to consul, correct? Can’t you just add the ca file to the trusted root CAs in |
And use an https URL for consul obviously |
@magiconair yes that is an option and is what I currently do in my setups. I was merely adding this feature as detailed in the ticket #276 |
Ah, the cert and key are used for client cert authentication. The example uses the Consul server cert and that doesn't seem correct. I can see several use cases:
|
This change updates Fabio's configuration parameters to allow use and specification of TLS certificates for HTTPS. In Consul setups where full TLS is configured using CA private this allows Fabio to work in a secure manner. Closes fabiolb#276
Rebase consul tls
Fixes fabiolb#276, fabiolb#501 (fixes fabiolb#391)
@@ -168,6 +168,11 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c | |||
f.StringVar(&cfg.Registry.Consul.KVPath, "registry.consul.kvpath", defaultConfig.Registry.Consul.KVPath, "consul KV path for manual overrides") | |||
f.StringVar(&cfg.Registry.Consul.NoRouteHTMLPath, "registry.consul.noroutehtmlpath", defaultConfig.Registry.Consul.NoRouteHTMLPath, "consul KV path for HTML returned when no route is found") | |||
f.StringVar(&cfg.Registry.Consul.TagPrefix, "registry.consul.tagprefix", defaultConfig.Registry.Consul.TagPrefix, "prefix for consul tags") | |||
f.BoolVar(&cfg.Registry.Consul.EnableSSL, "registry.consul.enableSSL", defaultConfig.Registry.Consul.EnableSSL, "enable HTTPS communication with Consul") |
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.
uppercase words are not parsed by flag library. If you change it to lower case flag will parse it correctly. I mean - registry.consul.enableSSL
change to registry.consul.enablessl
same for other added parametes
- change to lowercase if property file
if cfg.EnableSSL { | ||
cfg.Scheme = "https" | ||
|
||
tls := &api.TLSConfig{ |
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.
tls variable is shadowed here. Here should be
tls = api.TLSConfig{
Hi, I also faced a need for this functionality today. BTW. @jrasell, if you abandoned this PR - I can pick it to implement changes that @magiconair requested |
Hello, i need this functionality too. Currently blocked because of lack of https to search consul. :( How can we help ? |
How about merge to next release? It's blocker feature for our deployment. |
This can be closed since #602 was merged, right? |
Yes, thanks for the reminder. |
This change updates Fabio's configuration parameters to allow use and specification of TLS certificates for HTTPS. In Consul setups where full TLS is configured using CA private this allows Fabio to work in a secure manner.
I have tested this change locally with a Consul TLS setup; log fragments are shown below to provide further detail:
Feedback would be appreciated, and any changes requested will be acted upon promptly.
Closes #276