From b865aca03a5c653356334c789b54e70c0bd0e08d Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Wed, 4 Oct 2023 18:16:22 +0200 Subject: [PATCH] - Fix #946: Forwarder returns servfail on upstream response noerror no data. --- Makefile.in | 2 +- doc/Changelog | 4 +++ iterator/iter_resptype.c | 25 ++++++++++++----- iterator/iter_resptype.h | 4 ++- iterator/iterator.c | 19 +++++++++++-- iterator/iterator.h | 7 +++++ testdata/iter_ignore_empty.rpl | 50 ++++++++++++++++++++++++++++++++++ 7 files changed, 99 insertions(+), 12 deletions(-) diff --git a/Makefile.in b/Makefile.in index 627a650f6..22fb75c12 100644 --- a/Makefile.in +++ b/Makefile.in @@ -793,7 +793,7 @@ iter_priv.lo iter_priv.o: $(srcdir)/iterator/iter_priv.c config.h $(srcdir)/iter $(srcdir)/util/data/msgparse.h $(srcdir)/sldns/pkthdr.h $(srcdir)/sldns/rrdef.h $(srcdir)/util/net_help.h \ $(srcdir)/util/storage/dnstree.h $(srcdir)/sldns/str2wire.h $(srcdir)/sldns/sbuffer.h iter_resptype.lo iter_resptype.o: $(srcdir)/iterator/iter_resptype.c config.h \ - $(srcdir)/iterator/iter_resptype.h $(srcdir)/iterator/iter_delegpt.h $(srcdir)/util/log.h \ + $(srcdir)/iterator/iter_resptype.h $(srcdir)/iterator/iter_delegpt.h $(srcdir)/iterator/iterator.h $(srcdir)/util/log.h \ $(srcdir)/services/cache/dns.h $(srcdir)/util/storage/lruhash.h $(srcdir)/util/locks.h \ $(srcdir)/util/data/msgreply.h $(srcdir)/util/data/packed_rrset.h $(srcdir)/util/net_help.h \ $(srcdir)/util/data/dname.h $(srcdir)/sldns/rrdef.h $(srcdir)/sldns/pkthdr.h diff --git a/doc/Changelog b/doc/Changelog index 22029a89b..f18f97374 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,7 @@ +4 October 2023: Wouter + - Fix #946: Forwarder returns servfail on upstream response noerror no + data. + 3 October 2023: George - Merge #881: Generalise the proxy protocol code. diff --git a/iterator/iter_resptype.c b/iterator/iter_resptype.c index e85595b84..38e186e79 100644 --- a/iterator/iter_resptype.c +++ b/iterator/iter_resptype.c @@ -42,6 +42,7 @@ #include "config.h" #include "iterator/iter_resptype.h" #include "iterator/iter_delegpt.h" +#include "iterator/iterator.h" #include "services/cache/dns.h" #include "util/net_help.h" #include "util/data/dname.h" @@ -105,7 +106,8 @@ response_type_from_cache(struct dns_msg* msg, enum response_type response_type_from_server(int rdset, - struct dns_msg* msg, struct query_info* request, struct delegpt* dp) + struct dns_msg* msg, struct query_info* request, struct delegpt* dp, + int* empty_nodata_found) { uint8_t* origzone = (uint8_t*)"\000"; /* the default */ struct ub_packed_rrset_key* s; @@ -284,13 +286,22 @@ response_type_from_server(int rdset, /* If we've gotten this far, this is NOERROR/NODATA (which could * be an entirely empty message) */ - /* but ignore entirely empty messages, noerror/nodata has a soa - * negative ttl value in the authority section, this makes it try - * again at another authority. And turns it from a 5 second empty - * message into a 5 second servfail response. */ + /* For entirely empty messages, try again, at first, then accept + * it it happens more. A regular noerror/nodata response has a soa + * negative ttl value in the authority section. This makes it try + * again at another authority. And decides between storing a 5 second + * empty message or a 5 second servfail response. */ if(msg->rep->an_numrrsets == 0 && msg->rep->ns_numrrsets == 0 && - msg->rep->ar_numrrsets == 0) - return RESPONSE_TYPE_THROWAWAY; + msg->rep->ar_numrrsets == 0) { + if(empty_nodata_found) { + /* detect as throwaway at first, but accept later. */ + (*empty_nodata_found)++; + if(*empty_nodata_found < EMPTY_NODATA_RETRY_COUNT) + return RESPONSE_TYPE_THROWAWAY; + return RESPONSE_TYPE_ANSWER; + } + return RESPONSE_TYPE_ANSWER; + } /* check if recursive answer; saying it has empty cache */ if( (msg->rep->flags&BIT_RA) && !(msg->rep->flags&BIT_AA) && !rdset) return RESPONSE_TYPE_REC_LAME; diff --git a/iterator/iter_resptype.h b/iterator/iter_resptype.h index fee9ef35f..bfd4b664f 100644 --- a/iterator/iter_resptype.h +++ b/iterator/iter_resptype.h @@ -119,9 +119,11 @@ enum response_type response_type_from_cache(struct dns_msg* msg, * @param request: the request that generated the response. * @param dp: The delegation point that was being queried * when the response was returned. + * @param empty_nodata_found: flag to keep track of empty nodata detection. * @return the response type (CNAME or ANSWER). */ enum response_type response_type_from_server(int rdset, - struct dns_msg* msg, struct query_info* request, struct delegpt* dp); + struct dns_msg* msg, struct query_info* request, struct delegpt* dp, + int* empty_nodata_found); #endif /* ITERATOR_ITER_RESPTYPE_H */ diff --git a/iterator/iterator.c b/iterator/iterator.c index 9f78aa17d..106e2877e 100644 --- a/iterator/iterator.c +++ b/iterator/iterator.c @@ -2940,7 +2940,7 @@ static int processQueryResponse(struct module_qstate* qstate, struct iter_qstate* iq, struct iter_env* ie, int id) { - int dnsseclame = 0, origtypecname = 0; + int dnsseclame = 0, origtypecname = 0, orig_empty_nodata_found; enum response_type type; iq->num_current_queries--; @@ -2960,12 +2960,25 @@ processQueryResponse(struct module_qstate* qstate, struct iter_qstate* iq, return next_state(iq, QUERYTARGETS_STATE); } iq->timeout_count = 0; + orig_empty_nodata_found = iq->empty_nodata_found; type = response_type_from_server( (int)((iq->chase_flags&BIT_RD) || iq->chase_to_rd), - iq->response, &iq->qinfo_out, iq->dp); + iq->response, &iq->qinfo_out, iq->dp, &iq->empty_nodata_found); iq->chase_to_rd = 0; /* remove TC flag, if this is erroneously set by TCP upstream */ iq->response->rep->flags &= ~BIT_TC; + if(orig_empty_nodata_found != iq->empty_nodata_found && + iq->empty_nodata_found < EMPTY_NODATA_RETRY_COUNT) { + /* try to search at another server */ + if(qstate->reply) { + struct delegpt_addr* a = delegpt_find_addr( + iq->dp, &qstate->reply->remote_addr, + qstate->reply->remote_addrlen); + /* make selection disprefer it */ + if(a) a->lame = 1; + } + return next_state(iq, QUERYTARGETS_STATE); + } if(type == RESPONSE_TYPE_REFERRAL && (iq->chase_flags&BIT_RD) && !iq->auth_zone_response) { /* When forwarding (RD bit is set), we handle referrals @@ -3501,7 +3514,7 @@ processPrimeResponse(struct module_qstate* qstate, int id) iq->response->rep->flags &= ~(BIT_RD|BIT_RA); /* ignore rec-lame */ type = response_type_from_server( (int)((iq->chase_flags&BIT_RD) || iq->chase_to_rd), - iq->response, &iq->qchase, iq->dp); + iq->response, &iq->qchase, iq->dp, NULL); if(type == RESPONSE_TYPE_ANSWER) { qstate->return_rcode = LDNS_RCODE_NOERROR; qstate->return_msg = iq->response; diff --git a/iterator/iterator.h b/iterator/iterator.h index fad7f03e6..e253f3f7e 100644 --- a/iterator/iterator.h +++ b/iterator/iterator.h @@ -101,6 +101,8 @@ extern int BLACKLIST_PENALTY; * Chosen so that the UNKNOWN_SERVER_NICENESS falls within the band of a * fast server, this causes server exploration as a side benefit. msec. */ #define RTT_BAND 400 +/** Number of retries for empty nodata packets before it is accepted. */ +#define EMPTY_NODATA_RETRY_COUNT 2 /** * Global state for the iterator. @@ -415,6 +417,11 @@ struct iter_qstate { */ int refetch_glue; + /** + * This flag detects that a completely empty nodata was received, + * already so that it is accepted later. */ + int empty_nodata_found; + /** list of pending queries to authoritative servers. */ struct outbound_list outlist; diff --git a/testdata/iter_ignore_empty.rpl b/testdata/iter_ignore_empty.rpl index c70dd7e8d..4b2f695b8 100644 --- a/testdata/iter_ignore_empty.rpl +++ b/testdata/iter_ignore_empty.rpl @@ -78,6 +78,18 @@ example2.com. IN NS ns2.example2.com. SECTION ADDITIONAL ns2.example2.com. IN A 1.2.3.5 ENTRY_END + +ENTRY_BEGIN +MATCH opcode subdomain +ADJUST copy_id copy_query +REPLY QR NOERROR +SECTION QUESTION +foo.com. IN NS +SECTION AUTHORITY +foo.com. IN NS ns.foo.com. +SECTION ADDITIONAL +ns.foo.com. IN A 1.2.3.5 +ENTRY_END RANGE_END ; ns.example.com. @@ -172,6 +184,27 @@ www.example.com. IN A SECTION ANSWER www.example.com. IN A 10.20.30.40 ENTRY_END + +; foo.com +ENTRY_BEGIN +MATCH opcode qtype qname +ADJUST copy_id +REPLY QR AA NOERROR +SECTION QUESTION +www.foo.com. IN A +SECTION ANSWER +ENTRY_END + +ENTRY_BEGIN +MATCH opcode qtype qname +ADJUST copy_id +REPLY QR AA NOERROR +SECTION QUESTION +ns.foo.com. IN AAAA +SECTION ANSWER +SECTION AUTHORITY +;foo.com. IN SOA ns2.foo.com root.foo.com 4 14400 3600 604800 3600 +ENTRY_END RANGE_END STEP 1 QUERY @@ -195,4 +228,21 @@ ENTRY_END ; wait for pending nameserver lookups. STEP 20 TRAFFIC +; Test that a nodata stays a nodata. +STEP 30 QUERY +ENTRY_BEGIN +REPLY RD +SECTION QUESTION +www.foo.com. IN A +ENTRY_END + +STEP 40 CHECK_ANSWER +ENTRY_BEGIN +MATCH all +REPLY QR RD RA NOERROR +SECTION QUESTION +www.foo.com. IN A +SECTION ANSWER +ENTRY_END + SCENARIO_END -- 2.47.3