From 4ec856f347be0ab788b5303e942a2986069a8893 Mon Sep 17 00:00:00 2001 From: Ville Vesilehto Date: Sun, 25 Oct 2026 04:45:58 +0200 Subject: [PATCH] Merge commit from fork EndpointSlice FQDN addresses were copied into endpoint records as if they were IP addresses. CoreDNS then treated each value as a CNAME target. A tenant could build a branching chain of headless Services that made one query perform exponential work and exhaust CoreDNS memory. Publish endpoints only from IPv4 and IPv6 slices and validate every address against its declared family. Deduplicate CNAME targets for the whole query so repeated or converging paths are chased once. Protect internal self-lookups with an independent depth limit. This stops direct and indirect k8s_external CNAME loops without consuming the loop counter already maintained by the file and rewrite plugins. Add regression coverage for address types, recursive convergence, repeated k8s_external targets, and direct and indirect self-reference. Signed-off-by: Ville Vesilehto --- plugin/backend_lookup.go | 42 ++++++++ plugin/backend_lookup_test.go | 122 +++++++++++++++++++++ plugin/k8s_external/external_test.go | 125 ++++++++++++++++++++++ plugin/k8s_external/msg_to_dns.go | 26 +++++ plugin/kubernetes/README.md | 9 ++ plugin/kubernetes/controller_test.go | 5 + plugin/kubernetes/object/endpoint.go | 25 +++++ plugin/kubernetes/object/endpoint_test.go | 65 +++++++++++ plugin/pkg/upstream/upstream.go | 28 +++++ plugin/pkg/upstream/upstream_test.go | 101 +++++++++++++++++ 10 files changed, 548 insertions(+) create mode 100644 plugin/kubernetes/object/endpoint_test.go create mode 100644 plugin/pkg/upstream/upstream_test.go diff --git a/plugin/backend_lookup.go b/plugin/backend_lookup.go index c1ad9c952..1ece3b696 100644 --- a/plugin/backend_lookup.go +++ b/plugin/backend_lookup.go @@ -15,6 +15,27 @@ import ( const maxCnameChainLength = 10 +type cnameVisitKey struct{} + +func cnameVisits(ctx context.Context) (context.Context, map[string]struct{}) { + if visited, ok := ctx.Value(cnameVisitKey{}).(map[string]struct{}); ok { + return ctx, visited + } + visited := make(map[string]struct{}) + return context.WithValue(ctx, cnameVisitKey{}, visited), visited +} + +// firstCNAMEVisit reports whether target has not already been chased while +// answering the current client query. +func firstCNAMEVisit(visited map[string]struct{}, target string) bool { + target = dns.CanonicalName(target) + if _, ok := visited[target]; ok { + return false + } + visited[target] = struct{}{} + return true +} + // A returns A records from Backend or an error. func A(ctx context.Context, b ServiceBackend, zone string, state request.Request, previousRecords []dns.RR, opt Options) (records []dns.RR, truncated bool, err error) { services, err := checkForApex(ctx, b, zone, state, opt) @@ -23,6 +44,7 @@ func A(ctx context.Context, b ServiceBackend, zone string, state request.Request } dup := make(map[string]struct{}) + var cnames map[string]struct{} for _, serv := range services { what, ip := serv.HostType() @@ -43,6 +65,12 @@ func A(ctx context.Context, b ServiceBackend, zone string, state request.Request if dnsutil.DuplicateCNAME(newRecord, previousRecords) { continue } + if cnames == nil { + ctx, cnames = cnameVisits(ctx) + } + if !firstCNAMEVisit(cnames, newRecord.Target) { + continue + } if dns.IsSubDomain(zone, dns.Fqdn(serv.Host)) { state1 := state.NewWithQuestion(serv.Host, state.QType()) state1.Zone = zone @@ -96,6 +124,7 @@ func AAAA(ctx context.Context, b ServiceBackend, zone string, state request.Requ } dup := make(map[string]struct{}) + var cnames map[string]struct{} for _, serv := range services { what, ip := serv.HostType() @@ -117,6 +146,12 @@ func AAAA(ctx context.Context, b ServiceBackend, zone string, state request.Requ if dnsutil.DuplicateCNAME(newRecord, previousRecords) { continue } + if cnames == nil { + ctx, cnames = cnameVisits(ctx) + } + if !firstCNAMEVisit(cnames, newRecord.Target) { + continue + } if dns.IsSubDomain(zone, dns.Fqdn(serv.Host)) { state1 := state.NewWithQuestion(serv.Host, state.QType()) state1.Zone = zone @@ -350,6 +385,7 @@ func TXT(ctx context.Context, b ServiceBackend, zone string, state request.Reque } dup := make(map[string]struct{}) + var cnames map[string]struct{} for _, serv := range services { what, _ := serv.HostType() @@ -370,6 +406,12 @@ func TXT(ctx context.Context, b ServiceBackend, zone string, state request.Reque if dnsutil.DuplicateCNAME(newRecord, previousRecords) { continue } + if cnames == nil { + ctx, cnames = cnameVisits(ctx) + } + if !firstCNAMEVisit(cnames, newRecord.Target) { + continue + } if dns.IsSubDomain(zone, dns.Fqdn(serv.Host)) { state1 := state.NewWithQuestion(serv.Host, state.QType()) state1.Zone = zone diff --git a/plugin/backend_lookup_test.go b/plugin/backend_lookup_test.go index a21ad540a..7acb24fea 100644 --- a/plugin/backend_lookup_test.go +++ b/plugin/backend_lookup_test.go @@ -680,3 +680,125 @@ func TestIsDuplicate(t *testing.T) { t.Fatal("expected duplicate on second insert (addr)") } } + +// TestCNAMETargetsChasedOnce checks that a backend returning the same CNAME +// target many times (one per endpoint address, say) causes a single chase of +// that target, both for targets outside the zone (one upstream lookup) and +// for targets inside the zone (one recursive backend lookup). +func TestCNAMETargetsChasedOnce(t *testing.T) { + const fanout = 50 + for _, qtype := range []uint16{dns.TypeA, dns.TypeAAAA, dns.TypeTXT} { + lookups := 0 + backendCalls := map[string]int{} + b := &mockBackend{ + mockServices: func(_ context.Context, state request.Request, _ bool, _ Options) ([]msg.Service, error) { + backendCalls[state.Name()]++ + switch state.Name() { + case "a.example.org.": + var svcs []msg.Service + for i := 0; i < fanout; i++ { + svcs = append(svcs, msg.Service{Host: "b.example.org", Key: "/skydns/org/example/a"}) + svcs = append(svcs, msg.Service{Host: "ext.example.com", Key: "/skydns/org/example/a"}) + } + return svcs, nil + case "b.example.org.": + if qtype == dns.TypeTXT { + return []msg.Service{{Text: "hello", Key: "/skydns/org/example/b"}}, nil + } + return []msg.Service{{Host: "10.0.0.1", Key: "/skydns/org/example/b"}, {Host: "2001:db8::1", Key: "/skydns/org/example/b"}}, nil + } + return nil, errors.New("not found") + }, + mockLookup: func(_ context.Context, _ request.Request, name string, _ uint16) (*dns.Msg, error) { + lookups++ + m := new(dns.Msg) + m.SetQuestion(name, qtype) + return m, nil + }, + } + + m := new(dns.Msg) + m.SetQuestion("a.example.org.", qtype) + state := request.Request{W: &test.ResponseWriter{}, Req: m, Zone: "example.org."} + + var records []dns.RR + var err error + switch qtype { + case dns.TypeA: + records, _, err = A(context.TODO(), b, "example.org.", state, nil, Options{}) + case dns.TypeAAAA: + records, _, err = AAAA(context.TODO(), b, "example.org.", state, nil, Options{}) + case dns.TypeTXT: + records, _, err = TXT(context.TODO(), b, "example.org.", state, nil, Options{}) + } + if err != nil { + t.Fatalf("%s: unexpected error: %v", dns.TypeToString[qtype], err) + } + if lookups != 1 { + t.Errorf("%s: expected 1 upstream lookup for the repeated external target, got %d", dns.TypeToString[qtype], lookups) + } + if backendCalls["b.example.org."] != 1 { + t.Errorf("%s: expected 1 backend lookup for the repeated internal target, got %d", dns.TypeToString[qtype], backendCalls["b.example.org."]) + } + cnames := 0 + for _, rr := range records { + if rr.Header().Rrtype == dns.TypeCNAME { + cnames++ + } + } + // One CNAME to b (followed by its data) and one to ext (whose upstream answer is empty here). + if cnames != 2 { + t.Errorf("%s: expected 2 CNAME records in the answer, got %d: %v", dns.TypeToString[qtype], cnames, records) + } + } +} + +func TestCNAMETargetsChasedOnceAcrossRecursion(t *testing.T) { + for _, qtype := range []uint16{dns.TypeA, dns.TypeAAAA, dns.TypeTXT} { + backendCalls := map[string]int{} + b := &mockBackend{ + mockServices: func(_ context.Context, state request.Request, _ bool, _ Options) ([]msg.Service, error) { + backendCalls[state.Name()]++ + switch state.Name() { + case "a.example.org.": + return []msg.Service{{Host: "b.example.org"}, {Host: "c.example.org"}}, nil + case "b.example.org.", "c.example.org.": + return []msg.Service{{Host: "d.example.org"}}, nil + case "d.example.org.": + switch qtype { + case dns.TypeA: + return []msg.Service{{Host: "10.0.0.1"}}, nil + case dns.TypeAAAA: + return []msg.Service{{Host: "2001:db8::1"}}, nil + default: + return []msg.Service{{Text: "hello"}}, nil + } + } + return nil, errors.New("not found") + }, + mockLookup: func(_ context.Context, _ request.Request, _ string, _ uint16) (*dns.Msg, error) { + return nil, errors.New("unexpected external lookup") + }, + } + + m := new(dns.Msg) + m.SetQuestion("a.example.org.", qtype) + state := request.Request{W: &test.ResponseWriter{}, Req: m, Zone: "example.org."} + + var err error + switch qtype { + case dns.TypeA: + _, _, err = A(context.Background(), b, "example.org.", state, nil, Options{}) + case dns.TypeAAAA: + _, _, err = AAAA(context.Background(), b, "example.org.", state, nil, Options{}) + case dns.TypeTXT: + _, _, err = TXT(context.Background(), b, "example.org.", state, nil, Options{}) + } + if err != nil { + t.Fatalf("%s: unexpected error: %v", dns.TypeToString[qtype], err) + } + if backendCalls["d.example.org."] != 1 { + t.Errorf("%s: expected converged target to be chased once, got %d", dns.TypeToString[qtype], backendCalls["d.example.org."]) + } + } +} diff --git a/plugin/k8s_external/external_test.go b/plugin/k8s_external/external_test.go index e8d000efe..63c17ca1d 100644 --- a/plugin/k8s_external/external_test.go +++ b/plugin/k8s_external/external_test.go @@ -6,6 +6,7 @@ import ( "github.com/coredns/coredns/core/dnsserver" "github.com/coredns/coredns/plugin" + "github.com/coredns/coredns/plugin/etcd/msg" "github.com/coredns/coredns/plugin/kubernetes" "github.com/coredns/coredns/plugin/kubernetes/object" "github.com/coredns/coredns/plugin/pkg/dnstest" @@ -101,6 +102,130 @@ func TestExternalCNAMENilUpstreamResponse(t *testing.T) { } } +func TestExternalCNAMETargetsChasedOnce(t *testing.T) { + lookups := 0 + cfg := &dnsserver.Config{ + Zone: ".", + Plugin: []plugin.Plugin{ + func(plugin.Handler) plugin.Handler { + return plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + lookups++ + m := new(dns.Msg) + m.SetReply(r) + w.WriteMsg(m) + return dns.RcodeSuccess, nil + }) + }, + }, + } + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + + e := New() + e.upstream = upstream.New() + services := make([]msg.Service, 50) + for i := range services { + services[i] = msg.Service{Host: "target.example.org"} + } + + for _, qtype := range []uint16{dns.TypeA, dns.TypeAAAA} { + lookups = 0 + m := new(dns.Msg) + m.SetQuestion("service.example.com.", qtype) + state := request.Request{W: &test.ResponseWriter{}, Req: m} + if qtype == dns.TypeA { + e.a(ctx, services, state) + } else { + e.aaaa(ctx, services, state) + } + if lookups != 1 { + t.Errorf("%s: expected one lookup for a repeated target, got %d", dns.TypeToString[qtype], lookups) + } + } +} + +func TestExternalCNAMESelfLookupIsBounded(t *testing.T) { + e := New() + e.Zones = []string{"example.com."} + e.headless = true + e.upstream = upstream.New() + e.externalSerialFunc = externalSerial + + calls := 0 + e.externalFunc = func(state request.Request, _ bool) ([]msg.Service, int) { + calls++ + return []msg.Service{{Host: state.QName(), TTL: 5}}, dns.RcodeSuccess + } + + cfg := &dnsserver.Config{ + Zone: ".", + Plugin: []plugin.Plugin{ + func(plugin.Handler) plugin.Handler { return e }, + }, + } + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + + m := new(dns.Msg) + m.SetQuestion("service.example.com.", dns.TypeA) + w := dnstest.NewRecorder(&test.ResponseWriter{}) + server.ServeDNS(ctx, w, m) + + if calls != 1 { + t.Fatalf("expected a direct self-reference to stop immediately, got %d calls", calls) + } + if w.Msg == nil { + t.Fatal("expected a response") + } +} + +func TestExternalCNAMEIndirectLoopIsBounded(t *testing.T) { + e := New() + e.Zones = []string{"example.com."} + e.headless = true + e.upstream = upstream.New() + + calls := 0 + e.externalFunc = func(state request.Request, _ bool) ([]msg.Service, int) { + calls++ + target := "service.example.com." + if state.Name() == target { + target = "other.example.com." + } + return []msg.Service{{Host: target, TTL: 5}}, dns.RcodeSuccess + } + + cfg := &dnsserver.Config{ + Zone: ".", + Plugin: []plugin.Plugin{ + func(plugin.Handler) plugin.Handler { return e }, + }, + } + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + + m := new(dns.Msg) + m.SetQuestion("service.example.com.", dns.TypeA) + w := dnstest.NewRecorder(&test.ResponseWriter{}) + server.ServeDNS(ctx, w, m) + + if calls != 11 { + t.Fatalf("expected the initial query plus ten bounded self-lookups, got %d calls", calls) + } + if w.Msg == nil { + t.Fatal("expected a response") + } +} + var tests = []test.Case{ // PTR reverse lookup { diff --git a/plugin/k8s_external/msg_to_dns.go b/plugin/k8s_external/msg_to_dns.go index 2b162386c..aa9c16063 100644 --- a/plugin/k8s_external/msg_to_dns.go +++ b/plugin/k8s_external/msg_to_dns.go @@ -13,12 +13,26 @@ import ( func (e *External) a(ctx context.Context, services []msg.Service, state request.Request) (records []dns.RR, truncated bool) { dup := make(map[string]struct{}) + qname := "" for _, s := range services { what, ip := s.HostType() switch what { case dns.TypeCNAME: + // One chase per distinct target: every lookup re-enters the plugin + // chain, and a headless Service can list the same name many times. + target := dns.CanonicalName(s.Host) + if qname == "" { + qname = dns.CanonicalName(state.Name()) + } + if target == qname { + continue + } + if _, ok := dup[target]; ok { + continue + } + dup[target] = struct{}{} rr := s.NewCNAME(state.QName(), s.Host) records = append(records, rr) if resp, err := e.upstream.Lookup(ctx, state, dns.Fqdn(s.Host), dns.TypeA); err == nil && resp != nil { @@ -45,12 +59,24 @@ func (e *External) a(ctx context.Context, services []msg.Service, state request. func (e *External) aaaa(ctx context.Context, services []msg.Service, state request.Request) (records []dns.RR, truncated bool) { dup := make(map[string]struct{}) + qname := "" for _, s := range services { what, ip := s.HostType() switch what { case dns.TypeCNAME: + target := dns.CanonicalName(s.Host) + if qname == "" { + qname = dns.CanonicalName(state.Name()) + } + if target == qname { + continue + } + if _, ok := dup[target]; ok { + continue + } + dup[target] = struct{}{} rr := s.NewCNAME(state.QName(), s.Host) records = append(records, rr) if resp, err := e.upstream.Lookup(ctx, state, dns.Fqdn(s.Host), dns.TypeAAAA); err == nil && resp != nil { diff --git a/plugin/kubernetes/README.md b/plugin/kubernetes/README.md index 70913ae0b..cee7897ed 100644 --- a/plugin/kubernetes/README.md +++ b/plugin/kubernetes/README.md @@ -232,6 +232,15 @@ Kubernetes API. This plugin creates PTR records for every Pod selected by a Service. If a given Pod is selected by more than one Service a separate PTR record will exist for each Service selecting it. +## EndpointSlice Address Types + +Endpoint records are served from EndpointSlices of address type `IPv4` and `IPv6` only. The +[Kubernetes EndpointSlice API reference](https://kubernetes.io/docs/reference/kubernetes-api/discovery/endpoint-slice-v1/) +documents `FQDN` as deprecated, defines no semantics for it, and states that kube-proxy only processes +`IPv4` and `IPv6` slices. An FQDN slice carries names chosen by whoever wrote it rather than addresses, +and the plugin publishes no records for it. A `CNAME` target for a Service is expressed with a Service +of type `ExternalName`. + ## Examples Handle all queries in the `cluster.local` zone. Connect to Kubernetes in-cluster. Also handle all diff --git a/plugin/kubernetes/controller_test.go b/plugin/kubernetes/controller_test.go index 6b87a8962..ab1085688 100644 --- a/plugin/kubernetes/controller_test.go +++ b/plugin/kubernetes/controller_test.go @@ -145,7 +145,12 @@ func generateEndpointSlices(cidr string, svcType string, client kubernetes.Inter port := int32(80) protocol := api.Protocol("tcp") name := "http" + addressType := discovery.AddressTypeIPv4 + if ip.To4() == nil { + addressType = discovery.AddressTypeIPv6 + } eps := &discovery.EndpointSlice{ + AddressType: addressType, Ports: []discovery.EndpointPort{ { Port: &port, diff --git a/plugin/kubernetes/object/endpoint.go b/plugin/kubernetes/object/endpoint.go index 1cdb306b6..ec0ecb317 100644 --- a/plugin/kubernetes/object/endpoint.go +++ b/plugin/kubernetes/object/endpoint.go @@ -3,6 +3,7 @@ package object import ( "fmt" "maps" + "net/netip" "strings" discovery "k8s.io/api/discovery/v1" @@ -103,11 +104,35 @@ func endpointSliceToEndpoints(obj meta.Object, withZones bool) (meta.Object, err } } + // The Kubernetes DNS specification defines endpoint records for IP + // addresses only. Other address types carry data that must not be treated + // as an address record. In particular, treating an FQDN address as record + // data turns it into a CNAME that CoreDNS chases through its plugin chain. + // Keep the object so the Service still exists, but publish no addresses. + switch ends.AddressType { + case discovery.AddressTypeIPv4, discovery.AddressTypeIPv6: + default: + *ends = discovery.EndpointSlice{} + return e, nil + } + for _, end := range ends.Endpoints { if !endpointsliceReady(end.Conditions.Ready) { continue } for _, a := range end.Addresses { + // The API server validates each address against the slice's address + // type. Retain the check here for non-conforming API implementations. + ip, err := netip.ParseAddr(a) + if err != nil { + continue + } + if ends.AddressType == discovery.AddressTypeIPv4 && !ip.Is4() { + continue + } + if ends.AddressType == discovery.AddressTypeIPv6 && !ip.Is6() { + continue + } ea := EndpointAddress{IP: a} if end.Hostname != nil { ea.Hostname = *end.Hostname diff --git a/plugin/kubernetes/object/endpoint_test.go b/plugin/kubernetes/object/endpoint_test.go new file mode 100644 index 000000000..ce848f55b --- /dev/null +++ b/plugin/kubernetes/object/endpoint_test.go @@ -0,0 +1,65 @@ +package object + +import ( + "testing" + + discovery "k8s.io/api/discovery/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestEndpointSliceToEndpointsAddressTypes(t *testing.T) { + ready := true + tests := []struct { + name string + addrType discovery.AddressType + addresses []string + want []string + }{ + {"ipv4", discovery.AddressTypeIPv4, []string{"172.0.0.1", "172.0.0.2"}, []string{"172.0.0.1", "172.0.0.2"}}, + {"ipv4 slices drop ipv6 addresses", discovery.AddressTypeIPv4, []string{"172.0.0.1", "fd00::1"}, []string{"172.0.0.1"}}, + {"ipv6", discovery.AddressTypeIPv6, []string{"fd00::1"}, []string{"fd00::1"}}, + {"ipv6 slices drop ipv4 addresses", discovery.AddressTypeIPv6, []string{"fd00::1", "172.0.0.1"}, []string{"fd00::1"}}, + {"fqdn slices publish no addresses", discovery.AddressTypeFQDN, []string{"other.ns.svc.cluster.local", "db.example.com"}, nil}, + {"unknown address types publish no addresses", discovery.AddressType("Future"), []string{"172.0.0.1"}, nil}, + {"non-ip address in an ip slice is dropped", discovery.AddressTypeIPv4, []string{"172.0.0.1", "not-an-ip"}, []string{"172.0.0.1"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + slice := &discovery.EndpointSlice{ + ObjectMeta: meta.ObjectMeta{Name: "svc1-abcde", Namespace: "testns", Labels: map[string]string{discovery.LabelServiceName: "svc1"}}, + AddressType: tc.addrType, + Ports: []discovery.EndpointPort{{}}, + } + for _, a := range tc.addresses { + slice.Endpoints = append(slice.Endpoints, discovery.Endpoint{Addresses: []string{a}, Conditions: discovery.EndpointConditions{Ready: &ready}}) + } + for _, convert := range []ToFunc{EndpointSliceToEndpoints, EndpointSliceToEndpointsWithZones} { + obj, err := convert(slice.DeepCopy()) + if err != nil { + t.Fatalf("conversion failed: %v", err) + } + e := obj.(*Endpoints) + if e.Index != EndpointsKey("svc1", "testns") { + t.Errorf("Index = %q, want %q", e.Index, EndpointsKey("svc1", "testns")) + } + var got []string + for _, s := range e.Subsets { + for _, a := range s.Addresses { + got = append(got, a.IP) + } + } + if len(got) != len(tc.want) { + t.Fatalf("addresses = %v, want %v", got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("address %d = %q, want %q", i, got[i], tc.want[i]) + } + } + if len(e.IndexIP) != len(tc.want) { + t.Errorf("IndexIP = %v, want %v", e.IndexIP, tc.want) + } + } + }) + } +} diff --git a/plugin/pkg/upstream/upstream.go b/plugin/pkg/upstream/upstream.go index b531b70b5..7bee2a328 100644 --- a/plugin/pkg/upstream/upstream.go +++ b/plugin/pkg/upstream/upstream.go @@ -3,6 +3,7 @@ package upstream import ( "context" + "errors" "fmt" "github.com/coredns/coredns/core/dnsserver" @@ -18,14 +19,41 @@ type Upstream struct{} // New creates a new Upstream to resolve names using the coredns process. func New() *Upstream { return &Upstream{} } +// maxDepth is the number of nested self-lookups one client query may cause. +// Each Lookup re-enters the server's plugin chain, and a plugin that answers +// with another CNAME re-enters it again. Backends serve data that other +// parties write (a Kubernetes tenant's Service and EndpointSlice, an etcd +// record), so a target that leads back to itself must stop here instead of +// growing the Go stack until the process dies. This matches the generic +// backend's CNAME-chain limit. +const maxDepth = 10 + +// errMaxDepth is returned when a lookup would nest deeper than maxDepth. +var errMaxDepth = errors.New("upstream lookup depth exceeded") + +// depthKey is deliberately separate from dnsserver.LoopKey. The file and +// rewrite plugins already maintain LoopKey around their own CNAME processing; +// sharing it here would count each re-entry twice and shorten valid chains. +type depthKey struct{} + // Lookup routes lookups to our selves to make it follow the plugin chain *again*, but with a (possibly) new query. As // we are doing the query against ourselves again, there is no actual new hop, as such RFC 6891 does not apply and we // need the EDNS0 option present in the *original* query to be present here too. +// +// The nesting depth travels with the context so chains that alternate between +// plugins are bounded as one. Lookup returns errMaxDepth once maxDepth nested +// lookups are in progress for the client query. func (u *Upstream) Lookup(ctx context.Context, state request.Request, name string, typ uint16) (*dns.Msg, error) { server, ok := ctx.Value(dnsserver.Key{}).(*dnsserver.Server) if !ok { return nil, fmt.Errorf("no full server is running") } + depth, _ := ctx.Value(depthKey{}).(int) + if depth >= maxDepth { + return nil, errMaxDepth + } + ctx = context.WithValue(ctx, depthKey{}, depth+1) + req := state.NewWithQuestion(name, typ) nw := nonwriter.New(state.W) diff --git a/plugin/pkg/upstream/upstream_test.go b/plugin/pkg/upstream/upstream_test.go new file mode 100644 index 000000000..b7f690e82 --- /dev/null +++ b/plugin/pkg/upstream/upstream_test.go @@ -0,0 +1,101 @@ +package upstream + +import ( + "context" + "errors" + "testing" + + "github.com/coredns/coredns/core/dnsserver" + "github.com/coredns/coredns/plugin" + "github.com/coredns/coredns/plugin/test" + "github.com/coredns/coredns/request" + + "github.com/miekg/dns" +) + +// selfReferencing answers every query with a CNAME to the query name itself +// and then asks the upstream for that target, the way a backend does for a +// CNAME-typed service whose target leads back to the backend. +type selfReferencing struct { + up *Upstream + calls int +} + +func (s *selfReferencing) Name() string { return "selfref" } + +func (s *selfReferencing) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + s.calls++ + state := request.Request{W: w, Req: r} + m := new(dns.Msg) + m.SetReply(r) + m.Answer = []dns.RR{test.CNAME(state.QName() + " 5 IN CNAME " + state.QName())} + if resp, err := s.up.Lookup(ctx, state, state.QName(), state.QType()); err == nil && resp != nil { + m.Answer = append(m.Answer, resp.Answer...) + } + w.WriteMsg(m) + return dns.RcodeSuccess, nil +} + +func TestLookupDepthIsBounded(t *testing.T) { + h := &selfReferencing{up: New()} + cfg := &dnsserver.Config{Zone: ".", Plugin: []plugin.Plugin{func(plugin.Handler) plugin.Handler { return h }}} + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + m := new(dns.Msg) + m.SetQuestion("x.example.org.", dns.TypeA) + rec := &test.ResponseWriter{} + server.ServeDNS(ctx, rec, m) + + // The client query plus maxDepth nested lookups. + if want := maxDepth + 1; h.calls != want { + t.Fatalf("expected %d invocations of the plugin chain, got %d", want, h.calls) + } +} + +func TestLookupReturnsErrMaxDepth(t *testing.T) { + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{{Zone: "."}}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + ctx = context.WithValue(ctx, depthKey{}, maxDepth) + + m := new(dns.Msg) + m.SetQuestion("x.example.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: m} + if _, err := New().Lookup(ctx, state, "y.example.org.", dns.TypeA); !errors.Is(err, errMaxDepth) { + t.Fatalf("expected errMaxDepth, got %v", err) + } +} + +func TestLookupDoesNotConsumeServerLoopDepth(t *testing.T) { + h := &selfReferencing{up: New()} + cfg := &dnsserver.Config{Zone: ".", Plugin: []plugin.Plugin{func(plugin.Handler) plugin.Handler { return h }}} + server, err := dnsserver.NewServer("dns://:53", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + + ctx := context.WithValue(context.Background(), dnsserver.Key{}, server) + ctx = context.WithValue(ctx, dnsserver.LoopKey{}, maxDepth) + + m := new(dns.Msg) + m.SetQuestion("x.example.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: m} + if _, err := New().Lookup(ctx, state, "y.example.org.", dns.TypeA); err != nil { + t.Fatalf("Lookup unexpectedly used dnsserver.LoopKey: %v", err) + } +} + +func TestLookupWithoutServer(t *testing.T) { + m := new(dns.Msg) + m.SetQuestion("x.example.org.", dns.TypeA) + state := request.Request{W: &test.ResponseWriter{}, Req: m} + if _, err := New().Lookup(context.Background(), state, "y.example.org.", dns.TypeA); err == nil { + t.Fatal("expected an error without a server in the context") + } +}