Opened 8 years ago

Closed 8 years ago

#1570 closed task (complete)

DS query handling in auth::Query

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: Sprint-20120221
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 10.80 Internal?: no

Description

Type DS query requires special processing. auth::Query::process()
needs to be updated to support the special cases. We need to consider
the cases:

  • where it has authority for the parent zone
  • where it has authority for the child zone
  • where the query name is the root

See also 3.1.4.1 of RFC4035.

Subtickets

Change History (18)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 5 to 6

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120207
  • Priority changed from major to critical

comment:4 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:5 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to assigned

I somehow don't feel productive with this task, so I'm putting it back to Unassigned for someone else to pick up. I created two tests, or at last drafts of them, but that's it.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from assigned to accepted

comment:7 Changed 8 years ago by jinmei

trac1570 is ready for review.

The branch has become a bit bigger than I originally expected, so
the reviewer may want to divide the review into several stages. Here
are some hints:

  • The first few commits are cleanup and refactoring in the tests for further extension. This part can be reviewed separately. (from 848e9d9 to 00de2fb)
  • The main protocol work consists of several (mostly) independent scenarios:
    • From 47bca52 to 32d9c52: major case, DS query handled in the parent
    • 0d1406f: DS query that happens to be sent to the child
    • afc721f to efd87c8: handled the "grand parent" problem
    • f364574: special consideration for ./DS query
  • Other cleanups. The last one (53cde03) is not directly related to this task, but I thought it would be good to test it explicitly while I worked on the branch.

I'd consider this work a kind of bug fix, so I think it's worth a
changelog:

