diff --git a/client/internal/dns/handler_chain.go b/client/internal/dns/handler_chain.go index dc20146eb..c4b8ff938 100644 --- a/client/internal/dns/handler_chain.go +++ b/client/internal/dns/handler_chain.go @@ -3,6 +3,7 @@ package dns import ( "context" "fmt" + "maps" "math" "net" "slices" @@ -31,6 +32,26 @@ type SubdomainMatcher interface { MatchSubdomains() bool } +// responseMeta holds the annotations handlers attach to a request to explain the +// response the chain ends up writing. It survives a deferral, so an answer that +// did not come from the handler that owns the name still says who stepped aside +// and why. +type responseMeta map[resutil.MetaKey]string + +// format renders the annotations for the response log line. The order is stable +// so the same event reads the same way every time; a map's own order is not. +func (m responseMeta) format() string { + if len(m) == 0 { + return "" + } + + var b strings.Builder + for _, k := range slices.Sorted(maps.Keys(m)) { + b.WriteString(" " + string(k) + "=" + m[k]) + } + return b.String() +} + type HandlerEntry struct { Handler dns.Handler Priority int @@ -52,8 +73,19 @@ type ResponseWriterChain struct { origPattern string requestID string shouldContinue bool - response *dns.Msg - meta map[string]string + // softNegative suppresses a poisoning negative verdict for this request. A + // handler that owns the name but cannot answer this query type sets it + // before deferring, and it stays set for every handler that runs after. + softNegative bool + // clientHasEdns records whether the original query advertised EDNS0, taken + // before any handler ran: handlers add EDNS0 to the query they forward + // upstream, so the message itself no longer answers the question later. + clientHasEdns bool + response *dns.Msg + // meta is handed to the next handler when this one defers, so the same map + // outlives the writer that created it. Handlers must only set metadata from + // within ServeDNS, never from a goroutine that outlives the call. + meta responseMeta } // RequestID returns the request ID for tracing @@ -62,26 +94,64 @@ func (w *ResponseWriterChain) RequestID() string { } // SetMeta sets a metadata key-value pair for logging -func (w *ResponseWriterChain) SetMeta(key, value string) { +func (w *ResponseWriterChain) SetMeta(key resutil.MetaKey, value string) { if w.meta == nil { - w.meta = make(map[string]string) + w.meta = make(responseMeta) } w.meta[key] = value } +// RequestSoftNegative marks the request so a downstream NXDOMAIN is turned into +// NODATA before it reaches the client. Set by a handler that defers a query for +// a name it owns but a type it cannot resolve. +func (w *ResponseWriterChain) RequestSoftNegative() { + w.softNegative = true +} + func (w *ResponseWriterChain) WriteMsg(m *dns.Msg) error { // Check if this is a continue signal (NXDOMAIN with Zero bit set) if m.Rcode == dns.RcodeNameError && m.MsgHdr.Zero { w.shouldContinue = true return nil } + if w.softNegative && m.Rcode == dns.RcodeNameError { + m = softenNegative(m, w.clientHasEdns) + w.SetMeta(resutil.MetaKeySoftened, "nxdomain->nodata") + } w.response = m if m.MsgHdr.Truncated { - w.SetMeta("truncated", "true") + w.SetMeta(resutil.MetaKeyTruncated, "true") } return w.ResponseWriter.WriteMsg(m) } +// softenNegative downgrades an NXDOMAIN to NODATA for a request a handler that +// owns the name deferred. NXDOMAIN is cached for the name and every type below +// it, so a resolver that has never heard of a routed name would take out the +// record types the route does serve; NODATA is cached for this name and type +// only. The authority section goes with it: the negative TTL of a zone we just +// overruled does not apply, and RFC 2308 keeps a negative answer that carries no +// SOA out of caches altogether, so the rewrite cannot outlive the route. +// +// The rewrite is ours, not the answering resolver's, so an EDNS0 client is told +// as much: the reply we hand back travels a path no capture on this host sees, +// and an empty answer is otherwise indistinguishable from a real one. Returns a +// copy so the answering handler keeps whatever it may hold on to. +func softenNegative(m *dns.Msg, clientHasEdns bool) *dns.Msg { + out := m.Copy() + out.Rcode = dns.RcodeSuccess + out.Ns = nil + + if clientHasEdns { + resutil.AttachEDE(out, resutil.EDENetbirdSoftenedNegative, + "netbird: name is served locally, NXDOMAIN from the fallthrough resolver suppressed") + } else { + resutil.StripOPT(out) + } + + return out +} + func NewHandlerChain() *HandlerChain { return &HandlerChain{ handlers: make([]HandlerEntry, 0), @@ -223,6 +293,19 @@ func (c *HandlerChain) dispatch(w dns.ResponseWriter, r *dns.Msg, maxPriority in handlers := slices.Clone(c.handlers) c.mu.RUnlock() + // Carried across deferrals: once a handler that owns the name defers, no + // handler after it may answer with a verdict that poisons the name. The + // metadata of a handler that stepped aside is carried too, so the response + // log line explains an answer that did not come from the handler that owns + // the name. + var softNegative bool + var carried responseMeta + + // Taken before any handler runs: handlers advertise EDNS0 on the query they + // forward upstream, so afterwards the message no longer tells us whether the + // client did. + clientHasEdns := r.IsEdns0() != nil + // Try handlers in priority order for _, entry := range handlers { if entry.Priority > maxPriority { @@ -245,11 +328,16 @@ func (c *HandlerChain) dispatch(w dns.ResponseWriter, r *dns.Msg, maxPriority in ResponseWriter: w, origPattern: entry.OrigPattern, requestID: requestID, + softNegative: softNegative, + clientHasEdns: clientHasEdns, + meta: carried, } entry.Handler.ServeDNS(chainWriter, r) // If handler wants to continue, try next handler if chainWriter.shouldContinue { + softNegative = softNegative || chainWriter.softNegative + carried = chainWriter.meta if entry.Priority != PriorityMgmtCache { logger.Tracef("handler requested continue for domain=%s", qname) } @@ -265,30 +353,56 @@ func (c *HandlerChain) dispatch(w dns.ResponseWriter, r *dns.Msg, maxPriority in qname, dns.TypeToString[question.Qtype], dns.ClassToString[question.Qclass]) resp := &dns.Msg{} resp.SetRcode(r, dns.RcodeRefused) + // A handler that owns the name deferred and nothing below it could answer + // (a client with no primary nameserver group). The name exists as far as + // this client is concerned, since the route serves its addresses, so REFUSED + // would contradict the route: a stub that takes it as "not served here" and + // retries another resolver can come back with an NXDOMAIN that takes the + // whole name down. Answer "no records of this type" instead, without an SOA, + // so RFC 2308 keeps it out of negative caches, and tell an EDNS0 client the + // empty answer is ours rather than a resolver's. + if softNegative { + resp.Rcode = dns.RcodeSuccess + if clientHasEdns { + resutil.AttachEDE(resp, resutil.EDENetbirdSoftenedNegative, + "netbird: name is served locally, no fallthrough resolver for this query type") + } + logger.Tracef("no handler below the deferring one for domain=%s type=%s, answering NODATA", + qname, dns.TypeToString[question.Qtype]) + } if err := w.WriteMsg(resp); err != nil { logger.Errorf("failed to write DNS response: %v", err) } } func (c *HandlerChain) logResponse(logger *log.Entry, cw *ResponseWriterChain, qname string, startTime time.Time) { + // Runs for every query, and the arguments below are not free: Len() packs + // the message to measure it, and formatting the answers and the metadata + // allocates. None of it is worth doing when the line is discarded. + if !log.IsLevelEnabled(log.TraceLevel) { + return + } + if cw.response == nil { return } - var meta string - for k, v := range cw.meta { - meta += " " + k + "=" + v - } - logger.Tracef("response: domain=%s rcode=%s answers=%s size=%dB%s took=%s", qname, dns.RcodeToString[cw.response.Rcode], resutil.FormatAnswers(cw.response.Answer), - cw.response.Len(), meta, time.Since(startTime)) + cw.response.Len(), cw.meta.format(), time.Since(startTime)) } // ResolveInternal runs an in-process DNS query against the chain, skipping any // handler with priority > maxPriority. Used by internal callers (e.g. the mgmt -// cache refresher) that must bypass themselves to avoid loops. Honors ctx -// cancellation; on ctx.Done the dispatch goroutine is left to drain on its own +// cache refresher) that must bypass themselves to avoid loops. +// +// "Nothing answered" is read off RcodeRefused, which a request soft-negatived by +// a deferring handler never carries: it ends in an empty NOERROR instead, and +// would look resolved. No caller can reach that today, since every handler that +// defers sits above the maxPriority any caller passes. Lowering one below it +// means this check needs the soft-negative case too. +// +// Honors ctx cancellation; on ctx.Done the dispatch goroutine is left to drain on its own // (bounded by the invoked handler's internal timeout). func (c *HandlerChain) ResolveInternal(ctx context.Context, r *dns.Msg, maxPriority int) (*dns.Msg, error) { if len(r.Question) == 0 { diff --git a/client/internal/dns/handler_chain_test.go b/client/internal/dns/handler_chain_test.go index b3db97ba3..7bb1d6d9b 100644 --- a/client/internal/dns/handler_chain_test.go +++ b/client/internal/dns/handler_chain_test.go @@ -3,15 +3,19 @@ package dns_test import ( "context" "net" + "strings" "testing" "time" "github.com/miekg/dns" + log "github.com/sirupsen/logrus" + logtest "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" nbdns "github.com/netbirdio/netbird/client/internal/dns" + "github.com/netbirdio/netbird/client/internal/dns/resutil" "github.com/netbirdio/netbird/client/internal/dns/test" ) @@ -1238,6 +1242,274 @@ func TestHandlerChain_ResolveInternal_HonorsContextTimeout(t *testing.T) { assert.Less(t, elapsed, 500*time.Millisecond, "ResolveInternal must return shortly after ctx deadline") } +// requestSoftNegative asks the chain to soften a negative verdict produced by +// the handlers that run after the caller defers, reporting whether the chain +// supports the signal. Written as a type assertion so these tests compile +// against a chain that does not support it yet. +func requestSoftNegative(w dns.ResponseWriter) bool { + sn, ok := w.(interface{ RequestSoftNegative() }) + if ok { + sn.RequestSoftNegative() + } + return ok +} + +// deferringHandler defers to the next handler in the chain, optionally asking +// for the negative verdict of whatever answers instead to be softened. This is +// what a DNS route handler does for a record type its routing peer cannot +// resolve. +type deferringHandler struct { + softNegative bool + called bool + supported bool +} + +func (h *deferringHandler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { + h.called = true + if h.softNegative { + h.supported = requestSoftNegative(w) + // The real handler records why it stepped aside, for the response log + // line of whichever handler answers instead. + resutil.SetMeta(w, resutil.MetaKeyDeferredBy, "test handler") + } + resp := new(dns.Msg) + resp.SetRcode(r, dns.RcodeNameError) + resp.MsgHdr.Zero = true + _ = w.WriteMsg(resp) +} + +// nxdomainHandler answers an authoritative NXDOMAIN with an SOA in the +// authority section, the way a public resolver answers for a name that only +// exists inside the routed network. +type nxdomainHandler struct{} + +func (h *nxdomainHandler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { + resp := new(dns.Msg) + resp.SetRcode(r, dns.RcodeNameError) + resp.Ns = []dns.RR{&dns.SOA{ + Hdr: dns.RR_Header{Name: "example.com.", Rrtype: dns.TypeSOA, Class: dns.ClassINET, Ttl: 3600}, + Ns: "ns1.example.com.", + Mbox: "hostmaster.example.com.", + Minttl: 3600, + Expire: 604800, + Refresh: 7200, + Retry: 3600, + }} + _ = w.WriteMsg(resp) +} + +// TestHandlerChain_SoftNegative_DowngradesDownstreamNXDOMAIN is the whole point +// of the soft-negative signal: a route handler may only defer a query to the +// public chain if the answer cannot poison the routed name. NXDOMAIN is cached +// for the name and every type under it (RFC 2308, RFC 8020), so it has to be +// rewritten to NODATA, which is cached per name and type only. +func TestHandlerChain_SoftNegative_DowngradesDownstreamNXDOMAIN(t *testing.T) { + chain := nbdns.NewHandlerChain() + + route := &deferringHandler{softNegative: true} + chain.AddHandler("*.example.com.", route, nbdns.PriorityDNSRoute) + chain.AddHandler(".", &nxdomainHandler{}, nbdns.PriorityDefault) + + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + mw := &test.MockResponseWriter{} + chain.ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp, "a response must reach the client") + assert.True(t, route.called, "the route handler must run first") + require.True(t, route.supported, "the chain writer must accept a soft-negative request") + assert.Equal(t, dns.RcodeSuccess, resp.Rcode, "NXDOMAIN must be softened to NODATA") + assert.Empty(t, resp.Answer, "a softened negative carries no answer") + assert.Empty(t, resp.Ns, "the downstream zone's SOA must not set the negative TTL for a name we overrode") +} + +// responseLineFor returns the chain's response log line for one query. Raising +// the level to trace also unmutes whatever else is logging in this package, so +// the line has to be picked by the name it was asked about rather than by being +// the last one seen. +func responseLineFor(hook *logtest.Hook, qname string) string { + for _, e := range hook.AllEntries() { + if strings.HasPrefix(e.Message, "response:") && strings.Contains(e.Message, qname) { + return e.Message + } + } + return "" +} + +// TestHandlerChain_SoftNegative_IsVisibleToTheClient covers observability of the +// rewrite. The reply we hand the application travels over loopback, which the +// bundle capture does not see, so a softened verdict has to say so on the wire: +// without it an empty answer is indistinguishable from a real "no such record" +// in a dig output or a capture taken next to the application. +func TestHandlerChain_SoftNegative_IsVisibleToTheClient(t *testing.T) { + newChain := func() *nbdns.HandlerChain { + chain := nbdns.NewHandlerChain() + chain.AddHandler("*.example.com.", &deferringHandler{softNegative: true}, nbdns.PriorityDNSRoute) + chain.AddHandler(".", &nxdomainHandler{}, nbdns.PriorityDefault) + return chain + } + + t.Run("EDNS0 client gets an extended error", func(t *testing.T) { + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + r.SetEdns0(dns.DefaultMsgSize, false) + + mw := &test.MockResponseWriter{} + newChain().ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp) + require.Equal(t, dns.RcodeSuccess, resp.Rcode) + + ede, ok := resutil.ExtractEDE(resp) + require.True(t, ok, "a softened verdict must carry an extended DNS error") + assert.Equal(t, resutil.EDENetbirdSoftenedNegative, ede.InfoCode) + assert.Contains(t, ede.ExtraText, "netbird", "the text must name us as the source of the rewrite") + }) + + t.Run("plain client gets no OPT", func(t *testing.T) { + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + mw := &test.MockResponseWriter{} + newChain().ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp) + assert.Nil(t, resp.IsEdns0(), "RFC 6891 forbids an OPT toward a client that did not advertise EDNS0") + }) + + // The response line is what support reads out of a debug bundle, and it now + // carries several annotations at once. Built from a map, their order would + // differ on every query, so the same event never looks the same twice. + t.Run("log fields keep a stable order", func(t *testing.T) { + const qname = "_mongodb._tcp.stable.example.com." + + hook := logtest.NewGlobal() + t.Cleanup(hook.Reset) + + prev := log.GetLevel() + log.SetLevel(log.TraceLevel) + t.Cleanup(func() { log.SetLevel(prev) }) + + lineFor := func() string { + hook.Reset() + + r := new(dns.Msg) + r.SetQuestion(qname, dns.TypeSRV) + + mw := &test.MockResponseWriter{} + newChain().ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + line := responseLineFor(hook, qname) + // The duration differs per query and is not what we compare. + line, _, _ = strings.Cut(line, " took=") + return line + } + + first := lineFor() + require.NotEmpty(t, first, "the chain must log the response it wrote") + require.Contains(t, first, "deferred_by=", "the line must carry more than one annotation to be worth ordering") + require.Contains(t, first, "softened=") + + for range 20 { + assert.Equal(t, first, lineFor(), "the same event must produce the same line") + } + }) + + t.Run("logged with the reason it was deferred", func(t *testing.T) { + const qname = "_mongodb._tcp.reason.example.com." + + hook := logtest.NewGlobal() + t.Cleanup(hook.Reset) + + prev := log.GetLevel() + log.SetLevel(log.TraceLevel) + t.Cleanup(func() { log.SetLevel(prev) }) + + r := new(dns.Msg) + r.SetQuestion(qname, dns.TypeSRV) + + mw := &test.MockResponseWriter{} + newChain().ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + response := responseLineFor(hook, qname) + require.NotEmpty(t, response, "the chain must log the response it wrote") + assert.Contains(t, response, "softened=", "the log must show the verdict was rewritten") + assert.Contains(t, response, "deferred_by=", "the log must name the handler that deferred") + }) +} + +// TestHandlerChain_SoftNegative_NoHandlerBelow covers a client with no primary +// nameserver group: the deferred query reaches the end of the chain unanswered. +// REFUSED would say the name is not served here while the route serves its +// addresses, and a stub that acts on that by asking elsewhere can bring back an +// NXDOMAIN for the whole name. The answer must be an empty, uncacheable NODATA. +func TestHandlerChain_SoftNegative_NoHandlerBelow(t *testing.T) { + chain := nbdns.NewHandlerChain() + + route := &deferringHandler{softNegative: true} + chain.AddHandler("*.example.com.", route, nbdns.PriorityDNSRoute) + + r := new(dns.Msg) + r.SetQuestion("db.example.com.", dns.TypeHTTPS) + + mw := &test.MockResponseWriter{} + chain.ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp, "a response must reach the client") + assert.Equal(t, dns.RcodeSuccess, resp.Rcode, "an unanswered soft-negative query must be NODATA, not REFUSED") + assert.Empty(t, resp.Answer) + assert.Empty(t, resp.Ns, + "no SOA, so RFC 2308 keeps the empty answer out of negative caches and it cannot outlive the route") +} + +// TestHandlerChain_SoftNegative_KeepsRealAnswers guards the other direction: +// softening applies to negative verdicts only. A real answer from a downstream +// handler must reach the client untouched. +func TestHandlerChain_SoftNegative_KeepsRealAnswers(t *testing.T) { + chain := nbdns.NewHandlerChain() + + route := &deferringHandler{softNegative: true} + chain.AddHandler("*.example.com.", route, nbdns.PriorityDNSRoute) + chain.AddHandler(".", &answeringHandler{name: "public", ip: "203.0.113.10"}, nbdns.PriorityDefault) + + r := new(dns.Msg) + r.SetQuestion("db.example.com.", dns.TypeA) + + mw := &test.MockResponseWriter{} + chain.ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp, "a response must reach the client") + assert.Equal(t, dns.RcodeSuccess, resp.Rcode) + require.Len(t, resp.Answer, 1, "the downstream answer must pass through") +} + +// TestHandlerChain_NXDOMAINPreservedWithoutSoftNegative makes sure the +// softening is opt-in: an ordinary chain continuation still yields NXDOMAIN, so +// genuine non-existence keeps being reported. +func TestHandlerChain_NXDOMAINPreservedWithoutSoftNegative(t *testing.T) { + chain := nbdns.NewHandlerChain() + + route := &deferringHandler{} + chain.AddHandler("*.example.com.", route, nbdns.PriorityDNSRoute) + chain.AddHandler(".", &nxdomainHandler{}, nbdns.PriorityDefault) + + r := new(dns.Msg) + r.SetQuestion("nope.example.com.", dns.TypeA) + + mw := &test.MockResponseWriter{} + chain.ServeDNS(&nbdns.ResponseWriterChain{ResponseWriter: mw}, r) + + resp := mw.GetLastResponse() + require.NotNil(t, resp, "a response must reach the client") + assert.Equal(t, dns.RcodeNameError, resp.Rcode, "without the signal a real NXDOMAIN must survive") +} + func TestHandlerChain_HasRootHandlerAtOrBelow(t *testing.T) { chain := nbdns.NewHandlerChain() h := &answeringHandler{name: "h", ip: "10.0.0.1"} diff --git a/client/internal/dns/resutil/resolve.go b/client/internal/dns/resutil/resolve.go index 931938755..c58c460ac 100644 --- a/client/internal/dns/resutil/resolve.go +++ b/client/internal/dns/resutil/resolve.go @@ -19,6 +19,34 @@ import ( // uses when a resolved host has no addresses of the requested family. const errNoSuitableAddress = "no suitable address found" +// Extended DNS Error info codes NetBird emits so a client can see why an answer +// looks the way it does without reading this peer's logs. They live in the RFC +// 8914 Private Use range (49152-65535) and are registered here, in one place, +// because nothing else guarantees two NetBird components pick distinct codes. +const ( + // EDENetbirdUpstreamTimeout: a DNS forwarder's upstream did not answer. + EDENetbirdUpstreamTimeout uint16 = 49152 + // EDENetbirdUpstreamFailure: a DNS forwarder's upstream failed. + EDENetbirdUpstreamFailure uint16 = 49153 + // EDENetbirdSoftenedNegative: the empty answer is ours, not the answering + // resolver's. A handler that owns the name deferred this query type, and the + // negative verdict that came back was downgraded so it cannot poison the + // name. + EDENetbirdSoftenedNegative uint16 = 49154 +) + +// AttachEDE adds an Extended DNS Error (RFC 8914) option to a message, creating +// the OPT pseudo-record if it has none. Callers must only use it toward a client +// that advertised EDNS0: per RFC 6891 an OPT must not appear otherwise. +func AttachEDE(msg *dns.Msg, code uint16, text string) { + opt := msg.IsEdns0() + if opt == nil { + msg.SetEdns0(dns.DefaultMsgSize, false) + opt = msg.IsEdns0() + } + opt.Option = append(opt.Option, &dns.EDNS0_EDE{InfoCode: code, ExtraText: text}) +} + // GenerateRequestID creates a random 8-character hex string for request tracing. func GenerateRequestID() string { bytes := make([]byte, 4) @@ -78,10 +106,38 @@ type resolver interface { LookupNetIP(ctx context.Context, network, host string) ([]netip.Addr, error) } +// MetaKey names an annotation a handler attaches to a request to explain the +// response the chain ends up writing. The set is closed and rendered onto a +// single log line, so an unrecognized key is a typo inventing a field rather +// than a new fact. +type MetaKey string + +const ( + // MetaKeyProtocol: the transport a query arrived on. + MetaKeyProtocol MetaKey = "protocol" + // MetaKeyUpstream: the upstream that answered. + MetaKeyUpstream MetaKey = "upstream" + // MetaKeyUpstreamProtocol: the transport used toward that upstream. + MetaKeyUpstreamProtocol MetaKey = "upstream_protocol" + // MetaKeyPeer: the routing peer whose forwarder answered. + MetaKeyPeer MetaKey = "peer" + // MetaKeyEDE: an Extended DNS Error carried by the answer. + MetaKeyEDE MetaKey = "ede" + // MetaKeyTruncated: the answer did not fit and was truncated. + MetaKeyTruncated MetaKey = "truncated" + // MetaKeySoftened: a negative verdict was rewritten so it cannot poison a + // name served locally. + MetaKeySoftened MetaKey = "softened" + // MetaKeyDeferredBy: the handler that owned the name and stepped aside. + MetaKeyDeferredBy MetaKey = "deferred_by" + // MetaKeyDeferredReason: why it stepped aside. + MetaKeyDeferredReason MetaKey = "deferred_reason" +) + // chainedWriter is implemented by ResponseWriters that carry request metadata type chainedWriter interface { RequestID() string - SetMeta(key, value string) + SetMeta(key MetaKey, value string) } // GetRequestID extracts a request ID from the ResponseWriter if available, @@ -96,12 +152,30 @@ func GetRequestID(w dns.ResponseWriter) string { } // SetMeta sets metadata on the ResponseWriter if it supports it. -func SetMeta(w dns.ResponseWriter, key, value string) { +func SetMeta(w dns.ResponseWriter, key MetaKey, value string) { if cw, ok := w.(chainedWriter); ok { cw.SetMeta(key, value) } } +// softNegativeRequester is implemented by chain writers that can soften the +// negative verdict of the handlers a deferring handler falls through to. +type softNegativeRequester interface { + RequestSoftNegative() +} + +// RequestSoftNegative asks the handler chain to downgrade an NXDOMAIN from the +// handlers that run after the caller defers. A handler that owns a name but +// cannot answer one query type for it needs this: the resolvers it falls +// through to cannot prove the name absent, and an NXDOMAIN from them is cached +// for the name and every type under it (RFC 2308, RFC 8020), taking the +// addresses the handler does serve down with it. +func RequestSoftNegative(w dns.ResponseWriter) { + if sn, ok := w.(softNegativeRequester); ok { + sn.RequestSoftNegative() + } +} + // LookupResult contains the result of an external DNS lookup type LookupResult struct { IPs []netip.Addr @@ -199,11 +273,27 @@ type RecordResolver interface { LookupAddr(ctx context.Context, addr string) ([]string, error) } +// SupportedRecordQtype reports whether LookupRecords can resolve qtype. The +// set is bounded by the net.Resolver API, which exposes no way to query an +// arbitrary record type, and going around it with a raw exchange would mean +// picking nameservers ourselves instead of resolving the way the host does. +// +// Both ends read this: a DNS forwarder answers these types for the domains it +// routes, and a client uses it to tell which types are worth forwarding to a +// peer at all. +func SupportedRecordQtype(qtype uint16) bool { + switch qtype { + case dns.TypeMX, dns.TypeTXT, dns.TypeNS, dns.TypeSRV, dns.TypeCNAME, dns.TypePTR: + return true + default: + return false + } +} + // LookupRecords resolves a non-address DNS record type through the host -// resolver and returns the resource records and the DNS rcode. Types the host -// resolver cannot answer (anything not covered by the net.Resolver Lookup* -// methods) yield NODATA so that a routed name is never poisoned with NXDOMAIN -// for an unsupported type. +// resolver and returns the resource records and the DNS rcode. Types outside +// SupportedRecordQtype yield NODATA so that a routed name is never poisoned +// with NXDOMAIN for a type we cannot look up. func LookupRecords(ctx context.Context, r RecordResolver, name string, qtype uint16, ttl uint32) ([]dns.RR, int) { fqdn := dns.Fqdn(name) diff --git a/client/internal/dns/upstream.go b/client/internal/dns/upstream.go index 72fc0450c..28aed687d 100644 --- a/client/internal/dns/upstream.go +++ b/client/internal/dns/upstream.go @@ -295,7 +295,7 @@ func (u *upstreamResolverBase) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { if addr := w.RemoteAddr(); addr != nil { network := addr.Network() ctx = contextWithDNSProtocol(ctx, network) - resutil.SetMeta(w, "protocol", network) + resutil.SetMeta(w, resutil.MetaKeyProtocol, network) } ok, failures := u.tryUpstreamServers(ctx, w, r, logger) @@ -331,7 +331,7 @@ func (u *upstreamResolverBase) tryOnlyRace(ctx context.Context, w dns.ResponseWr return false, res.failures } if res.ede != "" { - resutil.SetMeta(w, "ede", res.ede) + resutil.SetMeta(w, resutil.MetaKeyEDE, res.ede) } u.writeSuccessResponse(w, res.msg, res.upstream, r.Question[0].Name, res.protocol, logger) return true, res.failures @@ -361,7 +361,7 @@ func (u *upstreamResolverBase) raceAll(ctx context.Context, w dns.ResponseWriter failures = append(failures, res.failures...) if res.msg != nil { if res.ede != "" { - resutil.SetMeta(w, "ede", res.ede) + resutil.SetMeta(w, resutil.MetaKeyEDE, res.ede) } u.writeSuccessResponse(w, res.msg, res.upstream, r.Question[0].Name, res.protocol, logger) return true, failures @@ -550,9 +550,9 @@ func (u *upstreamResolverBase) debugUpstreamTimeout(upstream netip.AddrPort) str } func (u *upstreamResolverBase) writeSuccessResponse(w dns.ResponseWriter, rm *dns.Msg, upstream netip.AddrPort, domain string, proto string, logger *log.Entry) { - resutil.SetMeta(w, "upstream", upstream.String()) + resutil.SetMeta(w, resutil.MetaKeyUpstream, upstream.String()) if proto != "" { - resutil.SetMeta(w, "upstream_protocol", proto) + resutil.SetMeta(w, resutil.MetaKeyUpstreamProtocol, proto) } // Clear Zero bit from external responses to prevent upstream servers from diff --git a/client/internal/dnsfwd/forwarder.go b/client/internal/dnsfwd/forwarder.go index b7e5a10e3..558de6453 100644 --- a/client/internal/dnsfwd/forwarder.go +++ b/client/internal/dnsfwd/forwarder.go @@ -26,15 +26,6 @@ import ( const errResolveFailed = "failed to resolve query for domain=%s: %v" const upstreamTimeout = 15 * time.Second -// EDE info codes the forwarder emits on upstream failures so the querying -// client can see the reason without inspecting this peer's logs. They live in -// the RFC 8914 Private Use range (49152-65535); the Go resolver never exposes a -// real upstream EDE here, so these cannot collide with a genuine code. -const ( - edeNetbirdUpstreamTimeout uint16 = 49152 - edeNetbirdUpstreamFailure uint16 = 49153 -) - type resolver interface { LookupNetIP(ctx context.Context, network, host string) ([]netip.Addr, error) LookupMX(ctx context.Context, name string) ([]*net.MX, error) @@ -216,6 +207,9 @@ func (f *DNSForwarder) handleDNSQuery(logger *log.Entry, w dns.ResponseWriter, q qname, dns.TypeToString[question.Qtype], dns.ClassToString[question.Qclass]) resp := query.SetReply(query) + // Every answer here comes from a recursive lookup on this peer. SetReply + // leaves RA unset, which reads to a client as a server that cannot recurse. + resp.RecursionAvailable = true mostSpecificResId, matchingEntries := f.getMatchingEntries(strings.TrimSuffix(qname, ".")) if mostSpecificResId == "" { @@ -229,20 +223,22 @@ func (f *DNSForwarder) handleDNSQuery(logger *log.Entry, w dns.ResponseWriter, q reqHasEdns := query.IsEdns0() != nil - switch question.Qtype { - case dns.TypeA, dns.TypeAAAA: + switch { + case question.Qtype == dns.TypeA || question.Qtype == dns.TypeAAAA: f.handleAddressQuery(ctx, logger, w, resp, mostSpecificResId, matchingEntries, reqHasEdns, startTime) - case dns.TypeMX, dns.TypeTXT, dns.TypeNS, dns.TypeSRV, dns.TypeCNAME, dns.TypePTR: + case resutil.SupportedRecordQtype(question.Qtype): f.handleRecordQuery(ctx, logger, w, resp, startTime) default: // The domain is routed here, so any other type is answered NODATA // (NOERROR, empty answer) rather than falling back to a resolver that // would poison the name with NXDOMAIN. The Extended DNS Error lets a // client tell this capability-driven NODATA apart from an - // authoritative one. The OPT pseudo-record must not appear unless the - // query advertised EDNS0. + // authoritative one; a current client knows the type is unsupported and + // resolves it through its own handler chain instead of asking at all. + // The OPT pseudo-record must not appear unless the query advertised + // EDNS0. if reqHasEdns { - attachEDE(resp, dns.ExtendedErrorCodeNotSupported, "netbird forwarder: unsupported query type") + resutil.AttachEDE(resp, dns.ExtendedErrorCodeNotSupported, "netbird forwarder: unsupported query type") } f.writeResponse(logger, w, resp, qname, startTime) } @@ -441,7 +437,7 @@ func (f *DNSForwarder) handleDNSError( } if reqHasEdns { - attachEDE(resp, edeCodeFor(dnsErr), edeText(dnsErr)) + resutil.AttachEDE(resp, edeCodeFor(dnsErr), edeText(dnsErr)) } f.writeResponse(logger, w, resp, domain, startTime) @@ -488,9 +484,9 @@ func (f *DNSForwarder) getMatchingEntries(domain string) (route.ResID, []*Forwar // edeCodeFor maps an upstream lookup error to the NetBird EDE info code. func edeCodeFor(dnsErr *net.DNSError) uint16 { if dnsErr != nil && dnsErr.IsTimeout { - return edeNetbirdUpstreamTimeout + return resutil.EDENetbirdUpstreamTimeout } - return edeNetbirdUpstreamFailure + return resutil.EDENetbirdUpstreamFailure } // edeText builds the EDE extra-text describing the class of upstream failure. @@ -503,14 +499,3 @@ func edeText(dnsErr *net.DNSError) string { } return "netbird forwarder: upstream failure" } - -// attachEDE adds an Extended DNS Error (RFC 8914) option to the response, -// creating the OPT pseudo-record if the response does not already carry one. -func attachEDE(resp *dns.Msg, code uint16, text string) { - opt := resp.IsEdns0() - if opt == nil { - resp.SetEdns0(dns.DefaultMsgSize, false) - opt = resp.IsEdns0() - } - opt.Option = append(opt.Option, &dns.EDNS0_EDE{InfoCode: code, ExtraText: text}) -} diff --git a/client/internal/dnsfwd/forwarder_test.go b/client/internal/dnsfwd/forwarder_test.go index c69a9166e..53d2a3c53 100644 --- a/client/internal/dnsfwd/forwarder_test.go +++ b/client/internal/dnsfwd/forwarder_test.go @@ -649,6 +649,39 @@ func TestDNSForwarder_ResponseCodes(t *testing.T) { } } +// TestDNSForwarder_RecursionAvailable covers the RA bit. Every answer this +// forwarder produces comes from a recursive lookup on the routing peer, but +// dns.Msg.SetReply leaves RA unset, so clients report "recursion not available" +// and some stub resolvers treat the server as unable to serve the query. +func TestDNSForwarder_RecursionAvailable(t *testing.T) { + t.Run("record answer", func(t *testing.T) { + mockResolver := &MockResolver{} + forwarder := newRecordTestForwarder(t, mockResolver, "example.com") + + mockResolver.On("LookupMX", mock.Anything, "example.com."). + Return([]*net.MX{{Host: "mail.example.com.", Pref: 10}}, nil).Once() + + resp := runRecordQuery(t, forwarder, "example.com", dns.TypeMX) + assert.True(t, resp.RecursionAvailable, "the forwarder resolves recursively") + }) + + t.Run("unsupported type NODATA", func(t *testing.T) { + forwarder := newRecordTestForwarder(t, &MockResolver{}, "example.com") + + resp := runRecordQuery(t, forwarder, "example.com", dns.TypeCAA) + require.Equal(t, dns.RcodeSuccess, resp.Rcode) + assert.True(t, resp.RecursionAvailable, "RA describes the server, not the query type") + }) + + t.Run("unauthorized domain", func(t *testing.T) { + forwarder := newRecordTestForwarder(t, &MockResolver{}, "example.com") + + resp := runRecordQuery(t, forwarder, "other.com", dns.TypeMX) + require.Equal(t, dns.RcodeRefused, resp.Rcode) + assert.True(t, resp.RecursionAvailable, "RA describes the server, not the verdict") + }) +} + func hasEDE(m *dns.Msg, code uint16) bool { opt := m.IsEdns0() if opt == nil { @@ -859,7 +892,7 @@ func TestDNSForwarder_UpstreamFailureEDE(t *testing.T) { lookupErr: &net.DNSError{Err: "i/o timeout", Server: "10.0.0.53:53", IsTimeout: true}, reqEdns: true, wantEDE: true, - wantCode: edeNetbirdUpstreamTimeout, + wantCode: resutil.EDENetbirdUpstreamTimeout, wantTextHas: "netbird forwarder: upstream timeout", }, { @@ -867,7 +900,7 @@ func TestDNSForwarder_UpstreamFailureEDE(t *testing.T) { lookupErr: &net.DNSError{Err: "server misbehaving", Server: "10.0.0.53:53"}, reqEdns: true, wantEDE: true, - wantCode: edeNetbirdUpstreamFailure, + wantCode: resutil.EDENetbirdUpstreamFailure, wantTextHas: "netbird forwarder: upstream failure", }, { diff --git a/client/internal/routemanager/dnsinterceptor/handler.go b/client/internal/routemanager/dnsinterceptor/handler.go index b784cc274..ade7a914f 100644 --- a/client/internal/routemanager/dnsinterceptor/handler.go +++ b/client/internal/routemanager/dnsinterceptor/handler.go @@ -22,7 +22,6 @@ import ( nbdns "github.com/netbirdio/netbird/client/internal/dns" "github.com/netbirdio/netbird/client/internal/dns/resutil" "github.com/netbirdio/netbird/client/internal/peer" - "github.com/netbirdio/netbird/client/internal/peerstore" "github.com/netbirdio/netbird/client/internal/routemanager/common" "github.com/netbirdio/netbird/client/internal/routemanager/fakeip" iface "github.com/netbirdio/netbird/client/internal/routemanager/iface" @@ -40,6 +39,61 @@ type internalDNATer interface { AddInternalDNATMapping(netip.Addr, netip.Addr) error } +// peerAllowedIPs reports the tunnel address a peer is reachable on. +type peerAllowedIPs interface { + AllowedIP(pubKey string) (netip.Addr, bool) +} + +// disposition says where a query for a given record type has to be answered. +type disposition int + +const ( + // dispositionPeer: the routing peer owns the answer. Address records are + // what a DNS route exists for, and their answer programs the routes, + // allowed IPs and firewall sets, so they are never resolved anywhere else. + dispositionPeer disposition = iota + // dispositionPeerFirst: the peer's forwarder resolves the type, and the + // records may only exist inside the routed network, so it has to be asked. + // A peer running a client that predates that support cannot answer, which + // only its reply reveals. + dispositionPeerFirst + // dispositionChain: no forwarder resolves the type, so asking would cost a + // tunnel round trip for a reply that says nothing. Public records for the + // name are still better than none, so the query goes to the rest of the + // chain. + dispositionChain +) + +func dispositionFor(qtype uint16) disposition { + switch { + case qtype == dns.TypeA || qtype == dns.TypeAAAA: + return dispositionPeer + case resutil.SupportedRecordQtype(qtype): + return dispositionPeerFirst + default: + return dispositionChain + } +} + +// peerCannotAnswer reports whether a forwarder reply means the peer is unable to +// resolve this query type, as opposed to having resolved it and found nothing. +// A forwarder older than the one that learned to resolve non-address types +// answers NOTIMP to all of them, before it even looks at the domain; FORMERR +// covers one that cannot parse the EDNS0 we add to the query. +// +// REFUSED is deliberately not here. It says the peer does not hold the name, +// which happens while the client and the peer disagree about the route set, and +// falling through then would hand the name of an internal-only domain to a +// public resolver. No forwarder version reports a missing record type that way. +func peerCannotAnswer(rcode int) bool { + switch rcode { + case dns.RcodeNotImplemented, dns.RcodeFormatError: + return true + default: + return false + } +} + type DnsInterceptor struct { mu sync.RWMutex route *route.Route @@ -50,7 +104,7 @@ type DnsInterceptor struct { currentPeerKey string interceptedDomains domainMap wgInterface iface.WGIface - peerStore *peerstore.Store + peerStore peerAllowedIPs firewall firewall.Manager fakeIPManager *fakeip.Manager forwarderPort *atomic.Uint32 @@ -226,11 +280,14 @@ func (d *DnsInterceptor) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { return } - // All query types for an intercepted domain are forwarded to the peer's - // DNS forwarder, which owns the name. Falling through to the system - // resolver would let it answer NXDOMAIN for a name it isn't authoritative - // for, poisoning the whole name (including the A/AAAA records the route - // does serve). The forwarder answers NODATA for types it cannot resolve. + qtype := r.Question[0].Qtype + dispose := dispositionFor(qtype) + + if dispose == dispositionChain { + d.deferToChain(w, r, logger, "unsupported-qtype") + return + } + d.mu.RLock() peerKey := d.currentPeerKey d.mu.RUnlock() @@ -246,35 +303,32 @@ func (d *DnsInterceptor) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { return } - if r.Extra == nil { - r.MsgHdr.AuthenticatedData = true - } - - // Advertise EDNS0 to the forwarder so it may return an Extended DNS Error - // describing why a lookup failed. The OPT is stripped from the reply when - // the original client did not request EDNS0. - hadEdns := r.IsEdns0() != nil - if !hadEdns { - r.SetEdns0(dns.DefaultMsgSize, false) - } + query, hadEdns := peerQuery(r) upstream := net.JoinHostPort(upstreamIP.String(), strconv.FormatUint(uint64(d.forwarderPort.Load()), 10)) ctx, cancel := context.WithTimeout(context.Background(), dnsTimeout) defer cancel() - reply := d.queryUpstreamDNS(ctx, w, r, upstream, upstreamIP, peerKey, logger) + reply := d.queryUpstreamDNS(ctx, w, query, upstream, upstreamIP, peerKey, logger) if reply == nil { return } + // The peer owns the name but its forwarder cannot resolve this type. No + // capability is announced anywhere, so the round trip above is the probe. + if dispose == dispositionPeerFirst && peerCannotAnswer(reply.Rcode) { + d.deferToChain(w, r, logger, "peer-rcode-"+dns.RcodeToString[reply.Rcode]) + return + } + if ede, ok := resutil.ExtractEDE(reply); ok { - resutil.SetMeta(w, "ede", fmt.Sprintf("%d %s", ede.InfoCode, ede.ExtraText)) + resutil.SetMeta(w, resutil.MetaKeyEDE, fmt.Sprintf("%d %s", ede.InfoCode, ede.ExtraText)) } if !hadEdns { resutil.StripOPT(reply) } - resutil.SetMeta(w, "peer", peerKey) + resutil.SetMeta(w, resutil.MetaKeyPeer, peerKey) reply.Id = r.Id if err := d.writeMsg(w, reply, logger); err != nil { @@ -282,6 +336,30 @@ func (d *DnsInterceptor) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { } } +// deferToChain hands the query to the next handler in the chain and asks the +// chain to soften the negative verdict of whatever answers instead. The +// resolvers below cannot prove a name inside the routed network absent, so their +// NXDOMAIN must not reach the client: it would be cached for the name and every +// type under it, taking the addresses this route does serve with it. +func (d *DnsInterceptor) deferToChain(w dns.ResponseWriter, r *dns.Msg, logger *log.Entry, reason string) { + logger.Tracef("continuing to next handler for domain=%s type=%s reason=%s", + r.Question[0].Name, dns.TypeToString[r.Question[0].Qtype], reason) + + resutil.RequestSoftNegative(w) + // Carried to the chain's response log line: without it the answer looks like + // it came from the fallthrough resolver on its own. + resutil.SetMeta(w, resutil.MetaKeyDeferredBy, "dns-route") + resutil.SetMeta(w, resutil.MetaKeyDeferredReason, reason) + + resp := new(dns.Msg) + resp.SetRcode(r, dns.RcodeNameError) + // Set Zero bit to signal handler chain to continue + resp.MsgHdr.Zero = true + if err := w.WriteMsg(resp); err != nil { + logger.Errorf("failed writing DNS continue response: %v", err) + } +} + func (d *DnsInterceptor) writeDNSError(w dns.ResponseWriter, r *dns.Msg, logger *log.Entry, reason string) { logger.Warnf("failed to query upstream for domain=%s: %s", r.Question[0].Name, reason) @@ -621,3 +699,26 @@ func (d *DnsInterceptor) debugPeerTimeout(peerIP netip.Addr, peerKey string) str return fmt.Sprintf(" (peer %s)", nbdns.FormatPeerStatus(&peerState)) } + +// peerQuery builds the query sent to a peer's DNS forwarder and reports whether +// the client itself advertised EDNS0. EDNS0 is added so the forwarder can return +// an Extended DNS Error describing an upstream failure, and the OPT is stripped +// from the reply again when the client did not ask for it. The client's message +// is left untouched: it may still be handed to the next handler in the chain, +// and neither the OPT nor the AD bit is ours to put on the wire on its behalf. +func peerQuery(r *dns.Msg) (query *dns.Msg, hadEdns bool) { + query = r.Copy() + hadEdns = query.IsEdns0() != nil + + // AD tells the forwarder we understand authenticated data. Only set when the + // client sent no additional section of its own, so we never overrule what it + // asked for. + if len(query.Extra) == 0 { + query.MsgHdr.AuthenticatedData = true + } + if !hadEdns { + query.SetEdns0(dns.DefaultMsgSize, false) + } + + return query, hadEdns +} diff --git a/client/internal/routemanager/dnsinterceptor/handler_test.go b/client/internal/routemanager/dnsinterceptor/handler_test.go new file mode 100644 index 000000000..9f9614e8e --- /dev/null +++ b/client/internal/routemanager/dnsinterceptor/handler_test.go @@ -0,0 +1,392 @@ +package dnsinterceptor + +import ( + "net" + "net/netip" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.zx2c4.com/wireguard/tun/netstack" + + "github.com/netbirdio/netbird/client/iface/device" + "github.com/netbirdio/netbird/client/iface/wgaddr" + "github.com/netbirdio/netbird/client/internal/dns/resutil" + "github.com/netbirdio/netbird/client/internal/dns/test" + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/route" +) + +// softNegativeWriter records what the handler told the chain: whether to soften +// a negative verdict from the handlers it defers to, and the metadata that ends +// up on the chain's response log line. +type softNegativeWriter struct { + test.MockResponseWriter + softNegative bool + meta map[resutil.MetaKey]string +} + +func (w *softNegativeWriter) RequestSoftNegative() { w.softNegative = true } + +func (w *softNegativeWriter) RequestID() string { return "test" } + +func (w *softNegativeWriter) SetMeta(key resutil.MetaKey, value string) { + if w.meta == nil { + w.meta = make(map[resutil.MetaKey]string) + } + w.meta[key] = value +} + +// TestServeDNS_QtypeTheForwarderCannotResolve covers the record types no DNS +// forwarder can answer, because the host resolver exposes no API for them. +// Asking the peer only burns a tunnel round trip, so the query goes straight to +// the rest of the chain. No peer key is configured, proving no round trip is +// attempted. +func TestServeDNS_QtypeTheForwarderCannotResolve(t *testing.T) { + qtypes := []uint16{ + dns.TypeHTTPS, + dns.TypeSVCB, + dns.TypeCAA, + dns.TypeNAPTR, + dns.TypeTLSA, + dns.TypeSOA, + } + + for _, qtype := range qtypes { + t.Run(dns.TypeToString[qtype], func(t *testing.T) { + d := &DnsInterceptor{} + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("db.example.com.", qtype) + + d.ServeDNS(w, r) + + resp := w.GetLastResponse() + require.NotNil(t, resp, "a response must be written") + assert.Equal(t, dns.RcodeNameError, resp.Rcode, "chain continuation is signalled as NXDOMAIN") + assert.True(t, resp.MsgHdr.Zero, "Zero bit must be set so the chain continues") + assert.True(t, w.softNegative, + "a downstream NXDOMAIN must be softened, or the fallthrough poisons the routed name") + assert.Equal(t, "dns-route", w.meta[resutil.MetaKeyDeferredBy], + "the chain's response log line must name us as the handler that stepped aside") + assert.Equal(t, "unsupported-qtype", w.meta[resutil.MetaKeyDeferredReason], + "the reason must survive to the log line, or the fallthrough is invisible") + }) + } +} + +// TestServeDNS_AddressQtypeNeverFallsThrough locks in the opposite case: A and +// AAAA are what the route exists for, and the peer is authoritative for them. +// A missing peer is an error the client must see, never a fallthrough to a +// resolver that knows nothing about the routed name. +func TestServeDNS_AddressQtypeNeverFallsThrough(t *testing.T) { + for _, qtype := range []uint16{dns.TypeA, dns.TypeAAAA} { + t.Run(dns.TypeToString[qtype], func(t *testing.T) { + d := &DnsInterceptor{} + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("db.example.com.", qtype) + + d.ServeDNS(w, r) + + resp := w.GetLastResponse() + require.NotNil(t, resp, "a response must be written") + assert.Equal(t, dns.RcodeServerFailure, resp.Rcode, "an unusable route must fail, not fall through") + assert.False(t, resp.MsgHdr.Zero, "the chain must not continue for address queries") + assert.False(t, w.softNegative, "no fallthrough means nothing to soften") + }) + } +} + +// TestDispositionFor pins the query-type policy, and ties the types we ask a +// peer about to the types a forwarder can actually resolve, so the two ends +// cannot drift apart. +func TestDispositionFor(t *testing.T) { + tests := []struct { + qtype uint16 + want disposition + }{ + {dns.TypeA, dispositionPeer}, + {dns.TypeAAAA, dispositionPeer}, + {dns.TypeMX, dispositionPeerFirst}, + {dns.TypeTXT, dispositionPeerFirst}, + {dns.TypeNS, dispositionPeerFirst}, + {dns.TypeSRV, dispositionPeerFirst}, + {dns.TypeCNAME, dispositionPeerFirst}, + {dns.TypePTR, dispositionPeerFirst}, + {dns.TypeHTTPS, dispositionChain}, + {dns.TypeSVCB, dispositionChain}, + {dns.TypeCAA, dispositionChain}, + {dns.TypeNAPTR, dispositionChain}, + {dns.TypeTLSA, dispositionChain}, + {dns.TypeDS, dispositionChain}, + {dns.TypeDNSKEY, dispositionChain}, + {dns.TypeSOA, dispositionChain}, + {dns.TypeANY, dispositionChain}, + } + + for _, tt := range tests { + t.Run(dns.TypeToString[tt.qtype], func(t *testing.T) { + assert.Equal(t, tt.want, dispositionFor(tt.qtype), "disposition for %s", dns.TypeToString[tt.qtype]) + + if tt.want != dispositionPeer { + assert.Equal(t, tt.want == dispositionPeerFirst, resutil.SupportedRecordQtype(tt.qtype), + "only types a forwarder resolves may be sent to a peer") + } + }) + } +} + +// TestPeerCannotAnswer separates a peer that cannot resolve a type from one that +// resolved it and found nothing. Only the former may be retried elsewhere: +// treating NODATA or NXDOMAIN as a capability failure would send the client to a +// public resolver behind the peer's back and prefer a public record over the +// private one the route exists to reach. +func TestPeerCannotAnswer(t *testing.T) { + cannot := []int{dns.RcodeNotImplemented, dns.RcodeFormatError} + for _, rcode := range cannot { + assert.True(t, peerCannotAnswer(rcode), "%s means the peer cannot answer", dns.RcodeToString[rcode]) + } + + // REFUSED belongs here, not above: it says the peer does not hold the name, + // so retrying elsewhere would leak the name of an internal-only domain to a + // public resolver. No forwarder version reports a missing record type this + // way, so nothing is lost by keeping it out. + can := []int{ + dns.RcodeSuccess, + dns.RcodeNameError, + dns.RcodeServerFailure, + dns.RcodeNotAuth, + dns.RcodeRefused, + } + for _, rcode := range can { + assert.False(t, peerCannotAnswer(rcode), "%s is the peer's answer, not a capability failure", dns.RcodeToString[rcode]) + } +} + +// fakeWGIface is the minimum an interceptor needs to reach a peer's DNS +// forwarder over a plain socket. A nil netstack keeps the exchange on the host +// stack so the test can point it at a loopback listener. +type fakeWGIface struct{} + +func (fakeWGIface) AddAllowedIP(string, netip.Prefix) error { return nil } +func (fakeWGIface) RemoveAllowedIP(string, netip.Prefix) error { return nil } +func (fakeWGIface) Name() string { return "wt0" } +func (fakeWGIface) Address() wgaddr.Address { return wgaddr.Address{} } +func (fakeWGIface) ToInterface() *net.Interface { return nil } +func (fakeWGIface) IsUserspaceBind() bool { return false } +func (fakeWGIface) GetFilter() device.PacketFilter { return nil } +func (fakeWGIface) GetDevice() *device.FilteredDevice { return nil } +func (fakeWGIface) GetNet() *netstack.Net { return nil } + +// fakePeerIPs resolves every peer key to the loopback address the test's +// forwarder stub listens on. +type fakePeerIPs struct { + addr netip.Addr +} + +func (p fakePeerIPs) AllowedIP(string) (netip.Addr, bool) { return p.addr, p.addr.IsValid() } + +// forwarderStub stands in for the DNS forwarder of a routing peer, recording +// what it was asked and answering with a canned reply. +type forwarderStub struct { + mu sync.Mutex + queries []*dns.Msg + reply func(q *dns.Msg) *dns.Msg + port uint16 + shutdown func() +} + +func (s *forwarderStub) received() []*dns.Msg { + s.mu.Lock() + defer s.mu.Unlock() + return append([]*dns.Msg(nil), s.queries...) +} + +func newForwarderStub(t *testing.T, reply func(q *dns.Msg) *dns.Msg) *forwarderStub { + t.Helper() + + conn, err := net.ListenUDP("udp", net.UDPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:0"))) + require.NoError(t, err, "listen for the forwarder stub") + + stub := &forwarderStub{reply: reply} + stub.port = uint16(conn.LocalAddr().(*net.UDPAddr).Port) + + mux := dns.NewServeMux() + mux.HandleFunc(".", func(w dns.ResponseWriter, q *dns.Msg) { + stub.mu.Lock() + stub.queries = append(stub.queries, q.Copy()) + stub.mu.Unlock() + _ = w.WriteMsg(stub.reply(q)) + }) + + srv := &dns.Server{PacketConn: conn, Handler: mux} + started := make(chan struct{}) + srv.NotifyStartedFunc = func() { close(started) } + go func() { + _ = srv.ActivateAndServe() + }() + select { + case <-started: + case <-time.After(5 * time.Second): + t.Fatal("forwarder stub did not start") + } + + stub.shutdown = func() { _ = srv.Shutdown() } + t.Cleanup(stub.shutdown) + + return stub +} + +func newTestInterceptor(t *testing.T, stub *forwarderStub) *DnsInterceptor { + t.Helper() + + port := new(atomic.Uint32) + port.Store(uint32(stub.port)) + + return &DnsInterceptor{ + route: &route.Route{Domains: nil}, + statusRecorder: peer.NewRecorder("https://mgm"), + currentPeerKey: "peer-key", + interceptedDomains: make(domainMap), + wgInterface: fakeWGIface{}, + peerStore: fakePeerIPs{addr: netip.MustParseAddr("127.0.0.1")}, + forwarderPort: port, + } +} + +// TestServeDNS_PeerCannotResolveQtype is the reported regression: a routing peer +// running a client older than the one that taught the forwarder about non-address +// record types answers NOTIMP for every SRV query, and the interceptor used to +// hand that straight to the application, breaking SRV-based service discovery. +// The client cannot know the peer's capability up front, so it must try the peer +// and fall back to the chain when the peer says it cannot answer. +func TestServeDNS_PeerCannotResolveQtype(t *testing.T) { + for _, rcode := range []int{dns.RcodeNotImplemented, dns.RcodeFormatError} { + t.Run(dns.RcodeToString[rcode], func(t *testing.T) { + stub := newForwarderStub(t, func(q *dns.Msg) *dns.Msg { + resp := new(dns.Msg) + resp.SetRcode(q, rcode) + return resp + }) + d := newTestInterceptor(t, stub) + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + d.ServeDNS(w, r) + + require.Len(t, stub.received(), 1, "the peer must be asked before giving up on it") + + resp := w.GetLastResponse() + require.NotNil(t, resp, "a response must be written") + assert.Equal(t, dns.RcodeNameError, resp.Rcode, "chain continuation is signalled as NXDOMAIN") + assert.True(t, resp.MsgHdr.Zero, "Zero bit must be set so the chain continues") + assert.True(t, w.softNegative, "the fallthrough must not be allowed to poison the routed name") + assert.Equal(t, "dns-route", w.meta[resutil.MetaKeyDeferredBy]) + assert.Equal(t, "peer-rcode-"+dns.RcodeToString[rcode], w.meta[resutil.MetaKeyDeferredReason], + "the peer's verdict must be visible in the log, it is the only trace of the probe") + }) + } +} + +// TestServeDNS_PeerVerdictPassesThrough covers the replies that are the peer's +// answer rather than a statement about its capability. The peer is authoritative +// for a name routed to it, so its verdict reaches the client as it is: retrying +// these elsewhere would send the name of an internal-only domain to a public +// resolver and prefer whatever that resolver says over the route. +func TestServeDNS_PeerVerdictPassesThrough(t *testing.T) { + // REFUSED means the peer does not consider the name routed to it, which + // happens while the client and the peer disagree about the route set. It is + // never how a peer reports a record type it cannot resolve: a forwarder too + // old for non-address types answers NOTIMP before it looks at the domain. + for _, rcode := range []int{dns.RcodeRefused, dns.RcodeNameError, dns.RcodeSuccess} { + t.Run(dns.RcodeToString[rcode], func(t *testing.T) { + stub := newForwarderStub(t, func(q *dns.Msg) *dns.Msg { + resp := new(dns.Msg) + resp.SetRcode(q, rcode) + return resp + }) + d := newTestInterceptor(t, stub) + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + d.ServeDNS(w, r) + + require.Len(t, stub.received(), 1, "the peer must be asked exactly once") + + resp := w.GetLastResponse() + require.NotNil(t, resp, "a response must be written") + assert.Equal(t, rcode, resp.Rcode, "the peer's verdict must reach the client unchanged") + assert.False(t, resp.MsgHdr.Zero, "the chain must not continue past an answered query") + assert.False(t, w.softNegative, "nothing was deferred, so nothing may be softened") + }) + } +} + +// TestServeDNS_PeerResolvesQtype guards the common case: a peer that can answer +// the type stays authoritative, and its records reach the client unchanged. +func TestServeDNS_PeerResolvesQtype(t *testing.T) { + stub := newForwarderStub(t, func(q *dns.Msg) *dns.Msg { + resp := new(dns.Msg) + resp.SetReply(q) + resp.Answer = []dns.RR{&dns.SRV{ + Hdr: dns.RR_Header{Name: q.Question[0].Name, Rrtype: dns.TypeSRV, Class: dns.ClassINET, Ttl: 60}, + Target: "shard-00.db.example.com.", + Port: 27017, + }} + return resp + }) + d := newTestInterceptor(t, stub) + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + d.ServeDNS(w, r) + + resp := w.GetLastResponse() + require.NotNil(t, resp, "a response must be written") + assert.Equal(t, dns.RcodeSuccess, resp.Rcode) + require.Len(t, resp.Answer, 1, "the peer's records must pass through") + assert.False(t, resp.MsgHdr.Zero, "an answered query must not continue the chain") + assert.False(t, w.softNegative) +} + +// TestServeDNS_DeferredQueryIsPristine covers what the fallthrough hands to the +// next handler. The interceptor advertises EDNS0 to the peer so the forwarder can +// return an Extended DNS Error, and sets AD, but neither belongs to the client's +// query: on the deferred path they would travel to the public resolver and the +// reply could come back carrying an OPT the client never advertised, which +// RFC 6891 forbids us from passing on. +func TestServeDNS_DeferredQueryIsPristine(t *testing.T) { + stub := newForwarderStub(t, func(q *dns.Msg) *dns.Msg { + resp := new(dns.Msg) + resp.SetRcode(q, dns.RcodeNotImplemented) + return resp + }) + d := newTestInterceptor(t, stub) + w := &softNegativeWriter{} + + r := new(dns.Msg) + r.SetQuestion("_mongodb._tcp.db.example.com.", dns.TypeSRV) + + d.ServeDNS(w, r) + + sent := stub.received() + require.Len(t, sent, 1) + assert.NotNil(t, sent[0].IsEdns0(), "the peer must be asked with EDNS0 so it can return an EDE") + + assert.Nil(t, r.IsEdns0(), "the deferred query must not carry an OPT the client never sent") + assert.False(t, r.AuthenticatedData, "the deferred query must not carry an AD bit the client never set") + assert.Empty(t, r.Extra, "the deferred query must reach the next handler as the client sent it") +} diff --git a/client/internal/routemanager/dnsinterceptor/handler_windows_test.go b/client/internal/routemanager/dnsinterceptor/handler_windows_test.go new file mode 100644 index 000000000..2f08bee74 --- /dev/null +++ b/client/internal/routemanager/dnsinterceptor/handler_windows_test.go @@ -0,0 +1,7 @@ +//go:build windows + +package dnsinterceptor + +// GetInterfaceGUIDString completes iface.WGIface on Windows, which requires the +// interface GUID for DNS registration. +func (fakeWGIface) GetInterfaceGUIDString() (string, error) { return "", nil }