httpcaddyfile: Detect ambiguous site definitions (fix #4635)

Previously, our "duplicate key in server block" logic was flawed because
it did not account for the site's bind address. We defer this check to
when the listener addresses have been assigned, but before we commit
a server block to its listener.

Also refined how network address parsing and joining works, which was
necessary for a less convoluted fix.

2 files changed, 28 insertions, 16 deletions

diff --git a/caddyconfig/httpcaddyfile/addresses.go b/caddyconfig/httpcaddyfile/addresses.go
index c7923e8..03083d8 100644
--- a/caddyconfig/httpcaddyfile/addresses.go
+++ b/caddyconfig/httpcaddyfile/addresses.go
@@ -183,6 +183,8 @@ func (st *ServerType) consolidateAddrMappings(addrToServerBlocks map[string][]se
 	return sbaddrs
 }
 
+// listenerAddrsForServerBlockKey essentially converts the Caddyfile
+// site addresses to Caddy listener addresses for each server block.
 func (st *ServerType) listenerAddrsForServerBlockKey(sblock serverBlock, key string,
 	options map[string]interface{}) ([]string, error) {
 	addr, err := ParseAddress(key)
@@ -232,12 +234,14 @@ func (st *ServerType) listenerAddrsForServerBlockKey(sblock serverBlock, key str
 	// use a map to prevent duplication
 	listeners := make(map[string]struct{})
 	for _, host := range lnHosts {
-		addr, err := caddy.ParseNetworkAddress(host)
-		if err == nil && addr.IsUnixNetwork() {
-			listeners[host] = struct{}{}
-		} else {
-			listeners[host+":"+lnPort] = struct{}{}
+		// host can have network + host (e.g. "tcp6/localhost") but
+		// will/should not have port information because this usually
+		// comes from the bind directive, so we append the port
+		addr, err := caddy.ParseNetworkAddress(host + ":" + lnPort)
+		if err != nil {
+			return nil, fmt.Errorf("parsing network address: %v", err)
 		}
+		listeners[addr.String()] = struct{}{}
 	}
 
 	// now turn map into list
diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index 5d78244..b114059 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -58,22 +58,13 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
 	gc := counter{new(int)}
 	state := make(map[string]interface{})
 
-	// load all the server blocks and associate them with a "pile"
-	// of config values; also prohibit duplicate keys because they
-	// can make a config confusing if more than one server block is
-	// chosen to handle a request - we actually will make each
-	// server block's route terminal so that only one will run
-	sbKeys := make(map[string]struct{})
+	// load all the server blocks and associate them with a "pile" of config values
 	originalServerBlocks := make([]serverBlock, 0, len(inputServerBlocks))
-	for i, sblock := range inputServerBlocks {
+	for _, sblock := range inputServerBlocks {
 		for j, k := range sblock.Keys {
 			if j == 0 && strings.HasPrefix(k, "@") {
 				return nil, warnings, fmt.Errorf("cannot define a matcher outside of a site block: '%s'", k)
 			}
-			if _, ok := sbKeys[k]; ok {
-				return nil, warnings, fmt.Errorf("duplicate site address not allowed: '%s' in %v (site block %d, key %d)", k, sblock.Keys, i, j)
-			}
-			sbKeys[k] = struct{}{}
 		}
 		originalServerBlocks = append(originalServerBlocks, serverBlock{
 			block: sblock,
@@ -420,6 +411,23 @@ func (st *ServerType) serversFromPairings(
 	}
 
 	for i, p := range pairings {
+		// detect ambiguous site definitions: server blocks which
+		// have the same host bound to the same interface (listener
+		// address), otherwise their routes will improperly be added
+		// to the same server (see issue #4635)
+		for j, sblock1 := range p.serverBlocks {
+			for _, key := range sblock1.block.Keys {
+				for k, sblock2 := range p.serverBlocks {
+					if k == j {
+						continue
+					}
+					if sliceContains(sblock2.block.Keys, key) {
+						return nil, fmt.Errorf("ambiguous site definition: %s", key)
+					}
+				}
+			}
+		}
+
 		srv := &caddyhttp.Server{
 			Listen: p.addresses,
 		}