Opened 8 years ago

Closed 8 years ago

#1217 closed defect (fixed)

Error in automatically updating TTLs that do not match in an RRset

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20111122
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 5 Internal?: no

Description

In our new datasource code, we automatically update the TTL of an RR if we are adding it to an RRset with a different TTL. This is fine in general, but we should not do it blindly; there may be more, but at least in the case of an RRSIG 'rrset' we shouldn't do it; RRSIGS for the same name can have a different TTL.

For direct queries this is not much of a problem; we refuse direct RRSIG queries currently, and for covered types the TTL of the covered set is used.

It would be a problem with AXFR out though (or, lower-level, the zone iterators). If we want to be as strict as possible, we should look at whether the covered types are equal too before modifying the TTL of RRSIG RRs. Or not check TTL for RRSIG at all.

PS. Also played around with this for a bit, current AXFR IN modifies TTLs badly as well (though it gets fixed again when querying). Have not tried current AXFR out, but if the values are wrong in the database already it's probably not going to be good ;) Anyway with the refactor coming to an end, we should focus on the new stuff, and I don't think we need to fix the old one.

Subtickets

Change History (17)

comment:1 in reply to: ↑ description Changed 8 years ago by jinmei

Replying to jelte:

It would be a problem with AXFR out though (or, lower-level, the zone iterators). If we want to be as strict as possible, we should look at whether the covered types are equal too before modifying the TTL of RRSIG RRs. Or not check TTL for RRSIG at all.

For the iteration case, maybe we should provide an "as is" or "RR"
(instead of RRset) mode, where the iterator passes RRs of the zone
in the form of an RRset containing exactly one RDATA, ignoring any
duplicate, inconsistent TTLs, etc, and lets the caller deal with those.

comment:2 Changed 8 years ago by jelte

Ack. Or let it consider a different TTL as another indicator that this is a new RRset. For AXFR it shouldn't matter, nor for iteration to create a zone file etc.

comment:3 Changed 8 years ago by shane

  • Defect Severity changed from N/A to Medium
  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:4 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111025

comment:6 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:8 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:9 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

I've added a parameter 'individual_rrs' to getIterator(), which if true (default value false), makes getNextRRset always stop after it found 1 rr.

Added tests for and implemented in sqlite backend.

As for the memory backend, the tests are still disabled (#1346), and in the source the parameter is ignored, and might not even be relevant, as the data is already stored as rrsets (so if not stored individually, the ttl info is already lost). So it would depend on how they are added. But atm I consider this out of scope for this specific ticket (and we should really do 1346 first)

comment:10 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:11 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed commit 701ffebae5b357a693e764bbef904dc374ebb591

src/lib/datasrc/memory_datasrc.{h,cc}

As for the memory backend, the tests are still disabled (#1346), and in the source the parameter is ignored, and might not even be relevant, as the data is already stored as rrsets (so if not stored individually, the ttl info is already lost)

This may prove surprising if future code attempts to iterate through a zone one RR at a time. Rather than the individual_rr parameter being ignored, I would suggest either:

  • Implementing the functionality.
  • Check the value of the argument and if true, throw a NotImplemented exception.

src/lib/datasrc/tests/database_unittest.cc
ttldiff_individual test: within the "while" loop, the "if" test should include an "else" branch which calls FAIL(). (Just an additional check that the data is as expected.)

comment:12 in reply to: ↑ 11 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

  • Implementing the functionality.
  • Check the value of the argument and if true, throw a NotImplemented exception.

Neither option really appealed to me, tbh; the first would just be a waste of resources, and the second would make the option useless (we will need it for some cases like xfrout, and then we will always need it)

So i opted for a slightly different approach; more in line with what i originally had in mind actually; i've changed the argument to 'adjust_ttl', if true, it will behave as it used to (if they differ, modify). If false, it will view a different ttl as an indicator that the RR belongs to a different RRset.

How's that?

src/lib/datasrc/tests/database_unittest.cc
ttldiff_individual test: within the "while" loop, the "if" test should include an "else" branch which calls FAIL(). (Just an additional check that the data is as expected.)

ack, added

comment:13 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed commit c5f69488232bd0464cd7e2174be96b30b51b7e83

Just one comment:

src/lib/python/isc/datasrc/client_python.cc
DataSourceClient_getIterator(): According to the documentation, PyObject_Not() can return -1 on error. This possibility should be checked.

comment:14 Changed 8 years ago by jelte

  • Owner changed from jelte to stephen

ack, done, please check f2ffe07f7e25c037855685b7693ea4d4eed1cd0c

comment:15 Changed 8 years ago by stephen

  • Owner changed from stephen to jelte

Looks OK, please merge.

comment:16 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:17 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 5

oh I had merged this already (and the resulting problems will be addressed ni #1384 ;))

Note: See TracTickets for help on using tickets.