Skip to content
Snippets Groups Projects

IXFR special cases handling (2)

Merged IXFR special cases handling (2)
ixfr-single-soa2 into master
Merged Ghost User requested to merge ixfr-single-soa2 into master

Fixes #307 (closed), see the issue description and the discussion below for details. I went for option A for case (2) as no other people voted. ;-). Case (3) has been modified as proposed by @jvcelak.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
src/knot/nameserver/ixfr.c
680 674 }
681 675
682 676 if (adata->ext == NULL) {
683 if (!ixfr_enough_data(pkt)) {
  • Jan Včelák
    Jan Včelák @jvcelak ·
    Contributor

    By removing this check, a code flow with empty answer section is not terminated. That will cause OOM access a few lines below in zone_transfer_needed.

  • Ghost User
    Ghost User @ghost ·
    Author Contributor

    Right, didn't think about case with no Answer records. Will handle that (probably by reintroducing, but modifying the check).

  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/nameserver/ixfr.c
    706 696 if (adata->ext == NULL) {
    707 697 NS_NEED_TSIG_SIGNED(&adata->param->tsig_ctx, 0);
    708 698 if (!zone_transfer_needed(adata->param->zone, pkt)) {
    709 IXFRIN_LOG(LOG_WARNING, "master sent older serial, "
    710 "ignoring");
    699 if (knot_pkt_section(pkt, KNOT_ANSWER)->count > 1) {
    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      The answer is probably malformed even if count == 0. And if the first RR is not a SOA, the zone_transfer_needed will return false as well. Which means that if you really want to check whether the answer is malformed, you should check it's content. Not only the count of records.

      I don't know if this is worth logging, if there is so much extra work we have to do to evaluate if the answer is malformed.

    • Ghost User
      Ghost User @ghost ·
      Author Contributor

      The best will be to create a function, that checks the basic constraints for proper IXFR response: at least one SOA RR at the start of the Answer section. That's not much overhead and I think it's quite important to know why a transfer was ignored.

  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    137 139 self.check_record(section="answer", rtype=self.rtype, ttl=ttl,
    138 140 rdata=rdata, nordata=nordata)
    139 141
    142 def check_xfr(self, rcode="NOERROR"):
    143 '''Checks XFR message'''
    144
    145 self.resp, iter_copy = itertools.tee(self.resp)
    146
    147 # Get the first message.
    148 for msg in iter_copy:
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    137 139 self.check_record(section="answer", rtype=self.rtype, ttl=ttl,
    138 140 rdata=rdata, nordata=nordata)
    139 141
    142 def check_xfr(self, rcode="NOERROR"):
    143 '''Checks XFR message'''
    144
    145 self.resp, iter_copy = itertools.tee(self.resp)
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    174 for rrset in msg.answer:
    175 for rr in rrset:
    176 if rr_count == 0:
    177 if rr.rdtype != dns.rdatatype.SOA:
    178 set_err("First RR is not SOA")
    179 return
    180 else:
    181 soa_count += 1
    182
    183 elif rr_count == 1:
    184 if rr.rdtype == dns.rdatatype.SOA:
    185 set_err("Second RR is SOA")
    186 return
    187 else:
    188 # OK, it has the format of AXFR
    189 return
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    171
    172 self.resp, iter_copy = itertools.tee(self.resp)
    173 for msg in iter_copy:
    174 for rrset in msg.answer:
    175 for rr in rrset:
    176 if rr_count == 0:
    177 if rr.rdtype != dns.rdatatype.SOA:
    178 set_err("First RR is not SOA")
    179 return
    180 else:
    181 soa_count += 1
    182
    183 elif rr_count == 1:
    184 if rr.rdtype == dns.rdatatype.SOA:
    185 set_err("Second RR is SOA")
    186 return
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    158 compare(dns.rcode.to_text(msg.rcode()), rc, "RCODE")
    159
    160 # Check the first message only.
    161 break
    162
    163 # Checks whether the transfer is an AXFR-style IXFR
    164 def check_axfr_style_ixfr(self, axfr=None):
    165 # 1) QTYPE == IXFR && RCODE == NOERROR
    166 self.check_xfr()
    167
    168 # 2) Check if Answer contains AXFR data (first SOA, second non-SOA)
    169 soa_count = 0
    170 rr_count = 0
    171
    172 self.resp, iter_copy = itertools.tee(self.resp)
    173 for msg in iter_copy:
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • tests-extra/tools/dnstest/response.py
    250 cnt = 0
    251 if isinstance(self.resp, collections.Iterable):
    252 self.resp, iter_copy = itertools.tee(self.resp)
    253 for msg in iter_copy:
    254 if not section or section == "answer":
    255 sect = msg.answer
    256 elif section == "additional":
    257 sect = msg.additional
    258 elif section == "authority":
    259 sect = msg.authority
    260
    261 for rrset in sect:
    262 if rrset.rdtype == rtype or rtype == dns.rdatatype.ANY:
    263 cnt += len(rrset)
    264 else:
    265 if not section or section == "answer":
  • Ghost User Added 1 new commit:

    Added 1 new commit:

    • 6438a774 - ixfr: Format checking + removed redundant test
  • Ghost User Added 1 new commit:

    Added 1 new commit:

    • e8f7952c - ixfr: Fixed format checking.
  • Daniel Salzman mentioned in commit 975c9464

    mentioned in commit 975c9464

    • Please register or sign in to reply