letsencrypt: Activate during config load just after tls directive

Before, we were activating Let's Encrypt after all the directives were executed. This means their setup functions had access to potentially erroneous information about the server's TLS setup, since the letsencrypt package makes changes to the port, etc. Now, we execute all directives up to and including tls, then activate letsencrypt, then finish with the rest of the directives. It's a bit ugly, but I do think it is more correct. It also fixes some bugs, for example: a host that only has a catch-all redirect.

4 files changed, 89 insertions, 19 deletions

diff --git a/caddy/caddy.go b/caddy/caddy.go
index 830f046d5..5d3785e13 100644
--- a/caddy/caddy.go
+++ b/caddy/caddy.go
@@ -113,14 +113,8 @@ func Start(cdyfile Input) error {
 	caddyfile = cdyfile
 	caddyfileMu.Unlock()
 
-	// load the server configs
-	configs, err := load(path.Base(cdyfile.Path()), bytes.NewReader(cdyfile.Body()))
-	if err != nil {
-		return err
-	}
-
-	// secure all the things
-	configs, err = letsencrypt.Activate(configs)
+	// load the server configs (activates Let's Encrypt)
+	configs, err := loadConfigs(path.Base(cdyfile.Path()), bytes.NewReader(cdyfile.Body()))
 	if err != nil {
 		return err
 	}
diff --git a/caddy/config.go b/caddy/config.go
index 503faecc3..4bf111a1b 100644
--- a/caddy/config.go
+++ b/caddy/config.go
@@ -7,6 +7,7 @@ import (
 	"net"
 	"sync"
 
+	"github.com/mholt/caddy/caddy/letsencrypt"
 	"github.com/mholt/caddy/caddy/parse"
 	"github.com/mholt/caddy/caddy/setup"
 	"github.com/mholt/caddy/middleware"
@@ -19,15 +20,20 @@ const (
 	DefaultConfigFile = "Caddyfile"
 )
 
-// load reads input (named filename) and parses it, returning the
-// server configurations in the order they appeared in the input.
-func load(filename string, input io.Reader) ([]server.Config, error) {
+// loadConfigs reads input (named filename) and parses it, returning the
+// server configurations in the order they appeared in the input. As part
+// of this, it activates Let's Encrypt for the configs that are produced.
+// Thus, the returned configs are already optimally configured optimally
+// for HTTPS.
+func loadConfigs(filename string, input io.Reader) ([]server.Config, error) {
 	var configs []server.Config
 
 	// turn off timestamp for parsing
 	flags := log.Flags()
 	log.SetFlags(0)
 
+	// Each server block represents similar hosts/addresses, since they
+	// were grouped together in the Caddyfile.
 	serverBlocks, err := parse.ServerBlocks(filename, input, true)
 	if err != nil {
 		return nil, err
@@ -36,9 +42,11 @@ func load(filename string, input io.Reader) ([]server.Config, error) {
 		return []server.Config{NewDefault()}, nil
 	}
 
-	// Each server block represents similar hosts/addresses.
+	var lastDirectiveIndex int // we set up directives in two parts; this stores where we left off
+
 	// Iterate each server block and make a config for each one,
-	// executing the directives that were parsed.
+	// executing the directives that were parsed in order up to the tls
+	// directive; this is because we must activate Let's Encrypt.
 	for i, sb := range serverBlocks {
 		onces := makeOnces()
 		storages := makeStorages()
@@ -55,12 +63,12 @@ func load(filename string, input io.Reader) ([]server.Config, error) {
 			}
 
 			// It is crucial that directives are executed in the proper order.
-			for _, dir := range directiveOrder {
+			for k, dir := range directiveOrder {
 				// Execute directive if it is in the server block
 				if tokens, ok := sb.Tokens[dir.name]; ok {
-					// Each setup function gets a controller, which is the
-					// server config and the dispenser containing only
-					// this directive's tokens.
+					// Each setup function gets a controller, from which setup functions
+					// get access to the config, tokens, and other state information useful
+					// to set up its own host only.
 					controller := &setup.Controller{
 						Config:    &config,
 						Dispenser: parse.NewDispenserTokens(filename, tokens),
@@ -76,7 +84,7 @@ func load(filename string, input io.Reader) ([]server.Config, error) {
 						ServerBlockHosts:     sb.HostList(),
 						ServerBlockStorage:   storages[dir.name],
 					}
-
+					// execute setup function and append middleware handler, if any
 					midware, err := dir.setup(controller)
 					if err != nil {
 						return nil, err
@@ -87,12 +95,74 @@ func load(filename string, input io.Reader) ([]server.Config, error) {
 					}
 					storages[dir.name] = controller.ServerBlockStorage // persist for this server block
 				}
+
+				// Stop after TLS setup, since we need to activate Let's Encrypt before continuing;
+				// it makes some changes to the configs that middlewares might want to know about.
+				if dir.name == "tls" {
+					lastDirectiveIndex = k
+					break
+				}
 			}
 
 			configs = append(configs, config)
 		}
 	}
 
+	// Now we have all the configs, but they have only been set up to the
+	// point of tls. We need to activate Let's Encrypt before setting up
+	// the rest of the middlewares so they have correct information regarding
+	// TLS configuration, if necessary. (this call is append-only, so our
+	// iterations below shouldn't be affected)
+	configs, err = letsencrypt.Activate(configs)
+	if err != nil {
+		return nil, err
+	}
+
+	// Finish setting up the rest of the directives, now that TLS is
+	// optimally configured. These loops are similar to above except
+	// we don't iterate all the directives from the beginning and we
+	// don't create new configs.
+	configIndex := -1
+	for i, sb := range serverBlocks {
+		onces := makeOnces()
+		storages := makeStorages()
+
+		for j := range sb.Addresses {
+			configIndex++
+
+			for k := lastDirectiveIndex + 1; k < len(directiveOrder); k++ {
+				dir := directiveOrder[k]
+
+				if tokens, ok := sb.Tokens[dir.name]; ok {
+					controller := &setup.Controller{
+						Config:    &configs[configIndex],
+						Dispenser: parse.NewDispenserTokens(filename, tokens),
+						OncePerServerBlock: func(f func() error) error {
+							var err error
+							onces[dir.name].Do(func() {
+								err = f()
+							})
+							return err
+						},
+						ServerBlockIndex:     i,
+						ServerBlockHostIndex: j,
+						ServerBlockHosts:     sb.HostList(),
+						ServerBlockStorage:   storages[dir.name],
+					}
+					midware, err := dir.setup(controller)
+					if err != nil {
+						return nil, err
+					}
+					if midware != nil {
+						// TODO: For now, we only support the default path scope /
+						configs[configIndex].Middleware["/"] = append(configs[configIndex].Middleware["/"], midware)
+					}
+					storages[dir.name] = controller.ServerBlockStorage // persist for this server block
+				}
+			}
+		}
+	}
+
 	// restore logging settings
 	log.SetFlags(flags)
 
diff --git a/caddy/directives.go b/caddy/directives.go
index 3ebee7955..542985022 100644
--- a/caddy/directives.go
+++ b/caddy/directives.go
@@ -42,7 +42,7 @@ func init() {
 var directiveOrder = []directive{
 	// Essential directives that initialize vital configuration settings
 	{"root", setup.Root},
-	{"tls", setup.TLS},
+	{"tls", setup.TLS}, // letsencrypt is set up just after tls
 	{"bind", setup.BindHost},
 
 	// Other directives that don't create HTTP handlers
diff --git a/caddy/letsencrypt/letsencrypt.go b/caddy/letsencrypt/letsencrypt.go
index b060afa71..eb8e83f82 100644
--- a/caddy/letsencrypt/letsencrypt.go
+++ b/caddy/letsencrypt/letsencrypt.go
@@ -40,10 +40,16 @@ import (
 // Activate returns the updated list of configs, since
 // some may have been appended, for example, to redirect
 // plaintext HTTP requests to their HTTPS counterpart.
+// This function only appends; it does not prepend or splice.
 func Activate(configs []server.Config) ([]server.Config, error) {
 	// just in case previous caller forgot...
 	Deactivate()
 
+	// TODO: All the output the end user should see when running caddy is something
+	// simple like "Setting up HTTPS..." (and maybe 'done' at the end of the line when finished).
+	// In other words, hide all the other logging except for on errors. Or maybe
+	// have a place to put those logs.
+
 	// reset cached ocsp statuses from any previous activations
 	ocspStatus = make(map[*[]byte]int)