dns: return NXDOMAIN if datacenter is invalid (#3200) (#3596)

Queries to the DNS server can contain an optional datacenter
name in the query name. You can query for 'foo.service.consul'
or 'foo.service.dc.consul' to get a response for either the
default or a specific datacenter.

Datacenter names cannot have dots, therefore the datacenter
name can refer to only one element in the DNS query name.

The DNS server allowed extra labels between the optional
datacenter name and the domain and returned a valid response
instead of returning NXDOMAIN. For example, if the domain
is set to '.consul' then 'foo.service.dc1.extra.consul'
should return NXDOMAIN because of 'extra' being between
the datacenter name 'dc1' and the domain '.consul'.

Fixes #3200

agent/dns.go

@@ -369,6 +369,9 @@ func (d *DNSServer) dispatch(network string, req, resp *dns.Msg) {
 	// Split into the label parts
 	labels := dns.SplitDomainName(qName)
 
+	// Provide a flag for remembering whether the datacenter name was parsed already.
+	var dcParsed bool
+
 	// The last label is either "node", "service", "query", "_<protocol>", or a datacenter name
 PARSE:
 	n := len(labels)
@@ -475,6 +478,22 @@ PARSE:
 		}
 
 	default:
+		// https://github.com/hashicorp/consul/issues/3200
+		//
+		// Since datacenter names cannot contain dots we can only allow one
+		// label between the query type and the domain to be the datacenter name.
+		// Since the datacenter name is optional and the parser strips off labels at the end until it finds a suitable
+		// query type label we return NXDOMAIN when we encounter another label
+		// which could be the datacenter name.
+		//
+		// If '.consul' is the domain then
+		//  * foo.service.dc.consul is OK
+		//  * foo.service.dc.stuff.consul is not OK
+		if dcParsed {
+			goto INVALID
+		}
+		dcParsed = true
+
 		// Store the DC, and re-parse
 		datacenter = labels[n-1]
 		labels = labels[:n-1]

agent/dns_test.go

@@ -4154,6 +4154,9 @@ func TestDNS_InvalidQueries(t *testing.T) {
 		"node.consul.",
 		"service.consul.",
 		"query.consul.",
+		"foo.node.dc1.extra.consul.",
+		"foo.service.dc1.extra.consul.",
+		"foo.query.dc1.extra.consul.",
 	}
 	for _, question := range questions {
 		m := new(dns.Msg)