Merge pull request #2015 from mholt/cert-cache

tls: Restructure and improve certificate management

caddy.go

@@ -77,8 +77,18 @@ var (
 	mu sync.Mutex
 )
 
+func init() {
+	OnProcessExit = append(OnProcessExit, func() {
+		if PidFile != "" {
+			os.Remove(PidFile)
+		}
+	})
+}
+
 // Instance contains the state of servers created as a result of
 // calling Start and can be used to access or control those servers.
+// It is literally an instance of a server type. Instance values
+// should NOT be copied. Use *Instance for safety.
 type Instance struct {
 	// serverType is the name of the instance's server type
 	serverType string
@@ -89,10 +99,11 @@ type Instance struct {
 	// wg is used to wait for all servers to shut down
 	wg *sync.WaitGroup
 
-	// context is the context created for this instance.
+	// context is the context created for this instance,
+	// used to coordinate the setting up of the server type
 	context Context
 
-	// servers is the list of servers with their listeners.
+	// servers is the list of servers with their listeners
 	servers []ServerListener
 
 	// these callbacks execute when certain events occur
@@ -101,6 +112,18 @@ type Instance struct {
 	onRestart       []func() error // before restart commences
 	onShutdown      []func() error // stopping, even as part of a restart
 	onFinalShutdown []func() error // stopping, not as part of a restart
+
+	// storing values on an instance is preferable to
+	// global state because these will get garbage-
+	// collected after in-process reloads when the
+	// old instances are destroyed; use StorageMu
+	// to access this value safely
+	Storage   map[interface{}]interface{}
+	StorageMu sync.RWMutex
+}
+
+func Instances() []*Instance {
+	return instances
 }
 
 // Servers returns the ServerListeners in i.
@@ -196,7 +219,7 @@ func (i *Instance) Restart(newCaddyfile Input) (*Instance, error) {
 	}
 
 	// create new instance; if the restart fails, it is simply discarded
-	newInst := &Instance{serverType: newCaddyfile.ServerType(), wg: i.wg}
+	newInst := &Instance{serverType: newCaddyfile.ServerType(), wg: i.wg, Storage: make(map[interface{}]interface{})}
 
 	// attempt to start new instance
 	err := startWithListenerFds(newCaddyfile, newInst, restartFds)
@@ -455,7 +478,7 @@ func (i *Instance) Caddyfile() Input {
 //
 // This function blocks until all the servers are listening.
 func Start(cdyfile Input) (*Instance, error) {
-	inst := &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup)}
+	inst := &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup), Storage: make(map[interface{}]interface{})}
 	err := startWithListenerFds(cdyfile, inst, nil)
 	if err != nil {
 		return inst, err
@@ -468,11 +491,34 @@ func Start(cdyfile Input) (*Instance, error) {
 }
 
 func startWithListenerFds(cdyfile Input, inst *Instance, restartFds map[string]restartTriple) error {
+	// save this instance in the list now so that
+	// plugins can access it if need be, for example
+	// the caddytls package, so it can perform cert
+	// renewals while starting up; we just have to
+	// remove the instance from the list later if
+	// it fails
+	instancesMu.Lock()
+	instances = append(instances, inst)
+	instancesMu.Unlock()
+	var err error
+	defer func() {
+		if err != nil {
+			instancesMu.Lock()
+			for i, otherInst := range instances {
+				if otherInst == inst {
+					instances = append(instances[:i], instances[i+1:]...)
+					break
+				}
+			}
+			instancesMu.Unlock()
+		}
+	}()
+
 	if cdyfile == nil {
 		cdyfile = CaddyfileInput{}
 	}
 
-	err := ValidateAndExecuteDirectives(cdyfile, inst, false)
+	err = ValidateAndExecuteDirectives(cdyfile, inst, false)
 	if err != nil {
 		return err
 	}
@@ -504,10 +550,6 @@ func startWithListenerFds(cdyfile Input, inst *Instance, restartFds map[string]r
 		return err
 	}
 
-	instancesMu.Lock()
-	instances = append(instances, inst)
-	instancesMu.Unlock()
-
 	// run any AfterStartup callbacks if this is not
 	// part of a restart; then show file descriptor notice
 	if restartFds == nil {
@@ -546,7 +588,7 @@ func startWithListenerFds(cdyfile Input, inst *Instance, restartFds map[string]r
 func ValidateAndExecuteDirectives(cdyfile Input, inst *Instance, justValidate bool) error {
 	// If parsing only inst will be nil, create an instance for this function call only.
 	if justValidate {
-		inst = &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup)}
+		inst = &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup), Storage: make(map[interface{}]interface{})}
 	}
 
 	stypeName := cdyfile.ServerType()
@@ -563,14 +605,14 @@ func ValidateAndExecuteDirectives(cdyfile Input, inst *Instance, justValidate bo
 		return err
 	}
 
-	inst.context = stype.NewContext()
+	inst.context = stype.NewContext(inst)
 	if inst.context == nil {
 		return fmt.Errorf("server type %s produced a nil Context", stypeName)
 	}
 
 	sblocks, err = inst.context.InspectServerBlocks(cdyfile.Path(), sblocks)
 	if err != nil {
-		return err
+		return fmt.Errorf("error inspecting server blocks: %v", err)
 	}
 
 	return executeDirectives(inst, cdyfile.Path(), stype.Directives(), sblocks, justValidate)

caddyhttp/httpserver/https.go

@@ -27,7 +27,7 @@ func activateHTTPS(cctx caddy.Context) error {
 	operatorPresent := !caddy.Started()
 
 	if !caddy.Quiet && operatorPresent {
-		fmt.Print("Activating privacy features...")
+		fmt.Print("Activating privacy features... ")
 	}
 
 	ctx := cctx.(*httpContext)
@@ -69,7 +69,7 @@ func activateHTTPS(cctx caddy.Context) error {
 	}
 
 	if !caddy.Quiet && operatorPresent {
-		fmt.Println(" done.")
+		fmt.Println("done.")
 	}
 
 	return nil
@@ -160,33 +160,49 @@ func hostHasOtherPort(allConfigs []*SiteConfig, thisConfigIdx int, otherPort str
 // to listen on HTTPPort. The TLS field of cfg must not be nil.
 func redirPlaintextHost(cfg *SiteConfig) *SiteConfig {
 	redirPort := cfg.Addr.Port
-	if redirPort == DefaultHTTPSPort {
-		redirPort = "" // default port is redundant
+	if redirPort == HTTPSPort {
+		// By default, HTTPSPort should be DefaultHTTPSPort,
+		// which of course doesn't need to be explicitly stated
+		// in the Location header. Even if HTTPSPort is changed
+		// so that it is no longer DefaultHTTPSPort, we shouldn't
+		// append it to the URL in the Location because changing
+		// the HTTPS port is assumed to be an internal-only change
+		// (in other words, we assume port forwarding is going on);
+		// but redirects go back to a presumably-external client.
+		// (If redirect clients are also internal, that is more
+		// advanced, and the user should configure HTTP->HTTPS
+		// redirects themselves.)
+		redirPort = ""
 	}
+
 	redirMiddleware := func(next Handler) Handler {
 		return HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) {
-			// Construct the URL to which to redirect. Note that the Host in a request might
-			// contain a port, but we just need the hostname; we'll set the port if needed.
+			// Construct the URL to which to redirect. Note that the Host in a
+			// request might contain a port, but we just need the hostname from
+			// it; and we'll set the port if needed.
 			toURL := "https://"
 			requestHost, _, err := net.SplitHostPort(r.Host)
 			if err != nil {
-				requestHost = r.Host // Host did not contain a port; great
+				requestHost = r.Host // Host did not contain a port, so use the whole value
 			}
 			if redirPort == "" {
 				toURL += requestHost
 			} else {
 				toURL += net.JoinHostPort(requestHost, redirPort)
 			}
+
 			toURL += r.URL.RequestURI()
 
 			w.Header().Set("Connection", "close")
 			http.Redirect(w, r, toURL, http.StatusMovedPermanently)
 			return 0, nil
 		})
 	}
+
 	host := cfg.Addr.Host
 	port := HTTPPort
 	addr := net.JoinHostPort(host, port)
+
 	return &SiteConfig{
 		Addr:       Address{Original: addr, Host: host, Port: port},
 		ListenHost: cfg.ListenHost,

caddyhttp/httpserver/https_test.go

@@ -53,7 +53,7 @@ func TestRedirPlaintextHost(t *testing.T) {
 		},
 		{
 			Host: "foohost",
-			Port: "443", // since this is the default HTTPS port, should not be included in Location value
+			Port: HTTPSPort, // since this is the 'default' HTTPS port, should not be included in Location value
 		},
 		{
 			Host:        "*.example.com",

caddyhttp/httpserver/plugin.go

@@ -91,11 +91,13 @@ func hideCaddyfile(cctx caddy.Context) error {
 	return nil
 }
 
-func newContext() caddy.Context {
-	return &httpContext{keysToSiteConfigs: make(map[string]*SiteConfig)}
+func newContext(inst *caddy.Instance) caddy.Context {
+	return &httpContext{instance: inst, keysToSiteConfigs: make(map[string]*SiteConfig)}
 }
 
 type httpContext struct {
+	instance *caddy.Instance
+
 	// keysToSiteConfigs maps an address at the top of a
 	// server block (a "key") to its SiteConfig. Not all
 	// SiteConfigs will be represented here, only ones
@@ -115,12 +117,14 @@ func (h *httpContext) saveConfig(key string, cfg *SiteConfig) {
 // executing directives and otherwise prepares the directives to
 // be parsed and executed.
 func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []caddyfile.ServerBlock) ([]caddyfile.ServerBlock, error) {
+	siteAddrs := make(map[string]string)
+
 	// For each address in each server block, make a new config
 	for _, sb := range serverBlocks {
 		for _, key := range sb.Keys {
 			key = strings.ToLower(key)
 			if _, dup := h.keysToSiteConfigs[key]; dup {
-				return serverBlocks, fmt.Errorf("duplicate site address: %s", key)
+				return serverBlocks, fmt.Errorf("duplicate site key: %s", key)
 			}
 			addr, err := standardizeAddress(key)
 			if err != nil {
@@ -136,6 +140,23 @@ func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []cadd
 				addr.Port = Port
 			}
 
+			// Make sure the adjusted site address is distinct
+			addrCopy := addr // make copy so we don't disturb the original, carefully-parsed address struct
+			if addrCopy.Port == "" && Port == DefaultPort {
+				addrCopy.Port = Port
+			}
+			addrStr := strings.ToLower(addrCopy.String())
+			if otherSiteKey, dup := siteAddrs[addrStr]; dup {
+				err := fmt.Errorf("duplicate site address: %s", addrStr)
+				if (addrCopy.Host == Host && Host != DefaultHost) ||
+					(addrCopy.Port == Port && Port != DefaultPort) {
+					err = fmt.Errorf("site defined as %s is a duplicate of %s because of modified "+
+						"default host and/or port values (usually via -host or -port flags)", key, otherSiteKey)
+				}
+				return serverBlocks, err
+			}
+			siteAddrs[addrStr] = key
+
 			// If default HTTP or HTTPS ports have been customized,
 			// make sure the ACME challenge ports match
 			var altHTTPPort, altTLSSNIPort string
@@ -146,15 +167,19 @@ func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []cadd
 				altTLSSNIPort = HTTPSPort
 			}
 
+			// Make our caddytls.Config, which has a pointer to the
+			// instance's certificate cache and enough information
+			// to use automatic HTTPS when the time comes
+			caddytlsConfig := caddytls.NewConfig(h.instance)
+			caddytlsConfig.Hostname = addr.Host
+			caddytlsConfig.AltHTTPPort = altHTTPPort
+			caddytlsConfig.AltTLSSNIPort = altTLSSNIPort
+
 			// Save the config to our master list, and key it for lookups
 			cfg := &SiteConfig{
-				Addr: addr,
-				Root: Root,
-				TLS: &caddytls.Config{
-					Hostname:      addr.Host,
-					AltHTTPPort:   altHTTPPort,
-					AltTLSSNIPort: altTLSSNIPort,
-				},
+				Addr:            addr,
+				Root:            Root,
+				TLS:             caddytlsConfig,
 				originCaddyfile: sourceFile,
 				IndexPages:      staticfiles.DefaultIndexPages,
 			}

caddyhttp/httpserver/plugin_test.go

@@ -137,7 +137,7 @@ func TestAddressString(t *testing.T) {
 func TestInspectServerBlocksWithCustomDefaultPort(t *testing.T) {
 	Port = "9999"
 	filename := "Testfile"
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	input := strings.NewReader(`localhost`)
 	sblocks, err := caddyfile.Parse(filename, input, nil)
 	if err != nil {
@@ -153,9 +153,26 @@ func TestInspectServerBlocksWithCustomDefaultPort(t *testing.T) {
 	}
 }
 
+// See discussion on PR #2015
+func TestInspectServerBlocksWithAdjustedAddress(t *testing.T) {
+	Port = DefaultPort
+	Host = "example.com"
+	filename := "Testfile"
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
+	input := strings.NewReader("example.com {\n}\n:2015 {\n}")
+	sblocks, err := caddyfile.Parse(filename, input, nil)
+	if err != nil {
+		t.Fatalf("Expected no error setting up test, got: %v", err)
+	}
+	_, err = ctx.InspectServerBlocks(filename, sblocks)
+	if err == nil {
+		t.Fatalf("Expected an error because site definitions should overlap, got: %v", err)
+	}
+}
+
 func TestInspectServerBlocksCaseInsensitiveKey(t *testing.T) {
 	filename := "Testfile"
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	input := strings.NewReader("localhost {\n}\nLOCALHOST {\n}")
 	sblocks, err := caddyfile.Parse(filename, input, nil)
 	if err != nil {
@@ -207,7 +224,7 @@ func TestDirectivesList(t *testing.T) {
 }
 
 func TestContextSaveConfig(t *testing.T) {
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	ctx.saveConfig("foo", new(SiteConfig))
 	if _, ok := ctx.keysToSiteConfigs["foo"]; !ok {
 		t.Error("Expected config to be saved, but it wasn't")
@@ -226,7 +243,7 @@ func TestContextSaveConfig(t *testing.T) {
 
 // Test to make sure we are correctly hiding the Caddyfile
 func TestHideCaddyfile(t *testing.T) {
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	ctx.saveConfig("test", &SiteConfig{
 		Root:            Root,
 		originCaddyfile: "Testfile",