374.?	[bug]		jinmei, vorner
	The new query handling module of b10-auth now handles type DS
	query correctly.
	(Trac #1570, git TBD)

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:9 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

As suggested, this has been reviewed in several stages:

ee0782b to 848e9d9
OK

848e9d9 to 00de2fb
query_unittest.cc
A really trivial request: in the list of RRs at the top of the file, some are of the form:

const char* rr = "xxxxx..."

... but most are of are of the form

const char* rr =
    "xxx...."

The contents of the mock zone as a whole would be easier to read if they were all of the second form. (My editor does syntax highlighting so text strings stand out in a different colour. It is relatively easy to focus on these strings, especially if they all start at the same offset from the left-hand side of the screen. In the same vein, the contents of the mock zone would also be easier to read if the fields in the individual RRs were aligned under one another.)

00de2fb to 47bca52
OK

47bca52 to 32d9c52
OK

32d9c52 to 0d1406f
src/bin/auth/tests/query_unittest.cc
The comment above dsBelowDelegation test: although modified from "This one checks a DS record at the apex is not returned, even if it exists" to "This one checks a DS record at the apex is not returned", the comment still implies that a DS record exists at the apex.

dsBelowDelegation: although the check in this test is worthwhile, it is not clear why the nameserver should return a DS record when one does not exist. If we do test this, we should really test the following cases:

  • No DS record in either child or parent - nothing returned
  • DS record in parent but not child - DS record returned
  • DS record in child but not in parent - nothing returned
  • Parent and child each have a (different) DS record - DS from parent returned.

... plus of course the case tested in dsNoZone, i.e. DS query received in an inapplicable zone.

Also, what about a test for the case of multiple DS records in the parent zone (as could be present in the case of a KSK rollover.)

0d1406f to efd87c8
It would be helpful to explain how the case of a query for a grandchild zone with the server having authority for the child zone differs from the case of a query for the child zone with the server having authority for the parent.

efd87c8 to efd87c8
dsAtRootZone: as you say, pathological and it doesn't matter in practice how we respond. There is no need to change anything, but it is an interesting case. The rule used for DS records in the code is that a query for a zone's DS record is sent to the parent zone. The behaviour in this test implies that the parent of the root zone is the root zone. On the other hand, if we do any search up the tree, we always terminate the search at the root zone - which could be interpreted as the root zone having no parent. So there appears to be a slight discrepancy in the interpretation of the idea of a root zone parent.

efd87c8 to 53cde03
src/bin/auth/query.h
Header for processDSAtChild() - first line should end "and is called if".

ChangeLog Entry
This is OK.

comment:11 in reply to: ↑ 10 Changed 8 years ago by jinmei

Replying to stephen:

Thanks for the review.

848e9d9 to 00de2fb
query_unittest.cc
A really trivial request: in the list of RRs at the top of the file, some are of the form:

const char* rr = "xxxxx..."

... but most are of are of the form

const char* rr =
    "xxx...."

The contents of the mock zone as a whole would be easier to read if they were all of the second form.

This doesn't seem to be specific to this branch, but I have no problem
with changing the style. Done in b82c785. I suspect the style policy
adopted here will easily be ignored in future updates, however. I
added comments about it, but it will still be easily missed.

32d9c52 to 0d1406f
src/bin/auth/tests/query_unittest.cc
The comment above dsBelowDelegation test: although modified from "This one checks a DS record at the apex is not returned, even if it exists" to "This one checks a DS record at the apex is not returned", the comment still implies that a DS record exists at the apex.

Tried to clarify it in 42b7d6a.

dsBelowDelegation: although the check in this test is worthwhile, it is not clear why the nameserver should return a DS record when one does not exist.

I'm afraid I don't understand this comment...first, do you mean
dsBelowDelegationWithDS, not dsBelowDelegation? Second, whichever
test of dsBelow... you are referring to, I don't understand "why the
nameserver should return a DS record when one does not exist". Or, is
this comment related to the previous version of the code comment for
dsBelowDelegation and has been addressed in 42b7d6a?

If we do test this, we should really test the following cases:

  • No DS record in either child or parent - nothing returned

I agree "No DS record in parent" case should better be tested
explicitly. So I've added it 170556d. I don't think we need to break
it down further to cases where the server has authority for the child
or not. In either case, the server should not even look into the
child zone, and it's confirmed in the dsAboveDelegation tests.

In this sense, the following three are already covered in
dsAboveDelegation and dsAboveDelegationNoData, and I don't think we
need to explicitly reproduce the specific situations of the child
zone. Is that acceptable for you?

  • DS record in parent but not child - DS record returned
  • DS record in child but not in parent - nothing returned
  • Parent and child each have a (different) DS record - DS from parent returned.

Also, what about a test for the case of multiple DS records in the parent zone (as could be present in the case of a KSK rollover.)

I think it's out of scope of query logic tests. As long as the
underlying data source (its find() method, specifically) handles DS as
an RRset this should be ensured. The query logic should be able to
rely on the abstraction of RRset.

0d1406f to efd87c8
It would be helpful to explain how the case of a query for a grandchild zone with the server having authority for the child zone differs from the case of a query for the child zone with the server having authority for the parent.

I don't understand this comment...would it be addressed if I added
comments like this to query.c?

@@ -416,7 +416,9 @@ Query::process() {
             // If a DS query resulted in delegation, we also need to check
             // if we are an authority of the child, too.  If so, we need to
             // complete the process in the child as specified in Section
-            // 2.2.1.2. of RFC3658.
+            // 2.2.1.2. of RFC3658.  Note that this cannot happen if this
+            // server is the parent, in which case find() should handle it
+            // as an in-zone search, whether the result is positive or not.
             if (qtype_ == RRType::DS() && processDSAtChild()) {
                 return;
             }

efd87c8 to efd87c8
dsAtRootZone: as you say, pathological and it doesn't matter in practice how we respond. There is no need to change anything,

So I didn't update the code for this comment.

but it is an interesting case. The rule used for DS records in the code is that a query for a zone's DS record is sent to the parent zone. The behaviour in this test implies that the parent of the root zone is the root zone. On the other hand, if we do any search up the tree, we always terminate the search at the root zone - which could be interpreted as the root zone having no parent. So there appears to be a slight discrepancy in the interpretation of the idea of a root zone parent.

Yes, in this case the root zone could be both considered a "parent" or
"child". In our code it treats as the parent, but in practice the
answer would be the same anyway: "returning a no data response". As
explicitly tested in dsAtRootWithDS, if the root zone really has a DS,
the behavior will be different from the implementation that would
handle it as a child. In any case that shouldn't matter in practice.

efd87c8 to 53cde03
src/bin/auth/query.h
Header for processDSAtChild() - first line should end "and is called if".

Good catch, fixed.

ChangeLog Entry
This is OK.

On second thought, actually, I now think it might be too blunt just
like the infamous BIND 9 changelog entries. Here's a revised
proposed:

374.?	[bug]		jinmei, vorner
	The new query handling module of b10-auth did not handle type DS
	query correctly: It didn't look for it in the parent zone, and
	it incorrectly returned a DS from the child zone if it
	happened to exist there.  Both were corrected, and it now also
	handles the case of having authority for the child and a grand
	ancestor.
	(Trac #1570, git TBD)

comment:12 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

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

  • Owner changed from stephen to jinmei

dsBelowDelegation: although the check in this test is worthwhile, it is not clear why the nameserver should return a DS record when one does not exist.

I'm afraid I don't understand this comment..

Sorry, my fault, misunderstood the contents of that test. Ignore the comment.

In this sense, the following three are already covered in dsAboveDelegation and dsAboveDelegationNoData, and I don't think we need to explicitly reproduce the specific situations of the child zone. Is that acceptable for you?

DS record in parent but not child - DS record returned
DS record in child but not in parent - nothing returned
Parent and child each have a (different) DS record - DS from parent returned.

OK

Also, what about a test for the case of multiple DS records in the parent zone (as could be present in the case of a KSK rollover.)

I think it's out of scope of query logic tests. As long as the underlying data source (its find() method, specifically) handles DS as an RRset this should be ensured. The query logic should be able to rely on the abstraction of RRset.

OK

It would be helpful to explain how the case of a query for a grandchild zone with the server having authority for the child zone differs from the case of a query for the child zone with the server having authority for the parent.

I don't understand this comment...

It was prompted by the comment above the test dsAtGrandParentAndChild, which says:

// DS query for a "grandchild" zone, and the server has authority of the
// child zone, too.  In this case the query should be handled in the child
// side and should result in no data with SOA.

The comment implies that the test is checking that a DS query to the child zone in a parent-child relationship is handled by the parent, a test that is done elsewhere. I think the comment should be rephrased to say that the test is checking that the query is handled by the parent of the zone to which the query is directed, and not by any ancestors of the parent.

Additional test in src/bin/auth/tests/query_unittest.cc is OK.

Updated ChangeLog is OK.

comment:14 in reply to: ↑ 13 Changed 8 years ago by jinmei

Replying to stephen:

It would be helpful to explain how the case of a query for a grandchild zone with the server having authority for the child zone differs from the case of a query for the child zone with the server having authority for the parent.

I don't understand this comment...

It was prompted by the comment above the test dsAtGrandParentAndChild, which says:

// DS query for a "grandchild" zone, and the server has authority of the
// child zone, too.  In this case the query should be handled in the child
// side and should result in no data with SOA.

The comment implies that the test is checking that a DS query to the child zone in a parent-child relationship is handled by the parent, a test that is done elsewhere. I think the comment should be rephrased to say that the test is checking that the query is handled by the parent of the zone to which the query is directed, and not by any ancestors of the parent.

I'm still not sure if I fully understand the concern, but I see the
above cited comment can be confusing (or inaccurate). I revised it
hoping it's now more understandable. How about that?

comment:15 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

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

  • Owner changed from stephen to jinmei

Fine now, please merge.

comment:17 in reply to: ↑ 16 Changed 8 years ago by jinmei

Replying to stephen:

Fine now, please merge.

Thanks, merge done, closing.

comment:18 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 10.80
Note: See TracTickets for help on using tickets.