Better error handling at startup and fixed some bugs

Fixed bug where manually specifying port 443 disabled TLS (whoops); otherHostHasScheme was the culprit, since it would return true even if it was the same config that had that scheme.

Also, an error at startup (if not a restart) is now fatal, rather than keeping a half-alive zombie server.

caddy/caddy.go

@@ -127,7 +127,7 @@ func Start(cdyfile Input) error {
 	}
 
 	// Close remaining file descriptors we may have inherited that we don't need
-	if isRestart() {
+	if IsRestart() {
 		for _, fdIndex := range loadedGob.ListenerFds {
 			file := os.NewFile(fdIndex, "")
 			fln, err := net.FileListener(file)
@@ -138,7 +138,7 @@ func Start(cdyfile Input) error {
 	}
 
 	// Show initialization output
-	if !Quiet && !isRestart() {
+	if !Quiet && !IsRestart() {
 		var checkedFdLimit bool
 		for _, group := range groupings {
 			for _, conf := range group.Configs {
@@ -159,7 +159,7 @@ func Start(cdyfile Input) error {
 	}
 
 	// Tell parent process that we got this
-	if isRestart() {
+	if IsRestart() {
 		ppipe := os.NewFile(3, "") // parent is listening on pipe at index 3
 		ppipe.Write([]byte("success"))
 		ppipe.Close()
@@ -174,6 +174,7 @@ func Start(cdyfile Input) error {
 // until the servers are listening.
 func startServers(groupings Group) error {
 	var startupWg sync.WaitGroup
+	errChan := make(chan error)
 
 	for _, group := range groupings {
 		s, err := server.New(group.BindAddr.String(), group.Configs)
@@ -183,7 +184,7 @@ func startServers(groupings Group) error {
 		s.HTTP2 = HTTP2 // TODO: This setting is temporary
 
 		var ln server.ListenerFile
-		if isRestart() {
+		if IsRestart() {
 			// Look up this server's listener in the map of inherited file descriptors;
 			// if we don't have one, we must make a new one.
 			if fdIndex, ok := loadedGob.ListenerFds[s.Addr]; ok {
@@ -215,11 +216,7 @@ func startServers(groupings Group) error {
 
 			// "use of closed network connection" is normal if doing graceful shutdown...
 			if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
-				if isRestart() {
+				errChan <- err
-					log.Fatal(err)
-				} else {
-					log.Println(err)
-				}
 			}
 		}(s, ln)
 
@@ -234,8 +231,16 @@ func startServers(groupings Group) error {
 		serversMu.Unlock()
 	}
 
+	// Wait for all servers to finish starting
 	startupWg.Wait()
 
+	// Return the first error, if any
+	select {
+	case err := <-errChan:
+		return err
+	default:
+	}
+
 	return nil
 }
 
@@ -269,7 +274,7 @@ func Wait() {
 func LoadCaddyfile(loader func() (Input, error)) (cdyfile Input, err error) {
 	// If we are a fork, finishing the restart is highest priority;
 	// piped input is required in this case.
-	if isRestart() {
+	if IsRestart() {
 		err := gob.NewDecoder(os.Stdin).Decode(&loadedGob)
 		if err != nil {
 			return nil, err

caddy/helpers.go

@@ -46,9 +46,9 @@ type caddyfileGob struct {
 	Caddyfile   Input
 }
 
-// isRestart returns whether this process is, according
+// IsRestart returns whether this process is, according
 // to env variables, a fork as part of a graceful restart.
-func isRestart() bool {
+func IsRestart() bool {
 	return os.Getenv("CADDY_RESTART") == "true"
 }
 

caddy/letsencrypt/letsencrypt.go

@@ -57,8 +57,8 @@ func Activate(configs []server.Config) ([]server.Config, error) {
 	// we already have certs and keys in storage from last time.
 	configLen := len(configs) // avoid infinite loop since this loop appends plaintext to the slice
 	for i := 0; i < configLen; i++ {
-		if existingCertAndKey(configs[i].Host) && configQualifies(configs[i], configs) {
+		if existingCertAndKey(configs[i].Host) && configQualifies(configs, i) {
-			configs = autoConfigure(&configs[i], configs)
+			configs = autoConfigure(configs, i)
 		}
 	}
 
@@ -123,7 +123,7 @@ func Activate(configs []server.Config) ([]server.Config, error) {
 
 		// it all comes down to this: turning on TLS with all the new certs
 		for i := 0; i < len(serverConfigs); i++ {
-			configs = autoConfigure(serverConfigs[i], configs)
+			configs = autoConfigure(configs, i)
 		}
 	}
 
@@ -151,10 +151,11 @@ func Deactivate() (err error) {
 	return
 }
 
-// configQualifies returns true if cfg qualifes for automatic LE activation,
+// configQualifies returns true if the config at cfgIndex (within allConfigs)
-// but it does require the list of all configs to be passed in as well.
+// qualifes for automatic LE activation. It does NOT check to see if a cert
-// It does NOT check to see if a cert and key already exist for cfg.
+// and key already exist for the config.
-func configQualifies(cfg server.Config, allConfigs []server.Config) bool {
+func configQualifies(allConfigs []server.Config, cfgIndex int) bool {
+	cfg := allConfigs[cfgIndex]
 	return cfg.TLS.Certificate == "" && // user could provide their own cert and key
 		cfg.TLS.Key == "" &&
 
@@ -167,11 +168,11 @@ func configQualifies(cfg server.Config, allConfigs []server.Config) bool {
 		cfg.Host != "" &&
 		cfg.Host != "0.0.0.0" &&
 		cfg.Host != "::1" &&
-		!strings.HasPrefix(cfg.Host, "127.") && // to use a boulder on your own machine, add fake domain to hosts file
+		!strings.HasPrefix(cfg.Host, "127.") && // to use boulder on your own machine, add fake domain to hosts file
 		// not excluding 10.* and 192.168.* hosts for possibility of running internal Boulder instance
 
-		// make sure an HTTPS version of this config doesn't exist in the list already
+		// make sure another HTTPS version of this config doesn't exist in the list already
-		!hostHasOtherScheme(cfg.Host, "https", allConfigs)
+		!otherHostHasScheme(allConfigs, cfgIndex, "https")
 }
 
 // groupConfigsByEmail groups configs by user email address. The returned map is
@@ -186,7 +187,7 @@ func groupConfigsByEmail(configs []server.Config) (map[string][]*server.Config,
 		// that we won't be obtaining certs for - this way we won't
 		// bother the user for an email address unnecessarily and
 		// we don't obtain new certs for a host we already have certs for.
-		if existingCertAndKey(configs[i].Host) || !configQualifies(configs[i], configs) {
+		if existingCertAndKey(configs[i].Host) || !configQualifies(configs, i) {
 			continue
 		}
 		leEmail := getEmail(configs[i])
@@ -311,12 +312,14 @@ func saveCertsAndKeys(certificates []acme.CertificateResource) error {
 	return nil
 }
 
-// autoConfigure enables TLS on cfg and appends, if necessary, a new config
+// autoConfigure enables TLS on allConfigs[cfgIndex] and appends, if necessary,
-// to allConfigs that redirects plaintext HTTP to its new HTTPS counterpart.
+// a new config to allConfigs that redirects plaintext HTTP to its new HTTPS
-// It expects the certificate and key to already be in storage. It returns
+// counterpart. It expects the certificate and key to already be in storage. It
-// the new list of allConfigs, since it may append a new config. This function
+// returns the new list of allConfigs, since it may append a new config. This
-// assumes that cfg was already set up for HTTPS.
+// function assumes that allConfigs[cfgIndex] is already set up for HTTPS.
-func autoConfigure(cfg *server.Config, allConfigs []server.Config) []server.Config {
+func autoConfigure(allConfigs []server.Config, cfgIndex int) []server.Config {
+	cfg := &allConfigs[cfgIndex]
+
 	bundleBytes, err := ioutil.ReadFile(storage.SiteCertFile(cfg.Host))
 	// TODO: Handle these errors better
 	if err == nil {
@@ -344,28 +347,32 @@ func autoConfigure(cfg *server.Config, allConfigs []server.Config) []server.Conf
 		acmeHandlers[cfg.Host] = handler
 	}
 
-	// Set up http->https redirect as long as there isn't already
+	// Set up http->https redirect as long as there isn't already a http counterpart
-	// a http counterpart in the configs
+	// in the configs and this isn't, for some reason, already on port 80
-	if !hostHasOtherScheme(cfg.Host, "http", allConfigs) {
+	if !otherHostHasScheme(allConfigs, cfgIndex, "http") &&
+		cfg.Port != "80" && cfg.Port != "http" { // (would not be http port with current program flow, but just in case)
 		allConfigs = append(allConfigs, redirPlaintextHost(*cfg))
 	}
 
 	return allConfigs
 }
 
-// hostHasOtherScheme tells you whether there is another config in the list
+// otherHostHasScheme tells you whether there is ANOTHER config in allConfigs
-// for the same host but with the port equal to scheme. For example, to see
+// for the same host but with the port equal to scheme as allConfigs[cfgIndex].
-// if example.com has a https variant already, pass in example.com and
+// This function considers "443" and "https" to be the same scheme, as well as
-// "https" along with the list of configs. This function considers "443"
+// "http" and "80". It does not tell you whether there is ANY config with scheme,
-// and "https" to be the same scheme, as well as "http" and "80".
+// only if there's a different one with it.
-func hostHasOtherScheme(host, scheme string, allConfigs []server.Config) bool {
+func otherHostHasScheme(allConfigs []server.Config, cfgIndex int, scheme string) bool {
 	if scheme == "80" {
 		scheme = "http"
 	} else if scheme == "443" {
 		scheme = "https"
 	}
-	for _, otherCfg := range allConfigs {
+	for i, otherCfg := range allConfigs {
-		if otherCfg.Host == host {
+		if i == cfgIndex {
+			continue // has to be a config OTHER than the one we're comparing against
+		}
+		if otherCfg.Host == allConfigs[cfgIndex].Host {
 			if (otherCfg.Port == scheme) ||
 				(scheme == "https" && otherCfg.Port == "443") ||
 				(scheme == "http" && otherCfg.Port == "80") {

main.go

@@ -79,7 +79,11 @@ func main() {
 	// Start your engines
 	err = caddy.Start(caddyfile)
 	if err != nil {
-		log.Fatal(err)
+		if caddy.IsRestart() {
+			log.Println("error starting servers:", err)
+		} else {
+			log.Fatal(err)
+		}
 	}
 
 	// Twiddle your thumbs