tsdns: be more strict with type validation.

Previously, a type AAAA query would be answered with an A record
if only an IPv4 address was available. This is irrelevant for us
while we only use IPv4, but it will be a bug one day,
so it's worth being precise about semantics.

Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>

wgengine/tsdns/tsdns.go

@@ -182,7 +182,8 @@ func (r *Resolver) NextResponse() (Packet, error) {
 	}
 }
 
-// Resolve maps a given domain name to the IP address of the host that owns it.
+// Resolve maps a given domain name to the IP address of the host that owns it,
+// if the IP address conforms to the DNS resource type given by tp (one of A, AAAA, ALL).
 // The domain name must be in canonical form (with a trailing period).
 func (r *Resolver) Resolve(domain string, tp dns.Type) (netaddr.IP, dns.RCode, error) {
 	r.mu.Lock()
@@ -209,8 +210,18 @@ func (r *Resolver) Resolve(domain string, tp dns.Type) (netaddr.IP, dns.RCode, e
 		return netaddr.IP{}, dns.RCodeNameError, nil
 	}
 
-	switch tp {
-	case dns.TypeA, dns.TypeAAAA, dns.TypeALL:
+	// Refactoring note: this must happen after we check suffixes,
+	// otherwise we will respond with NOTIMP to requests that should be forwarded.
+	switch {
+	case tp == dns.TypeA || tp == dns.TypeALL:
+		if !addr.Is4() {
+			return netaddr.IP{}, dns.RCodeSuccess, nil
+		}
+		return addr, dns.RCodeSuccess, nil
+	case tp == dns.TypeAAAA || tp == dns.TypeALL:
+		if !addr.Is6() {
+			return netaddr.IP{}, dns.RCodeSuccess, nil
+		}
 		return addr, dns.RCodeSuccess, nil
 	default:
 		return netaddr.IP{}, dns.RCodeNotImplemented, errNotImplemented
@@ -284,7 +295,7 @@ type response struct {
 	Question dns.Question
 	// Name is the response to a PTR query.
 	Name string
-	// IP is the response to an A, AAAA, or ANY query.
+	// IP is the response to an A, AAAA, or ALL query.
 	IP netaddr.IP
 }
 
@@ -395,7 +406,7 @@ func marshalResponse(resp *response) ([]byte, error) {
 	case dns.TypeA, dns.TypeAAAA, dns.TypeALL:
 		if resp.IP.Is4() {
 			err = marshalARecord(resp.Question.Name, resp.IP, &builder)
-		} else {
+		} else if resp.IP.Is6() {
 			err = marshalAAAARecord(resp.Question.Name, resp.IP, &builder)
 		}
 	case dns.TypePTR:

wgengine/tsdns/tsdns_test.go

@@ -211,6 +211,7 @@ func TestResolve(t *testing.T) {
 	}{
 		{"ipv4", "test1.ipn.dev.", dns.TypeA, testipv4, dns.RCodeSuccess},
 		{"ipv6", "test2.ipn.dev.", dns.TypeAAAA, testipv6, dns.RCodeSuccess},
+		{"no-ipv6", "test1.ipn.dev.", dns.TypeAAAA, netaddr.IP{}, dns.RCodeSuccess},
 		{"nxdomain", "test3.ipn.dev.", dns.TypeA, netaddr.IP{}, dns.RCodeNameError},
 		{"foreign domain", "google.com.", dns.TypeA, netaddr.IP{}, dns.RCodeRefused},
 	}
@@ -503,6 +504,23 @@ func TestConcurrentSetUpstreams(t *testing.T) {
 	wg.Wait()
 }
 
+var allResponse = []byte{
+	0x00, 0x00, // transaction id: 0
+	0x84, 0x00, // flags: response, authoritative, no error
+	0x00, 0x01, // one question
+	0x00, 0x01, // one answer
+	0x00, 0x00, 0x00, 0x00, // no authority or additional RRs
+	// Question:
+	0x05, 0x74, 0x65, 0x73, 0x74, 0x31, 0x03, 0x69, 0x70, 0x6e, 0x03, 0x64, 0x65, 0x76, 0x00, // name
+	0x00, 0xff, 0x00, 0x01, // type ALL, class IN
+	// Answer:
+	0x05, 0x74, 0x65, 0x73, 0x74, 0x31, 0x03, 0x69, 0x70, 0x6e, 0x03, 0x64, 0x65, 0x76, 0x00, // name
+	0x00, 0x01, 0x00, 0x01, // type A, class IN
+	0x00, 0x00, 0x02, 0x58, // TTL: 600
+	0x00, 0x04, // length: 4 bytes
+	0x01, 0x02, 0x03, 0x04, // A: 1.2.3.4
+}
+
 var ipv4Response = []byte{
 	0x00, 0x00, // transaction id: 0
 	0x84, 0x00, // flags: response, authoritative, no error
@@ -586,6 +604,17 @@ var nxdomainResponse = []byte{
 	0x00, 0x01, 0x00, 0x01, // type A, class IN
 }
 
+var emptyResponse = []byte{
+	0x00, 0x00, // transaction id: 0
+	0x84, 0x00, // flags: response, authoritative, no error
+	0x00, 0x01, // one question
+	0x00, 0x00, // no answers
+	0x00, 0x00, 0x00, 0x00, // no authority or additional RRs
+	// Question:
+	0x05, 0x74, 0x65, 0x73, 0x74, 0x31, 0x03, 0x69, 0x70, 0x6e, 0x03, 0x64, 0x65, 0x76, 0x00, // name
+	0x00, 0x1c, 0x00, 0x01, // type AAAA, class IN
+}
+
 func TestFull(t *testing.T) {
 	r := NewResolver(ResolverConfig{Logf: t.Logf, Forward: false})
 	r.SetMap(dnsMap)
@@ -601,8 +630,10 @@ func TestFull(t *testing.T) {
 		request  []byte
 		response []byte
 	}{
+		{"all", dnspacket("test1.ipn.dev.", dns.TypeALL), allResponse},
 		{"ipv4", dnspacket("test1.ipn.dev.", dns.TypeA), ipv4Response},
 		{"ipv6", dnspacket("test2.ipn.dev.", dns.TypeAAAA), ipv6Response},
+		{"no-ipv6", dnspacket("test1.ipn.dev.", dns.TypeAAAA), emptyResponse},
 		{"upper", dnspacket("TEST1.IPN.DEV.", dns.TypeA), ipv4UppercaseResponse},
 		{"ptr", dnspacket("4.3.2.1.in-addr.arpa.", dns.TypePTR), ptrResponse},
 		{"nxdomain", dnspacket("test3.ipn.dev.", dns.TypeA), nxdomainResponse},