Allow UI/API to be served over https #258
Conversation
config/default.go
Outdated
| @@ -51,6 +51,7 @@ var defaultConfig = &Config{ | |||
| ServiceStatus: []string{"passing"}, | |||
| CheckInterval: time.Second, | |||
| CheckTimeout: 3 * time.Second, | |||
| CheckScheme: "http://", | |||
There was a problem hiding this comment.
this should be either http or https without ://
config/default.go
Outdated
| Color: "light-green", | ||
| Addr: ":9998", | ||
| Color: "light-green", | ||
| CertFile: "", |
There was a problem hiding this comment.
These are redundant. Pls remove.
config/load.go
Outdated
| @@ -166,6 +166,8 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c | |||
| f.StringVar(&cfg.UI.Addr, "ui.addr", defaultConfig.UI.Addr, "address the UI/API is listening on") | |||
| f.StringVar(&cfg.UI.Color, "ui.color", defaultConfig.UI.Color, "background color of the UI") | |||
| f.StringVar(&cfg.UI.Title, "ui.title", defaultConfig.UI.Title, "optional title for the UI") | |||
| f.StringVar(&cfg.UI.CertFile, "ui.certfile", defaultConfig.UI.CertFile, "optional cert for serving UI/API https, if set ui.keyfile is required") | |||
There was a problem hiding this comment.
You can store a cert and key in a single file. Make keyfile optional.
config/load.go
Outdated
| @@ -186,6 +188,15 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c | |||
|
|
|||
| cfg.Registry.Consul.Scheme, cfg.Registry.Consul.Addr = parseScheme(cfg.Registry.Consul.Addr) | |||
|
|
|||
| if cfg.UI.CertFile != "" { | |||
There was a problem hiding this comment.
same here. keyfile optional. CheckScheme should be https
config/load_test.go
Outdated
| @@ -587,6 +587,15 @@ func TestLoad(t *testing.T) { | |||
| }, | |||
| }, | |||
| { | |||
| args: []string{"-ui.certfile", "cert_value", "-ui.keyfile", "key_value"}, | |||
|
I think I addressed all the requested changes. As for the config names, I just used similar options to what consul itself has, but I can easily change them if you prefer something else. And as for basic auth, I can look into that, but can it be in a separate PR? |
There was a problem hiding this comment.
I'm wondering whether it would make more sense to just configure another cert source since there already is all the cert management there. Although this seems like a lean approach to the problem it feels redundant. What if you just add a cs= option to the ui listener?
config/load_test.go
Outdated
| args: []string{"-ui.keyfile", "key_value"}, | ||
| cfg: func(cfg *Config) *Config { | ||
| cfg.UI.KeyFile = "key_value" | ||
| cfg.Registry.Consul.CheckScheme = "http" |
There was a problem hiding this comment.
The way this is currently coded it is, in that if you just specify a keyFile it will ignore it and still server http, as you need to specify a certFile to get https. I don't think it would work with just a private key.
There was a problem hiding this comment.
No it won't. Question is whether this should then be an error and fabio refuse to start since this config doesn't make sense.
There was a problem hiding this comment.
An error sounds better, rather than silently defaulting to http. I can make that change
admin/server.go
Outdated
| @@ -28,6 +28,9 @@ func (s *Server) ListenAndServe(addr string) error { | |||
| http.Handle("/routes", &ui.RoutesHandler{Color: s.Color, Title: s.Title, Version: s.Version}) | |||
| http.HandleFunc("/health", handleHealth) | |||
| http.Handle("/", http.RedirectHandler("/routes", http.StatusSeeOther)) | |||
| if s.Cfg.UI.CertFile != "" { | |||
There was a problem hiding this comment.
I don't think this is how this works. Probably more like this:
certFile, keyFile := s.Cfg.UI.CertFile, s.Cfg.UI.KeyFile
if certFile == "" {
return http.ListenAndServe(addr, nil)
}
if keyFile == "" {
keyFile = certFile
}
return http.ListenAndServe(addr, certFile, keyFile)There was a problem hiding this comment.
I can try that out, but I think how I had it at first was correct, in that you should be specifying a certFile which contains the server's certificate and any intermediates. Then the private key should be specified as the keyFile.
That seems in line with http.ListenAndServeTLS's docs.
There was a problem hiding this comment.
OK, I've just written that from memory from the time I've coded the cert stores. I'll have another look.
There was a problem hiding this comment.
Did some experimenting and it looks like your code should work
| @@ -51,6 +51,7 @@ var defaultConfig = &Config{ | |||
| ServiceStatus: []string{"passing"}, | |||
| CheckInterval: time.Second, | |||
| CheckTimeout: 3 * time.Second, | |||
| CheckScheme: "http", | |||
There was a problem hiding this comment.
I'm thinking that you could default to "" == "http". Would save you an else. What do you think?
There was a problem hiding this comment.
I'm not sure what you mean by this and the change in registry/consul/register.go
fabio.properties
Outdated
| # ui.certfile configures the UI/API with a TLS server certificate. | ||
| # If this is specified, the UI/API will be served over HTTPS. | ||
| # | ||
| # The default is |
There was a problem hiding this comment.
I see two spaces default__is. Also, default should be ui.certfile = . Same below.
# ui.certfile contains the path to the TLS certificate for the UI/API endpoint
# which enables serving the UI and API over HTTPS.
#
# If the file containing the certificate also contains the private key then the
# ui.keyfile option can be omitted.
#
# The default is
#
# ui.certfile =
fabio.properties
Outdated
| # ui.certfile | ||
|
|
||
|
|
||
| # ui.keyfile configures the UI/API with a TLS private key. |
There was a problem hiding this comment.
# ui.keyfile contains the path to the private key of the TLS certificate for the
# UI/API endpoint.
#
# If the file containing the certificate also contains the private key then the
# ui.keyfile option can be omitted.
#
# The default is
#
# ui.keyfile =
registry/consul/register.go
Outdated
| @@ -103,9 +103,9 @@ func serviceRegistration(addr, name string, tags []string, interval, timeout tim | |||
|
|
|||
| serviceID := fmt.Sprintf("%s-%s-%d", name, hostname, port) | |||
|
|
|||
| checkURL := fmt.Sprintf("http://%s:%d/health", ip, port) | |||
| checkURL := fmt.Sprintf("%s://%s:%d/health", checkScheme, ip, port) | |||
There was a problem hiding this comment.
Fix here if you choose checkScheme == "" -> "http"
|
Did you see the comment about using a cert store instead of certFile/keyFile?
|
|
@shadowfax-chc BTW, going to bed now. I'm on the CEST timezone. You're in NZ? |
|
I would have to look into how the cert stores are being used. From what it looks like to me the UI is starting up differently than the proxy listeners, but it could probably be refactored to use a I'm not sure what the config would then be like, are you thinking something like: I think I kind of see how using the cert store could be nice as it would allow for getting the UI's certs from any of the supported cert store backends. But I also like the idea of having the cert stores only be for the proxy listeners, and having the UI just use a simple file based configuration, since the UI/API is a special case. And no, I'm on EDT time. |
|
Addressed some more of the comments. However, if you would rather the certs come from a cert store, I can refactor this. Just need some clarification on what the configuration would look like. |
|
It just occurred to me again why the cert stores are there in the first place: so that you do not have to distribute the certificates to every fabio node with something like puppet or chef. Yes, there are the Therefore, this needs to use the cert stores. Sorry, that I didn't realize this sooner. What you need to change is the following: If you replace the At that point you can use
You also wouldn't need a At some point I want to refactor the code a bit to have generic providers like a |
|
Makes sense, I can get this refactored to use the cert store. |
|
Thank you. |
|
Refactored to use the CertStore. It might still need some cleanup to reduce duplication, but let me know if this is the right idea. |
|
I can't figure out how I could push a set of simple changes to your branch since I prefer a single commit for a change. I'm used to the gerrit workflow where multiple people can collaborate on a single change until it is right without creating different branches but can't figure out a good way of doing this on github. Maybe you have an idea. I've done some simple cleanup. Basically, diff --git a/config/default.go b/config/default.go
index 1abf758..3e187d5 100644
--- a/config/default.go
+++ b/config/default.go
@@ -10,12 +10,12 @@ var defaultValues = struct {
CertSourcesValue []map[string]string
ReadTimeout time.Duration
WriteTimeout time.Duration
- UIAddr string
+ UIListenerValue string
GZIPContentTypesValue string
}{
ListenerValue: []string{":9999"},
CertSourcesValue: []map[string]string{},
- UIAddr: ":9998",
+ UIListenerValue: ":9998",
}
var defaultConfig = &Config{
diff --git a/config/load.go b/config/load.go
index d569360..d91b8fd 100644
--- a/config/load.go
+++ b/config/load.go
@@ -109,9 +109,9 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
// config values
var listenerValue []string
+ var uiListenerValue string
var certSourcesValue []map[string]string
var readTimeout, writeTimeout time.Duration
- var uiAddrValue string
var gzipContentTypesValue string
f.IntVar(&cfg.Proxy.MaxConn, "proxy.maxconn", defaultConfig.Proxy.MaxConn, "maximum number of cached connections")
@@ -164,7 +164,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
f.DurationVar(&cfg.Registry.Consul.CheckTimeout, "registry.consul.register.checkTimeout", defaultConfig.Registry.Consul.CheckTimeout, "service check timeout")
f.IntVar(&cfg.Runtime.GOGC, "runtime.gogc", defaultConfig.Runtime.GOGC, "sets runtime.GOGC")
f.IntVar(&cfg.Runtime.GOMAXPROCS, "runtime.gomaxprocs", defaultConfig.Runtime.GOMAXPROCS, "sets runtime.GOMAXPROCS")
- f.StringVar(&uiAddrValue, "ui.addr", defaultValues.UIAddr, "Address the UI/API is listening on")
+ f.StringVar(&uiListenerValue, "ui.addr", defaultValues.UIListenerValue, "Address the UI/API is listening on")
f.StringVar(&cfg.UI.Color, "ui.color", defaultConfig.UI.Color, "background color of the UI")
f.StringVar(&cfg.UI.Title, "ui.title", defaultConfig.UI.Title, "optional title for the UI")
@@ -192,8 +192,8 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
return nil, err
}
- if uiAddrValue != "" {
- cfg.UI.Listen, err = parseListen(uiAddrValue, certSources, 0, 0)
+ if uiListenerValue != "" {
+ cfg.UI.Listen, err = parseListen(uiListenerValue, certSources, 0, 0)
if err != nil {
return nil, err
}
@@ -204,10 +204,9 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
return nil, err
}
+ cfg.Registry.Consul.CheckScheme = defaultConfig.Registry.Consul.CheckScheme
if cfg.UI.Listen.CertSource.Name != "" {
cfg.Registry.Consul.CheckScheme = "https"
- } else {
- cfg.Registry.Consul.CheckScheme = defaultConfig.Registry.Consul.CheckScheme
}
if gzipContentTypesValue != "" {
diff --git a/main.go b/main.go
index b893ed6..472fe7e 100644
--- a/main.go
+++ b/main.go
@@ -161,17 +161,17 @@ func lookupHostFn(cfg *config.Config) func(string) string {
}
}
-func createTLSConfig(l config.Listen) *tls.Config {
- var tlscfg *tls.Config
- if l.CertSource.Name != "" {
- src, err := cert.NewSource(l.CertSource)
- if err != nil {
- exit.Fatalf("[FATAL] Failed to create cert source %s. %s", l.CertSource.Name, err)
- }
- tlscfg, err = cert.TLSConfig(src, l.StrictMatch)
- if err != nil {
- exit.Fatalf("[FATAL] Failed to create TLS config for cert source %s. %s", l.CertSource.Name, err)
- }
+func makeTLSConfig(l config.Listen) *tls.Config {
+ if l.CertSource.Name == "" {
+ return nil
+ }
+ src, err := cert.NewSource(l.CertSource)
+ if err != nil {
+ exit.Fatalf("[FATAL] Failed to create cert source %s. %s", l.CertSource.Name, err)
+ }
+ tlscfg, err := cert.TLSConfig(src, l.StrictMatch)
+ if err != nil {
+ exit.Fatalf("[FATAL] Failed to create TLS config for cert source %s. %s", l.CertSource.Name, err)
}
return tlscfg
}
@@ -180,7 +180,7 @@ func startAdmin(cfg *config.Config) {
log.Printf("[INFO] Admin server listening on %q", cfg.UI.Listen.Addr)
go func() {
l := cfg.UI.Listen
- tlscfg := createTLSConfig(l)
+ tlscfg := makeTLSConfig(l)
srv := &admin.Server{
Color: cfg.UI.Color,
Title: cfg.UI.Title,
@@ -196,7 +196,7 @@ func startAdmin(cfg *config.Config) {
func startServers(cfg *config.Config) {
for _, l := range cfg.Listen {
- tlscfg := createTLSConfig(l)
+ tlscfg := makeTLSConfig(l)
log.Printf("[INFO] %s proxy listening on %s", strings.ToUpper(l.Proto), l.Addr)
if tlscfg != nil && tlscfg.ClientAuth == tls.RequireAndVerifyClientCert {
diff --git a/registry/consul/backend.go b/registry/consul/backend.go
index e06f186..3ad0ece 100644
--- a/registry/consul/backend.go
+++ b/registry/consul/backend.go
@@ -42,7 +42,7 @@ func (b *be) Register() error {
return nil
}
- service, err := serviceRegistration(b.cfg.ServiceAddr, b.cfg.ServiceName, b.cfg.ServiceTags, b.cfg.CheckInterval, b.cfg.CheckTimeout, b.cfg.CheckScheme)
+ service, err := serviceRegistration(b.cfg)
if err != nil {
return err
}
diff --git a/registry/consul/register.go b/registry/consul/register.go
index 760e02b..021dbee 100644
--- a/registry/consul/register.go
+++ b/registry/consul/register.go
@@ -76,12 +76,12 @@ func register(c *api.Client, service *api.AgentServiceRegistration) (dereg chan
return dereg
}
-func serviceRegistration(addr, name string, tags []string, interval, timeout time.Duration, checkScheme string) (*api.AgentServiceRegistration, error) {
+func serviceRegistration(cfg *config.Consul) (*api.AgentServiceRegistration, error) {
hostname, err := os.Hostname()
if err != nil {
return nil, err
}
- ipstr, portstr, err := net.SplitHostPort(addr)
+ ipstr, portstr, err := net.SplitHostPort(cfg.ServiceAddr)
if err != nil {
return nil, err
}
@@ -101,23 +101,23 @@ func serviceRegistration(addr, name string, tags []string, interval, timeout tim
}
}
- serviceID := fmt.Sprintf("%s-%s-%d", name, hostname, port)
+ serviceID := fmt.Sprintf("%s-%s-%d", cfg.ServiceName, hostname, port)
- checkURL := fmt.Sprintf("%s://%s:%d/health", checkScheme, ip, port)
+ checkURL := fmt.Sprintf("%s://%s:%d/health", cfg.CheckScheme, ip, port)
if ip.To16() != nil {
- checkURL = fmt.Sprintf("%s://[%s]:%d/health", checkScheme, ip, port)
+ checkURL = fmt.Sprintf("%s://[%s]:%d/health", cfg.CheckScheme, ip, port)
}
service := &api.AgentServiceRegistration{
ID: serviceID,
- Name: name,
+ Name: cfg.ServiceName,
Address: ip.String(),
Port: port,
- Tags: tags,
+ Tags: cfg.ServiceTags,
Check: &api.AgentServiceCheck{
HTTP: checkURL,
- Interval: interval.String(),
- Timeout: timeout.String(),
+ Interval: cfg.CheckInterval.String(),
+ Timeout: cfg.CheckTimeout.String(),
},
} |
This allows for using the cert store to provide a cert for the ui and to
server it over https. For example:
./fabio \
-proxy.cs 'cs=ui;type=file;cert=./cert.pem;key=./key.pem' \
-ui.addr ':9998;cs=ui'
|
Did the requested refactoring, and squashed it down to one commit. |
|
LGTM |
Provides options to specify a cert and key so that the UI and API can be HTTPS