plugin/file: resolve each additional section target only once (#8286)

* plugin/file: resolve each additional section target only once

additionalProcessing walked the answer and appended each target's address records without keeping track of the targets it had already resolved. An answer holding several records that point at one target, two MX records differing only in preference for instance, therefore repeated that target's A, AAAA and RRSIGs once per record. SRV, SVCB and HTTPS take the same path and behaved the same way. plugin/backend_lookup.go already skips targets it has seen; the file plugin never did.

Targets are compared in canonical form because SRV targets are not lowercased on insert, so one target can reach additional processing spelled two ways while the zone's tree still matches it case-insensitively. The map is allocated on first use, as additionalProcessing runs for every answer and most carry no target at all.

Fixes #6626

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

* plugin/file: move additional section tests to lookup_test.go

The cases exercise additionalProcessing, which lives in lookup.go, so they belong with the rest of the lookup tests. No test logic is changed.

Addresses review feedback.

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

---------

Signed-off-by: baltasarblanco <baltablanco9008@gmail.com>

plugin/file/lookup.go

@@ -434,8 +434,11 @@ func (z *Zone) doLookup(ctx context.Context, state request.Request, target strin
 }
 
 // additionalProcessing checks the current answer section and retrieves A or AAAA records
-// (and possible SIGs) to need to be put in the additional section.
+// (and possible SIGs) to need to be put in the additional section. A target referenced by
+// more than one record is only resolved once.
 func (z *Zone) additionalProcessing(answer []dns.RR, do bool) (extra []dns.RR) {
+	var lookup map[string]struct{}
+
 	for _, rr := range answer {
 		name := ""
 		switch x := rr.(type) {
@@ -452,6 +455,20 @@ func (z *Zone) additionalProcessing(answer []dns.RR, do bool) (extra []dns.RR) {
 			continue
 		}
 
+		// The answer can reference one target more than once, e.g. two MX records that only
+		// differ in preference. Its addresses belong in the additional section once. Compare
+		// canonically: SRV targets are not lowercased on insert (see Zone.Insert), while the
+		// zone's tree matches names case-insensitively.
+		target := dns.CanonicalName(name)
+		if _, ok := lookup[target]; ok {
+			continue
+		}
+		if lookup == nil {
+			// Allocate on first use: this runs for every answer, most of which carry no target.
+			lookup = make(map[string]struct{}, len(answer))
+		}
+		lookup[target] = struct{}{}
+
 		elem, _ := z.Search(name)
 		if elem == nil {
 			continue

plugin/file/lookup_test.go

@@ -360,3 +360,148 @@ txt     IN	TXT     "v=spf1 a mx ~all"
 caa     IN  CAA    0 issue letsencrypt.org
 *.nodata    IN   A      139.162.196.79
 ext-cname   IN   CNAME  example.com.`
+
+var additionalAuth = []dns.RR{
+	test.NS("example.org. 1800 IN NS ns.example.org."),
+}
+
+var additionalTestCases = []test.Case{
+	{
+		// Two MX records that only differ in preference share a target. Its addresses
+		// belong in the additional section once.
+		Qname: "mx.example.org.", Qtype: dns.TypeMX,
+		Answer: []dns.RR{
+			test.MX("mx.example.org. 1800 IN MX 10 host.example.org."),
+			test.MX("mx.example.org. 1800 IN MX 20 host.example.org."),
+		},
+		Ns: additionalAuth,
+		Extra: []dns.RR{
+			test.A("host.example.org. 1800 IN A 192.0.2.10"),
+			test.AAAA("host.example.org. 1800 IN AAAA 2001:db8::10"),
+		},
+	},
+	{
+		// SRV runs through the same additional processing.
+		Qname: "srv.example.org.", Qtype: dns.TypeSRV,
+		Answer: []dns.RR{
+			test.SRV("srv.example.org. 1800 IN SRV 10 50 8080 host.example.org."),
+			test.SRV("srv.example.org. 1800 IN SRV 20 50 8080 host.example.org."),
+		},
+		Ns: additionalAuth,
+		Extra: []dns.RR{
+			test.A("host.example.org. 1800 IN A 192.0.2.10"),
+			test.AAAA("host.example.org. 1800 IN AAAA 2001:db8::10"),
+		},
+	},
+	{
+		// Distinct targets must each still be resolved.
+		Qname: "two.example.org.", Qtype: dns.TypeMX,
+		Answer: []dns.RR{
+			test.MX("two.example.org. 1800 IN MX 10 host.example.org."),
+			test.MX("two.example.org. 1800 IN MX 20 other.example.org."),
+		},
+		Ns: additionalAuth,
+		Extra: []dns.RR{
+			test.A("host.example.org. 1800 IN A 192.0.2.10"),
+			test.AAAA("host.example.org. 1800 IN AAAA 2001:db8::10"),
+			test.A("other.example.org. 1800 IN A 192.0.2.20"),
+			test.AAAA("other.example.org. 1800 IN AAAA 2001:db8::20"),
+		},
+	},
+}
+
+func TestAdditionalSectionDeduplication(t *testing.T) {
+	zone, err := Parse(strings.NewReader(dbAdditionalExample), testAdditionalOrigin, "stdin", 0)
+	if err != nil {
+		t.Fatalf("Expected no error when reading zone, got %q", err)
+	}
+
+	fm := File{Next: test.ErrorHandler(), Zones: Zones{Z: map[string]*Zone{testAdditionalOrigin: zone}, Names: []string{testAdditionalOrigin}}}
+	ctx := context.TODO()
+
+	for _, tc := range additionalTestCases {
+		m := tc.Msg()
+
+		rec := dnstest.NewRecorder(&test.ResponseWriter{})
+		_, err := fm.ServeDNS(ctx, rec, m)
+		if err != nil {
+			t.Errorf("Expected no error for %q/%d, got %v", tc.Qname, tc.Qtype, err)
+			continue
+		}
+
+		resp := rec.Msg
+		if err := test.SortAndCheck(resp, tc); err != nil {
+			t.Errorf("Test %q/%d: %v", tc.Qname, tc.Qtype, err)
+		}
+	}
+}
+
+func TestAdditionalSectionDeduplicationMixedCase(t *testing.T) {
+	// SRV targets are not lowercased on insert, so one target can reach additional
+	// processing under two spellings. The zone's tree matches names case-insensitively,
+	// so both resolve to the same addresses and those belong in the response once.
+	zone, err := Parse(strings.NewReader(dbAdditionalExample), testAdditionalOrigin, "stdin", 0)
+	if err != nil {
+		t.Fatalf("Expected no error when reading zone, got %q", err)
+	}
+
+	fm := File{Next: test.ErrorHandler(), Zones: Zones{Z: map[string]*Zone{testAdditionalOrigin: zone}, Names: []string{testAdditionalOrigin}}}
+	ctx := context.TODO()
+
+	m := new(dns.Msg)
+	m.SetQuestion("mixed.example.org.", dns.TypeSRV)
+
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	if _, err := fm.ServeDNS(ctx, rec, m); err != nil {
+		t.Fatalf("Expected no error, got %v", err)
+	}
+
+	resp := rec.Msg
+	if len(resp.Answer) != 2 {
+		t.Fatalf("Expected 2 answers, got %d", len(resp.Answer))
+	}
+	if len(resp.Extra) != 2 {
+		t.Errorf("Expected 2 additional records (one A, one AAAA), got %d: %v", len(resp.Extra), resp.Extra)
+	}
+}
+
+const testAdditionalOrigin = "example.org."
+
+const dbAdditionalExample = `
+$TTL 30M
+$ORIGIN example.org.
+@	IN SOA	ns.example.org. admin.example.org. (
+			2024010100 ; serial
+			14400      ; refresh (4 hours)
+			3600       ; retry (1 hour)
+			604800     ; expire (1 week)
+			14400      ; minimum (4 hours)
+			)
+	IN NS	ns.example.org.
+
+ns		IN A	192.0.2.1
+
+; The target shared by the records below.
+host		IN A	192.0.2.10
+		IN AAAA	2001:db8::10
+
+; A second, distinct target.
+other		IN A	192.0.2.20
+		IN AAAA	2001:db8::20
+
+; Two MX records differing only in preference, pointing at one target.
+mx		IN MX	10 host.example.org.
+		IN MX	20 host.example.org.
+
+; Two SRV records pointing at one target.
+srv		IN SRV	10 50 8080 host.example.org.
+		IN SRV	20 50 8080 host.example.org.
+
+; The same target, spelled differently. SRV targets are not normalized on insert.
+mixed		IN SRV	10 50 8080 host.example.org.
+		IN SRV	20 50 8080 HOST.EXAMPLE.ORG.
+
+; Two MX records with distinct targets.
+two		IN MX	10 host.example.org.
+		IN MX	20 other.example.org.
+`