From 350932161d6745836c1b2f39849bddb0f9fb52fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Frank=20Schr=C3=B6der?= <frank.schroeder@gmail.com>
Date: Sat, 21 Oct 2017 01:49:17 +0200
Subject: [PATCH] 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      | 19 +++++++++++++++++++
 agent/dns_test.go |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/agent/dns.go b/agent/dns.go
index e3dabebe74..d77a51735e 100644
--- a/agent/dns.go
+++ b/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]
diff --git a/agent/dns_test.go b/agent/dns_test.go
index 776314a4e3..ba9006b2ff 100644
--- a/agent/dns_test.go
+++ b/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)